linux-kernel - Re: [PATCH v2 0/7] nfsd: CB_SEQUENCE error handling fixes and cleanups

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: <5760673a9adc597c1b235dd6a1670ed801d2feb1.camel@kernel.org>
Date: Wed, 29 Jan 2025 09:27:02 -0500
From: Jeff Layton <jlayton@...nel.org>
To: "J. Bruce Fields" <bfields@...ldses.org>
Cc: Chuck Lever <chuck.lever@...cle.com>, Neil Brown <neilb@...e.de>, Olga
 Kornievskaia <okorniev@...hat.com>, Dai Ngo <Dai.Ngo@...cle.com>, Tom
 Talpey <tom@...pey.com>, Kinglong Mee	 <kinglongmee@...il.com>, Trond
 Myklebust <trondmy@...nel.org>, Anna Schumaker	 <anna@...nel.org>,
 linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 0/7] nfsd: CB_SEQUENCE error handling fixes and
 cleanups

On Wed, 2025-01-29 at 09:21 -0500, J. Bruce Fields wrote:
> On Wed, Jan 29, 2025 at 08:39:53AM -0500, Jeff Layton wrote:
> > While looking over the CB_SEQUENCE error handling, I discovered that
> > callbacks don't hold a reference to a session, and the
> > clp->cl_cb_session could easily change between request and response.
> > If that happens at an inopportune time, there could be UAFs or weird
> > slot/sequence handling problems.
> 
> Nobody should place too much faith in my understanding of how any of
> this works at this point, but....  My vague memory is that a lot of
> things are serialized simply by being run only on the cl_callback_wq.
> Modifying clp->cl_cb_session is such a thing.
> 
> 

It is, but that doesn't save us here. The workqueue is just there to
submit jobs to the RPC client. Once that happens they are run via
rpciod's workqueue (and in parallel with one another since they're
async RPC calls).

So, it's possible that while we're waiting for a response from one
callback, another is submitted, and that workqueue job changes the
clp->cl_cb_session.

Thanks for taking a look, Bruce!

> 
> > This series changes the nfsd4_session to be RCU-freed, and then adds a
> > new method of session refcounting that is compatible with the old.
> > nfsd4_callback RPCs will now hold a lightweight reference to the session
> > in addition to the slot. Then, all of the callback handling is switched
> > to use that session instead of dereferencing clp->cb_cb_session.
> > I've also reworked the error handling in nfsd4_cb_sequence_done()
> > based on review comments, and lifted the v4.0 handing out of that
> > function.
> > 
> > This passes pynfs, nfstests, and fstests for me, but I'm not sure how
> > much any of that stresses the backchannel's error handling.
> > 
> > These should probably go in via Chuck's tree, but the last patch touches
> > some NFS cnd sunrpc client code, so it'd be good to have R-b's or A-b's
> > from Trond and/or Anna on that one.
> > 
> > Signed-off-by: Jeff Layton <jlayton@...nel.org>
> > ---
> > Changes in v2:
> > - make nfsd4_session be RCU-freed
> > - change code to keep reference to session over callback RPCs
> > - rework error handling in nfsd4_cb_sequence_done()
> > - move NFSv4.0 handling out of nfsd4_cb_sequence_done()
> > - Link to v1: https://lore.kernel.org/r/20250123-nfsd-6-14-v1-0-c1137a4fa2ae@kernel.org
> > 
> > ---
> > Jeff Layton (7):
> >       nfsd: add routines to get/put session references for callbacks
> >       nfsd: make clp->cl_cb_session be an RCU managed pointer
> >       nfsd: add a cb_ses pointer to nfsd4_callback and use it instead of clp->cb_cb_session
> >       nfsd: overhaul CB_SEQUENCE error handling
> >       nfsd: remove unneeded forward declaration of nfsd4_mark_cb_fault()
> >       nfsd: lift NFSv4.0 handling out of nfsd4_cb_sequence_done()
> >       sunrpc: make rpc_restart_call() and rpc_restart_call_prepare() void return
> > 
> >  fs/nfs/nfs4proc.c           |  12 ++-
> >  fs/nfsd/nfs4callback.c      | 212 ++++++++++++++++++++++++++++++++------------
> >  fs/nfsd/nfs4state.c         |  43 ++++++++-
> >  fs/nfsd/state.h             |   6 +-
> >  fs/nfsd/trace.h             |   6 +-
> >  include/linux/sunrpc/clnt.h |   4 +-
> >  net/sunrpc/clnt.c           |   7 +-
> >  7 files changed, 210 insertions(+), 80 deletions(-)
> > ---
> > base-commit: a05af3c6103b703d1d38d8180b3ebbe0a03c2f07
> > change-id: 20250123-nfsd-6-14-b0797e385dc0
> > 
> > Best regards,
> > -- 
> > Jeff Layton <jlayton@...nel.org>

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