linux-kernel - Re: [PATCH] ovl: check before dereferencing s

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: <CAOQ4uxgXvwumYvJm3cLDFfx-TsU3g5-yVsTiG=6i8KS48dn0mQ@mail.gmail.com>
Date: Mon, 15 Sep 2025 15:01:13 +0200
From: Amir Goldstein <amir73il@...il.com>
To: Jakub Acs <acsjakub@...zon.de>, Jan Kara <jack@...e.cz>
Cc: linux-unionfs@...r.kernel.org, Miklos Szeredi <miklos@...redi.hu>, 
	linux-kernel@...r.kernel.org, stable@...r.kernel.org, 
	Christian Brauner <brauner@...nel.org>, linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] ovl: check before dereferencing s_root field

On Mon, Sep 15, 2025 at 12:15 PM Jakub Acs <acsjakub@...zon.de> wrote:
>
> Calling intotify_show_fdinfo() on fd watching an overlayfs inode, while
> the overlayfs is being unmounted, can lead to dereferencing NULL ptr.
>
> This issue was found by syzkaller.
>
> Race Condition Diagram:
>
> Thread 1                           Thread 2
> --------                           --------
>
> generic_shutdown_super()
>  shrink_dcache_for_umount
>   sb->s_root = NULL
>
>                     |
>                     |             vfs_read()
>                     |              inotify_fdinfo()
>                     |               * inode get from mark *
>                     |               show_mark_fhandle(m, inode)
>                     |                exportfs_encode_fid(inode, ..)
>                     |                 ovl_encode_fh(inode, ..)
>                     |                  ovl_check_encode_origin(inode)
>                     |                   * deref i_sb->s_root *
>                     |
>                     |
>                     v
>  fsnotify_sb_delete(sb)
>
> Which then leads to:
>
> [   32.133461] Oops: general protection fault, probably for non-canonical address 0xdffffc0000000006: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN NOPTI
> [   32.134438] KASAN: null-ptr-deref in range [0x0000000000000030-0x0000000000000037]
> [   32.135032] CPU: 1 UID: 0 PID: 4468 Comm: systemd-coredum Not tainted 6.17.0-rc6 #22 PREEMPT(none)
>
> <snip registers, unreliable trace>
>
> [   32.143353] Call Trace:
> [   32.143732]  ovl_encode_fh+0xd5/0x170
> [   32.144031]  exportfs_encode_inode_fh+0x12f/0x300
> [   32.144425]  show_mark_fhandle+0xbe/0x1f0
> [   32.145805]  inotify_fdinfo+0x226/0x2d0
> [   32.146442]  inotify_show_fdinfo+0x1c5/0x350
> [   32.147168]  seq_show+0x530/0x6f0
> [   32.147449]  seq_read_iter+0x503/0x12a0
> [   32.148419]  seq_read+0x31f/0x410
> [   32.150714]  vfs_read+0x1f0/0x9e0
> [   32.152297]  ksys_read+0x125/0x240
>
> IOW ovl_check_encode_origin derefs inode->i_sb->s_root, after it was set
> to NULL in the unmount path.
>
> Minimize the window of opportunity by adding explicit check.
>
> Fixes: c45beebfde34 ("ovl: support encoding fid from inode with no alias")
> Signed-off-by: Jakub Acs <acsjakub@...zon.de>
> Cc: Miklos Szeredi <miklos@...redi.hu>
> Cc: Amir Goldstein <amir73il@...il.com>
> Cc: linux-unionfs@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> Cc: stable@...r.kernel.org
> ---
>
> I'm happy to take suggestions for a better fix - I looked at taking
> s_umount for reading, but it wasn't clear to me for how long would the
> fdinfo path need to hold it. Hence the most primitive suggestion in this
> v1.
>
> I'm also not sure if ENOENT or EBUSY is better?.. or even something else?
>
>  fs/overlayfs/export.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 83f80fdb1567..424c73188e06 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -195,6 +195,8 @@ static int ovl_check_encode_origin(struct inode *inode)
>         if (!ovl_inode_lower(inode))
>                 return 0;
>
> +       if (!inode->i_sb->s_root)
> +               return -ENOENT;

For a filesystem method to have to check that its own root is still alive sounds
like the wrong way to me.
That's one of the things that should be taken for granted by fs code.

I don't think this is an overlayfs specific issue, because other fs would be
happy if encode_fh() would be called with NULL sb->s_root.

Jan,

Can we change the order of generic_shutdown_super() so that
fsnotify_sb_delete(sb) is called before setting s_root to NULL?

Or is there a better solution for this race?

Thanks,
Amir.