Skip to contentSkip to conversation

kNFSD Signed Filehandles

PATCH Superseded Benjamin Coddington 4 participants 14 messages Feb 9 – Feb 10

Subsystems (8)

v5 of 7 Superseded Latest is v7 →

lore
Share & tools
lore
#0 lore raw

v5 cover kNFSD Signed Filehandles

The following series enables the linux NFS server to add a Message Authentication Code (MAC) to the filehandles it gives to clients. This provides additional protection to the exported filesystem against filehandle guessing attacks.

Filesystems generate their own filehandles through the export_operation "encode_fh" and a filehandle provides sufficient access to open a file without needing to perform a lookup. A trusted NFS client holding a valid filehandle can remotely access the corresponding file without reference to access-path restrictions that might be imposed by the ancestor directories or the server exports.

In order to acquire a filehandle, you must perform lookup operations on the parent directory(ies), and the permissions on those directories may prohibit you from walking into them to find the files within. This would normally be considered sufficient protection on a local filesystem to prohibit users from accessing those files, however when the filesystem is exported via NFS an exported file can be accessed whenever the NFS server is presented with the correct filehandle, which can be guessed or acquired by means other than LOOKUP.

Filehandles are easy to guess because they are well-formed. The open_by_handle_at(2) man page contains an example C program (t_name_to_handle_at.c) that can display a filehandle given a path. Here's an example filehandle from a fairly modern XFS:

# ./t_name_to_handle_at /exports/foo 
57
12 129    99 00 00 00 00 00 00 00 b4 10 0b 8c
          ^---------  filehandle  ----------^
          ^------- inode -------^ ^-- gen --^

This filehandle consists of a 64-bit inode number and 32-bit generation number. Because the handle is well-formed, its easy to fabricate filehandles that match other files within the same filesystem. You can simply insert inode numbers and iterate on the generation number. Eventually you'll be able to access the file using open_by_handle_at(2). For a local system, open_by_handle_at(2) requires CAP_DAC_READ_SEARCH, which protects against guessing attacks by unprivileged users.

Simple testing confirms that the correct generation number can be found within ~1200 minutes using open_by_handle_at() over NFS on a local system and it is estimated that adding network delay with genuine NFS calls may only increase this to around 24 hours.

In contrast to a local user using open_by_handle(2), the NFS server must permissively allow remote clients to open by filehandle without being able to check or trust the remote caller's access. Therefore additional protection against this attack is needed for NFS case. We propose to sign filehandles by appending an 8-byte MAC which is the siphash of the filehandle from a key set from the nfs-utilities. NFS server can then ensure that guessing a valid filehandle+MAC is practically impossible without knowledge of the MAC's key. The NFS server performs optional signing by possessing a key set from userspace and having the "sign_fh" export option.

Because filehandles are long-lived, and there's no method for expiring them, the server's key should be set once and not changed. It also should be persisted across restarts. The methods to set the key allow only setting it once, afterward it cannot be changed. A separate patchset for nfs-utils contains the userspace changes required to set the server's key.

I had planned on adding additional work to enable the server to check whether the 8-byte MAC will overflow maximum filehandle length for the protocol at export time. There could be some filesystems with 40-byte fileid and 24-byte fsid which would break NFSv3's 64-byte filehandle maximum with an 8-byte MAC appended. The server should refuse to export those filesystems when "sign_fh" is requested. However, the way the export caches work (the server may not even be running when a user sets up the export) its impossible to do this check at export time. Instead, the server will refuse to give out filehandles at mount time and emit a pr_warn().

Thanks for any comments and critique.

Changes from encrypt_fh posting:
https://lore.kernel.org/linux-nfs/[email protected]
- sign filehandles instead of encrypt them (Eric Biggers)
- fix the NFSEXP_ macros, specifically NFSEXP_ALLFLAGS (NeilBrown)
- rebase onto cel/nfsd-next (Chuck Lever)
- condensed/clarified problem explantion (thanks Chuck Lever)
- add nfsctl file "fh_key" for rpc.nfsd to also set the key

Changes from v1 posting:
https://lore.kernel.org/linux-nfs/[email protected]
- remove fh_fileid_offset() (Chuck Lever)
- fix pr_warns, fix memcmp (Chuck Lever)
- remove incorrect rootfh comment (NeilBrown)
- make fh_key setting an optional attr to threads verb (Jeff Layton)
- drop BIT() EXP_ flag conversion
- cover-letter tune-ups (NeilBrown, Chuck Lever)
- fix NFSEXP_ALLFLAGS on 2/3
- cast fh->fh_size + sizeof(hash) result to int (avoid x86_64 WARNING)
- move MAC signing into __fh_update() (Chuck Lever)

Changes from v2 posting:
https://lore.kernel.org/linux-nfs/[email protected]
- more cover-letter detail (NeilBrown)
- Documentation/filesystems/nfs/exporting.rst section (Jeff Layton)
- fix key copy (Eric Biggers)
- use NFSD_A_SERVER_MAX (NeilBrown)
- remove procfs fh_key interface (Chuck Lever)
- remove FH_AT_MAC (Chuck Lever)
- allow fh_key change when server is not running (Chuck/Jeff)
- accept fh_key as netlink attribute instead of command (Jeff Layton)

Changes from v3 posting:
https://lore.kernel.org/linux-nfs/[email protected]
- /actually/ fix up endianness problems (Eric Biggers)
- comment typo
- fix Documentation underline warnings
- fix possible uninitialized fh_key var

Changes from v4 posting:
https://lore.kernel.org/linux-nfs/[email protected]
- again (!!) fix endian copy from userspace (Chuck Lever)
- fixup protocol return error for MAC verification failure (Chuck Lever)
- fix filehandle size after MAC verification (Chuck Lever)
- fix two sparse errors (LKP)

Benjamin Coddington (3):
NFSD: Add a key for signing filehandles
NFSD/export: Add sign_fh export option
NFSD: Sign filehandles

 Documentation/filesystems/nfs/exporting.rst | 85 +++++++++++++++++++++
 Documentation/netlink/specs/nfsd.yaml       |  6 ++
 fs/nfsd/export.c                            |  5 +-
 fs/nfsd/netlink.c                           |  5 +-
 fs/nfsd/netns.h                             |  2 +
 fs/nfsd/nfsctl.c                            | 38 ++++++++-
 fs/nfsd/nfsfh.c                             | 70 ++++++++++++++++-
 fs/nfsd/trace.h                             | 26 +++++++
 include/uapi/linux/nfsd/export.h            |  4 +-
 include/uapi/linux/nfsd_netlink.h           |  1 +
 10 files changed, 231 insertions(+), 11 deletions(-)

base-commit: e3934bbd57c73b3835a77562ca47b5fbc6f34287

Signature · 1 line
2.50.1
#1 lore raw

PATCH v5 1/3 NFSD: Add a key for signing filehandles

A future patch will enable NFSD to sign filehandles by appending a Message Authentication Code(MAC). To do this, NFSD requires a secret 128-bit key that can persist across reboots. A persisted key allows the server to accept filehandles after a restart. Enable NFSD to be configured with this key the netlink interface.

---
 Documentation/netlink/specs/nfsd.yaml |  6 +++++
 fs/nfsd/netlink.c                     |  5 ++--
 fs/nfsd/netns.h                       |  2 ++
 fs/nfsd/nfsctl.c                      | 38 ++++++++++++++++++++++++++-
 fs/nfsd/trace.h                       | 25 ++++++++++++++++++
 include/uapi/linux/nfsd_netlink.h     |  1 +
 6 files changed, 74 insertions(+), 3 deletions(-)
Documentation/netlink/specs/nfsd.yaml +6 -0
index f87b5a05e5e9..8ab43c8253b2 100644
@@ -81,6 +81,11 @@ attribute-sets:
       -
         name: min-threads
         type: u32
+      -
+        name: fh-key
+        type: binary
+        checks:
+            exact-len: 16
   -
     name: version
     attributes:
@@ -163,6 +168,7 @@ operations:
             - leasetime
             - scope
             - min-threads
+            - fh-key
     -
       name: threads-get
       doc: get the maximum number of running threads
fs/nfsd/netlink.c +3 -2
index 887525964451..4e08c1a6b394 100644
@@ -24,12 +24,13 @@ const struct nla_policy nfsd_version_nl_policy[NFSD_A_VERSION_ENABLED + 1] = {
 };
 
 /* NFSD_CMD_THREADS_SET - do */
