[bug report] NFSD: allow inter server COPY to have a STALE source server fh

[bug report] NFSD: allow inter server COPY to have a STALE source server fh

[Dan Carpenter]

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

Re: [bug report] NFSD: allow inter server COPY to have a STALE source server fh

[Olga Kornievskaia]

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

Re: [bug report] NFSD: allow inter server COPY to have a STALE source server fh

[J. Bruce Fields]

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.

Re: [bug report] NFSD: allow inter server COPY to have a STALE source server fh

[J. Bruce Fields]

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.

Re: [bug report] NFSD: allow inter server COPY to have a STALE source server fh

[J. Bruce Fields]

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:

Re: [bug report] NFSD: allow inter server COPY to have a STALE source server fh

[J. Bruce Fields]

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:

Re: [bug report] NFSD: allow inter server COPY to have a STALE source server fh

[Olga Kornievskaia]

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.