linux-kernel - Re: [PATCH] cifs: Fix lstat() and AT_SYMLINK_NOFOLLOW to work on broken symlink nodes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250621122139.3xq675cbs5kgkd7t@pali>
Date: Sat, 21 Jun 2025 14:21:39 +0200
From: Pali Rohár <pali@...nel.org>
To: Paulo Alcantara <pc@...guebit.org>
Cc: Steve French <sfrench@...ba.org>, Remy Monsen <monsen@...sen.cc>,
	linux-cifs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] cifs: Fix lstat() and AT_SYMLINK_NOFOLLOW to work on
 broken symlink nodes

On Friday 20 June 2025 20:44:37 Paulo Alcantara wrote:
> Pali Rohár <pali@...nel.org> writes:
> 
> > Currently Linux SMB client returns EIO for lstat() and AT_SYMLINK_NOFOLLOW
> > calls on symlink node when the symlink target location is broken or cannot
> > be read or parsed.
> >
> > Fix this problem by relaxing the errors from various locations which parses
> > information about symlink file node (UNIX SMB1, native SMB2+, NFS-style,
> > WSL-style) and let readlink() syscall to return EIO when the symlink target
> > location is not available.
> 
> Please, don't.  We still want those validations for the other types of
> symlinks.

Well, validation was not removed. Validation is still there, just the
error is signalled by the readlink() syscall instead of the lstat() or
AT_SYMLINK_NOFOLLOW syscalls.

My opinion is that the lstat() or AT_SYMLINK_NOFOLLOW should work on
symlink node independently of where the symlink points (and whether the
symlink target is valid POSIX path or not). That is because the lstat()
and AT_SYMLINK_NOFOLLOW says that the symlink target location must not
be used and must not be resolved.

But still the invalid / incorrect / broken or non-representable symlink
target path in POSIX notation should be reported as an issue and the
readlink() is the correct syscall which should report these errors.

> The problem is just that cifs.ko can't handle absolute
> symlink targets in the form of '\??\UNC\srv\share\foo', while Windows
> client can.  They are still valid symlink targets, but cifs.ko doesn't
> know how to follow them.

Windows client can represent and follow such symlink because the symlink
is in the NT style format and Windows kernel uses NT style of paths
internally. Linux kernel uses POSIX paths and POSIX does not contain any
GLOBAL?? namespace for NT object hierarchy.

Leaking raw NT object hierarchy from SMB to POSIX userspace via
readlink() syscall is a bad idea. Applications are really not expecting
that the readlink() syscall will return NT kernel internals (exported
over SMB protocol and passed to cifs.ko).

For UNC paths encoded in NT object hierarchy, which is just some subset
of all possible NT paths, I had an idea that we could convert these
paths to some format like:

   <prefix>/server/share/path...

Where <prefix> would be specified by the string mount option. So user
could say that wants all UNC symlinks pointing to /mnt/unc/.

And in the same way if user would want to create symlink pointing to
/mnt/unc/server/share/path... then cifs.ko will transform it into valid
NT UNC path and create a symlink to this location.

But this would solve only problem with UNC symlink, not symlinks
pointing to NT object hierarchy in general.

> The following should do it and then restore old behavior
> 
> diff --git a/fs/smb/client/reparse.c b/fs/smb/client/reparse.c
> index bb25e77c5540..11d44288e75a 100644
> --- a/fs/smb/client/reparse.c
> +++ b/fs/smb/client/reparse.c
> @@ -875,15 +875,8 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
>  			abs_path += sizeof("\\DosDevices\\")-1;
>  		else if (strstarts(abs_path, "\\GLOBAL??\\"))
>  			abs_path += sizeof("\\GLOBAL??\\")-1;
> -		else {
> -			/* Unhandled absolute symlink, points outside of DOS/Win32 */
> -			cifs_dbg(VFS,
> -				 "absolute symlink '%s' cannot be converted from NT format "
> -				 "because points to unknown target\n",
> -				 smb_target);
> -			rc = -EIO;
> -			goto out;
> -		}
> +		else
> +			goto out_unhandled_target;
>  
>  		/* Sometimes path separator after \?? is double backslash */
>  		if (abs_path[0] == '\\')
> @@ -910,13 +903,7 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
>  			abs_path++;
>  			abs_path[0] = drive_letter;
>  		} else {
> -			/* Unhandled absolute symlink. Report an error. */
> -			cifs_dbg(VFS,
> -				 "absolute symlink '%s' cannot be converted from NT format "
> -				 "because points to unknown target\n",
> -				 smb_target);
> -			rc = -EIO;
> -			goto out;
> +			goto out_unhandled_target;
>  		}
>  
>  		abs_path_len = strlen(abs_path)+1;
> @@ -966,6 +953,7 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
>  		 * These paths have same format as Linux symlinks, so no
>  		 * conversion is needed.
>  		 */
> +out_unhandled_target:
>  		linux_target = smb_target;
>  		smb_target = NULL;
>  	}

I'm really not sure if removing the messages and error reporting about
symlinks which cannot be represented in POSIX system is a good idea.