Re: [PATCH v2 3/3] NFSD: Sign filehandles

[Chuck Lever <cel@kernel.org>]

On 1/23/26 5:21 PM, NeilBrown wrote:
> On Sat, 24 Jan 2026, Chuck Lever wrote:
>>
>> On Wed, Jan 21, 2026, at 3:24 PM, Benjamin Coddington wrote:
>>> NFS clients may bypass restrictive directory permissions by using
>>> open_by_handle() (or other available OS system call) to guess the
>>> filehandles for files below that directory.
>>>
>>> In order to harden knfsd servers against this attack, create a method to
>>> sign and verify filehandles using siphash as a MAC (Message Authentication
>>> Code).  Filehandles that have been signed cannot be tampered with, nor can
>>> clients reasonably guess correct filehandles and hashes that may exist in
>>> parts of the filesystem they cannot access due to directory permissions.
>>>
>>> Append the 8 byte siphash to encoded filehandles for exports that have set
>>> the "sign_fh" export option.  The filehandle's fh_auth_type is set to
>>> FH_AT_MAC(1) to indicate the filehandle is signed.  Filehandles received from
>>> clients are verified by comparing the appended hash to the expected hash.
>>> If the MAC does not match the server responds with NFS error _BADHANDLE.
>>> If unsigned filehandles are received for an export with "sign_fh" they are
>>> rejected with NFS error _BADHANDLE.
>>>
>>> Link: 
>>> https://lore.kernel.org/linux-nfs/cover.1769026777.git.bcodding@hammerspace.com
>>> Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
>>> ---
>>>  fs/nfsd/nfsfh.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++--
>>>  fs/nfsd/nfsfh.h |  3 ++
>>>  2 files changed, 73 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
>>> index ed85dd43da18..ea3473acbf71 100644
>>> --- a/fs/nfsd/nfsfh.c
>>> +++ b/fs/nfsd/nfsfh.c
>>> @@ -11,6 +11,7 @@
>>>  #include <linux/exportfs.h>
>>>
>>>  #include <linux/sunrpc/svcauth_gss.h>
>>> +#include <crypto/utils.h>
>>>  #include "nfsd.h"
>>>  #include "vfs.h"
>>>  #include "auth.h"
>>> @@ -137,6 +138,61 @@ static inline __be32 check_pseudo_root(struct 
>>> dentry *dentry,
>>>  	return nfs_ok;
>>>  }
>>>
>>> +/*
>>> + * Append an 8-byte MAC to the filehandle hashed from the server's 
>>> fh_key:
>>> + */
>>> +static int fh_append_mac(struct svc_fh *fhp, struct net *net)
>>> +{
>>> +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>>> +	struct knfsd_fh *fh = &fhp->fh_handle;
>>> +	siphash_key_t *fh_key = nn->fh_key;
>>> +	u64 hash;
>>> +
>>> +	if (!(fhp->fh_export->ex_flags & NFSEXP_SIGN_FH))
>>> +		return 0;
>>> +
>>> +	if (!fh_key) {
>>> +		pr_warn_ratelimited("NFSD: unable to sign filehandles, fh_key not 
>>> set.\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (fh->fh_size + sizeof(hash) > fhp->fh_maxsize) {
>>> +		pr_warn_ratelimited("NFSD: unable to sign filehandles, fh_size %d 
>>> would be greater"
>>> +			" than fh_maxsize %d.\n", (int)(fh->fh_size + sizeof(hash)), 
>>> fhp->fh_maxsize);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	fh->fh_auth_type = FH_AT_MAC;
>>> +	hash = siphash(&fh->fh_raw, fh->fh_size, fh_key);
>>> +	memcpy(&fh->fh_raw[fh->fh_size], &hash, sizeof(hash));
>>> +	fh->fh_size += sizeof(hash);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +/*
>>> + * Verify that the the filehandle's MAC was hashed from this filehandle
>>> + * given the server's fh_key:
>>> + */
>>> +static int fh_verify_mac(struct svc_fh *fhp, struct net *net)
>>> +{
>>> +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>>> +	struct knfsd_fh *fh = &fhp->fh_handle;
>>> +	siphash_key_t *fh_key = nn->fh_key;
>>> +	u64 hash;
>>> +
>>> +	if (fhp->fh_handle.fh_auth_type != FH_AT_MAC)
>>> +		return -EINVAL;
>>> +
>>> +	if (!fh_key) {
>>> +		pr_warn_ratelimited("NFSD: unable to verify signed filehandles, 
>>> fh_key not set.\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	hash = siphash(&fh->fh_raw, fh->fh_size - sizeof(hash),  fh_key);
>>> +	return crypto_memneq(&fh->fh_raw[fh->fh_size - sizeof(hash)], &hash, 
>>> sizeof(hash));
>>> +}
>>> +
>>>  /*
>>>   * Use the given filehandle to look up the corresponding export and
>>>   * dentry.  On success, the results are used to set fh_export and
>>> @@ -166,8 +222,11 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst 
>>> *rqstp, struct net *net,
>>>
>>>  	if (--data_left < 0)
>>>  		return error;
>>> -	if (fh->fh_auth_type != 0)
>>> +
>>> +	/* either FH_AT_NONE or FH_AT_MAC */
>>> +	if (fh->fh_auth_type > 1)
>>>  		return error;
>>> +
>>>  	len = key_len(fh->fh_fsid_type) / 4;
>>>  	if (len == 0)
>>>  		return error;
>>> @@ -237,9 +296,14 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst 
>>> *rqstp, struct net *net,
>>>
>>>  	fileid_type = fh->fh_fileid_type;
>>>
>>> -	if (fileid_type == FILEID_ROOT)
>>> +	if (fileid_type == FILEID_ROOT) {
>>>  		dentry = dget(exp->ex_path.dentry);
>>> -	else {
>>> +	} else {
>>> +		if (exp->ex_flags & NFSEXP_SIGN_FH && fh_verify_mac(fhp, net)) {
>>> +			trace_nfsd_set_fh_dentry_badhandle(rqstp, fhp, -EKEYREJECTED);
>>> +			goto out;
>>> +		}
>>> +
>>>  		dentry = exportfs_decode_fh_raw(exp->ex_path.mnt, fid,
>>>  						data_left, fileid_type, 0,
>>>  						nfsd_acceptable, exp);
>>> @@ -495,6 +559,9 @@ static void _fh_update(struct svc_fh *fhp, struct 
>>> svc_export *exp,
>>>  		fhp->fh_handle.fh_fileid_type =
>>>  			fileid_type > 0 ? fileid_type : FILEID_INVALID;
>>>  		fhp->fh_handle.fh_size += maxsize * 4;
>>> +
>>> +		if (fh_append_mac(fhp, exp->cd->net))
>>> +			fhp->fh_handle.fh_fileid_type = FILEID_INVALID;
>>>  	} else {
>>>  		fhp->fh_handle.fh_fileid_type = FILEID_ROOT;
>>>  	}
>>> diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
>>> index 5ef7191f8ad8..7fff46ac2ba8 100644
>>> --- a/fs/nfsd/nfsfh.h
>>> +++ b/fs/nfsd/nfsfh.h
>>> @@ -59,6 +59,9 @@ struct knfsd_fh {
>>>  #define fh_fsid_type		fh_raw[2]
>>>  #define fh_fileid_type		fh_raw[3]
>>>
>>> +#define FH_AT_NONE		0
>>> +#define FH_AT_MAC		1
>>
>> I'm pleased at how much this patch has shrunk since v1.
>>
>> This might not be an actionable review comment, but help me understand
>> this particular point. Why do you need both a sign_fh export option
>> and a new FH auth type? Shouldn't the server just look for and
>> validate FH signatures whenever the sign_fh export option is
>> present?
> 
> ...and also generate valid signatures on outgoing file handles.
> 
> What does the server do to "look for" an FH signature so that it can
> "validate" it?  Answer: it inspects the fh_auth_type to see if it is
> FT_AT_MAC. 

No, NFSD checks the sign_fh export option. At first glance the two
seem redundant, and I might hesitate to inspect or not inspect
depending on information content received from a remote system. The
security policy is defined precisely by the "sign_fh" export option I
would think?


>> It seems a little subtle, so perhaps a code comment somewhere could
>> explain the need for both.
> 
> /* 
>  * FT_AT_MAC allows the server to detect if a signature is expected
>  * in the last 8 bytes of the file handle.
>  */
> 
> I wonder if it is really "last 8" for NFSv2 ...  or even if v2 is
> supported.  I should check the code I guess.

I believe NFSv2 is not supported.


>>
>>> +
>>>  static inline u32 *fh_fsid(const struct knfsd_fh *fh)
>>>  {
>>>  	return (u32 *)&fh->fh_raw[4];
>>> -- 
>>> 2.50.1
>>
>> -- 
>> Chuck Lever
>>
> 


-- 
Chuck Lever