> syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
>
> Looks like it is improper check order that causes this bug.
Sorry for wrong command.
#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index 52615e6090e1..6480cd2d371d 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -594,10 +594,11 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
u8 *mrec_end = (u8 *)ctx->mrec +
le32_to_cpu(ctx->mrec->bytes_allocated);
+ if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
+ break;
u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
a->name_length * sizeof(ntfschar);
- if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
- name_end > mrec_end)
+ if (name_end > mrec_end)
break;
ctx->attr = a;
if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||
--
2.25.1On Fri, 26 Aug 2022 20:32:57 +0800 Hawkins Jiawei <yin31149@gmail.com> wrote: > > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > > > > Looks like it is improper check order that causes this bug. um, what bug? > Sorry for wrong command. > #syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master Please prepare a full changelog for the next version? Describe the user-visible runtime effects of the bug, describe what the code does wrong and how the patch repairs it.
On Sat, 27 Aug 2022 at 10:42, Andrew Morton <akpm@linux-foundation.org> wrote: > > On Fri, 26 Aug 2022 20:32:57 +0800 Hawkins Jiawei <yin31149@gmail.com> wrote: > > > > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > > > > > > Looks like it is improper check order that causes this bug. > > um, what bug? > > > Sorry for wrong command. > > #syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > > Please prepare a full changelog for the next version? Describe the > user-visible runtime effects of the bug, describe what the code does > wrong and how the patch repairs it. > I am sorry for that improper email. I send that email just wants to confirm whether my guess is right by syzbot. That is not an official patch email. Thanks for your remind, I will take care next time!
On Fri, Aug 26, 2022 at 08:32:57PM +0800, Hawkins Jiawei wrote:
> > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> >
> > Looks like it is improper check order that causes this bug.
>
> Sorry for wrong command.
> #syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
>
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index 52615e6090e1..6480cd2d371d 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -594,10 +594,11 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
> for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
> u8 *mrec_end = (u8 *)ctx->mrec +
> le32_to_cpu(ctx->mrec->bytes_allocated);
> + if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
> + break;
This definitely seems like a bug. But your code won't build. Syzbot
must have -Werror turned off?
Btw, this was in the original code, but those casts are ugly. Ideally
there would be some way to get rid of them. But otherwise at least
put a space after the u8. "(u8 *)a < (u8 *)ctx->mrec".
> u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
> a->name_length * sizeof(ntfschar);
> - if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
> - name_end > mrec_end)
> + if (name_end > mrec_end)
> break;
regards,
dan carpenter
On Fri, 26 Aug 2022 at 23:15, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Fri, Aug 26, 2022 at 08:32:57PM +0800, Hawkins Jiawei wrote:
> > > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > >
> > > Looks like it is improper check order that causes this bug.
> >
> > Sorry for wrong command.
> > #syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> >
> > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> > index 52615e6090e1..6480cd2d371d 100644
> > --- a/fs/ntfs/attrib.c
> > +++ b/fs/ntfs/attrib.c
> > @@ -594,10 +594,11 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
> > for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
> > u8 *mrec_end = (u8 *)ctx->mrec +
> > le32_to_cpu(ctx->mrec->bytes_allocated);
> > + if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
> > + break;
>
> This definitely seems like a bug. But your code won't build. Syzbot
> must have -Werror turned off?
Hi Dan,
Did you mean we should put the variable declares at the beginning of the function?
(Correct me if I understand anything wrong)
>
> Btw, this was in the original code, but those casts are ugly. Ideally
> there would be some way to get rid of them. But otherwise at least
> put a space after the u8. "(u8 *)a < (u8 *)ctx->mrec".
>
> > u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
> > a->name_length * sizeof(ntfschar);
> > - if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
> > - name_end > mrec_end)
> > + if (name_end > mrec_end)
> > break;
>
> regards,
> dan carpenter
So maybe I can try to refactor these codes. But I wonder if this can be
done in a seperate bug
On Fri, Aug 26, 2022 at 11:42:32PM +0800, Hawkins Jiawei wrote:
> On Fri, 26 Aug 2022 at 23:15, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Fri, Aug 26, 2022 at 08:32:57PM +0800, Hawkins Jiawei wrote:
> > > > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > >
> > > > Looks like it is improper check order that causes this bug.
> > >
> > > Sorry for wrong command.
> > > #syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > >
> > > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> > > index 52615e6090e1..6480cd2d371d 100644
> > > --- a/fs/ntfs/attrib.c
> > > +++ b/fs/ntfs/attrib.c
> > > @@ -594,10 +594,11 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
> > > for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
> > > u8 *mrec_end = (u8 *)ctx->mrec +
> > > le32_to_cpu(ctx->mrec->bytes_allocated);
> > > + if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
> > > + break;
> >
> > This definitely seems like a bug. But your code won't build. Syzbot
> > must have -Werror turned off?
> Hi Dan,
> Did you mean we should put the variable declares at the beginning of the function?
> (Correct me if I understand anything wrong)
You can declare it at the beginning of the block.
>
> >
> > Btw, this was in the original code, but those casts are ugly. Ideally
> > there would be some way to get rid of them. But otherwise at least
> > put a space after the u8. "(u8 *)a < (u8 *)ctx->mrec".
> >
> > > u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
> > > a->name_length * sizeof(ntfschar);
> > > - if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
> > > - name_end > mrec_end)
> > > + if (name_end > mrec_end)
> > > break;
> >
> > regards,
> > dan carpenter
> So maybe I can try to refactor these codes. But I wonder if this can be
> done in a seperate bug
The kernel has a strict "one thing per patch rule". Those rules are
for reviewers and easier backporting. So the trick is to write the
commit message to persuade the reviewer that the way you've written the
patch is the easiest way to review it. So here is how I would write the
commit message:
[PATCH] ntfs: fix out of bounds read in ntfs_attr_find()
This code deferences "a" to calculate "name_end" and then it checks to
ensure that "a" is within bounds. Move the bounds checks earlier and
add some comments to make it more clear what they're doing. Then
calculate "name_end" and check that.
(Btw, are the wrap around checks really sufficient? It seems like it
could wrap to something still within the ->mrec buffer but before the
current entry so it would end up in a forever loop or something?)
diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index 52615e6090e1..90d567acb2a3 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -594,11 +594,20 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
u8 *mrec_end = (u8 *)ctx->mrec +
le32_to_cpu(ctx->mrec->bytes_allocated);
- u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
- a->name_length * sizeof(ntfschar);
- if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
- name_end > mrec_end)
+ u8 *name_end;
+
+ /* check for wrap around */
+ if ((u8 *)a < (u8 *)ctx->mrec)
+ break;
+ /* check for overflow */
+ if ((u8 *)a > mrec_end)
break;
+
+ name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
+ a->name_length * sizeof(ntfschar);
+ if (name_end > mrec_end)
+ break;
+
ctx->attr = a;
if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||
a->type == AT_END))
On Sat, 27 Aug 2022 at 14:42, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Fri, Aug 26, 2022 at 11:42:32PM +0800, Hawkins Jiawei wrote:
> > On Fri, 26 Aug 2022 at 23:15, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Fri, Aug 26, 2022 at 08:32:57PM +0800, Hawkins Jiawei wrote:
> > > > > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > > >
> > > > > Looks like it is improper check order that causes this bug.
> > > >
> > > > Sorry for wrong command.
> > > > #syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > >
> > > > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> > > > index 52615e6090e1..6480cd2d371d 100644
> > > > --- a/fs/ntfs/attrib.c
> > > > +++ b/fs/ntfs/attrib.c
> > > > @@ -594,10 +594,11 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
> > > > for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
> > > > u8 *mrec_end = (u8 *)ctx->mrec +
> > > > le32_to_cpu(ctx->mrec->bytes_allocated);
> > > > + if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
> > > > + break;
> > >
> > > This definitely seems like a bug. But your code won't build. Syzbot
> > > must have -Werror turned off?
> > Hi Dan,
> > Did you mean we should put the variable declares at the beginning of the function?
> > (Correct me if I understand anything wrong)
>
> You can declare it at the beginning of the block.
OK, I will do like that.
>
> >
> > >
> > > Btw, this was in the original code, but those casts are ugly. Ideally
> > > there would be some way to get rid of them. But otherwise at least
> > > put a space after the u8. "(u8 *)a < (u8 *)ctx->mrec".
> > >
> > > > u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
> > > > a->name_length * sizeof(ntfschar);
> > > > - if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
> > > > - name_end > mrec_end)
> > > > + if (name_end > mrec_end)
> > > > break;
> > >
> > > regards,
> > > dan carpenter
> > So maybe I can try to refactor these codes. But I wonder if this can be
> > done in a seperate bug
>
> The kernel has a strict "one thing per patch rule". Those rules are
> for reviewers and easier backporting. So the trick is to write the
> commit message to persuade the reviewer that the way you've written the
> patch is the easiest way to review it. So here is how I would write the
> commit message:
>
> [PATCH] ntfs: fix out of bounds read in ntfs_attr_find()
>
> This code deferences "a" to calculate "name_end" and then it checks to
> ensure that "a" is within bounds. Move the bounds checks earlier and
> add some comments to make it more clear what they're doing. Then
> calculate "name_end" and check that.
>
> (Btw, are the wrap around checks really sufficient? It seems like it
> could wrap to something still within the ->mrec buffer but before the
> current entry so it would end up in a forever loop or something?)
I am not for sure, but it seems that it is OK before.
As for the forever loop, there is a break when a->length is 0 in the loop,
So I think it probably would not end up in a forever loop?(Correct me if
I am wrong)
>
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index 52615e6090e1..90d567acb2a3 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -594,11 +594,20 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
> for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
> u8 *mrec_end = (u8 *)ctx->mrec +
> le32_to_cpu(ctx->mrec->bytes_allocated);
> - u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
> - a->name_length * sizeof(ntfschar);
> - if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
> - name_end > mrec_end)
> + u8 *name_end;
> +
> + /* check for wrap around */
> + if ((u8 *)a < (u8 *)ctx->mrec)
> + break;
> + /* check for overflow */
> + if ((u8 *)a > mrec_end)
> break;
> +
> + name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
> + a->name_length * sizeof(ntfschar);
> + if (name_end > mrec_end)
> + break;
> +
> ctx->attr = a;
> if (unlikely(le32_to_cpu(a->type) > le32_to_cpu(type) ||
> a->type == AT_END))
Thanks for your suggestion, I will refactor these codes in this way.
On Sat, Aug 27, 2022 at 05:02:31PM +0800, Hawkins Jiawei wrote:
> On Sat, 27 Aug 2022 at 14:42, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Fri, Aug 26, 2022 at 11:42:32PM +0800, Hawkins Jiawei wrote:
> > > On Fri, 26 Aug 2022 at 23:15, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > >
> > > > On Fri, Aug 26, 2022 at 08:32:57PM +0800, Hawkins Jiawei wrote:
> > > > > > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > > > >
> > > > > > Looks like it is improper check order that causes this bug.
> > > > >
> > > > > Sorry for wrong command.
> > > > > #syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > > >
> > > > > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> > > > > index 52615e6090e1..6480cd2d371d 100644
> > > > > --- a/fs/ntfs/attrib.c
> > > > > +++ b/fs/ntfs/attrib.c
> > > > > @@ -594,10 +594,11 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
> > > > > for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
> > > > > u8 *mrec_end = (u8 *)ctx->mrec +
> > > > > le32_to_cpu(ctx->mrec->bytes_allocated);
> > > > > + if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
> > > > > + break;
> > > >
> > > > This definitely seems like a bug. But your code won't build. Syzbot
> > > > must have -Werror turned off?
> > > Hi Dan,
> > > Did you mean we should put the variable declares at the beginning of the function?
> > > (Correct me if I understand anything wrong)
> >
> > You can declare it at the beginning of the block.
> OK, I will do like that.
>
> >
> > >
> > > >
> > > > Btw, this was in the original code, but those casts are ugly. Ideally
> > > > there would be some way to get rid of them. But otherwise at least
> > > > put a space after the u8. "(u8 *)a < (u8 *)ctx->mrec".
> > > >
> > > > > u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
> > > > > a->name_length * sizeof(ntfschar);
> > > > > - if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
> > > > > - name_end > mrec_end)
> > > > > + if (name_end > mrec_end)
> > > > > break;
> > > >
> > > > regards,
> > > > dan carpenter
> > > So maybe I can try to refactor these codes. But I wonder if this can be
> > > done in a seperate bug
> >
> > The kernel has a strict "one thing per patch rule". Those rules are
> > for reviewers and easier backporting. So the trick is to write the
> > commit message to persuade the reviewer that the way you've written the
> > patch is the easiest way to review it. So here is how I would write the
> > commit message:
> >
> > [PATCH] ntfs: fix out of bounds read in ntfs_attr_find()
> >
> > This code deferences "a" to calculate "name_end" and then it checks to
> > ensure that "a" is within bounds. Move the bounds checks earlier and
> > add some comments to make it more clear what they're doing. Then
> > calculate "name_end" and check that.
> >
> > (Btw, are the wrap around checks really sufficient? It seems like it
> > could wrap to something still within the ->mrec buffer but before the
> > current entry so it would end up in a forever loop or something?)
> I am not for sure, but it seems that it is OK before.
> As for the forever loop, there is a break when a->length is 0 in the loop,
> So I think it probably would not end up in a forever loop?(Correct me if
> I am wrong)
>
Checking for zero is not sufficient because it can wrap on 32 bit
systems. It needs something like:
return -ENOENT;
len = le32_to_cpu(a->length);
if (!len ||
(void *)a + len < (void *)a ||
(void *)a + len > mrec_end)
break;
if (a->type != type)
continue;
Sort of ugly, but hopefully it gives the idea of what I'm saying.
regards,
dan carpenter
On Sat, 27 Aug 2022 at 18:59, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Sat, Aug 27, 2022 at 05:02:31PM +0800, Hawkins Jiawei wrote:
> > On Sat, 27 Aug 2022 at 14:42, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Fri, Aug 26, 2022 at 11:42:32PM +0800, Hawkins Jiawei wrote:
> > > > On Fri, 26 Aug 2022 at 23:15, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > >
> > > > > On Fri, Aug 26, 2022 at 08:32:57PM +0800, Hawkins Jiawei wrote:
> > > > > > > syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > > > > >
> > > > > > > Looks like it is improper check order that causes this bug.
> > > > > >
> > > > > > Sorry for wrong command.
> > > > > > #syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > > > >
> > > > > > diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> > > > > > index 52615e6090e1..6480cd2d371d 100644
> > > > > > --- a/fs/ntfs/attrib.c
> > > > > > +++ b/fs/ntfs/attrib.c
> > > > > > @@ -594,10 +594,11 @@ static int ntfs_attr_find(const ATTR_TYPE type, const ntfschar *name,
> > > > > > for (;; a = (ATTR_RECORD*)((u8*)a + le32_to_cpu(a->length))) {
> > > > > > u8 *mrec_end = (u8 *)ctx->mrec +
> > > > > > le32_to_cpu(ctx->mrec->bytes_allocated);
> > > > > > + if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end)
> > > > > > + break;
> > > > >
> > > > > This definitely seems like a bug. But your code won't build. Syzbot
> > > > > must have -Werror turned off?
> > > > Hi Dan,
> > > > Did you mean we should put the variable declares at the beginning of the function?
> > > > (Correct me if I understand anything wrong)
> > >
> > > You can declare it at the beginning of the block.
> > OK, I will do like that.
> >
> > >
> > > >
> > > > >
> > > > > Btw, this was in the original code, but those casts are ugly. Ideally
> > > > > there would be some way to get rid of them. But otherwise at least
> > > > > put a space after the u8. "(u8 *)a < (u8 *)ctx->mrec".
> > > > >
> > > > > > u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) +
> > > > > > a->name_length * sizeof(ntfschar);
> > > > > > - if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end ||
> > > > > > - name_end > mrec_end)
> > > > > > + if (name_end > mrec_end)
> > > > > > break;
> > > > >
> > > > > regards,
> > > > > dan carpenter
> > > > So maybe I can try to refactor these codes. But I wonder if this can be
> > > > done in a seperate bug
> > >
> > > The kernel has a strict "one thing per patch rule". Those rules are
> > > for reviewers and easier backporting. So the trick is to write the
> > > commit message to persuade the reviewer that the way you've written the
> > > patch is the easiest way to review it. So here is how I would write the
> > > commit message:
> > >
> > > [PATCH] ntfs: fix out of bounds read in ntfs_attr_find()
> > >
> > > This code deferences "a" to calculate "name_end" and then it checks to
> > > ensure that "a" is within bounds. Move the bounds checks earlier and
> > > add some comments to make it more clear what they're doing. Then
> > > calculate "name_end" and check that.
> > >
> > > (Btw, are the wrap around checks really sufficient? It seems like it
> > > could wrap to something still within the ->mrec buffer but before the
> > > current entry so it would end up in a forever loop or something?)
> > I am not for sure, but it seems that it is OK before.
> > As for the forever loop, there is a break when a->length is 0 in the loop,
> > So I think it probably would not end up in a forever loop?(Correct me if
> > I am wrong)
> >
>
> Checking for zero is not sufficient because it can wrap on 32 bit
> systems. It needs something like:
>
> return -ENOENT;
> len = le32_to_cpu(a->length);
> if (!len ||
> (void *)a + len < (void *)a ||
> (void *)a + len > mrec_end)
> break;
> if (a->type != type)
> continue;
>
> Sort of ugly, but hopefully it gives the idea of what I'm saying.
>
> regards,
> dan carpenter
Hi, Dan
Do you mean there may be an overflow on 32bit systems?
It seems that it is, so it may end up in a forever loop
as you point out before.
I will try to add an overflow checking helper to fix it.
On Fri, 26 Aug 2022 at 23:49, Hawkins Jiawei <yin31149@gmail.com> wrote: > > > > > Btw, this was in the original code, but those casts are ugly. Ideally > > there would be some way to get rid of them. But otherwise at least > > put a space after the u8. "(u8 *)a < (u8 *)ctx->mrec". > > > > > u8 *name_end = (u8 *)a + le16_to_cpu(a->name_offset) + > > > a->name_length * sizeof(ntfschar); > > > - if ((u8*)a < (u8*)ctx->mrec || (u8*)a > mrec_end || > > > - name_end > mrec_end) > > > + if (name_end > mrec_end) > > > break; > > > > regards, > > dan carpenter > So maybe I can try to refactor these codes. But I wonder if this can be > done in a seperate bug Sorry for the typing error. I mean refactoring these code in another seperate patch seems better.
Hello, syzbot has tested the proposed patch and the reproducer did not trigger any issue: Reported-and-tested-by: syzbot+5f8dcabe4a3b2c51c607@syzkaller.appspotmail.com Tested on: commit: 4c612826 Merge tag 'net-6.0-rc3' of git://git.kernel.o.. git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master console output: https://syzkaller.appspot.com/x/log.txt?x=115a8b13080000 kernel config: https://syzkaller.appspot.com/x/.config?x=911efaff115942bb dashboard link: https://syzkaller.appspot.com/bug?extid=5f8dcabe4a3b2c51c607 compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2 patch: https://syzkaller.appspot.com/x/patch.diff?x=1229de4d080000 Note: testing is done by a robot and is best-effort only.
© 2016 - 2026 Red Hat, Inc.