linux-kernel - Re: Patch for a overwriting/corruption of the file system

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]

Date:   Tue, 14 Mar 2023 16:57:08 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Christian Brauner <brauner@...nel.org>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Kirtikumar Anandrao Ramchandani <kirtiar15502@...il.com>,
        security@...nel.org, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org,
        Linus Torvalds <torvalds@...uxfoundation.org>
Subject: Re: Patch for a overwriting/corruption of the file system

On Tue, Mar 14, 2023 at 10:55:39AM +0100, Christian Brauner wrote:
> On Mon, Mar 13, 2023 at 11:32:08AM +0100, Greg KH wrote:
> > On Mon, Mar 13, 2023 at 03:54:55PM +0530, Kirtikumar Anandrao Ramchandani wrote:
> > > Seems like again it got rejected. I am sending it in the body if it works:
> > > 
> > > >From 839cae91705e044b49397590f2d85a5dd289f0c5 Mon Sep 17 00:00:00 2001
> > > From: KirtiRamchandani <kirtar15502@...il.com>
> > > Date: Mon, 13 Mar 2023 15:05:08 +0530
> > > Subject: [PATCH] Fix bug in affs_rename() function. The `affs_rename()`
> > >  function in the AFFS filesystem has a bug that can cause the `retval`
> > >  variable to be overwritten before it is used. Specifically, the function
> > >  assigns `retval` a value in two separate code blocks, but then only checks
> > >  its value in one of those blocks. This commit fixes the bug by ensuring
> > > that
> > >  `retval` is properly checked in both code blocks.
> > > 
> > > Signed-off-by: KirtiRamchandani <kirtar15502@...il.com>
> > > ---
> > >  namei.c | 4++++--
> > >  1 file changed, 4 insertions(+), 2 deletion(-)
> > > 
> > > diff --git a/fs/affs/namei.c b/fs/affs/namei.c
> > > index d1084e5..a54c700 100644
> > > --- a/fs/affs/namei.c
> > > +++ b/fs/affs/namei.c
> > > @@ -488,7 +488,8 @@ affs_xrename(struct inode *old_dir, struct dentry
> > > *old_dentry,
> > >         affs_lock_dir(new_dir);
> > >         retval = affs_insert_hash(new_dir, bh_old);
> > >         affs_unlock_dir(new_dir);
> > > -
> > > +       if (retval)
> > > +               goto done;
> > 
> > The patch is corrupted and can not be applied.
> 
> Yeah, that patch is pretty borked. This should probably be sm like:
> 
> >From f3a7758bb53cc776820656c6ac66b13fb8ed9022 Mon Sep 17 00:00:00 2001
> From: KirtiRamchandani <kirtar15502@...il.com>
> Date: Tue, 14 Mar 2023 10:49:38 +0100
> Subject: [PATCH] affs: handle errors in affs_xrename()
> 
> Fix a bug in the affs_xrename() function. The affs_xrename() function in
> the AFFS filesystem has a bug that can cause the retval variable to be
> overwritten before it is used. Specifically, the function assigns retval
> a value in two separate code blocks, but then only checks its value in
> one of those blocks. This commit fixes the bug by ensuring that retval
> is properly checked in both code blocks.

"Properly checked" as in...?

> Signed-off-by: KirtiRamchandani <kirtar15502@...il.com>
> ---
>  fs/affs/namei.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/affs/namei.c b/fs/affs/namei.c
> index d12ccfd2a83d..98525d69391d 100644
> --- a/fs/affs/namei.c
> +++ b/fs/affs/namei.c
> @@ -488,6 +488,8 @@ affs_xrename(struct inode *old_dir, struct dentry *old_dentry,
>  	affs_lock_dir(new_dir);
>  	retval = affs_insert_hash(new_dir, bh_old);
>  	affs_unlock_dir(new_dir);
> +	if (retval)
> +		goto done;

OK, so you've got an IO error and insertion has failed.  Both entries had already
been removed from their directories.  Sure, we must report an error, but why is
leaking *both* entries the right thing to do?

>  	/* Insert new into the old directory with the old name. */
>  	affs_copy_name(AFFS_TAIL(sb, bh_new)->name, old_dentry);
> @@ -495,6 +497,8 @@ affs_xrename(struct inode *old_dir, struct dentry *old_dentry,
>  	affs_lock_dir(old_dir);
>  	retval = affs_insert_hash(old_dir, bh_new);
>  	affs_unlock_dir(old_dir);
> +	if (retval)
> +		goto done;
>  done:

Really?  How could that possibly make any sense?  I mean, look for the target of
that goto...

The bug here (AFFS awful layout aside) is that error from the first insert_hash
is always lost.  And it needs to be reported.  But this is no way to fix that.