-static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_MIN_THREADS + 1] = {
+static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_FH_KEY + 1] = {
 	[NFSD_A_SERVER_THREADS] = { .type = NLA_U32, },
 	[NFSD_A_SERVER_GRACETIME] = { .type = NLA_U32, },
 	[NFSD_A_SERVER_LEASETIME] = { .type = NLA_U32, },
 	[NFSD_A_SERVER_SCOPE] = { .type = NLA_NUL_STRING, },
 	[NFSD_A_SERVER_MIN_THREADS] = { .type = NLA_U32, },
+	[NFSD_A_SERVER_FH_KEY] = NLA_POLICY_EXACT_LEN(16),
 };
 
 /* NFSD_CMD_VERSION_SET - do */
@@ -58,7 +59,7 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
 		.cmd		= NFSD_CMD_THREADS_SET,
 		.doit		= nfsd_nl_threads_set_doit,
 		.policy		= nfsd_threads_set_nl_policy,
-		.maxattr	= NFSD_A_SERVER_MIN_THREADS,
+		.maxattr	= NFSD_A_SERVER_MAX,
 		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
 	},
 	{
fs/nfsd/netns.h +2 -0
index 9fa600602658..c8ed733240a0 100644
@@ -16,6 +16,7 @@
 #include <linux/percpu-refcount.h>
 #include <linux/siphash.h>
 #include <linux/sunrpc/stats.h>
+#include <linux/siphash.h>
 
 /* Hash tables for nfs4_clientid state */
 #define CLIENT_HASH_BITS                 4
@@ -224,6 +225,7 @@ struct nfsd_net {
 	spinlock_t              local_clients_lock;
 	struct list_head	local_clients;
 #endif
+	siphash_key_t		*fh_key;
 };
 
 /* Simple check to find out if a given net was properly initialized */
fs/nfsd/nfsctl.c +37 -1
index a58eb1adac0f..36e2acf1d18b 100644
@@ -1571,6 +1571,32 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
 	return ret;
 }
 
+/**
+ * nfsd_nl_fh_key_set - helper to copy fh_key from userspace
+ * @attr: nlattr NFSD_A_SERVER_FH_KEY
+ * @nn: nfsd_net
+ *
+ * Callers should hold nfsd_mutex, returns 0 on success or negative errno.
+ */
+static int nfsd_nl_fh_key_set(const struct nlattr *attr, struct nfsd_net *nn)
+{
+	siphash_key_t *fh_key = nn->fh_key;
+
+	if (nla_len(attr) != sizeof(siphash_key_t))
+		return -EINVAL;
+
+	if (!fh_key) {
+		fh_key = kmalloc(sizeof(siphash_key_t), GFP_KERNEL);
+		if (!fh_key)
+			return -ENOMEM;
+		nn->fh_key = fh_key;
+	}
+
+	fh_key->key[0] = get_unaligned_le64(nla_data(attr));
+	fh_key->key[1] = get_unaligned_le64(nla_data(attr) + 8);
+	return 0;
+}
+
 /**
  * nfsd_nl_threads_set_doit - set the number of running threads
  * @skb: reply buffer
@@ -1612,7 +1638,8 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
 
 	if (info->attrs[NFSD_A_SERVER_GRACETIME] ||
 	    info->attrs[NFSD_A_SERVER_LEASETIME] ||
-	    info->attrs[NFSD_A_SERVER_SCOPE]) {
+	    info->attrs[NFSD_A_SERVER_SCOPE] ||
+	    info->attrs[NFSD_A_SERVER_FH_KEY]) {
 		ret = -EBUSY;
 		if (nn->nfsd_serv && nn->nfsd_serv->sv_nrthreads)
 			goto out_unlock;
@@ -1641,6 +1668,14 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
 		attr = info->attrs[NFSD_A_SERVER_SCOPE];
 		if (attr)
 			scope = nla_data(attr);
+
+		attr = info->attrs[NFSD_A_SERVER_FH_KEY];
+		if (attr) {
+			ret = nfsd_nl_fh_key_set(attr, nn);
+			trace_nfsd_ctl_fh_key_set((const char *)nn->fh_key, ret);
+			if (ret)
+				goto out_unlock;
+		}
 	}
 
 	attr = info->attrs[NFSD_A_SERVER_MIN_THREADS];
@@ -2240,6 +2275,7 @@ static __net_exit void nfsd_net_exit(struct net *net)
 {
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 
+	kfree_sensitive(nn->fh_key);
 	nfsd_proc_stat_shutdown(net);
 	percpu_counter_destroy_many(nn->counter, NFSD_STATS_COUNTERS_NUM);
 	nfsd_idmap_shutdown(net);
fs/nfsd/trace.h +25 -0
index d1d0b0dd0545..c1a5f2fa44ab 100644
@@ -2240,6 +2240,31 @@ TRACE_EVENT(nfsd_end_grace,
 	)
 );
 
+TRACE_EVENT(nfsd_ctl_fh_key_set,
+	TP_PROTO(
+		const char *key,
+		int result
+	),
+	TP_ARGS(key, result),
+	TP_STRUCT__entry(
+		__array(unsigned char, key, 16)
+		__field(unsigned long, result)
+		__field(bool, key_set)
+	),
+	TP_fast_assign(
+		__entry->key_set = true;
+		if (!key)
+			__entry->key_set = false;
+		else
+			memcpy(__entry->key, key, 16);
+		__entry->result = result;
+	),
+	TP_printk("key=%s result=%ld",
+		__entry->key_set ? __print_hex_str(__entry->key, 16) : "(null)",
+		__entry->result
+	)
+);
+
 DECLARE_EVENT_CLASS(nfsd_copy_class,
 	TP_PROTO(
 		const struct nfsd4_copy *copy
include/uapi/linux/nfsd_netlink.h +1 -0
index e9efbc9e63d8..97c7447f4d14 100644
@@ -36,6 +36,7 @@ enum {
 	NFSD_A_SERVER_LEASETIME,
 	NFSD_A_SERVER_SCOPE,
 	NFSD_A_SERVER_MIN_THREADS,
+	NFSD_A_SERVER_FH_KEY,
 
 	__NFSD_A_SERVER_MAX,
 	NFSD_A_SERVER_MAX = (__NFSD_A_SERVER_MAX - 1)
Signature · 1 line
2.50.1
#2 lore raw

Re: [PATCH v5 1/3] NFSD: Add a key for signing filehandles

Quoted 75 lines · Benjamin Coddington

A future patch will enable NFSD to sign filehandles by appending a Message Authentication Code(MAC). To do this, NFSD requires a secret 128-bit key that can persist across reboots. A persisted key allows the server to accept filehandles after a restart. Enable NFSD to be configured with this key the netlink interface.

---
 Documentation/netlink/specs/nfsd.yaml |  6 +++++
 fs/nfsd/netlink.c                     |  5 ++--
 fs/nfsd/netns.h                       |  2 ++
 fs/nfsd/nfsctl.c                      | 38 ++++++++++++++++++++++++++-
 fs/nfsd/trace.h                       | 25 ++++++++++++++++++
 include/uapi/linux/nfsd_netlink.h     |  1 +
 6 files changed, 74 insertions(+), 3 deletions(-)
Documentation/netlink/specs/nfsd.yaml +6 -0
index f87b5a05e5e9..8ab43c8253b2 100644
@@ -81,6 +81,11 @@ attribute-sets:
       -
         name: min-threads
         type: u32
+      -
+        name: fh-key
+        type: binary
+        checks:
+            exact-len: 16
   -
     name: version
     attributes:
@@ -163,6 +168,7 @@ operations:
             - leasetime
             - scope
             - min-threads
+            - fh-key
     -
       name: threads-get
       doc: get the maximum number of running threads
fs/nfsd/netlink.c +3 -2
index 887525964451..4e08c1a6b394 100644
@@ -24,12 +24,13 @@ const struct nla_policy nfsd_version_nl_policy[NFSD_A_VERSION_ENABLED + 1] = {
 };
 
 /* NFSD_CMD_THREADS_SET - do */
