[PATCH] cifs: Fix getting reparse points from server without WSL support

[PATCH] cifs: Fix getting reparse points from server without WSL support

[Pali Rohár]

If SMB server does not support WSL EAs then for SMB2_OP_QUERY_WSL_EA
request it returns STATUS_EAS_NOT_SUPPORTED. Function smb2_compound_op()
translates STATUS_EAS_NOT_SUPPORTED to -EOPNOTSUPP which cause failure
during calling lstat() syscall from userspace on any reparse point,
including Native SMB symlink (which does not use any EAs). This issue
happen for example when trying to resolve Native NTFS symlink from SMB
server on Windows 8.

Avoid this problem by calling SMB2_OP_QUERY_WSL_EA only when detected
reparse point is of WSL type. Note that this is not solve this problem
fully as WSL reparse points can be created also on other SMB server
which do not support Extended Attributes at all.

Fixes: ea41367b2a60 ("smb: client: introduce SMB2_OP_QUERY_WSL_EA")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 fs/smb/client/smb2inode.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 11a1c53c64e0..d65471aa55e6 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -941,7 +941,20 @@ int smb2_query_path_info(const unsigned int xid,
 		if (rc || !data->reparse_point)
 			goto out;
 
-		cmds[num_cmds++] = SMB2_OP_QUERY_WSL_EA;
+		/*
+		 * Skip SMB2_OP_QUERY_WSL_EA if reparse point is not WSL.
+		 * The server file system does not have to support Extended
+		 * Attributes and in this case returns STATUS_EAS_NOT_SUPPORTED
+		 * which smb2_compound_op() translate to -EOPNOTSUPP. This will
+		 * present failure during reading of non-WSL special files.
+		 */
+		if (data->reparse.tag == IO_REPARSE_TAG_LX_SYMLINK ||
+		    data->reparse.tag == IO_REPARSE_TAG_AF_UNIX ||
+		    data->reparse.tag == IO_REPARSE_TAG_LX_FIFO ||
+		    data->reparse.tag == IO_REPARSE_TAG_LX_CHR ||
+		    data->reparse.tag == IO_REPARSE_TAG_LX_BLK)
+			cmds[num_cmds++] = SMB2_OP_QUERY_WSL_EA;
+
 		/*
 		 * Skip SMB2_OP_GET_REPARSE if symlink already parsed in create
 		 * response.
-- 
2.20.1

Re: [PATCH] cifs: Fix getting reparse points from server without WSL support

[Pali Rohár]

Paulo, please look at this patch as it is related to WSL attributes
which you introduced in the mentioned commit. I think that the proper
fix should be to change SMB2_OP_QUERY_WSL_EA code to not trigger that
-EOPNOTSUPP error which is delivered to userspace. I just checked that
this my patch works fine for Native NTFS symlinks and NFS-style reparse
point special files.

On Friday 13 September 2024 22:02:04 Pali Rohár wrote:
> If SMB server does not support WSL EAs then for SMB2_OP_QUERY_WSL_EA
> request it returns STATUS_EAS_NOT_SUPPORTED. Function smb2_compound_op()
> translates STATUS_EAS_NOT_SUPPORTED to -EOPNOTSUPP which cause failure
> during calling lstat() syscall from userspace on any reparse point,
> including Native SMB symlink (which does not use any EAs). This issue
> happen for example when trying to resolve Native NTFS symlink from SMB
> server on Windows 8.
> 
> Avoid this problem by calling SMB2_OP_QUERY_WSL_EA only when detected
> reparse point is of WSL type. Note that this is not solve this problem
> fully as WSL reparse points can be created also on other SMB server
> which do not support Extended Attributes at all.
> 
> Fixes: ea41367b2a60 ("smb: client: introduce SMB2_OP_QUERY_WSL_EA")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  fs/smb/client/smb2inode.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
> index 11a1c53c64e0..d65471aa55e6 100644
> --- a/fs/smb/client/smb2inode.c
> +++ b/fs/smb/client/smb2inode.c
> @@ -941,7 +941,20 @@ int smb2_query_path_info(const unsigned int xid,
>  		if (rc || !data->reparse_point)
>  			goto out;
>  
> -		cmds[num_cmds++] = SMB2_OP_QUERY_WSL_EA;
> +		/*
> +		 * Skip SMB2_OP_QUERY_WSL_EA if reparse point is not WSL.
> +		 * The server file system does not have to support Extended
> +		 * Attributes and in this case returns STATUS_EAS_NOT_SUPPORTED
> +		 * which smb2_compound_op() translate to -EOPNOTSUPP. This will
> +		 * present failure during reading of non-WSL special files.
> +		 */
> +		if (data->reparse.tag == IO_REPARSE_TAG_LX_SYMLINK ||
> +		    data->reparse.tag == IO_REPARSE_TAG_AF_UNIX ||
> +		    data->reparse.tag == IO_REPARSE_TAG_LX_FIFO ||
> +		    data->reparse.tag == IO_REPARSE_TAG_LX_CHR ||
> +		    data->reparse.tag == IO_REPARSE_TAG_LX_BLK)
> +			cmds[num_cmds++] = SMB2_OP_QUERY_WSL_EA;
> +
>  		/*
>  		 * Skip SMB2_OP_GET_REPARSE if symlink already parsed in create
>  		 * response.
> -- 
> 2.20.1
> 

Re: [PATCH] cifs: Fix getting reparse points from server without WSL support

[Pali Rohár]

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.

On Friday 13 September 2024 22:10:41 Pali Rohár wrote:
> Paulo, please look at this patch as it is related to WSL attributes
> which you introduced in the mentioned commit. I think that the proper
> fix should be to change SMB2_OP_QUERY_WSL_EA code to not trigger that
> -EOPNOTSUPP error which is delivered to userspace. I just checked that
> this my patch works fine for Native NTFS symlinks and NFS-style reparse
> point special files.
> 
> On Friday 13 September 2024 22:02:04 Pali Rohár wrote:
> > If SMB server does not support WSL EAs then for SMB2_OP_QUERY_WSL_EA
> > request it returns STATUS_EAS_NOT_SUPPORTED. Function smb2_compound_op()
> > translates STATUS_EAS_NOT_SUPPORTED to -EOPNOTSUPP which cause failure
> > during calling lstat() syscall from userspace on any reparse point,
> > including Native SMB symlink (which does not use any EAs). This issue
> > happen for example when trying to resolve Native NTFS symlink from SMB
> > server on Windows 8.
> > 
> > Avoid this problem by calling SMB2_OP_QUERY_WSL_EA only when detected
> > reparse point is of WSL type. Note that this is not solve this problem
> > fully as WSL reparse points can be created also on other SMB server
> > which do not support Extended Attributes at all.
> > 
> > Fixes: ea41367b2a60 ("smb: client: introduce SMB2_OP_QUERY_WSL_EA")
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >  fs/smb/client/smb2inode.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
> > index 11a1c53c64e0..d65471aa55e6 100644
> > --- a/fs/smb/client/smb2inode.c
> > +++ b/fs/smb/client/smb2inode.c
> > @@ -941,7 +941,20 @@ int smb2_query_path_info(const unsigned int xid,
> >  		if (rc || !data->reparse_point)
> >  			goto out;
> >  
> > -		cmds[num_cmds++] = SMB2_OP_QUERY_WSL_EA;
> > +		/*
> > +		 * Skip SMB2_OP_QUERY_WSL_EA if reparse point is not WSL.
> > +		 * The server file system does not have to support Extended
> > +		 * Attributes and in this case returns STATUS_EAS_NOT_SUPPORTED
> > +		 * which smb2_compound_op() translate to -EOPNOTSUPP. This will
> > +		 * present failure during reading of non-WSL special files.
> > +		 */
> > +		if (data->reparse.tag == IO_REPARSE_TAG_LX_SYMLINK ||
> > +		    data->reparse.tag == IO_REPARSE_TAG_AF_UNIX ||
> > +		    data->reparse.tag == IO_REPARSE_TAG_LX_FIFO ||
> > +		    data->reparse.tag == IO_REPARSE_TAG_LX_CHR ||
> > +		    data->reparse.tag == IO_REPARSE_TAG_LX_BLK)
> > +			cmds[num_cmds++] = SMB2_OP_QUERY_WSL_EA;
> > +
> >  		/*
> >  		 * Skip SMB2_OP_GET_REPARSE if symlink already parsed in create
> >  		 * response.
> > -- 
> > 2.20.1
> > 

Re: [PATCH] cifs: Fix getting reparse points from server without WSL support

[Jeremy Allison]

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.

Re: [PATCH] cifs: Fix getting reparse points from server without WSL support

[Pali Rohár]

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?

Re: [PATCH] cifs: Fix getting reparse points from server without WSL support

[Jeremy Allison]

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.

Re: [PATCH] cifs: Fix getting reparse points from server without WSL support

[Pali Rohár]

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?

Re: [PATCH] cifs: Fix getting reparse points from server without WSL support

[Jeremy Allison]

On Tue, Sep 17, 2024 at 10:34:31PM +0200, Pali Rohár wrote:
>On Tuesday 17 September 2024 13:31:22 Jeremy Allison wrote:
>> 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?
>

Yes. I think that should be the case. It's not my code
though, so take that as my free 2 cents opinion :-).

Samba will never expose uids / gids on the wire over
SMB2+ though (it's too late for that mistake we made
in SMB1).

Re: [PATCH] cifs: Fix getting reparse points from server without WSL support

[ronnie sahlberg]

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.

>

Re: [PATCH] cifs: Fix getting reparse points from server without WSL support

[Jeremy Allison]

On Wed, Sep 18, 2024 at 06:44:39AM +1000, ronnie sahlberg wrote:
>On Wed, 18 Sept 2024 at 06:37, Pali Rohár <pali@kernel.org> wrote:
>>
>> 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.

Thanks Ronnie. You said that much better than I did :-) :-).

Re: [PATCH] cifs: Fix getting reparse points from server without WSL support

[Pali Rohár]

On Tuesday 17 September 2024 13:46:18 Jeremy Allison wrote:
> On Wed, Sep 18, 2024 at 06:44:39AM +1000, ronnie sahlberg wrote:
> > On Wed, 18 Sept 2024 at 06:37, Pali Rohár <pali@kernel.org> wrote:
> > > 
> > > 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.
> 
> Thanks Ronnie. You said that much better than I did :-) :-).

Understood, thank you!

So based on this for me it looks like that for client it would be safer
to ignore uid an gid for reparse points and use only SIDs.

I hope that somebody will recheck that client code in wsl_to_fattr() function.

Re: [PATCH] cifs: Fix getting reparse points from server without WSL support

[Steve French]

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

Do not use UID and GID from EAs

[Pali Rohár]

[-- 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

Re: Do not use UID and GID from EAs

[Pali Rohár]

On Monday 11 August 2025 12:52:58 Pali Rohár wrote:
> 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?

Hello, have you looked at the proposed change in the previous email?

Re: [PATCH] cifs: Fix getting reparse points from server without WSL support

[Paulo Alcantara]

Pali Rohár <pali@kernel.org> writes:

> Paulo, please look at this patch as it is related to WSL attributes
> which you introduced in the mentioned commit. I think that the proper
> fix should be to change SMB2_OP_QUERY_WSL_EA code to not trigger that
> -EOPNOTSUPP error which is delivered to userspace. I just checked that
> this my patch works fine for Native NTFS symlinks and NFS-style reparse
> point special files.

Thanks for the patch.  The problem is that the client is considering
that the entire compound request failed when the server doesn't support
EA.  The client should still parse the rest of the response that
contains the getinfo and get reparse info data.

Re: [PATCH] cifs: Fix getting reparse points from server without WSL support

[Pali Rohár]

On Tuesday 17 September 2024 18:04:37 Paulo Alcantara wrote:
> Pali Rohár <pali@kernel.org> writes:
> 
> > Paulo, please look at this patch as it is related to WSL attributes
> > which you introduced in the mentioned commit. I think that the proper
> > fix should be to change SMB2_OP_QUERY_WSL_EA code to not trigger that
> > -EOPNOTSUPP error which is delivered to userspace. I just checked that
> > this my patch works fine for Native NTFS symlinks and NFS-style reparse
> > point special files.
> 
> Thanks for the patch.  The problem is that the client is considering
> that the entire compound request failed when the server doesn't support
> EA.  The client should still parse the rest of the response that
> contains the getinfo and get reparse info data.

I agree with you. This sounds like the best option.

Would you be able to fix the client code to do this?

Re: [PATCH] cifs: Fix getting reparse points from server without WSL support

[Paulo Alcantara]

Pali Rohár <pali@kernel.org> writes:

> Would you be able to fix the client code to do this?

Yep.  Will send it to ML soon.

Re: [PATCH] cifs: Fix getting reparse points from server without WSL support

[Pali Rohár]

On Tuesday 17 September 2024 18:17:08 Paulo Alcantara wrote:
> Pali Rohár <pali@kernel.org> writes:
> 
> > Would you be able to fix the client code to do this?
> 
> Yep.  Will send it to ML soon.

Perfect, let me know when you have a working version, so I can test it.

Re: [PATCH] cifs: Fix getting reparse points from server without WSL support

[Pali Rohár]

On Monday 23 September 2024 20:10:00 Pali Rohár wrote:
> On Tuesday 17 September 2024 18:17:08 Paulo Alcantara wrote:
> > Pali Rohár <pali@kernel.org> writes:
> > 
> > > Would you be able to fix the client code to do this?
> > 
> > Yep.  Will send it to ML soon.
> 
> Perfect, let me know when you have a working version, so I can test it.

Steve, Paulo, I think that fixing this part of he code has higher
priority than any other my patches as since commit ea41367b2a60 ("smb:
client: introduce SMB2_OP_QUERY_WSL_EA") Linux CIFS client is not able
to fetch & recognize any reparse points from servers without EAs support
and also from servers which do not support reparse points and EAs
together, which are all non-recent Windows SMB servers.

Re: [PATCH] cifs: Fix getting reparse points from server without WSL support

[Steve French]

On Tue, Sep 17, 2024 at 4:04 PM Paulo Alcantara <pc@manguebit.com> wrote:
>
> Pali Rohár <pali@kernel.org> writes:
>
> > Paulo, please look at this patch as it is related to WSL attributes
> > which you introduced in the mentioned commit. I think that the proper
> > fix should be to change SMB2_OP_QUERY_WSL_EA code to not trigger that
> > -EOPNOTSUPP error which is delivered to userspace. I just checked that
> > this my patch works fine for Native NTFS symlinks and NFS-style reparse
> > point special files.
>
> Thanks for the patch.  The problem is that the client is considering
> that the entire compound request failed when the server doesn't support
> EA.  The client should still parse the rest of the response that
> contains the getinfo and get reparse info data.

Yes.  Agreed.

And on the

-- 
Thanks,

Steve

Re: [PATCH] cifs: Fix getting reparse points from server without WSL support

[Pali Rohár]

On Friday 13 September 2024 22:02:04 Pali Rohár wrote:
> If SMB server does not support WSL EAs then for SMB2_OP_QUERY_WSL_EA
> request it returns STATUS_EAS_NOT_SUPPORTED. Function smb2_compound_op()
> translates STATUS_EAS_NOT_SUPPORTED to -EOPNOTSUPP which cause failure
> during calling lstat() syscall from userspace on any reparse point,
> including Native SMB symlink (which does not use any EAs). This issue
> happen for example when trying to resolve Native NTFS symlink from SMB
> server on Windows 8.

Just for completeness, why this happens also on Windows server with NTFS
which supports both EAs and Reparse Points.

Older versions of Windows do not allow to set _both_ EAs and Reparse
point at the same time for one file. This is documented limitation of
NTFS. It looks like that this limitation was fixed in later Windows
versions where is running WSL.

So QUERY EA request for file which has already set reparse point ends
with STATUS_EAS_NOT_SUPPORTED error, even server supports EAs.

And similarly, FSCTL_SET_REPARSE_POINT call fails with error
STATUS_EAS_NOT_SUPPORTED when EAs are already set on the file. It is
rather cripple error for FSCTL_SET_REPARSE_POINT, but it is documented:
https://learn.microsoft.com/en-us/windows-hardware/drivers/ifs/fsctl-set-reparse-point

So Linux SMB client code should be extended to expect that compound
operation with: CREATE_with_EAs, FSCTL_SET_REPARSE_POINT, CLOSE will
fail on the FSCTL_SET_REPARSE_POINT with STATUS_EAS_NOT_SUPPORTED (even
EAs are correctly set by CREATE). And that QUERY_EA will fail on
STATUS_EAS_NOT_SUPPORTED even when EAs are supported by server.