[PATCH] nfsd: release POSIX ACLs of unexecuted compound ops

[email protected]
Newsgroups org.kernel.vger.linux-nfs
Message-ID <[email protected]>
From: Prabhakar Pujeri <[email protected]>

nfsd4_decode_fattr4() allocates POSIX ACLs while decoding OP_OPEN,
OP_CREATE, and OP_SETATTR. Normally a processed OPEN releases them from
its op_release callback, while CREATE and SETATTR transfer ownership to
struct nfsd_attrs and clear their pointers.

If compound processing stops before a decoded ACL-bearing operation,
nfsd4_release_compoundargs() frees the operation array without dropping
those ACL references. A client can repeat an aborting compound to leak
server memory.

Release ACLs that remain attached to decoded operations when the
compound arguments are destroyed. Clear the OPEN pointers in its release
callback so the final sweep remains idempotent for processed operations;
CREATE and SETATTR already clear theirs when transferring ownership.

The procedure release callback also runs after a hard XDR decode failure.
Keep opcnt limited to fully initialized entries on a truncated operation
number, and clear it when the dynamic operation array cannot be
allocated, so cleanup never walks stale inline entries or beyond the
inline array.

An A/B kmemleak test with 512 crafted NFSv4.2 compounds found 347--349
leaked 64-byte ACL objects without this change and none with it. On the
rebased kernel, 512 runtime-abort and 512 hard-decode compounds completed
without a kmemleak or KASAN report.

Fixes: 5fc51dfc2eb1 ("NFSD: Add support for XDR decoding POSIX draft ACLs")
Signed-off-by: Prabhakar Pujeri <[email protected]>
---
 fs/nfsd/nfs4proc.c |  2 ++
 fs/nfsd/nfs4xdr.c  | 31 ++++++++++++++++++++++++++++++-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 88385a161b4d..3d0404592580 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -800,6 +800,8 @@ nfsd4_open_release(union nfsd4_op_u *u)
 {
 	posix_acl_release(u->open.op_dpacl);
 	posix_acl_release(u->open.op_pacl);
+	u->open.op_dpacl = NULL;
+	u->open.op_pacl = NULL;
 }
 
 /*
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index a154b02d82b3..6be367998a40 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2680,6 +2680,7 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
 		argp->ops = vcalloc(argp->opcnt, sizeof(*argp->ops));
 		if (!argp->ops) {
 			argp->ops = argp->iops;
+			argp->opcnt = 0;
 			return false;
 		}
 	}
@@ -2692,8 +2693,10 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
 		op->replay = NULL;
 		op->opdesc = NULL;
 
-		if (xdr_stream_decode_u32(argp->xdr, &op->opnum) < 0)
+		if (xdr_stream_decode_u32(argp->xdr, &op->opnum) < 0) {
+			argp->opcnt = i;
 			return false;
+		}
 		if (nfsd4_opnum_in_range(argp, op)) {
 			op->opdesc = OPDESC(op);
 			op->status = nfsd4_dec_ops[op->opnum](argp, &op->u);
@@ -6836,6 +6839,32 @@ void nfsd4_encode_replay(struct xdr_stream *xdr, struct nfsd4_op *op)
 void nfsd4_release_compoundargs(struct svc_rqst *rqstp)
 {
 	struct nfsd4_compoundargs *args = rqstp->rq_argp;
+	unsigned int i;
+
+	/*
+	 * Ops that were decoded but never executed (a later op failed to
+	 * decode, or an earlier op failed at runtime) may still hold
+	 * decoded POSIX ACLs; release them here.  Executed ops have
+	 * already released or NULLed their ACL pointers.
+	 */
+	for (i = 0; i < args->opcnt; i++) {
+		struct nfsd4_op *op = &args->ops[i];
+
+		switch (op->opnum) {
+		case OP_OPEN:
+			posix_acl_release(op->u.open.op_dpacl);
+			posix_acl_release(op->u.open.op_pacl);
+			break;
+		case OP_CREATE:
+			posix_acl_release(op->u.create.cr_dpacl);
+			posix_acl_release(op->u.create.cr_pacl);
+			break;
+		case OP_SETATTR:
+			posix_acl_release(op->u.setattr.sa_dpacl);
+			posix_acl_release(op->u.setattr.sa_pacl);
+			break;
+		}
+	}
 
 	args->opcnt = 0;
 	if (args->ops != args->iops) {

base-commit: 01c2994ccb0197cb44b0db89aacab460110f6347
-- 
2.54.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.