-static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_MIN_THREADS + 1] = {
+static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_FH_KEY + 1] = {
 	[NFSD_A_SERVER_THREADS] = { .type = NLA_U32, },
 	[NFSD_A_SERVER_GRACETIME] = { .type = NLA_U32, },
 	[NFSD_A_SERVER_LEASETIME] = { .type = NLA_U32, },
 	[NFSD_A_SERVER_SCOPE] = { .type = NLA_NUL_STRING, },
 	[NFSD_A_SERVER_MIN_THREADS] = { .type = NLA_U32, },
+	[NFSD_A_SERVER_FH_KEY] = NLA_POLICY_EXACT_LEN(16),
 };
 
 /* NFSD_CMD_VERSION_SET - do */
@@ -58,7 +59,7 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
 		.cmd		= NFSD_CMD_THREADS_SET,
 		.doit		= nfsd_nl_threads_set_doit,
 		.policy		= nfsd_threads_set_nl_policy,
-		.maxattr	= NFSD_A_SERVER_MIN_THREADS,
+		.maxattr	= NFSD_A_SERVER_MAX,
 		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
 	},
 	{
fs/nfsd/netns.h +0 -0
index 9fa600602658..c8ed733240a0 100644

fs/nfsd/netns.h line 19

Quoted 4 lines
fs/nfsd/netns.h +1 -0
@@ -16,6 +16,7 @@
 #include <linux/percpu-refcount.h>
 #include <linux/siphash.h>
 #include <linux/sunrpc/stats.h>
+#include <linux/siphash.h>

The added #include is a duplicate.

fs/nfsd/netns.h struct nfsd_net { · line 228

Quoted 8 lines
fs/nfsd/netns.h +1 -0
 /* Hash tables for nfs4_clientid state */
 #define CLIENT_HASH_BITS                 4
@@ -224,6 +225,7 @@ struct nfsd_net {
 	spinlock_t              local_clients_lock;
 	struct list_head	local_clients;
 #endif
+	siphash_key_t		*fh_key;

I will make a note-to-self to update the field name of the other siphash key in this structure to match its function/purpose.

As a performance note, is this field co-located in the same cache line(s) as other fields that are accessed by the FH management code?

Quoted 8 lines
fs/nfsd/netns.h +0 -0
 };
 
 /* Simple check to find out if a given net was properly initialized */
fs/nfsd/nfsctl.c +0 -0
index a58eb1adac0f..36e2acf1d18b 100644

fs/nfsd/nfsctl.c int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb, · line 1580

Quoted 10 lines
fs/nfsd/nfsctl.c +7 -0
@@ -1571,6 +1571,32 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
 	return ret;
 }
 
+/**
+ * nfsd_nl_fh_key_set - helper to copy fh_key from userspace
+ * @attr: nlattr NFSD_A_SERVER_FH_KEY
+ * @nn: nfsd_net
+ *
+ * Callers should hold nfsd_mutex, returns 0 on success or negative errno.
+ */

The sv_nrthreads == 0 guard at line 1642 prevents setting the key while the server is running. This not only guards against spurious key replacement, but the implementation depends on this to prevent races from exposing a torn key to the FH management code. That needs to be documented somewhere as part of the API contract.

Quoted 62 lines
fs/nfsd/nfsctl.c +30 -1
+static int nfsd_nl_fh_key_set(const struct nlattr *attr, struct nfsd_net *nn)
+{
+	siphash_key_t *fh_key = nn->fh_key;
+
+	if (nla_len(attr) != sizeof(siphash_key_t))
+		return -EINVAL;
+
+	if (!fh_key) {
+		fh_key = kmalloc(sizeof(siphash_key_t), GFP_KERNEL);
+		if (!fh_key)
+			return -ENOMEM;
+		nn->fh_key = fh_key;
+	}
+
+	fh_key->key[0] = get_unaligned_le64(nla_data(attr));
+	fh_key->key[1] = get_unaligned_le64(nla_data(attr) + 8);
+	return 0;
+}
+
 /**
  * nfsd_nl_threads_set_doit - set the number of running threads
  * @skb: reply buffer
@@ -1612,7 +1638,8 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
 
 	if (info->attrs[NFSD_A_SERVER_GRACETIME] ||
 	    info->attrs[NFSD_A_SERVER_LEASETIME] ||
-	    info->attrs[NFSD_A_SERVER_SCOPE]) {
+	    info->attrs[NFSD_A_SERVER_SCOPE] ||
+	    info->attrs[NFSD_A_SERVER_FH_KEY]) {
 		ret = -EBUSY;
 		if (nn->nfsd_serv && nn->nfsd_serv->sv_nrthreads)
 			goto out_unlock;
@@ -1641,6 +1668,14 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
 		attr = info->attrs[NFSD_A_SERVER_SCOPE];
 		if (attr)
 			scope = nla_data(attr);
+
+		attr = info->attrs[NFSD_A_SERVER_FH_KEY];
+		if (attr) {
+			ret = nfsd_nl_fh_key_set(attr, nn);
+			trace_nfsd_ctl_fh_key_set((const char *)nn->fh_key, ret);
+			if (ret)
+				goto out_unlock;
+		}
 	}
 
 	attr = info->attrs[NFSD_A_SERVER_MIN_THREADS];
@@ -2240,6 +2275,7 @@ static __net_exit void nfsd_net_exit(struct net *net)
 {
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 
+	kfree_sensitive(nn->fh_key);
 	nfsd_proc_stat_shutdown(net);
 	percpu_counter_destroy_many(nn->counter, NFSD_STATS_COUNTERS_NUM);
 	nfsd_idmap_shutdown(net);
fs/nfsd/trace.h +0 -0
index d1d0b0dd0545..c1a5f2fa44ab 100644
@@ -2240,6 +2240,31 @@ TRACE_EVENT(nfsd_end_grace,
 	)
 );

fs/nfsd/trace.h TRACE_EVENT(nfsd_end_grace, · line 2251

Quoted 10 lines
fs/nfsd/trace.h +9 -0
 
+TRACE_EVENT(nfsd_ctl_fh_key_set,
+	TP_PROTO(
+		const char *key,
+		int result
+	),
+	TP_ARGS(key, result),
+	TP_STRUCT__entry(
+		__array(unsigned char, key, 16)
+		__field(unsigned long, result)

If the trace infrastructure isn't passing "result" to print_symbolic() or its brethren, then let's match its type to the "result" argument
above: int.

But see below:

Quoted 5 lines
fs/nfsd/trace.h +5 -0
+		__field(bool, key_set)
+	),
+	TP_fast_assign(
+		__entry->key_set = true;
+		if (!key)

fs/nfsd/trace.h TRACE_EVENT(nfsd_end_grace, · line 2266

Quoted 10 lines
fs/nfsd/trace.h +10 -0
+			__entry->key_set = false;
+		else
+			memcpy(__entry->key, key, 16);
+		__entry->result = result;
+	),
+	TP_printk("key=%s result=%ld",
+		__entry->key_set ? __print_hex_str(__entry->key, 16) : "(null)",
+		__entry->result
+	)
+);

Not sure how I missed this before...

We need to discuss the security implications of writing sensitive material like the server FH key to the trace log. AFAICT, no other NFSD tracepoint logs cryptographic material.

Quoted 16 lines
fs/nfsd/trace.h +1 -0
+
 DECLARE_EVENT_CLASS(nfsd_copy_class,
 	TP_PROTO(
 		const struct nfsd4_copy *copy
include/uapi/linux/nfsd_netlink.h +1 -0
index e9efbc9e63d8..97c7447f4d14 100644
@@ -36,6 +36,7 @@ enum {
 	NFSD_A_SERVER_LEASETIME,
 	NFSD_A_SERVER_SCOPE,
 	NFSD_A_SERVER_MIN_THREADS,
+	NFSD_A_SERVER_FH_KEY,
 
 	__NFSD_A_SERVER_MAX,
 	NFSD_A_SERVER_MAX = (__NFSD_A_SERVER_MAX - 1)
Signature · 1 line
Chuck Lever
#3 lore raw

Re: [PATCH v5 1/3] NFSD: Add a key for signing filehandles

Quoted 2 lines · Chuck Lever
Quoted 1 line · Benjamin Coddington

A future patch will enable NFSD to sign filehandles by appending a Message

...

Quoted 11 lines

fs/nfsd/netns.h line 19

Quoted 9 lines
fs/nfsd/netns.h +1 -0
index 9fa600602658..c8ed733240a0 100644
@@ -16,6 +16,7 @@
 #include <linux/percpu-refcount.h>
 #include <linux/siphash.h>
 #include <linux/sunrpc/stats.h>
+#include <linux/siphash.h>

The added #include is a duplicate.

Quality.

Quoted 12 lines

fs/nfsd/netns.h struct nfsd_net { · line 228

Quoted 8 lines
fs/nfsd/netns.h +1 -0
 /* Hash tables for nfs4_clientid state */
 #define CLIENT_HASH_BITS                 4
@@ -224,6 +225,7 @@ struct nfsd_net {
 	spinlock_t              local_clients_lock;
 	struct list_head	local_clients;
 #endif
+	siphash_key_t		*fh_key;

I will make a note-to-self to update the field name of the other siphash key in this structure to match its function/purpose.

Quoted 3 lines

As a performance note, is this field co-located in the same cache line(s) as other fields that are accessed by the FH management code?

The only other nfsd_net field is used by a rare error path in nfsd_stats_fh_stale_inc(), a per-cpu counter. I could try to re-arrange things for this, risk is something else gets a bit slower.

Maybe we can optimize if needed later?

Quoted 11 lines
Quoted 11 lines
fs/nfsd/netns.h +0 -0
 };
 
 /* Simple check to find out if a given net was properly initialized */
fs/nfsd/nfsctl.c +0 -0
index a58eb1adac0f..36e2acf1d18b 100644
@@ -1571,6 +1571,32 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
 	return ret;
 }
 
