linux-kernel - Re: [PATCH] nfsd: make a copy of struct iattr before calling notify

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 for Android: free password hash cracker in your pocket

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

Message-ID: <a55a7bfc8be6210dfc7e7721825ac915795a48cf.camel@kernel.org>
Date:   Wed, 17 May 2023 15:05:45 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     Chuck Lever III <chuck.lever@...cle.com>
Cc:     Zhi Li <yieli@...hat.com>,
        Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] nfsd: make a copy of struct iattr before calling
 notify_change

On Wed, 2023-05-17 at 17:47 +0000, Chuck Lever III wrote:
> 
> > On May 17, 2023, at 12:26 PM, Jeff Layton <jlayton@...nel.org> wrote:
> > 
> > notify_change can modify the iattr structure. In particular it can can
> > end up setting ATTR_MODE when ATTR_KILL_SUID is already set, causing a
> > BUG() if the same iattr is passed to notify_change more than once.
> > 
> > Make a copy of the struct iattr before calling notify_change.
> > 
> > Fixes: 34b91dda7124 NFSD: Make nfsd4_setattr() wait before returning NFS4ERR_DELAY
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2207969
> > Reported-by: Zhi Li <yieli@...hat.com>
> > Signed-off-by: Jeff Layton <jlayton@...nel.org>
> > ---
> > fs/nfsd/vfs.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index c4ef24c5ffd0..ad0c5cd900b1 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -538,7 +538,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > 
> > inode_lock(inode);
> > for (retries = 1;;) {
> > - host_err = __nfsd_setattr(dentry, iap);
> > + struct iattr attrs = *iap;
> 
> This construct always makes me queazy. I'm never sure if an
> initializer inside a loop is "only once" or "every time". I
> fixed a bug like this once.
> 
> But if you've tested it and it addresses the BUG, then let's
> go with this. I can apply it to nfsd-fixes.
> 


I've done some light testing with this kernel, but this was found by Zhi
while testing with the lustre racer test, so it involves some raciness.
I've never hit this myself.

I'm pretty sure though that this has to be initialized every time. The
assignment is inside the loop, after all. I'm ok with moving the
assignment to a different line if you like though:

	struct iattr attrs;

	attrs = *iap;
	...

> > +
> > + host_err = __nfsd_setattr(dentry, &attrs);
> > if (host_err != -EAGAIN || !retries--)
> > break;
> > if (!nfsd_wait_for_delegreturn(rqstp, inode))
> > -- 
> > 2.40.1
> > 
> 
> --
> Chuck Lever
> 
> 

-- 
Jeff Layton <jlayton@...nel.org>