Re: [PATCH] nilfs2: fix a uaf in nilfs_find_entry

[Edward Adam Davis <eadavis@qq.com>]

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@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624
> > Signed-off-by: Edward Adam Davis <eadavis@qq.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
> 
> Regards,
> Ryusuke Konishi
> 
> 
> >
> > diff --git a/fs/nilfs2/namei.c b/fs/nilfs2/namei.c
> > index 9b108052d9f7..0b57bcd9c2c5 100644
> > --- a/fs/nilfs2/namei.c
> > +++ b/fs/nilfs2/namei.c
> > @@ -60,6 +60,9 @@ nilfs_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
> >         if (dentry->d_name.len > NILFS_NAME_LEN)
> >                 return ERR_PTR(-ENAMETOOLONG);
> >
> > +       if (!dir->i_size)
> > +               return ERR_PTR(-EINVAL);
> > +
> >         res = nilfs_inode_by_name(dir, &dentry->d_name, &ino);
> >         if (res) {
> >                 if (res != -ENOENT)
> > --
> > 2.43.0
> >