| Message ID | 20230330185729.22895-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: don't allow OPDESC to walk off the end of nfsd4_ops | expand |
> 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
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.
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]; }
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.