fs: Additional checks on new and old dir

[PATCH] fs: Additional checks on new and old dir

Posted by Edward Adam Davis 9 months ago

In the reproducer, when calling renameat2(), olddirfd and newdirfd passed
are the same value r0, see [1]. This situation should be avoided.

[1]
renameat2(r0, &(0x7f0000000240)='./bus/file0\x00', r0, &(0x7f00000001c0)='./file0\x00', 0x0)

Reported-by: syzbot+321477fad98ea6dd35b7@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=321477fad98ea6dd35b7
Tested-by: syzbot+321477fad98ea6dd35b7@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 fs/namei.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/namei.c b/fs/namei.c
index 84a0e0b0111c..ff843007ca94 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -5013,7 +5013,7 @@ int vfs_rename(struct renamedata *rd)
 	struct name_snapshot old_name;
 	bool lock_old_subdir, lock_new_subdir;
 
-	if (source == target)
+	if (source == target || old_dir == target)
 		return 0;
 
 	error = may_delete(rd->old_mnt_idmap, old_dir, old_dentry, is_dir);
-- 
2.43.0

Re: [PATCH] fs: Additional checks on new and old dir

Posted by Al Viro 8 months, 3 weeks ago

On Wed, May 14, 2025 at 06:39:40AM +0800, Edward Adam Davis wrote:
> In the reproducer, when calling renameat2(), olddirfd and newdirfd passed
> are the same value r0, see [1]. This situation should be avoided.
> 
> [1]
> renameat2(r0, &(0x7f0000000240)='./bus/file0\x00', r0, &(0x7f00000001c0)='./file0\x00', 0x0)
> 
> Reported-by: syzbot+321477fad98ea6dd35b7@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=321477fad98ea6dd35b7
> Tested-by: syzbot+321477fad98ea6dd35b7@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
>  fs/namei.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 84a0e0b0111c..ff843007ca94 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -5013,7 +5013,7 @@ int vfs_rename(struct renamedata *rd)
>  	struct name_snapshot old_name;
>  	bool lock_old_subdir, lock_new_subdir;
>  
> -	if (source == target)
> +	if (source == target || old_dir == target)
>  		return 0;

What the hell?

1) olddirfd and newdirfd have nothing to do with vfs_rename() - they are
bloody well gone by the time we get there.

2) there's nothing wrong with having the same value passed in both -
and it's certainly not a "quietly do nothing".

3) the check added in this patch is... odd.  You are checking essentically
for rename("foo/bar", "foo").  It should fail (-ENOTEMPTY or -EINVAL, depending
upon RENAME_EXCHANGE in flags) without having reached vfs_rename().

[pox on syzbot - again][exfat] exfat_mkdir() breakage on corrupted image

Posted by Al Viro 8 months, 3 weeks ago

On Fri, May 16, 2025 at 08:31:22PM +0100, Al Viro wrote:
> On Wed, May 14, 2025 at 06:39:40AM +0800, Edward Adam Davis wrote:
> > In the reproducer, when calling renameat2(), olddirfd and newdirfd passed
> > are the same value r0, see [1]. This situation should be avoided.
> > 
> > [1]
> > renameat2(r0, &(0x7f0000000240)='./bus/file0\x00', r0, &(0x7f00000001c0)='./file0\x00', 0x0)
> > 
> > Reported-by: syzbot+321477fad98ea6dd35b7@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=321477fad98ea6dd35b7
> > Tested-by: syzbot+321477fad98ea6dd35b7@syzkaller.appspotmail.com
> > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > ---
> >  fs/namei.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 84a0e0b0111c..ff843007ca94 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -5013,7 +5013,7 @@ int vfs_rename(struct renamedata *rd)
> >  	struct name_snapshot old_name;
> >  	bool lock_old_subdir, lock_new_subdir;
> >  
> > -	if (source == target)
> > +	if (source == target || old_dir == target)
> >  		return 0;
> 
> What the hell?
> 
> 1) olddirfd and newdirfd have nothing to do with vfs_rename() - they are
> bloody well gone by the time we get there.
> 
> 2) there's nothing wrong with having the same value passed in both -
> and it's certainly not a "quietly do nothing".
> 
> 3) the check added in this patch is... odd.  You are checking essentically
> for rename("foo/bar", "foo").  It should fail (-ENOTEMPTY or -EINVAL, depending
> upon RENAME_EXCHANGE in flags) without having reached vfs_rename().

