Skip to content

Commit 74d7970

Browse files
namjaejeonsmfrench
authored andcommitted
ksmbd: fix racy issue from using ->d_parent and ->d_name
Al pointed out that ksmbd has racy issue from using ->d_parent and ->d_name in ksmbd_vfs_unlink and smb2_vfs_rename(). and use new lock_rename_child() to lock stable parent while underlying rename racy. Introduce vfs_path_parent_lookup helper to avoid out of share access and export vfs functions like the following ones to use vfs_path_parent_lookup(). - rename __lookup_hash() to lookup_one_qstr_excl(). - export lookup_one_qstr_excl(). - export getname_kernel() and putname(). vfs_path_parent_lookup() is used for parent lookup of destination file using absolute pathname given from FILE_RENAME_INFORMATION request. Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> Signed-off-by: Steve French <stfrench@microsoft.com>
1 parent af36c51 commit 74d7970

6 files changed

Lines changed: 283 additions & 386 deletions

File tree

fs/ksmbd/smb2pdu.c

Lines changed: 35 additions & 112 deletions
Original file line numberDiff line numberDiff line change
@@ -2408,7 +2408,7 @@ static int smb2_creat(struct ksmbd_work *work, struct path *path, char *name,
24082408
return rc;
24092409
}
24102410

2411-
rc = ksmbd_vfs_kern_path(work, name, 0, path, 0);
2411+
rc = ksmbd_vfs_kern_path_locked(work, name, 0, path, 0);
24122412
if (rc) {
24132413
pr_err("cannot get linux path (%s), err = %d\n",
24142414
name, rc);
@@ -2699,8 +2699,10 @@ int smb2_open(struct ksmbd_work *work)
26992699
goto err_out1;
27002700
}
27012701

