Re: [PATCH] nfsd: prevent SAVEFH from bypassing export security

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2026-10-27 at 12:44 +0000, Adolph Zeng wrote:
> 
> Hi,
> I was informed that Hotmail may reject incoming messages from kernel.org, so I am switching to this Gmail address to avoid missing any replies.
> My original patch was sent from coderzeng@hotmail.com. Please use coderzeng@gmail.com for any further correspondence.
> Apologies for the inconvenience.
> Best regards,
> Adolph Zeng
> From: Zeng Adolph <coderzeng@hotmail.com>
> Sent: Tuesday, October 27, 2026 8:40 PM
> To: cel@kernel.org <cel@kernel.org>; jlayton@kernel.org <jlayton@kernel.org>; neil@brown.name <neil@brown.name>; okorniev@redhat.com <okorniev@redhat.com>; Dai.Ngo@oracle.com <Dai.Ngo@oracle.com>; tom@talpey.com <tom@talpey.com>
> Cc: linux-nfs@vger.kernel.org <linux-nfs@vger.kernel.org>
> Subject: [PATCH] nfsd: prevent SAVEFH from bypassing export security
> 
>  
> 
> 
> An AUTH_SYS NFSv4 client can read or write a file on a sec=krb5p
> export using another client's valid OPEN stateid by inserting
> SAVEFH between PUTFH and READ or WRITE.
> 
> 
> PUTFH resolves the handle with NFSD_MAY_BYPASS_GSS, expecting
> a later security check. SAVEFH carries OP_HANDLES_WRONGSEC even though
> it only saves the handle, so PUTFH skips NFS4ERR_WRONGSEC. Stateid I/O
> can then reuse a cached nfsd_file.
> 
> 
> The original report exported /srv/nfs-sec to * with these options:
> 
> 
>   rw,sync,no_subtree_check,fsid=0,sec=krb5p,secure,root_squash
> 
> 
> The 0600, nobody:nogroup file /srv/nfs-sec/secret.txt held 32 random
> bytes. A Kerberos privacy NFSv4.0 client ran SETCLIENTID, OPEN and
> OPEN_CONFIRM, then kept its read/write stateid live. A separate
> AUTH_SYS client used UID/GID 65534 and a reserved source port.
> 
> 
> The AUTH_SYS client enumerated a bounded range of nearby clientid
> counters, stateid numbers and seqids to find the live OPEN stateid.
> With it, PUTFH, READ returned NFS4ERR_WRONGSEC, while PUTFH, SAVEFH,
> READ returned the file data and PUTFH, SAVEFH, WRITE changed it. The
> Kerberos client subsequently read the changed contents.
> 
> 
> SAVEFH leaves CURRENT_FH in place. When deciding whether PUTFH must
> report NFS4ERR_WRONGSEC, look past SAVEFH to the first operation that
> uses or replaces CURRENT_FH. If another PUTFH replaces it, leave the
> earlier PUTFH unchecked, as required by RFC 8881 Section 2.6.3.1.1.
> 
> 
> Local COPY and CLONE consume SAVED_FH as the source. Check the
> saved export with check_nfsd_access() before using its stateid:
> a cached open stateid can otherwise bypass the sec= policy. Map
> NFS4ERR_WRONGSEC to NFS4ERR_ACCESS at COPY or CLONE, since neither
> operation permits NFS4ERR_WRONGSEC. The check also applies when
> SECINFO_NO_NAME or RESTOREFH intervenes. LINK and RENAME already
> check their saved handle in their own paths.
> 
> 
> An inter-server COPY carries a foreign SAVED_FH. The source server
> validates it; the destination must not apply its own export policy
> to that handle. The destination PUTFH still checks its local export.
> 
> 
> KASAN guests reproduced the NFSv4.0 and local NFSv4.2 bypasses on
> d135ab453a74. With this patch, direct READ and SAVEFH READ/WRITE
> returned NFS4ERR_WRONGSEC. Local COPY/CLONE with a protected saved
> source returned NFS4ERR_ACCESS; foreign-handle inter-server COPY
> completed.
> 
> Fixes: 68d93184352f ("nfsd4: fix wrongsec handling for PUTFH + op cases")
> Assisted-by: LLM
> Signed-off-by: Zeng Adolph <coderzeng@hotmail.com>
> ---
> Testing: two x86_64 guests covered the original NFSv4.0 report,
> six SAVEFH security boundaries, NFSv4.2 local COPY/CLONE, and a
> foreign-handle inter-server COPY. W=1 nfs4proc.o built with
> CONFIG_NFSD_V4_2_INTER_SSC enabled and disabled. The ASan/UBSan
> decision harness passed 262 cases. The KASAN guest reported no
> KASAN, Oops, or NFSD warnings.
> 

