Do not use UID and GID from EAs

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

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

On Tuesday 17 September 2024 16:19:08 Steve French wrote:
> On Tue, Sep 17, 2024 at 3:45 PM ronnie sahlberg
> <ronniesahlberg@gmail.com> wrote:
> >
> > On Wed, 18 Sept 2024 at 06:37, Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Tuesday 17 September 2024 13:31:22 Jeremy Allison wrote:
> > > > On Tue, Sep 17, 2024 at 10:29:21PM +0200, Pali Rohár wrote:
> > > > > On Tuesday 17 September 2024 13:23:40 Jeremy Allison wrote:
> > > > > > On Tue, Sep 17, 2024 at 10:06:00PM +0200, Pali Rohár wrote:
> > > > > > > And seems that SMB2_OP_QUERY_WSL_EA is useful not only for reparse
> > > > > > > points, but also for any regular file or directory as it can contain
> > > > > > > UNIX mode and UID/GID ownership.
> > > > > >
> > > > > > uid/gid should *never* be exposed over the wire for SMB.
> > > > > >
> > > > > > That way lies madness.
> > > > >
> > > > > Hello Jeremy, if I understood wsl_to_fattr() function correctly then it
> > > > > is already doing it, it fills uid/gid for stat() from data which were
> > > > > exposed over the wire for SMB. Could you check that function if it is
> > > > > truth?
> > > >
> > > > I'm sure the Windows implementation is doing it - however, any Linux
> > > > server implementations should not do this (IMHO).
> > > >
> > > > It will break all SID -> uid / gid mapping that servers must
> > > > carefully set up.
> > > >
> > > > On the wire - SIDs must be the only source of identity.
> > >
> > > Ok. But then I do not understand why Linux client parses and uses uid
> > > and gids which are sent over the wire. If you are saying that the SIDs
> > > must be the only source of truth then Linux client should rather ignore
> > > uid and gid values?
> >
> > What I think Jeremy is refering to is that mixing uids and sids in the
> > protocol itself is
> > a protocol design mistake.
> > Because this means that some PDUs in the protocol operate on SIDs but
> > others operate on
> > UID/GIDs and this means there is great risk of mistakes and have the
> > sid<->uid mapping return
> > different results depending on the actual PDU.
> >
> > Sometimes the sid<->uid mapping happens in the server, at other times
> > the mapping happens in the client
> > and it is very difficult to guarantee that the mapping is consistent
> > across PDUs in the protocol as well as across different clients.
> 
> Yes - agreed.
> 
> SIDs are globally unique and should always be used/sent over the wire
> (never send or use the local uid/gid which is not guaranteed to be
> unique).  Whether retrieving ownership information via
> the SMB ACL or via an SMB3.1.1 POSIX response, the SID is the correct
> thing to send/use in the protocol.  For cases where the client is not
> domain joined, the UID/GID can be encoded in the SID, for cases that
> are domain joined the Linux UIDs/GIDs can be mapped consistently via
> the SID.
> 
> -- 
> Thanks,
> 
> Steve

Hello Steve, based on the above discussion I'm proposing a change which
stops parsing UID and GID values stored in EAs on the SMB server for
SMB2 and SMB3 dialects. Change is in the attachment.

Steve, Ronnie, Jeremy and Paulo, could you review this change?