Quoted 13 lines

fs/nfsd/nfsctl.c int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb, · line 1580

Quoted 7 lines
fs/nfsd/nfsctl.c +7 -0
+/**
+ * nfsd_nl_fh_key_set - helper to copy fh_key from userspace
+ * @attr: nlattr NFSD_A_SERVER_FH_KEY
+ * @nn: nfsd_net
+ *
+ * Callers should hold nfsd_mutex, returns 0 on success or negative errno.
+ */

The sv_nrthreads == 0 guard at line 1642 prevents setting the key while the server is running. This not only guards against spurious key replacement, but the implementation depends on this to prevent races from exposing a torn key to the FH management code. That needs to be documented somewhere as part of the API contract.

Ok - I'll document it right there on the function.

Quoted 63 lines
Quoted 63 lines
fs/nfsd/nfsctl.c +30 -1
+static int nfsd_nl_fh_key_set(const struct nlattr *attr, struct nfsd_net *nn)
+{
+	siphash_key_t *fh_key = nn->fh_key;
+
+	if (nla_len(attr) != sizeof(siphash_key_t))
+		return -EINVAL;
+
+	if (!fh_key) {
+		fh_key = kmalloc(sizeof(siphash_key_t), GFP_KERNEL);
+		if (!fh_key)
+			return -ENOMEM;
+		nn->fh_key = fh_key;
+	}
+
+	fh_key->key[0] = get_unaligned_le64(nla_data(attr));
+	fh_key->key[1] = get_unaligned_le64(nla_data(attr) + 8);
+	return 0;
+}
+
 /**
  * nfsd_nl_threads_set_doit - set the number of running threads
  * @skb: reply buffer
@@ -1612,7 +1638,8 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
 
 	if (info->attrs[NFSD_A_SERVER_GRACETIME] ||
 	    info->attrs[NFSD_A_SERVER_LEASETIME] ||
-	    info->attrs[NFSD_A_SERVER_SCOPE]) {
+	    info->attrs[NFSD_A_SERVER_SCOPE] ||
+	    info->attrs[NFSD_A_SERVER_FH_KEY]) {
 		ret = -EBUSY;
 		if (nn->nfsd_serv && nn->nfsd_serv->sv_nrthreads)
 			goto out_unlock;
@@ -1641,6 +1668,14 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
 		attr = info->attrs[NFSD_A_SERVER_SCOPE];
 		if (attr)
 			scope = nla_data(attr);
+
+		attr = info->attrs[NFSD_A_SERVER_FH_KEY];
+		if (attr) {
+			ret = nfsd_nl_fh_key_set(attr, nn);
+			trace_nfsd_ctl_fh_key_set((const char *)nn->fh_key, ret);
+			if (ret)
+				goto out_unlock;
+		}
 	}
 
 	attr = info->attrs[NFSD_A_SERVER_MIN_THREADS];
@@ -2240,6 +2275,7 @@ static __net_exit void nfsd_net_exit(struct net *net)
 {
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 
+	kfree_sensitive(nn->fh_key);
 	nfsd_proc_stat_shutdown(net);
 	percpu_counter_destroy_many(nn->counter, NFSD_STATS_COUNTERS_NUM);
 	nfsd_idmap_shutdown(net);
fs/nfsd/trace.h +0 -0
index d1d0b0dd0545..c1a5f2fa44ab 100644
@@ -2240,6 +2240,31 @@ TRACE_EVENT(nfsd_end_grace,
 	)
 );
 
Quoted 13 lines

fs/nfsd/trace.h TRACE_EVENT(nfsd_end_grace, · line 2251

Quoted 9 lines
fs/nfsd/trace.h +9 -0
+TRACE_EVENT(nfsd_ctl_fh_key_set,
+	TP_PROTO(
+		const char *key,
+		int result
+	),
+	TP_ARGS(key, result),
+	TP_STRUCT__entry(
+		__array(unsigned char, key, 16)
+		__field(unsigned long, result)

If the trace infrastructure isn't passing "result" to print_symbolic() or its brethren, then let's match its type to the "result" argument
above: int.

Ok.

Quoted 22 lines

But see below:

Quoted 5 lines
fs/nfsd/trace.h +5 -0
+		__field(bool, key_set)
+	),
+	TP_fast_assign(
+		__entry->key_set = true;
+		if (!key)

fs/nfsd/trace.h TRACE_EVENT(nfsd_end_grace, · line 2266

Quoted 10 lines
fs/nfsd/trace.h +10 -0
+			__entry->key_set = false;
+		else
+			memcpy(__entry->key, key, 16);
+		__entry->result = result;
+	),
+	TP_printk("key=%s result=%ld",
+		__entry->key_set ? __print_hex_str(__entry->key, 16) : "(null)",
+		__entry->result
+	)
+);

Not sure how I missed this before...

Quoted 3 lines

We need to discuss the security implications of writing sensitive material like the server FH key to the trace log. AFAICT, no other NFSD tracepoint logs cryptographic material.

I thought this could come up: consider only root can view these tracepoints, many vulnerabilities can be exposed by tracepoint data. The reason for it:
sysadmin can absolutely verify that the key is getting set correctly to a expected value. There's not a lot of other visibility in our tooling for this one point.

In development, its been essential to prove that userspace is doing the hashing, that keys are changing properly when key file changes.

Let me know what you want to see here, we can hash the key again (like we do with pointer hashing), or just remove it altogether.

Ben

maintainer
#4 lore raw

Re: [PATCH v5 1/3] NFSD: Add a key for signing filehandles

Quoted 22 lines · Benjamin Coddington
Quoted 2 lines · Chuck Lever
Quoted 1 line · Benjamin Coddington

A future patch will enable NFSD to sign filehandles by appending a Message

...

Quoted 13 lines

fs/nfsd/netns.h struct nfsd_net { · line 228

Quoted 9 lines
fs/nfsd/netns.h +1 -0
index 9fa600602658..c8ed733240a0 100644
@@ -224,6 +225,7 @@ struct nfsd_net {
 	spinlock_t              local_clients_lock;
 	struct list_head	local_clients;
 #endif
+	siphash_key_t		*fh_key;

I will make a note-to-self to update the field name of the other siphash key in this structure to match its function/purpose.

Quoted 3 lines

As a performance note, is this field co-located in the same cache line(s) as other fields that are accessed by the FH management code?

Quoted 5 lines

The only other nfsd_net field is used by a rare error path in nfsd_stats_fh_stale_inc(), a per-cpu counter. I could try to re-arrange things for this, risk is something else gets a bit slower.

Maybe we can optimize if needed later?

Fair enough, later it is.

Quoted 18 lines
Quoted 18 lines
Quoted 5 lines
fs/nfsd/netns.h +5 -0
+		__field(bool, key_set)
+	),
+	TP_fast_assign(
+		__entry->key_set = true;
+		if (!key)

fs/nfsd/netns.h struct nfsd_net {

Quoted 10 lines
fs/nfsd/netns.h +10 -0
+			__entry->key_set = false;
+		else
+			memcpy(__entry->key, key, 16);
+		__entry->result = result;
+	),
+	TP_printk("key=%s result=%ld",
+		__entry->key_set ? __print_hex_str(__entry->key, 16) : "(null)",
+		__entry->result
+	)
+);

Not sure how I missed this before...

Quoted 5 lines
Quoted 3 lines

We need to discuss the security implications of writing sensitive material like the server FH key to the trace log. AFAICT, no other NFSD tracepoint logs cryptographic material.

I thought this could come up: consider only root can view these tracepoints,

That's not quite true. Yes, only root can enable tracing. But:

  # trace-cmd record -e nfsd

does record the trace event in a file and often the default mode of that file is 644.

Quoted 7 lines

many vulnerabilities can be exposed by tracepoint data. The reason for it:
sysadmin can absolutely verify that the key is getting set correctly to a expected value. There's not a lot of other visibility in our tooling for this one point.

In development, its been essential to prove that userspace is doing the hashing, that keys are changing properly when key file changes.

Development generally has different needs than deployment and/or support engineering. Security, I'm thinking, is probably the most critical need here.

Quoted 2 lines

Let me know what you want to see here, we can hash the key again (like we do with pointer hashing), or just remove it altogether.

Yeah, I was thinking of hashing it for display, but then that loses the ability to confirm the key's actual value. Another option is to leave the trace point in for the initial merge, but add a comment that says "to be removed" so that when we have confidence key setting is working reliably, it can be made more secure by removing the key from the trace record.

Signature · 1 line
Chuck Lever
#5 lore raw

Re: [PATCH v5 1/3] NFSD: Add a key for signing filehandles

Quoted 7 lines · Chuck Lever
Quoted 5 lines · Benjamin Coddington

On 9 Feb 2026, at 15:29, Chuck Lever wrote:

Let me know what you want to see here, we can hash the key again (like we do with pointer hashing), or just remove it altogether.

Quoted 6 lines

Yeah, I was thinking of hashing it for display, but then that loses the ability to confirm the key's actual value. Another option is to leave the trace point in for the initial merge, but add a comment that says "to be removed" so that when we have confidence key setting is working reliably, it can be made more secure by removing the key from the trace record.

Could just redact half of it too.. that could serve both purposes until we pull the whole tracepoint.

Ben

#6 lore raw

Re: [PATCH v5 1/3] NFSD: Add a key for signing filehandles

Quoted 16 lines · Benjamin Coddington
Quoted 7 lines · Chuck Lever
Quoted 5 lines · Benjamin Coddington

On 9 Feb 2026, at 15:29, Chuck Lever wrote:

Let me know what you want to see here, we can hash the key again (like we do with pointer hashing), or just remove it altogether.

Quoted 6 lines

Yeah, I was thinking of hashing it for display, but then that loses the ability to confirm the key's actual value. Another option is to leave the trace point in for the initial merge, but add a comment that says "to be removed" so that when we have confidence key setting is working reliably, it can be made more secure by removing the key from the trace record.

Quoted 2 lines

Could just redact half of it too.. that could serve both purposes until we pull the whole tracepoint.

Thinking about this a bit more - it matters a lot less what the key actually is, and much more what the key is compared to the last start or if the key is actually getting set to a non-zero value.

I'll probably submit to just do the same crc_hash on it that we do to filehandles, and if we still want to rip out the tracepoint later that's what we can do.

Ben

#7 lore raw

PATCH v5 2/3 NFSD/export: Add sign_fh export option

In reply to Benjamin Coddington

In order to signal that filehandles on this export should be signed, add a "sign_fh" export option. Filehandle signing can help the server defend against certain filehandle guessing attacks.

Setting the "sign_fh" export option sets NFSEXP_SIGN_FH. In a future patch NFSD uses this signal to append a MAC onto filehandles for that export.

While we're in here, tidy a few stray expflags to more closely align to the export flag order.

---
 fs/nfsd/export.c                 | 5 +++--
 include/uapi/linux/nfsd/export.h | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)
fs/nfsd/export.c +3 -2
index 2a1499f2ad19..19c7a91c5373 100644
@@ -1349,13 +1349,14 @@ static struct flags {
 	{ NFSEXP_ASYNC, {"async", "sync"}},
 	{ NFSEXP_GATHERED_WRITES, {"wdelay", "no_wdelay"}},
 	{ NFSEXP_NOREADDIRPLUS, {"nordirplus", ""}},
+	{ NFSEXP_SECURITY_LABEL, {"security_label", ""}},
+	{ NFSEXP_SIGN_FH, {"sign_fh", ""}},
 	{ NFSEXP_NOHIDE, {"nohide", ""}},
-	{ NFSEXP_CROSSMOUNT, {"crossmnt", ""}},
 	{ NFSEXP_NOSUBTREECHECK, {"no_subtree_check", ""}},
 	{ NFSEXP_NOAUTHNLM, {"insecure_locks", ""}},
+	{ NFSEXP_CROSSMOUNT, {"crossmnt", ""}},
 	{ NFSEXP_V4ROOT, {"v4root", ""}},
 	{ NFSEXP_PNFS, {"pnfs", ""}},
-	{ NFSEXP_SECURITY_LABEL, {"security_label", ""}},
 	{ 0, {"", ""}}
 };
 
include/uapi/linux/nfsd/export.h +2 -2
index a73ca3703abb..de647cf166c3 100644
@@ -34,7 +34,7 @@
 #define NFSEXP_GATHERED_WRITES	0x0020
 #define NFSEXP_NOREADDIRPLUS    0x0040
 #define NFSEXP_SECURITY_LABEL	0x0080
-/* 0x100 currently unused */
+#define NFSEXP_SIGN_FH		0x0100
 #define NFSEXP_NOHIDE		0x0200
 #define NFSEXP_NOSUBTREECHECK	0x0400
 #define	NFSEXP_NOAUTHNLM	0x0800		/* Don't authenticate NLM requests - just trust */
