* [PATCH v4] ksmbd: use LOOKUP_BENEATH to prevent the out of share access
@ 2021-09-24 15:06 Hyunchul Lee
2021-09-24 17:20 ` Jeremy Allison
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Hyunchul Lee @ 2021-09-24 15:06 UTC (permalink / raw)
To: linux-cifs
Cc: Hyunchul Lee, Ronnie Sahlberg, Ralph Boehme, Steve French, Namjae Jeon
instead of removing '..' in a given path, call
kern_path with LOOKUP_BENEATH flag to prevent
the out of share access.
ran various test on this:
smb2-cat-async smb://127.0.0.1/homes/../out_of_share
smb2-cat-async smb://127.0.0.1/homes/foo/../../out_of_share
smbclient //127.0.0.1/homes -c "mkdir ../foo2"
smbclient //127.0.0.1/homes -c "rename bar ../bar"
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Boehme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Cc: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
---
Changes from v1:
- pass path of file that is relative to share to ksmbd vfs functions.
Changes from v2:
- fix smbtorture smb2.streams.rename, smb2.streams.rename2 failure.
Changes from v3:
- fix uninitialized variable free in ksmbd_vfs_kern_path.
fs/ksmbd/misc.c | 100 ++++++-----------------------
fs/ksmbd/misc.h | 7 +-
fs/ksmbd/smb2pdu.c | 74 ++++++++-------------
fs/ksmbd/vfs.c | 156 ++++++++++++++++++++++++---------------------
fs/ksmbd/vfs.h | 9 ++-
5 files changed, 140 insertions(+), 206 deletions(-)
diff --git a/fs/ksmbd/misc.c b/fs/ksmbd/misc.c
index 3eac3c01749f..6a19f4bc692d 100644
--- a/fs/ksmbd/misc.c
+++ b/fs/ksmbd/misc.c
@@ -158,25 +158,21 @@ int parse_stream_name(char *filename, char **stream_name, int *s_type)
* Return : windows path string or error
*/
-char *convert_to_nt_pathname(char *filename, char *sharepath)
+char *convert_to_nt_pathname(char *filename)
{
char *ab_pathname;
- int len, name_len;
- name_len = strlen(filename);
- ab_pathname = kmalloc(name_len, GFP_KERNEL);
- if (!ab_pathname)
- return NULL;
-
- ab_pathname[0] = '\\';
- ab_pathname[1] = '\0';
+ if (strlen(filename) == 0) {
+ ab_pathname = kmalloc(2, GFP_KERNEL);
+ ab_pathname[0] = '\\';
+ ab_pathname[1] = '\0';
+ } else {
+ ab_pathname = kstrdup(filename, GFP_KERNEL);
+ if (!ab_pathname)
+ return NULL;
- len = strlen(sharepath);
- if (!strncmp(filename, sharepath, len) && name_len != len) {
- strscpy(ab_pathname, &filename[len], name_len);
ksmbd_conv_path_to_windows(ab_pathname);
}
-
return ab_pathname;
}
@@ -191,77 +187,19 @@ int get_nlink(struct kstat *st)
return nlink;
}
-char *ksmbd_conv_path_to_unix(char *path)
+void ksmbd_conv_path_to_unix(char *path)
{
- size_t path_len, remain_path_len, out_path_len;
- char *out_path, *out_next;
- int i, pre_dotdot_cnt = 0, slash_cnt = 0;
- bool is_last;
-
strreplace(path, '\\', '/');
- path_len = strlen(path);
- remain_path_len = path_len;
- if (path_len == 0)
- return ERR_PTR(-EINVAL);
-
- out_path = kzalloc(path_len + 2, GFP_KERNEL);
- if (!out_path)
- return ERR_PTR(-ENOMEM);
- out_path_len = 0;
- out_next = out_path;
-
- do {
- char *name = path + path_len - remain_path_len;
- char *next = strchrnul(name, '/');
- size_t name_len = next - name;
-
- is_last = !next[0];
- if (name_len == 2 && name[0] == '.' && name[1] == '.') {
- pre_dotdot_cnt++;
- /* handle the case that path ends with "/.." */
- if (is_last)
- goto follow_dotdot;
- } else {
- if (pre_dotdot_cnt) {
-follow_dotdot:
- slash_cnt = 0;
- for (i = out_path_len - 1; i >= 0; i--) {
- if (out_path[i] == '/' &&
- ++slash_cnt == pre_dotdot_cnt + 1)
- break;
- }
-
- if (i < 0 &&
- slash_cnt != pre_dotdot_cnt) {
- kfree(out_path);
- return ERR_PTR(-EINVAL);
- }
-
- out_next = &out_path[i+1];
- *out_next = '\0';
- out_path_len = i + 1;
-
- }
-
- if (name_len != 0 &&
- !(name_len == 1 && name[0] == '.') &&
- !(name_len == 2 && name[0] == '.' && name[1] == '.')) {
- next[0] = '\0';
- sprintf(out_next, "%s/", name);
- out_next += name_len + 1;
- out_path_len += name_len + 1;
- next[0] = '/';
- }
- pre_dotdot_cnt = 0;
- }
+}
- remain_path_len -= name_len + 1;
- } while (!is_last);
+void ksmbd_strip_last_slash(char *path)
+{
+ int len = strlen(path);
- if (out_path_len > 0)
- out_path[out_path_len-1] = '\0';
- path[path_len] = '\0';
- return out_path;
+ while (len && path[len - 1] == '/') {
+ path[len - 1] = '\0';
+ len--;
+ }
}
void ksmbd_conv_path_to_windows(char *path)
@@ -298,7 +236,7 @@ char *ksmbd_extract_sharename(char *treename)
*
* Return: converted name on success, otherwise NULL
*/
-char *convert_to_unix_name(struct ksmbd_share_config *share, char *name)
+char *convert_to_unix_name(struct ksmbd_share_config *share, const char *name)
{
int no_slash = 0, name_len, path_len;
char *new_name;
diff --git a/fs/ksmbd/misc.h b/fs/ksmbd/misc.h
index b7b10139ada2..253366bd0951 100644
--- a/fs/ksmbd/misc.h
+++ b/fs/ksmbd/misc.h
@@ -14,12 +14,13 @@ struct ksmbd_file;
int match_pattern(const char *str, size_t len, const char *pattern);
int ksmbd_validate_filename(char *filename);
int parse_stream_name(char *filename, char **stream_name, int *s_type);
-char *convert_to_nt_pathname(char *filename, char *sharepath);
+char *convert_to_nt_pathname(char *filename);
int get_nlink(struct kstat *st);
-char *ksmbd_conv_path_to_unix(char *path);
+void ksmbd_conv_path_to_unix(char *path);
+void ksmbd_strip_last_slash(char *path);
void ksmbd_conv_path_to_windows(char *path);
char *ksmbd_extract_sharename(char *treename);
-char *convert_to_unix_name(struct ksmbd_share_config *share, char *name);
+char *convert_to_unix_name(struct ksmbd_share_config *share, const char *name);
#define KSMBD_DIR_INFO_ALIGNMENT 8
struct ksmbd_dir_info;
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 378e0b4a216d..4c799fef9883 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -643,7 +643,7 @@ static char *
smb2_get_name(struct ksmbd_share_config *share, const char *src,
const int maxlen, struct nls_table *local_nls)
{
- char *name, *norm_name, *unixname;
+ char *name;
name = smb_strndup_from_utf16(src, maxlen, 1, local_nls);
if (IS_ERR(name)) {
@@ -651,23 +651,9 @@ smb2_get_name(struct ksmbd_share_config *share, const char *src,
return name;
}
- /* change it to absolute unix name */
- norm_name = ksmbd_conv_path_to_unix(name);
- if (IS_ERR(norm_name)) {
- kfree(name);
- return norm_name;
- }
- kfree(name);
-
- unixname = convert_to_unix_name(share, norm_name);
- kfree(norm_name);
- if (!unixname) {
- pr_err("can not convert absolute name\n");
- return ERR_PTR(-ENOMEM);
- }
-
- ksmbd_debug(SMB, "absolute name = %s\n", unixname);
- return unixname;
+ ksmbd_conv_path_to_unix(name);
+ ksmbd_strip_last_slash(name);
+ return name;
}
int setup_async_work(struct ksmbd_work *work, void (*fn)(void **), void **arg)
@@ -2412,7 +2398,7 @@ static int smb2_creat(struct ksmbd_work *work, struct path *path, char *name,
return rc;
}
- rc = ksmbd_vfs_kern_path(name, 0, path, 0);
+ rc = ksmbd_vfs_kern_path(work, name, 0, path, 0);
if (rc) {
pr_err("cannot get linux path (%s), err = %d\n",
name, rc);
@@ -2487,7 +2473,7 @@ int smb2_open(struct ksmbd_work *work)
struct oplock_info *opinfo;
__le32 *next_ptr = NULL;
int req_op_level = 0, open_flags = 0, may_flags = 0, file_info = 0;
- int rc = 0, len = 0;
+ int rc = 0;
int contxt_cnt = 0, query_disk_id = 0;
int maximal_access_ctxt = 0, posix_ctxt = 0;
int s_type = 0;
@@ -2559,17 +2545,11 @@ int smb2_open(struct ksmbd_work *work)
goto err_out1;
}
} else {
- len = strlen(share->path);
- ksmbd_debug(SMB, "share path len %d\n", len);
- name = kmalloc(len + 1, GFP_KERNEL);
+ name = kstrdup("", GFP_KERNEL);
if (!name) {
- rsp->hdr.Status = STATUS_NO_MEMORY;
rc = -ENOMEM;
goto err_out1;
}
-
- memcpy(name, share->path, len);
- *(name + len) = '\0';
}
req_op_level = req->RequestedOplockLevel;
@@ -2692,7 +2672,7 @@ int smb2_open(struct ksmbd_work *work)
goto err_out1;
}
- rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1);
+ rc = ksmbd_vfs_kern_path(work, name, LOOKUP_NO_SYMLINKS, &path, 1);
if (!rc) {
if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) {
/*
@@ -2721,11 +2701,8 @@ int smb2_open(struct ksmbd_work *work)
}
if (rc) {
- if (rc == -EACCES) {
- ksmbd_debug(SMB,
- "User does not have right permission\n");
+ if (rc != -ENOENT)
goto err_out;
- }
ksmbd_debug(SMB, "can not get linux path for %s, rc = %d\n",
name, rc);
rc = 0;
@@ -3229,7 +3206,7 @@ int smb2_open(struct ksmbd_work *work)
rsp->hdr.Status = STATUS_INVALID_PARAMETER;
else if (rc == -EOPNOTSUPP)
rsp->hdr.Status = STATUS_NOT_SUPPORTED;
- else if (rc == -EACCES || rc == -ESTALE)
+ else if (rc == -EACCES || rc == -ESTALE || rc == -EXDEV)
rsp->hdr.Status = STATUS_ACCESS_DENIED;
else if (rc == -ENOENT)
rsp->hdr.Status = STATUS_OBJECT_NAME_INVALID;
@@ -4345,8 +4322,7 @@ static int get_file_all_info(struct ksmbd_work *work,
return -EACCES;
}
- filename = convert_to_nt_pathname(fp->filename,
- work->tcon->share_conf->path);
+ filename = convert_to_nt_pathname(fp->filename);
if (!filename)
return -ENOMEM;
@@ -4801,7 +4777,7 @@ static int smb2_get_info_filesystem(struct ksmbd_work *work,
int rc = 0, len;
int fs_infoclass_size = 0;
- rc = ksmbd_vfs_kern_path(share->path, LOOKUP_NO_SYMLINKS, &path, 0);
+ rc = kern_path(share->path, LOOKUP_NO_SYMLINKS, &path);
if (rc) {
pr_err("cannot create vfs path\n");
return -EIO;
@@ -5350,7 +5326,7 @@ static int smb2_rename(struct ksmbd_work *work,
goto out;
len = strlen(new_name);
- if (new_name[len - 1] != '/') {
+ if (len > 0 && new_name[len - 1] != '/') {
pr_err("not allow base filename in rename\n");
rc = -ESHARE;
goto out;
@@ -5378,11 +5354,14 @@ static int smb2_rename(struct ksmbd_work *work,
}
ksmbd_debug(SMB, "new name %s\n", new_name);
- rc = ksmbd_vfs_kern_path(new_name, LOOKUP_NO_SYMLINKS, &path, 1);
- if (rc)
+ rc = ksmbd_vfs_kern_path(work, new_name, LOOKUP_NO_SYMLINKS, &path, 1);
+ if (rc) {
+ if (rc != -ENOENT)
+ goto out;
file_present = false;
- else
+ } else {
path_put(&path);
+ }
if (ksmbd_share_veto_filename(share, new_name)) {
rc = -ENOENT;
@@ -5456,11 +5435,14 @@ static int smb2_create_link(struct ksmbd_work *work,
}
ksmbd_debug(SMB, "target name is %s\n", target_name);
- rc = ksmbd_vfs_kern_path(link_name, LOOKUP_NO_SYMLINKS, &path, 0);
- if (rc)
+ rc = ksmbd_vfs_kern_path(work, link_name, LOOKUP_NO_SYMLINKS, &path, 0);
+ if (rc) {
+ if (rc != -ENOENT)
+ goto out;
file_present = false;
- else
+ } else {
path_put(&path);
+ }
if (file_info->ReplaceIfExists) {
if (file_present) {
@@ -5618,7 +5600,7 @@ static int set_file_allocation_info(struct ksmbd_work *work,
* inode size is retained by backup inode size.
*/
size = i_size_read(inode);
- rc = ksmbd_vfs_truncate(work, NULL, fp, alloc_blks * 512);
+ rc = ksmbd_vfs_truncate(work, fp, alloc_blks * 512);
if (rc) {
pr_err("truncate failed! filename : %s, err %d\n",
fp->filename, rc);
@@ -5653,7 +5635,7 @@ static int set_end_of_file_info(struct ksmbd_work *work, struct ksmbd_file *fp,
if (inode->i_sb->s_magic != MSDOS_SUPER_MAGIC) {
ksmbd_debug(SMB, "filename : %s truncated to newsize %lld\n",
fp->filename, newsize);
- rc = ksmbd_vfs_truncate(work, NULL, fp, newsize);
+ rc = ksmbd_vfs_truncate(work, fp, newsize);
if (rc) {
ksmbd_debug(SMB, "truncate failed! filename : %s err %d\n",
fp->filename, rc);
@@ -5975,7 +5957,7 @@ int smb2_set_info(struct ksmbd_work *work)
return 0;
err_out:
- if (rc == -EACCES || rc == -EPERM)
+ if (rc == -EACCES || rc == -EPERM || rc == -EXDEV)
rsp->hdr.Status = STATUS_ACCESS_DENIED;
else if (rc == -EINVAL)
rsp->hdr.Status = STATUS_INVALID_PARAMETER;
diff --git a/fs/ksmbd/vfs.c b/fs/ksmbd/vfs.c
index 3733e4944c1d..b41954294d38 100644
--- a/fs/ksmbd/vfs.c
+++ b/fs/ksmbd/vfs.c
@@ -19,6 +19,8 @@
#include <linux/sched/xacct.h>
#include <linux/crc32c.h>
+#include "../internal.h" /* for vfs_path_lookup */
+
#include "glob.h"
#include "oplock.h"
#include "connection.h"
@@ -44,7 +46,6 @@ static char *extract_last_component(char *path)
p++;
} else {
p = NULL;
- pr_err("Invalid path %s\n", path);
}
return p;
}
@@ -155,7 +156,7 @@ int ksmbd_vfs_query_maximal_access(struct user_namespace *user_ns,
/**
* ksmbd_vfs_create() - vfs helper for smb create file
* @work: work
- * @name: file name
+ * @name: file name that is relative to share
* @mode: file create mode
*
* Return: 0 on success, otherwise error
@@ -166,7 +167,8 @@ int ksmbd_vfs_create(struct ksmbd_work *work, const char *name, umode_t mode)
struct dentry *dentry;
int err;
- dentry = kern_path_create(AT_FDCWD, name, &path, LOOKUP_NO_SYMLINKS);
+ dentry = ksmbd_vfs_kern_path_create(work, name,
+ LOOKUP_NO_SYMLINKS, &path);
if (IS_ERR(dentry)) {
err = PTR_ERR(dentry);
if (err != -ENOENT)
@@ -191,7 +193,7 @@ int ksmbd_vfs_create(struct ksmbd_work *work, const char *name, umode_t mode)
/**
* ksmbd_vfs_mkdir() - vfs helper for smb create directory
* @work: work
- * @name: directory name
+ * @name: directory name that is relative to share
* @mode: directory create mode
*
* Return: 0 on success, otherwise error
@@ -203,8 +205,9 @@ int ksmbd_vfs_mkdir(struct ksmbd_work *work, const char *name, umode_t mode)
struct dentry *dentry;
int err;
- dentry = kern_path_create(AT_FDCWD, name, &path,
- LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY);
+ dentry = ksmbd_vfs_kern_path_create(work, name,
+ LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY,
+ &path);
if (IS_ERR(dentry)) {
err = PTR_ERR(dentry);
if (err != -EEXIST)
@@ -579,7 +582,7 @@ int ksmbd_vfs_fsync(struct ksmbd_work *work, u64 fid, u64 p_id)
/**
* ksmbd_vfs_remove_file() - vfs helper for smb rmdir or unlink
- * @name: absolute directory or file name
+ * @name: directory or file name that is relative to share
*
* Return: 0 on success, otherwise error
*/
@@ -593,7 +596,7 @@ int ksmbd_vfs_remove_file(struct ksmbd_work *work, char *name)
if (ksmbd_override_fsids(work))
return -ENOMEM;
- err = kern_path(name, LOOKUP_NO_SYMLINKS, &path);
+ err = ksmbd_vfs_kern_path(work, name, LOOKUP_NO_SYMLINKS, &path, false);
if (err) {
ksmbd_debug(VFS, "can't get %s, err %d\n", name, err);
ksmbd_revert_fsids(work);
@@ -638,7 +641,7 @@ int ksmbd_vfs_remove_file(struct ksmbd_work *work, char *name)
/**
* ksmbd_vfs_link() - vfs helper for creating smb hardlink
* @oldname: source file name
- * @newname: hardlink name
+ * @newname: hardlink name that is relative to share
*
* Return: 0 on success, otherwise error
*/
@@ -659,8 +662,9 @@ int ksmbd_vfs_link(struct ksmbd_work *work, const char *oldname,
goto out1;
}
- dentry = kern_path_create(AT_FDCWD, newname, &newpath,
- LOOKUP_NO_SYMLINKS | LOOKUP_REVAL);
+ dentry = ksmbd_vfs_kern_path_create(work, newname,
+ LOOKUP_NO_SYMLINKS | LOOKUP_REVAL,
+ &newpath);
if (IS_ERR(dentry)) {
err = PTR_ERR(dentry);
pr_err("path create err for %s, err %d\n", newname, err);
@@ -781,14 +785,17 @@ int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
int err;
dst_name = extract_last_component(newname);
- if (!dst_name)
- return -EINVAL;
+ if (!dst_name) {
+ dst_name = newname;
+ newname = "";
+ }
src_dent_parent = dget_parent(fp->filp->f_path.dentry);
src_dent = fp->filp->f_path.dentry;
- err = kern_path(newname, LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY,
- &dst_path);
+ err = ksmbd_vfs_kern_path(work, newname,
+ LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY,
+ &dst_path, false);
if (err) {
ksmbd_debug(VFS, "Cannot get path for %s [%d]\n", newname, err);
goto out;
@@ -834,61 +841,43 @@ int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
/**
* ksmbd_vfs_truncate() - vfs helper for smb file truncate
* @work: work
- * @name: old filename
* @fid: file id of old file
* @size: truncate to given size
*
* Return: 0 on success, otherwise error
*/
-int ksmbd_vfs_truncate(struct ksmbd_work *work, const char *name,
+int ksmbd_vfs_truncate(struct ksmbd_work *work,
struct ksmbd_file *fp, loff_t size)
{
- struct path path;
int err = 0;
+ struct file *filp;
- if (name) {
- err = kern_path(name, LOOKUP_NO_SYMLINKS, &path);
- if (err) {
- pr_err("cannot get linux path for %s, err %d\n",
- name, err);
- return err;
- }
- err = vfs_truncate(&path, size);
- if (err)
- pr_err("truncate failed for %s err %d\n",
- name, err);
- path_put(&path);
- } else {
- struct file *filp;
-
- filp = fp->filp;
-
- /* Do we need to break any of a levelII oplock? */
- smb_break_all_levII_oplock(work, fp, 1);
+ filp = fp->filp;
- if (!work->tcon->posix_extensions) {
- struct inode *inode = file_inode(filp);
+ /* Do we need to break any of a levelII oplock? */
+ smb_break_all_levII_oplock(work, fp, 1);
- if (size < inode->i_size) {
- err = check_lock_range(filp, size,
- inode->i_size - 1, WRITE);
- } else {
- err = check_lock_range(filp, inode->i_size,
- size - 1, WRITE);
- }
+ if (!work->tcon->posix_extensions) {
+ struct inode *inode = file_inode(filp);
- if (err) {
- pr_err("failed due to lock\n");
- return -EAGAIN;
- }
+ if (size < inode->i_size) {
+ err = check_lock_range(filp, size,
+ inode->i_size - 1, WRITE);
+ } else {
+ err = check_lock_range(filp, inode->i_size,
+ size - 1, WRITE);
}
- err = vfs_truncate(&filp->f_path, size);
- if (err)
- pr_err("truncate failed for filename : %s err %d\n",
- fp->filename, err);
+ if (err) {
+ pr_err("failed due to lock\n");
+ return -EAGAIN;
+ }
}
+ err = vfs_truncate(&filp->f_path, size);
+ if (err)
+ pr_err("truncate failed for filename : %s err %d\n",
+ fp->filename, err);
return err;
}
@@ -1206,22 +1195,25 @@ static int ksmbd_vfs_lookup_in_dir(struct path *dir, char *name, size_t namelen)
/**
* ksmbd_vfs_kern_path() - lookup a file and get path info
- * @name: name of file for lookup
+ * @name: file path that is relative to share
* @flags: lookup flags
* @path: if lookup succeed, return path info
* @caseless: caseless filename lookup
*
* Return: 0 on success, otherwise error
*/
-int ksmbd_vfs_kern_path(char *name, unsigned int flags, struct path *path,
- bool caseless)
+int ksmbd_vfs_kern_path(struct ksmbd_work *work, char *name,
+ unsigned int flags, struct path *path, bool caseless)
{
+ struct ksmbd_share_config *share_conf = work->tcon->share_conf;
int err;
- if (name[0] != '/')
- return -EINVAL;
-
- err = kern_path(name, flags, path);
+ flags |= LOOKUP_BENEATH;
+ err = vfs_path_lookup(share_conf->vfs_path.dentry,
+ share_conf->vfs_path.mnt,
+ name,
+ flags,
+ path);
if (!err)
return 0;
@@ -1235,11 +1227,10 @@ int ksmbd_vfs_kern_path(char *name, unsigned int flags, struct path *path,
return -ENOMEM;
path_len = strlen(filepath);
- remain_len = path_len - 1;
+ remain_len = path_len;
- err = kern_path("/", flags, &parent);
- if (err)
- goto out;
+ parent = share_conf->vfs_path;
+ path_get(&parent);
while (d_can_lookup(parent.dentry)) {
char *filename = filepath + path_len - remain_len;
@@ -1252,21 +1243,21 @@ int ksmbd_vfs_kern_path(char *name, unsigned int flags, struct path *path,
err = ksmbd_vfs_lookup_in_dir(&parent, filename,
filename_len);
- if (err) {
- path_put(&parent);
+ path_put(&parent);
+ if (err)
goto out;
- }
- path_put(&parent);
next[0] = '\0';
- err = kern_path(filepath, flags, &parent);
+ err = vfs_path_lookup(share_conf->vfs_path.dentry,
+ share_conf->vfs_path.mnt,
+ filepath,
+ flags,
+ &parent);
if (err)
goto out;
-
- if (is_last) {
- path->mnt = parent.mnt;
- path->dentry = parent.dentry;
+ else if (is_last) {
+ *path = parent;
goto out;
}
@@ -1282,6 +1273,23 @@ int ksmbd_vfs_kern_path(char *name, unsigned int flags, struct path *path,
return err;
}
+struct dentry *ksmbd_vfs_kern_path_create(struct ksmbd_work *work,
+ const char *name,
+ unsigned int flags,
+ struct path *path)
+{
+ char *abs_name;
+ struct dentry *dent;
+
+ abs_name = convert_to_unix_name(work->tcon->share_conf, name);
+ if (!abs_name)
+ return ERR_PTR(-ENOMEM);
+
+ dent = kern_path_create(AT_FDCWD, abs_name, path, flags);
+ kfree(abs_name);
+ return dent;
+}
+
int ksmbd_vfs_remove_acl_xattrs(struct user_namespace *user_ns,
struct dentry *dentry)
{
diff --git a/fs/ksmbd/vfs.h b/fs/ksmbd/vfs.h
index 85db50abdb24..7b1dcaa3fbdc 100644
--- a/fs/ksmbd/vfs.h
+++ b/fs/ksmbd/vfs.h
@@ -126,7 +126,7 @@ int ksmbd_vfs_link(struct ksmbd_work *work,
int ksmbd_vfs_getattr(struct path *path, struct kstat *stat);
int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
char *newname);
-int ksmbd_vfs_truncate(struct ksmbd_work *work, const char *name,
+int ksmbd_vfs_truncate(struct ksmbd_work *work,
struct ksmbd_file *fp, loff_t size);
struct srv_copychunk;
int ksmbd_vfs_copy_file_ranges(struct ksmbd_work *work,
@@ -152,8 +152,13 @@ int ksmbd_vfs_xattr_stream_name(char *stream_name, char **xattr_stream_name,
size_t *xattr_stream_name_size, int s_type);
int ksmbd_vfs_remove_xattr(struct user_namespace *user_ns,
struct dentry *dentry, char *attr_name);
-int ksmbd_vfs_kern_path(char *name, unsigned int flags, struct path *path,
+int ksmbd_vfs_kern_path(struct ksmbd_work *work,
+ char *name, unsigned int flags, struct path *path,
bool caseless);
+struct dentry *ksmbd_vfs_kern_path_create(struct ksmbd_work *work,
+ const char *name,
+ unsigned int flags,
+ struct path *path);
int ksmbd_vfs_empty_dir(struct ksmbd_file *fp);
void ksmbd_vfs_set_fadvise(struct file *filp, __le32 option);
int ksmbd_vfs_zero_data(struct ksmbd_work *work, struct ksmbd_file *fp,
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v4] ksmbd: use LOOKUP_BENEATH to prevent the out of share access
2021-09-24 15:06 [PATCH v4] ksmbd: use LOOKUP_BENEATH to prevent the out of share access Hyunchul Lee
@ 2021-09-24 17:20 ` Jeremy Allison
2021-09-25 0:08 ` Namjae Jeon
2021-09-25 0:43 ` Namjae Jeon
2021-09-28 15:18 ` Tom Talpey
2 siblings, 1 reply; 11+ messages in thread
From: Jeremy Allison @ 2021-09-24 17:20 UTC (permalink / raw)
To: Hyunchul Lee
Cc: linux-cifs, Ronnie Sahlberg, Ralph Boehme, Steve French, Namjae Jeon
On Sat, Sep 25, 2021 at 12:06:16AM +0900, Hyunchul Lee wrote:
>instead of removing '..' in a given path, call
>kern_path with LOOKUP_BENEATH flag to prevent
>the out of share access.
>
>ran various test on this:
>smb2-cat-async smb://127.0.0.1/homes/../out_of_share
>smb2-cat-async smb://127.0.0.1/homes/foo/../../out_of_share
>smbclient //127.0.0.1/homes -c "mkdir ../foo2"
>smbclient //127.0.0.1/homes -c "rename bar ../bar"
FYI, MS-FSCC states:
"Except where explicitly permitted, a pathname component that is a dot directory name MUST NOT
be sent over the wire."
so it might be easier to just refuse with an
error a pathname containing "." or ".." on input
processing rather than try and deal with it.
Might be interesting to test this against a
Windows server and see what it does here.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] ksmbd: use LOOKUP_BENEATH to prevent the out of share access
2021-09-24 17:20 ` Jeremy Allison
@ 2021-09-25 0:08 ` Namjae Jeon
2021-09-25 0:21 ` Namjae Jeon
0 siblings, 1 reply; 11+ messages in thread
From: Namjae Jeon @ 2021-09-25 0:08 UTC (permalink / raw)
To: Jeremy Allison
Cc: Hyunchul Lee, linux-cifs, Ronnie Sahlberg, Ralph Boehme, Steve French
2021-09-25 2:20 GMT+09:00, Jeremy Allison <jra@samba.org>:
> On Sat, Sep 25, 2021 at 12:06:16AM +0900, Hyunchul Lee wrote:
>>instead of removing '..' in a given path, call
>>kern_path with LOOKUP_BENEATH flag to prevent
>>the out of share access.
>>
>>ran various test on this:
>>smb2-cat-async smb://127.0.0.1/homes/../out_of_share
>>smb2-cat-async smb://127.0.0.1/homes/foo/../../out_of_share
>>smbclient //127.0.0.1/homes -c "mkdir ../foo2"
>>smbclient //127.0.0.1/homes -c "rename bar ../bar"
>
> FYI, MS-FSCC states:
>
> "Except where explicitly permitted, a pathname component that is a dot
> directory name MUST NOT
> be sent over the wire."
>
> so it might be easier to just refuse with an
> error a pathname containing "." or ".." on input
> processing rather than try and deal with it.
>
> Might be interesting to test this against a
> Windows server and see what it does here.
When I have tested it, it's allowed...
$ ./examples/smb2-ls-async smb://172.30.1.42/homes2/foo/./bar/../
bar DIRECTORY 0 Sat Sep 25 08:50:02 2021
.. DIRECTORY 0 Sat Sep 25 09:02:12 2021
. DIRECTORY 0 Sat Sep 25 09:02:12 2021
When last component is dotdot(..) and first component is dot(.), it
seem to refuse connection.
$ ./examples/smb2-ls-async smb://172.30.1.42/homes2/../
failed to create/open directory (Invalid argument) Opendir failed with
(0xc000000d) STATUS_INVALID_PARAMETER.
$ ./examples/smb2-ls-async smb://172.30.1.42/homes2/./
failed to create/open directory (Input/output error) Opendir failed
with (0xc0000033) STATUS_OBJECT_NAME_INVALID.
./examples/smb2-ls-async smb://172.30.1.42/homes2/foo/../
failed to create/open directory (Invalid argument) Opendir failed with
(0xc000000d) STATUS_INVALID_PARAMETER.
$ ./examples/smb2-ls-async smb://172.30.1.42/homes2/./foo
failed to create/open directory (No such file or directory) Opendir
failed with (0xc000003a) STATUS_OBJECT_PATH_NOT_FOUND.
$ ./examples/smb2-ls-async smb://172.30.1.42/homes2/foo/.
bar DIRECTORY 0 Sat Sep 25 08:50:02 2021
.. DIRECTORY 0 Sat Sep 25 09:02:12 2021
. DIRECTORY 0 Sat Sep 25 09:02:12 2021
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] ksmbd: use LOOKUP_BENEATH to prevent the out of share access
2021-09-25 0:08 ` Namjae Jeon
@ 2021-09-25 0:21 ` Namjae Jeon
0 siblings, 0 replies; 11+ messages in thread
From: Namjae Jeon @ 2021-09-25 0:21 UTC (permalink / raw)
To: Jeremy Allison
Cc: Hyunchul Lee, linux-cifs, Ronnie Sahlberg, Ralph Boehme, Steve French
2021-09-25 9:08 GMT+09:00, Namjae Jeon <linkinjeon@kernel.org>:
> 2021-09-25 2:20 GMT+09:00, Jeremy Allison <jra@samba.org>:
>> On Sat, Sep 25, 2021 at 12:06:16AM +0900, Hyunchul Lee wrote:
>>>instead of removing '..' in a given path, call
>>>kern_path with LOOKUP_BENEATH flag to prevent
>>>the out of share access.
>>>
>>>ran various test on this:
>>>smb2-cat-async smb://127.0.0.1/homes/../out_of_share
>>>smb2-cat-async smb://127.0.0.1/homes/foo/../../out_of_share
>>>smbclient //127.0.0.1/homes -c "mkdir ../foo2"
>>>smbclient //127.0.0.1/homes -c "rename bar ../bar"
>>
>> FYI, MS-FSCC states:
>>
>> "Except where explicitly permitted, a pathname component that is a dot
>> directory name MUST NOT
>> be sent over the wire."
>>
>> so it might be easier to just refuse with an
>> error a pathname containing "." or ".." on input
>> processing rather than try and deal with it.
>>
>> Might be interesting to test this against a
>> Windows server and see what it does here.
> When I have tested it, it's allowed...
>
> $ ./examples/smb2-ls-async smb://172.30.1.42/homes2/foo/./bar/../
> bar DIRECTORY 0 Sat Sep 25 08:50:02 2021
>
> .. DIRECTORY 0 Sat Sep 25 09:02:12 2021
>
> . DIRECTORY 0 Sat Sep 25 09:02:12 2021
>
>
> When last component is dotdot(..) and first component is dot(.), it
> seem to refuse connection.
It's not true.. and it is not easy. Using LOOKUP_BENEATH is the easiest choice.
$ ./examples/smb2-ls-async smb://172.30.1.42/homes2/foo/../.
failed to create/open directory (Invalid argument) Opendir failed with
(0xc000000d) STATUS_INVALID_PARAMETER.
$ ./examples/smb2-ls-async smb://172.30.1.42/homes2/foo/../foo/
failed to create/open directory (Invalid argument) Opendir failed with
(0xc000000d) STATUS_INVALID_PARAMETER.
>
> $ ./examples/smb2-ls-async smb://172.30.1.42/homes2/../
> failed to create/open directory (Invalid argument) Opendir failed with
> (0xc000000d) STATUS_INVALID_PARAMETER.
>
> $ ./examples/smb2-ls-async smb://172.30.1.42/homes2/./
> failed to create/open directory (Input/output error) Opendir failed
> with (0xc0000033) STATUS_OBJECT_NAME_INVALID.
>
> ./examples/smb2-ls-async smb://172.30.1.42/homes2/foo/../
> failed to create/open directory (Invalid argument) Opendir failed with
> (0xc000000d) STATUS_INVALID_PARAMETER.
>
> $ ./examples/smb2-ls-async smb://172.30.1.42/homes2/./foo
> failed to create/open directory (No such file or directory) Opendir
> failed with (0xc000003a) STATUS_OBJECT_PATH_NOT_FOUND.
>
> $ ./examples/smb2-ls-async smb://172.30.1.42/homes2/foo/.
> bar DIRECTORY 0 Sat Sep 25 08:50:02 2021
>
> .. DIRECTORY 0 Sat Sep 25 09:02:12 2021
>
> . DIRECTORY 0 Sat Sep 25 09:02:12 2021
>
>
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] ksmbd: use LOOKUP_BENEATH to prevent the out of share access
2021-09-24 15:06 [PATCH v4] ksmbd: use LOOKUP_BENEATH to prevent the out of share access Hyunchul Lee
2021-09-24 17:20 ` Jeremy Allison
@ 2021-09-25 0:43 ` Namjae Jeon
2021-09-25 17:45 ` Steve French
2021-09-28 15:18 ` Tom Talpey
2 siblings, 1 reply; 11+ messages in thread
From: Namjae Jeon @ 2021-09-25 0:43 UTC (permalink / raw)
To: Hyunchul Lee; +Cc: linux-cifs, Ronnie Sahlberg, Ralph Boehme, Steve French
2021-09-25 0:06 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> instead of removing '..' in a given path, call
> kern_path with LOOKUP_BENEATH flag to prevent
> the out of share access.
>
> ran various test on this:
> smb2-cat-async smb://127.0.0.1/homes/../out_of_share
> smb2-cat-async smb://127.0.0.1/homes/foo/../../out_of_share
> smbclient //127.0.0.1/homes -c "mkdir ../foo2"
> smbclient //127.0.0.1/homes -c "rename bar ../bar"
>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Boehme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Cc: Namjae Jeon <linkinjeon@kernel.org>
> Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
Looks good to me!
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Thanks!
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] ksmbd: use LOOKUP_BENEATH to prevent the out of share access
2021-09-25 0:43 ` Namjae Jeon
@ 2021-09-25 17:45 ` Steve French
0 siblings, 0 replies; 11+ messages in thread
From: Steve French @ 2021-09-25 17:45 UTC (permalink / raw)
To: Namjae Jeon; +Cc: Hyunchul Lee, CIFS, Ronnie Sahlberg, Ralph Boehme
I tested this as well with some simple examples trying to escape the
share - testing going fine so far.
Also ran the buildbot from current linux next on client to current
linux next for ksmbd (and it passed)
http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/8/builds/73
On Fri, Sep 24, 2021 at 7:43 PM Namjae Jeon <linkinjeon@kernel.org> wrote:
>
> 2021-09-25 0:06 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> > instead of removing '..' in a given path, call
> > kern_path with LOOKUP_BENEATH flag to prevent
> > the out of share access.
> >
> > ran various test on this:
> > smb2-cat-async smb://127.0.0.1/homes/../out_of_share
> > smb2-cat-async smb://127.0.0.1/homes/foo/../../out_of_share
> > smbclient //127.0.0.1/homes -c "mkdir ../foo2"
> > smbclient //127.0.0.1/homes -c "rename bar ../bar"
> >
> > Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> > Cc: Ralph Boehme <slow@samba.org>
> > Cc: Steve French <smfrench@gmail.com>
> > Cc: Namjae Jeon <linkinjeon@kernel.org>
> > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
> Looks good to me!
> Acked-by: Namjae Jeon <linkinjeon@kernel.org>
>
> Thanks!
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] ksmbd: use LOOKUP_BENEATH to prevent the out of share access
2021-09-24 15:06 [PATCH v4] ksmbd: use LOOKUP_BENEATH to prevent the out of share access Hyunchul Lee
2021-09-24 17:20 ` Jeremy Allison
2021-09-25 0:43 ` Namjae Jeon
@ 2021-09-28 15:18 ` Tom Talpey
2021-09-29 12:40 ` Namjae Jeon
2021-09-30 0:01 ` Hyunchul Lee
2 siblings, 2 replies; 11+ messages in thread
From: Tom Talpey @ 2021-09-28 15:18 UTC (permalink / raw)
To: Hyunchul Lee, linux-cifs
Cc: Ronnie Sahlberg, Ralph Boehme, Steve French, Namjae Jeon
On 9/24/2021 11:06 AM, Hyunchul Lee wrote:
> instead of removing '..' in a given path, call
> kern_path with LOOKUP_BENEATH flag to prevent
> the out of share access.
>
> ran various test on this:
> smb2-cat-async smb://127.0.0.1/homes/../out_of_share
> smb2-cat-async smb://127.0.0.1/homes/foo/../../out_of_share
> smbclient //127.0.0.1/homes -c "mkdir ../foo2"
> smbclient //127.0.0.1/homes -c "rename bar ../bar"
>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Boehme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Cc: Namjae Jeon <linkinjeon@kernel.org>
> Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
> ---
> Changes from v1:
> - pass path of file that is relative to share to ksmbd vfs functions.
> Changes from v2:
> - fix smbtorture smb2.streams.rename, smb2.streams.rename2 failure.
> Changes from v3:
> - fix uninitialized variable free in ksmbd_vfs_kern_path.
>
> fs/ksmbd/misc.c | 100 ++++++-----------------------
> fs/ksmbd/misc.h | 7 +-
> fs/ksmbd/smb2pdu.c | 74 ++++++++-------------
> fs/ksmbd/vfs.c | 156 ++++++++++++++++++++++++---------------------
> fs/ksmbd/vfs.h | 9 ++-
> 5 files changed, 140 insertions(+), 206 deletions(-)
>
> diff --git a/fs/ksmbd/misc.c b/fs/ksmbd/misc.c
> index 3eac3c01749f..6a19f4bc692d 100644
> --- a/fs/ksmbd/misc.c
> +++ b/fs/ksmbd/misc.c
> @@ -158,25 +158,21 @@ int parse_stream_name(char *filename, char **stream_name, int *s_type)
> * Return : windows path string or error
> */
>
> -char *convert_to_nt_pathname(char *filename, char *sharepath)
> +char *convert_to_nt_pathname(char *filename)
> {
> char *ab_pathname;
> - int len, name_len;
>
> - name_len = strlen(filename);
> - ab_pathname = kmalloc(name_len, GFP_KERNEL);
> - if (!ab_pathname)
> - return NULL;
> -
> - ab_pathname[0] = '\\';
> - ab_pathname[1] = '\0';
> + if (strlen(filename) == 0) {
> + ab_pathname = kmalloc(2, GFP_KERNEL);
> + ab_pathname[0] = '\\';
> + ab_pathname[1] = '\0';
This converts the empty filename to "\" - the volume root!?
> + } else {
> + ab_pathname = kstrdup(filename, GFP_KERNEL);
> + if (!ab_pathname)
> + return NULL;
>
> - len = strlen(sharepath);
> - if (!strncmp(filename, sharepath, len) && name_len != len) {
> - strscpy(ab_pathname, &filename[len], name_len);
> ksmbd_conv_path_to_windows(ab_pathname);
> }
> -
> return ab_pathname;
> }
>
> @@ -191,77 +187,19 @@ int get_nlink(struct kstat *st)
> return nlink;
> }
>
> -char *ksmbd_conv_path_to_unix(char *path)
> +void ksmbd_conv_path_to_unix(char *path)
> {
> - size_t path_len, remain_path_len, out_path_len;
> - char *out_path, *out_next;
> - int i, pre_dotdot_cnt = 0, slash_cnt = 0;
> - bool is_last;
> -
> strreplace(path, '\\', '/');
> - path_len = strlen(path);
> - remain_path_len = path_len;
> - if (path_len == 0)
> - return ERR_PTR(-EINVAL);
> -
> - out_path = kzalloc(path_len + 2, GFP_KERNEL);
> - if (!out_path)
> - return ERR_PTR(-ENOMEM);
> - out_path_len = 0;
> - out_next = out_path;
> -
> - do {
> - char *name = path + path_len - remain_path_len;
> - char *next = strchrnul(name, '/');
> - size_t name_len = next - name;
> -
> - is_last = !next[0];
> - if (name_len == 2 && name[0] == '.' && name[1] == '.') {
> - pre_dotdot_cnt++;
> - /* handle the case that path ends with "/.." */
> - if (is_last)
> - goto follow_dotdot;
> - } else {
> - if (pre_dotdot_cnt) {
> -follow_dotdot:
> - slash_cnt = 0;
> - for (i = out_path_len - 1; i >= 0; i--) {
> - if (out_path[i] == '/' &&
> - ++slash_cnt == pre_dotdot_cnt + 1)
> - break;
> - }
> -
> - if (i < 0 &&
> - slash_cnt != pre_dotdot_cnt) {
> - kfree(out_path);
> - return ERR_PTR(-EINVAL);
> - }
> -
> - out_next = &out_path[i+1];
> - *out_next = '\0';
> - out_path_len = i + 1;
> -
> - }
> -
> - if (name_len != 0 &&
> - !(name_len == 1 && name[0] == '.') &&
> - !(name_len == 2 && name[0] == '.' && name[1] == '.')) {
> - next[0] = '\0';
> - sprintf(out_next, "%s/", name);
> - out_next += name_len + 1;
> - out_path_len += name_len + 1;
> - next[0] = '/';
> - }
> - pre_dotdot_cnt = 0;
> - }
> +}
>
> - remain_path_len -= name_len + 1;
> - } while (!is_last);
> +void ksmbd_strip_last_slash(char *path)
> +{
> + int len = strlen(path);
>
> - if (out_path_len > 0)
> - out_path[out_path_len-1] = '\0';
> - path[path_len] = '\0';
> - return out_path;
> + while (len && path[len - 1] == '/') {
> + path[len - 1] = '\0';
> + len--;
> + }
I guess it's intentional that "/////////" will be compacted to "/",
but the open-coded nature of all this really troubles me.
> }
>
> void ksmbd_conv_path_to_windows(char *path)
> @@ -298,7 +236,7 @@ char *ksmbd_extract_sharename(char *treename)
> *
> * Return: converted name on success, otherwise NULL
> */
> -char *convert_to_unix_name(struct ksmbd_share_config *share, char *name)
> +char *convert_to_unix_name(struct ksmbd_share_config *share, const char *name)
> {
> int no_slash = 0, name_len, path_len;
> char *new_name;
> diff --git a/fs/ksmbd/misc.h b/fs/ksmbd/misc.h
> index b7b10139ada2..253366bd0951 100644
> --- a/fs/ksmbd/misc.h
> +++ b/fs/ksmbd/misc.h
> @@ -14,12 +14,13 @@ struct ksmbd_file;
> int match_pattern(const char *str, size_t len, const char *pattern);
> int ksmbd_validate_filename(char *filename);
> int parse_stream_name(char *filename, char **stream_name, int *s_type);
> -char *convert_to_nt_pathname(char *filename, char *sharepath);
> +char *convert_to_nt_pathname(char *filename);
> int get_nlink(struct kstat *st);
> -char *ksmbd_conv_path_to_unix(char *path);
> +void ksmbd_conv_path_to_unix(char *path);
> +void ksmbd_strip_last_slash(char *path);
> void ksmbd_conv_path_to_windows(char *path);
> char *ksmbd_extract_sharename(char *treename);
> -char *convert_to_unix_name(struct ksmbd_share_config *share, char *name);
> +char *convert_to_unix_name(struct ksmbd_share_config *share, const char *name);
>
> #define KSMBD_DIR_INFO_ALIGNMENT 8
> struct ksmbd_dir_info;
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 378e0b4a216d..4c799fef9883 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -643,7 +643,7 @@ static char *
> smb2_get_name(struct ksmbd_share_config *share, const char *src,
> const int maxlen, struct nls_table *local_nls)
> {
> - char *name, *norm_name, *unixname;
> + char *name;
>
> name = smb_strndup_from_utf16(src, maxlen, 1, local_nls);
> if (IS_ERR(name)) {
> @@ -651,23 +651,9 @@ smb2_get_name(struct ksmbd_share_config *share, const char *src,
> return name;
> }
>
> - /* change it to absolute unix name */
> - norm_name = ksmbd_conv_path_to_unix(name);
> - if (IS_ERR(norm_name)) {
> - kfree(name);
> - return norm_name;
> - }
> - kfree(name);
> -
> - unixname = convert_to_unix_name(share, norm_name);
> - kfree(norm_name);
> - if (!unixname) {
> - pr_err("can not convert absolute name\n");
> - return ERR_PTR(-ENOMEM);
> - }
> -
> - ksmbd_debug(SMB, "absolute name = %s\n", unixname);
> - return unixname;
> + ksmbd_conv_path_to_unix(name);
> + ksmbd_strip_last_slash(name);
> + return name;
> }
>
> int setup_async_work(struct ksmbd_work *work, void (*fn)(void **), void **arg)
> @@ -2412,7 +2398,7 @@ static int smb2_creat(struct ksmbd_work *work, struct path *path, char *name,
> return rc;
> }
>
> - rc = ksmbd_vfs_kern_path(name, 0, path, 0);
> + rc = ksmbd_vfs_kern_path(work, name, 0, path, 0);
> if (rc) {
> pr_err("cannot get linux path (%s), err = %d\n",
> name, rc);
> @@ -2487,7 +2473,7 @@ int smb2_open(struct ksmbd_work *work)
> struct oplock_info *opinfo;
> __le32 *next_ptr = NULL;
> int req_op_level = 0, open_flags = 0, may_flags = 0, file_info = 0;
> - int rc = 0, len = 0;
> + int rc = 0;
> int contxt_cnt = 0, query_disk_id = 0;
> int maximal_access_ctxt = 0, posix_ctxt = 0;
> int s_type = 0;
> @@ -2559,17 +2545,11 @@ int smb2_open(struct ksmbd_work *work)
> goto err_out1;
> }
> } else {
> - len = strlen(share->path);
> - ksmbd_debug(SMB, "share path len %d\n", len);
> - name = kmalloc(len + 1, GFP_KERNEL);
> + name = kstrdup("", GFP_KERNEL);
This kstrdup's the empty string! Surely, this means to copy the sharepath?
> if (!name) {
> - rsp->hdr.Status = STATUS_NO_MEMORY;
> rc = -ENOMEM;
> goto err_out1;
> }
> -
> - memcpy(name, share->path, len);
> - *(name + len) = '\0';
> }
>
> req_op_level = req->RequestedOplockLevel;
> @@ -2692,7 +2672,7 @@ int smb2_open(struct ksmbd_work *work)
> goto err_out1;
> }
>
> - rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1);
> + rc = ksmbd_vfs_kern_path(work, name, LOOKUP_NO_SYMLINKS, &path, 1);
> if (!rc) {
> if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) {
> /*
> @@ -2721,11 +2701,8 @@ int smb2_open(struct ksmbd_work *work)
> }
>
> if (rc) {
> - if (rc == -EACCES) {
> - ksmbd_debug(SMB,
> - "User does not have right permission\n");
> + if (rc != -ENOENT)
> goto err_out;
> - }
> ksmbd_debug(SMB, "can not get linux path for %s, rc = %d\n",
> name, rc);
> rc = 0;
> @@ -3229,7 +3206,7 @@ int smb2_open(struct ksmbd_work *work)
> rsp->hdr.Status = STATUS_INVALID_PARAMETER;
> else if (rc == -EOPNOTSUPP)
> rsp->hdr.Status = STATUS_NOT_SUPPORTED;
> - else if (rc == -EACCES || rc == -ESTALE)
> + else if (rc == -EACCES || rc == -ESTALE || rc == -EXDEV)
> rsp->hdr.Status = STATUS_ACCESS_DENIED;
> else if (rc == -ENOENT)
> rsp->hdr.Status = STATUS_OBJECT_NAME_INVALID;
> @@ -4345,8 +4322,7 @@ static int get_file_all_info(struct ksmbd_work *work,
> return -EACCES;
> }
>
> - filename = convert_to_nt_pathname(fp->filename,
> - work->tcon->share_conf->path);
> + filename = convert_to_nt_pathname(fp->filename);
> if (!filename)
> return -ENOMEM;
>
> @@ -4801,7 +4777,7 @@ static int smb2_get_info_filesystem(struct ksmbd_work *work,
> int rc = 0, len;
> int fs_infoclass_size = 0;
>
> - rc = ksmbd_vfs_kern_path(share->path, LOOKUP_NO_SYMLINKS, &path, 0);
> + rc = kern_path(share->path, LOOKUP_NO_SYMLINKS, &path);
> if (rc) {
> pr_err("cannot create vfs path\n");
> return -EIO;
> @@ -5350,7 +5326,7 @@ static int smb2_rename(struct ksmbd_work *work,
> goto out;
>
> len = strlen(new_name);
> - if (new_name[len - 1] != '/') {
> + if (len > 0 && new_name[len - 1] != '/') {
> pr_err("not allow base filename in rename\n");
> rc = -ESHARE;
> goto out;
> @@ -5378,11 +5354,14 @@ static int smb2_rename(struct ksmbd_work *work,
> }
>
> ksmbd_debug(SMB, "new name %s\n", new_name);
> - rc = ksmbd_vfs_kern_path(new_name, LOOKUP_NO_SYMLINKS, &path, 1);
> - if (rc)
> + rc = ksmbd_vfs_kern_path(work, new_name, LOOKUP_NO_SYMLINKS, &path, 1);
> + if (rc) {
> + if (rc != -ENOENT)
> + goto out;
> file_present = false;
> - else
> + } else {
> path_put(&path);
> + }
>
> if (ksmbd_share_veto_filename(share, new_name)) {
> rc = -ENOENT;
> @@ -5456,11 +5435,14 @@ static int smb2_create_link(struct ksmbd_work *work,
> }
>
> ksmbd_debug(SMB, "target name is %s\n", target_name);
> - rc = ksmbd_vfs_kern_path(link_name, LOOKUP_NO_SYMLINKS, &path, 0);
> - if (rc)
> + rc = ksmbd_vfs_kern_path(work, link_name, LOOKUP_NO_SYMLINKS, &path, 0);
> + if (rc) {
> + if (rc != -ENOENT)
> + goto out;
> file_present = false;
> - else
> + } else {
> path_put(&path);
> + }
>
> if (file_info->ReplaceIfExists) {
> if (file_present) {
> @@ -5618,7 +5600,7 @@ static int set_file_allocation_info(struct ksmbd_work *work,
> * inode size is retained by backup inode size.
> */
> size = i_size_read(inode);
> - rc = ksmbd_vfs_truncate(work, NULL, fp, alloc_blks * 512);
> + rc = ksmbd_vfs_truncate(work, fp, alloc_blks * 512);
> if (rc) {
> pr_err("truncate failed! filename : %s, err %d\n",
> fp->filename, rc);
> @@ -5653,7 +5635,7 @@ static int set_end_of_file_info(struct ksmbd_work *work, struct ksmbd_file *fp,
> if (inode->i_sb->s_magic != MSDOS_SUPER_MAGIC) {
> ksmbd_debug(SMB, "filename : %s truncated to newsize %lld\n",
> fp->filename, newsize);
> - rc = ksmbd_vfs_truncate(work, NULL, fp, newsize);
> + rc = ksmbd_vfs_truncate(work, fp, newsize);
> if (rc) {
> ksmbd_debug(SMB, "truncate failed! filename : %s err %d\n",
> fp->filename, rc);
> @@ -5975,7 +5957,7 @@ int smb2_set_info(struct ksmbd_work *work)
> return 0;
>
> err_out:
> - if (rc == -EACCES || rc == -EPERM)
> + if (rc == -EACCES || rc == -EPERM || rc == -EXDEV)
> rsp->hdr.Status = STATUS_ACCESS_DENIED;
> else if (rc == -EINVAL)
> rsp->hdr.Status = STATUS_INVALID_PARAMETER;
> diff --git a/fs/ksmbd/vfs.c b/fs/ksmbd/vfs.c
> index 3733e4944c1d..b41954294d38 100644
> --- a/fs/ksmbd/vfs.c
> +++ b/fs/ksmbd/vfs.c
> @@ -19,6 +19,8 @@
> #include <linux/sched/xacct.h>
> #include <linux/crc32c.h>
>
> +#include "../internal.h" /* for vfs_path_lookup */
> +
> #include "glob.h"
> #include "oplock.h"
> #include "connection.h"
> @@ -44,7 +46,6 @@ static char *extract_last_component(char *path)
> p++;
> } else {
> p = NULL;
> - pr_err("Invalid path %s\n", path);
> }
> return p;
> }
> @@ -155,7 +156,7 @@ int ksmbd_vfs_query_maximal_access(struct user_namespace *user_ns,
> /**
> * ksmbd_vfs_create() - vfs helper for smb create file
> * @work: work
> - * @name: file name
> + * @name: file name that is relative to share
> * @mode: file create mode
> *
> * Return: 0 on success, otherwise error
> @@ -166,7 +167,8 @@ int ksmbd_vfs_create(struct ksmbd_work *work, const char *name, umode_t mode)
> struct dentry *dentry;
> int err;
>
> - dentry = kern_path_create(AT_FDCWD, name, &path, LOOKUP_NO_SYMLINKS);
> + dentry = ksmbd_vfs_kern_path_create(work, name,
> + LOOKUP_NO_SYMLINKS, &path);
> if (IS_ERR(dentry)) {
> err = PTR_ERR(dentry);
> if (err != -ENOENT)
> @@ -191,7 +193,7 @@ int ksmbd_vfs_create(struct ksmbd_work *work, const char *name, umode_t mode)
> /**
> * ksmbd_vfs_mkdir() - vfs helper for smb create directory
> * @work: work
> - * @name: directory name
> + * @name: directory name that is relative to share
> * @mode: directory create mode
> *
> * Return: 0 on success, otherwise error
> @@ -203,8 +205,9 @@ int ksmbd_vfs_mkdir(struct ksmbd_work *work, const char *name, umode_t mode)
> struct dentry *dentry;
> int err;
>
> - dentry = kern_path_create(AT_FDCWD, name, &path,
> - LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY);
> + dentry = ksmbd_vfs_kern_path_create(work, name,
> + LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY,
> + &path);
> if (IS_ERR(dentry)) {
> err = PTR_ERR(dentry);
> if (err != -EEXIST)
> @@ -579,7 +582,7 @@ int ksmbd_vfs_fsync(struct ksmbd_work *work, u64 fid, u64 p_id)
>
> /**
> * ksmbd_vfs_remove_file() - vfs helper for smb rmdir or unlink
> - * @name: absolute directory or file name
> + * @name: directory or file name that is relative to share
> *
> * Return: 0 on success, otherwise error
> */
> @@ -593,7 +596,7 @@ int ksmbd_vfs_remove_file(struct ksmbd_work *work, char *name)
> if (ksmbd_override_fsids(work))
> return -ENOMEM;
>
> - err = kern_path(name, LOOKUP_NO_SYMLINKS, &path);
> + err = ksmbd_vfs_kern_path(work, name, LOOKUP_NO_SYMLINKS, &path, false);
> if (err) {
> ksmbd_debug(VFS, "can't get %s, err %d\n", name, err);
> ksmbd_revert_fsids(work);
> @@ -638,7 +641,7 @@ int ksmbd_vfs_remove_file(struct ksmbd_work *work, char *name)
> /**
> * ksmbd_vfs_link() - vfs helper for creating smb hardlink
> * @oldname: source file name
> - * @newname: hardlink name
> + * @newname: hardlink name that is relative to share
> *
> * Return: 0 on success, otherwise error
> */
> @@ -659,8 +662,9 @@ int ksmbd_vfs_link(struct ksmbd_work *work, const char *oldname,
> goto out1;
> }
>
> - dentry = kern_path_create(AT_FDCWD, newname, &newpath,
> - LOOKUP_NO_SYMLINKS | LOOKUP_REVAL);
> + dentry = ksmbd_vfs_kern_path_create(work, newname,
> + LOOKUP_NO_SYMLINKS | LOOKUP_REVAL,
> + &newpath);
> if (IS_ERR(dentry)) {
> err = PTR_ERR(dentry);
> pr_err("path create err for %s, err %d\n", newname, err);
> @@ -781,14 +785,17 @@ int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
> int err;
>
> dst_name = extract_last_component(newname);
> - if (!dst_name)
> - return -EINVAL;
> + if (!dst_name) {
> + dst_name = newname;
> + newname = "";
> + }
>
> src_dent_parent = dget_parent(fp->filp->f_path.dentry);
> src_dent = fp->filp->f_path.dentry;
>
> - err = kern_path(newname, LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY,
> - &dst_path);
> + err = ksmbd_vfs_kern_path(work, newname,
> + LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY,
> + &dst_path, false);
> if (err) {
> ksmbd_debug(VFS, "Cannot get path for %s [%d]\n", newname, err);
> goto out;
> @@ -834,61 +841,43 @@ int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
> /**
> * ksmbd_vfs_truncate() - vfs helper for smb file truncate
> * @work: work
> - * @name: old filename
> * @fid: file id of old file
> * @size: truncate to given size
> *
> * Return: 0 on success, otherwise error
> */
> -int ksmbd_vfs_truncate(struct ksmbd_work *work, const char *name,
> +int ksmbd_vfs_truncate(struct ksmbd_work *work,
> struct ksmbd_file *fp, loff_t size)
> {
> - struct path path;
> int err = 0;
> + struct file *filp;
>
> - if (name) {
> - err = kern_path(name, LOOKUP_NO_SYMLINKS, &path);
> - if (err) {
> - pr_err("cannot get linux path for %s, err %d\n",
> - name, err);
> - return err;
> - }
> - err = vfs_truncate(&path, size);
> - if (err)
> - pr_err("truncate failed for %s err %d\n",
> - name, err);
> - path_put(&path);
> - } else {
> - struct file *filp;
> -
> - filp = fp->filp;
> -
> - /* Do we need to break any of a levelII oplock? */
> - smb_break_all_levII_oplock(work, fp, 1);
> + filp = fp->filp;
>
> - if (!work->tcon->posix_extensions) {
> - struct inode *inode = file_inode(filp);
> + /* Do we need to break any of a levelII oplock? */
> + smb_break_all_levII_oplock(work, fp, 1);
>
> - if (size < inode->i_size) {
> - err = check_lock_range(filp, size,
> - inode->i_size - 1, WRITE);
> - } else {
> - err = check_lock_range(filp, inode->i_size,
> - size - 1, WRITE);
> - }
> + if (!work->tcon->posix_extensions) {
> + struct inode *inode = file_inode(filp);
>
> - if (err) {
> - pr_err("failed due to lock\n");
> - return -EAGAIN;
> - }
> + if (size < inode->i_size) {
> + err = check_lock_range(filp, size,
> + inode->i_size - 1, WRITE);
> + } else {
> + err = check_lock_range(filp, inode->i_size,
> + size - 1, WRITE);
> }
>
> - err = vfs_truncate(&filp->f_path, size);
> - if (err)
> - pr_err("truncate failed for filename : %s err %d\n",
> - fp->filename, err);
> + if (err) {
> + pr_err("failed due to lock\n");
> + return -EAGAIN;
> + }
> }
>
> + err = vfs_truncate(&filp->f_path, size);
> + if (err)
> + pr_err("truncate failed for filename : %s err %d\n",
> + fp->filename, err);
> return err;
> }
>
> @@ -1206,22 +1195,25 @@ static int ksmbd_vfs_lookup_in_dir(struct path *dir, char *name, size_t namelen)
>
> /**
> * ksmbd_vfs_kern_path() - lookup a file and get path info
> - * @name: name of file for lookup
> + * @name: file path that is relative to share
> * @flags: lookup flags
> * @path: if lookup succeed, return path info
> * @caseless: caseless filename lookup
> *
> * Return: 0 on success, otherwise error
> */
> -int ksmbd_vfs_kern_path(char *name, unsigned int flags, struct path *path,
> - bool caseless)
> +int ksmbd_vfs_kern_path(struct ksmbd_work *work, char *name,
> + unsigned int flags, struct path *path, bool caseless)
> {
> + struct ksmbd_share_config *share_conf = work->tcon->share_conf;
> int err;
>
> - if (name[0] != '/')
> - return -EINVAL;
> -
> - err = kern_path(name, flags, path);
> + flags |= LOOKUP_BENEATH;
> + err = vfs_path_lookup(share_conf->vfs_path.dentry,
> + share_conf->vfs_path.mnt,
> + name,
> + flags,
> + path);
> if (!err)
> return 0;
>
> @@ -1235,11 +1227,10 @@ int ksmbd_vfs_kern_path(char *name, unsigned int flags, struct path *path,
> return -ENOMEM;
>
> path_len = strlen(filepath);
> - remain_len = path_len - 1;
> + remain_len = path_len;
>
> - err = kern_path("/", flags, &parent);
> - if (err)
> - goto out;
> + parent = share_conf->vfs_path;
> + path_get(&parent);
>
> while (d_can_lookup(parent.dentry)) {
> char *filename = filepath + path_len - remain_len;
> @@ -1252,21 +1243,21 @@ int ksmbd_vfs_kern_path(char *name, unsigned int flags, struct path *path,
>
> err = ksmbd_vfs_lookup_in_dir(&parent, filename,
> filename_len);
> - if (err) {
> - path_put(&parent);
> + path_put(&parent);
> + if (err)
> goto out;
> - }
>
> - path_put(&parent);
> next[0] = '\0';
>
> - err = kern_path(filepath, flags, &parent);
> + err = vfs_path_lookup(share_conf->vfs_path.dentry,
> + share_conf->vfs_path.mnt,
> + filepath,
> + flags,
> + &parent);
> if (err)
> goto out;
> -
> - if (is_last) {
> - path->mnt = parent.mnt;
> - path->dentry = parent.dentry;
> + else if (is_last) {
> + *path = parent;
> goto out;
> }
>
> @@ -1282,6 +1273,23 @@ int ksmbd_vfs_kern_path(char *name, unsigned int flags, struct path *path,
> return err;
> }
>
> +struct dentry *ksmbd_vfs_kern_path_create(struct ksmbd_work *work,
> + const char *name,
> + unsigned int flags,
> + struct path *path)
> +{
> + char *abs_name;
> + struct dentry *dent;
> +
> + abs_name = convert_to_unix_name(work->tcon->share_conf, name);
> + if (!abs_name)
> + return ERR_PTR(-ENOMEM);
> +
> + dent = kern_path_create(AT_FDCWD, abs_name, path, flags);
> + kfree(abs_name);
> + return dent;
> +}
> +
> int ksmbd_vfs_remove_acl_xattrs(struct user_namespace *user_ns,
> struct dentry *dentry)
> {
> diff --git a/fs/ksmbd/vfs.h b/fs/ksmbd/vfs.h
> index 85db50abdb24..7b1dcaa3fbdc 100644
> --- a/fs/ksmbd/vfs.h
> +++ b/fs/ksmbd/vfs.h
> @@ -126,7 +126,7 @@ int ksmbd_vfs_link(struct ksmbd_work *work,
> int ksmbd_vfs_getattr(struct path *path, struct kstat *stat);
> int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
> char *newname);
> -int ksmbd_vfs_truncate(struct ksmbd_work *work, const char *name,
> +int ksmbd_vfs_truncate(struct ksmbd_work *work,
> struct ksmbd_file *fp, loff_t size);
> struct srv_copychunk;
> int ksmbd_vfs_copy_file_ranges(struct ksmbd_work *work,
> @@ -152,8 +152,13 @@ int ksmbd_vfs_xattr_stream_name(char *stream_name, char **xattr_stream_name,
> size_t *xattr_stream_name_size, int s_type);
> int ksmbd_vfs_remove_xattr(struct user_namespace *user_ns,
> struct dentry *dentry, char *attr_name);
> -int ksmbd_vfs_kern_path(char *name, unsigned int flags, struct path *path,
> +int ksmbd_vfs_kern_path(struct ksmbd_work *work,
> + char *name, unsigned int flags, struct path *path,
> bool caseless);
> +struct dentry *ksmbd_vfs_kern_path_create(struct ksmbd_work *work,
> + const char *name,
> + unsigned int flags,
> + struct path *path);
> int ksmbd_vfs_empty_dir(struct ksmbd_file *fp);
> void ksmbd_vfs_set_fadvise(struct file *filp, __le32 option);
> int ksmbd_vfs_zero_data(struct ksmbd_work *work, struct ksmbd_file *fp,
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] ksmbd: use LOOKUP_BENEATH to prevent the out of share access
2021-09-28 15:18 ` Tom Talpey
@ 2021-09-29 12:40 ` Namjae Jeon
2021-09-29 15:01 ` Tom Talpey
2021-09-30 0:01 ` Hyunchul Lee
1 sibling, 1 reply; 11+ messages in thread
From: Namjae Jeon @ 2021-09-29 12:40 UTC (permalink / raw)
To: Tom Talpey
Cc: Hyunchul Lee, linux-cifs, Ronnie Sahlberg, Ralph Boehme, Steve French
2021-09-29 0:18 GMT+09:00, Tom Talpey <tom@talpey.com>:
> On 9/24/2021 11:06 AM, Hyunchul Lee wrote:
>> instead of removing '..' in a given path, call
>> kern_path with LOOKUP_BENEATH flag to prevent
>> the out of share access.
>>
>> ran various test on this:
>> smb2-cat-async smb://127.0.0.1/homes/../out_of_share
>> smb2-cat-async smb://127.0.0.1/homes/foo/../../out_of_share
>> smbclient //127.0.0.1/homes -c "mkdir ../foo2"
>> smbclient //127.0.0.1/homes -c "rename bar ../bar"
>>
>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> Cc: Ralph Boehme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Cc: Namjae Jeon <linkinjeon@kernel.org>
>> Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
>> ---
>> Changes from v1:
>> - pass path of file that is relative to share to ksmbd vfs functions.
>> Changes from v2:
>> - fix smbtorture smb2.streams.rename, smb2.streams.rename2 failure.
>> Changes from v3:
>> - fix uninitialized variable free in ksmbd_vfs_kern_path.
>>
>> fs/ksmbd/misc.c | 100 ++++++-----------------------
>> fs/ksmbd/misc.h | 7 +-
>> fs/ksmbd/smb2pdu.c | 74 ++++++++-------------
>> fs/ksmbd/vfs.c | 156 ++++++++++++++++++++++++---------------------
>> fs/ksmbd/vfs.h | 9 ++-
>> 5 files changed, 140 insertions(+), 206 deletions(-)
>>
>> diff --git a/fs/ksmbd/misc.c b/fs/ksmbd/misc.c
>> index 3eac3c01749f..6a19f4bc692d 100644
>> --- a/fs/ksmbd/misc.c
>> +++ b/fs/ksmbd/misc.c
>> @@ -158,25 +158,21 @@ int parse_stream_name(char *filename, char
>> **stream_name, int *s_type)
>> * Return : windows path string or error
>> */
>>
>> -char *convert_to_nt_pathname(char *filename, char *sharepath)
>> +char *convert_to_nt_pathname(char *filename)
>> {
>> char *ab_pathname;
>> - int len, name_len;
>>
>> - name_len = strlen(filename);
>> - ab_pathname = kmalloc(name_len, GFP_KERNEL);
>> - if (!ab_pathname)
>> - return NULL;
>> -
>> - ab_pathname[0] = '\\';
>> - ab_pathname[1] = '\0';
>> + if (strlen(filename) == 0) {
>> + ab_pathname = kmalloc(2, GFP_KERNEL);
>> + ab_pathname[0] = '\\';
>> + ab_pathname[1] = '\0';
>
> This converts the empty filename to "\" - the volume root!?
"\" is relative to the share. i.e. the share root.
>
>> + } else {
>> + ab_pathname = kstrdup(filename, GFP_KERNEL);
>> + if (!ab_pathname)
>> + return NULL;
>>
>> - len = strlen(sharepath);
>> - if (!strncmp(filename, sharepath, len) && name_len != len) {
>> - strscpy(ab_pathname, &filename[len], name_len);
>> ksmbd_conv_path_to_windows(ab_pathname);
>> }
>> -
>> return ab_pathname;
>> }
>>
>> @@ -191,77 +187,19 @@ int get_nlink(struct kstat *st)
>> return nlink;
>> }
>>
>> -char *ksmbd_conv_path_to_unix(char *path)
>> +void ksmbd_conv_path_to_unix(char *path)
>> {
>> - size_t path_len, remain_path_len, out_path_len;
>> - char *out_path, *out_next;
>> - int i, pre_dotdot_cnt = 0, slash_cnt = 0;
>> - bool is_last;
>> -
>> strreplace(path, '\\', '/');
>> - path_len = strlen(path);
>> - remain_path_len = path_len;
>> - if (path_len == 0)
>> - return ERR_PTR(-EINVAL);
>> -
>> - out_path = kzalloc(path_len + 2, GFP_KERNEL);
>> - if (!out_path)
>> - return ERR_PTR(-ENOMEM);
>> - out_path_len = 0;
>> - out_next = out_path;
>> -
>> - do {
>> - char *name = path + path_len - remain_path_len;
>> - char *next = strchrnul(name, '/');
>> - size_t name_len = next - name;
>> -
>> - is_last = !next[0];
>> - if (name_len == 2 && name[0] == '.' && name[1] == '.') {
>> - pre_dotdot_cnt++;
>> - /* handle the case that path ends with "/.." */
>> - if (is_last)
>> - goto follow_dotdot;
>> - } else {
>> - if (pre_dotdot_cnt) {
>> -follow_dotdot:
>> - slash_cnt = 0;
>> - for (i = out_path_len - 1; i >= 0; i--) {
>> - if (out_path[i] == '/' &&
>> - ++slash_cnt == pre_dotdot_cnt + 1)
>> - break;
>> - }
>> -
>> - if (i < 0 &&
>> - slash_cnt != pre_dotdot_cnt) {
>> - kfree(out_path);
>> - return ERR_PTR(-EINVAL);
>> - }
>> -
>> - out_next = &out_path[i+1];
>> - *out_next = '\0';
>> - out_path_len = i + 1;
>> -
>> - }
>> -
>> - if (name_len != 0 &&
>> - !(name_len == 1 && name[0] == '.') &&
>> - !(name_len == 2 && name[0] == '.' && name[1] == '.')) {
>> - next[0] = '\0';
>> - sprintf(out_next, "%s/", name);
>> - out_next += name_len + 1;
>> - out_path_len += name_len + 1;
>> - next[0] = '/';
>> - }
>> - pre_dotdot_cnt = 0;
>> - }
>> +}
>>
>> - remain_path_len -= name_len + 1;
>> - } while (!is_last);
>> +void ksmbd_strip_last_slash(char *path)
>> +{
>> + int len = strlen(path);
>>
>> - if (out_path_len > 0)
>> - out_path[out_path_len-1] = '\0';
>> - path[path_len] = '\0';
>> - return out_path;
>> + while (len && path[len - 1] == '/') {
>> + path[len - 1] = '\0';
>> + len--;
>> + }
>
> I guess it's intentional that "/////////" will be compacted to "/",
> but the open-coded nature of all this really troubles me.
>
>> }
>>
>> void ksmbd_conv_path_to_windows(char *path)
>> @@ -298,7 +236,7 @@ char *ksmbd_extract_sharename(char *treename)
>> *
>> * Return: converted name on success, otherwise NULL
>> */
>> -char *convert_to_unix_name(struct ksmbd_share_config *share, char *name)
>> +char *convert_to_unix_name(struct ksmbd_share_config *share, const char
>> *name)
>> {
>> int no_slash = 0, name_len, path_len;
>> char *new_name;
>> diff --git a/fs/ksmbd/misc.h b/fs/ksmbd/misc.h
>> index b7b10139ada2..253366bd0951 100644
>> --- a/fs/ksmbd/misc.h
>> +++ b/fs/ksmbd/misc.h
>> @@ -14,12 +14,13 @@ struct ksmbd_file;
>> int match_pattern(const char *str, size_t len, const char *pattern);
>> int ksmbd_validate_filename(char *filename);
>> int parse_stream_name(char *filename, char **stream_name, int *s_type);
>> -char *convert_to_nt_pathname(char *filename, char *sharepath);
>> +char *convert_to_nt_pathname(char *filename);
>> int get_nlink(struct kstat *st);
>> -char *ksmbd_conv_path_to_unix(char *path);
>> +void ksmbd_conv_path_to_unix(char *path);
>> +void ksmbd_strip_last_slash(char *path);
>> void ksmbd_conv_path_to_windows(char *path);
>> char *ksmbd_extract_sharename(char *treename);
>> -char *convert_to_unix_name(struct ksmbd_share_config *share, char
>> *name);
>> +char *convert_to_unix_name(struct ksmbd_share_config *share, const char
>> *name);
>>
>> #define KSMBD_DIR_INFO_ALIGNMENT 8
>> struct ksmbd_dir_info;
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index 378e0b4a216d..4c799fef9883 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -643,7 +643,7 @@ static char *
>> smb2_get_name(struct ksmbd_share_config *share, const char *src,
>> const int maxlen, struct nls_table *local_nls)
>> {
>> - char *name, *norm_name, *unixname;
>> + char *name;
>>
>> name = smb_strndup_from_utf16(src, maxlen, 1, local_nls);
>> if (IS_ERR(name)) {
>> @@ -651,23 +651,9 @@ smb2_get_name(struct ksmbd_share_config *share, const
>> char *src,
>> return name;
>> }
>>
>> - /* change it to absolute unix name */
>> - norm_name = ksmbd_conv_path_to_unix(name);
>> - if (IS_ERR(norm_name)) {
>> - kfree(name);
>> - return norm_name;
>> - }
>> - kfree(name);
>> -
>> - unixname = convert_to_unix_name(share, norm_name);
>> - kfree(norm_name);
>> - if (!unixname) {
>> - pr_err("can not convert absolute name\n");
>> - return ERR_PTR(-ENOMEM);
>> - }
>> -
>> - ksmbd_debug(SMB, "absolute name = %s\n", unixname);
>> - return unixname;
>> + ksmbd_conv_path_to_unix(name);
>> + ksmbd_strip_last_slash(name);
>> + return name;
>> }
>>
>> int setup_async_work(struct ksmbd_work *work, void (*fn)(void **), void
>> **arg)
>> @@ -2412,7 +2398,7 @@ static int smb2_creat(struct ksmbd_work *work,
>> struct path *path, char *name,
>> return rc;
>> }
>>
>> - rc = ksmbd_vfs_kern_path(name, 0, path, 0);
>> + rc = ksmbd_vfs_kern_path(work, name, 0, path, 0);
>> if (rc) {
>> pr_err("cannot get linux path (%s), err = %d\n",
>> name, rc);
>> @@ -2487,7 +2473,7 @@ int smb2_open(struct ksmbd_work *work)
>> struct oplock_info *opinfo;
>> __le32 *next_ptr = NULL;
>> int req_op_level = 0, open_flags = 0, may_flags = 0, file_info = 0;
>> - int rc = 0, len = 0;
>> + int rc = 0;
>> int contxt_cnt = 0, query_disk_id = 0;
>> int maximal_access_ctxt = 0, posix_ctxt = 0;
>> int s_type = 0;
>> @@ -2559,17 +2545,11 @@ int smb2_open(struct ksmbd_work *work)
>> goto err_out1;
>> }
>> } else {
>> - len = strlen(share->path);
>> - ksmbd_debug(SMB, "share path len %d\n", len);
>> - name = kmalloc(len + 1, GFP_KERNEL);
>> + name = kstrdup("", GFP_KERNEL);
>
> This kstrdup's the empty string! Surely, this means to copy the sharepath?
>
>> if (!name) {
>> - rsp->hdr.Status = STATUS_NO_MEMORY;
>> rc = -ENOMEM;
>> goto err_out1;
>> }
>> -
>> - memcpy(name, share->path, len);
>> - *(name + len) = '\0';
>> }
>>
>> req_op_level = req->RequestedOplockLevel;
>> @@ -2692,7 +2672,7 @@ int smb2_open(struct ksmbd_work *work)
>> goto err_out1;
>> }
>>
>> - rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1);
>> + rc = ksmbd_vfs_kern_path(work, name, LOOKUP_NO_SYMLINKS, &path, 1);
>> if (!rc) {
>> if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) {
>> /*
>> @@ -2721,11 +2701,8 @@ int smb2_open(struct ksmbd_work *work)
>> }
>>
>> if (rc) {
>> - if (rc == -EACCES) {
>> - ksmbd_debug(SMB,
>> - "User does not have right permission\n");
>> + if (rc != -ENOENT)
>> goto err_out;
>> - }
>> ksmbd_debug(SMB, "can not get linux path for %s, rc = %d\n",
>> name, rc);
>> rc = 0;
>> @@ -3229,7 +3206,7 @@ int smb2_open(struct ksmbd_work *work)
>> rsp->hdr.Status = STATUS_INVALID_PARAMETER;
>> else if (rc == -EOPNOTSUPP)
>> rsp->hdr.Status = STATUS_NOT_SUPPORTED;
>> - else if (rc == -EACCES || rc == -ESTALE)
>> + else if (rc == -EACCES || rc == -ESTALE || rc == -EXDEV)
>> rsp->hdr.Status = STATUS_ACCESS_DENIED;
>> else if (rc == -ENOENT)
>> rsp->hdr.Status = STATUS_OBJECT_NAME_INVALID;
>> @@ -4345,8 +4322,7 @@ static int get_file_all_info(struct ksmbd_work
>> *work,
>> return -EACCES;
>> }
>>
>> - filename = convert_to_nt_pathname(fp->filename,
>> - work->tcon->share_conf->path);
>> + filename = convert_to_nt_pathname(fp->filename);
>> if (!filename)
>> return -ENOMEM;
>>
>> @@ -4801,7 +4777,7 @@ static int smb2_get_info_filesystem(struct
>> ksmbd_work *work,
>> int rc = 0, len;
>> int fs_infoclass_size = 0;
>>
>> - rc = ksmbd_vfs_kern_path(share->path, LOOKUP_NO_SYMLINKS, &path, 0);
>> + rc = kern_path(share->path, LOOKUP_NO_SYMLINKS, &path);
>> if (rc) {
>> pr_err("cannot create vfs path\n");
>> return -EIO;
>> @@ -5350,7 +5326,7 @@ static int smb2_rename(struct ksmbd_work *work,
>> goto out;
>>
>> len = strlen(new_name);
>> - if (new_name[len - 1] != '/') {
>> + if (len > 0 && new_name[len - 1] != '/') {
>> pr_err("not allow base filename in rename\n");
>> rc = -ESHARE;
>> goto out;
>> @@ -5378,11 +5354,14 @@ static int smb2_rename(struct ksmbd_work *work,
>> }
>>
>> ksmbd_debug(SMB, "new name %s\n", new_name);
>> - rc = ksmbd_vfs_kern_path(new_name, LOOKUP_NO_SYMLINKS, &path, 1);
>> - if (rc)
>> + rc = ksmbd_vfs_kern_path(work, new_name, LOOKUP_NO_SYMLINKS, &path, 1);
>> + if (rc) {
>> + if (rc != -ENOENT)
>> + goto out;
>> file_present = false;
>> - else
>> + } else {
>> path_put(&path);
>> + }
>>
>> if (ksmbd_share_veto_filename(share, new_name)) {
>> rc = -ENOENT;
>> @@ -5456,11 +5435,14 @@ static int smb2_create_link(struct ksmbd_work
>> *work,
>> }
>>
>> ksmbd_debug(SMB, "target name is %s\n", target_name);
>> - rc = ksmbd_vfs_kern_path(link_name, LOOKUP_NO_SYMLINKS, &path, 0);
>> - if (rc)
>> + rc = ksmbd_vfs_kern_path(work, link_name, LOOKUP_NO_SYMLINKS, &path,
>> 0);
>> + if (rc) {
>> + if (rc != -ENOENT)
>> + goto out;
>> file_present = false;
>> - else
>> + } else {
>> path_put(&path);
>> + }
>>
>> if (file_info->ReplaceIfExists) {
>> if (file_present) {
>> @@ -5618,7 +5600,7 @@ static int set_file_allocation_info(struct
>> ksmbd_work *work,
>> * inode size is retained by backup inode size.
>> */
>> size = i_size_read(inode);
>> - rc = ksmbd_vfs_truncate(work, NULL, fp, alloc_blks * 512);
>> + rc = ksmbd_vfs_truncate(work, fp, alloc_blks * 512);
>> if (rc) {
>> pr_err("truncate failed! filename : %s, err %d\n",
>> fp->filename, rc);
>> @@ -5653,7 +5635,7 @@ static int set_end_of_file_info(struct ksmbd_work
>> *work, struct ksmbd_file *fp,
>> if (inode->i_sb->s_magic != MSDOS_SUPER_MAGIC) {
>> ksmbd_debug(SMB, "filename : %s truncated to newsize %lld\n",
>> fp->filename, newsize);
>> - rc = ksmbd_vfs_truncate(work, NULL, fp, newsize);
>> + rc = ksmbd_vfs_truncate(work, fp, newsize);
>> if (rc) {
>> ksmbd_debug(SMB, "truncate failed! filename : %s err %d\n",
>> fp->filename, rc);
>> @@ -5975,7 +5957,7 @@ int smb2_set_info(struct ksmbd_work *work)
>> return 0;
>>
>> err_out:
>> - if (rc == -EACCES || rc == -EPERM)
>> + if (rc == -EACCES || rc == -EPERM || rc == -EXDEV)
>> rsp->hdr.Status = STATUS_ACCESS_DENIED;
>> else if (rc == -EINVAL)
>> rsp->hdr.Status = STATUS_INVALID_PARAMETER;
>> diff --git a/fs/ksmbd/vfs.c b/fs/ksmbd/vfs.c
>> index 3733e4944c1d..b41954294d38 100644
>> --- a/fs/ksmbd/vfs.c
>> +++ b/fs/ksmbd/vfs.c
>> @@ -19,6 +19,8 @@
>> #include <linux/sched/xacct.h>
>> #include <linux/crc32c.h>
>>
>> +#include "../internal.h" /* for vfs_path_lookup */
>> +
>> #include "glob.h"
>> #include "oplock.h"
>> #include "connection.h"
>> @@ -44,7 +46,6 @@ static char *extract_last_component(char *path)
>> p++;
>> } else {
>> p = NULL;
>> - pr_err("Invalid path %s\n", path);
>> }
>> return p;
>> }
>> @@ -155,7 +156,7 @@ int ksmbd_vfs_query_maximal_access(struct
>> user_namespace *user_ns,
>> /**
>> * ksmbd_vfs_create() - vfs helper for smb create file
>> * @work: work
>> - * @name: file name
>> + * @name: file name that is relative to share
>> * @mode: file create mode
>> *
>> * Return: 0 on success, otherwise error
>> @@ -166,7 +167,8 @@ int ksmbd_vfs_create(struct ksmbd_work *work, const
>> char *name, umode_t mode)
>> struct dentry *dentry;
>> int err;
>>
>> - dentry = kern_path_create(AT_FDCWD, name, &path, LOOKUP_NO_SYMLINKS);
>> + dentry = ksmbd_vfs_kern_path_create(work, name,
>> + LOOKUP_NO_SYMLINKS, &path);
>> if (IS_ERR(dentry)) {
>> err = PTR_ERR(dentry);
>> if (err != -ENOENT)
>> @@ -191,7 +193,7 @@ int ksmbd_vfs_create(struct ksmbd_work *work, const
>> char *name, umode_t mode)
>> /**
>> * ksmbd_vfs_mkdir() - vfs helper for smb create directory
>> * @work: work
>> - * @name: directory name
>> + * @name: directory name that is relative to share
>> * @mode: directory create mode
>> *
>> * Return: 0 on success, otherwise error
>> @@ -203,8 +205,9 @@ int ksmbd_vfs_mkdir(struct ksmbd_work *work, const
>> char *name, umode_t mode)
>> struct dentry *dentry;
>> int err;
>>
>> - dentry = kern_path_create(AT_FDCWD, name, &path,
>> - LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY);
>> + dentry = ksmbd_vfs_kern_path_create(work, name,
>> + LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY,
>> + &path);
>> if (IS_ERR(dentry)) {
>> err = PTR_ERR(dentry);
>> if (err != -EEXIST)
>> @@ -579,7 +582,7 @@ int ksmbd_vfs_fsync(struct ksmbd_work *work, u64 fid,
>> u64 p_id)
>>
>> /**
>> * ksmbd_vfs_remove_file() - vfs helper for smb rmdir or unlink
>> - * @name: absolute directory or file name
>> + * @name: directory or file name that is relative to share
>> *
>> * Return: 0 on success, otherwise error
>> */
>> @@ -593,7 +596,7 @@ int ksmbd_vfs_remove_file(struct ksmbd_work *work,
>> char *name)
>> if (ksmbd_override_fsids(work))
>> return -ENOMEM;
>>
>> - err = kern_path(name, LOOKUP_NO_SYMLINKS, &path);
>> + err = ksmbd_vfs_kern_path(work, name, LOOKUP_NO_SYMLINKS, &path,
>> false);
>> if (err) {
>> ksmbd_debug(VFS, "can't get %s, err %d\n", name, err);
>> ksmbd_revert_fsids(work);
>> @@ -638,7 +641,7 @@ int ksmbd_vfs_remove_file(struct ksmbd_work *work,
>> char *name)
>> /**
>> * ksmbd_vfs_link() - vfs helper for creating smb hardlink
>> * @oldname: source file name
>> - * @newname: hardlink name
>> + * @newname: hardlink name that is relative to share
>> *
>> * Return: 0 on success, otherwise error
>> */
>> @@ -659,8 +662,9 @@ int ksmbd_vfs_link(struct ksmbd_work *work, const char
>> *oldname,
>> goto out1;
>> }
>>
>> - dentry = kern_path_create(AT_FDCWD, newname, &newpath,
>> - LOOKUP_NO_SYMLINKS | LOOKUP_REVAL);
>> + dentry = ksmbd_vfs_kern_path_create(work, newname,
>> + LOOKUP_NO_SYMLINKS | LOOKUP_REVAL,
>> + &newpath);
>> if (IS_ERR(dentry)) {
>> err = PTR_ERR(dentry);
>> pr_err("path create err for %s, err %d\n", newname, err);
>> @@ -781,14 +785,17 @@ int ksmbd_vfs_fp_rename(struct ksmbd_work *work,
>> struct ksmbd_file *fp,
>> int err;
>>
>> dst_name = extract_last_component(newname);
>> - if (!dst_name)
>> - return -EINVAL;
>> + if (!dst_name) {
>> + dst_name = newname;
>> + newname = "";
>> + }
>>
>> src_dent_parent = dget_parent(fp->filp->f_path.dentry);
>> src_dent = fp->filp->f_path.dentry;
>>
>> - err = kern_path(newname, LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY,
>> - &dst_path);
>> + err = ksmbd_vfs_kern_path(work, newname,
>> + LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY,
>> + &dst_path, false);
>> if (err) {
>> ksmbd_debug(VFS, "Cannot get path for %s [%d]\n", newname, err);
>> goto out;
>> @@ -834,61 +841,43 @@ int ksmbd_vfs_fp_rename(struct ksmbd_work *work,
>> struct ksmbd_file *fp,
>> /**
>> * ksmbd_vfs_truncate() - vfs helper for smb file truncate
>> * @work: work
>> - * @name: old filename
>> * @fid: file id of old file
>> * @size: truncate to given size
>> *
>> * Return: 0 on success, otherwise error
>> */
>> -int ksmbd_vfs_truncate(struct ksmbd_work *work, const char *name,
>> +int ksmbd_vfs_truncate(struct ksmbd_work *work,
>> struct ksmbd_file *fp, loff_t size)
>> {
>> - struct path path;
>> int err = 0;
>> + struct file *filp;
>>
>> - if (name) {
>> - err = kern_path(name, LOOKUP_NO_SYMLINKS, &path);
>> - if (err) {
>> - pr_err("cannot get linux path for %s, err %d\n",
>> - name, err);
>> - return err;
>> - }
>> - err = vfs_truncate(&path, size);
>> - if (err)
>> - pr_err("truncate failed for %s err %d\n",
>> - name, err);
>> - path_put(&path);
>> - } else {
>> - struct file *filp;
>> -
>> - filp = fp->filp;
>> -
>> - /* Do we need to break any of a levelII oplock? */
>> - smb_break_all_levII_oplock(work, fp, 1);
>> + filp = fp->filp;
>>
>> - if (!work->tcon->posix_extensions) {
>> - struct inode *inode = file_inode(filp);
>> + /* Do we need to break any of a levelII oplock? */
>> + smb_break_all_levII_oplock(work, fp, 1);
>>
>> - if (size < inode->i_size) {
>> - err = check_lock_range(filp, size,
>> - inode->i_size - 1, WRITE);
>> - } else {
>> - err = check_lock_range(filp, inode->i_size,
>> - size - 1, WRITE);
>> - }
>> + if (!work->tcon->posix_extensions) {
>> + struct inode *inode = file_inode(filp);
>>
>> - if (err) {
>> - pr_err("failed due to lock\n");
>> - return -EAGAIN;
>> - }
>> + if (size < inode->i_size) {
>> + err = check_lock_range(filp, size,
>> + inode->i_size - 1, WRITE);
>> + } else {
>> + err = check_lock_range(filp, inode->i_size,
>> + size - 1, WRITE);
>> }
>>
>> - err = vfs_truncate(&filp->f_path, size);
>> - if (err)
>> - pr_err("truncate failed for filename : %s err %d\n",
>> - fp->filename, err);
>> + if (err) {
>> + pr_err("failed due to lock\n");
>> + return -EAGAIN;
>> + }
>> }
>>
>> + err = vfs_truncate(&filp->f_path, size);
>> + if (err)
>> + pr_err("truncate failed for filename : %s err %d\n",
>> + fp->filename, err);
>> return err;
>> }
>>
>> @@ -1206,22 +1195,25 @@ static int ksmbd_vfs_lookup_in_dir(struct path
>> *dir, char *name, size_t namelen)
>>
>> /**
>> * ksmbd_vfs_kern_path() - lookup a file and get path info
>> - * @name: name of file for lookup
>> + * @name: file path that is relative to share
>> * @flags: lookup flags
>> * @path: if lookup succeed, return path info
>> * @caseless: caseless filename lookup
>> *
>> * Return: 0 on success, otherwise error
>> */
>> -int ksmbd_vfs_kern_path(char *name, unsigned int flags, struct path
>> *path,
>> - bool caseless)
>> +int ksmbd_vfs_kern_path(struct ksmbd_work *work, char *name,
>> + unsigned int flags, struct path *path, bool caseless)
>> {
>> + struct ksmbd_share_config *share_conf = work->tcon->share_conf;
>> int err;
>>
>> - if (name[0] != '/')
>> - return -EINVAL;
>> -
>> - err = kern_path(name, flags, path);
>> + flags |= LOOKUP_BENEATH;
>> + err = vfs_path_lookup(share_conf->vfs_path.dentry,
>> + share_conf->vfs_path.mnt,
>> + name,
>> + flags,
>> + path);
>> if (!err)
>> return 0;
>>
>> @@ -1235,11 +1227,10 @@ int ksmbd_vfs_kern_path(char *name, unsigned int
>> flags, struct path *path,
>> return -ENOMEM;
>>
>> path_len = strlen(filepath);
>> - remain_len = path_len - 1;
>> + remain_len = path_len;
>>
>> - err = kern_path("/", flags, &parent);
>> - if (err)
>> - goto out;
>> + parent = share_conf->vfs_path;
>> + path_get(&parent);
>>
>> while (d_can_lookup(parent.dentry)) {
>> char *filename = filepath + path_len - remain_len;
>> @@ -1252,21 +1243,21 @@ int ksmbd_vfs_kern_path(char *name, unsigned int
>> flags, struct path *path,
>>
>> err = ksmbd_vfs_lookup_in_dir(&parent, filename,
>> filename_len);
>> - if (err) {
>> - path_put(&parent);
>> + path_put(&parent);
>> + if (err)
>> goto out;
>> - }
>>
>> - path_put(&parent);
>> next[0] = '\0';
>>
>> - err = kern_path(filepath, flags, &parent);
>> + err = vfs_path_lookup(share_conf->vfs_path.dentry,
>> + share_conf->vfs_path.mnt,
>> + filepath,
>> + flags,
>> + &parent);
>> if (err)
>> goto out;
>> -
>> - if (is_last) {
>> - path->mnt = parent.mnt;
>> - path->dentry = parent.dentry;
>> + else if (is_last) {
>> + *path = parent;
>> goto out;
>> }
>>
>> @@ -1282,6 +1273,23 @@ int ksmbd_vfs_kern_path(char *name, unsigned int
>> flags, struct path *path,
>> return err;
>> }
>>
>> +struct dentry *ksmbd_vfs_kern_path_create(struct ksmbd_work *work,
>> + const char *name,
>> + unsigned int flags,
>> + struct path *path)
>> +{
>> + char *abs_name;
>> + struct dentry *dent;
>> +
>> + abs_name = convert_to_unix_name(work->tcon->share_conf, name);
>> + if (!abs_name)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + dent = kern_path_create(AT_FDCWD, abs_name, path, flags);
>> + kfree(abs_name);
>> + return dent;
>> +}
>> +
>> int ksmbd_vfs_remove_acl_xattrs(struct user_namespace *user_ns,
>> struct dentry *dentry)
>> {
>> diff --git a/fs/ksmbd/vfs.h b/fs/ksmbd/vfs.h
>> index 85db50abdb24..7b1dcaa3fbdc 100644
>> --- a/fs/ksmbd/vfs.h
>> +++ b/fs/ksmbd/vfs.h
>> @@ -126,7 +126,7 @@ int ksmbd_vfs_link(struct ksmbd_work *work,
>> int ksmbd_vfs_getattr(struct path *path, struct kstat *stat);
>> int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
>> char *newname);
>> -int ksmbd_vfs_truncate(struct ksmbd_work *work, const char *name,
>> +int ksmbd_vfs_truncate(struct ksmbd_work *work,
>> struct ksmbd_file *fp, loff_t size);
>> struct srv_copychunk;
>> int ksmbd_vfs_copy_file_ranges(struct ksmbd_work *work,
>> @@ -152,8 +152,13 @@ int ksmbd_vfs_xattr_stream_name(char *stream_name,
>> char **xattr_stream_name,
>> size_t *xattr_stream_name_size, int s_type);
>> int ksmbd_vfs_remove_xattr(struct user_namespace *user_ns,
>> struct dentry *dentry, char *attr_name);
>> -int ksmbd_vfs_kern_path(char *name, unsigned int flags, struct path
>> *path,
>> +int ksmbd_vfs_kern_path(struct ksmbd_work *work,
>> + char *name, unsigned int flags, struct path *path,
>> bool caseless);
>> +struct dentry *ksmbd_vfs_kern_path_create(struct ksmbd_work *work,
>> + const char *name,
>> + unsigned int flags,
>> + struct path *path);
>> int ksmbd_vfs_empty_dir(struct ksmbd_file *fp);
>> void ksmbd_vfs_set_fadvise(struct file *filp, __le32 option);
>> int ksmbd_vfs_zero_data(struct ksmbd_work *work, struct ksmbd_file *fp,
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] ksmbd: use LOOKUP_BENEATH to prevent the out of share access
2021-09-29 12:40 ` Namjae Jeon
@ 2021-09-29 15:01 ` Tom Talpey
2021-09-29 23:52 ` Namjae Jeon
0 siblings, 1 reply; 11+ messages in thread
From: Tom Talpey @ 2021-09-29 15:01 UTC (permalink / raw)
To: Namjae Jeon
Cc: Hyunchul Lee, linux-cifs, Ronnie Sahlberg, Ralph Boehme, Steve French
On 9/29/2021 8:40 AM, Namjae Jeon wrote:
> 2021-09-29 0:18 GMT+09:00, Tom Talpey <tom@talpey.com>:
>> On 9/24/2021 11:06 AM, Hyunchul Lee wrote:
>>> instead of removing '..' in a given path, call
>>> kern_path with LOOKUP_BENEATH flag to prevent
>>> the out of share access.
>>> <snip> <snip> <snip>
>>> -char *convert_to_nt_pathname(char *filename, char *sharepath)
>>> +char *convert_to_nt_pathname(char *filename)
>>> {
>>> char *ab_pathname;
>>> - int len, name_len;
>>>
>>> - name_len = strlen(filename);
>>> - ab_pathname = kmalloc(name_len, GFP_KERNEL);
>>> - if (!ab_pathname)
>>> - return NULL;
>>> -
>>> - ab_pathname[0] = '\\';
>>> - ab_pathname[1] = '\0';
>>> + if (strlen(filename) == 0) {
>>> + ab_pathname = kmalloc(2, GFP_KERNEL);
>>> + ab_pathname[0] = '\\';
>>> + ab_pathname[1] = '\0';
>>
>> This converts the empty filename to "\" - the volume root!?
> "\" is relative to the share. i.e. the share root.
Is that the right thing to do? Does the Samba server do this?
I believe the Windows server will fail such a path, but I can't
check right now.
Tom.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] ksmbd: use LOOKUP_BENEATH to prevent the out of share access
2021-09-29 15:01 ` Tom Talpey
@ 2021-09-29 23:52 ` Namjae Jeon
0 siblings, 0 replies; 11+ messages in thread
From: Namjae Jeon @ 2021-09-29 23:52 UTC (permalink / raw)
To: Tom Talpey
Cc: Hyunchul Lee, linux-cifs, Ronnie Sahlberg, Ralph Boehme, Steve French
2021-09-30 0:01 GMT+09:00, Tom Talpey <tom@talpey.com>:
> On 9/29/2021 8:40 AM, Namjae Jeon wrote:
>> 2021-09-29 0:18 GMT+09:00, Tom Talpey <tom@talpey.com>:
>>> On 9/24/2021 11:06 AM, Hyunchul Lee wrote:
>>>> instead of removing '..' in a given path, call
>>>> kern_path with LOOKUP_BENEATH flag to prevent
>>>> the out of share access.
>>>> <snip> <snip> <snip>
>>>> -char *convert_to_nt_pathname(char *filename, char *sharepath)
>>>> +char *convert_to_nt_pathname(char *filename)
>>>> {
>>>> char *ab_pathname;
>>>> - int len, name_len;
>>>>
>>>> - name_len = strlen(filename);
>>>> - ab_pathname = kmalloc(name_len, GFP_KERNEL);
>>>> - if (!ab_pathname)
>>>> - return NULL;
>>>> -
>>>> - ab_pathname[0] = '\\';
>>>> - ab_pathname[1] = '\0';
>>>> + if (strlen(filename) == 0) {
>>>> + ab_pathname = kmalloc(2, GFP_KERNEL);
>>>> + ab_pathname[0] = '\\';
>>>> + ab_pathname[1] = '\0';
>>>
>>> This converts the empty filename to "\" - the volume root!?
>> "\" is relative to the share. i.e. the share root.
>
> Is that the right thing to do? Does the Samba server do this?
>
> I believe the Windows server will fail such a path, but I can't
> check right now.
I am trying to check whether windows fail, but windows doesn't send
FILE_ALL_INFORMATION to ksmbd...
And smbtorture of samba have passed regardless of "/". So I didn't
probably notice such issue. I will fix it on another patch.
Thanks for your review!
>
> Tom.
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] ksmbd: use LOOKUP_BENEATH to prevent the out of share access
2021-09-28 15:18 ` Tom Talpey
2021-09-29 12:40 ` Namjae Jeon
@ 2021-09-30 0:01 ` Hyunchul Lee
1 sibling, 0 replies; 11+ messages in thread
From: Hyunchul Lee @ 2021-09-30 0:01 UTC (permalink / raw)
To: Tom Talpey
Cc: linux-cifs, Ronnie Sahlberg, Ralph Boehme, Steve French, Namjae Jeon
2021년 9월 29일 (수) 오전 12:18, Tom Talpey <tom@talpey.com>님이 작성:
>
> On 9/24/2021 11:06 AM, Hyunchul Lee wrote:
> > instead of removing '..' in a given path, call
> > kern_path with LOOKUP_BENEATH flag to prevent
> > the out of share access.
> >
> > ran various test on this:
> > smb2-cat-async smb://127.0.0.1/homes/../out_of_share
> > smb2-cat-async smb://127.0.0.1/homes/foo/../../out_of_share
> > smbclient //127.0.0.1/homes -c "mkdir ../foo2"
> > smbclient //127.0.0.1/homes -c "rename bar ../bar"
> >
> > Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> > Cc: Ralph Boehme <slow@samba.org>
> > Cc: Steve French <smfrench@gmail.com>
> > Cc: Namjae Jeon <linkinjeon@kernel.org>
> > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
> > ---
> > Changes from v1:
> > - pass path of file that is relative to share to ksmbd vfs functions.
> > Changes from v2:
> > - fix smbtorture smb2.streams.rename, smb2.streams.rename2 failure.
> > Changes from v3:
> > - fix uninitialized variable free in ksmbd_vfs_kern_path.
> >
> > fs/ksmbd/misc.c | 100 ++++++-----------------------
> > fs/ksmbd/misc.h | 7 +-
> > fs/ksmbd/smb2pdu.c | 74 ++++++++-------------
> > fs/ksmbd/vfs.c | 156 ++++++++++++++++++++++++---------------------
> > fs/ksmbd/vfs.h | 9 ++-
> > 5 files changed, 140 insertions(+), 206 deletions(-)
> >
> > diff --git a/fs/ksmbd/misc.c b/fs/ksmbd/misc.c
> > index 3eac3c01749f..6a19f4bc692d 100644
> > --- a/fs/ksmbd/misc.c
> > +++ b/fs/ksmbd/misc.c
> > @@ -158,25 +158,21 @@ int parse_stream_name(char *filename, char **stream_name, int *s_type)
> > * Return : windows path string or error
> > */
> >
> > -char *convert_to_nt_pathname(char *filename, char *sharepath)
> > +char *convert_to_nt_pathname(char *filename)
> > {
> > char *ab_pathname;
> > - int len, name_len;
> >
> > - name_len = strlen(filename);
> > - ab_pathname = kmalloc(name_len, GFP_KERNEL);
> > - if (!ab_pathname)
> > - return NULL;
> > -
> > - ab_pathname[0] = '\\';
> > - ab_pathname[1] = '\0';
> > + if (strlen(filename) == 0) {
> > + ab_pathname = kmalloc(2, GFP_KERNEL);
> > + ab_pathname[0] = '\\';
> > + ab_pathname[1] = '\0';
>
> This converts the empty filename to "\" - the volume root!?
>
> > + } else {
> > + ab_pathname = kstrdup(filename, GFP_KERNEL);
> > + if (!ab_pathname)
> > + return NULL;
> >
> > - len = strlen(sharepath);
> > - if (!strncmp(filename, sharepath, len) && name_len != len) {
> > - strscpy(ab_pathname, &filename[len], name_len);
> > ksmbd_conv_path_to_windows(ab_pathname);
> > }
> > -
> > return ab_pathname;
> > }
> >
> > @@ -191,77 +187,19 @@ int get_nlink(struct kstat *st)
> > return nlink;
> > }
> >
> > -char *ksmbd_conv_path_to_unix(char *path)
> > +void ksmbd_conv_path_to_unix(char *path)
> > {
> > - size_t path_len, remain_path_len, out_path_len;
> > - char *out_path, *out_next;
> > - int i, pre_dotdot_cnt = 0, slash_cnt = 0;
> > - bool is_last;
> > -
> > strreplace(path, '\\', '/');
> > - path_len = strlen(path);
> > - remain_path_len = path_len;
> > - if (path_len == 0)
> > - return ERR_PTR(-EINVAL);
> > -
> > - out_path = kzalloc(path_len + 2, GFP_KERNEL);
> > - if (!out_path)
> > - return ERR_PTR(-ENOMEM);
> > - out_path_len = 0;
> > - out_next = out_path;
> > -
> > - do {
> > - char *name = path + path_len - remain_path_len;
> > - char *next = strchrnul(name, '/');
> > - size_t name_len = next - name;
> > -
> > - is_last = !next[0];
> > - if (name_len == 2 && name[0] == '.' && name[1] == '.') {
> > - pre_dotdot_cnt++;
> > - /* handle the case that path ends with "/.." */
> > - if (is_last)
> > - goto follow_dotdot;
> > - } else {
> > - if (pre_dotdot_cnt) {
> > -follow_dotdot:
> > - slash_cnt = 0;
> > - for (i = out_path_len - 1; i >= 0; i--) {
> > - if (out_path[i] == '/' &&
> > - ++slash_cnt == pre_dotdot_cnt + 1)
> > - break;
> > - }
> > -
> > - if (i < 0 &&
> > - slash_cnt != pre_dotdot_cnt) {
> > - kfree(out_path);
> > - return ERR_PTR(-EINVAL);
> > - }
> > -
> > - out_next = &out_path[i+1];
> > - *out_next = '\0';
> > - out_path_len = i + 1;
> > -
> > - }
> > -
> > - if (name_len != 0 &&
> > - !(name_len == 1 && name[0] == '.') &&
> > - !(name_len == 2 && name[0] == '.' && name[1] == '.')) {
> > - next[0] = '\0';
> > - sprintf(out_next, "%s/", name);
> > - out_next += name_len + 1;
> > - out_path_len += name_len + 1;
> > - next[0] = '/';
> > - }
> > - pre_dotdot_cnt = 0;
> > - }
> > +}
> >
> > - remain_path_len -= name_len + 1;
> > - } while (!is_last);
> > +void ksmbd_strip_last_slash(char *path)
> > +{
> > + int len = strlen(path);
> >
> > - if (out_path_len > 0)
> > - out_path[out_path_len-1] = '\0';
> > - path[path_len] = '\0';
> > - return out_path;
> > + while (len && path[len - 1] == '/') {
> > + path[len - 1] = '\0';
> > + len--;
> > + }
>
> I guess it's intentional that "/////////" will be compacted to "/",
> but the open-coded nature of all this really troubles me.
this function removes all of trailing "/" from the path.
Do you mean we need to replace this function with string library's functions?
>
> > }
> >
> > void ksmbd_conv_path_to_windows(char *path)
> > @@ -298,7 +236,7 @@ char *ksmbd_extract_sharename(char *treename)
> > *
> > * Return: converted name on success, otherwise NULL
> > */
> > -char *convert_to_unix_name(struct ksmbd_share_config *share, char *name)
> > +char *convert_to_unix_name(struct ksmbd_share_config *share, const char *name)
> > {
> > int no_slash = 0, name_len, path_len;
> > char *new_name;
> > diff --git a/fs/ksmbd/misc.h b/fs/ksmbd/misc.h
> > index b7b10139ada2..253366bd0951 100644
> > --- a/fs/ksmbd/misc.h
> > +++ b/fs/ksmbd/misc.h
> > @@ -14,12 +14,13 @@ struct ksmbd_file;
> > int match_pattern(const char *str, size_t len, const char *pattern);
> > int ksmbd_validate_filename(char *filename);
> > int parse_stream_name(char *filename, char **stream_name, int *s_type);
> > -char *convert_to_nt_pathname(char *filename, char *sharepath);
> > +char *convert_to_nt_pathname(char *filename);
> > int get_nlink(struct kstat *st);
> > -char *ksmbd_conv_path_to_unix(char *path);
> > +void ksmbd_conv_path_to_unix(char *path);
> > +void ksmbd_strip_last_slash(char *path);
> > void ksmbd_conv_path_to_windows(char *path);
> > char *ksmbd_extract_sharename(char *treename);
> > -char *convert_to_unix_name(struct ksmbd_share_config *share, char *name);
> > +char *convert_to_unix_name(struct ksmbd_share_config *share, const char *name);
> >
> > #define KSMBD_DIR_INFO_ALIGNMENT 8
> > struct ksmbd_dir_info;
> > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> > index 378e0b4a216d..4c799fef9883 100644
> > --- a/fs/ksmbd/smb2pdu.c
> > +++ b/fs/ksmbd/smb2pdu.c
> > @@ -643,7 +643,7 @@ static char *
> > smb2_get_name(struct ksmbd_share_config *share, const char *src,
> > const int maxlen, struct nls_table *local_nls)
> > {
> > - char *name, *norm_name, *unixname;
> > + char *name;
> >
> > name = smb_strndup_from_utf16(src, maxlen, 1, local_nls);
> > if (IS_ERR(name)) {
> > @@ -651,23 +651,9 @@ smb2_get_name(struct ksmbd_share_config *share, const char *src,
> > return name;
> > }
> >
> > - /* change it to absolute unix name */
> > - norm_name = ksmbd_conv_path_to_unix(name);
> > - if (IS_ERR(norm_name)) {
> > - kfree(name);
> > - return norm_name;
> > - }
> > - kfree(name);
> > -
> > - unixname = convert_to_unix_name(share, norm_name);
> > - kfree(norm_name);
> > - if (!unixname) {
> > - pr_err("can not convert absolute name\n");
> > - return ERR_PTR(-ENOMEM);
> > - }
> > -
> > - ksmbd_debug(SMB, "absolute name = %s\n", unixname);
> > - return unixname;
> > + ksmbd_conv_path_to_unix(name);
> > + ksmbd_strip_last_slash(name);
> > + return name;
> > }
> >
> > int setup_async_work(struct ksmbd_work *work, void (*fn)(void **), void **arg)
> > @@ -2412,7 +2398,7 @@ static int smb2_creat(struct ksmbd_work *work, struct path *path, char *name,
> > return rc;
> > }
> >
> > - rc = ksmbd_vfs_kern_path(name, 0, path, 0);
> > + rc = ksmbd_vfs_kern_path(work, name, 0, path, 0);
> > if (rc) {
> > pr_err("cannot get linux path (%s), err = %d\n",
> > name, rc);
> > @@ -2487,7 +2473,7 @@ int smb2_open(struct ksmbd_work *work)
> > struct oplock_info *opinfo;
> > __le32 *next_ptr = NULL;
> > int req_op_level = 0, open_flags = 0, may_flags = 0, file_info = 0;
> > - int rc = 0, len = 0;
> > + int rc = 0;
> > int contxt_cnt = 0, query_disk_id = 0;
> > int maximal_access_ctxt = 0, posix_ctxt = 0;
> > int s_type = 0;
> > @@ -2559,17 +2545,11 @@ int smb2_open(struct ksmbd_work *work)
> > goto err_out1;
> > }
> > } else {
> > - len = strlen(share->path);
> > - ksmbd_debug(SMB, "share path len %d\n", len);
> > - name = kmalloc(len + 1, GFP_KERNEL);
> > + name = kstrdup("", GFP_KERNEL);
>
> This kstrdup's the empty string! Surely, this means to copy the sharepath?
If NameLengh is 0 in SMB2_CREATE request, this allocates an empty string
for the path, which is relative to share.
This is needed to avoid the check that the relative path is not NULL.
>
> > if (!name) {
> > - rsp->hdr.Status = STATUS_NO_MEMORY;
> > rc = -ENOMEM;
> > goto err_out1;
> > }
> > -
> > - memcpy(name, share->path, len);
> > - *(name + len) = '\0';
> > }
> >
> > req_op_level = req->RequestedOplockLevel;
> > @@ -2692,7 +2672,7 @@ int smb2_open(struct ksmbd_work *work)
> > goto err_out1;
> > }
> >
> > - rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1);
> > + rc = ksmbd_vfs_kern_path(work, name, LOOKUP_NO_SYMLINKS, &path, 1);
> > if (!rc) {
> > if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) {
> > /*
> > @@ -2721,11 +2701,8 @@ int smb2_open(struct ksmbd_work *work)
> > }
> >
> > if (rc) {
> > - if (rc == -EACCES) {
> > - ksmbd_debug(SMB,
> > - "User does not have right permission\n");
> > + if (rc != -ENOENT)
> > goto err_out;
> > - }
> > ksmbd_debug(SMB, "can not get linux path for %s, rc = %d\n",
> > name, rc);
> > rc = 0;
> > @@ -3229,7 +3206,7 @@ int smb2_open(struct ksmbd_work *work)
> > rsp->hdr.Status = STATUS_INVALID_PARAMETER;
> > else if (rc == -EOPNOTSUPP)
> > rsp->hdr.Status = STATUS_NOT_SUPPORTED;
> > - else if (rc == -EACCES || rc == -ESTALE)
> > + else if (rc == -EACCES || rc == -ESTALE || rc == -EXDEV)
> > rsp->hdr.Status = STATUS_ACCESS_DENIED;
> > else if (rc == -ENOENT)
> > rsp->hdr.Status = STATUS_OBJECT_NAME_INVALID;
> > @@ -4345,8 +4322,7 @@ static int get_file_all_info(struct ksmbd_work *work,
> > return -EACCES;
> > }
> >
> > - filename = convert_to_nt_pathname(fp->filename,
> > - work->tcon->share_conf->path);
> > + filename = convert_to_nt_pathname(fp->filename);
> > if (!filename)
> > return -ENOMEM;
> >
> > @@ -4801,7 +4777,7 @@ static int smb2_get_info_filesystem(struct ksmbd_work *work,
> > int rc = 0, len;
> > int fs_infoclass_size = 0;
> >
> > - rc = ksmbd_vfs_kern_path(share->path, LOOKUP_NO_SYMLINKS, &path, 0);
> > + rc = kern_path(share->path, LOOKUP_NO_SYMLINKS, &path);
> > if (rc) {
> > pr_err("cannot create vfs path\n");
> > return -EIO;
> > @@ -5350,7 +5326,7 @@ static int smb2_rename(struct ksmbd_work *work,
> > goto out;
> >
> > len = strlen(new_name);
> > - if (new_name[len - 1] != '/') {
> > + if (len > 0 && new_name[len - 1] != '/') {
> > pr_err("not allow base filename in rename\n");
> > rc = -ESHARE;
> > goto out;
> > @@ -5378,11 +5354,14 @@ static int smb2_rename(struct ksmbd_work *work,
> > }
> >
> > ksmbd_debug(SMB, "new name %s\n", new_name);
> > - rc = ksmbd_vfs_kern_path(new_name, LOOKUP_NO_SYMLINKS, &path, 1);
> > - if (rc)
> > + rc = ksmbd_vfs_kern_path(work, new_name, LOOKUP_NO_SYMLINKS, &path, 1);
> > + if (rc) {
> > + if (rc != -ENOENT)
> > + goto out;
> > file_present = false;
> > - else
> > + } else {
> > path_put(&path);
> > + }
> >
> > if (ksmbd_share_veto_filename(share, new_name)) {
> > rc = -ENOENT;
> > @@ -5456,11 +5435,14 @@ static int smb2_create_link(struct ksmbd_work *work,
> > }
> >
> > ksmbd_debug(SMB, "target name is %s\n", target_name);
> > - rc = ksmbd_vfs_kern_path(link_name, LOOKUP_NO_SYMLINKS, &path, 0);
> > - if (rc)
> > + rc = ksmbd_vfs_kern_path(work, link_name, LOOKUP_NO_SYMLINKS, &path, 0);
> > + if (rc) {
> > + if (rc != -ENOENT)
> > + goto out;
> > file_present = false;
> > - else
> > + } else {
> > path_put(&path);
> > + }
> >
> > if (file_info->ReplaceIfExists) {
> > if (file_present) {
> > @@ -5618,7 +5600,7 @@ static int set_file_allocation_info(struct ksmbd_work *work,
> > * inode size is retained by backup inode size.
> > */
> > size = i_size_read(inode);
> > - rc = ksmbd_vfs_truncate(work, NULL, fp, alloc_blks * 512);
> > + rc = ksmbd_vfs_truncate(work, fp, alloc_blks * 512);
> > if (rc) {
> > pr_err("truncate failed! filename : %s, err %d\n",
> > fp->filename, rc);
> > @@ -5653,7 +5635,7 @@ static int set_end_of_file_info(struct ksmbd_work *work, struct ksmbd_file *fp,
> > if (inode->i_sb->s_magic != MSDOS_SUPER_MAGIC) {
> > ksmbd_debug(SMB, "filename : %s truncated to newsize %lld\n",
> > fp->filename, newsize);
> > - rc = ksmbd_vfs_truncate(work, NULL, fp, newsize);
> > + rc = ksmbd_vfs_truncate(work, fp, newsize);
> > if (rc) {
> > ksmbd_debug(SMB, "truncate failed! filename : %s err %d\n",
> > fp->filename, rc);
> > @@ -5975,7 +5957,7 @@ int smb2_set_info(struct ksmbd_work *work)
> > return 0;
> >
> > err_out:
> > - if (rc == -EACCES || rc == -EPERM)
> > + if (rc == -EACCES || rc == -EPERM || rc == -EXDEV)
> > rsp->hdr.Status = STATUS_ACCESS_DENIED;
> > else if (rc == -EINVAL)
> > rsp->hdr.Status = STATUS_INVALID_PARAMETER;
> > diff --git a/fs/ksmbd/vfs.c b/fs/ksmbd/vfs.c
> > index 3733e4944c1d..b41954294d38 100644
> > --- a/fs/ksmbd/vfs.c
> > +++ b/fs/ksmbd/vfs.c
> > @@ -19,6 +19,8 @@
> > #include <linux/sched/xacct.h>
> > #include <linux/crc32c.h>
> >
> > +#include "../internal.h" /* for vfs_path_lookup */
> > +
> > #include "glob.h"
> > #include "oplock.h"
> > #include "connection.h"
> > @@ -44,7 +46,6 @@ static char *extract_last_component(char *path)
> > p++;
> > } else {
> > p = NULL;
> > - pr_err("Invalid path %s\n", path);
> > }
> > return p;
> > }
> > @@ -155,7 +156,7 @@ int ksmbd_vfs_query_maximal_access(struct user_namespace *user_ns,
> > /**
> > * ksmbd_vfs_create() - vfs helper for smb create file
> > * @work: work
> > - * @name: file name
> > + * @name: file name that is relative to share
> > * @mode: file create mode
> > *
> > * Return: 0 on success, otherwise error
> > @@ -166,7 +167,8 @@ int ksmbd_vfs_create(struct ksmbd_work *work, const char *name, umode_t mode)
> > struct dentry *dentry;
> > int err;
> >
> > - dentry = kern_path_create(AT_FDCWD, name, &path, LOOKUP_NO_SYMLINKS);
> > + dentry = ksmbd_vfs_kern_path_create(work, name,
> > + LOOKUP_NO_SYMLINKS, &path);
> > if (IS_ERR(dentry)) {
> > err = PTR_ERR(dentry);
> > if (err != -ENOENT)
> > @@ -191,7 +193,7 @@ int ksmbd_vfs_create(struct ksmbd_work *work, const char *name, umode_t mode)
> > /**
> > * ksmbd_vfs_mkdir() - vfs helper for smb create directory
> > * @work: work
> > - * @name: directory name
> > + * @name: directory name that is relative to share
> > * @mode: directory create mode
> > *
> > * Return: 0 on success, otherwise error
> > @@ -203,8 +205,9 @@ int ksmbd_vfs_mkdir(struct ksmbd_work *work, const char *name, umode_t mode)
> > struct dentry *dentry;
> > int err;
> >
> > - dentry = kern_path_create(AT_FDCWD, name, &path,
> > - LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY);
> > + dentry = ksmbd_vfs_kern_path_create(work, name,
> > + LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY,
> > + &path);
> > if (IS_ERR(dentry)) {
> > err = PTR_ERR(dentry);
> > if (err != -EEXIST)
> > @@ -579,7 +582,7 @@ int ksmbd_vfs_fsync(struct ksmbd_work *work, u64 fid, u64 p_id)
> >
> > /**
> > * ksmbd_vfs_remove_file() - vfs helper for smb rmdir or unlink
> > - * @name: absolute directory or file name
> > + * @name: directory or file name that is relative to share
> > *
> > * Return: 0 on success, otherwise error
> > */
> > @@ -593,7 +596,7 @@ int ksmbd_vfs_remove_file(struct ksmbd_work *work, char *name)
> > if (ksmbd_override_fsids(work))
> > return -ENOMEM;
> >
> > - err = kern_path(name, LOOKUP_NO_SYMLINKS, &path);
> > + err = ksmbd_vfs_kern_path(work, name, LOOKUP_NO_SYMLINKS, &path, false);
> > if (err) {
> > ksmbd_debug(VFS, "can't get %s, err %d\n", name, err);
> > ksmbd_revert_fsids(work);
> > @@ -638,7 +641,7 @@ int ksmbd_vfs_remove_file(struct ksmbd_work *work, char *name)
> > /**
> > * ksmbd_vfs_link() - vfs helper for creating smb hardlink
> > * @oldname: source file name
> > - * @newname: hardlink name
> > + * @newname: hardlink name that is relative to share
> > *
> > * Return: 0 on success, otherwise error
> > */
> > @@ -659,8 +662,9 @@ int ksmbd_vfs_link(struct ksmbd_work *work, const char *oldname,
> > goto out1;
> > }
> >
> > - dentry = kern_path_create(AT_FDCWD, newname, &newpath,
> > - LOOKUP_NO_SYMLINKS | LOOKUP_REVAL);
> > + dentry = ksmbd_vfs_kern_path_create(work, newname,
> > + LOOKUP_NO_SYMLINKS | LOOKUP_REVAL,
> > + &newpath);
> > if (IS_ERR(dentry)) {
> > err = PTR_ERR(dentry);
> > pr_err("path create err for %s, err %d\n", newname, err);
> > @@ -781,14 +785,17 @@ int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
> > int err;
> >
> > dst_name = extract_last_component(newname);
> > - if (!dst_name)
> > - return -EINVAL;
> > + if (!dst_name) {
> > + dst_name = newname;
> > + newname = "";
> > + }
> >
> > src_dent_parent = dget_parent(fp->filp->f_path.dentry);
> > src_dent = fp->filp->f_path.dentry;
> >
> > - err = kern_path(newname, LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY,
> > - &dst_path);
> > + err = ksmbd_vfs_kern_path(work, newname,
> > + LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY,
> > + &dst_path, false);
> > if (err) {
> > ksmbd_debug(VFS, "Cannot get path for %s [%d]\n", newname, err);
> > goto out;
> > @@ -834,61 +841,43 @@ int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
> > /**
> > * ksmbd_vfs_truncate() - vfs helper for smb file truncate
> > * @work: work
> > - * @name: old filename
> > * @fid: file id of old file
> > * @size: truncate to given size
> > *
> > * Return: 0 on success, otherwise error
> > */
> > -int ksmbd_vfs_truncate(struct ksmbd_work *work, const char *name,
> > +int ksmbd_vfs_truncate(struct ksmbd_work *work,
> > struct ksmbd_file *fp, loff_t size)
> > {
> > - struct path path;
> > int err = 0;
> > + struct file *filp;
> >
> > - if (name) {
> > - err = kern_path(name, LOOKUP_NO_SYMLINKS, &path);
> > - if (err) {
> > - pr_err("cannot get linux path for %s, err %d\n",
> > - name, err);
> > - return err;
> > - }
> > - err = vfs_truncate(&path, size);
> > - if (err)
> > - pr_err("truncate failed for %s err %d\n",
> > - name, err);
> > - path_put(&path);
> > - } else {
> > - struct file *filp;
> > -
> > - filp = fp->filp;
> > -
> > - /* Do we need to break any of a levelII oplock? */
> > - smb_break_all_levII_oplock(work, fp, 1);
> > + filp = fp->filp;
> >
> > - if (!work->tcon->posix_extensions) {
> > - struct inode *inode = file_inode(filp);
> > + /* Do we need to break any of a levelII oplock? */
> > + smb_break_all_levII_oplock(work, fp, 1);
> >
> > - if (size < inode->i_size) {
> > - err = check_lock_range(filp, size,
> > - inode->i_size - 1, WRITE);
> > - } else {
> > - err = check_lock_range(filp, inode->i_size,
> > - size - 1, WRITE);
> > - }
> > + if (!work->tcon->posix_extensions) {
> > + struct inode *inode = file_inode(filp);
> >
> > - if (err) {
> > - pr_err("failed due to lock\n");
> > - return -EAGAIN;
> > - }
> > + if (size < inode->i_size) {
> > + err = check_lock_range(filp, size,
> > + inode->i_size - 1, WRITE);
> > + } else {
> > + err = check_lock_range(filp, inode->i_size,
> > + size - 1, WRITE);
> > }
> >
> > - err = vfs_truncate(&filp->f_path, size);
> > - if (err)
> > - pr_err("truncate failed for filename : %s err %d\n",
> > - fp->filename, err);
> > + if (err) {
> > + pr_err("failed due to lock\n");
> > + return -EAGAIN;
> > + }
> > }
> >
> > + err = vfs_truncate(&filp->f_path, size);
> > + if (err)
> > + pr_err("truncate failed for filename : %s err %d\n",
> > + fp->filename, err);
> > return err;
> > }
> >
> > @@ -1206,22 +1195,25 @@ static int ksmbd_vfs_lookup_in_dir(struct path *dir, char *name, size_t namelen)
> >
> > /**
> > * ksmbd_vfs_kern_path() - lookup a file and get path info
> > - * @name: name of file for lookup
> > + * @name: file path that is relative to share
> > * @flags: lookup flags
> > * @path: if lookup succeed, return path info
> > * @caseless: caseless filename lookup
> > *
> > * Return: 0 on success, otherwise error
> > */
> > -int ksmbd_vfs_kern_path(char *name, unsigned int flags, struct path *path,
> > - bool caseless)
> > +int ksmbd_vfs_kern_path(struct ksmbd_work *work, char *name,
> > + unsigned int flags, struct path *path, bool caseless)
> > {
> > + struct ksmbd_share_config *share_conf = work->tcon->share_conf;
> > int err;
> >
> > - if (name[0] != '/')
> > - return -EINVAL;
> > -
> > - err = kern_path(name, flags, path);
> > + flags |= LOOKUP_BENEATH;
> > + err = vfs_path_lookup(share_conf->vfs_path.dentry,
> > + share_conf->vfs_path.mnt,
> > + name,
> > + flags,
> > + path);
> > if (!err)
> > return 0;
> >
> > @@ -1235,11 +1227,10 @@ int ksmbd_vfs_kern_path(char *name, unsigned int flags, struct path *path,
> > return -ENOMEM;
> >
> > path_len = strlen(filepath);
> > - remain_len = path_len - 1;
> > + remain_len = path_len;
> >
> > - err = kern_path("/", flags, &parent);
> > - if (err)
> > - goto out;
> > + parent = share_conf->vfs_path;
> > + path_get(&parent);
> >
> > while (d_can_lookup(parent.dentry)) {
> > char *filename = filepath + path_len - remain_len;
> > @@ -1252,21 +1243,21 @@ int ksmbd_vfs_kern_path(char *name, unsigned int flags, struct path *path,
> >
> > err = ksmbd_vfs_lookup_in_dir(&parent, filename,
> > filename_len);
> > - if (err) {
> > - path_put(&parent);
> > + path_put(&parent);
> > + if (err)
> > goto out;
> > - }
> >
> > - path_put(&parent);
> > next[0] = '\0';
> >
> > - err = kern_path(filepath, flags, &parent);
> > + err = vfs_path_lookup(share_conf->vfs_path.dentry,
> > + share_conf->vfs_path.mnt,
> > + filepath,
> > + flags,
> > + &parent);
> > if (err)
> > goto out;
> > -
> > - if (is_last) {
> > - path->mnt = parent.mnt;
> > - path->dentry = parent.dentry;
> > + else if (is_last) {
> > + *path = parent;
> > goto out;
> > }
> >
> > @@ -1282,6 +1273,23 @@ int ksmbd_vfs_kern_path(char *name, unsigned int flags, struct path *path,
> > return err;
> > }
> >
> > +struct dentry *ksmbd_vfs_kern_path_create(struct ksmbd_work *work,
> > + const char *name,
> > + unsigned int flags,
> > + struct path *path)
> > +{
> > + char *abs_name;
> > + struct dentry *dent;
> > +
> > + abs_name = convert_to_unix_name(work->tcon->share_conf, name);
> > + if (!abs_name)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + dent = kern_path_create(AT_FDCWD, abs_name, path, flags);
> > + kfree(abs_name);
> > + return dent;
> > +}
> > +
> > int ksmbd_vfs_remove_acl_xattrs(struct user_namespace *user_ns,
> > struct dentry *dentry)
> > {
> > diff --git a/fs/ksmbd/vfs.h b/fs/ksmbd/vfs.h
> > index 85db50abdb24..7b1dcaa3fbdc 100644
> > --- a/fs/ksmbd/vfs.h
> > +++ b/fs/ksmbd/vfs.h
> > @@ -126,7 +126,7 @@ int ksmbd_vfs_link(struct ksmbd_work *work,
> > int ksmbd_vfs_getattr(struct path *path, struct kstat *stat);
> > int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
> > char *newname);
> > -int ksmbd_vfs_truncate(struct ksmbd_work *work, const char *name,
> > +int ksmbd_vfs_truncate(struct ksmbd_work *work,
> > struct ksmbd_file *fp, loff_t size);
> > struct srv_copychunk;
> > int ksmbd_vfs_copy_file_ranges(struct ksmbd_work *work,
> > @@ -152,8 +152,13 @@ int ksmbd_vfs_xattr_stream_name(char *stream_name, char **xattr_stream_name,
> > size_t *xattr_stream_name_size, int s_type);
> > int ksmbd_vfs_remove_xattr(struct user_namespace *user_ns,
> > struct dentry *dentry, char *attr_name);
> > -int ksmbd_vfs_kern_path(char *name, unsigned int flags, struct path *path,
> > +int ksmbd_vfs_kern_path(struct ksmbd_work *work,
> > + char *name, unsigned int flags, struct path *path,
> > bool caseless);
> > +struct dentry *ksmbd_vfs_kern_path_create(struct ksmbd_work *work,
> > + const char *name,
> > + unsigned int flags,
> > + struct path *path);
> > int ksmbd_vfs_empty_dir(struct ksmbd_file *fp);
> > void ksmbd_vfs_set_fadvise(struct file *filp, __le32 option);
> > int ksmbd_vfs_zero_data(struct ksmbd_work *work, struct ksmbd_file *fp,
> >
--
Thanks,
Hyunchul
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-09-30 0:01 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-24 15:06 [PATCH v4] ksmbd: use LOOKUP_BENEATH to prevent the out of share access Hyunchul Lee
2021-09-24 17:20 ` Jeremy Allison
2021-09-25 0:08 ` Namjae Jeon
2021-09-25 0:21 ` Namjae Jeon
2021-09-25 0:43 ` Namjae Jeon
2021-09-25 17:45 ` Steve French
2021-09-28 15:18 ` Tom Talpey
2021-09-29 12:40 ` Namjae Jeon
2021-09-29 15:01 ` Tom Talpey
2021-09-29 23:52 ` Namjae Jeon
2021-09-30 0:01 ` Hyunchul Lee
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).