[PATCH] nfsd: don't allow OPDESC to walk off the end of nfsd4_ops

[PATCH] nfsd: don't allow OPDESC to walk off the end of nfsd4_ops

[Jeff Layton]

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@kernel.org>
---
 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.

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];
 }
 
-- 
2.39.2

Re: [PATCH] nfsd: don't allow OPDESC to walk off the end of nfsd4_ops

[Chuck Lever III]

> On Mar 30, 2023, at 2:57 PM, Jeff Layton <jlayton@kernel.org> 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@kernel.org>
> ---
> 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

Re: [PATCH] nfsd: don't allow OPDESC to walk off the end of nfsd4_ops

[Jeff Layton]

On Thu, 2023-03-30 at 19:32 +0000, Chuck Lever III wrote:
> 
> > On Mar 30, 2023, at 2:57 PM, Jeff Layton <jlayton@kernel.org> 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@kernel.org>
> > ---
> > 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.
> 
> 

Your call.

> > 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. ;-)
> 

Yep, but the alternative is that they go off into la-la land and
probably just crash anyway with a GPF. You might get lucky and not
crash, but it's doubtful that it'd do anything you'd expect. At least by
setting it early to a NULL pointer, you're more likely to crash earlier,
at a point where you might be able to determine the cause.

> 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:
> 
> 

I'm fine with that approach. In fact, that was basically what I had in
an earlier iteration of fixing this.

-- 
Jeff Layton <jlayton@kernel.org>