> On Mar 30, 2023, at 2:57 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> Ensure that OPDESC() doesn't return a pointer that doesn't lie within
> the array. In particular, this is a problem when this funtion is passed
> OP_ILLEGAL, but let's return NULL for any invalid value.
>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> fs/nfsd/nfs4proc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> This is the patch that I think we want ahead of this one:
>
> nfsd: call op_release, even when op_func returns an error
>
> If you end up with OP_ILLEGAL, then op->opdesc ends up pointing
> somewhere far, far away, and the new op_release changes can trip over
> that. We could add a Fixes tag for this, I suppose:
>
> 22b03214962e nfsd4: introduce OPDESC helper
>
> ...but that commit is from 2011, so it's probably not worth it.
Well, my concern would be that we want this fix in stable if the
op_release fix is applied as well. I think we will need to either
squash these two or mark this one with an explicit Fixes: tag.
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 5ae670807449..5e7b4ca7a266 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2494,6 +2494,8 @@ static __be32 nfs41_check_op_ordering(struct nfsd4_compoundargs *args)
>
> const struct nfsd4_operation *OPDESC(struct nfsd4_op *op)
> {
> + if (op->opnum < FIRST_NFS4_OP || op->opnum > LAST_NFS42_OP)
> + return NULL;
> return &nfsd4_ops[op->opnum];
> }
Several OPDESC callers appear to expect the return value will be
a non-NULL pointer, so this will either crash the system, or
crash the human reading the code. ;-)
Besides, those callers appear to have already range-checked the
opnum (on cursory inspection). It's only nfsd4_decode_compound()
that looks dodgy.
How about something like this (untested) instead?
NFSD: Don't call OPDESC with a potentially illegal opnum
[ Fill in your description here, or squash this patch ]
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 97edb32be77f..67bbd2d6334c 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2476,10 +2476,12 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
for (i = 0; i < argp->opcnt; i++) {
op = &argp->ops[i];
op->replay = NULL;
+ op->opdesc = NULL;
if (xdr_stream_decode_u32(argp->xdr, &op->opnum) < 0)
return false;
if (nfsd4_opnum_in_range(argp, op)) {
+ op->opdesc = OPDESC(op);
op->status = nfsd4_dec_ops[op->opnum](argp, &op->u);
if (op->status != nfs_ok)
trace_nfsd_compound_decode_err(argp->rqstp,
@@ -2490,7 +2492,7 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
op->opnum = OP_ILLEGAL;
op->status = nfserr_op_illegal;
}
- op->opdesc = OPDESC(op);
+
/*
* We'll try to cache the result in the DRC if any one
* op in the compound wants to be cached:
--
Chuck Lever
