Hi Al,
2022-02-18 8:41 GMT+09:00, Al Viro <viro@xxxxxxxxxxxxxxxxxx>:
> On Thu, Feb 17, 2022 at 08:03:19AM +0900, Namjae Jeon wrote:
>> Al pointed out that ksmbd has racy issue from using ->d_parent and
>> ->d_name
>> in ksmbd_vfs_unlink and smb2_vfs_rename(). and he suggested changing from
>> the way it start with dget_parent(), which can cause retry loop and
>> unexpected errors, to find the parent of child, lock it and then look for
>> a child in locked directory.
>>
>> This patch introduce a new helper(vfs_path_parent_lookup()) to avoid
>> out of share access and export vfs functions like the following ones to
>> use
>> vfs_path_parent_lookup() and filename_parentat().
>> - __lookup_hash().
>> - getname_kernel() and putname().
>> - filename_parentat()
>
> First of all, your vfs_path_parent_lookup() calling conventions are wrong.
> You have 3 callers:
> err = vfs_path_parent_lookup(share->vfs_path.dentry,
> share->vfs_path.mnt, filename_struct,
> LOOKUP_NO_SYMLINKS | LOOKUP_BENEATH,
> &path, &last, &type);
> err = vfs_path_parent_lookup(share_conf->vfs_path.dentry,
> share_conf->vfs_path.mnt, to,
> lookup_flags | LOOKUP_BENEATH,
> &new_path, &new_last, &new_type);
> err = vfs_path_parent_lookup(share->vfs_path.dentry,
> share->vfs_path.mnt, filename_struct,
> LOOKUP_NO_SYMLINKS | LOOKUP_BENEATH,
> &path, &last, &type);
> Note that in all of them the first two arguments come from ->dentry and
> ->mnt of the same struct path instance. Now, look at the function itself:
>
> int vfs_path_parent_lookup(struct dentry *dentry, struct vfsmount *mnt,
> struct filename *filename, unsigned int flags,
> struct path *parent, struct qstr *last, int *type)
> {
> struct path root = {.mnt = mnt, .dentry = dentry};
>
> return __filename_parentat(AT_FDCWD, filename, flags, parent, last,
> type, &root);
> }
>
> What about the __filename_parentat() last argument? It's declared as
> struct path *root and passed to set_nameidata(). No other uses. And
> set_nameidata() gets it via const struct path *root argument. IOW,
> it's not going to modify the contents of that struct path. Since
> you __filename_parentat() doesn't do anything else with its root
> argument, there's no reason not to make _that_ const struct path *,
> is there?
Yep, No reason.
>
> Now, if you do that, you can safely turn vfs_path_parent_lookup()
> take const struct path * instead of dentry/vfsmount pair of arguments
> and drop the local struct path instance in the vfs_path_parent_lookup()
> itself.
Yes. I will change it.
>
> The fact that vfs_path_lookup() passes vfsmount and dentry separately
> doesn't mean you need to do the same - look at the existing callers
> of vfs_path_lookup() (outside of ksmbd itself) and you'll see the
> difference. Incidentally, this
> fs/ksmbd/vfs.c:22:#include "../internal.h" /* for vfs_path_lookup */
> had been a really bad idea. And no, nfsd doing the same is not a good
> thing either...
>
> General rule: if it's exported, it's *NOT* internal.
Okay. Then as another patch, I will move vfs_path_lookup prototype in
internal.h to linux/namei.h.
>
>
> Next:
>
>> index 077b8761d099..b094cd1d4951 100644
>> --- a/fs/ksmbd/oplock.c
>> +++ b/fs/ksmbd/oplock.c
>> @@ -1713,11 +1713,14 @@ int smb2_check_durable_oplock(struct ksmbd_file
>> *fp,
>> ret = -EBADF;
>> goto out;
>> }
>> + down_read(&fp->filename_lock);
>> if (name && strcmp(fp->filename, name)) {
>> + up_read(&fp->filename_lock);
>> pr_err("invalid name reconnect %s\n", name);
>> ret = -EINVAL;
>> goto out;
>> }
>> + up_read(&fp->filename_lock);
>
> What assumptions do you make about those strings? Note that opened file
> is *NOT* guaranteed to have its pathname remain unchanged - having
> /tmp/foo/bar/baz/blah opened will not prevent mv /tmp/foo /tmp/barf
> and the file will remain opened (and working just fine). AFAICS, you
> only update it in smb2_rename(), which is not going to be called by
> mv(1) called by admin on server.
Whenever a FILE_ALL_INFORMATION request is received from a client,
ksmbd need to call d_path(then, removing the share path in pathname is
required) to obtain pathname for windows. To avoid the issue you
mentioned, we can remove the all uses of ->filename and calling
d_path() whenever pathname is need.
>
> BTW, while grepping through the related code, convert_to_nt_pathname()
> is Not Nice(tm). Seriously, strlen(s) == 0 is not an idiomatic way to
> check that s is an empty string. What's more, return value of that
> function ends up passed to kfree(). Which is not a good thing to do
> to a string constant. That can be recovered by use of kfree_const() in
> get_file_all_info(), but.. ouch.
Okay.
>
> ksmbd_vfs_rename(): UGH.
> * you allocate a buffer
> * do d_path() into it
> * then use getname_kernel() to allocate another one and copy the contents
> into it. By that point the string might have nothing to do with the actual
> location of object, BTW (see above)
Can we use dget_parent() and take_dentry_name_snapshot() for source
file instead of d_path(), getname_kernel() and filename_parentat()?
Because ksmbd receive fileid(ksmbd_file) for source file from client.
[See control flow the below]
> * then you use filename_parentat() (BTW, the need to export both it and
> vfs_path_parent_lookup() is an artefact of bad calling conventions -
> passing
> NULL as const struct path * would do the right thing, if not for the fact
> that
> with your calling conventions you have to pass a non-NULL pointer - that to
> a local struct path in your vfs_path_parent_lookup()).
Okay.
> * then you use vfs_path_parent_lookup() to find the new parent. OK,
> but...
> you proceed to check if it has somehow returned you a symlink. Huh? How
> does
> one get a symlink from path_parentat() or anything that would use it?
As security issues, We made it not to allow symlinks.
> I would very much appreciate a reproducer for that.
> * you use lock_rename() to lock both parents. Which relies upon the
> caller having checked that they live on the same filesystem. Neither old
> nor
> new version do that, which means relatively easy deadlocks.
Okay. will add the check for this.
> * look the last components up. NB: the old one might very well have
> nothing to do with the path.dentry.
Okay.
> * do usual checks re loop prevention (with slightly unusual error
> values, but whatever)
I understood that you pointed out to add retry_estale() check and retry.
> * call ksmbd_lookup_fd_inode() on the old parent. Then dereference
> the return value (if non-NULL)... and never do anything else to it. How
> can
> that possibly work? What's there to prevent freeing of that struct
> ksmbd_file
> just as ksmbd_lookup_fd_inode() returns it? Looks like it's either a leak
> or
> use-after-free, and looking at ksmbd_lookup_fd_inode() it's probably the
> latter.
Right. Need to increase reference count of ksmbd_file. I will fix it.
> * proceed with vfs_rename(), drop the stuff you'd acquired and go
> away.
Okay.
>
> ksmbd_vfs_unlink():
> * const char *filename, please, unless you really modify it there.
Okay.
> * what the hell is that ihold/iput pair for?
I refered codes in do_unlinkat() for this. If this pair is not needed,
I'll delete it,
but could you please tell me why it's needed in unlinkat() ?
>
> I'm not sure that the set you'd exported is the right one, but that's
> secondary - I'd really like to understand what assumptions are you
> making about the ->filename contents, as well as the control flow
> from protocol request that demands rename to the actual call of
> vfs_rename(). Could you elaborate on that? I am not familiar with
> the protocol, other than bits and pieces I'd observed in fs/cifs
> code.
As I said above, the uses of ->filename can be replaced with d_path().
control flow for rename is the following.
a. Receiving smb2 create(open) request from client.
- Getting struct file after calling vfs_path_lookup() and
dentry_open() using pathname from client.
- create ksmbd_file and add struct file to fp->filp and generate
fileid for struct ksmbd_file.
- send smb2 create response included fileid of ksmbd_file to client.
b. Receiving smb2_set_info file(FILE_RENAME_INFORMATION) request from client.
- lookup ksmbd_file using fileid of request. This will source
file for rename.
- get absolute pathname for destination fille in smb2 set info
file for rename.
- find both parents of source and destination and lock and do rename...
c. Receiving smb2 close.
Thanks for your review!
>
