| Message ID | 20240929185053.10554-6-pali@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: Improve support for native SMB symlinks | expand |
obvious question is ... is there any risk that this could break POSIX behavior when creating server side symlinks (ie not using mfsylmlinks, but native symlink reparse point) remotely ...? On Sun, Sep 29, 2024 at 1:51 PM Pali Rohár <pali@kernel.org> 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. > > This mimics Windows behavior of native SMB symlinks. > > Signed-off-by: Pali Rohár <pali@kernel.org> > --- > fs/smb/client/reparse.c | 4 ++++ > fs/smb/client/smb2file.c | 46 +++++++++++++++++++++++++++++++++++++++ > fs/smb/client/smb2inode.c | 4 ++++ > fs/smb/client/smb2proto.h | 1 + > 4 files changed, 55 insertions(+) > > diff --git a/fs/smb/client/reparse.c b/fs/smb/client/reparse.c > index d8edb513556f..5a738f65b190 100644 > --- a/fs/smb/client/reparse.c > +++ b/fs/smb/client/reparse.c > @@ -589,6 +589,10 @@ static int parse_reparse_symlink(struct reparse_symlink_data_buffer *sym, > le32_to_cpu(sym->Flags) & SYMLINK_FLAG_RELATIVE, > full_path, > cifs_sb); > + if (!rc) { > + bool directory = le32_to_cpu(data->fi.Attributes) & ATTR_DIRECTORY; > + rc = smb2_fix_symlink_target_type(&data->symlink_target, directory); > + } > return rc; > } > > diff --git a/fs/smb/client/smb2file.c b/fs/smb/client/smb2file.c > index dc52995f5591..8a1a1b2a1c81 100644 > --- a/fs/smb/client/smb2file.c > +++ b/fs/smb/client/smb2file.c > @@ -63,6 +63,48 @@ static struct smb2_symlink_err_rsp *symlink_data(const struct kvec *iov) > return sym; > } > > +int smb2_fix_symlink_target_type(char **target, bool directory) > +{ > + char *buf; > + int 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] == '.')) /* last component is ".." */ > + 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) > { > @@ -132,6 +174,10 @@ int smb2_open_file(const unsigned int xid, struct cifs_open_parms *oparms, __u32 > rc = SMB2_open(xid, oparms, smb2_path, &smb2_oplock, smb2_data, > 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); > + } > } > } > } > diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c > index c9cdac7d2d50..faf0a8344faa 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); > + } > break; > case -EREMOTE: > break; > diff --git a/fs/smb/client/smb2proto.h b/fs/smb/client/smb2proto.h > index 11cef65fa831..d308f3c2f8df 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); > int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len, > bool unicode, bool relative, > const char *full_path, > -- > 2.20.1 > >
I was thinking about it and in my opinion this should be OK. If you create native directory symlink on Windows via mklink /D and it would point to file, then trying to open that symlink fails on Windows with error. On Linux if you append slash to symlink target path then opening it will fail if the target path is file. On Linux the symlink pointing to directory may have trailing slash but it not required. But testing by other people would be useful to confirm that there is not some hidden issue. On Sunday 29 September 2024 16:47:48 Steve French wrote: > obvious question is ... is there any risk that this could break POSIX > behavior when creating server side symlinks (ie not using mfsylmlinks, > but native symlink reparse point) remotely ...? > > On Sun, Sep 29, 2024 at 1:51 PM Pali Rohár <pali@kernel.org> 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. > > > > This mimics Windows behavior of native SMB symlinks. > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > --- > > fs/smb/client/reparse.c | 4 ++++ > > fs/smb/client/smb2file.c | 46 +++++++++++++++++++++++++++++++++++++++ > > fs/smb/client/smb2inode.c | 4 ++++ > > fs/smb/client/smb2proto.h | 1 + > > 4 files changed, 55 insertions(+) > > > > diff --git a/fs/smb/client/reparse.c b/fs/smb/client/reparse.c > > index d8edb513556f..5a738f65b190 100644 > > --- a/fs/smb/client/reparse.c > > +++ b/fs/smb/client/reparse.c > > @@ -589,6 +589,10 @@ static int parse_reparse_symlink(struct reparse_symlink_data_buffer *sym, > > le32_to_cpu(sym->Flags) & SYMLINK_FLAG_RELATIVE, > > full_path, > > cifs_sb); > > + if (!rc) { > > + bool directory = le32_to_cpu(data->fi.Attributes) & ATTR_DIRECTORY; > > + rc = smb2_fix_symlink_target_type(&data->symlink_target, directory); > > + } > > return rc; > > } > > > > diff --git a/fs/smb/client/smb2file.c b/fs/smb/client/smb2file.c > > index dc52995f5591..8a1a1b2a1c81 100644 > > --- a/fs/smb/client/smb2file.c > > +++ b/fs/smb/client/smb2file.c > > @@ -63,6 +63,48 @@ static struct smb2_symlink_err_rsp *symlink_data(const struct kvec *iov) > > return sym; > > } > > > > +int smb2_fix_symlink_target_type(char **target, bool directory) > > +{ > > + char *buf; > > + int 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] == '.')) /* last component is ".." */ > > + 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) > > { > > @@ -132,6 +174,10 @@ int smb2_open_file(const unsigned int xid, struct cifs_open_parms *oparms, __u32 > > rc = SMB2_open(xid, oparms, smb2_path, &smb2_oplock, smb2_data, > > 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); > > + } > > } > > } > > } > > diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c > > index c9cdac7d2d50..faf0a8344faa 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); > > + } > > break; > > case -EREMOTE: > > break; > > diff --git a/fs/smb/client/smb2proto.h b/fs/smb/client/smb2proto.h > > index 11cef65fa831..d308f3c2f8df 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); > > 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
diff --git a/fs/smb/client/reparse.c b/fs/smb/client/reparse.c index d8edb513556f..5a738f65b190 100644 --- a/fs/smb/client/reparse.c +++ b/fs/smb/client/reparse.c @@ -589,6 +589,10 @@ static int parse_reparse_symlink(struct reparse_symlink_data_buffer *sym, le32_to_cpu(sym->Flags) & SYMLINK_FLAG_RELATIVE, full_path, cifs_sb); + if (!rc) { + bool directory = le32_to_cpu(data->fi.Attributes) & ATTR_DIRECTORY; + rc = smb2_fix_symlink_target_type(&data->symlink_target, directory); + } return rc; } diff --git a/fs/smb/client/smb2file.c b/fs/smb/client/smb2file.c index dc52995f5591..8a1a1b2a1c81 100644 --- a/fs/smb/client/smb2file.c +++ b/fs/smb/client/smb2file.c @@ -63,6 +63,48 @@ static struct smb2_symlink_err_rsp *symlink_data(const struct kvec *iov) return sym; } +int smb2_fix_symlink_target_type(char **target, bool directory) +{ + char *buf; + int 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] == '.')) /* last component is ".." */ + 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) { @@ -132,6 +174,10 @@ int smb2_open_file(const unsigned int xid, struct cifs_open_parms *oparms, __u32 rc = SMB2_open(xid, oparms, smb2_path, &smb2_oplock, smb2_data, 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); + } } } } diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c index c9cdac7d2d50..faf0a8344faa 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); + } break; case -EREMOTE: break; diff --git a/fs/smb/client/smb2proto.h b/fs/smb/client/smb2proto.h index 11cef65fa831..d308f3c2f8df 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); int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len, bool unicode, bool relative, const char *full_path,
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. This mimics Windows behavior of native SMB symlinks. Signed-off-by: Pali Rohár <pali@kernel.org> --- fs/smb/client/reparse.c | 4 ++++ fs/smb/client/smb2file.c | 46 +++++++++++++++++++++++++++++++++++++++ fs/smb/client/smb2inode.c | 4 ++++ fs/smb/client/smb2proto.h | 1 + 4 files changed, 55 insertions(+)