* [PATCH v2] ksmbd: remove follow symlinks support
@ 2021-09-21 4:40 Namjae Jeon
2021-09-21 4:40 ` [PATCH v2] ksmbd: add request buffer validation in smb2_set_info Namjae Jeon
2021-09-21 4:49 ` [PATCH v2] ksmbd: remove follow symlinks support Leif Sahlberg
0 siblings, 2 replies; 6+ messages in thread
From: Namjae Jeon @ 2021-09-21 4:40 UTC (permalink / raw)
To: linux-cifs; +Cc: Namjae Jeon, Ronnie Sahlberg, Ralph Böhme, Steve French
This patch remove symlink support that can be vulnerable and access out
of share, and we re-implement it as reparse point later.
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
v2:
- reorganize path lookup in smb2_open to call only one
ksmbd_vfs_kern_path().
fs/ksmbd/smb2pdu.c | 60 ++++++++++------------------------------------
fs/ksmbd/vfs.c | 50 ++++++++------------------------------
2 files changed, 22 insertions(+), 88 deletions(-)
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index e08c6b26b6f0..de044802fc5b 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -2648,19 +2648,9 @@ int smb2_open(struct ksmbd_work *work)
goto err_out1;
}
- if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) {
- if (test_share_config_flag(work->tcon->share_conf,
- KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) {
- /*
- * On delete request, instead of following up, need to
- * look the current entity
- */
- rc = ksmbd_vfs_kern_path(name, 0, &path, 1);
- } else {
- rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1);
- }
-
- if (!rc) {
+ rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1);
+ if (!rc) {
+ if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) {
/*
* If file exists with under flags, return access
* denied error.
@@ -2679,26 +2669,10 @@ int smb2_open(struct ksmbd_work *work)
path_put(&path);
goto err_out;
}
- }
- } else {
- if (test_share_config_flag(work->tcon->share_conf,
- KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) {
- /*
- * Use LOOKUP_FOLLOW to follow the path of
- * symlink in path buildup
- */
- rc = ksmbd_vfs_kern_path(name, LOOKUP_FOLLOW, &path, 1);
- if (rc) { /* Case for broken link ?*/
- rc = ksmbd_vfs_kern_path(name, 0, &path, 1);
- }
- } else {
- rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS,
- &path, 1);
- if (!rc && d_is_symlink(path.dentry)) {
- rc = -EACCES;
- path_put(&path);
- goto err_out;
- }
+ } else if (d_is_symlink(path.dentry)) {
+ rc = -EACCES;
+ path_put(&path);
+ goto err_out;
}
}
@@ -4781,12 +4755,8 @@ static int smb2_get_info_filesystem(struct ksmbd_work *work,
struct path path;
int rc = 0, len;
int fs_infoclass_size = 0;
- int lookup_flags = LOOKUP_NO_SYMLINKS;
-
- if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
- lookup_flags = LOOKUP_FOLLOW;
- rc = ksmbd_vfs_kern_path(share->path, lookup_flags, &path, 0);
+ rc = ksmbd_vfs_kern_path(share->path, LOOKUP_NO_SYMLINKS, &path, 0);
if (rc) {
pr_err("cannot create vfs path\n");
return -EIO;
@@ -5293,7 +5263,7 @@ static int smb2_rename(struct ksmbd_work *work,
char *pathname = NULL;
struct path path;
bool file_present = true;
- int rc, lookup_flags = LOOKUP_NO_SYMLINKS;
+ int rc;
ksmbd_debug(SMB, "setting FILE_RENAME_INFO\n");
pathname = kmalloc(PATH_MAX, GFP_KERNEL);
@@ -5362,11 +5332,8 @@ static int smb2_rename(struct ksmbd_work *work,
goto out;
}
- if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
- lookup_flags = LOOKUP_FOLLOW;
-
ksmbd_debug(SMB, "new name %s\n", new_name);
- rc = ksmbd_vfs_kern_path(new_name, lookup_flags, &path, 1);
+ rc = ksmbd_vfs_kern_path(new_name, LOOKUP_NO_SYMLINKS, &path, 1);
if (rc)
file_present = false;
else
@@ -5416,7 +5383,7 @@ static int smb2_create_link(struct ksmbd_work *work,
char *link_name = NULL, *target_name = NULL, *pathname = NULL;
struct path path;
bool file_present = true;
- int rc, lookup_flags = LOOKUP_NO_SYMLINKS;
+ int rc;
ksmbd_debug(SMB, "setting FILE_LINK_INFORMATION\n");
pathname = kmalloc(PATH_MAX, GFP_KERNEL);
@@ -5439,11 +5406,8 @@ static int smb2_create_link(struct ksmbd_work *work,
goto out;
}
- if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
- lookup_flags = LOOKUP_FOLLOW;
-
ksmbd_debug(SMB, "target name is %s\n", target_name);
- rc = ksmbd_vfs_kern_path(link_name, lookup_flags, &path, 0);
+ rc = ksmbd_vfs_kern_path(link_name, LOOKUP_NO_SYMLINKS, &path, 0);
if (rc)
file_present = false;
else
diff --git a/fs/ksmbd/vfs.c b/fs/ksmbd/vfs.c
index 53047f013371..3733e4944c1d 100644
--- a/fs/ksmbd/vfs.c
+++ b/fs/ksmbd/vfs.c
@@ -164,13 +164,9 @@ int ksmbd_vfs_create(struct ksmbd_work *work, const char *name, umode_t mode)
{
struct path path;
struct dentry *dentry;
- int err, lookup_flags = LOOKUP_NO_SYMLINKS;
-
- if (test_share_config_flag(work->tcon->share_conf,
- KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
- lookup_flags = LOOKUP_FOLLOW;
+ int err;
- dentry = kern_path_create(AT_FDCWD, name, &path, lookup_flags);
+ dentry = kern_path_create(AT_FDCWD, name, &path, LOOKUP_NO_SYMLINKS);
if (IS_ERR(dentry)) {
err = PTR_ERR(dentry);
if (err != -ENOENT)
@@ -205,14 +201,10 @@ int ksmbd_vfs_mkdir(struct ksmbd_work *work, const char *name, umode_t mode)
struct user_namespace *user_ns;
struct path path;
struct dentry *dentry;
- int err, lookup_flags = LOOKUP_NO_SYMLINKS;
-
- if (test_share_config_flag(work->tcon->share_conf,
- KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
- lookup_flags = LOOKUP_FOLLOW;
+ int err;
dentry = kern_path_create(AT_FDCWD, name, &path,
- lookup_flags | LOOKUP_DIRECTORY);
+ LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY);
if (IS_ERR(dentry)) {
err = PTR_ERR(dentry);
if (err != -EEXIST)
@@ -597,16 +589,11 @@ int ksmbd_vfs_remove_file(struct ksmbd_work *work, char *name)
struct path path;
struct dentry *parent;
int err;
- int flags = LOOKUP_NO_SYMLINKS;
if (ksmbd_override_fsids(work))
return -ENOMEM;
- if (test_share_config_flag(work->tcon->share_conf,
- KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
- flags = LOOKUP_FOLLOW;
-
- err = kern_path(name, flags, &path);
+ err = kern_path(name, LOOKUP_NO_SYMLINKS, &path);
if (err) {
ksmbd_debug(VFS, "can't get %s, err %d\n", name, err);
ksmbd_revert_fsids(work);
@@ -661,16 +648,11 @@ int ksmbd_vfs_link(struct ksmbd_work *work, const char *oldname,
struct path oldpath, newpath;
struct dentry *dentry;
int err;
- int flags = LOOKUP_NO_SYMLINKS;
if (ksmbd_override_fsids(work))
return -ENOMEM;
- if (test_share_config_flag(work->tcon->share_conf,
- KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
- flags = LOOKUP_FOLLOW;
-
- err = kern_path(oldname, flags, &oldpath);
+ err = kern_path(oldname, LOOKUP_NO_SYMLINKS, &oldpath);
if (err) {
pr_err("cannot get linux path for %s, err = %d\n",
oldname, err);
@@ -678,7 +660,7 @@ int ksmbd_vfs_link(struct ksmbd_work *work, const char *oldname,
}
dentry = kern_path_create(AT_FDCWD, newname, &newpath,
- flags | LOOKUP_REVAL);
+ LOOKUP_NO_SYMLINKS | LOOKUP_REVAL);
if (IS_ERR(dentry)) {
err = PTR_ERR(dentry);
pr_err("path create err for %s, err %d\n", newname, err);
@@ -797,7 +779,6 @@ int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
struct dentry *src_dent, *trap_dent, *src_child;
char *dst_name;
int err;
- int flags = LOOKUP_NO_SYMLINKS;
dst_name = extract_last_component(newname);
if (!dst_name)
@@ -806,13 +787,8 @@ int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
src_dent_parent = dget_parent(fp->filp->f_path.dentry);
src_dent = fp->filp->f_path.dentry;
- flags = LOOKUP_DIRECTORY;
- if (test_share_config_flag(work->tcon->share_conf,
- KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
- flags = LOOKUP_FOLLOW;
- flags |= LOOKUP_DIRECTORY;
-
- err = kern_path(newname, flags, &dst_path);
+ err = kern_path(newname, LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY,
+ &dst_path);
if (err) {
ksmbd_debug(VFS, "Cannot get path for %s [%d]\n", newname, err);
goto out;
@@ -871,13 +847,7 @@ int ksmbd_vfs_truncate(struct ksmbd_work *work, const char *name,
int err = 0;
if (name) {
- int flags = LOOKUP_NO_SYMLINKS;
-
- if (test_share_config_flag(work->tcon->share_conf,
- KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
- flags = LOOKUP_FOLLOW;
-
- err = kern_path(name, flags, &path);
+ err = kern_path(name, LOOKUP_NO_SYMLINKS, &path);
if (err) {
pr_err("cannot get linux path for %s, err %d\n",
name, err);
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2] ksmbd: add request buffer validation in smb2_set_info
2021-09-21 4:40 [PATCH v2] ksmbd: remove follow symlinks support Namjae Jeon
@ 2021-09-21 4:40 ` Namjae Jeon
2021-09-21 7:38 ` Ralph Boehme
2021-09-21 4:49 ` [PATCH v2] ksmbd: remove follow symlinks support Leif Sahlberg
1 sibling, 1 reply; 6+ messages in thread
From: Namjae Jeon @ 2021-09-21 4:40 UTC (permalink / raw)
To: linux-cifs; +Cc: Namjae Jeon, Ronnie Sahlberg, Ralph Böhme, Steve French
Add buffer validation in smb2_set_info.
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
v2:
- smb2_set_info infolevel functions take structure pointer argument.
fs/ksmbd/smb2pdu.c | 141 ++++++++++++++++++++++++++++++++-------------
fs/ksmbd/smb2pdu.h | 9 +++
2 files changed, 109 insertions(+), 41 deletions(-)
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index de044802fc5b..e26e6b29e655 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -2107,17 +2107,23 @@ static noinline int create_smb2_pipe(struct ksmbd_work *work)
* smb2_set_ea() - handler for setting extended attributes using set
* info command
* @eabuf: set info command buffer
+ * @buf_len: set info command buffer length
* @path: dentry path for get ea
*
* Return: 0 on success, otherwise error
*/
-static int smb2_set_ea(struct smb2_ea_info *eabuf, struct path *path)
+static int smb2_set_ea(struct smb2_ea_info *eabuf, unsigned int buf_len,
+ struct path *path)
{
struct user_namespace *user_ns = mnt_user_ns(path->mnt);
char *attr_name = NULL, *value;
int rc = 0;
int next = 0;
+ if (buf_len < sizeof(struct smb2_ea_info) + eabuf->EaNameLength +
+ le16_to_cpu(eabuf->EaValueLength))
+ return -EINVAL;
+
attr_name = kmalloc(XATTR_NAME_MAX + 1, GFP_KERNEL);
if (!attr_name)
return -ENOMEM;
@@ -2181,7 +2187,13 @@ static int smb2_set_ea(struct smb2_ea_info *eabuf, struct path *path)
next:
next = le32_to_cpu(eabuf->NextEntryOffset);
+ if (next == 0 || buf_len < next)
+ break;
+ buf_len -= next;
eabuf = (struct smb2_ea_info *)((char *)eabuf + next);
+ if (next < eabuf->EaNameLength + le16_to_cpu(eabuf->EaValueLength))
+ break;
+
} while (next != 0);
kfree(attr_name);
@@ -2787,7 +2799,9 @@ int smb2_open(struct ksmbd_work *work)
created = true;
user_ns = mnt_user_ns(path.mnt);
if (ea_buf) {
- rc = smb2_set_ea(&ea_buf->ea, &path);
+ rc = smb2_set_ea(&ea_buf->ea,
+ le32_to_cpu(ea_buf->ccontext.DataLength),
+ &path);
if (rc == -EOPNOTSUPP)
rc = 0;
else if (rc)
@@ -5377,7 +5391,7 @@ static int smb2_rename(struct ksmbd_work *work,
static int smb2_create_link(struct ksmbd_work *work,
struct ksmbd_share_config *share,
struct smb2_file_link_info *file_info,
- struct file *filp,
+ int buf_len, struct file *filp,
struct nls_table *local_nls)
{
char *link_name = NULL, *target_name = NULL, *pathname = NULL;
@@ -5385,6 +5399,10 @@ static int smb2_create_link(struct ksmbd_work *work,
bool file_present = true;
int rc;
+ if (buf_len < sizeof(struct smb2_file_link_info) +
+ le32_to_cpu(file_info->FileNameLength))
+ return -EINVAL;
+
ksmbd_debug(SMB, "setting FILE_LINK_INFORMATION\n");
pathname = kmalloc(PATH_MAX, GFP_KERNEL);
if (!pathname)
@@ -5441,10 +5459,10 @@ static int smb2_create_link(struct ksmbd_work *work,
return rc;
}
-static int set_file_basic_info(struct ksmbd_file *fp, char *buf,
+static int set_file_basic_info(struct ksmbd_file *fp,
+ struct smb2_file_basic_info *file_info,
struct ksmbd_share_config *share)
{
- struct smb2_file_all_info *file_info;
struct iattr attrs;
struct timespec64 ctime;
struct file *filp;
@@ -5455,7 +5473,6 @@ static int set_file_basic_info(struct ksmbd_file *fp, char *buf,
if (!(fp->daccess & FILE_WRITE_ATTRIBUTES_LE))
return -EACCES;
- file_info = (struct smb2_file_all_info *)buf;
attrs.ia_valid = 0;
filp = fp->filp;
inode = file_inode(filp);
@@ -5532,7 +5549,8 @@ static int set_file_basic_info(struct ksmbd_file *fp, char *buf,
}
static int set_file_allocation_info(struct ksmbd_work *work,
- struct ksmbd_file *fp, char *buf)
+ struct ksmbd_file *fp,
+ struct smb2_file_alloc_info *file_alloc_info)
{
/*
* TODO : It's working fine only when store dos attributes
@@ -5540,7 +5558,6 @@ static int set_file_allocation_info(struct ksmbd_work *work,
* properly with any smb.conf option
*/
- struct smb2_file_alloc_info *file_alloc_info;
loff_t alloc_blks;
struct inode *inode;
int rc;
@@ -5548,7 +5565,6 @@ static int set_file_allocation_info(struct ksmbd_work *work,
if (!(fp->daccess & FILE_WRITE_DATA_LE))
return -EACCES;
- file_alloc_info = (struct smb2_file_alloc_info *)buf;
alloc_blks = (le64_to_cpu(file_alloc_info->AllocationSize) + 511) >> 9;
inode = file_inode(fp->filp);
@@ -5584,9 +5600,8 @@ static int set_file_allocation_info(struct ksmbd_work *work,
}
static int set_end_of_file_info(struct ksmbd_work *work, struct ksmbd_file *fp,
- char *buf)
+ struct smb2_file_eof_info *file_eof_info)
{
- struct smb2_file_eof_info *file_eof_info;
loff_t newsize;
struct inode *inode;
int rc;
@@ -5594,7 +5609,6 @@ static int set_end_of_file_info(struct ksmbd_work *work, struct ksmbd_file *fp,
if (!(fp->daccess & FILE_WRITE_DATA_LE))
return -EACCES;
- file_eof_info = (struct smb2_file_eof_info *)buf;
newsize = le64_to_cpu(file_eof_info->EndOfFile);
inode = file_inode(fp->filp);
@@ -5621,7 +5635,8 @@ static int set_end_of_file_info(struct ksmbd_work *work, struct ksmbd_file *fp,
}
static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp,
- char *buf)
+ struct smb2_file_rename_info *rename_info,
+ int buf_len)
{
struct user_namespace *user_ns;
struct ksmbd_file *parent_fp;
@@ -5634,6 +5649,10 @@ static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp,
return -EACCES;
}
+ if (buf_len < sizeof(struct smb2_file_rename_info) +
+ le32_to_cpu(rename_info->FileNameLength))
+ return -EINVAL;
+
user_ns = file_mnt_user_ns(fp->filp);
if (ksmbd_stream_fd(fp))
goto next;
@@ -5656,14 +5675,13 @@ static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp,
}
}
next:
- return smb2_rename(work, fp, user_ns,
- (struct smb2_file_rename_info *)buf,
+ return smb2_rename(work, fp, user_ns, rename_info,
work->sess->conn->local_nls);
}
-static int set_file_disposition_info(struct ksmbd_file *fp, char *buf)
+static int set_file_disposition_info(struct ksmbd_file *fp,
+ struct smb2_file_disposition_info *file_info)
{
- struct smb2_file_disposition_info *file_info;
struct inode *inode;
if (!(fp->daccess & FILE_DELETE_LE)) {
@@ -5672,7 +5690,6 @@ static int set_file_disposition_info(struct ksmbd_file *fp, char *buf)
}
inode = file_inode(fp->filp);
- file_info = (struct smb2_file_disposition_info *)buf;
if (file_info->DeletePending) {
if (S_ISDIR(inode->i_mode) &&
ksmbd_vfs_empty_dir(fp) == -ENOTEMPTY)
@@ -5684,15 +5701,14 @@ static int set_file_disposition_info(struct ksmbd_file *fp, char *buf)
return 0;
}
-static int set_file_position_info(struct ksmbd_file *fp, char *buf)
+static int set_file_position_info(struct ksmbd_file *fp,
+ struct smb2_file_pos_info *file_info)
{
- struct smb2_file_pos_info *file_info;
loff_t current_byte_offset;
unsigned long sector_size;
struct inode *inode;
inode = file_inode(fp->filp);
- file_info = (struct smb2_file_pos_info *)buf;
current_byte_offset = le64_to_cpu(file_info->CurrentByteOffset);
sector_size = inode->i_sb->s_blocksize;
@@ -5708,12 +5724,11 @@ static int set_file_position_info(struct ksmbd_file *fp, char *buf)
return 0;
}
-static int set_file_mode_info(struct ksmbd_file *fp, char *buf)
+static int set_file_mode_info(struct ksmbd_file *fp,
+ struct smb2_file_mode_info *file_info)
{
- struct smb2_file_mode_info *file_info;
__le32 mode;
- file_info = (struct smb2_file_mode_info *)buf;
mode = file_info->Mode;
if ((mode & ~FILE_MODE_INFO_MASK) ||
@@ -5743,40 +5758,74 @@ static int set_file_mode_info(struct ksmbd_file *fp, char *buf)
* TODO: need to implement an error handling for STATUS_INFO_LENGTH_MISMATCH
*/
static int smb2_set_info_file(struct ksmbd_work *work, struct ksmbd_file *fp,
- int info_class, char *buf,
+ struct smb2_set_info_req *req,
struct ksmbd_share_config *share)
{
- switch (info_class) {
+ int buf_len = le32_to_cpu(req->BufferLength);
+
+ switch (req->FileInfoClass) {
case FILE_BASIC_INFORMATION:
- return set_file_basic_info(fp, buf, share);
+ {
+ if (buf_len < sizeof(struct smb2_file_basic_info))
+ return -EINVAL;
+ return set_file_basic_info(fp, (struct smb2_file_basic_info *)req->Buffer, share);
+ }
case FILE_ALLOCATION_INFORMATION:
- return set_file_allocation_info(work, fp, buf);
+ {
+ if (buf_len < sizeof(struct smb2_file_alloc_info))
+ return -EINVAL;
+ return set_file_allocation_info(work, fp,
+ (struct smb2_file_alloc_info *)req->Buffer);
+ }
case FILE_END_OF_FILE_INFORMATION:
- return set_end_of_file_info(work, fp, buf);
+ {
+ if (buf_len < sizeof(struct smb2_file_eof_info))
+ return -EINVAL;
+ return set_end_of_file_info(work, fp,
+ (struct smb2_file_eof_info *)req->Buffer);
+ }
case FILE_RENAME_INFORMATION:
+ {
if (!test_tree_conn_flag(work->tcon, KSMBD_TREE_CONN_FLAG_WRITABLE)) {
ksmbd_debug(SMB,
"User does not have write permission\n");
return -EACCES;
}
- return set_rename_info(work, fp, buf);
+ if (buf_len < sizeof(struct smb2_file_rename_info))
+ return -EINVAL;
+
+ return set_rename_info(work, fp,
+ (struct smb2_file_rename_info *)req->Buffer,
+ buf_len);
+ }
case FILE_LINK_INFORMATION:
+ {
+ if (buf_len < sizeof(struct smb2_file_link_info))
+ return -EINVAL;
+
return smb2_create_link(work, work->tcon->share_conf,
- (struct smb2_file_link_info *)buf, fp->filp,
+ (struct smb2_file_link_info *)req->Buffer,
+ buf_len, fp->filp,
work->sess->conn->local_nls);
-
+ }
case FILE_DISPOSITION_INFORMATION:
+ {
if (!test_tree_conn_flag(work->tcon, KSMBD_TREE_CONN_FLAG_WRITABLE)) {
ksmbd_debug(SMB,
"User does not have write permission\n");
return -EACCES;
}
- return set_file_disposition_info(fp, buf);
+ if (buf_len < sizeof(struct smb2_file_disposition_info))
+ return -EINVAL;
+
+ return set_file_disposition_info(fp,
+ (struct smb2_file_disposition_info *)req->Buffer);
+ }
case FILE_FULL_EA_INFORMATION:
{
if (!(fp->daccess & FILE_WRITE_EA_LE)) {
@@ -5785,18 +5834,29 @@ static int smb2_set_info_file(struct ksmbd_work *work, struct ksmbd_file *fp,
return -EACCES;
}
- return smb2_set_ea((struct smb2_ea_info *)buf,
- &fp->filp->f_path);
- }
+ if (buf_len < sizeof(struct smb2_ea_info))
+ return -EINVAL;
+ return smb2_set_ea((struct smb2_ea_info *)req->Buffer,
+ buf_len, &fp->filp->f_path);
+ }
case FILE_POSITION_INFORMATION:
- return set_file_position_info(fp, buf);
+ {
+ if (buf_len < sizeof(struct smb2_file_pos_info))
+ return -EINVAL;
+ return set_file_position_info(fp, (struct smb2_file_pos_info *)req->Buffer);
+ }
case FILE_MODE_INFORMATION:
- return set_file_mode_info(fp, buf);
+ {
+ if (buf_len < sizeof(struct smb2_file_mode_info))
+ return -EINVAL;
+
+ return set_file_mode_info(fp, (struct smb2_file_mode_info *)req->Buffer);
+ }
}
- pr_err("Unimplemented Fileinfoclass :%d\n", info_class);
+ pr_err("Unimplemented Fileinfoclass :%d\n", req->FileInfoClass);
return -EOPNOTSUPP;
}
@@ -5857,8 +5917,7 @@ int smb2_set_info(struct ksmbd_work *work)
switch (req->InfoType) {
case SMB2_O_INFO_FILE:
ksmbd_debug(SMB, "GOT SMB2_O_INFO_FILE\n");
- rc = smb2_set_info_file(work, fp, req->FileInfoClass,
- req->Buffer, work->tcon->share_conf);
+ rc = smb2_set_info_file(work, fp, req, work->tcon->share_conf);
break;
case SMB2_O_INFO_SECURITY:
ksmbd_debug(SMB, "GOT SMB2_O_INFO_SECURITY\n");
diff --git a/fs/ksmbd/smb2pdu.h b/fs/ksmbd/smb2pdu.h
index bcec845b03f3..261825d06391 100644
--- a/fs/ksmbd/smb2pdu.h
+++ b/fs/ksmbd/smb2pdu.h
@@ -1464,6 +1464,15 @@ struct smb2_file_all_info { /* data block encoding of response to level 18 */
char FileName[1];
} __packed; /* level 18 Query */
+struct smb2_file_basic_info { /* data block encoding of response to level 18 */
+ __le64 CreationTime; /* Beginning of FILE_BASIC_INFO equivalent */
+ __le64 LastAccessTime;
+ __le64 LastWriteTime;
+ __le64 ChangeTime;
+ __le32 Attributes;
+ __u32 Pad1; /* End of FILE_BASIC_INFO_INFO equivalent */
+} __packed;
+
struct smb2_file_alt_name_info {
__le32 FileNameLength;
char FileName[0];
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ksmbd: remove follow symlinks support
2021-09-21 4:40 [PATCH v2] ksmbd: remove follow symlinks support Namjae Jeon
2021-09-21 4:40 ` [PATCH v2] ksmbd: add request buffer validation in smb2_set_info Namjae Jeon
@ 2021-09-21 4:49 ` Leif Sahlberg
1 sibling, 0 replies; 6+ messages in thread
From: Leif Sahlberg @ 2021-09-21 4:49 UTC (permalink / raw)
To: Namjae Jeon; +Cc: linux-cifs, Ronnie Sahlberg, Ralph Böhme, Steve French
On Tue, Sep 21, 2021 at 2:41 PM Namjae Jeon <linkinjeon@kernel.org> wrote:
>
> This patch remove symlink support that can be vulnerable and access out
> of share, and we re-implement it as reparse point later.
Very good.
Acked-by: Ronnie Sahlberg <lsahlber@redhat.com>
>
>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
> v2:
> - reorganize path lookup in smb2_open to call only one
> ksmbd_vfs_kern_path().
>
> fs/ksmbd/smb2pdu.c | 60 ++++++++++------------------------------------
> fs/ksmbd/vfs.c | 50 ++++++++------------------------------
> 2 files changed, 22 insertions(+), 88 deletions(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index e08c6b26b6f0..de044802fc5b 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -2648,19 +2648,9 @@ int smb2_open(struct ksmbd_work *work)
> goto err_out1;
> }
>
> - if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) {
> - if (test_share_config_flag(work->tcon->share_conf,
> - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) {
> - /*
> - * On delete request, instead of following up, need to
> - * look the current entity
> - */
> - rc = ksmbd_vfs_kern_path(name, 0, &path, 1);
> - } else {
> - rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1);
> - }
> -
> - if (!rc) {
> + rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1);
> + if (!rc) {
> + if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) {
> /*
> * If file exists with under flags, return access
> * denied error.
> @@ -2679,26 +2669,10 @@ int smb2_open(struct ksmbd_work *work)
> path_put(&path);
> goto err_out;
> }
> - }
> - } else {
> - if (test_share_config_flag(work->tcon->share_conf,
> - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) {
> - /*
> - * Use LOOKUP_FOLLOW to follow the path of
> - * symlink in path buildup
> - */
> - rc = ksmbd_vfs_kern_path(name, LOOKUP_FOLLOW, &path, 1);
> - if (rc) { /* Case for broken link ?*/
> - rc = ksmbd_vfs_kern_path(name, 0, &path, 1);
> - }
> - } else {
> - rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS,
> - &path, 1);
> - if (!rc && d_is_symlink(path.dentry)) {
> - rc = -EACCES;
> - path_put(&path);
> - goto err_out;
> - }
> + } else if (d_is_symlink(path.dentry)) {
> + rc = -EACCES;
> + path_put(&path);
> + goto err_out;
> }
> }
>
> @@ -4781,12 +4755,8 @@ static int smb2_get_info_filesystem(struct ksmbd_work *work,
> struct path path;
> int rc = 0, len;
> int fs_infoclass_size = 0;
> - int lookup_flags = LOOKUP_NO_SYMLINKS;
> -
> - if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
> - lookup_flags = LOOKUP_FOLLOW;
>
> - rc = ksmbd_vfs_kern_path(share->path, lookup_flags, &path, 0);
> + rc = ksmbd_vfs_kern_path(share->path, LOOKUP_NO_SYMLINKS, &path, 0);
> if (rc) {
> pr_err("cannot create vfs path\n");
> return -EIO;
> @@ -5293,7 +5263,7 @@ static int smb2_rename(struct ksmbd_work *work,
> char *pathname = NULL;
> struct path path;
> bool file_present = true;
> - int rc, lookup_flags = LOOKUP_NO_SYMLINKS;
> + int rc;
>
> ksmbd_debug(SMB, "setting FILE_RENAME_INFO\n");
> pathname = kmalloc(PATH_MAX, GFP_KERNEL);
> @@ -5362,11 +5332,8 @@ static int smb2_rename(struct ksmbd_work *work,
> goto out;
> }
>
> - if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
> - lookup_flags = LOOKUP_FOLLOW;
> -
> ksmbd_debug(SMB, "new name %s\n", new_name);
> - rc = ksmbd_vfs_kern_path(new_name, lookup_flags, &path, 1);
> + rc = ksmbd_vfs_kern_path(new_name, LOOKUP_NO_SYMLINKS, &path, 1);
> if (rc)
> file_present = false;
> else
> @@ -5416,7 +5383,7 @@ static int smb2_create_link(struct ksmbd_work *work,
> char *link_name = NULL, *target_name = NULL, *pathname = NULL;
> struct path path;
> bool file_present = true;
> - int rc, lookup_flags = LOOKUP_NO_SYMLINKS;
> + int rc;
>
> ksmbd_debug(SMB, "setting FILE_LINK_INFORMATION\n");
> pathname = kmalloc(PATH_MAX, GFP_KERNEL);
> @@ -5439,11 +5406,8 @@ static int smb2_create_link(struct ksmbd_work *work,
> goto out;
> }
>
> - if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
> - lookup_flags = LOOKUP_FOLLOW;
> -
> ksmbd_debug(SMB, "target name is %s\n", target_name);
> - rc = ksmbd_vfs_kern_path(link_name, lookup_flags, &path, 0);
> + rc = ksmbd_vfs_kern_path(link_name, LOOKUP_NO_SYMLINKS, &path, 0);
> if (rc)
> file_present = false;
> else
> diff --git a/fs/ksmbd/vfs.c b/fs/ksmbd/vfs.c
> index 53047f013371..3733e4944c1d 100644
> --- a/fs/ksmbd/vfs.c
> +++ b/fs/ksmbd/vfs.c
> @@ -164,13 +164,9 @@ int ksmbd_vfs_create(struct ksmbd_work *work, const char *name, umode_t mode)
> {
> struct path path;
> struct dentry *dentry;
> - int err, lookup_flags = LOOKUP_NO_SYMLINKS;
> -
> - if (test_share_config_flag(work->tcon->share_conf,
> - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
> - lookup_flags = LOOKUP_FOLLOW;
> + int err;
>
> - dentry = kern_path_create(AT_FDCWD, name, &path, lookup_flags);
> + dentry = kern_path_create(AT_FDCWD, name, &path, LOOKUP_NO_SYMLINKS);
> if (IS_ERR(dentry)) {
> err = PTR_ERR(dentry);
> if (err != -ENOENT)
> @@ -205,14 +201,10 @@ int ksmbd_vfs_mkdir(struct ksmbd_work *work, const char *name, umode_t mode)
> struct user_namespace *user_ns;
> struct path path;
> struct dentry *dentry;
> - int err, lookup_flags = LOOKUP_NO_SYMLINKS;
> -
> - if (test_share_config_flag(work->tcon->share_conf,
> - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
> - lookup_flags = LOOKUP_FOLLOW;
> + int err;
>
> dentry = kern_path_create(AT_FDCWD, name, &path,
> - lookup_flags | LOOKUP_DIRECTORY);
> + LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY);
> if (IS_ERR(dentry)) {
> err = PTR_ERR(dentry);
> if (err != -EEXIST)
> @@ -597,16 +589,11 @@ int ksmbd_vfs_remove_file(struct ksmbd_work *work, char *name)
> struct path path;
> struct dentry *parent;
> int err;
> - int flags = LOOKUP_NO_SYMLINKS;
>
> if (ksmbd_override_fsids(work))
> return -ENOMEM;
>
> - if (test_share_config_flag(work->tcon->share_conf,
> - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
> - flags = LOOKUP_FOLLOW;
> -
> - err = kern_path(name, flags, &path);
> + err = kern_path(name, LOOKUP_NO_SYMLINKS, &path);
> if (err) {
> ksmbd_debug(VFS, "can't get %s, err %d\n", name, err);
> ksmbd_revert_fsids(work);
> @@ -661,16 +648,11 @@ int ksmbd_vfs_link(struct ksmbd_work *work, const char *oldname,
> struct path oldpath, newpath;
> struct dentry *dentry;
> int err;
> - int flags = LOOKUP_NO_SYMLINKS;
>
> if (ksmbd_override_fsids(work))
> return -ENOMEM;
>
> - if (test_share_config_flag(work->tcon->share_conf,
> - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
> - flags = LOOKUP_FOLLOW;
> -
> - err = kern_path(oldname, flags, &oldpath);
> + err = kern_path(oldname, LOOKUP_NO_SYMLINKS, &oldpath);
> if (err) {
> pr_err("cannot get linux path for %s, err = %d\n",
> oldname, err);
> @@ -678,7 +660,7 @@ int ksmbd_vfs_link(struct ksmbd_work *work, const char *oldname,
> }
>
> dentry = kern_path_create(AT_FDCWD, newname, &newpath,
> - flags | LOOKUP_REVAL);
> + LOOKUP_NO_SYMLINKS | LOOKUP_REVAL);
> if (IS_ERR(dentry)) {
> err = PTR_ERR(dentry);
> pr_err("path create err for %s, err %d\n", newname, err);
> @@ -797,7 +779,6 @@ int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
> struct dentry *src_dent, *trap_dent, *src_child;
> char *dst_name;
> int err;
> - int flags = LOOKUP_NO_SYMLINKS;
>
> dst_name = extract_last_component(newname);
> if (!dst_name)
> @@ -806,13 +787,8 @@ int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
> src_dent_parent = dget_parent(fp->filp->f_path.dentry);
> src_dent = fp->filp->f_path.dentry;
>
> - flags = LOOKUP_DIRECTORY;
> - if (test_share_config_flag(work->tcon->share_conf,
> - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
> - flags = LOOKUP_FOLLOW;
> - flags |= LOOKUP_DIRECTORY;
> -
> - err = kern_path(newname, flags, &dst_path);
> + err = kern_path(newname, LOOKUP_NO_SYMLINKS | LOOKUP_DIRECTORY,
> + &dst_path);
> if (err) {
> ksmbd_debug(VFS, "Cannot get path for %s [%d]\n", newname, err);
> goto out;
> @@ -871,13 +847,7 @@ int ksmbd_vfs_truncate(struct ksmbd_work *work, const char *name,
> int err = 0;
>
> if (name) {
> - int flags = LOOKUP_NO_SYMLINKS;
> -
> - if (test_share_config_flag(work->tcon->share_conf,
> - KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
> - flags = LOOKUP_FOLLOW;
> -
> - err = kern_path(name, flags, &path);
> + err = kern_path(name, LOOKUP_NO_SYMLINKS, &path);
> if (err) {
> pr_err("cannot get linux path for %s, err %d\n",
> name, err);
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ksmbd: add request buffer validation in smb2_set_info
2021-09-21 4:40 ` [PATCH v2] ksmbd: add request buffer validation in smb2_set_info Namjae Jeon
@ 2021-09-21 7:38 ` Ralph Boehme
2021-09-21 11:06 ` Namjae Jeon
2021-09-21 19:36 ` Steve French
0 siblings, 2 replies; 6+ messages in thread
From: Ralph Boehme @ 2021-09-21 7:38 UTC (permalink / raw)
To: Namjae Jeon, linux-cifs; +Cc: Ronnie Sahlberg, Steve French
[-- Attachment #1.1: Type: text/plain, Size: 735 bytes --]
Hi Namjae,
Am 21.09.21 um 06:40 schrieb Namjae Jeon:
> Add buffer validation in smb2_set_info.
>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
> v2:
> - smb2_set_info infolevel functions take structure pointer argument.
>
perfect, thanks! One nitpick: we should either split out the
smb2_file_basic_info fix into a preceeding commit or at least mention it
in the commit message.
With that change: reviewed-by: me.
Thanks!
-slow
--
Ralph Boehme, Samba Team https://samba.org/
SerNet Samba Team Lead https://sernet.de/en/team-samba
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ksmbd: add request buffer validation in smb2_set_info
2021-09-21 7:38 ` Ralph Boehme
@ 2021-09-21 11:06 ` Namjae Jeon
2021-09-21 19:36 ` Steve French
1 sibling, 0 replies; 6+ messages in thread
From: Namjae Jeon @ 2021-09-21 11:06 UTC (permalink / raw)
To: Ralph Boehme; +Cc: linux-cifs, Ronnie Sahlberg, Steve French
2021-09-21 16:38 GMT+09:00, Ralph Boehme <slow@samba.org>:
> Hi Namjae,
>
>
> Am 21.09.21 um 06:40 schrieb Namjae Jeon:
>> Add buffer validation in smb2_set_info.
>>
>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> Cc: Ralph Böhme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>> v2:
>> - smb2_set_info infolevel functions take structure pointer argument.
>>
>
> perfect, thanks! One nitpick: we should either split out the
> smb2_file_basic_info fix into a preceeding commit or at least mention it
> in the commit message.
Will update patch header description.
>
> With that change: reviewed-by: me.
Thanks for your review!
>
> Thanks!
> -slow
>
> --
> Ralph Boehme, Samba Team https://samba.org/
> SerNet Samba Team Lead https://sernet.de/en/team-samba
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ksmbd: add request buffer validation in smb2_set_info
2021-09-21 7:38 ` Ralph Boehme
2021-09-21 11:06 ` Namjae Jeon
@ 2021-09-21 19:36 ` Steve French
1 sibling, 0 replies; 6+ messages in thread
From: Steve French @ 2021-09-21 19:36 UTC (permalink / raw)
To: Ralph Boehme; +Cc: Namjae Jeon, CIFS, Ronnie Sahlberg
updated comment in header and remerged into cifsd-for-next
On Tue, Sep 21, 2021 at 2:38 AM Ralph Boehme <slow@samba.org> wrote:
>
> Hi Namjae,
>
>
> Am 21.09.21 um 06:40 schrieb Namjae Jeon:
> > Add buffer validation in smb2_set_info.
> >
> > Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> > Cc: Ralph Böhme <slow@samba.org>
> > Cc: Steve French <smfrench@gmail.com>
> > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> > ---
> > v2:
> > - smb2_set_info infolevel functions take structure pointer argument.
> >
>
> perfect, thanks! One nitpick: we should either split out the
> smb2_file_basic_info fix into a preceeding commit or at least mention it
> in the commit message.
>
> With that change: reviewed-by: me.
>
> Thanks!
> -slow
>
> --
> Ralph Boehme, Samba Team https://samba.org/
> SerNet Samba Team Lead https://sernet.de/en/team-samba
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-21 19:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-21 4:40 [PATCH v2] ksmbd: remove follow symlinks support Namjae Jeon
2021-09-21 4:40 ` [PATCH v2] ksmbd: add request buffer validation in smb2_set_info Namjae Jeon
2021-09-21 7:38 ` Ralph Boehme
2021-09-21 11:06 ` Namjae Jeon
2021-09-21 19:36 ` Steve French
2021-09-21 4:49 ` [PATCH v2] ksmbd: remove follow symlinks support Leif Sahlberg
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).