4) it's definitely an exfat bug, since we are getting
	old_dentry->d_parent != target
	old_dentry->d_parent->d_inode == target->d_inode
	S_ISDIR(target->d_inode->i_mode)
All objects involved are on the same super_block, which has "exfat" for
->s_type->name, so it's exfat ending up with multiple dentries for
the same directory inode, and once that kind of thing has happened,
the system is FUBAR.

As for the root cause, almost certainly their ->mkdir() is deciding
that it has just created a new inode - and ending up with existing one,
already in icache and already with a dentry attached to it.

<adds BUG_ON(!hlist_empty(&inode->i_dentry)) into exfat_mkdir()>

   [   84.780875] exFAT-fs (loop0): Volume was not properly unmounted. Some data may be corrupt. Please run fsck.
   [   84.781411] exFAT-fs (loop0): Medium has reported failures. Some data may be lost.
   [   84.782209] exFAT-fs (loop0): failed to load upcase table (idx : 0x00010000, chksum : 0xe62de5da, utbl_chksum : 0xe619d30d)
   [   84.783272] ------------[ cut here ]------------
   [   84.783546] kernel BUG at fs/exfat/namei.c:881!

... and there we go.  exfat_mkdir() getting an existing in-core inode
and attaching an alias to it, with expected fun results.

For crying out loud, how many times do syzbot folks need to be told that
getting report to attention of relevant filesystem folks is important?

Subject: [syzbot] [fs?] INFO: task hung in vfs_rename (2)

mentionings of anything exfat-related: 0.

Cc: brauner@kernel.org, jack@suse.cz, linux-fsdevel@vger.kernel.org,
	 linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com,
		 viro@zeniv.linux.org.uk

mentionings of anything exfat-related: 0.

