I see what you mean. The last check in smb2_fix_symlink_target_type
will disallow to use symlinks to "." and ".." created by older linux
client as it created it as symlink to file, instead of symlink to dir.
This is a good point. I can send a new version with dropped last check
in smb2_fix_symlink_target_type.
On Sunday 13 October 2024 12:56:23 Steve French wrote:
> Would this break any pure Linux client example, mounted to Windows,
> where previously the Linux client created all symlinks as file
> symlinks? e.g. If there were two Linux clients writing to the share,
> one that included this fix and one that did not.
>
> On Sat, Oct 5, 2024 at 9:03 AM Pali Rohár <pali@xxxxxxxxxx> wrote:
> >
> > As SMB protocol distinguish between symlink to directory and symlink to
> > file, add some mechanism to disallow resolving incompatible types.
> >
> > When SMB symlink is of the directory type, ensure that its target path ends
> > with slash. This forces Linux to not allow resolving such symlink to file.
> >
> > And when SMB symlink is of the file type and its target path ends with
> > slash then returns an error as such symlink is unresolvable. Such symlink
> > always points to invalid location as file cannot end with slash.
> >
> > As POSIX server does not distinguish between symlinks to file and symlink
> > directory, do not apply this change for symlinks from POSIX SMB server. For
> > POSIX SMB servers, this change does nothing.
> >
> > This mimics Windows behavior of native SMB symlinks.
> >
> > Signed-off-by: Pali Rohár <pali@xxxxxxxxxx>
> > ---
> > fs/smb/client/inode.c | 5 ++++
> > fs/smb/client/smb2file.c | 55 +++++++++++++++++++++++++++++++++++++++
> > fs/smb/client/smb2inode.c | 4 +++
> > fs/smb/client/smb2proto.h | 1 +
> > 4 files changed, 65 insertions(+)
> >
> > diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> > index 0fe54b2d2561..aa38a3935f8f 100644
> > --- a/fs/smb/client/inode.c
> > +++ b/fs/smb/client/inode.c
> > @@ -1110,6 +1110,11 @@ static int reparse_info_to_fattr(struct cifs_open_info_data *data,
> > full_path,
> > iov, data);
> > }
> > +
> > + if (data->reparse.tag == IO_REPARSE_TAG_SYMLINK && !rc) {
> > + bool directory = le32_to_cpu(data->fi.Attributes) & ATTR_DIRECTORY;
> > + rc = smb2_fix_symlink_target_type(&data->symlink_target, directory, cifs_sb);
> > + }
> > break;
> > }
> >
> > diff --git a/fs/smb/client/smb2file.c b/fs/smb/client/smb2file.c
> > index dc52995f5591..149449d9c1c0 100644
> > --- a/fs/smb/client/smb2file.c
> > +++ b/fs/smb/client/smb2file.c
> > @@ -63,6 +63,56 @@ static struct smb2_symlink_err_rsp *symlink_data(const struct kvec *iov)
> > return sym;
> > }
> >
> > +int smb2_fix_symlink_target_type(char **target, bool directory, struct cifs_sb_info *cifs_sb)
> > +{
> > + char *buf;
> > + int len;
> > +
> > + /*
> > + * POSIX server does not distinguish between symlinks to file and
> > + * symlink directory. So nothing is needed to fix on the client side.
> > + */
> > + if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS)
> > + return 0;
> > +
> > + len = strlen(*target);
> > + if (!len)
> > + return -EIO;
> > +
> > + /*
> > + * If this is directory symlink and it does not have trailing slash then
> > + * append it. Trailing slash simulates Windows/SMB behavior which do not
> > + * allow resolving directory symlink to file.
> > + */
> > + if (directory && (*target)[len-1] != '/') {
> > + buf = kzalloc(len+2, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > + memcpy(buf, *target, len);
> > + buf[len] = '/';
> > + kfree(*target);
> > + *target = buf;
> > + }
> > +
> > + /*
> > + * If this is a symlink which points to file name with trailing slash,
> > + * or to file named "." or file named ".." then this symlink cannot be
> > + * resolved on Linux because Linux does not allow files with such names.
> > + * So return an error to prevent resolving this file type symlink to
> > + * directory, as it do not point to directory at all.
> > + */
> > + if (!directory) {
> > + const char *basename = kbasename(*target);
> > + int basename_len = strlen(basename);
> > + if (basename_len == 0 || /* symname ends with slash */
> > + (basename_len == 1 && basename[0] == '.') || /* last component is "." */
> > + (basename_len == 2 && basename[0] == '.' && basename[1] == '.')) /* or ".." */
> > + return -EIO;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > int smb2_parse_symlink_response(struct cifs_sb_info *cifs_sb, const struct kvec *iov,
> > const char *full_path, char **path)
> > {
> > @@ -133,6 +183,11 @@ int smb2_open_file(const unsigned int xid, struct cifs_open_parms *oparms, __u32
> > NULL, NULL, NULL);
> > oparms->create_options &= ~OPEN_REPARSE_POINT;
> > }
> > + if (!rc) {
> > + bool directory = le32_to_cpu(data->fi.Attributes) & ATTR_DIRECTORY;
> > + rc = smb2_fix_symlink_target_type(&data->symlink_target,
> > + directory, oparms->cifs_sb);
> > + }
> > }
> > }
> >
> > diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
> > index 9a28a30ec1a3..06bb6f7fbf0f 100644
> > --- a/fs/smb/client/smb2inode.c
> > +++ b/fs/smb/client/smb2inode.c
> > @@ -960,6 +960,10 @@ int smb2_query_path_info(const unsigned int xid,
> > rc = smb2_compound_op(xid, tcon, cifs_sb, full_path,
> > &oparms, in_iov, cmds, num_cmds,
> > cfile, NULL, NULL, NULL);
> > + if (data->reparse.tag == IO_REPARSE_TAG_SYMLINK && !rc) {
> > + bool directory = le32_to_cpu(data->fi.Attributes) & ATTR_DIRECTORY;
> > + rc = smb2_fix_symlink_target_type(&data->symlink_target, directory, cifs_sb);
> > + }
> > break;
> > case -EREMOTE:
> > break;
> > diff --git a/fs/smb/client/smb2proto.h b/fs/smb/client/smb2proto.h
> > index aa01ae234732..1828b825c7d3 100644
> > --- a/fs/smb/client/smb2proto.h
> > +++ b/fs/smb/client/smb2proto.h
> > @@ -113,6 +113,7 @@ extern int smb3_query_mf_symlink(unsigned int xid, struct cifs_tcon *tcon,
> > struct cifs_sb_info *cifs_sb,
> > const unsigned char *path, char *pbuf,
> > unsigned int *pbytes_read);
> > +int smb2_fix_symlink_target_type(char **target, bool directory, struct cifs_sb_info *cifs_sb);
> > int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
> > bool unicode, bool relative,
> > const char *full_path,
> > --
> > 2.20.1
> >
> >
>
>
> --
> Thanks,
>
> Steve