This needs to be sent as a proper inline patch. We can't apply this as
it's whitespace-damaged.

Also, some suggestions from Claude below that look valid:


> 
>  fs/nfsd/nfs4proc.c | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index a50f9dcb3..25fb7fb61 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1491,6 +1491,11 @@ nfsd4_verify_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>       if (!cstate->save_fh.fh_dentry)
>             return nfserr_nofilehandle;
>  
> +     /* A cached open stateid does not authorize the saved source export. */
> +     status = check_nfsd_access(cstate->save_fh.fh_export, rqstp);
> +     if (status)
> +           return nfserr_acces;
> +

This is a point fix for the one remaining saved-fh consumer. The common
factor in every variant of this bug is that nfs4_check_file() only
calls nfsd_permission() when it finds a cached nfsd_file. It never
checks the security flavor:

fs/nfsd/nfs4state.c:nfs4_check_file() {
	nf = nfs4_find_file(s, flags);
	if (nf) {
		status = nfsd_permission(&rqstp->rq_cred,
					 fhp->fh_export, fhp->fh_dentry,
				acc | NFSD_MAY_OWNER_OVERRIDE);

Only the nfsd_file_acquire() path goes through fh_verify(). Would it be
worth also adding a check_nfsd_access() there (mapped to nfserr_acces),
as a backstop in case there are other op orderings that skip the PUTFH
check?

>       status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->save_fh,
>                                   src_stateid, RD_STATE, src, NULL);
>       if (status)
> @@ -3237,32 +3242,29 @@ static bool need_wrongsec_check(struct svc_rqst *rqstp)
>       struct nfsd4_compoundres *resp = rqstp->rq_resp;
>       struct nfsd4_compoundargs *argp = rqstp->rq_argp;
>       struct nfsd4_op *this = &argp->ops[resp->opcnt - 1];
> -     struct nfsd4_op *next = &argp->ops[resp->opcnt];
> +     struct nfsd4_op *next;
>       const struct nfsd4_operation *thisd = OPDESC(this);
>       const struct nfsd4_operation *nextd;
> +     unsigned int i;
>  
> -     /*
> -      * Most ops check wronsec on our own; only the putfh-like ops
> -      * have special rules.
> -      */
>       if (!(thisd->op_flags & OP_IS_PUTFH_LIKE))
>             return false;
> -     /*
> -      * rfc 5661 2.6.3.1.1.6: don't bother erroring out a
> -      * put-filehandle operation if we're not going to use the
> -      * result:
> -      */
> -     if (argp->opcnt == resp->opcnt)
> +
> +     /* SAVEFH does not consume the current filehandle. */
> +     for (i = resp->opcnt; i < argp->opcnt; i++) {
> +           next = &argp->ops[i];
> +           if (next->opnum != OP_SAVEFH)
> +                 break;
> +     }
> +     if (i == argp->opcnt)
>             return false;
> +
> +     next = &argp->ops[i];

This isn't a bug, but next already points to argp->ops[i] when the loop
breaks early, so isn't this assignment redundant?

>       if (next->opnum == OP_ILLEGAL)
>             return false;
>       nextd = OPDESC(next);
> -     /*
> -      * Rest of 2.6.3.1.1: certain operations will return WRONGSEC
> -      * errors themselves as necessary; others should check for them
> -      * now:
> -      */
> -     return !(nextd->op_flags & OP_HANDLES_WRONGSEC);
> +     return !(nextd->op_flags &
> +             (OP_HANDLES_WRONGSEC | OP_IS_PUTFH_LIKE));

This isn't a bug, but the existing comments explaining which RFC rule
each test implements were dropped. The function now encodes three
separate rules (2.6.3.1.1.1 for SAVEFH, 2.6.3.1.1.2 for consecutive
put-filehandle ops, 2.6.3.1.1.6 for end of compound). Could the
comments be kept and updated to cite the RFC 8881 sections instead?

>  }
>  
>  #ifdef CONFIG_NFSD_V4_2_INTER_SSC
> --
> 2.43.0
> 

-- 
Jeff Layton <jlayton@kernel.org>