@@ -55,7 +55,7 @@
 #define NFSEXP_PNFS		0x20000
 
 /* All flags that we claim to support.  (Note we don't support NOACL.) */
-#define NFSEXP_ALLFLAGS		0x3FEFF
+#define NFSEXP_ALLFLAGS		0x3FFFF
 
 /* The flags that may vary depending on security flavor: */
 #define NFSEXP_SECINFO_FLAGS	(NFSEXP_READONLY | NFSEXP_ROOTSQUASH \
Signature · 1 line
2.50.1
#8 lore raw

PATCH v5 3/3 NFSD: Sign filehandles

In reply to Benjamin Coddington

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. 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.

---
 Documentation/filesystems/nfs/exporting.rst | 85 +++++++++++++++++++++
 fs/nfsd/nfsfh.c                             | 70 ++++++++++++++++-
 fs/nfsd/trace.h                             |  1 +
 3 files changed, 152 insertions(+), 4 deletions(-)
Documentation/filesystems/nfs/exporting.rst +85 -0
index de64d2d002a2..54343f4cc4fd 100644
@@ -238,3 +238,88 @@ following flags are defined:
     all of an inode's dirty data on last close. Exports that behave this
     way should set EXPORT_OP_FLUSH_ON_CLOSE so that NFSD knows to skip
     waiting for writeback when closing such files.
