* [bug report] NFSD: allow inter server COPY to have a STALE source server fh @ 2019-12-04 8:00 Dan Carpenter 2019-12-04 20:11 ` Olga Kornievskaia 0 siblings, 1 reply; 7+ messages in thread From: Dan Carpenter @ 2019-12-04 8:00 UTC (permalink / raw) To: olga.kornievskaia; +Cc: linux-nfs 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] NFSD: allow inter server COPY to have a STALE source server fh 2019-12-04 8:00 [bug report] NFSD: allow inter server COPY to have a STALE source server fh Dan Carpenter @ 2019-12-04 20:11 ` Olga Kornievskaia 2019-12-04 22:04 ` J. Bruce Fields 0 siblings, 1 reply; 7+ messages in thread From: Olga Kornievskaia @ 2019-12-04 20:11 UTC (permalink / raw) To: Dan Carpenter, J. Bruce Fields; +Cc: linux-nfs On Wed, Dec 4, 2019 at 3:00 AM Dan Carpenter <dan.carpenter@oracle.com> 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] NFSD: allow inter server COPY to have a STALE source server fh 2019-12-04 20:11 ` Olga Kornievskaia @ 2019-12-04 22:04 ` J. Bruce Fields 2019-12-05 2:38 ` J. Bruce Fields 0 siblings, 1 reply; 7+ messages in thread From: J. Bruce Fields @ 2019-12-04 22:04 UTC (permalink / raw) To: Olga Kornievskaia; +Cc: Dan Carpenter, linux-nfs On Wed, Dec 04, 2019 at 03:11:01PM -0500, Olga Kornievskaia wrote: > On Wed, Dec 4, 2019 at 3:00 AM Dan Carpenter <dan.carpenter@oracle.com> 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] NFSD: allow inter server COPY to have a STALE source server fh 2019-12-04 22:04 ` J. Bruce Fields @ 2019-12-05 2:38 ` J. Bruce Fields 2019-12-06 21:14 ` J. Bruce Fields 0 siblings, 1 reply; 7+ messages in thread From: J. Bruce Fields @ 2019-12-05 2:38 UTC (permalink / raw) To: Olga Kornievskaia; +Cc: Dan Carpenter, linux-nfs 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 <dan.carpenter@oracle.com> 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] NFSD: allow inter server COPY to have a STALE source server fh 2019-12-05 2:38 ` J. Bruce Fields @ 2019-12-06 21:14 ` J. Bruce Fields 2019-12-06 21:15 ` J. Bruce Fields 0 siblings, 1 reply; 7+ messages in thread From: J. Bruce Fields @ 2019-12-06 21:14 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Olga Kornievskaia, Dan Carpenter, linux-nfs 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 <bfields@redhat.com> 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 <dan.carpenter@oracle.com> Fixes: 4e48f1cccab3 "NFSD: allow inter server COPY to have... " Signed-off-by: J. Bruce Fields <bfields@redhat.com> 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: ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [bug report] NFSD: allow inter server COPY to have a STALE source server fh 2019-12-06 21:14 ` J. Bruce Fields @ 2019-12-06 21:15 ` J. Bruce Fields 2019-12-06 21:27 ` Olga Kornievskaia 0 siblings, 1 reply; 7+ messages in thread From: J. Bruce Fields @ 2019-12-06 21:15 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Olga Kornievskaia, Dan Carpenter, linux-nfs 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: ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] NFSD: allow inter server COPY to have a STALE source server fh 2019-12-06 21:15 ` J. Bruce Fields @ 2019-12-06 21:27 ` Olga Kornievskaia 0 siblings, 0 replies; 7+ messages in thread From: Olga Kornievskaia @ 2019-12-06 21:27 UTC (permalink / raw) To: J. Bruce Fields; +Cc: J. Bruce Fields, Dan Carpenter, linux-nfs On Fri, Dec 6, 2019 at 4:15 PM J. Bruce Fields <bfields@fieldses.org> 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-12-06 21:27 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-04 8:00 [bug report] NFSD: allow inter server COPY to have a STALE source server fh Dan Carpenter 2019-12-04 20:11 ` Olga Kornievskaia 2019-12-04 22:04 ` J. Bruce Fields 2019-12-05 2:38 ` J. Bruce Fields 2019-12-06 21:14 ` J. Bruce Fields 2019-12-06 21:15 ` J. Bruce Fields 2019-12-06 21:27 ` Olga Kornievskaia
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).