Hello Olga Kornievskaia,
This is a semi-automatic email about new static checker warnings.
The patch 4e48f1cccab3: "NFSD: allow inter server COPY to have a
STALE source server fh" from Oct 7, 2019, leads to the following
Smatch complaint:
fs/nfsd/nfs4proc.c:2371 nfsd4_proc_compound()
error: we previously assumed 'current_fh->fh_export' could be null (see line 2325)
fs/nfsd/nfs4proc.c
2324 }
2325 } else if (current_fh->fh_export &&
^^^^^^^^^^^^^^^^^^^^^
The patch adds a check for NULL
2326 current_fh->fh_export->ex_fslocs.migrated &&
2327 !(op->opdesc->op_flags & ALLOWED_ON_ABSENT_FS)) {
2328 op->status = nfserr_moved;
2329 goto encode_op;
2330 }
2331
2332 fh_clear_wcc(current_fh);
2333
2334 /* If op is non-idempotent */
2335 if (op->opdesc->op_flags & OP_MODIFIES_SOMETHING) {
2336 /*
2337 * Don't execute this op if we couldn't encode a
2338 * succesful reply:
2339 */
2340 u32 plen = op->opdesc->op_rsize_bop(rqstp, op);
2341 /*
2342 * Plus if there's another operation, make sure
2343 * we'll have space to at least encode an error:
2344 */
2345 if (resp->opcnt < args->opcnt)
2346 plen += COMPOUND_ERR_SLACK_SPACE;
2347 op->status = nfsd4_check_resp_size(resp, plen);
2348 }
2349
2350 if (op->status)
2351 goto encode_op;
2352
2353 if (op->opdesc->op_get_currentstateid)
2354 op->opdesc->op_get_currentstateid(cstate, &op->u);
2355 op->status = op->opdesc->op_func(rqstp, cstate, &op->u);
2356
2357 /* Only from SEQUENCE */
2358 if (cstate->status == nfserr_replay_cache) {
2359 dprintk("%s NFS4.1 replay from cache\n", __func__);
2360 status = op->status;
2361 goto out;
2362 }
2363 if (!op->status) {
2364 if (op->opdesc->op_set_currentstateid)
2365 op->opdesc->op_set_currentstateid(cstate, &op->u);
2366
2367 if (op->opdesc->op_flags & OP_CLEAR_STATEID)
2368 clear_current_stateid(cstate);
2369
2370 if (need_wrongsec_check(rqstp))
2371 op->status = check_nfsd_access(current_fh->fh_export, rqstp);
^^^^^^^^^^^^^^^^^^^^^
Is it required here as well?
2372 }
2373 encode_op:
regards,
dan carpenter
On Wed, Dec 4, 2019 at 3:00 AM Dan Carpenter <[email protected]> wrote:
>
> Hello Olga Kornievskaia,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 4e48f1cccab3: "NFSD: allow inter server COPY to have a
> STALE source server fh" from Oct 7, 2019, leads to the following
> Smatch complaint:
>
> fs/nfsd/nfs4proc.c:2371 nfsd4_proc_compound()
> error: we previously assumed 'current_fh->fh_export' could be null (see line 2325)
>
> fs/nfsd/nfs4proc.c
> 2324 }
> 2325 } else if (current_fh->fh_export &&
> ^^^^^^^^^^^^^^^^^^^^^
> The patch adds a check for NULL
>
> 2326 current_fh->fh_export->ex_fslocs.migrated &&
> 2327 !(op->opdesc->op_flags & ALLOWED_ON_ABSENT_FS)) {
> 2328 op->status = nfserr_moved;
> 2329 goto encode_op;
> 2330 }
> 2331
> 2332 fh_clear_wcc(current_fh);
> 2333
> 2334 /* If op is non-idempotent */
> 2335 if (op->opdesc->op_flags & OP_MODIFIES_SOMETHING) {
> 2336 /*
> 2337 * Don't execute this op if we couldn't encode a
> 2338 * succesful reply:
> 2339 */
> 2340 u32 plen = op->opdesc->op_rsize_bop(rqstp, op);
> 2341 /*
> 2342 * Plus if there's another operation, make sure
> 2343 * we'll have space to at least encode an error:
> 2344 */
> 2345 if (resp->opcnt < args->opcnt)
> 2346 plen += COMPOUND_ERR_SLACK_SPACE;
> 2347 op->status = nfsd4_check_resp_size(resp, plen);
> 2348 }
> 2349
> 2350 if (op->status)
> 2351 goto encode_op;
> 2352
> 2353 if (op->opdesc->op_get_currentstateid)
> 2354 op->opdesc->op_get_currentstateid(cstate, &op->u);
> 2355 op->status = op->opdesc->op_func(rqstp, cstate, &op->u);
> 2356
> 2357 /* Only from SEQUENCE */
> 2358 if (cstate->status == nfserr_replay_cache) {
> 2359 dprintk("%s NFS4.1 replay from cache\n", __func__);
> 2360 status = op->status;
> 2361 goto out;
> 2362 }
> 2363 if (!op->status) {
> 2364 if (op->opdesc->op_set_currentstateid)
> 2365 op->opdesc->op_set_currentstateid(cstate, &op->u);
> 2366
> 2367 if (op->opdesc->op_flags & OP_CLEAR_STATEID)
> 2368 clear_current_stateid(cstate);
> 2369
> 2370 if (need_wrongsec_check(rqstp))
> 2371 op->status = check_nfsd_access(current_fh->fh_export, rqstp);
> ^^^^^^^^^^^^^^^^^^^^^
> Is it required here as well?
Bruce, correct me if I'm wrong but I think we are ok here. Because for
the COPY operation for which the current_fh->fh_export can be null,
need_wrongsec_check() would be false.
>
> 2372 }
> 2373 encode_op:
>
> regards,
> dan carpenter
On Wed, Dec 04, 2019 at 03:11:01PM -0500, Olga Kornievskaia wrote:
> On Wed, Dec 4, 2019 at 3:00 AM Dan Carpenter <[email protected]> wrote:
> >
> > Hello Olga Kornievskaia,
> >
> > This is a semi-automatic email about new static checker warnings.
> >
> > The patch 4e48f1cccab3: "NFSD: allow inter server COPY to have a
> > STALE source server fh" from Oct 7, 2019, leads to the following
> > Smatch complaint:
> >
> > fs/nfsd/nfs4proc.c:2371 nfsd4_proc_compound()
> > error: we previously assumed 'current_fh->fh_export' could be null (see line 2325)
> >
> > fs/nfsd/nfs4proc.c
> > 2324 }
> > 2325 } else if (current_fh->fh_export &&
> > ^^^^^^^^^^^^^^^^^^^^^
> > The patch adds a check for NULL
> >
> > 2326 current_fh->fh_export->ex_fslocs.migrated &&
> > 2327 !(op->opdesc->op_flags & ALLOWED_ON_ABSENT_FS)) {
> > 2328 op->status = nfserr_moved;
> > 2329 goto encode_op;
> > 2330 }
> > 2331
> > 2332 fh_clear_wcc(current_fh);
> > 2333
> > 2334 /* If op is non-idempotent */
> > 2335 if (op->opdesc->op_flags & OP_MODIFIES_SOMETHING) {
> > 2336 /*
> > 2337 * Don't execute this op if we couldn't encode a
> > 2338 * succesful reply:
> > 2339 */
> > 2340 u32 plen = op->opdesc->op_rsize_bop(rqstp, op);
> > 2341 /*
> > 2342 * Plus if there's another operation, make sure
> > 2343 * we'll have space to at least encode an error:
> > 2344 */
> > 2345 if (resp->opcnt < args->opcnt)
> > 2346 plen += COMPOUND_ERR_SLACK_SPACE;
> > 2347 op->status = nfsd4_check_resp_size(resp, plen);
> > 2348 }
> > 2349
> > 2350 if (op->status)
> > 2351 goto encode_op;
> > 2352
> > 2353 if (op->opdesc->op_get_currentstateid)
> > 2354 op->opdesc->op_get_currentstateid(cstate, &op->u);
> > 2355 op->status = op->opdesc->op_func(rqstp, cstate, &op->u);
> > 2356
> > 2357 /* Only from SEQUENCE */
> > 2358 if (cstate->status == nfserr_replay_cache) {
> > 2359 dprintk("%s NFS4.1 replay from cache\n", __func__);
> > 2360 status = op->status;
> > 2361 goto out;
> > 2362 }
> > 2363 if (!op->status) {
> > 2364 if (op->opdesc->op_set_currentstateid)
> > 2365 op->opdesc->op_set_currentstateid(cstate, &op->u);
> > 2366
> > 2367 if (op->opdesc->op_flags & OP_CLEAR_STATEID)
> > 2368 clear_current_stateid(cstate);
> > 2369
> > 2370 if (need_wrongsec_check(rqstp))
> > 2371 op->status = check_nfsd_access(current_fh->fh_export, rqstp);
> > ^^^^^^^^^^^^^^^^^^^^^
> > Is it required here as well?
>
> Bruce, correct me if I'm wrong but I think we are ok here. Because for
> the COPY operation for which the current_fh->fh_export can be null,
> need_wrongsec_check() would be false.
Honestly.... I've spent a few minutes thinking about it, but haven't
been able to come up either with an example where this will attempt a
NULL dereference, or a convincing argument that it never will.
I'll think about it some more and I'll figure it out. But I worry that
the the logic is fragile.
One other thing I noticed: in the no_verify case, we're depending on
fh_verify returning a stale error on a foreign filehandle. But I don't
think we can count on it. It might, by coincidence, turn out that
fh_verify returns some other error, and then a legitimate COPY could
fail for no reason.
--b.
On Wed, Dec 04, 2019 at 05:04:35PM -0500, J. Bruce Fields wrote:
> On Wed, Dec 04, 2019 at 03:11:01PM -0500, Olga Kornievskaia wrote:
> > On Wed, Dec 4, 2019 at 3:00 AM Dan Carpenter <[email protected]> wrote:
> > >
> > > Hello Olga Kornievskaia,
> > >
> > > This is a semi-automatic email about new static checker warnings.
> > >
> > > The patch 4e48f1cccab3: "NFSD: allow inter server COPY to have a
> > > STALE source server fh" from Oct 7, 2019, leads to the following
> > > Smatch complaint:
> > >
> > > fs/nfsd/nfs4proc.c:2371 nfsd4_proc_compound()
> > > error: we previously assumed 'current_fh->fh_export' could be null (see line 2325)
> > >
> > > fs/nfsd/nfs4proc.c
> > > 2324 }
> > > 2325 } else if (current_fh->fh_export &&
> > > ^^^^^^^^^^^^^^^^^^^^^
> > > The patch adds a check for NULL
> > >
> > > 2326 current_fh->fh_export->ex_fslocs.migrated &&
> > > 2327 !(op->opdesc->op_flags & ALLOWED_ON_ABSENT_FS)) {
> > > 2328 op->status = nfserr_moved;
> > > 2329 goto encode_op;
> > > 2330 }
> > > 2331
> > > 2332 fh_clear_wcc(current_fh);
> > > 2333
> > > 2334 /* If op is non-idempotent */
> > > 2335 if (op->opdesc->op_flags & OP_MODIFIES_SOMETHING) {
> > > 2336 /*
> > > 2337 * Don't execute this op if we couldn't encode a
> > > 2338 * succesful reply:
> > > 2339 */
> > > 2340 u32 plen = op->opdesc->op_rsize_bop(rqstp, op);
> > > 2341 /*
> > > 2342 * Plus if there's another operation, make sure
> > > 2343 * we'll have space to at least encode an error:
> > > 2344 */
> > > 2345 if (resp->opcnt < args->opcnt)
> > > 2346 plen += COMPOUND_ERR_SLACK_SPACE;
> > > 2347 op->status = nfsd4_check_resp_size(resp, plen);
> > > 2348 }
> > > 2349
> > > 2350 if (op->status)
> > > 2351 goto encode_op;
> > > 2352
> > > 2353 if (op->opdesc->op_get_currentstateid)
> > > 2354 op->opdesc->op_get_currentstateid(cstate, &op->u);
> > > 2355 op->status = op->opdesc->op_func(rqstp, cstate, &op->u);
> > > 2356
> > > 2357 /* Only from SEQUENCE */
> > > 2358 if (cstate->status == nfserr_replay_cache) {
> > > 2359 dprintk("%s NFS4.1 replay from cache\n", __func__);
> > > 2360 status = op->status;
> > > 2361 goto out;
> > > 2362 }
> > > 2363 if (!op->status) {
> > > 2364 if (op->opdesc->op_set_currentstateid)
> > > 2365 op->opdesc->op_set_currentstateid(cstate, &op->u);
> > > 2366
> > > 2367 if (op->opdesc->op_flags & OP_CLEAR_STATEID)
> > > 2368 clear_current_stateid(cstate);
> > > 2369
> > > 2370 if (need_wrongsec_check(rqstp))
> > > 2371 op->status = check_nfsd_access(current_fh->fh_export, rqstp);
> > > ^^^^^^^^^^^^^^^^^^^^^
> > > Is it required here as well?
> >
> > Bruce, correct me if I'm wrong but I think we are ok here. Because for
> > the COPY operation for which the current_fh->fh_export can be null,
> > need_wrongsec_check() would be false.
>
> Honestly.... I've spent a few minutes thinking about it, but haven't
> been able to come up either with an example where this will attempt a
> NULL dereference, or a convincing argument that it never will.
>
> I'll think about it some more and I'll figure it out. But I worry that
> the the logic is fragile.
Seems like a compound like this should trigger a NULL dereference:
PUTFH(foreign filehandle)
GETATTR
SAVEFH
COPY
First, check_if_stalefh_allowed sets no_verify on the first (PUTFH) op.
Then op_func = nfsd4_putfh runs and leaves current_fh->fh_export NULL.
need_wrongsec_check returns true, since this PUTFH has OP_IS_PUTFH_LIKE
set and GETATTR does not have OP_HANDLES_WRONGSEC set.
Haven't actually tested that, maybe I'm missing something.
So, stuff we could do:
- add an extra check of fh_export or something here.
- make check_if_stalefh_allowed more careful--it'd be easy for
it to spot that a compound like the above is going to fail.
- double-check that we don't assume the filehandle is verified
elsewhere in nfsd_compound.
- ?
> One other thing I noticed: in the no_verify case, we're depending on
> fh_verify returning a stale error on a foreign filehandle. But I don't
> think we can count on it. It might, by coincidence, turn out that
> fh_verify returns some other error, and then a legitimate COPY could
> fail for no reason.
Also nervous about cases like the above where we'll be passing the
foreign filehandle into GETATTR and counting on fh_verify failing in a
useful way. I mean, maybe the client gets what it deserves for sending
an obviously nonsensical compound, but still it'd probably be better to
catch that earlier.
--b.
On Wed, Dec 04, 2019 at 09:38:26PM -0500, J. Bruce Fields wrote:
> So, stuff we could do:
>
> - add an extra check of fh_export or something here.
So, I'm applying the following for now.
--b.
commit a0a906b965b0
Author: J. Bruce Fields <[email protected]>
Date: Fri Dec 6 16:07:32 2019 -0500
nfsd4: avoid NULL deference on strange COPY compounds
With cross-server COPY we've introduced the possibility that the current
or saved filehandle might not have fh_dentry/fh_export filled in, but we
missed a place that assumed it was. I think this could be triggered by
a compound like:
PUTFH(foreign filehandle)
GETATTR
SAVEFH
COPY
First, check_if_stalefh_allowed sets no_verify on the first (PUTFH) op.
Then op_func = nfsd4_putfh runs and leaves current_fh->fh_export NULL.
need_wrongsec_check returns true, since this PUTFH has OP_IS_PUTFH_LIKE
set and GETATTR does not have OP_HANDLES_WRONGSEC set.
We should probably also consider tightening the checks in
check_if_stalefh_allowed and double-checking that we don't assume the
filehandle is verified elsewhere in the compound. But I think this
fixes the immediate issue.
Reported-by: Dan Carpenter <[email protected]>
Fixes: 4e48f1cccab3 "NFSD: allow inter server COPY to have... "
Signed-off-by: J. Bruce Fields <[email protected]>
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index d33c39c18cdd..5c7f622fed29 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -2368,7 +2368,8 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
if (op->opdesc->op_flags & OP_CLEAR_STATEID)
clear_current_stateid(cstate);
- if (need_wrongsec_check(rqstp))
+ if (current->fh->fh_export &&
+ need_wrongsec_check(rqstp))
op->status = check_nfsd_access(current_fh->fh_export, rqstp);
}
encode_op:
On Fri, Dec 06, 2019 at 04:14:42PM -0500, bfields wrote:
> On Wed, Dec 04, 2019 at 09:38:26PM -0500, J. Bruce Fields wrote:
> > So, stuff we could do:
> >
> > - add an extra check of fh_export or something here.
>
> So, I'm applying the following for now.
> + if (current->fh->fh_export &&
^^^
Um, maybe with a typo or two fixed.
> + need_wrongsec_check(rqstp))
> op->status = check_nfsd_access(current_fh->fh_export, rqstp);
> }
> encode_op:
On Fri, Dec 6, 2019 at 4:15 PM J. Bruce Fields <[email protected]> wrote:
>
> On Fri, Dec 06, 2019 at 04:14:42PM -0500, bfields wrote:
> > On Wed, Dec 04, 2019 at 09:38:26PM -0500, J. Bruce Fields wrote:
> > > So, stuff we could do:
> > >
> > > - add an extra check of fh_export or something here.
> >
> > So, I'm applying the following for now.
> > + if (current->fh->fh_export &&
> ^^^
>
> Um, maybe with a typo or two fixed.
>
>
> > + need_wrongsec_check(rqstp))
> > op->status = check_nfsd_access(current_fh->fh_export, rqstp);
> > }
> > encode_op:
Sure thing. But I just finished up and have hacked up the client to
send a GETATTR after the 1st PUTFH in the COPY compound of the inter
copy. The server doesn't croak but returns an ERR_STALE on the 1st
PUTFH (I believe this is due to the logic that it's not a valid inter
COPY compound.. so logic works).
but I have nothing against adding the check.