+
+Signed Filehandles
+------------------
+
+To protect against filehandle guessing attacks, the Linux NFS server can be
+configured to sign filehandles with a Message Authentication Code (MAC).
+
+Standard NFS filehandles are often predictable. If an attacker can guess
+a valid filehandle for a file they do not have permission to access via
+directory traversal, they may be able to bypass path-based permissions
+(though they still remain subject to inode-level permissions).
+
+Signed filehandles prevent this by appending a MAC to the filehandle
+before it is sent to the client. Upon receiving a filehandle back from a
+client, the server re-calculates the MAC using its internal key and
+verifies it against the one provided. If the signatures do not match,
+the server treats the filehandle as invalid (returning NFS[34]ERR_STALE).
+
+Note that signing filehandles provides integrity and authenticity but
+not confidentiality. The contents of the filehandle remain visible to
+the client; they simply cannot be forged or modified.
+
+Configuration
+~~~~~~~~~~~~~
+
+To enable signed filehandles, the administrator must provide a signing
+key to the kernel and enable the "sign_fh" export option.
+
+1. Providing a Key
+   The signing key is managed via the nfsd netlink interface. This key
+   is per-network-namespace and must be set before any exports using
+   "sign_fh" become active.
+
+2. Export Options
+   The feature is controlled on a per-export basis in /etc/exports:
+
+   sign_fh
+     Enables signing for all filehandles generated under this export.
+
+   no_sign_fh
+     (Default) Disables signing.
+
+Key Management and Rotation
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The security of this mechanism relies entirely on the secrecy of the
+signing key.
+
+Initial Setup:
+  The key should be generated using a high-quality random source and
+  loaded early in the boot process or during the nfs-server startup
+  sequence.
+
+Changing Keys:
+  If a key is changed while clients have active mounts, existing
+  filehandles held by those clients will become invalid, resulting in
+  "Stale file handle" errors on the client side.
+
+Safe Rotation:
+  Currently, there is no mechanism for "graceful" key rotation
+  (maintaining multiple valid keys). Changing the key is an atomic
+  operation that immediately invalidates all previous signatures.
+
+Transitioning Exports
+~~~~~~~~~~~~~~~~~~~~~
+
+When adding or removing the "sign_fh" flag from an active export, the
+following behaviors should be expected:
+
++-------------------+---------------------------------------------------+
+| Change            | Result for Existing Clients                       |
++===================+===================================================+
+| Adding sign_fh    | Clients holding unsigned filehandles will find    |
+|                   | them rejected, as the server now expects a        |
+|                   | signature.                                        |
++-------------------+---------------------------------------------------+
+| Removing sign_fh  | Clients holding signed filehandles will find them |
+|                   | rejected, as the server now expects the           |
+|                   | filehandle to end at its traditional boundary     |
+|                   | without a MAC.                                    |
++-------------------+---------------------------------------------------+
+
+Because filehandles are often cached persistently by clients, adding or
+removing this option should generally be done during a scheduled maintenance
+window involving a NFS client unmount/remount.
fs/nfsd/nfsfh.c +66 -4

103 lines not on this page · raw message

fs/nfsd/trace.h +1 -0
index c1a5f2fa44ab..8f0917b1b55d 100644
@@ -373,6 +373,7 @@ DEFINE_EVENT_CONDITION(nfsd_fh_err_class, nfsd_##name,	\
 
 DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badexport);
 DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badhandle);
+DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badmac);
 
 TRACE_EVENT(nfsd_exp_find_key,
 	TP_PROTO(const struct svc_expkey *key,
Signature · 1 line
2.50.1
#9 lore raw

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

Quoted 9 lines · Benjamin Coddington

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.

It's been pointed out to me that siphash is a PRF designed for hash tables, not a standard MAC. We suggested siphash as it may be sufficient here for preventing 8-byte tag guessing, but the commit message and documentation calls it a "MAC" which is a misnomer. Can the commit message (or even the new .rst file) document why siphash is adequate for this threat model?

Perhaps Eric has some thoughts on this.

Quoted 6 lines

Append the 8 byte siphash to encoded filehandles for exports that have set the "sign_fh" export option. 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.

Should this be "with NFS error _STALE." ?

Quoted 156 lines
---
 Documentation/filesystems/nfs/exporting.rst | 85 +++++++++++++++++++++
 fs/nfsd/nfsfh.c                             | 70 ++++++++++++++++-
 fs/nfsd/trace.h                             |  1 +
 3 files changed, 152 insertions(+), 4 deletions(-)
Documentation/filesystems/nfs/exporting.rst +85 -0
index de64d2d002a2..54343f4cc4fd 100644
@@ -238,3 +238,88 @@ following flags are defined:
     all of an inode's dirty data on last close. Exports that behave this
     way should set EXPORT_OP_FLUSH_ON_CLOSE so that NFSD knows to skip
     waiting for writeback when closing such files.
+
+Signed Filehandles
+------------------
+
+To protect against filehandle guessing attacks, the Linux NFS server can be
+configured to sign filehandles with a Message Authentication Code (MAC).
+
+Standard NFS filehandles are often predictable. If an attacker can guess
+a valid filehandle for a file they do not have permission to access via
+directory traversal, they may be able to bypass path-based permissions
+(though they still remain subject to inode-level permissions).
+
+Signed filehandles prevent this by appending a MAC to the filehandle
+before it is sent to the client. Upon receiving a filehandle back from a
+client, the server re-calculates the MAC using its internal key and
+verifies it against the one provided. If the signatures do not match,
+the server treats the filehandle as invalid (returning NFS[34]ERR_STALE).
+
+Note that signing filehandles provides integrity and authenticity but
+not confidentiality. The contents of the filehandle remain visible to
+the client; they simply cannot be forged or modified.
+
+Configuration
+~~~~~~~~~~~~~
+
+To enable signed filehandles, the administrator must provide a signing
+key to the kernel and enable the "sign_fh" export option.
+
+1. Providing a Key
+   The signing key is managed via the nfsd netlink interface. This key
+   is per-network-namespace and must be set before any exports using
+   "sign_fh" become active.
+
+2. Export Options
+   The feature is controlled on a per-export basis in /etc/exports:
+
+   sign_fh
+     Enables signing for all filehandles generated under this export.
+
+   no_sign_fh
+     (Default) Disables signing.
+
+Key Management and Rotation
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The security of this mechanism relies entirely on the secrecy of the
+signing key.
+
+Initial Setup:
+  The key should be generated using a high-quality random source and
+  loaded early in the boot process or during the nfs-server startup
+  sequence.
+
+Changing Keys:
+  If a key is changed while clients have active mounts, existing
+  filehandles held by those clients will become invalid, resulting in
+  "Stale file handle" errors on the client side.
+
+Safe Rotation:
+  Currently, there is no mechanism for "graceful" key rotation
+  (maintaining multiple valid keys). Changing the key is an atomic
+  operation that immediately invalidates all previous signatures.
+
+Transitioning Exports
+~~~~~~~~~~~~~~~~~~~~~
+
+When adding or removing the "sign_fh" flag from an active export, the
+following behaviors should be expected:
+
++-------------------+---------------------------------------------------+
+| Change            | Result for Existing Clients                       |
++===================+===================================================+
+| Adding sign_fh    | Clients holding unsigned filehandles will find    |
+|                   | them rejected, as the server now expects a        |
+|                   | signature.                                        |
++-------------------+---------------------------------------------------+
+| Removing sign_fh  | Clients holding signed filehandles will find them |
+|                   | rejected, as the server now expects the           |
+|                   | filehandle to end at its traditional boundary     |
+|                   | without a MAC.                                    |
++-------------------+---------------------------------------------------+
+
+Because filehandles are often cached persistently by clients, adding or
+removing this option should generally be done during a scheduled maintenance
+window involving a NFS client unmount/remount.
fs/nfsd/nfsfh.c +40 -0
index 68b629fbaaeb..3bab2ad0b21f 100644
@@ -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"
@@ -140,6 +141,57 @@ 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;
+	__le64 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;
+	}
+
+	hash = cpu_to_le64(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 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;

fs/nfsd/nfsfh.c static inline __be32 check_pseudo_root(struct dentry *dentry, · line 192

Quoted 10 lines
fs/nfsd/nfsfh.c +10 -0
+	siphash_key_t *fh_key = nn->fh_key;
+	__le64 hash;
+
+	if (!fh_key) {
+		pr_warn_ratelimited("NFSD: unable to verify signed filehandles, fh_key not set.\n");
+		return -EINVAL;
+	}
+
+	hash = cpu_to_le64(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));

Nit: fh_verify_mac() here returns positive-on-error (crypto_memneq convention) while fh_append_mac() returns negative-on-error (errno convention). Would it be better if both returned bool?

Quoted 6 lines
fs/nfsd/nfsfh.c +2 -0
+}
+
 /*
  * Use the given filehandle to look up the corresponding export and
  * dentry.  On success, the results are used to set fh_export and

fs/nfsd/nfsfh.c static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, · line 294

Quoted 10 lines
fs/nfsd/nfsfh.c +1 -3
@@ -236,13 +288,18 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net,
 	/*
 	 * Look up the dentry using the NFS file handle.
 	 */
-	error = nfserr_badhandle;
-
 	fileid_type = fh->fh_fileid_type;
 
-	if (fileid_type == FILEID_ROOT)
+	if (fileid_type == FILEID_ROOT) {
 		dentry = dget(exp->ex_path.dentry);

The control flow silently skips MAC verification for FILEID_ROOT filehandles. The rationale (export root contains no per-file identity to protect) exists nowhere in the code. A maintainer investigating security boundaries must deduce this from branching logic. A comment explaining the exemption would be helpful.

fs/nfsd/nfsfh.c static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, · line 297

Quoted 4 lines
fs/nfsd/nfsfh.c +3 -1
-	else {
+	} else {
+		if (exp->ex_flags & NFSEXP_SIGN_FH && fh_verify_mac(fhp, net)) {
+			trace_nfsd_set_fh_dentry_badmac(rqstp, fhp, -EKEYREJECTED);

-ESTALE might be more consistent with the documentation. Any opinion about that?

fs/nfsd/nfsfh.c static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, · line 302

Quoted 5 lines
fs/nfsd/nfsfh.c +5 -0
+			goto out;
+		} else {
+			data_left -= sizeof(u64)/4;
+		}
+

When NFSEXP_SIGN_FH is not set, the condition short-circuits to false and the else branch executes, subtracting 2 from data_left even though no MAC was appended to the filehandle.

For a typical FILEID_INO32_GEN filehandle on a non-signed export, data_left at this point is 2 (the fileid portion). After the subtraction it becomes 0, which is then passed to exportfs_decode_fh_raw(). In generic_fh_to_dentry():

    if (fh_len < 2)
        return NULL;

This returns NULL, and nfsd_set_fh_dentry() treats that as nfserr_badhandle/stale. Does the data_left calculation break all non- root filehandle lookups on exports without sign_fh?

Also, let's replace sizeof(u64)/4 with a symbolic constant to survive MAC size changes and better document the computation.

Quoted 34 lines
fs/nfsd/nfsfh.c +5 -0
 		dentry = exportfs_decode_fh_raw(exp->ex_path.mnt, fid,
 						data_left, fileid_type, 0,
 						nfsd_acceptable, exp);
@@ -258,6 +315,8 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net,
 			}
 		}
 	}
