linux-kernel - Re: [PATCH] nilfs2: fix a uaf in nilfs_find

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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

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".

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);
        ..

        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)

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
>