Re: [PATCH] ksmbd: check if a mount point is crossed during path lookup

[Tom Talpey <tom@xxxxxxxxxx>]

Answering Namjae, well I have the same concern with Samba, but there at
least there's a more sophisticated vfs layer that can fix up semantic
variations. In-kernel, it's the raw filesystem.

Steve, yes absolutely a DFS redirect would be an appropriate
solution. But leaning on fsinfo/statfs as the lookup descends
the tree doesn't persuade me. No user and no app is going to
do that.

Tom.

On 7/14/2023 11:33 AM, Steve French wrote:
> In the future if we want to make this more visible to the client
> (although presumably query fs info/statfs can still return the correct
> information depending on the path given by the user) we could always
> make this a fake DFS referral after we cross a mount point on the
> server
>
> On Fri, Jul 14, 2023 at 8:25 AM Namjae Jeon <linkinjeon@xxxxxxxxxx> wrote:
> > 2023-07-14 21:44 GMT+09:00, Tom Talpey <tom@xxxxxxxxxx>:
> > > On 7/12/2023 7:40 PM, Namjae Jeon wrote:
> > > > 2023-07-13 2:23 GMT+09:00, Tom Talpey <tom@xxxxxxxxxx>:
> > > > > On 7/11/2023 8:30 AM, Namjae Jeon wrote:
> > > > > > Since commit 74d7970febf7 ("ksmbd: fix racy issue from using ->d_parent
> > > > > > and ->d_name"),
> > > > > > ksmbd can not lookup cross mount points. If last component is a cross
> > > > > > mount point during path lookup, check if it is crossed to follow it
> > > > > > down.
> > > > >
> > > > > I actually thought not crossing mount points was intended, since
> > > > > semantics can shift if the crossing changes filesystems or fs types.
> > > > >
> > > > > Does this change somehow prevent walking out of a mount via embedded
> > > > > "/../" in the path? It's not obvious to me whether
> > > > > vfs_path_parent_lookup
> > > > > denies this.
> > > > Yes, LOOKUP_BENEATH flags protect to lookup out of share.
> > > > > Tom.
> > > > >
> > > > > I'm not familiar enough with the VFS protection, but does this
> > > > I have checked it before and now again. There is no problem.
> > >
> > > I'm still concerned about this. Crossing mount points in the server is
> > > compleately invisible to the client, yet the semantics change radically.
> > Have you checked it with samba server ? This is exactly the same behavior
> > as samba server.
> >
> > And users are also wanting exactly the same behavior as before.
> > https://github.com/namjaejeon/ksmbd/issues/436
> > > What if someone shares "/mnt", and proceeds to mount an ext4, a USB
> > > FAT32, a CD-Rom, and an NFS filesystem in subdirectories? The client
> > > has no clue, and will walk into them all, finding a very different
> > > world in each.
> > >
> > > How will Posix extensions work across this? Case sensitivity/preserving?
> > > Caching and locks/leases?
> > I didn't understand exactly your question.
> > And NFS also supports crossmnt, and we will be able to add 'crossmnt' parameter
> > in smb.conf and work crossmnt by default.
> >
> > Thanks.
> > > Tom.
> > > > Thanks!
> > > > > > Fixes: 74d7970febf7 ("ksmbd: fix racy issue from using ->d_parent and
> > > > > > ->d_name")
> > > > > > Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx>
> > > > > > ---
> > > > > >     fs/smb/server/smb2pdu.c | 27 ++++++++++++--------
> > > > > >     fs/smb/server/vfs.c     | 56
> > > > > > +++++++++++++++++++++++------------------
> > > > > >     fs/smb/server/vfs.h     |  4 +--
> > > > > >     3 files changed, 49 insertions(+), 38 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
> > > > > > index cf8822103f50..ca276634fd58 100644
> > > > > > --- a/fs/smb/server/smb2pdu.c
> > > > > > +++ b/fs/smb/server/smb2pdu.c
> > > > > > @@ -2467,8 +2467,9 @@ static void smb2_update_xattrs(struct
> > > > > > ksmbd_tree_connect *tcon,
> > > > > >             }
> > > > > >     }
> > > > > >
> > > > > > -static int smb2_creat(struct ksmbd_work *work, struct path *path, char
> > > > > > *name,
> > > > > > -                int open_flags, umode_t posix_mode, bool is_dir)
> > > > > > +static int smb2_creat(struct ksmbd_work *work, struct path
> > > > > > *parent_path,
> > > > > > +                struct path *path, char *name, int open_flags,
> > > > > > +                umode_t posix_mode, bool is_dir)
> > > > > >     {
> > > > > >             struct ksmbd_tree_connect *tcon = work->tcon;
> > > > > >             struct ksmbd_share_config *share = tcon->share_conf;
> > > > > > @@ -2495,7 +2496,7 @@ static int smb2_creat(struct ksmbd_work *work,
> > > > > > struct path *path, char *name,
> > > > > >                             return rc;
> > > > > >             }
> > > > > >
> > > > > > -  rc = ksmbd_vfs_kern_path_locked(work, name, 0, path, 0);
> > > > > > +  rc = ksmbd_vfs_kern_path_locked(work, name, 0, parent_path, path, 0);
> > > > > >             if (rc) {
> > > > > >                     pr_err("cannot get linux path (%s), err = %d\n",
> > > > > >                            name, rc);
> > > > > > @@ -2565,7 +2566,7 @@ int smb2_open(struct ksmbd_work *work)
> > > > > >             struct ksmbd_tree_connect *tcon = work->tcon;
> > > > > >             struct smb2_create_req *req;
> > > > > >             struct smb2_create_rsp *rsp;
> > > > > > -  struct path path;
> > > > > > +  struct path path, parent_path;
> > > > > >             struct ksmbd_share_config *share = tcon->share_conf;
> > > > > >             struct ksmbd_file *fp = NULL;
> > > > > >             struct file *filp = NULL;
> > > > > > @@ -2786,7 +2787,8 @@ int smb2_open(struct ksmbd_work *work)
> > > > > >                     goto err_out1;
> > > > > >             }
> > > > > >
> > > > > > -  rc = ksmbd_vfs_kern_path_locked(work, name, LOOKUP_NO_SYMLINKS,
> > > > > > &path,
> > > > > > 1);
> > > > > > +  rc = ksmbd_vfs_kern_path_locked(work, name, LOOKUP_NO_SYMLINKS,
> > > > > > +                                  &parent_path, &path, 1);
> > > > > >             if (!rc) {
> > > > > >                     file_present = true;
> > > > > >
> > > > > > @@ -2906,7 +2908,8 @@ int smb2_open(struct ksmbd_work *work)
> > > > > >
> > > > > >             /*create file if not present */
> > > > > >             if (!file_present) {
> > > > > > -          rc = smb2_creat(work, &path, name, open_flags, posix_mode,
> > > > > > +          rc = smb2_creat(work, &parent_path, &path, name, open_flags,
> > > > > > +                          posix_mode,
> > > > > >                                     req->CreateOptions & FILE_DIRECTORY_FILE_LE);
> > > > > >                     if (rc) {
> > > > > >                             if (rc == -ENOENT) {
> > > > > > @@ -3321,8 +3324,9 @@ int smb2_open(struct ksmbd_work *work)
> > > > > >
> > > > > >     err_out:
> > > > > >             if (file_present || created) {
> > > > > > -          inode_unlock(d_inode(path.dentry->d_parent));
> > > > > > -          dput(path.dentry);
> > > > > > +          inode_unlock(d_inode(parent_path.dentry));
> > > > > > +          path_put(&path);
> > > > > > +          path_put(&parent_path);
> > > > > >             }
> > > > > >             ksmbd_revert_fsids(work);
> > > > > >     err_out1:
> > > > > > @@ -5545,7 +5549,7 @@ static int smb2_create_link(struct ksmbd_work
> > > > > > *work,
> > > > > >                                 struct nls_table *local_nls)
> > > > > >     {
> > > > > >             char *link_name = NULL, *target_name = NULL, *pathname = NULL;
> > > > > > -  struct path path;
> > > > > > +  struct path path, parent_path;
> > > > > >             bool file_present = false;
> > > > > >             int rc;
> > > > > >
> > > > > > @@ -5575,7 +5579,7 @@ static int smb2_create_link(struct ksmbd_work
> > > > > > *work,
> > > > > >
> > > > > >             ksmbd_debug(SMB, "target name is %s\n", target_name);
> > > > > >             rc = ksmbd_vfs_kern_path_locked(work, link_name,
> > > > > > LOOKUP_NO_SYMLINKS,
> > > > > > -                                  &path, 0);
> > > > > > +                                  &parent_path, &path, 0);
> > > > > >             if (rc) {
> > > > > >                     if (rc != -ENOENT)
> > > > > >                             goto out;
> > > > > > @@ -5605,8 +5609,9 @@ static int smb2_create_link(struct ksmbd_work
> > > > > > *work,
> > > > > >                     rc = -EINVAL;
> > > > > >     out:
> > > > > >             if (file_present) {
> > > > > > -          inode_unlock(d_inode(path.dentry->d_parent));
> > > > > > +          inode_unlock(d_inode(parent_path.dentry));
> > > > > >                     path_put(&path);
> > > > > > +          path_put(&parent_path);
> > > > > >             }
> > > > > >             if (!IS_ERR(link_name))
> > > > > >                     kfree(link_name);
> > > > > > diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
> > > > > > index e35914457350..fd06d267de61 100644
> > > > > > --- a/fs/smb/server/vfs.c
> > > > > > +++ b/fs/smb/server/vfs.c
> > > > > > @@ -63,13 +63,13 @@ int ksmbd_vfs_lock_parent(struct dentry *parent,
> > > > > > struct dentry *child)
> > > > > >
> > > > > >     static int ksmbd_vfs_path_lookup_locked(struct ksmbd_share_config
> > > > > > *share_conf,
> > > > > >                                             char *pathname, unsigned int flags,
> > > > > > +                                  struct path *parent_path,
> > > > > >                                             struct path *path)
> > > > > >     {
> > > > > >             struct qstr last;
> > > > > >             struct filename *filename;
> > > > > >             struct path *root_share_path = &share_conf->vfs_path;
> > > > > >             int err, type;
> > > > > > -  struct path parent_path;
> > > > > >             struct dentry *d;
> > > > > >
> > > > > >             if (pathname[0] == '\0') {
> > > > > > @@ -84,7 +84,7 @@ static int ksmbd_vfs_path_lookup_locked(struct
> > > > > > ksmbd_share_config *share_conf,
> > > > > >                     return PTR_ERR(filename);
> > > > > >
> > > > > >             err = vfs_path_parent_lookup(filename, flags,
> > > > > > -                               &parent_path, &last, &type,
> > > > > > +                               parent_path, &last, &type,
> > > > > >                                          root_share_path);
> > > > > >             if (err) {
> > > > > >                     putname(filename);
> > > > > > @@ -92,13 +92,13 @@ static int ksmbd_vfs_path_lookup_locked(struct
> > > > > > ksmbd_share_config *share_conf,
> > > > > >             }
> > > > > >
> > > > > >             if (unlikely(type != LAST_NORM)) {
> > > > > > -          path_put(&parent_path);
> > > > > > +          path_put(parent_path);
> > > > > >                     putname(filename);
> > > > > >                     return -ENOENT;
> > > > > >             }
> > > > > >
> > > > > > -  inode_lock_nested(parent_path.dentry->d_inode, I_MUTEX_PARENT);
> > > > > > -  d = lookup_one_qstr_excl(&last, parent_path.dentry, 0);
> > > > > > +  inode_lock_nested(parent_path->dentry->d_inode, I_MUTEX_PARENT);
> > > > > > +  d = lookup_one_qstr_excl(&last, parent_path->dentry, 0);
> > > > > >             if (IS_ERR(d))
> > > > > >                     goto err_out;
> > > > > >
> > > > > > @@ -108,15 +108,20 @@ static int ksmbd_vfs_path_lookup_locked(struct
> > > > > > ksmbd_share_config *share_conf,
> > > > > >             }
> > > > > >
> > > > > >             path->dentry = d;
> > > > > > -  path->mnt = share_conf->vfs_path.mnt;
> > > > > > -  path_put(&parent_path);
> > > > > > -  putname(filename);
> > > > > > +  path->mnt = mntget(parent_path->mnt);
> > > > > > +
> > > > > > +  err = follow_down(path, 0);
> > > > > > +  if (err < 0) {
> > > > > > +          path_put(path);
> > > > > > +          goto err_out;
> > > > > > +  }
> > > > > >
> > > > > > +  putname(filename);
> > > > > >             return 0;
> > > > > >
> > > > > >     err_out:
> > > > > > -  inode_unlock(parent_path.dentry->d_inode);
> > > > > > -  path_put(&parent_path);
> > > > > > +  inode_unlock(d_inode(parent_path->dentry));
> > > > > > +  path_put(parent_path);
> > > > > >             putname(filename);
> > > > > >             return -ENOENT;
> > > > > >     }
> > > > > > @@ -1194,14 +1199,14 @@ static int ksmbd_vfs_lookup_in_dir(const struct
> > > > > > path *dir, char *name,
> > > > > >      * Return:      0 on success, otherwise error
> > > > > >      */
> > > > > >     int ksmbd_vfs_kern_path_locked(struct ksmbd_work *work, char *name,
> > > > > > -                         unsigned int flags, struct path *path,
> > > > > > -                         bool caseless)
> > > > > > +                         unsigned int flags, struct path *parent_path,
> > > > > > +                         struct path *path, bool caseless)
> > > > > >     {
> > > > > >             struct ksmbd_share_config *share_conf = work->tcon->share_conf;
> > > > > >             int err;
> > > > > > -  struct path parent_path;
> > > > > >
> > > > > > -  err = ksmbd_vfs_path_lookup_locked(share_conf, name, flags, path);
> > > > > > +  err = ksmbd_vfs_path_lookup_locked(share_conf, name, flags,
> > > > > > parent_path,
> > > > > > +                                     path);
> > > > > >             if (!err)
> > > > > >                     return 0;
> > > > > >
> > > > > > @@ -1216,10 +1221,10 @@ int ksmbd_vfs_kern_path_locked(struct
> > > > > > ksmbd_work
> > > > > > *work, char *name,
> > > > > >                     path_len = strlen(filepath);
> > > > > >                     remain_len = path_len;
> > > > > >
> > > > > > -          parent_path = share_conf->vfs_path;
> > > > > > -          path_get(&parent_path);
> > > > > > +          *parent_path = share_conf->vfs_path;
> > > > > > +          path_get(parent_path);
> > > > > >
> > > > > > -          while (d_can_lookup(parent_path.dentry)) {
> > > > > > +          while (d_can_lookup(parent_path->dentry)) {
> > > > > >                             char *filename = filepath + path_len - remain_len;
> > > > > >                             char *next = strchrnul(filename, '/');
> > > > > >                             size_t filename_len = next - filename;
> > > > > > @@ -1228,7 +1233,7 @@ int ksmbd_vfs_kern_path_locked(struct ksmbd_work
> > > > > > *work, char *name,
> > > > > >                             if (filename_len == 0)
> > > > > >                                     break;
> > > > > >
> > > > > > -                  err = ksmbd_vfs_lookup_in_dir(&parent_path, filename,
> > > > > > +                  err = ksmbd_vfs_lookup_in_dir(parent_path, filename,
> > > > > >                                                           filename_len,
> > > > > >                                                           work->conn->um);
> > > > > >                             if (err)
> > > > > > @@ -1245,8 +1250,8 @@ int ksmbd_vfs_kern_path_locked(struct ksmbd_work
> > > > > > *work, char *name,
> > > > > >                                     goto out2;
> > > > > >                             else if (is_last)
> > > > > >                                     goto out1;
> > > > > > -                  path_put(&parent_path);
> > > > > > -                  parent_path = *path;
> > > > > > +                  path_put(parent_path);
> > > > > > +                  *parent_path = *path;
> > > > > >
> > > > > >                             next[0] = '/';
> > > > > >                             remain_len -= filename_len + 1;
> > > > > > @@ -1254,16 +1259,17 @@ int ksmbd_vfs_kern_path_locked(struct
> > > > > > ksmbd_work
> > > > > > *work, char *name,
> > > > > >
> > > > > >                     err = -EINVAL;
> > > > > >     out2:
> > > > > > -          path_put(&parent_path);
> > > > > > +          path_put(parent_path);
> > > > > >     out1:
> > > > > >                     kfree(filepath);
> > > > > >             }
> > > > > >
> > > > > >             if (!err) {
> > > > > > -          err = ksmbd_vfs_lock_parent(parent_path.dentry, path->dentry);
> > > > > > -          if (err)
> > > > > > -                  dput(path->dentry);
> > > > > > -          path_put(&parent_path);
> > > > > > +          err = ksmbd_vfs_lock_parent(parent_path->dentry, path->dentry);
> > > > > > +          if (err) {
> > > > > > +                  path_put(path);
> > > > > > +                  path_put(parent_path);
> > > > > > +          }
> > > > > >             }
> > > > > >             return err;
> > > > > >     }
> > > > > > diff --git a/fs/smb/server/vfs.h b/fs/smb/server/vfs.h
> > > > > > index 80039312c255..72f9fb4b48d1 100644
> > > > > > --- a/fs/smb/server/vfs.h
> > > > > > +++ b/fs/smb/server/vfs.h
> > > > > > @@ -115,8 +115,8 @@ int ksmbd_vfs_xattr_stream_name(char *stream_name,
> > > > > > char **xattr_stream_name,
> > > > > >     int ksmbd_vfs_remove_xattr(struct mnt_idmap *idmap,
> > > > > >                                const struct path *path, char *attr_name);
> > > > > >     int ksmbd_vfs_kern_path_locked(struct ksmbd_work *work, char *name,
> > > > > > -                         unsigned int flags, struct path *path,
> > > > > > -                         bool caseless);
> > > > > > +                         unsigned int flags, struct path *parent_path,
> > > > > > +                         struct path *path, bool caseless);
> > > > > >     struct dentry *ksmbd_vfs_kern_path_create(struct ksmbd_work *work,
> > > > > >                                               const char *name,
> > > > > >                                               unsigned int flags,