+
+	error = nfserr_badhandle;
 	if (dentry == NULL)
 		goto out;
 	if (IS_ERR(dentry)) {
@@ -498,6 +557,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;
 	}
fs/nfsd/trace.h +1 -0
index c1a5f2fa44ab..8f0917b1b55d 100644
@@ -373,6 +373,7 @@ DEFINE_EVENT_CONDITION(nfsd_fh_err_class, nfsd_##name,	\
 
 DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badexport);
 DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badhandle);
+DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badmac);
 
 TRACE_EVENT(nfsd_exp_find_key,
 	TP_PROTO(const struct svc_expkey *key,
Signature · 1 line
Chuck Lever
#10 lore raw

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

Quoted 11 lines · Chuck Lever
Quoted 9 lines · Benjamin Coddington

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.

Quoted 8 lines

It's been pointed out to me that siphash is a PRF designed for hash tables, not a standard MAC. We suggested siphash as it may be sufficient here for preventing 8-byte tag guessing, but the commit message and documentation calls it a "MAC" which is a misnomer. Can the commit message (or even the new .rst file) document why siphash is adequate for this threat model?

Perhaps Eric has some thoughts on this.

PRFs are also MACs, though. So SipHash is also a MAC. See the original
paper: https://cr.yp.to/siphash/siphash-20120918.pdf

However, SipHash's tag size is only 64 bits, which limits its resistance to forgeries. There will always be at least a 1 in 2^64 chance of a forgery.

In addition, the specific variant of SipHash implemented by the kernel's siphash library is SipHash-2-4. That's the performance-optimized variant. While no attack is known on that variant, and the SipHash paper claims that even this variant is a cryptographically strong PRF and thus also a MAC, SipHash-4-8 is the more conservative variant.

If you'd like to be more conservative with the cryptographic primitive and also bring the forgery chance down to 1 in 1^128, HMAC-SHA256 or BLAKE2s with 128-bit tags could be a good choice.

(In commit 2f3dd6ec901f29aef5fff3d7a63b1371d67c1760, I used HMAC-SHA256 with 256-bit tags for SCTP cookies. Probably overkill, but the struct already had 256 bits reserved for the tag.)

But again, SipHash (even SipHash-2-4) is indeed considered to be a MAC. So if the only concern is that it's "a PRF but not a MAC", that's not correct.

- Eric

#11 lore raw

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

Quoted 19 lines · Eric Biggers
Quoted 18 lines · Chuck Lever
Quoted 9 lines · Benjamin Coddington

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.

It's been pointed out to me that siphash is a PRF designed for hash tables, not a standard MAC. We suggested siphash as it may be sufficient here for preventing 8-byte tag guessing, but the commit message and documentation calls it a "MAC" which is a misnomer. Can the commit message (or even the new .rst file) document why siphash is adequate for this threat model?

Quoted 3 lines
Quoted 1 line

Perhaps Eric has some thoughts on this.

PRFs are also MACs, though.

In our case, the hash authenticates the file handle because only our NFS server knows the hash key. Fair enough.

Quoted 13 lines

So SipHash is also a MAC. See the original
paper: https://cr.yp.to/siphash/siphash-20120918.pdf

However, SipHash's tag size is only 64 bits, which limits its resistance to forgeries. There will always be at least a 1 in 2^64 chance of a forgery.

In addition, the specific variant of SipHash implemented by the kernel's siphash library is SipHash-2-4. That's the performance-optimized variant. While no attack is known on that variant, and the SipHash paper claims that even this variant is a cryptographically strong PRF and thus also a MAC, SipHash-4-8 is the more conservative variant.

Quoted 3 lines

If you'd like to be more conservative with the cryptographic primitive and also bring the forgery chance down to 1 in 1^128, HMAC-SHA256 or BLAKE2s with 128-bit tags could be a good choice.

We're looking for good performance because file handle verification will be done frequently, and also I imagine that we have a hash size budget in certain cases. I am relying on others to do the math regarding how much forgery protection is needed.

Since there is currently no mechanism for selecting a different hash for signing file handles, perhaps we should also consider the expected longevity of the protection offered by SipHash-2-4 versus file handle lifetime.

Quoted 7 lines

(In commit 2f3dd6ec901f29aef5fff3d7a63b1371d67c1760, I used HMAC-SHA256 with 256-bit tags for SCTP cookies. Probably overkill, but the struct already had 256 bits reserved for the tag.)

But again, SipHash (even SipHash-2-4) is indeed considered to be a MAC. So if the only concern is that it's "a PRF but not a MAC", that's not correct.

Then the rationale to be added might be nothing more than "According to
https://cr.yp.to/siphash/siphash-20120918.pdf SipHash can be used as a MAC, and our use of SipHash-2-4 provides a low 1 in 2^64 chance of forgery."

Signature · 1 line
Chuck Lever
#12 lore raw

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

In reply to Chuck Lever
Quoted 11 lines · Chuck Lever
Quoted 9 lines · Benjamin Coddington

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.

Quoted 8 lines

It's been pointed out to me that siphash is a PRF designed for hash tables, not a standard MAC. We suggested siphash as it may be sufficient here for preventing 8-byte tag guessing, but the commit message and documentation calls it a "MAC" which is a misnomer. Can the commit message (or even the new .rst file) document why siphash is adequate for this threat model?

Perhaps Eric has some thoughts on this.

I'll follow the guidance already posted from his response.

Quoted 8 lines
Quoted 6 lines

Append the 8 byte siphash to encoded filehandles for exports that have set the "sign_fh" export option. 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.

Should this be "with NFS error _STALE." ?

It should.

Quoted 162 lines
Quoted 162 lines
---
 Documentation/filesystems/nfs/exporting.rst | 85 +++++++++++++++++++++
 fs/nfsd/nfsfh.c                             | 70 ++++++++++++++++-
 fs/nfsd/trace.h                             |  1 +
 3 files changed, 152 insertions(+), 4 deletions(-)
Documentation/filesystems/nfs/exporting.rst +85 -0
index de64d2d002a2..54343f4cc4fd 100644
@@ -238,3 +238,88 @@ following flags are defined:
     all of an inode's dirty data on last close. Exports that behave this
     way should set EXPORT_OP_FLUSH_ON_CLOSE so that NFSD knows to skip
     waiting for writeback when closing such files.
+
+Signed Filehandles
+------------------
+
+To protect against filehandle guessing attacks, the Linux NFS server can be
+configured to sign filehandles with a Message Authentication Code (MAC).
+
+Standard NFS filehandles are often predictable. If an attacker can guess
+a valid filehandle for a file they do not have permission to access via
+directory traversal, they may be able to bypass path-based permissions
+(though they still remain subject to inode-level permissions).
+
+Signed filehandles prevent this by appending a MAC to the filehandle
+before it is sent to the client. Upon receiving a filehandle back from a
+client, the server re-calculates the MAC using its internal key and
+verifies it against the one provided. If the signatures do not match,
+the server treats the filehandle as invalid (returning NFS[34]ERR_STALE).
+
+Note that signing filehandles provides integrity and authenticity but
+not confidentiality. The contents of the filehandle remain visible to
+the client; they simply cannot be forged or modified.
+
+Configuration
+~~~~~~~~~~~~~
+
+To enable signed filehandles, the administrator must provide a signing
+key to the kernel and enable the "sign_fh" export option.
+
+1. Providing a Key
+   The signing key is managed via the nfsd netlink interface. This key
+   is per-network-namespace and must be set before any exports using
+   "sign_fh" become active.
+
+2. Export Options
+   The feature is controlled on a per-export basis in /etc/exports:
+
+   sign_fh
+     Enables signing for all filehandles generated under this export.
+
+   no_sign_fh
+     (Default) Disables signing.
+
+Key Management and Rotation
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The security of this mechanism relies entirely on the secrecy of the
+signing key.
+
+Initial Setup:
+  The key should be generated using a high-quality random source and
+  loaded early in the boot process or during the nfs-server startup
+  sequence.
+
+Changing Keys:
+  If a key is changed while clients have active mounts, existing
+  filehandles held by those clients will become invalid, resulting in
+  "Stale file handle" errors on the client side.
+
+Safe Rotation:
+  Currently, there is no mechanism for "graceful" key rotation
+  (maintaining multiple valid keys). Changing the key is an atomic
+  operation that immediately invalidates all previous signatures.
+
+Transitioning Exports
+~~~~~~~~~~~~~~~~~~~~~
+
+When adding or removing the "sign_fh" flag from an active export, the
+following behaviors should be expected:
+
++-------------------+---------------------------------------------------+
+| Change            | Result for Existing Clients                       |
++===================+===================================================+
+| Adding sign_fh    | Clients holding unsigned filehandles will find    |
+|                   | them rejected, as the server now expects a        |
+|                   | signature.                                        |
++-------------------+---------------------------------------------------+
+| Removing sign_fh  | Clients holding signed filehandles will find them |
+|                   | rejected, as the server now expects the           |
+|                   | filehandle to end at its traditional boundary     |
+|                   | without a MAC.                                    |
++-------------------+---------------------------------------------------+
+
+Because filehandles are often cached persistently by clients, adding or
+removing this option should generally be done during a scheduled maintenance
+window involving a NFS client unmount/remount.
fs/nfsd/nfsfh.c +46 -0
index 68b629fbaaeb..3bab2ad0b21f 100644
@@ -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"
@@ -140,6 +141,57 @@ 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;
+	__le64 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;
+	}
+
+	hash = cpu_to_le64(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 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;
+	__le64 hash;
+
+	if (!fh_key) {
+		pr_warn_ratelimited("NFSD: unable to verify signed filehandles, fh_key not set.\n");
+		return -EINVAL;
Quoted 8 lines

fs/nfsd/nfsfh.c static inline __be32 check_pseudo_root(struct dentry *dentry, · line 192

Quoted 4 lines
fs/nfsd/nfsfh.c +4 -0
+	}
+
+	hash = cpu_to_le64(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));

Nit: fh_verify_mac() here returns positive-on-error (crypto_memneq convention) while fh_append_mac() returns negative-on-error (errno convention). Would it be better if both returned bool?

I'd be inclined to change the function names if they both want to return a bool thing, so that the functions assert a fact that can be true or false, rather than explain the operation.

If you want them to both return a bool, then maybe prefix them with did_{fh_..}?

For me, the semantics make sense as they are..

Quoted 13 lines
Quoted 13 lines
fs/nfsd/nfsfh.c +2 -2
+}
+
 /*
  * Use the given filehandle to look up the corresponding export and
  * dentry.  On success, the results are used to set fh_export and
@@ -236,13 +288,18 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net,
 	/*
 	 * Look up the dentry using the NFS file handle.
 	 */
-	error = nfserr_badhandle;
-
 	fileid_type = fh->fh_fileid_type;
 
Quoted 9 lines

fs/nfsd/nfsfh.c static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, · line 294

Quoted 3 lines
fs/nfsd/nfsfh.c +1 -1
-	if (fileid_type == FILEID_ROOT)
+	if (fileid_type == FILEID_ROOT) {
 		dentry = dget(exp->ex_path.dentry);

The control flow silently skips MAC verification for FILEID_ROOT filehandles. The rationale (export root contains no per-file identity to protect) exists nowhere in the code. A maintainer investigating security boundaries must deduce this from branching logic. A comment explaining the exemption would be helpful.

Comment - roger.

Quoted 7 lines

fs/nfsd/nfsfh.c static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, · line 297

Quoted 4 lines
fs/nfsd/nfsfh.c +3 -1
-	else {
+	} else {
+		if (exp->ex_flags & NFSEXP_SIGN_FH && fh_verify_mac(fhp, net)) {
+			trace_nfsd_set_fh_dentry_badmac(rqstp, fhp, -EKEYREJECTED);

-ESTALE might be more consistent with the documentation. Any opinion about that?

Sure - after changing the tracepoint name from v4, it still shows where the problem happened.

Quoted 15 lines

fs/nfsd/nfsfh.c static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, · line 302

Quoted 5 lines
fs/nfsd/nfsfh.c +5 -0
+			goto out;
+		} else {
+			data_left -= sizeof(u64)/4;
+		}
+

When NFSEXP_SIGN_FH is not set, the condition short-circuits to false and the else branch executes, subtracting 2 from data_left even though no MAC was appended to the filehandle.

For a typical FILEID_INO32_GEN filehandle on a non-signed export, data_left at this point is 2 (the fileid portion). After the subtraction it becomes 0, which is then passed to exportfs_decode_fh_raw(). In generic_fh_to_dentry():

Quoted 6 lines
    if (fh_len < 2)
        return NULL;

This returns NULL, and nfsd_set_fh_dentry() treats that as nfserr_badhandle/stale. Does the data_left calculation break all non- root filehandle lookups on exports without sign_fh?

Oh yeah, I messed this one up bigtime, obvious dumb error - and then of course only tested with "sign_fh" on the version bump. Good catch.

Quoted 2 lines

Also, let's replace sizeof(u64)/4 with a symbolic constant to survive MAC size changes and better document the computation.

Sure - can just define it above the function..

Thanks for all the feedback and catching my brainos.

Ben

maintainer
#13 lore raw

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

Quoted 73 lines · Benjamin Coddington
Quoted 63 lines · Chuck Lever
Quoted 61 lines · Benjamin Coddington
fs/nfsd/nfsfh.c +46 -0
index 68b629fbaaeb..3bab2ad0b21f 100644
@@ -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"
@@ -140,6 +141,57 @@ 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;
+	__le64 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;
+	}
+
+	hash = cpu_to_le64(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 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;
+	__le64 hash;
+
+	if (!fh_key) {
+		pr_warn_ratelimited("NFSD: unable to verify signed filehandles, fh_key not set.\n");
+		return -EINVAL;
Quoted 8 lines

fs/nfsd/nfsfh.c static inline __be32 check_pseudo_root(struct dentry *dentry, · line 192

Quoted 4 lines
fs/nfsd/nfsfh.c +4 -0
+	}
+
+	hash = cpu_to_le64(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));

Nit: fh_verify_mac() here returns positive-on-error (crypto_memneq convention) while fh_append_mac() returns negative-on-error (errno convention). Would it be better if both returned bool?

Quoted 8 lines

I'd be inclined to change the function names if they both want to return a bool thing, so that the functions assert a fact that can be true or false, rather than explain the operation.

If you want them to both return a bool, then maybe prefix them with did_{fh_..}?

For me, the semantics make sense as they are..

The requested change is more about consistency with the surrounding source code so that maintainers have a better chance of not screwing this up when fixing a bug.

The current function names are consistent with existing code and the "positive return code" return value is not.

Signature · 1 line
Chuck Lever