2702-
rc = ksmbd_vfs_kern_path(work, name, LOOKUP_NO_SYMLINKS, &path, 1);
2702+
rc = ksmbd_vfs_kern_path_locked(work, name, LOOKUP_NO_SYMLINKS, &path, 1);
27032703
if (!rc) {
2704+
file_present = true;
2705+
27042706
if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) {
27052707
/*
27062708
* If file exists with under flags, return access
@@ -2709,34 +2711,30 @@ int smb2_open(struct ksmbd_work *work)
27092711
if (req->CreateDisposition == FILE_OVERWRITE_IF_LE ||
27102712
req->CreateDisposition == FILE_OPEN_IF_LE) {
27112713
rc = -EACCES;
2712-
path_put(&path);
27132714
goto err_out;
27142715
}
27152716

27162717
if (!test_tree_conn_flag(tcon, KSMBD_TREE_CONN_FLAG_WRITABLE)) {
27172718
ksmbd_debug(SMB,
27182719
"User does not have write permission\n");
27192720
rc = -EACCES;
2720-
path_put(&path);
27212721
goto err_out;
27222722
}
27232723
} else if (d_is_symlink(path.dentry)) {
27242724
rc = -EACCES;
2725-
path_put(&path);
27262725
goto err_out;
27272726
}
2728-
}
27292727

2730-
if (rc) {
2728+
file_present = true;
2729+
idmap = mnt_idmap(path.mnt);
2730+
} else {
27312731
if (rc != -ENOENT)
27322732
goto err_out;
27332733
ksmbd_debug(SMB, "can not get linux path for %s, rc = %d\n",
27342734
name, rc);
27352735
rc = 0;
2736-
} else {
2737-
file_present = true;
2738-
idmap = mnt_idmap(path.mnt);
27392736
}
2737+
27402738
if (stream_name) {
27412739
if (req->CreateOptions & FILE_DIRECTORY_FILE_LE) {
27422740
if (s_type == DATA_STREAM) {
@@ -2864,8 +2862,9 @@ int smb2_open(struct ksmbd_work *work)
28642862

28652863
if ((daccess & FILE_DELETE_LE) ||
28662864
(req->CreateOptions & FILE_DELETE_ON_CLOSE_LE)) {
2867-
rc = ksmbd_vfs_may_delete(idmap,
2868-
path.dentry);
2865+
rc = inode_permission(idmap,
2866+
d_inode(path.dentry->d_parent),
2867+
MAY_EXEC | MAY_WRITE);
28692868
if (rc)
28702869
goto err_out;
28712870
}
@@ -3236,10 +3235,13 @@ int smb2_open(struct ksmbd_work *work)
32363235
}
32373236

32383237
err_out:
3239-
if (file_present || created)
3240-
path_put(&path);
3238+
if (file_present || created) {
3239+
inode_unlock(d_inode(path.dentry->d_parent));
3240+
dput(path.dentry);
3241+
}
32413242
ksmbd_revert_fsids(work);
32423243
err_out1:
3244+
32433245
if (rc) {
32443246
if (rc == -EINVAL)
32453247
rsp->hdr.Status = STATUS_INVALID_PARAMETER;
@@ -5390,44 +5392,19 @@ int smb2_echo(struct ksmbd_work *work)
53905392

53915393
static int smb2_rename(struct ksmbd_work *work,
53925394
struct ksmbd_file *fp,
5393-
struct mnt_idmap *idmap,
53945395
struct smb2_file_rename_info *file_info,
53955396
struct nls_table *local_nls)
53965397
{
53975398
struct ksmbd_share_config *share = fp->tcon->share_conf;
5398-
char *new_name = NULL, *abs_oldname = NULL, *old_name = NULL;
5399-
char *pathname = NULL;
5400-
struct path path;
5401-
bool file_present = true;
5402-
int rc;
5399+
char *new_name = NULL;
5400+
int rc, flags = 0;
54035401

54045402
ksmbd_debug(SMB, "setting FILE_RENAME_INFO\n");
5405-
pathname = kmalloc(PATH_MAX, GFP_KERNEL);
5406-
if (!pathname)
5407-
return -ENOMEM;
5408-
5409-
abs_oldname = file_path(fp->filp, pathname, PATH_MAX);
5410-
if (IS_ERR(abs_oldname)) {
5411-
rc = -EINVAL;
5412-
goto out;
5413-
}
5414-
old_name = strrchr(abs_oldname, '/');
5415-
if (old_name && old_name[1] != '\0') {
5416-
old_name++;
5417-
} else {
5418-
ksmbd_debug(SMB, "can't get last component in path %s\n",
5419-
abs_oldname);
5420-
rc = -ENOENT;
5421-
goto out;
5422-
}
5423-
54245403
new_name = smb2_get_name(file_info->FileName,
54255404
le32_to_cpu(file_info->FileNameLength),
54265405
local_nls);
5427-
if (IS_ERR(new_name)) {
5428-
rc = PTR_ERR(new_name);
5429-
goto out;
5430-
}
5406+
if (IS_ERR(new_name))
5407+
return PTR_ERR(new_name);
54315408

54325409
if (strchr(new_name, ':')) {
54335410
int s_type;
@@ -5453,7 +5430,7 @@ static int smb2_rename(struct ksmbd_work *work,
54535430
if (rc)
54545431
goto out;
54555432

5456-
rc = ksmbd_vfs_setxattr(idmap,
5433+
rc = ksmbd_vfs_setxattr(file_mnt_idmap(fp->filp),
54575434
fp->filp->f_path.dentry,
54585435
xattr_stream_name,
54595436
NULL, 0, 0);
@@ -5468,47 +5445,18 @@ static int smb2_rename(struct ksmbd_work *work,
54685445
}
54695446

54705447
ksmbd_debug(SMB, "new name %s\n", new_name);
5471-
rc = ksmbd_vfs_kern_path(work, new_name, LOOKUP_NO_SYMLINKS, &path, 1);
5472-
if (rc) {
5473-
if (rc != -ENOENT)
5474-
goto out;
5475-
file_present = false;
5476-
} else {
5477-
path_put(&path);
5478-
}
5479-
54805448
if (ksmbd_share_veto_filename(share, new_name)) {
54815449
rc = -ENOENT;
54825450
ksmbd_debug(SMB, "Can't rename vetoed file: %s\n", new_name);
54835451
goto out;
54845452
}
54855453

5486-
if (file_info->ReplaceIfExists) {
5487-
if (file_present) {
5488-
rc = ksmbd_vfs_remove_file(work, new_name);
5489-
if (rc) {
5490-
if (rc != -ENOTEMPTY)
5491-
rc = -EINVAL;
5492-
ksmbd_debug(SMB, "cannot delete %s, rc %d\n",
5493-
new_name, rc);
5494-
goto out;
5495-
}
5496-
}
5497-
} else {
5498-
if (file_present &&
5499-
strncmp(old_name, path.dentry->d_name.name, strlen(old_name))) {
5500-
rc = -EEXIST;
5501-
ksmbd_debug(SMB,
5502-
"cannot rename already existing file\n");
5503-
goto out;
5504-
}
5505-
}
5454+
if (!file_info->ReplaceIfExists)
5455+
flags = RENAME_NOREPLACE;
55065456

5507-
rc = ksmbd_vfs_fp_rename(work, fp, new_name);
5457+
rc = ksmbd_vfs_rename(work, &fp->filp->f_path, new_name, flags);
55085458
out:
5509-
kfree(pathname);
5510-
if (!IS_ERR(new_name))
5511-
kfree(new_name);
5459+
kfree(new_name);
55125460
return rc;
55135461
}
55145462

@@ -5548,18 +5496,17 @@ static int smb2_create_link(struct ksmbd_work *work,
55485496
}
55495497

55505498
ksmbd_debug(SMB, "target name is %s\n", target_name);
5551-
rc = ksmbd_vfs_kern_path(work, link_name, LOOKUP_NO_SYMLINKS, &path, 0);
5499+
rc = ksmbd_vfs_kern_path_locked(work, link_name, LOOKUP_NO_SYMLINKS,
5500+
&path, 0);
55525501
if (rc) {
55535502
if (rc != -ENOENT)
55545503
goto out;
55555504
file_present = false;
5556-
} else {
5557-
path_put(&path);
55585505
}
55595506

55605507
if (file_info->ReplaceIfExists) {
55615508
if (file_present) {
5562-
rc = ksmbd_vfs_remove_file(work, link_name);
5509+
rc = ksmbd_vfs_remove_file(work, &path);
55635510
if (rc) {
55645511
rc = -EINVAL;
55655512
ksmbd_debug(SMB, "cannot delete %s\n",
@@ -5579,6 +5526,10 @@ static int smb2_create_link(struct ksmbd_work *work,
55795526
if (rc)
55805527
rc = -EINVAL;
55815528
out:
5529+
if (file_present) {
5530+
inode_unlock(d_inode(path.dentry->d_parent));
5531+
path_put(&path);
5532+
}
55825533
if (!IS_ERR(link_name))
55835534
kfree(link_name);
55845535
kfree(pathname);
@@ -5756,12 +5707,6 @@ static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp,
57565707
struct smb2_file_rename_info *rename_info,
57575708
unsigned int buf_len)
57585709
{
5759-
struct mnt_idmap *idmap;
5760-
struct ksmbd_file *parent_fp;
5761-
struct dentry *parent;
5762-
struct dentry *dentry = fp->filp->f_path.dentry;
5763-
int ret;
5764-
57655710
if (!(fp->daccess & FILE_DELETE_LE)) {
57665711
pr_err("no right to delete : 0x%x\n", fp->daccess);
57675712
return -EACCES;
@@ -5771,32 +5716,10 @@ static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp,
57715716
le32_to_cpu(rename_info->FileNameLength))
57725717
return -EINVAL;
57735718

5774-
idmap = file_mnt_idmap(fp->filp);
5775-
if (ksmbd_stream_fd(fp))
5776-
goto next;
5777-
5778-
parent = dget_parent(dentry);
5779-
ret = ksmbd_vfs_lock_parent(idmap, parent, dentry);
5780-
if (ret) {
5781-
dput(parent);
5782-
return ret;
5783-
}
5784-
5785-
parent_fp = ksmbd_lookup_fd_inode(d_inode(parent));
5786-
inode_unlock(d_inode(parent));
5787-
dput(parent);
5719+
if (!le32_to_cpu(rename_info->FileNameLength))
5720+
return -EINVAL;
57885721

5789-
if (parent_fp) {
5790-
if (parent_fp->daccess & FILE_DELETE_LE) {
5791-
pr_err("parent dir is opened with delete access\n");
5792-
ksmbd_fd_put(work, parent_fp);
5793-
return -ESHARE;
5794-
}
5795-
ksmbd_fd_put(work, parent_fp);
5796-
}
5797-
next:
5798-
return smb2_rename(work, fp, idmap, rename_info,
5799-
work->conn->local_nls);
5722+
return smb2_rename(work, fp, rename_info, work->conn->local_nls);
58005723
}
58015724

58025725
static int set_file_disposition_info(struct ksmbd_file *fp,

0 commit comments

Comments
 (0)