[PATCH v2] NFSv4.2: fix nfs4_listxattr size accounting

Achilles Gaikwad <[email protected]>
Newsgroups gmane.linux.nfs
Message-ID <[email protected]>
A call to listxattr() with a buffer size of 0 returns the actual
size of the buffer needed for a subsequent call. On an NFSv4.2
mount this triggers the following oops:

  [  399.768687] BUG: kernel NULL pointer dereference, address: 0000000000000000
  [  399.768705] RIP: 0010:_copy_from_pages+0x44/0xe0
  [  399.768722] Call Trace:
  [  399.768723]  nfs4_xattr_alloc_entry+0x1bf/0x1e0
  [  399.768730]  nfs4_xattr_cache_set_list+0x43/0x1f0
  [  399.768731]  nfs4_listxattr+0x21f/0x250
  [  399.768733]  vfs_listxattr+0x55/0xa0
  [  399.768736]  listxattr+0x23/0x160
  [  399.768737]  path_listxattrat+0xba/0x1e0
  [  399.768739]  do_syscall_64+0xe2/0x680

security_inode_listsecurity() (via the xattr_list_one() helper) now
decrements the remaining size even when the buffer pointer is NULL, so
in the size-query case, 'left' underflows to a huge size_t value. As a
result, nfs4_listxattr_nfs4_user() treats the NULL buffer as a real one,
leading to a NULL pointer dereference in _copy_from_pages().

security_inode_listsecurity() does not return the number of bytes
it added to the list, so the code derived it as
'size - error - left'. That is also wrong in the size-query case:
the generic_listxattr() contribution is only subtracted from 'left'
when a buffer is present. Thus, the query result comes up short by
exactly that contribution (e.g., "system.nfs4_acl" on a mount with
ACL support), and a caller that allocates the returned size gets
-ERANGE on the subsequent call.

Declare 'left' as ssize_t, use a scratch copy to measure security
hook consumption, and only decrement 'left' if a buffer is present.

Fixes: f71ece9712b7 ("security,fs,nfs,net: update security_inode_listsecurity() interface")
Suggested-by: Paul Moore <[email protected]>
Signed-off-by: Achilles Gaikwad <[email protected]>
---
Changes in v2:
 - Use a scratch variable to track security label size directly,
   replacing the old formula that undercounted the size-query case.
 - Drop the now-unneeded NULL-buffer special case for
   nfs4_listxattr_nfs4_user().
 - Retitled from "fix nfs4_listxattr NULL pointer dereference"
   (the same accounting bug caused both the oops and the undercount).
v1: https://lore.kernel.org/linux-nfs/20260703102759.9626-1-achillesgaikwad-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org/
 fs/nfs/nfs4proc.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 1360409d8de9..a3415082d610 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -10585,7 +10585,8 @@ const struct nfs4_minor_version_ops *nfs_v4_minor_ops[] = {
 static ssize_t nfs4_listxattr(struct dentry *dentry, char *list, size_t size)
 {
 	ssize_t error, error2, error3;
-	size_t left = size;
+	ssize_t left = size;
+	ssize_t left2;
 
 	error = generic_listxattr(dentry, list, left);
 	if (error < 0)
@@ -10595,10 +10596,13 @@ static ssize_t nfs4_listxattr(struct dentry *dentry, char *list, size_t size)
 		left -= error;
 	}
 
-	error2 = security_inode_listsecurity(d_inode(dentry), &list, &left);
+	left2 = left;
+	error2 = security_inode_listsecurity(d_inode(dentry), &list, &left2);
 	if (error2 < 0)
 		return error2;
-	error2 = size - error - left;
+	error2 = left - left2;
+	if (list)
+		left -= error2;
 
 	error3 = nfs4_listxattr_nfs4_user(d_inode(dentry), list, left);
 	if (error3 < 0)

base-commit: 6eb8711ece2ce27e52e327a5b7a628ed39b97f45
-- 
2.55.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.