Re: Regression with getcifsacl(1) in v6.14-rc1

["Pali Rohár" <pali@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]

On Wednesday 12 February 2025 19:19:00 Paulo Alcantara wrote:
> Pali Rohár <pali@kernel.org> writes:
> 
> > On Wednesday 12 February 2025 17:49:31 Paulo Alcantara wrote:
> >> Steve,
> >> 
> >> The commit 438e2116d7bd ("cifs: Change translation of
> >> STATUS_PRIVILEGE_NOT_HELD to -EPERM") regressed getcifsacl(1) because it
> >> expects -EIO to be returned from getxattr(2) when the client can't read
> >> system.cifs_ntsd_full attribute and then fall back to system.cifs_acl
> >> attribute.  Either -EIO or -EPERM is wrong for getxattr(2), but that's a
> >> different problem, though.
> >> 
> >> Reproduced against samba-4.22 server.
> >
> > That is bad. I can prepare a fix for cifs.ko getxattr syscall to
> > translate -EPERM to -EIO. This will ensure that getcifsacl will work as
> > before as it would still see -EIO error.
> 
> Sounds good.

Now I quickly prepared a fix, it is straightforward but I have not
tested it yet. Testing requires non-admin user which does not have
SeSecurityPrivilege privilege configured. Could you check if it is
fixing this problem?

[-- Attachment #2: 0001-cifs-Reports-EIO-for-getxattr-system.cifs_ntsd_full-.patch --]
[-- Type: text/x-diff, Size: 2534 bytes --]

From c9032ef054acd0a92ea641dc33a7b62a5833e00c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pali=20Roh=C3=A1r?= <pali@kernel.org>
Date: Thu, 13 Feb 2025 19:37:29 +0100
Subject: [PATCH] cifs: Reports -EIO for getxattr(system.cifs_ntsd_full)
 privilege error
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Commit 438e2116d7bd ("cifs: Change translation of STATUS_PRIVILEGE_NOT_HELD
to -EPERM") globally changed translation of STATUS_PRIVILEGE_NOT_HELD
status code from -EIO to -EPERM which is more appropriate errno code.

Unfortunately it broke getcifsacl utility when called by user which does
not have SeSecurityPrivilege privilege, which is required to fetch SACLs.

Userspace utility getcifsacl expects that kernel reports privilege error
for system.cifs_ntsd_full xattr as EIO errno, not as EPERM errno.

When privilege error via EIO errno is detected then getcifsacl request
security descriptor without SACLs via system.cifs_acl xattr. This is
allowed also without SeSecurityPrivilege privilege.

This change fixes the errno returned by getxattr(system.cifs_ntsd_full)
call, as required by backward compatibility for getcifsacl utility.
With this change EIO is returned as before the mentioned commit.

Fixes: 438e2116d7bd ("cifs: Change translation of STATUS_PRIVILEGE_NOT_HELD to -EPERM")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 fs/smb/client/xattr.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/smb/client/xattr.c b/fs/smb/client/xattr.c
index 58a584f0b27e..11207979c630 100644
--- a/fs/smb/client/xattr.c
+++ b/fs/smb/client/xattr.c
@@ -331,6 +331,20 @@ static int cifs_xattr_get(const struct xattr_handler *handler,
 			rc = PTR_ERR(pacl);
 			cifs_dbg(VFS, "%s: error %zd getting sec desc\n",
 				 __func__, rc);
+			if (rc == -EPERM && handler->flags == XATTR_CIFS_NTSD_FULL) {
+				/*
+				 * Report STATUS_PRIVILEGE_NOT_HELD error (signaled by -EPERM)
+				 * to userspace as EIO errno for system.cifs_ntsd_full xattr.
+				 * This is backward compatibility for old version of getcifsacl
+				 * utility which is doing fallback from system.cifs_ntsd_full xattr
+				 * to system.cifs_acl xattr when user does not have privilege to
+				 * fetch SACL and expects that kernel reports insufficient privilege
+				 * error via EIO errno (instead of EPERM errno).
+				 */
+				rc = -EIO;
+				cifs_dbg(FYI, "%s: error changed to %zd for compatibility\n",
+					 __func__, rc);
+			}
 		} else {
 			if (value) {
 				if (acllen > size)
-- 
2.20.1