[PATCH] NFS: idmap: fix race between key revocation and payload access

[PATCH] NFS: idmap: fix race between key revocation and payload access

[mdshahid03]

From: Mohammad Shahid <mdshahid03@gmail.com>

NFS idmapping retrieves the user key payload after validating the
key, but does not hold key semaphore while accessing the
payload.

The key revocation path acquires the key semaphore for writing
and clears the payload pointer through user_revoke().

Without holding the semaphore ,revocation can race with payload
access.In this case,user_key_payload_rcu can return NULL.

The existing IS_ERR_OR_NULL() check passes this NULL pointer to
PTR_ERR(),which returns zero and incoorectly reports scuccess.

Protect key validation and payload access with key semaphore
and use user_key_payload_locked() while holding the read lock.
Handle a NULL payload explicitly as revoked key.

This prevents concurrent revocation from changing the payload
while NFS is accessing it.

This issue is reported by smatch as possible NULL argunments to
PTR_ERR().

Signed-off-by: Mohammad Shahid <mdshahid03@gmail.com>
---
 fs/nfs/nfs4idmap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/nfs4idmap.c b/fs/nfs/nfs4idmap.c
index bc397110d977..06b193722660 100644
--- a/fs/nfs/nfs4idmap.c
+++ b/fs/nfs/nfs4idmap.c
@@ -317,16 +317,16 @@ static ssize_t nfs_idmap_get_key(const char *name, size_t namelen,
 		goto out;
 	}
 
-	rcu_read_lock();
 	rkey->perm |= KEY_USR_VIEW;
 
+	down_read(&rkey->sem);
 	ret = key_validate(rkey);
 	if (ret < 0)
 		goto out_up;
 
-	payload = user_key_payload_rcu(rkey);
-	if (IS_ERR_OR_NULL(payload)) {
-		ret = PTR_ERR(payload);
+	payload = user_key_payload_locked(rkey);
+	if (!payload) {
+		ret = -EKEYREVOKED;
 		goto out_up;
 	}
 
@@ -337,7 +337,7 @@ static ssize_t nfs_idmap_get_key(const char *name, size_t namelen,
 		ret = -EINVAL;
 
 out_up:
-	rcu_read_unlock();
+	up_read(&rkey->sem);
 	key_put(rkey);
 out:
 	return ret;
-- 
2.43.0

Re: [PATCH] NFS: idmap: fix race between key revocation and payload access

[Trond Myklebust]

On Thu, 2026-11-05 at 23:02 +0530, mdshahid03@gmail.com wrote:
> From: Mohammad Shahid <mdshahid03@gmail.com>
> 
> NFS idmapping retrieves the user key payload after validating the
> key, but does not hold key semaphore while accessing the
> payload.
> 
> The key revocation path acquires the key semaphore for writing
> and clears the payload pointer through user_revoke().
> 
> Without holding the semaphore ,revocation can race with payload
> access.In this case,user_key_payload_rcu can return NULL.
> 
> The existing IS_ERR_OR_NULL() check passes this NULL pointer to
> PTR_ERR(),which returns zero and incoorectly reports scuccess.
> 

Let's just fix the real problem, which is that the callers expect
nfs_idmap_get_key() to return either a non-zero length or an error
value.
Right now, it can return a zero length, which is interpreted correctly
by nfs_idmap_lookup_id(), but incorrectly by nfs_idmap_lookup_name().

The semaphore is unnecessary.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trondmy@kernel.org, trond.myklebust@hammerspace.com