In message body:
  fsck result: failed (log: https://syzkaller.appspot.com/x/fsck.log?x=12655204580000)

Why, that does sound like some filesystem bug might be involved
and presumably the damn thing knows which type had it been.
<start browser, cut'n'paste the sodding link>
... and the very first line is
fsck.exfat -n exited with status code 4

Result: 3 weeks later it *STILL* hasn't reached the relevant fs
maintainers.  Could that be a sufficient evidence to convince the
fine fellows working on syzbot that "you just need to click a few
links" DOES NOT WORK?

We'd been there several times already.  For relatively polite example,
see https://lore.kernel.org/all/Y5ZDjuSNuSLJd8Mn@ZenIV/ - I can't be arsed
to explain that again and again, and you don't seem to mind following
links in email, so...

Re: [pox on syzbot - again][exfat] exfat_mkdir() breakage on corrupted image

Posted by Aleksandr Nogikh 8 months, 3 weeks ago

Hi Al,

I've only just seen this email as it landed in my Spam folder for some reason.

On Sat, May 17, 2025 at 1:20 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Fri, May 16, 2025 at 08:31:22PM +0100, Al Viro wrote:
> > On Wed, May 14, 2025 at 06:39:40AM +0800, Edward Adam Davis wrote:
> > > In the reproducer, when calling renameat2(), olddirfd and newdirfd passed
> > > are the same value r0, see [1]. This situation should be avoided.
> > >
> > > [1]
> > > renameat2(r0, &(0x7f0000000240)='./bus/file0\x00', r0, &(0x7f00000001c0)='./file0\x00', 0x0)
> > >
> > > Reported-by: syzbot+321477fad98ea6dd35b7@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=321477fad98ea6dd35b7
> > > Tested-by: syzbot+321477fad98ea6dd35b7@syzkaller.appspotmail.com
> > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > ---
> > >  fs/namei.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index 84a0e0b0111c..ff843007ca94 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -5013,7 +5013,7 @@ int vfs_rename(struct renamedata *rd)
> > >     struct name_snapshot old_name;
> > >     bool lock_old_subdir, lock_new_subdir;
> > >
> > > -   if (source == target)
> > > +   if (source == target || old_dir == target)
> > >             return 0;
> >
> > What the hell?
> >
> > 1) olddirfd and newdirfd have nothing to do with vfs_rename() - they are
> > bloody well gone by the time we get there.
> >
> > 2) there's nothing wrong with having the same value passed in both -
> > and it's certainly not a "quietly do nothing".
> >
> > 3) the check added in this patch is... odd.  You are checking essentically
> > for rename("foo/bar", "foo").  It should fail (-ENOTEMPTY or -EINVAL, depending
> > upon RENAME_EXCHANGE in flags) without having reached vfs_rename().
>
> 4) it's definitely an exfat bug, since we are getting
>         old_dentry->d_parent != target
>         old_dentry->d_parent->d_inode == target->d_inode
>         S_ISDIR(target->d_inode->i_mode)
> All objects involved are on the same super_block, which has "exfat" for
> ->s_type->name, so it's exfat ending up with multiple dentries for
> the same directory inode, and once that kind of thing has happened,
> the system is FUBAR.
>
> As for the root cause, almost certainly their ->mkdir() is deciding
> that it has just created a new inode - and ending up with existing one,
> already in icache and already with a dentry attached to it.
>
> <adds BUG_ON(!hlist_empty(&inode->i_dentry)) into exfat_mkdir()>
>
>    [   84.780875] exFAT-fs (loop0): Volume was not properly unmounted. Some data may be corrupt. Please run fsck.
>    [   84.781411] exFAT-fs (loop0): Medium has reported failures. Some data may be lost.
>    [   84.782209] exFAT-fs (loop0): failed to load upcase table (idx : 0x00010000, chksum : 0xe62de5da, utbl_chksum : 0xe619d30d)
>    [   84.783272] ------------[ cut here ]------------
>    [   84.783546] kernel BUG at fs/exfat/namei.c:881!
>
> ... and there we go.  exfat_mkdir() getting an existing in-core inode
> and attaching an alias to it, with expected fun results.
>
> For crying out loud, how many times do syzbot folks need to be told that
> getting report to attention of relevant filesystem folks is important?
>
> Subject: [syzbot] [fs?] INFO: task hung in vfs_rename (2)
>
> mentionings of anything exfat-related: 0.
>
> Cc: brauner@kernel.org, jack@suse.cz, linux-fsdevel@vger.kernel.org,
>          linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com,
>                  viro@zeniv.linux.org.uk
>
> mentionings of anything exfat-related: 0.
>
> In message body:
>   fsck result: failed (log: https://syzkaller.appspot.com/x/fsck.log?x=12655204580000)
>
> Why, that does sound like some filesystem bug might be involved
> and presumably the damn thing knows which type had it been.
> <start browser, cut'n'paste the sodding link>
> ... and the very first line is
> fsck.exfat -n exited with status code 4
>
> Result: 3 weeks later it *STILL* hasn't reached the relevant fs
> maintainers.  Could that be a sufficient evidence to convince the
> fine fellows working on syzbot that "you just need to click a few
> links" DOES NOT WORK?
>
> We'd been there several times already.  For relatively polite example,
> see https://lore.kernel.org/all/Y5ZDjuSNuSLJd8Mn@ZenIV/ - I can't be arsed
> to explain that again and again, and you don't seem to mind following
> links in email, so...
>

I've checked the code, and there was indeed a bug in our
classification rules, specifically concerning the recognition of the
`syz_mount_image$exfat` call as an indicator for the "exfat"
subsystem. The fix will reach syzbot soon.

Sorry for the inconvenience it has caused.

-- 
Aleksandr