| 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
| ||
|
Message-ID: <20241005140813.3lmqjwcg2kli5jvm@pali>
Date: Sat, 5 Oct 2024 16:08:13 +0200
From: Pali Rohár <pali@...nel.org>
To: Paulo Alcantara <pc@...guebit.com>
Cc: Steve French <sfrench@...ba.org>,
Ronnie Sahlberg <ronniesahlberg@...il.com>,
linux-cifs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/7] cifs: Improve creating native symlinks pointing to
directory
On Monday 30 September 2024 19:17:15 Pali Rohár wrote:
> On Monday 30 September 2024 12:09:48 Paulo Alcantara wrote:
> > Pali Rohár <pali@...nel.org> writes:
> >
> > > SMB protocol for native symlinks distinguish between symlink to directory
> > > and symlink to file. These two symlink types cannot be exchanged, which
> > > means that symlink of file type pointing to directory cannot be resolved at
> > > all (and vice-versa).
> > >
> > > Windows follows this rule for local filesystems (NTFS) and also for SMB.
> > >
> > > Linux SMB client currenly creates all native symlinks of file type. Which
> > > means that Windows (and some other SMB clients) cannot resolve symlinks
> > > pointing to directory created by Linux SMB client.
> > >
> > > As Linux system does not distinguish between directory and file symlinks,
> > > its API does not provide enough information for Linux SMB client during
> > > creating of native symlinks.
> > >
> > > Add some heuristic into the Linux SMB client for choosing the correct
> > > symlink type during symlink creation. Check if the symlink target location
> > > ends with slash, or last path component is dot or dot dot, and check if the
> > > target location on SMB share exists and is a directory. If at least one
> > > condition is truth then create a new SMB symlink of directory type.
> > > Otherwise create it as file type symlink.
> > >
> > > This change improves interoperability with Windows systems. Windows systems
> > > would be able to resolve more SMB symlinks created by Linux SMB client
> > > which points to existing directory.
> > >
> > > Signed-off-by: Pali Rohár <pali@...nel.org>
> > > ---
> > > fs/smb/client/reparse.c | 131 ++++++++++++++++++++++++++++++++++++--
> > > fs/smb/client/smb2inode.c | 3 +-
> > > fs/smb/client/smb2proto.h | 1 +
> > > 3 files changed, 130 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/fs/smb/client/reparse.c b/fs/smb/client/reparse.c
> > > index 507e17244ed3..9390ab801696 100644
> > > --- a/fs/smb/client/reparse.c
> > > +++ b/fs/smb/client/reparse.c
> > > @@ -24,13 +24,16 @@ int smb2_create_reparse_symlink(const unsigned int xid, struct inode *inode,
> > > struct inode *new;
> > > struct kvec iov;
> > > __le16 *path;
> > > + bool directory = false;
> > > char *sym, sep = CIFS_DIR_SEP(cifs_sb);
> > > u16 len, plen;
> > > int rc = 0;
> > >
> > > - sym = kstrdup(symname, GFP_KERNEL);
> > > + len = strlen(symname)+1;
> > > + sym = kzalloc(len+1, GFP_KERNEL); /* +1 for possible directory slash */
> > > if (!sym)
> > > return -ENOMEM;
> > > + memcpy(sym, symname, len);
> > >
> > > data = (struct cifs_open_info_data) {
> > > .reparse_point = true,
> > > @@ -45,6 +48,125 @@ int smb2_create_reparse_symlink(const unsigned int xid, struct inode *inode,
> > > goto out;
> > > }
> > >
> > > + /*
> > > + * SMB distinguish between symlink to directory and symlink to file.
> > > + * They cannot be exchanged (symlink of file type which points to
> > > + * directory cannot be resolved and vice-versa). First do some simple
> > > + * check, if the original Linux symlink target ends with slash, or
> > > + * last path component is dot or dot dot then it is for sure symlink
> > > + * to the directory.
> > > + */
> > > + if (!directory) {
> > > + const char *basename = kbasename(symname);
> > > + 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 ".." */
> > > + directory = true;
> > > + }
> > > +
> > > + /*
> > > + * If it was not detected as directory yet and the symlink is relative
> > > + * then try to resolve the path on the SMB server, check if the path
> > > + * exists and determinate if it is a directory or not.
> > > + */
> > > + if (!directory && symname[0] != '/') {
> > > + __u32 oplock;
> > > + struct tcon_link *tlink;
> > > + struct cifs_tcon *tcon;
> > > + struct cifs_fid fid;
> > > + struct cifs_open_parms oparms;
> > > + char *resolved_path;
> > > + char *path_sep;
> > > + int open_rc;
> > > + int full_path_len = strlen(full_path);
> > > + int symname_len = strlen(symname);
> > > +
> > > + tlink = cifs_sb_tlink(cifs_sb);
> > > + if (IS_ERR(tlink)) {
> > > + rc = PTR_ERR(tlink);
> > > + goto out;
> > > + }
> > > +
> > > + resolved_path = kzalloc(full_path_len + symname_len + 1, GFP_KERNEL);
> > > + if (!resolved_path) {
> > > + rc = -ENOMEM;
> > > + goto out;
> > > + }
> >
> > If !@...olved_path, then you will end up leaking @tlink.
> >
> > > +
> > > + /*
> > > + * Compose the resolved SMB symlink path from the SMB full path
> > > + * and Linux target symlink path.
> > > + */
> > > + memcpy(resolved_path, full_path, full_path_len+1);
> > > + path_sep = strrchr(resolved_path, sep);
> > > + if (path_sep)
> > > + path_sep++;
> > > + else
> > > + path_sep = resolved_path;
> > > + memcpy(path_sep, symname, symname_len+1);
> > > + if (sep == '\\')
> > > + convert_delimiter(path_sep, sep);
> > > +
> > > + tcon = tlink_tcon(tlink);
> > > +
> > > + oparms = (struct cifs_open_parms) {
> > > + .tcon = tcon,
> > > + .cifs_sb = cifs_sb,
> > > + .desired_access = FILE_READ_ATTRIBUTES,
> > > + .disposition = FILE_OPEN,
> > > + .path = resolved_path,
> > > + .fid = &fid,
> > > + };
> >
> > Please use CIFS_OPARMS().
> >
> > > +
> > > + /* Try to open as NOT_FILE */
> > > + oplock = 0;
> > > + oparms.create_options = cifs_create_options(cifs_sb, CREATE_NOT_FILE);
> > > + open_rc = tcon->ses->server->ops->open(xid, &oparms, &oplock, NULL);
> > > + if (open_rc == 0) {
> > > + /* Successful open means that the target path is definitely a directory. */
> > > + directory = true;
> > > + tcon->ses->server->ops->close(xid, tcon, &fid);
> > > + } else if (open_rc != -ENOTDIR) {
> > > + /* Try to open as NOT_DIR */
> > > + oplock = 0;
> > > + oparms.create_options = cifs_create_options(cifs_sb, CREATE_NOT_DIR);
> > > + open_rc = tcon->ses->server->ops->open(xid, &oparms, &oplock, NULL);
> > > + if (open_rc == 0) {
> > > + tcon->ses->server->ops->close(xid, tcon, &fid);
> > > + } else if (open_rc == -EISDIR) {
> > > + /* -EISDIR means that the target path is definitely a directory. */
> > > + directory = true;
> > > + } else {
> > > + cifs_dbg(FYI,
> > > + "%s: cannot determinate if the symlink target path '%s' "
> > > + "is directory or not, creating '%s' as file symlink\n",
> > > + __func__, symname, full_path);
> > > + }
> > > + }
> > > +
> > > + kfree(resolved_path);
> > > + cifs_put_tlink(tlink);
> > > + }
> > > +
> > > + /*
> > > + * For absolute symlinks it is not possible to determinate
> > > + * if it should point to directory or file.
> > > + */
> > > + if (!directory && symname[0] == '/')
> > > + cifs_dbg(FYI,
> > > + "%s: cannot determinate if the symlink target path '%s' "
> > > + "is directory or not, creating '%s' as file symlink\n",
> > > + __func__, symname, full_path);
> > > +
> >
> > Create a helper with all of this and then call it in
> > smb2_create_reparse_symlink() to determine whether symlink target is a
> > directory or file.
>
> Hello, thanks for review! Of course I will update this patch to address
> all your points.
Hello, I addresses this issues in V2 of the patch series. Please look at it.
Powered by blists - more mailing lists