* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.