On Thu, 2024-12-05 at 09:13 +0000, Liu, Changxin<changxin.liu@xxxxxxxxxxxxxxxx> wrote: > [You don't often get email from changxin.liu@xxxxxxxxxxxxxxxx. Learn > why this is important at > https://aka.ms/LearnAboutSenderIdentification ;] > > The `nfs4_xdr_dec_open()` function does not properly check the return > status of the `ACCESS` operation. This oversight can result in > out-of-bounds memory access when decoding NFSv4 compound requests. > > For instance, in an NFSv4 compound request `{5, PUTFH, OPEN, GETFH, > ACCESS, GETATTR}`, if the `ACCESS` operation (step 4) returns an > error, > the function proceeds to decode the subsequent `GETATTR` operation > (step 5) without validating the RPC buffer's state. This can cause an > RPC buffer overflow, which leading to a system panic. This issue > can be reliably reproduced by running multiple `fsstress` tests in > the > same directory exported by the Ganesha NFS server. > > This patch introduces proper error handling for the `ACCESS` > operation > in `nfs4_xdr_dec_open()` and `nfs4_xdr_dec_open_noattr()`. When an > error is detected, the decoding process is terminated gracefully to > prevent further buffer corruption and ensure system stability. > > #7 [ffffa42b17337bc0] page_fault at ffffffff906010fe > [exception RIP: xdr_set_page_base+61] > RIP: ffffffffc12166dd RSP: ffffa42b17337c78 RFLAGS: 00010246 > RAX: 0000000000000000 RBX: ffffa42b17337db8 RCX: > 0000000000000000 > RDX: 0000000000000000 RSI: 0000000000000000 RDI: > ffffa42b17337db8 > RBP: 0000000000000000 R8: ffff904948b0a650 R9: > 0000000000000000 > R10: 8080808080808080 R11: ffff904ac3c68be4 R12: > 0000000000000009 > R13: ffffa42b17337db8 R14: ffff904aa6aee000 R15: > ffffffffc11f7f50 > ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018 > #8 [ffffa42b17337c78] xdr_set_next_buffer at ffffffffc1217b0b > [sunrpc] > #9 [ffffa42b17337c90] xdr_inline_decode at ffffffffc1218259 [sunrpc] > #10 [ffffa42b17337cb8] __decode_op_hdr at ffffffffc128d2c2 [nfsv4] > #11 [ffffa42b17337cf0] decode_getfattr_generic.constprop.124 at > ffffffffc12980a2 [nfsv4] > #12 [ffffa42b17337d58] nfs4_xdr_dec_open at ffffffffc1298374 [nfsv4] > #13 [ffffa42b17337db0] call_decode at ffffffffc11f8144 [sunrpc] > #14 [ffffa42b17337e28] __rpc_execute at ffffffffc1206ad5 [sunrpc] > #15 [ffffa42b17337e80] rpc_async_schedule at ffffffffc1206e39 > [sunrpc] > #16 [ffffa42b17337e98] process_one_work at ffffffff8fcfe397 > #17 [ffffa42b17337ed8] worker_thread at ffffffff8fcfea60 > #18 [ffffa42b17337f10] kthread at ffffffff8fd04406 > #19 [ffffa42b17337f50] ret_from_fork at ffffffff9060023f > > Signed-off-by: changxin.liu <changxin.liu@xxxxxxxxxxxxxxxx> > --- > fs/nfs/nfs4xdr.c | 26 ++++++++++++++++++-------- > 1 file changed, 18 insertions(+), 8 deletions(-) > > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > index e8ac3f615f93..819e3fd7487b 100644 > --- a/fs/nfs/nfs4xdr.c > +++ b/fs/nfs/nfs4xdr.c > @@ -6637,11 +6637,16 @@ static int nfs4_xdr_dec_open(struct rpc_rqst > *rqstp, struct xdr_stream *xdr, > status = decode_getfh(xdr, &res->fh); > if (status) > goto out; > - if (res->access_request) > - decode_access(xdr, &res->access_supported, &res- > >access_result); > - decode_getfattr(xdr, res->f_attr, res->server); > + if (res->access_request) { > + status = decode_access(xdr, &res->access_supported, > &res->access_result); > + if (status) > + goto out; > + } > + status = decode_getfattr(xdr, res->f_attr, res->server); > + if (status) > + goto out; > if (res->lg_res) > - decode_layoutget(xdr, rqstp, res->lg_res); > + status = decode_layoutget(xdr, rqstp, res->lg_res); > out: > return status; > } > @@ -6691,11 +6696,16 @@ static int nfs4_xdr_dec_open_noattr(struct > rpc_rqst *rqstp, > status = decode_open(xdr, res); > if (status) > goto out; > - if (res->access_request) > - decode_access(xdr, &res->access_supported, &res- > >access_result); > - decode_getfattr(xdr, res->f_attr, res->server); > + if (res->access_request) { > + status = decode_access(xdr, &res->access_supported, > &res->access_result); > + if (status) > + goto out; > + } > + status = decode_getfattr(xdr, res->f_attr, res->server); > + if (status) > + goto out; Big NACK to all these changes. We do *not* want the success of OPEN to depend on the success of all these sub-operations. That leads to the client and server being unable to agree on the open and locking state of the file. > if (res->lg_res) > - decode_layoutget(xdr, rqstp, res->lg_res); > + status = decode_layoutget(xdr, rqstp, res->lg_res); > out: > return status; > } > -- > 2.27.0 > > -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@xxxxxxxxxxxxxxx
