|
From: Hawkins J. <yin...@gm...> - 2022-08-27 09:02:45
|
On Sat, 27 Aug 2022 at 14:42, Dan Carpenter <dan...@or...> 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...@or...> 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. |
