Message-ID: <CAKFNMokni2i-qPtCxG7QQjnnRVo105zNZZtvL2qBvSqwszoxsg@mail.gmail.com>
Date: Fri, 15 Nov 2024 08:32:11 +0900
From: Ryusuke Konishi <konishi.ryusuke@...il.com>
To: Edward Adam Davis <eadavis@...com>
Cc: linux-kernel@...r.kernel.org, linux-nilfs@...r.kernel.org, 
	syzbot+96d5d14c47d97015c624@...kaller.appspotmail.com, 
	syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] nilfs2: fix a uaf in nilfs_find_entry

On Thu, Nov 14, 2024 at 9:01 PM Edward Adam Davis wrote:
>
> On Wed, 13 Nov 2024 23:54:39 +0900, Ryusuke Konishi wrote:
> > On Wed, Nov 13, 2024 at 11:28 AM Edward Adam Davis wrote:
> > >
> > > On Tue, 12 Nov 2024 23:38:11 +0900, Ryusuke Konishi wrote:
> > > > On Tue, Nov 12, 2024 at 7:56 PM Edward Adam Davis wrote:
> > > > >
> > > > > The i_size value of the directory "cgroup.controllers" opened by openat is 0,
> > > > > which causes 0 to be returned when calculating the last valid byte in
> > > > > nilfs_last_byte(), which ultimately causes kaddr to move forward by reclen
> > > > > (its value is 32 in this case), which ultimately triggers the uaf when
> > > > > accessing de->rec_len in nilfs_find_entry().
> > > > >
> > > > > To avoid this issue, add a check for i_size in nilfs_lookup().
> > > > >
> > > > > Reported-by: syzbot+96d5d14c47d97015c624@...kaller.appspotmail.com
> > > > > Closes: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624
> > > > > Signed-off-by: Edward Adam Davis <eadavis@...com>
> > > > > ---
> > > > >  fs/nilfs2/namei.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > >
> > > > Hi Edward, thanks for the debugging help and patch suggestion.
> > > >
> > > > But this fix is incorrect.
> > > >
> > > > Reproducers are not creating the situation where i_size == 0.
> > > > In my debug message output inserted in the while loop of
> > > > nilfs_find_entry(), i_size was a corrupted large value like this:
> > > >
> > > > NILFS (loop0): nilfs_find_entry: isize=422212465065984,
> > > > npages=103079215104, n=0, last_byte=0, reclen=32
> > > >
> > > > This is different from your debug result, because the type of i_size
> > > > in the debug patch you sent to syzbot is "%u".
> > > > The type of inode->i_size is "loff_t", which is "long long".
> > > > Therefore, the output format specification for i_size in the debug
> > > > output should be "%lld".
> > > Yes, you are right, I ignore the type of i_size.
> > > >
> > > > If you look at the beginning of nilfs_find_entry(), you can see that
> > > > your check is double-checked:
> > > >
> > > > struct nilfs_dir_entry *nilfs_find_entry(struct inode *dir,
> > > >                 const struct qstr *qstr, struct folio **foliop)
> > > > {
> > > >         ...
> > > >         unsigned long npages = dir_pages(dir);
> > > Yes, now I noticed dir_pages().
> > > >         ..
> > > >
> > > >         if (npages == 0)
> > > >                 goto out;
> > > >         ...
> > > >
> > > > Here, dir_pages() returns 0 if i_size is 0, so it jumps to "out" and
> > > > returns ERR_PTR(-ENOENT).
> > > >
> > > > I'm still debugging, but one problem is that the implementation of
> > > > nilfs_last_byte() is incorrect.
> > > > In the following part, the local variable "last_byte" is not of type
> > > > "loff_t", so depending on the value, it may be truncated and return a
> > > > wrong value (0 in this case):
> > > >
> > > > static unsigned int nilfs_last_byte(struct inode *inode, unsigned long page_nr)
> > > > {
> > > >         unsigned int last_byte = inode->i_size;
> > > >         ...
> > > > }
> > > >
> > > > If this is the only problem, the following fix will be effective. (To
> > > > complete this fix, I think we need to think more carefully about
> > > > whether it's okay for i_size to have any value, especially since
> > > > loff_t is a signed type):
> > > >
> > > > diff --git a/fs/nilfs2/dir.c b/fs/nilfs2/dir.c
> > > > index a8602729586a..6bc8f474a3e5 100644
> > > > --- a/fs/nilfs2/dir.c
> > > > +++ b/fs/nilfs2/dir.c
> > > > @@ -70,7 +70,7 @@ static inline unsigned int nilfs_chunk_size(struct
> > > > inode *inode)
> > > >   */
> > > >  static unsigned int nilfs_last_byte(struct inode *inode, unsigned long page_nr)
> > > >  {
> > > > -       unsigned int last_byte = inode->i_size;
> > > > +       loff_t last_byte = inode->i_size;
> > > >
> > > >         last_byte -= page_nr << PAGE_SHIFT;
> > > >         if (last_byte > PAGE_SIZE)
> > > >
> > > I have noticed nilfs_last_byte(), I have other concerns about it, such
> > > as the chance of last_byte overflowing when i_size is too small and page_nr
> > > is too large, or that it will be negative after being type-adjusted to loff_t.
> > > So, maybe following fix is more rigorous.
> > >
> > > diff --git a/fs/nilfs2/dir.c b/fs/nilfs2/dir.c
> > > index a8602729586a..0dbcf91538fd 100644
> > > --- a/fs/nilfs2/dir.c
> > > +++ b/fs/nilfs2/dir.c
> > > @@ -70,9 +70,10 @@ static inline unsigned int nilfs_chunk_size(struct inode *inode)
> > >   */
> > >  static unsigned int nilfs_last_byte(struct inode *inode, unsigned long page_nr)
> > >  {
> > > -       unsigned int last_byte = inode->i_size;
> > > +       loff_t last_byte = inode->i_size;
> > >
> > > -       last_byte -= page_nr << PAGE_SHIFT;
> > > +       if (last_byte > page_nr << PAGE_SHIFT)
> > > +               last_byte -= page_nr << PAGE_SHIFT;
> > >         if (last_byte > PAGE_SIZE)
> > >                 last_byte = PAGE_SIZE;
> > >         return last_byte;
> > > BR,
> > > Edward
> >
> > nilfs_last_byte itself does not return an error and is a function that
> > assumes that i_size is larger than the offset calculated from page_nr,
> > so let's limit the modification of this function to correcting bit
> > loss in assignment.
> >
> > If any caller is missing the necessary range check, add that check to
> > the caller. I will check again for omissions, but please let me know
> > if there are any callers that seem to have problems (I hope there
> > aren't any).
> Yes, I agree.
> >
> > To extend the bits of last_byte, declare last_byte as "u64" instead of "loff_t".
> > In assignments, the bit pattern is maintained regardless of whether it
> > is signed or not, and declaring it as u64 also avoids the problem of
> > negative i_size here.
> >
> > Comparisons between unsigned and signed integers may introduce
> > warnings in syntax checks at build time such as "make W=2" depending
> > on the environment, and may be reported by bots at a later date, so I
> > would like to maintain comparisons between unsigned integers.
> > (PAGE_SIZE is an unsigned constant)
> >
> > If the problem of negative i_size is actually a problem, I think we
> > should add a sanity check for i_size_read(inode) < 0 to the function
> > that reads inodes from block devices (such as
> > nilfs_read_inode_common).  So, I would like to deal with that
> > separately.
> >
> > I have already tested a change that modifies only the last_byte type
> > to "u64" with syzbot, but if you could proceed with creating a patch
> > that includes the commit log in this direction, I would like to adopt
> > it.
> You are such a nice person.
> If I did that, I personally feel that you would suffer a loss.
> There will be another chance in the future. I look forward to the next time.
>
> BR,
> Edward

Okay, I'll handle this bug fix.
I don't mind either way, but maybe it was a superfluous suggestion. Never mind.
Well then, maybe another time.

Thanks,
Ryusuke Konishi

Powered by blists - more mailing lists