[-- Attachment #2: 0001-cifs-Do-not-use-WSL-EAs-LXUID-LXGID-and-LXMOD-for-ow.patch --]
[-- Type: text/x-diff, Size: 5211 bytes --]

From e72661de4214b135b5852b95be9ff6f66014df41 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pali=20Roh=C3=A1r?= <pali@kernel.org>
Date: Thu, 30 Jan 2025 22:43:31 +0100
Subject: [PATCH] cifs: Do not use WSL EAs $LXUID, $LXGID and $LXMOD for
 ownership/permissions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When retrieving stat information about reparse points, do not use WSL EAs
$LXUID, $LXGID and $LXMOD for deducing Linux UID, GID and permission bits
of MODE. The source of truth for ownership and permissions should be always
only the SMB Security Descriptor, hence the ownership should comes from the
SID and permissions from ACL.

WSL EA $LXMOD contains not only permission bits, but also the file type
information. WSL subsystem requires from special files that this EA is
present and its encoded file type matches the reparse point type.

So let the EA $LXMOD code there, but use it only for validation of file
type (S_DT bits) that it matches the file type from reparse point tag.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 fs/smb/client/reparse.c   |  7 +------
 fs/smb/client/smb2inode.c | 10 ++--------
 fs/smb/client/smb2pdu.h   | 20 ++++++++++----------
 3 files changed, 13 insertions(+), 24 deletions(-)

diff --git a/fs/smb/client/reparse.c b/fs/smb/client/reparse.c
index a1aa10a572c2..3660f7353258 100644
--- a/fs/smb/client/reparse.c
+++ b/fs/smb/client/reparse.c
@@ -1358,15 +1358,10 @@ static bool wsl_to_fattr(struct cifs_open_info_data *data,
 		nlen = ea->ea_name_length;
 		v = (void *)((u8 *)ea->ea_data + ea->ea_name_length + 1);
 
-		if (!strncmp(name, SMB2_WSL_XATTR_UID, nlen))
-			fattr->cf_uid = wsl_make_kuid(cifs_sb, v);
-		else if (!strncmp(name, SMB2_WSL_XATTR_GID, nlen))
-			fattr->cf_gid = wsl_make_kgid(cifs_sb, v);
-		else if (!strncmp(name, SMB2_WSL_XATTR_MODE, nlen)) {
+		if (!strncmp(name, SMB2_WSL_XATTR_MODE, nlen)) {
 			/* File type in reparse point tag and in xattr mode must match. */
 			if (S_DT(reparse_mode_type) != S_DT(le32_to_cpu(*(__le32 *)v)))
 				return false;
-			fattr->cf_mode = (umode_t)le32_to_cpu(*(__le32 *)v);
 			have_xattr_mode = true;
 		} else if (!strncmp(name, SMB2_WSL_XATTR_DEV, nlen)) {
 			fattr->cf_rdev = reparse_mkdev(v);
diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 2a0316c514e4..18e5376a63ab 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -94,8 +94,6 @@ struct wsl_query_ea {
 #define NEXT_OFF cpu_to_le32(sizeof(struct wsl_query_ea))
 
 static const struct wsl_query_ea wsl_query_eas[] = {
-	{ .next = NEXT_OFF, .name_len = SMB2_WSL_XATTR_NAME_LEN, .name = SMB2_WSL_XATTR_UID, },
-	{ .next = NEXT_OFF, .name_len = SMB2_WSL_XATTR_NAME_LEN, .name = SMB2_WSL_XATTR_GID, },
 	{ .next = NEXT_OFF, .name_len = SMB2_WSL_XATTR_NAME_LEN, .name = SMB2_WSL_XATTR_MODE, },
 	{ .next = 0,        .name_len = SMB2_WSL_XATTR_NAME_LEN, .name = SMB2_WSL_XATTR_DEV, },
 };
@@ -130,9 +128,7 @@ static int check_wsl_eas(struct kvec *rsp_iov)
 
 		switch (vlen) {
 		case 4:
-			if (strncmp(ea->ea_data, SMB2_WSL_XATTR_UID, nlen) &&
-			    strncmp(ea->ea_data, SMB2_WSL_XATTR_GID, nlen) &&
-			    strncmp(ea->ea_data, SMB2_WSL_XATTR_MODE, nlen))
+			if (strncmp(ea->ea_data, SMB2_WSL_XATTR_MODE, nlen))
 				return -EINVAL;
 			break;
 		case 8:
@@ -140,9 +136,7 @@ static int check_wsl_eas(struct kvec *rsp_iov)
 				return -EINVAL;
 			break;
 		case 0:
-			if (!strncmp(ea->ea_data, SMB2_WSL_XATTR_UID, nlen) ||
-			    !strncmp(ea->ea_data, SMB2_WSL_XATTR_GID, nlen) ||
-			    !strncmp(ea->ea_data, SMB2_WSL_XATTR_MODE, nlen) ||
+			if (!strncmp(ea->ea_data, SMB2_WSL_XATTR_MODE, nlen) ||
 			    !strncmp(ea->ea_data, SMB2_WSL_XATTR_DEV, nlen))
 				break;
 			fallthrough;
diff --git a/fs/smb/client/smb2pdu.h b/fs/smb/client/smb2pdu.h
index 3c09a58dfd07..914e2737d1a2 100644
--- a/fs/smb/client/smb2pdu.h
+++ b/fs/smb/client/smb2pdu.h
@@ -425,7 +425,6 @@ struct smb2_create_ea_ctx {
 #define SMB2_WSL_XATTR_MODE		"$LXMOD"
 #define SMB2_WSL_XATTR_DEV		"$LXDEV"
 #define SMB2_WSL_XATTR_NAME_LEN	6
-#define SMB2_WSL_NUM_XATTRS		4
 
 #define SMB2_WSL_XATTR_UID_SIZE	4
 #define SMB2_WSL_XATTR_GID_SIZE	4
@@ -433,16 +432,17 @@ struct smb2_create_ea_ctx {
 #define SMB2_WSL_XATTR_DEV_SIZE	8
 
 #define SMB2_WSL_MIN_QUERY_EA_RESP_SIZE \
-	(ALIGN((SMB2_WSL_NUM_XATTRS - 1) * \
-	       (SMB2_WSL_XATTR_NAME_LEN + 1 + \
-		sizeof(struct smb2_file_full_ea_info)), 4) + \
-	 SMB2_WSL_XATTR_NAME_LEN + 1 + sizeof(struct smb2_file_full_ea_info))
+	(ALIGN(sizeof(struct smb2_file_full_ea_info) + \
+	       SMB2_WSL_XATTR_NAME_LEN + 1, 4) + \
+	 sizeof(struct smb2_file_full_ea_info) + \
+	 SMB2_WSL_XATTR_NAME_LEN + 1)
 
 #define SMB2_WSL_MAX_QUERY_EA_RESP_SIZE \
-	(ALIGN(SMB2_WSL_MIN_QUERY_EA_RESP_SIZE + \
-	       SMB2_WSL_XATTR_UID_SIZE + \
-	       SMB2_WSL_XATTR_GID_SIZE + \
-	       SMB2_WSL_XATTR_MODE_SIZE + \
-	       SMB2_WSL_XATTR_DEV_SIZE, 4))
+	(ALIGN(sizeof(struct smb2_file_full_ea_info) + \
+	       SMB2_WSL_XATTR_NAME_LEN + 1 + \
+	       SMB2_WSL_XATTR_MODE_SIZE, 4) + \
+	 sizeof(struct smb2_file_full_ea_info) + \
+	 SMB2_WSL_XATTR_NAME_LEN + 1 + \
+	 SMB2_WSL_XATTR_DEV_SIZE)
 
 #endif				/* _SMB2PDU_H */
-- 
2.20.1