Re: [PATCH v2 4/6] cifs: Fix parsing native symlinks directory/file type

[Pali Rohár]

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