NFSD: Report whether fh_key was actually updated

The nfsd_ctl_fh_key_set tracepoint was introduced to capture
operator activity on the filehandle signing key. Earlier revisions
logged the key bytes verbatim; the version that landed hashes the
16 key bytes through crc32_le and stores the result.

CRC32 is a linear projection of its input rather than a one-way
function, and truncating any hash of fixed-size secret material
leaves the key recoverable under offline brute force when the
threat model includes an attacker with access to the trace ring.

The operational question the fingerprint was meant to answer is
whether a NFSD_CMD_THREADS_SET call that carries an
NFSD_A_SERVER_FH_KEY attribute actually replaced the active key or
re-installed the value already in place. Answer that question
directly: compare the incoming key bytes against the current
nn->fh_key inside nfsd_nl_fh_key_set() and surface a single bit to
the tracepoint. The event now prints "updated" when the stored
key changed and "unmodified" otherwise. A first set that fails
kmalloc reports "unmodified" because no key was installed.

Reported-by: jaeyeong <fin@spl.team>
Fixes: 62346217fd72 ("NFSD: Add a key for signing filehandles")
Cc: Benjamin Coddington <bcodding@hammerspace.com>
Reviewed-by: Benjamin Coddington <bcodding@hammerspace.com>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 39e7012a60d8ebc4e86bcb2fed65051f2d3f357f..04e3954d54bd9f98da22635dec16fd7ea58bbea6 100644 (file)
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1594,16 +1594,27 @@ out_unlock:
 static int nfsd_nl_fh_key_set(const struct nlattr *attr, struct nfsd_net *nn)
 {
        siphash_key_t *fh_key = nn->fh_key;
+       u64 k0, k1;
+       bool changed;
+
+       k0 = get_unaligned_le64(nla_data(attr));
+       k1 = get_unaligned_le64(nla_data(attr) + 8);
 
        if (!fh_key) {
                fh_key = kmalloc(sizeof(siphash_key_t), GFP_KERNEL);
-               if (!fh_key)
+               if (!fh_key) {
+                       trace_nfsd_ctl_fh_key_set(false, -ENOMEM);
                        return -ENOMEM;
+               }
                nn->fh_key = fh_key;
+               changed = true;
+       } else {
+               changed = fh_key->key[0] != k0 || fh_key->key[1] != k1;
        }
 
-       fh_key->key[0] = get_unaligned_le64(nla_data(attr));
-       fh_key->key[1] = get_unaligned_le64(nla_data(attr) + 8);
+       fh_key->key[0] = k0;
+       fh_key->key[1] = k1;
+       trace_nfsd_ctl_fh_key_set(changed, 0);
        return 0;
 }
 
@@ -1682,7 +1693,6 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
                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;
                }

diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
index 5ad38f50836d725daa857054ee814a76c0ab1845..b631a472222be39fa273b786504540becb814159 100644 (file)
--- a/fs/nfsd/trace.h
+++ b/fs/nfsd/trace.h
@@ -2243,23 +2243,21 @@ TRACE_EVENT(nfsd_end_grace,
 
 TRACE_EVENT(nfsd_ctl_fh_key_set,
        TP_PROTO(
-               const char *key,
+               bool changed,
                int result
        ),
-       TP_ARGS(key, result),
+       TP_ARGS(changed, result),
        TP_STRUCT__entry(
-               __field(u32, key_hash)
+               __field(bool, changed)
                __field(int, result)
        ),
        TP_fast_assign(
-               if (key)
-                       __entry->key_hash = ~crc32_le(0xFFFFFFFF, key, 16);
-               else
-                       __entry->key_hash = 0;
+               __entry->changed = changed;
                __entry->result = result;
        ),
-       TP_printk("key=0x%08x result=%d",
-               __entry->key_hash, __entry->result
+       TP_printk("key %s, result=%d",
+               __entry->changed ? "updated" : "unmodified",
+               __entry->result
        )
 );