* [PATCH 1/7] ksmbd: add validation in smb2_ioctl
2021-09-24 2:12 [PATCH 0/7] a bunch of patches that have not yet been reviewed Namjae Jeon
@ 2021-09-24 2:12 ` Namjae Jeon
2021-09-25 10:16 ` Hyunchul Lee
2021-09-24 2:12 ` [PATCH 2/7] ksmbd: add request buffer validation in smb2_set_info Namjae Jeon
` (5 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Namjae Jeon @ 2021-09-24 2:12 UTC (permalink / raw)
To: linux-cifs
Cc: Namjae Jeon, Tom Talpey, Ronnie Sahlberg, Ralph Böhme,
Steve French, Hyunchul Lee, Sergey Senozhatsky
Add validation for request/response buffer size check in smb2_ioctl and
fsctl_copychunk() take copychunk_ioctl_req pointer and the other arguments
instead of smb2_ioctl_req structure and remove an unused smb2_ioctl_req
argument of fsctl_validate_negotiate_info.
Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
fs/ksmbd/smb2pdu.c | 87 ++++++++++++++++++++++++++++++++++++----------
1 file changed, 68 insertions(+), 19 deletions(-)
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 0c49a0e887d3..b22d4207c077 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -6927,24 +6927,26 @@ int smb2_lock(struct ksmbd_work *work)
return err;
}
-static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req,
+static int fsctl_copychunk(struct ksmbd_work *work,
+ struct copychunk_ioctl_req *ci_req,
+ unsigned int cnt_code,
+ unsigned int input_count,
+ unsigned long long volatile_id,
+ unsigned long long persistent_id,
struct smb2_ioctl_rsp *rsp)
{
- struct copychunk_ioctl_req *ci_req;
struct copychunk_ioctl_rsp *ci_rsp;
struct ksmbd_file *src_fp = NULL, *dst_fp = NULL;
struct srv_copychunk *chunks;
unsigned int i, chunk_count, chunk_count_written = 0;
unsigned int chunk_size_written = 0;
loff_t total_size_written = 0;
- int ret, cnt_code;
+ int ret = 0;
- cnt_code = le32_to_cpu(req->CntCode);
- ci_req = (struct copychunk_ioctl_req *)&req->Buffer[0];
ci_rsp = (struct copychunk_ioctl_rsp *)&rsp->Buffer[0];
- rsp->VolatileFileId = req->VolatileFileId;
- rsp->PersistentFileId = req->PersistentFileId;
+ rsp->VolatileFileId = cpu_to_le64(volatile_id);
+ rsp->PersistentFileId = cpu_to_le64(persistent_id);
ci_rsp->ChunksWritten =
cpu_to_le32(ksmbd_server_side_copy_max_chunk_count());
ci_rsp->ChunkBytesWritten =
@@ -6954,12 +6956,13 @@ static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req,
chunks = (struct srv_copychunk *)&ci_req->Chunks[0];
chunk_count = le32_to_cpu(ci_req->ChunkCount);
+ if (chunk_count == 0)
+ goto out;
total_size_written = 0;
/* verify the SRV_COPYCHUNK_COPY packet */
if (chunk_count > ksmbd_server_side_copy_max_chunk_count() ||
- le32_to_cpu(req->InputCount) <
- offsetof(struct copychunk_ioctl_req, Chunks) +
+ input_count < offsetof(struct copychunk_ioctl_req, Chunks) +
chunk_count * sizeof(struct srv_copychunk)) {
rsp->hdr.Status = STATUS_INVALID_PARAMETER;
return -EINVAL;
@@ -6980,9 +6983,7 @@ static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req,
src_fp = ksmbd_lookup_foreign_fd(work,
le64_to_cpu(ci_req->ResumeKey[0]));
- dst_fp = ksmbd_lookup_fd_slow(work,
- le64_to_cpu(req->VolatileFileId),
- le64_to_cpu(req->PersistentFileId));
+ dst_fp = ksmbd_lookup_fd_slow(work, volatile_id, persistent_id);
ret = -EINVAL;
if (!src_fp ||
src_fp->persistent_id != le64_to_cpu(ci_req->ResumeKey[1])) {
@@ -7057,8 +7058,8 @@ static __be32 idev_ipv4_address(struct in_device *idev)
}
static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn,
- struct smb2_ioctl_req *req,
- struct smb2_ioctl_rsp *rsp)
+ struct smb2_ioctl_rsp *rsp,
+ int out_buf_len)
{
struct network_interface_info_ioctl_rsp *nii_rsp = NULL;
int nbytes = 0;
@@ -7141,6 +7142,8 @@ static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn,
sockaddr_storage->addr6.ScopeId = 0;
}
+ if (out_buf_len < sizeof(struct network_interface_info_ioctl_rsp))
+ break;
nbytes += sizeof(struct network_interface_info_ioctl_rsp);
}
rtnl_unlock();
@@ -7161,11 +7164,16 @@ static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn,
static int fsctl_validate_negotiate_info(struct ksmbd_conn *conn,
struct validate_negotiate_info_req *neg_req,
- struct validate_negotiate_info_rsp *neg_rsp)
+ struct validate_negotiate_info_rsp *neg_rsp,
+ int in_buf_len)
{
int ret = 0;
int dialect;
+ if (in_buf_len < sizeof(struct validate_negotiate_info_req) +
+ le16_to_cpu(neg_req->DialectCount) * sizeof(__le16))
+ return -EINVAL;
+
dialect = ksmbd_lookup_dialect_by_id(neg_req->Dialects,
neg_req->DialectCount);
if (dialect == BAD_PROT_ID || dialect != conn->dialect) {
@@ -7341,7 +7349,7 @@ int smb2_ioctl(struct ksmbd_work *work)
struct smb2_ioctl_req *req;
struct smb2_ioctl_rsp *rsp, *rsp_org;
int cnt_code, nbytes = 0;
- int out_buf_len;
+ int out_buf_len, in_buf_len;
u64 id = KSMBD_NO_FID;
struct ksmbd_conn *conn = work->conn;
int ret = 0;
@@ -7371,6 +7379,7 @@ int smb2_ioctl(struct ksmbd_work *work)
cnt_code = le32_to_cpu(req->CntCode);
out_buf_len = le32_to_cpu(req->MaxOutputResponse);
out_buf_len = min(KSMBD_IPC_MAX_PAYLOAD, out_buf_len);
+ in_buf_len = le32_to_cpu(req->InputCount);
switch (cnt_code) {
case FSCTL_DFS_GET_REFERRALS:
@@ -7406,9 +7415,16 @@ int smb2_ioctl(struct ksmbd_work *work)
goto out;
}
+ if (in_buf_len < sizeof(struct validate_negotiate_info_req))
+ return -EINVAL;
+
+ if (out_buf_len < sizeof(struct validate_negotiate_info_rsp))
+ return -EINVAL;
+
ret = fsctl_validate_negotiate_info(conn,
(struct validate_negotiate_info_req *)&req->Buffer[0],
- (struct validate_negotiate_info_rsp *)&rsp->Buffer[0]);
+ (struct validate_negotiate_info_rsp *)&rsp->Buffer[0],
+ in_buf_len);
if (ret < 0)
goto out;
@@ -7417,7 +7433,7 @@ int smb2_ioctl(struct ksmbd_work *work)
rsp->VolatileFileId = cpu_to_le64(SMB2_NO_FID);
break;
case FSCTL_QUERY_NETWORK_INTERFACE_INFO:
- nbytes = fsctl_query_iface_info_ioctl(conn, req, rsp);
+ nbytes = fsctl_query_iface_info_ioctl(conn, rsp, out_buf_len);
if (nbytes < 0)
goto out;
break;
@@ -7444,15 +7460,33 @@ int smb2_ioctl(struct ksmbd_work *work)
goto out;
}
+ if (in_buf_len < sizeof(struct copychunk_ioctl_req)) {
+ ret = -EINVAL;
+ goto out;
+ }
+
if (out_buf_len < sizeof(struct copychunk_ioctl_rsp)) {
ret = -EINVAL;
goto out;
}
nbytes = sizeof(struct copychunk_ioctl_rsp);
- fsctl_copychunk(work, req, rsp);
+ rsp->VolatileFileId = req->VolatileFileId;
+ rsp->PersistentFileId = req->PersistentFileId;
+ fsctl_copychunk(work,
+ (struct copychunk_ioctl_req *)&req->Buffer[0],
+ le32_to_cpu(req->CntCode),
+ le32_to_cpu(req->InputCount),
+ le64_to_cpu(req->VolatileFileId),
+ le64_to_cpu(req->PersistentFileId),
+ rsp);
break;
case FSCTL_SET_SPARSE:
+ if (in_buf_len < sizeof(struct file_sparse)) {
+ ret = -EINVAL;
+ goto out;
+ }
+
ret = fsctl_set_sparse(work, id,
(struct file_sparse *)&req->Buffer[0]);
if (ret < 0)
@@ -7471,6 +7505,11 @@ int smb2_ioctl(struct ksmbd_work *work)
goto out;
}
+ if (in_buf_len < sizeof(struct file_zero_data_information)) {
+ ret = -EINVAL;
+ goto out;
+ }
+
zero_data =
(struct file_zero_data_information *)&req->Buffer[0];
@@ -7490,6 +7529,11 @@ int smb2_ioctl(struct ksmbd_work *work)
break;
}
case FSCTL_QUERY_ALLOCATED_RANGES:
+ if (in_buf_len < sizeof(struct file_allocated_range_buffer)) {
+ ret = -EINVAL;
+ goto out;
+ }
+
ret = fsctl_query_allocated_ranges(work, id,
(struct file_allocated_range_buffer *)&req->Buffer[0],
(struct file_allocated_range_buffer *)&rsp->Buffer[0],
@@ -7530,6 +7574,11 @@ int smb2_ioctl(struct ksmbd_work *work)
struct duplicate_extents_to_file *dup_ext;
loff_t src_off, dst_off, length, cloned;
+ if (in_buf_len < sizeof(struct duplicate_extents_to_file)) {
+ ret = -EINVAL;
+ goto out;
+ }
+
dup_ext = (struct duplicate_extents_to_file *)&req->Buffer[0];
fp_in = ksmbd_lookup_fd_slow(work, dup_ext->VolatileFileHandle,
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 1/7] ksmbd: add validation in smb2_ioctl
2021-09-24 2:12 ` [PATCH 1/7] ksmbd: add validation in smb2_ioctl Namjae Jeon
@ 2021-09-25 10:16 ` Hyunchul Lee
2021-09-25 10:44 ` Namjae Jeon
0 siblings, 1 reply; 19+ messages in thread
From: Hyunchul Lee @ 2021-09-25 10:16 UTC (permalink / raw)
To: Namjae Jeon
Cc: linux-cifs, Tom Talpey, Ronnie Sahlberg, Ralph Böhme,
Steve French, Sergey Senozhatsky
2021년 9월 24일 (금) 오전 11:13, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>
> Add validation for request/response buffer size check in smb2_ioctl and
> fsctl_copychunk() take copychunk_ioctl_req pointer and the other arguments
> instead of smb2_ioctl_req structure and remove an unused smb2_ioctl_req
> argument of fsctl_validate_negotiate_info.
>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Cc: Hyunchul Lee <hyc.lee@gmail.com>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
> fs/ksmbd/smb2pdu.c | 87 ++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 68 insertions(+), 19 deletions(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 0c49a0e887d3..b22d4207c077 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -6927,24 +6927,26 @@ int smb2_lock(struct ksmbd_work *work)
> return err;
> }
>
> -static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req,
> +static int fsctl_copychunk(struct ksmbd_work *work,
> + struct copychunk_ioctl_req *ci_req,
> + unsigned int cnt_code,
> + unsigned int input_count,
> + unsigned long long volatile_id,
> + unsigned long long persistent_id,
> struct smb2_ioctl_rsp *rsp)
> {
> - struct copychunk_ioctl_req *ci_req;
> struct copychunk_ioctl_rsp *ci_rsp;
> struct ksmbd_file *src_fp = NULL, *dst_fp = NULL;
> struct srv_copychunk *chunks;
> unsigned int i, chunk_count, chunk_count_written = 0;
> unsigned int chunk_size_written = 0;
> loff_t total_size_written = 0;
> - int ret, cnt_code;
> + int ret = 0;
>
> - cnt_code = le32_to_cpu(req->CntCode);
> - ci_req = (struct copychunk_ioctl_req *)&req->Buffer[0];
> ci_rsp = (struct copychunk_ioctl_rsp *)&rsp->Buffer[0];
>
> - rsp->VolatileFileId = req->VolatileFileId;
> - rsp->PersistentFileId = req->PersistentFileId;
> + rsp->VolatileFileId = cpu_to_le64(volatile_id);
> + rsp->PersistentFileId = cpu_to_le64(persistent_id);
> ci_rsp->ChunksWritten =
> cpu_to_le32(ksmbd_server_side_copy_max_chunk_count());
> ci_rsp->ChunkBytesWritten =
> @@ -6954,12 +6956,13 @@ static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req,
>
> chunks = (struct srv_copychunk *)&ci_req->Chunks[0];
> chunk_count = le32_to_cpu(ci_req->ChunkCount);
> + if (chunk_count == 0)
> + goto out;
> total_size_written = 0;
>
> /* verify the SRV_COPYCHUNK_COPY packet */
> if (chunk_count > ksmbd_server_side_copy_max_chunk_count() ||
> - le32_to_cpu(req->InputCount) <
> - offsetof(struct copychunk_ioctl_req, Chunks) +
> + input_count < offsetof(struct copychunk_ioctl_req, Chunks) +
> chunk_count * sizeof(struct srv_copychunk)) {
> rsp->hdr.Status = STATUS_INVALID_PARAMETER;
> return -EINVAL;
> @@ -6980,9 +6983,7 @@ static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req *req,
>
> src_fp = ksmbd_lookup_foreign_fd(work,
> le64_to_cpu(ci_req->ResumeKey[0]));
> - dst_fp = ksmbd_lookup_fd_slow(work,
> - le64_to_cpu(req->VolatileFileId),
> - le64_to_cpu(req->PersistentFileId));
> + dst_fp = ksmbd_lookup_fd_slow(work, volatile_id, persistent_id);
> ret = -EINVAL;
> if (!src_fp ||
> src_fp->persistent_id != le64_to_cpu(ci_req->ResumeKey[1])) {
> @@ -7057,8 +7058,8 @@ static __be32 idev_ipv4_address(struct in_device *idev)
> }
>
> static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn,
> - struct smb2_ioctl_req *req,
> - struct smb2_ioctl_rsp *rsp)
> + struct smb2_ioctl_rsp *rsp,
> + int out_buf_len)
> {
> struct network_interface_info_ioctl_rsp *nii_rsp = NULL;
> int nbytes = 0;
> @@ -7141,6 +7142,8 @@ static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn,
> sockaddr_storage->addr6.ScopeId = 0;
> }
>
> + if (out_buf_len < sizeof(struct network_interface_info_ioctl_rsp))
> + break;
Do we need to check whether the remaining(len out_buf_len - nbytes) is
less than sizeof(struct network_interface_info_ioctl_rsp)?
> nbytes += sizeof(struct network_interface_info_ioctl_rsp);
> }
> rtnl_unlock();
> @@ -7161,11 +7164,16 @@ static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn,
>
> static int fsctl_validate_negotiate_info(struct ksmbd_conn *conn,
> struct validate_negotiate_info_req *neg_req,
> - struct validate_negotiate_info_rsp *neg_rsp)
> + struct validate_negotiate_info_rsp *neg_rsp,
> + int in_buf_len)
> {
> int ret = 0;
> int dialect;
>
> + if (in_buf_len < sizeof(struct validate_negotiate_info_req) +
> + le16_to_cpu(neg_req->DialectCount) * sizeof(__le16))
> + return -EINVAL;
> +
> dialect = ksmbd_lookup_dialect_by_id(neg_req->Dialects,
> neg_req->DialectCount);
> if (dialect == BAD_PROT_ID || dialect != conn->dialect) {
> @@ -7341,7 +7349,7 @@ int smb2_ioctl(struct ksmbd_work *work)
> struct smb2_ioctl_req *req;
> struct smb2_ioctl_rsp *rsp, *rsp_org;
> int cnt_code, nbytes = 0;
> - int out_buf_len;
> + int out_buf_len, in_buf_len;
> u64 id = KSMBD_NO_FID;
> struct ksmbd_conn *conn = work->conn;
> int ret = 0;
> @@ -7371,6 +7379,7 @@ int smb2_ioctl(struct ksmbd_work *work)
> cnt_code = le32_to_cpu(req->CntCode);
> out_buf_len = le32_to_cpu(req->MaxOutputResponse);
> out_buf_len = min(KSMBD_IPC_MAX_PAYLOAD, out_buf_len);
> + in_buf_len = le32_to_cpu(req->InputCount);
>
> switch (cnt_code) {
> case FSCTL_DFS_GET_REFERRALS:
> @@ -7406,9 +7415,16 @@ int smb2_ioctl(struct ksmbd_work *work)
> goto out;
> }
>
> + if (in_buf_len < sizeof(struct validate_negotiate_info_req))
> + return -EINVAL;
> +
> + if (out_buf_len < sizeof(struct validate_negotiate_info_rsp))
> + return -EINVAL;
> +
> ret = fsctl_validate_negotiate_info(conn,
> (struct validate_negotiate_info_req *)&req->Buffer[0],
> - (struct validate_negotiate_info_rsp *)&rsp->Buffer[0]);
> + (struct validate_negotiate_info_rsp *)&rsp->Buffer[0],
> + in_buf_len);
> if (ret < 0)
> goto out;
>
> @@ -7417,7 +7433,7 @@ int smb2_ioctl(struct ksmbd_work *work)
> rsp->VolatileFileId = cpu_to_le64(SMB2_NO_FID);
> break;
> case FSCTL_QUERY_NETWORK_INTERFACE_INFO:
> - nbytes = fsctl_query_iface_info_ioctl(conn, req, rsp);
> + nbytes = fsctl_query_iface_info_ioctl(conn, rsp, out_buf_len);
> if (nbytes < 0)
> goto out;
> break;
> @@ -7444,15 +7460,33 @@ int smb2_ioctl(struct ksmbd_work *work)
> goto out;
> }
>
> + if (in_buf_len < sizeof(struct copychunk_ioctl_req)) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> if (out_buf_len < sizeof(struct copychunk_ioctl_rsp)) {
> ret = -EINVAL;
> goto out;
> }
>
> nbytes = sizeof(struct copychunk_ioctl_rsp);
> - fsctl_copychunk(work, req, rsp);
> + rsp->VolatileFileId = req->VolatileFileId;
> + rsp->PersistentFileId = req->PersistentFileId;
> + fsctl_copychunk(work,
> + (struct copychunk_ioctl_req *)&req->Buffer[0],
> + le32_to_cpu(req->CntCode),
> + le32_to_cpu(req->InputCount),
> + le64_to_cpu(req->VolatileFileId),
> + le64_to_cpu(req->PersistentFileId),
> + rsp);
> break;
> case FSCTL_SET_SPARSE:
> + if (in_buf_len < sizeof(struct file_sparse)) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> ret = fsctl_set_sparse(work, id,
> (struct file_sparse *)&req->Buffer[0]);
> if (ret < 0)
> @@ -7471,6 +7505,11 @@ int smb2_ioctl(struct ksmbd_work *work)
> goto out;
> }
>
> + if (in_buf_len < sizeof(struct file_zero_data_information)) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> zero_data =
> (struct file_zero_data_information *)&req->Buffer[0];
>
> @@ -7490,6 +7529,11 @@ int smb2_ioctl(struct ksmbd_work *work)
> break;
> }
> case FSCTL_QUERY_ALLOCATED_RANGES:
> + if (in_buf_len < sizeof(struct file_allocated_range_buffer)) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> ret = fsctl_query_allocated_ranges(work, id,
> (struct file_allocated_range_buffer *)&req->Buffer[0],
> (struct file_allocated_range_buffer *)&rsp->Buffer[0],
> @@ -7530,6 +7574,11 @@ int smb2_ioctl(struct ksmbd_work *work)
> struct duplicate_extents_to_file *dup_ext;
> loff_t src_off, dst_off, length, cloned;
>
> + if (in_buf_len < sizeof(struct duplicate_extents_to_file)) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> dup_ext = (struct duplicate_extents_to_file *)&req->Buffer[0];
>
> fp_in = ksmbd_lookup_fd_slow(work, dup_ext->VolatileFileHandle,
> --
> 2.25.1
>
--
Thanks,
Hyunchul
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 1/7] ksmbd: add validation in smb2_ioctl
2021-09-25 10:16 ` Hyunchul Lee
@ 2021-09-25 10:44 ` Namjae Jeon
0 siblings, 0 replies; 19+ messages in thread
From: Namjae Jeon @ 2021-09-25 10:44 UTC (permalink / raw)
To: Hyunchul Lee
Cc: linux-cifs, Tom Talpey, Ronnie Sahlberg, Ralph Böhme,
Steve French, Sergey Senozhatsky
2021-09-25 19:16 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> 2021년 9월 24일 (금) 오전 11:13, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>>
>> Add validation for request/response buffer size check in smb2_ioctl and
>> fsctl_copychunk() take copychunk_ioctl_req pointer and the other
>> arguments
>> instead of smb2_ioctl_req structure and remove an unused smb2_ioctl_req
>> argument of fsctl_validate_negotiate_info.
>>
>> Cc: Tom Talpey <tom@talpey.com>
>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> Cc: Ralph Böhme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Cc: Hyunchul Lee <hyc.lee@gmail.com>
>> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>> fs/ksmbd/smb2pdu.c | 87 ++++++++++++++++++++++++++++++++++++----------
>> 1 file changed, 68 insertions(+), 19 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index 0c49a0e887d3..b22d4207c077 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -6927,24 +6927,26 @@ int smb2_lock(struct ksmbd_work *work)
>> return err;
>> }
>>
>> -static int fsctl_copychunk(struct ksmbd_work *work, struct smb2_ioctl_req
>> *req,
>> +static int fsctl_copychunk(struct ksmbd_work *work,
>> + struct copychunk_ioctl_req *ci_req,
>> + unsigned int cnt_code,
>> + unsigned int input_count,
>> + unsigned long long volatile_id,
>> + unsigned long long persistent_id,
>> struct smb2_ioctl_rsp *rsp)
>> {
>> - struct copychunk_ioctl_req *ci_req;
>> struct copychunk_ioctl_rsp *ci_rsp;
>> struct ksmbd_file *src_fp = NULL, *dst_fp = NULL;
>> struct srv_copychunk *chunks;
>> unsigned int i, chunk_count, chunk_count_written = 0;
>> unsigned int chunk_size_written = 0;
>> loff_t total_size_written = 0;
>> - int ret, cnt_code;
>> + int ret = 0;
>>
>> - cnt_code = le32_to_cpu(req->CntCode);
>> - ci_req = (struct copychunk_ioctl_req *)&req->Buffer[0];
>> ci_rsp = (struct copychunk_ioctl_rsp *)&rsp->Buffer[0];
>>
>> - rsp->VolatileFileId = req->VolatileFileId;
>> - rsp->PersistentFileId = req->PersistentFileId;
>> + rsp->VolatileFileId = cpu_to_le64(volatile_id);
>> + rsp->PersistentFileId = cpu_to_le64(persistent_id);
>> ci_rsp->ChunksWritten =
>> cpu_to_le32(ksmbd_server_side_copy_max_chunk_count());
>> ci_rsp->ChunkBytesWritten =
>> @@ -6954,12 +6956,13 @@ static int fsctl_copychunk(struct ksmbd_work
>> *work, struct smb2_ioctl_req *req,
>>
>> chunks = (struct srv_copychunk *)&ci_req->Chunks[0];
>> chunk_count = le32_to_cpu(ci_req->ChunkCount);
>> + if (chunk_count == 0)
>> + goto out;
>> total_size_written = 0;
>>
>> /* verify the SRV_COPYCHUNK_COPY packet */
>> if (chunk_count > ksmbd_server_side_copy_max_chunk_count() ||
>> - le32_to_cpu(req->InputCount) <
>> - offsetof(struct copychunk_ioctl_req, Chunks) +
>> + input_count < offsetof(struct copychunk_ioctl_req, Chunks) +
>> chunk_count * sizeof(struct srv_copychunk)) {
>> rsp->hdr.Status = STATUS_INVALID_PARAMETER;
>> return -EINVAL;
>> @@ -6980,9 +6983,7 @@ static int fsctl_copychunk(struct ksmbd_work *work,
>> struct smb2_ioctl_req *req,
>>
>> src_fp = ksmbd_lookup_foreign_fd(work,
>>
>> le64_to_cpu(ci_req->ResumeKey[0]));
>> - dst_fp = ksmbd_lookup_fd_slow(work,
>> - le64_to_cpu(req->VolatileFileId),
>> -
>> le64_to_cpu(req->PersistentFileId));
>> + dst_fp = ksmbd_lookup_fd_slow(work, volatile_id, persistent_id);
>> ret = -EINVAL;
>> if (!src_fp ||
>> src_fp->persistent_id != le64_to_cpu(ci_req->ResumeKey[1])) {
>> @@ -7057,8 +7058,8 @@ static __be32 idev_ipv4_address(struct in_device
>> *idev)
>> }
>>
>> static int fsctl_query_iface_info_ioctl(struct ksmbd_conn *conn,
>> - struct smb2_ioctl_req *req,
>> - struct smb2_ioctl_rsp *rsp)
>> + struct smb2_ioctl_rsp *rsp,
>> + int out_buf_len)
>> {
>> struct network_interface_info_ioctl_rsp *nii_rsp = NULL;
>> int nbytes = 0;
>> @@ -7141,6 +7142,8 @@ static int fsctl_query_iface_info_ioctl(struct
>> ksmbd_conn *conn,
>> sockaddr_storage->addr6.ScopeId = 0;
>> }
>>
>> + if (out_buf_len < sizeof(struct
>> network_interface_info_ioctl_rsp))
>> + break;
>
> Do we need to check whether the remaining(len out_buf_len - nbytes) is
> less than sizeof(struct network_interface_info_ioctl_rsp)?
Right. will fix it on next version. Thanks for your review!
>
>> nbytes += sizeof(struct
>> network_interface_info_ioctl_rsp);
>> }
>> rtnl_unlock();
>> @@ -7161,11 +7164,16 @@ static int fsctl_query_iface_info_ioctl(struct
>> ksmbd_conn *conn,
>>
>> static int fsctl_validate_negotiate_info(struct ksmbd_conn *conn,
>> struct
>> validate_negotiate_info_req *neg_req,
>> - struct
>> validate_negotiate_info_rsp *neg_rsp)
>> + struct
>> validate_negotiate_info_rsp *neg_rsp,
>> + int in_buf_len)
>> {
>> int ret = 0;
>> int dialect;
>>
>> + if (in_buf_len < sizeof(struct validate_negotiate_info_req) +
>> + le16_to_cpu(neg_req->DialectCount) *
>> sizeof(__le16))
>> + return -EINVAL;
>> +
>> dialect = ksmbd_lookup_dialect_by_id(neg_req->Dialects,
>> neg_req->DialectCount);
>> if (dialect == BAD_PROT_ID || dialect != conn->dialect) {
>> @@ -7341,7 +7349,7 @@ int smb2_ioctl(struct ksmbd_work *work)
>> struct smb2_ioctl_req *req;
>> struct smb2_ioctl_rsp *rsp, *rsp_org;
>> int cnt_code, nbytes = 0;
>> - int out_buf_len;
>> + int out_buf_len, in_buf_len;
>> u64 id = KSMBD_NO_FID;
>> struct ksmbd_conn *conn = work->conn;
>> int ret = 0;
>> @@ -7371,6 +7379,7 @@ int smb2_ioctl(struct ksmbd_work *work)
>> cnt_code = le32_to_cpu(req->CntCode);
>> out_buf_len = le32_to_cpu(req->MaxOutputResponse);
>> out_buf_len = min(KSMBD_IPC_MAX_PAYLOAD, out_buf_len);
>> + in_buf_len = le32_to_cpu(req->InputCount);
>>
>> switch (cnt_code) {
>> case FSCTL_DFS_GET_REFERRALS:
>> @@ -7406,9 +7415,16 @@ int smb2_ioctl(struct ksmbd_work *work)
>> goto out;
>> }
>>
>> + if (in_buf_len < sizeof(struct
>> validate_negotiate_info_req))
>> + return -EINVAL;
>> +
>> + if (out_buf_len < sizeof(struct
>> validate_negotiate_info_rsp))
>> + return -EINVAL;
>> +
>> ret = fsctl_validate_negotiate_info(conn,
>> (struct validate_negotiate_info_req
>> *)&req->Buffer[0],
>> - (struct validate_negotiate_info_rsp
>> *)&rsp->Buffer[0]);
>> + (struct validate_negotiate_info_rsp
>> *)&rsp->Buffer[0],
>> + in_buf_len);
>> if (ret < 0)
>> goto out;
>>
>> @@ -7417,7 +7433,7 @@ int smb2_ioctl(struct ksmbd_work *work)
>> rsp->VolatileFileId = cpu_to_le64(SMB2_NO_FID);
>> break;
>> case FSCTL_QUERY_NETWORK_INTERFACE_INFO:
>> - nbytes = fsctl_query_iface_info_ioctl(conn, req, rsp);
>> + nbytes = fsctl_query_iface_info_ioctl(conn, rsp,
>> out_buf_len);
>> if (nbytes < 0)
>> goto out;
>> break;
>> @@ -7444,15 +7460,33 @@ int smb2_ioctl(struct ksmbd_work *work)
>> goto out;
>> }
>>
>> + if (in_buf_len < sizeof(struct copychunk_ioctl_req)) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> if (out_buf_len < sizeof(struct copychunk_ioctl_rsp)) {
>> ret = -EINVAL;
>> goto out;
>> }
>>
>> nbytes = sizeof(struct copychunk_ioctl_rsp);
>> - fsctl_copychunk(work, req, rsp);
>> + rsp->VolatileFileId = req->VolatileFileId;
>> + rsp->PersistentFileId = req->PersistentFileId;
>> + fsctl_copychunk(work,
>> + (struct copychunk_ioctl_req
>> *)&req->Buffer[0],
>> + le32_to_cpu(req->CntCode),
>> + le32_to_cpu(req->InputCount),
>> + le64_to_cpu(req->VolatileFileId),
>> + le64_to_cpu(req->PersistentFileId),
>> + rsp);
>> break;
>> case FSCTL_SET_SPARSE:
>> + if (in_buf_len < sizeof(struct file_sparse)) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> ret = fsctl_set_sparse(work, id,
>> (struct file_sparse
>> *)&req->Buffer[0]);
>> if (ret < 0)
>> @@ -7471,6 +7505,11 @@ int smb2_ioctl(struct ksmbd_work *work)
>> goto out;
>> }
>>
>> + if (in_buf_len < sizeof(struct
>> file_zero_data_information)) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> zero_data =
>> (struct file_zero_data_information
>> *)&req->Buffer[0];
>>
>> @@ -7490,6 +7529,11 @@ int smb2_ioctl(struct ksmbd_work *work)
>> break;
>> }
>> case FSCTL_QUERY_ALLOCATED_RANGES:
>> + if (in_buf_len < sizeof(struct
>> file_allocated_range_buffer)) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> ret = fsctl_query_allocated_ranges(work, id,
>> (struct file_allocated_range_buffer
>> *)&req->Buffer[0],
>> (struct file_allocated_range_buffer
>> *)&rsp->Buffer[0],
>> @@ -7530,6 +7574,11 @@ int smb2_ioctl(struct ksmbd_work *work)
>> struct duplicate_extents_to_file *dup_ext;
>> loff_t src_off, dst_off, length, cloned;
>>
>> + if (in_buf_len < sizeof(struct duplicate_extents_to_file))
>> {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> dup_ext = (struct duplicate_extents_to_file
>> *)&req->Buffer[0];
>>
>> fp_in = ksmbd_lookup_fd_slow(work,
>> dup_ext->VolatileFileHandle,
>> --
>> 2.25.1
>>
>
>
> --
> Thanks,
> Hyunchul
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/7] ksmbd: add request buffer validation in smb2_set_info
2021-09-24 2:12 [PATCH 0/7] a bunch of patches that have not yet been reviewed Namjae Jeon
2021-09-24 2:12 ` [PATCH 1/7] ksmbd: add validation in smb2_ioctl Namjae Jeon
@ 2021-09-24 2:12 ` Namjae Jeon
2021-09-25 8:13 ` Hyunchul Lee
2021-09-24 2:12 ` [PATCH 3/7] ksmbd: remove RFC1002 check in smb2 request Namjae Jeon
` (4 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Namjae Jeon @ 2021-09-24 2:12 UTC (permalink / raw)
To: linux-cifs
Cc: Namjae Jeon, Tom Talpey, Ronnie Sahlberg, Ralph Böhme,
Steve French, Hyunchul Lee, Sergey Senozhatsky
Add buffer validation in smb2_set_info, and remove unused variable
in set_file_basic_info. and smb2_set_info infolevel functions take
structure pointer argument.
Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
fs/ksmbd/smb2pdu.c | 149 ++++++++++++++++++++++++++++++++-------------
fs/ksmbd/smb2pdu.h | 9 +++
2 files changed, 116 insertions(+), 42 deletions(-)
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index b22d4207c077..a930838fd6ac 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -2107,16 +2107,22 @@ 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;
+ unsigned 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)
@@ -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);
@@ -2771,7 +2783,15 @@ 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);
+ if (le32_to_cpu(ea_buf->ccontext.DataLength) <
+ sizeof(struct smb2_ea_info)) {
+ rc = -EINVAL;
+ goto err_out;
+ }
+
+ rc = smb2_set_ea(&ea_buf->ea,
+ le32_to_cpu(ea_buf->ccontext.DataLength),
+ &path);
if (rc == -EOPNOTSUPP)
rc = 0;
else if (rc)
@@ -5354,7 +5374,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,
+ unsigned int buf_len, struct file *filp,
struct nls_table *local_nls)
{
char *link_name = NULL, *target_name = NULL, *pathname = NULL;
@@ -5362,6 +5382,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)
@@ -5418,10 +5442,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;
@@ -5432,7 +5456,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);
@@ -5509,7 +5532,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
@@ -5517,7 +5541,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;
@@ -5525,7 +5548,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);
@@ -5561,9 +5583,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;
@@ -5571,7 +5592,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);
@@ -5598,7 +5618,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,
+ unsigned int buf_len)
{
struct user_namespace *user_ns;
struct ksmbd_file *parent_fp;
@@ -5611,6 +5632,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;
@@ -5633,14 +5658,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)) {
@@ -5649,7 +5673,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)
@@ -5661,15 +5684,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;
@@ -5685,12 +5707,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) ||
@@ -5720,40 +5741,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) {
+ unsigned 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)) {
@@ -5762,18 +5817,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;
}
@@ -5834,8 +5900,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] 19+ messages in thread* Re: [PATCH 2/7] ksmbd: add request buffer validation in smb2_set_info
2021-09-24 2:12 ` [PATCH 2/7] ksmbd: add request buffer validation in smb2_set_info Namjae Jeon
@ 2021-09-25 8:13 ` Hyunchul Lee
2021-09-25 9:19 ` Namjae Jeon
0 siblings, 1 reply; 19+ messages in thread
From: Hyunchul Lee @ 2021-09-25 8:13 UTC (permalink / raw)
To: Namjae Jeon
Cc: linux-cifs, Tom Talpey, Ronnie Sahlberg, Ralph Böhme,
Steve French, Sergey Senozhatsky
2021년 9월 24일 (금) 오전 11:13, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>
> Add buffer validation in smb2_set_info, and remove unused variable
> in set_file_basic_info. and smb2_set_info infolevel functions take
> structure pointer argument.
>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Cc: Hyunchul Lee <hyc.lee@gmail.com>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
> fs/ksmbd/smb2pdu.c | 149 ++++++++++++++++++++++++++++++++-------------
> fs/ksmbd/smb2pdu.h | 9 +++
> 2 files changed, 116 insertions(+), 42 deletions(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index b22d4207c077..a930838fd6ac 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -2107,16 +2107,22 @@ 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;
> + unsigned 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)
> @@ -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);
> @@ -2771,7 +2783,15 @@ 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);
> + if (le32_to_cpu(ea_buf->ccontext.DataLength) <
> + sizeof(struct smb2_ea_info)) {
> + rc = -EINVAL;
> + goto err_out;
> + }
> +
> + rc = smb2_set_ea(&ea_buf->ea,
> + le32_to_cpu(ea_buf->ccontext.DataLength),
> + &path);
> if (rc == -EOPNOTSUPP)
> rc = 0;
> else if (rc)
> @@ -5354,7 +5374,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,
> + unsigned int buf_len, struct file *filp,
> struct nls_table *local_nls)
> {
> char *link_name = NULL, *target_name = NULL, *pathname = NULL;
> @@ -5362,6 +5382,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))
Do we need to cast this to u64 or check FileNameLength to prevent
32-bit overflow?
> + return -EINVAL;
> +
> ksmbd_debug(SMB, "setting FILE_LINK_INFORMATION\n");
> pathname = kmalloc(PATH_MAX, GFP_KERNEL);
> if (!pathname)
> @@ -5418,10 +5442,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;
> @@ -5432,7 +5456,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);
> @@ -5509,7 +5532,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
> @@ -5517,7 +5541,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;
> @@ -5525,7 +5548,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);
>
> @@ -5561,9 +5583,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;
> @@ -5571,7 +5592,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);
>
> @@ -5598,7 +5618,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,
> + unsigned int buf_len)
> {
> struct user_namespace *user_ns;
> struct ksmbd_file *parent_fp;
> @@ -5611,6 +5632,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))
Also we need to cast or check FileNameLength?
Looks good to me except these.
> + return -EINVAL;
> +
> user_ns = file_mnt_user_ns(fp->filp);
> if (ksmbd_stream_fd(fp))
> goto next;
> @@ -5633,14 +5658,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)) {
> @@ -5649,7 +5673,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)
> @@ -5661,15 +5684,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;
>
> @@ -5685,12 +5707,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) ||
> @@ -5720,40 +5741,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) {
> + unsigned 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)) {
> @@ -5762,18 +5817,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;
> }
>
> @@ -5834,8 +5900,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
>
--
Thanks,
Hyunchul
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/7] ksmbd: add request buffer validation in smb2_set_info
2021-09-25 8:13 ` Hyunchul Lee
@ 2021-09-25 9:19 ` Namjae Jeon
0 siblings, 0 replies; 19+ messages in thread
From: Namjae Jeon @ 2021-09-25 9:19 UTC (permalink / raw)
To: Hyunchul Lee
Cc: linux-cifs, Tom Talpey, Ronnie Sahlberg, Ralph Böhme,
Steve French, Sergey Senozhatsky
2021-09-25 17:13 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> 2021년 9월 24일 (금) 오전 11:13, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>
>>
>> Add buffer validation in smb2_set_info, and remove unused variable
>> in set_file_basic_info. and smb2_set_info infolevel functions take
>> structure pointer argument.
>>
>> Cc: Tom Talpey <tom@talpey.com>
>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> Cc: Ralph Böhme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Cc: Hyunchul Lee <hyc.lee@gmail.com>
>> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>> fs/ksmbd/smb2pdu.c | 149 ++++++++++++++++++++++++++++++++-------------
>> fs/ksmbd/smb2pdu.h | 9 +++
>> 2 files changed, 116 insertions(+), 42 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index b22d4207c077..a930838fd6ac 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -2107,16 +2107,22 @@ 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;
>> + unsigned 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)
>> @@ -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);
>> @@ -2771,7 +2783,15 @@ 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);
>> + if (le32_to_cpu(ea_buf->ccontext.DataLength) <
>> + sizeof(struct smb2_ea_info)) {
>> + rc = -EINVAL;
>> + goto err_out;
>> + }
>> +
>> + rc = smb2_set_ea(&ea_buf->ea,
>> +
>> le32_to_cpu(ea_buf->ccontext.DataLength),
>> + &path);
>> if (rc == -EOPNOTSUPP)
>> rc = 0;
>> else if (rc)
>> @@ -5354,7 +5374,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,
>> + unsigned int buf_len, struct file *filp,
>> struct nls_table *local_nls)
>> {
>> char *link_name = NULL, *target_name = NULL, *pathname = NULL;
>> @@ -5362,6 +5382,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))
>
> Do we need to cast this to u64 or check FileNameLength to prevent
> 32-bit overflow?
Yes, I will fix it on next version.
>
>> + return -EINVAL;
>> +
>> ksmbd_debug(SMB, "setting FILE_LINK_INFORMATION\n");
>> pathname = kmalloc(PATH_MAX, GFP_KERNEL);
>> if (!pathname)
>> @@ -5418,10 +5442,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;
>> @@ -5432,7 +5456,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);
>> @@ -5509,7 +5532,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
>> @@ -5517,7 +5541,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;
>> @@ -5525,7 +5548,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);
>>
>> @@ -5561,9 +5583,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;
>> @@ -5571,7 +5592,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);
>>
>> @@ -5598,7 +5618,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,
>> + unsigned int buf_len)
>> {
>> struct user_namespace *user_ns;
>> struct ksmbd_file *parent_fp;
>> @@ -5611,6 +5632,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))
>
> Also we need to cast or check FileNameLength?
Yes.
> Looks good to me except these.
Thanks for your review!
>
>> + return -EINVAL;
>> +
>> user_ns = file_mnt_user_ns(fp->filp);
>> if (ksmbd_stream_fd(fp))
>> goto next;
>> @@ -5633,14 +5658,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)) {
>> @@ -5649,7 +5673,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)
>> @@ -5661,15 +5684,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;
>>
>> @@ -5685,12 +5707,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) ||
>> @@ -5720,40 +5741,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) {
>> + unsigned 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)) {
>> @@ -5762,18 +5817,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;
>> }
>>
>> @@ -5834,8 +5900,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
>>
>
>
> --
> Thanks,
> Hyunchul
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/7] ksmbd: remove RFC1002 check in smb2 request
2021-09-24 2:12 [PATCH 0/7] a bunch of patches that have not yet been reviewed Namjae Jeon
2021-09-24 2:12 ` [PATCH 1/7] ksmbd: add validation in smb2_ioctl Namjae Jeon
2021-09-24 2:12 ` [PATCH 2/7] ksmbd: add request buffer validation in smb2_set_info Namjae Jeon
@ 2021-09-24 2:12 ` Namjae Jeon
2021-09-25 8:18 ` Hyunchul Lee
2021-09-24 2:12 ` [PATCH 4/7] ksmbd: check strictly data area in ksmbd_smb2_check_message() Namjae Jeon
` (3 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Namjae Jeon @ 2021-09-24 2:12 UTC (permalink / raw)
To: linux-cifs
Cc: Ronnie Sahlberg, Tom Talpey, Ronnie Sahlberg, Ralph Böhme,
Steve French, Hyunchul Lee, Sergey Senozhatsky, Namjae Jeon
From: Ronnie Sahlberg <lsahlber@redhat.com>
In smb_common.c you have this function : ksmbd_smb_request() which
is called from connection.c once you have read the initial 4 bytes for
the next length+smb2 blob.
It checks the first byte of this 4 byte preamble for valid values,
i.e. a NETBIOSoverTCP SESSION_MESSAGE or a SESSION_KEEP_ALIVE.
We don't need to check this for ksmbd since it only implements SMB2
over TCP port 445.
The netbios stuff was only used in very old servers when SMB ran over
TCP port 139.
Now that we run over TCP port 445, this is actually not a NB header anymore
and you can just treat it as a 4 byte length field that must be less
than 16Mbyte. and remove the references to the RFC1002 constants that no
longer applies.
Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
fs/ksmbd/smb_common.c | 15 +--------------
fs/ksmbd/smb_common.h | 8 --------
2 files changed, 1 insertion(+), 22 deletions(-)
diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
index 40f4fafa2e11..5901b2884c60 100644
--- a/fs/ksmbd/smb_common.c
+++ b/fs/ksmbd/smb_common.c
@@ -155,20 +155,7 @@ int ksmbd_verify_smb_message(struct ksmbd_work *work)
*/
bool ksmbd_smb_request(struct ksmbd_conn *conn)
{
- int type = *(char *)conn->request_buf;
-
- switch (type) {
- case RFC1002_SESSION_MESSAGE:
- /* Regular SMB request */
- return true;
- case RFC1002_SESSION_KEEP_ALIVE:
- ksmbd_debug(SMB, "RFC 1002 session keep alive\n");
- break;
- default:
- ksmbd_debug(SMB, "RFC 1002 unknown request type 0x%x\n", type);
- }
-
- return false;
+ return conn->request_buf[0] == 0;
}
static bool supported_protocol(int idx)
diff --git a/fs/ksmbd/smb_common.h b/fs/ksmbd/smb_common.h
index 0a6af447cc45..994abede27e9 100644
--- a/fs/ksmbd/smb_common.h
+++ b/fs/ksmbd/smb_common.h
@@ -48,14 +48,6 @@
#define CIFS_DEFAULT_IOSIZE (64 * 1024)
#define MAX_CIFS_SMALL_BUFFER_SIZE 448 /* big enough for most */
-/* RFC 1002 session packet types */
-#define RFC1002_SESSION_MESSAGE 0x00
-#define RFC1002_SESSION_REQUEST 0x81
-#define RFC1002_POSITIVE_SESSION_RESPONSE 0x82
-#define RFC1002_NEGATIVE_SESSION_RESPONSE 0x83
-#define RFC1002_RETARGET_SESSION_RESPONSE 0x84
-#define RFC1002_SESSION_KEEP_ALIVE 0x85
-
/* Responses when opening a file. */
#define F_SUPERSEDED 0
#define F_OPENED 1
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 3/7] ksmbd: remove RFC1002 check in smb2 request
2021-09-24 2:12 ` [PATCH 3/7] ksmbd: remove RFC1002 check in smb2 request Namjae Jeon
@ 2021-09-25 8:18 ` Hyunchul Lee
0 siblings, 0 replies; 19+ messages in thread
From: Hyunchul Lee @ 2021-09-25 8:18 UTC (permalink / raw)
To: Namjae Jeon
Cc: linux-cifs, Ronnie Sahlberg, Tom Talpey, Ronnie Sahlberg,
Ralph Böhme, Steve French, Sergey Senozhatsky
Looks good to me.
Acked-by: Hyunchul Lee <hyc.lee@gmail.com>
2021년 9월 24일 (금) 오전 11:13, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>
> From: Ronnie Sahlberg <lsahlber@redhat.com>
>
> In smb_common.c you have this function : ksmbd_smb_request() which
> is called from connection.c once you have read the initial 4 bytes for
> the next length+smb2 blob.
>
> It checks the first byte of this 4 byte preamble for valid values,
> i.e. a NETBIOSoverTCP SESSION_MESSAGE or a SESSION_KEEP_ALIVE.
>
> We don't need to check this for ksmbd since it only implements SMB2
> over TCP port 445.
> The netbios stuff was only used in very old servers when SMB ran over
> TCP port 139.
> Now that we run over TCP port 445, this is actually not a NB header anymore
> and you can just treat it as a 4 byte length field that must be less
> than 16Mbyte. and remove the references to the RFC1002 constants that no
> longer applies.
>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Cc: Hyunchul Lee <hyc.lee@gmail.com>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
> fs/ksmbd/smb_common.c | 15 +--------------
> fs/ksmbd/smb_common.h | 8 --------
> 2 files changed, 1 insertion(+), 22 deletions(-)
>
> diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
> index 40f4fafa2e11..5901b2884c60 100644
> --- a/fs/ksmbd/smb_common.c
> +++ b/fs/ksmbd/smb_common.c
> @@ -155,20 +155,7 @@ int ksmbd_verify_smb_message(struct ksmbd_work *work)
> */
> bool ksmbd_smb_request(struct ksmbd_conn *conn)
> {
> - int type = *(char *)conn->request_buf;
> -
> - switch (type) {
> - case RFC1002_SESSION_MESSAGE:
> - /* Regular SMB request */
> - return true;
> - case RFC1002_SESSION_KEEP_ALIVE:
> - ksmbd_debug(SMB, "RFC 1002 session keep alive\n");
> - break;
> - default:
> - ksmbd_debug(SMB, "RFC 1002 unknown request type 0x%x\n", type);
> - }
> -
> - return false;
> + return conn->request_buf[0] == 0;
> }
>
> static bool supported_protocol(int idx)
> diff --git a/fs/ksmbd/smb_common.h b/fs/ksmbd/smb_common.h
> index 0a6af447cc45..994abede27e9 100644
> --- a/fs/ksmbd/smb_common.h
> +++ b/fs/ksmbd/smb_common.h
> @@ -48,14 +48,6 @@
> #define CIFS_DEFAULT_IOSIZE (64 * 1024)
> #define MAX_CIFS_SMALL_BUFFER_SIZE 448 /* big enough for most */
>
> -/* RFC 1002 session packet types */
> -#define RFC1002_SESSION_MESSAGE 0x00
> -#define RFC1002_SESSION_REQUEST 0x81
> -#define RFC1002_POSITIVE_SESSION_RESPONSE 0x82
> -#define RFC1002_NEGATIVE_SESSION_RESPONSE 0x83
> -#define RFC1002_RETARGET_SESSION_RESPONSE 0x84
> -#define RFC1002_SESSION_KEEP_ALIVE 0x85
> -
> /* Responses when opening a file. */
> #define F_SUPERSEDED 0
> #define F_OPENED 1
> --
> 2.25.1
>
--
Thanks,
Hyunchul
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/7] ksmbd: check strictly data area in ksmbd_smb2_check_message()
2021-09-24 2:12 [PATCH 0/7] a bunch of patches that have not yet been reviewed Namjae Jeon
` (2 preceding siblings ...)
2021-09-24 2:12 ` [PATCH 3/7] ksmbd: remove RFC1002 check in smb2 request Namjae Jeon
@ 2021-09-24 2:12 ` Namjae Jeon
2021-09-25 10:27 ` Hyunchul Lee
2021-09-24 2:12 ` [PATCH 5/7] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value Namjae Jeon
` (2 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Namjae Jeon @ 2021-09-24 2:12 UTC (permalink / raw)
To: linux-cifs
Cc: Namjae Jeon, Tom Talpey, Ronnie Sahlberg, Ralph Böhme,
Steve French, Hyunchul Lee, Sergey Senozhatsky
When invalid data offset and data length in request,
ksmbd_smb2_check_message check strictly and doesn't allow to process such
requests.
Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
fs/ksmbd/smb2misc.c | 83 ++++++++++++++++++++-------------------------
1 file changed, 37 insertions(+), 46 deletions(-)
diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
index 9aa46bb3e10d..697285e47522 100644
--- a/fs/ksmbd/smb2misc.c
+++ b/fs/ksmbd/smb2misc.c
@@ -83,15 +83,18 @@ static const bool has_smb2_data_area[NUMBER_OF_SMB2_COMMANDS] = {
* Returns the pointer to the beginning of the data area. Length of the data
* area and the offset to it (from the beginning of the smb are also returned.
*/
-static char *smb2_get_data_area_len(int *off, int *len, struct smb2_hdr *hdr)
+static int smb2_get_data_area_len(unsigned int *off, unsigned int *len,
+ struct smb2_hdr *hdr)
{
+ int ret = 0;
+
*off = 0;
*len = 0;
/* error reqeusts do not have data area */
if (hdr->Status && hdr->Status != STATUS_MORE_PROCESSING_REQUIRED &&
(((struct smb2_err_rsp *)hdr)->StructureSize) == SMB2_ERROR_STRUCTURE_SIZE2_LE)
- return NULL;
+ return ret;
/*
* Following commands have data areas so we have to get the location
@@ -165,69 +168,52 @@ static char *smb2_get_data_area_len(int *off, int *len, struct smb2_hdr *hdr)
case SMB2_IOCTL:
*off = le32_to_cpu(((struct smb2_ioctl_req *)hdr)->InputOffset);
*len = le32_to_cpu(((struct smb2_ioctl_req *)hdr)->InputCount);
-
break;
default:
ksmbd_debug(SMB, "no length check for command\n");
break;
}
- /*
- * Invalid length or offset probably means data area is invalid, but
- * we have little choice but to ignore the data area in this case.
- */
if (*off > 4096) {
- ksmbd_debug(SMB, "offset %d too large, data area ignored\n",
- *off);
- *len = 0;
- *off = 0;
- } else if (*off < 0) {
- ksmbd_debug(SMB,
- "negative offset %d to data invalid ignore data area\n",
- *off);
- *off = 0;
- *len = 0;
- } else if (*len < 0) {
- ksmbd_debug(SMB,
- "negative data length %d invalid, data area ignored\n",
- *len);
- *len = 0;
- } else if (*len > 128 * 1024) {
- ksmbd_debug(SMB, "data area larger than 128K: %d\n", *len);
- *len = 0;
+ ksmbd_debug(SMB, "offset %d too large\n", *off);
+ ret = -EINVAL;
+ } else if ((u64)*off + *len > MAX_STREAM_PROT_LEN) {
+ ksmbd_debug(SMB, "Request is larger than maximum stream protocol length(%u): %llu\n",
+ MAX_STREAM_PROT_LEN, (u64)*off + *len);
+ ret = -EINVAL;
}
- /* return pointer to beginning of data area, ie offset from SMB start */
- if ((*off != 0) && (*len != 0))
- return (char *)hdr + *off;
- else
- return NULL;
+ return ret;
}
/*
* Calculate the size of the SMB message based on the fixed header
* portion, the number of word parameters and the data portion of the message.
*/
-static unsigned int smb2_calc_size(void *buf)
+static int smb2_calc_size(void *buf, unsigned int *len)
{
struct smb2_pdu *pdu = (struct smb2_pdu *)buf;
struct smb2_hdr *hdr = &pdu->hdr;
- int offset; /* the offset from the beginning of SMB to data area */
- int data_length; /* the length of the variable length data area */
+ unsigned int offset; /* the offset from the beginning of SMB to data area */
+ unsigned int data_length; /* the length of the variable length data area */
+ int ret;
+
/* Structure Size has already been checked to make sure it is 64 */
- int len = le16_to_cpu(hdr->StructureSize);
+ *len = le16_to_cpu(hdr->StructureSize);
/*
* StructureSize2, ie length of fixed parameter area has already
* been checked to make sure it is the correct length.
*/
- len += le16_to_cpu(pdu->StructureSize2);
+ *len += le16_to_cpu(pdu->StructureSize2);
if (has_smb2_data_area[le16_to_cpu(hdr->Command)] == false)
goto calc_size_exit;
- smb2_get_data_area_len(&offset, &data_length, hdr);
- ksmbd_debug(SMB, "SMB2 data length %d offset %d\n", data_length,
+ ret = smb2_get_data_area_len(&offset, &data_length, hdr);
+ if (ret)
+ return ret;
+ ksmbd_debug(SMB, "SMB2 data length %u offset %u\n", data_length,
offset);
if (data_length > 0) {
@@ -237,16 +223,19 @@ static unsigned int smb2_calc_size(void *buf)
* for some commands, typically those with odd StructureSize,
* so we must add one to the calculation.
*/
- if (offset + 1 < len)
+ if (offset + 1 < *len) {
ksmbd_debug(SMB,
- "data area offset %d overlaps SMB2 header %d\n",
- offset + 1, len);
- else
- len = offset + data_length;
+ "data area offset %d overlaps SMB2 header %u\n",
+ offset + 1, *len);
+ return -EINVAL;
+ }
+
+ *len = offset + data_length;
}
+
calc_size_exit:
- ksmbd_debug(SMB, "SMB2 len %d\n", len);
- return len;
+ ksmbd_debug(SMB, "SMB2 len %u\n", *len);
+ return 0;
}
static inline int smb2_query_info_req_len(struct smb2_query_info_req *h)
@@ -391,9 +380,11 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
return 1;
}
- clc_len = smb2_calc_size(hdr);
+ if (smb2_calc_size(hdr, &clc_len))
+ return 1;
+
if (len != clc_len) {
- /* server can return one byte more due to implied bcc[0] */
+ /* client can return one byte more due to implied bcc[0] */
if (clc_len == len + 1)
return 0;
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 4/7] ksmbd: check strictly data area in ksmbd_smb2_check_message()
2021-09-24 2:12 ` [PATCH 4/7] ksmbd: check strictly data area in ksmbd_smb2_check_message() Namjae Jeon
@ 2021-09-25 10:27 ` Hyunchul Lee
2021-09-25 10:46 ` Namjae Jeon
0 siblings, 1 reply; 19+ messages in thread
From: Hyunchul Lee @ 2021-09-25 10:27 UTC (permalink / raw)
To: Namjae Jeon
Cc: linux-cifs, Tom Talpey, Ronnie Sahlberg, Ralph Böhme,
Steve French, Sergey Senozhatsky
Looks good to me.
Acked-by: Hyunchul Lee <hyc.lee@gmail.com>
2021년 9월 24일 (금) 오전 11:13, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>
> When invalid data offset and data length in request,
> ksmbd_smb2_check_message check strictly and doesn't allow to process such
> requests.
>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Cc: Hyunchul Lee <hyc.lee@gmail.com>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
> fs/ksmbd/smb2misc.c | 83 ++++++++++++++++++++-------------------------
> 1 file changed, 37 insertions(+), 46 deletions(-)
>
> diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
> index 9aa46bb3e10d..697285e47522 100644
> --- a/fs/ksmbd/smb2misc.c
> +++ b/fs/ksmbd/smb2misc.c
> @@ -83,15 +83,18 @@ static const bool has_smb2_data_area[NUMBER_OF_SMB2_COMMANDS] = {
> * Returns the pointer to the beginning of the data area. Length of the data
Do we need to change this comment about the return value?
Looks good to me except this.
Acked-by: Hyunchul Lee <hyc.lee@gmail.com>
> * area and the offset to it (from the beginning of the smb are also returned.
> */
> -static char *smb2_get_data_area_len(int *off, int *len, struct smb2_hdr *hdr)
> +static int smb2_get_data_area_len(unsigned int *off, unsigned int *len,
> + struct smb2_hdr *hdr)
> {
> + int ret = 0;
> +
> *off = 0;
> *len = 0;
>
> /* error reqeusts do not have data area */
> if (hdr->Status && hdr->Status != STATUS_MORE_PROCESSING_REQUIRED &&
> (((struct smb2_err_rsp *)hdr)->StructureSize) == SMB2_ERROR_STRUCTURE_SIZE2_LE)
> - return NULL;
> + return ret;
>
> /*
> * Following commands have data areas so we have to get the location
> @@ -165,69 +168,52 @@ static char *smb2_get_data_area_len(int *off, int *len, struct smb2_hdr *hdr)
> case SMB2_IOCTL:
> *off = le32_to_cpu(((struct smb2_ioctl_req *)hdr)->InputOffset);
> *len = le32_to_cpu(((struct smb2_ioctl_req *)hdr)->InputCount);
> -
> break;
> default:
> ksmbd_debug(SMB, "no length check for command\n");
> break;
> }
>
> - /*
> - * Invalid length or offset probably means data area is invalid, but
> - * we have little choice but to ignore the data area in this case.
> - */
> if (*off > 4096) {
> - ksmbd_debug(SMB, "offset %d too large, data area ignored\n",
> - *off);
> - *len = 0;
> - *off = 0;
> - } else if (*off < 0) {
> - ksmbd_debug(SMB,
> - "negative offset %d to data invalid ignore data area\n",
> - *off);
> - *off = 0;
> - *len = 0;
> - } else if (*len < 0) {
> - ksmbd_debug(SMB,
> - "negative data length %d invalid, data area ignored\n",
> - *len);
> - *len = 0;
> - } else if (*len > 128 * 1024) {
> - ksmbd_debug(SMB, "data area larger than 128K: %d\n", *len);
> - *len = 0;
> + ksmbd_debug(SMB, "offset %d too large\n", *off);
> + ret = -EINVAL;
> + } else if ((u64)*off + *len > MAX_STREAM_PROT_LEN) {
> + ksmbd_debug(SMB, "Request is larger than maximum stream protocol length(%u): %llu\n",
> + MAX_STREAM_PROT_LEN, (u64)*off + *len);
> + ret = -EINVAL;
> }
>
> - /* return pointer to beginning of data area, ie offset from SMB start */
> - if ((*off != 0) && (*len != 0))
> - return (char *)hdr + *off;
> - else
> - return NULL;
> + return ret;
> }
>
> /*
> * Calculate the size of the SMB message based on the fixed header
> * portion, the number of word parameters and the data portion of the message.
> */
> -static unsigned int smb2_calc_size(void *buf)
> +static int smb2_calc_size(void *buf, unsigned int *len)
> {
> struct smb2_pdu *pdu = (struct smb2_pdu *)buf;
> struct smb2_hdr *hdr = &pdu->hdr;
> - int offset; /* the offset from the beginning of SMB to data area */
> - int data_length; /* the length of the variable length data area */
> + unsigned int offset; /* the offset from the beginning of SMB to data area */
> + unsigned int data_length; /* the length of the variable length data area */
> + int ret;
> +
> /* Structure Size has already been checked to make sure it is 64 */
> - int len = le16_to_cpu(hdr->StructureSize);
> + *len = le16_to_cpu(hdr->StructureSize);
>
> /*
> * StructureSize2, ie length of fixed parameter area has already
> * been checked to make sure it is the correct length.
> */
> - len += le16_to_cpu(pdu->StructureSize2);
> + *len += le16_to_cpu(pdu->StructureSize2);
>
> if (has_smb2_data_area[le16_to_cpu(hdr->Command)] == false)
> goto calc_size_exit;
>
> - smb2_get_data_area_len(&offset, &data_length, hdr);
> - ksmbd_debug(SMB, "SMB2 data length %d offset %d\n", data_length,
> + ret = smb2_get_data_area_len(&offset, &data_length, hdr);
> + if (ret)
> + return ret;
> + ksmbd_debug(SMB, "SMB2 data length %u offset %u\n", data_length,
> offset);
>
> if (data_length > 0) {
> @@ -237,16 +223,19 @@ static unsigned int smb2_calc_size(void *buf)
> * for some commands, typically those with odd StructureSize,
> * so we must add one to the calculation.
> */
> - if (offset + 1 < len)
> + if (offset + 1 < *len) {
> ksmbd_debug(SMB,
> - "data area offset %d overlaps SMB2 header %d\n",
> - offset + 1, len);
> - else
> - len = offset + data_length;
> + "data area offset %d overlaps SMB2 header %u\n",
> + offset + 1, *len);
> + return -EINVAL;
> + }
> +
> + *len = offset + data_length;
> }
> +
> calc_size_exit:
> - ksmbd_debug(SMB, "SMB2 len %d\n", len);
> - return len;
> + ksmbd_debug(SMB, "SMB2 len %u\n", *len);
> + return 0;
> }
>
> static inline int smb2_query_info_req_len(struct smb2_query_info_req *h)
> @@ -391,9 +380,11 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
> return 1;
> }
>
> - clc_len = smb2_calc_size(hdr);
> + if (smb2_calc_size(hdr, &clc_len))
> + return 1;
> +
> if (len != clc_len) {
> - /* server can return one byte more due to implied bcc[0] */
> + /* client can return one byte more due to implied bcc[0] */
> if (clc_len == len + 1)
> return 0;
>
> --
> 2.25.1
>
--
Thanks,
Hyunchul
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 4/7] ksmbd: check strictly data area in ksmbd_smb2_check_message()
2021-09-25 10:27 ` Hyunchul Lee
@ 2021-09-25 10:46 ` Namjae Jeon
0 siblings, 0 replies; 19+ messages in thread
From: Namjae Jeon @ 2021-09-25 10:46 UTC (permalink / raw)
To: Hyunchul Lee
Cc: linux-cifs, Tom Talpey, Ronnie Sahlberg, Ralph Böhme,
Steve French, Sergey Senozhatsky
2021-09-25 19:27 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> Looks good to me.
> Acked-by: Hyunchul Lee <hyc.lee@gmail.com>
>
> 2021년 9월 24일 (금) 오전 11:13, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>>
>> When invalid data offset and data length in request,
>> ksmbd_smb2_check_message check strictly and doesn't allow to process such
>> requests.
>>
>> Cc: Tom Talpey <tom@talpey.com>
>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> Cc: Ralph Böhme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Cc: Hyunchul Lee <hyc.lee@gmail.com>
>> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>> fs/ksmbd/smb2misc.c | 83 ++++++++++++++++++++-------------------------
>> 1 file changed, 37 insertions(+), 46 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
>> index 9aa46bb3e10d..697285e47522 100644
>> --- a/fs/ksmbd/smb2misc.c
>> +++ b/fs/ksmbd/smb2misc.c
>> @@ -83,15 +83,18 @@ static const bool
>> has_smb2_data_area[NUMBER_OF_SMB2_COMMANDS] = {
>> * Returns the pointer to the beginning of the data area. Length of the
>> data
>
> Do we need to change this comment about the return value?
Will update it on next version.
> Looks good to me except this.
> Acked-by: Hyunchul Lee <hyc.lee@gmail.com>
Thanks!
>
>
>> * area and the offset to it (from the beginning of the smb are also
>> returned.
>> */
>> -static char *smb2_get_data_area_len(int *off, int *len, struct smb2_hdr
>> *hdr)
>> +static int smb2_get_data_area_len(unsigned int *off, unsigned int *len,
>> + struct smb2_hdr *hdr)
>> {
>> + int ret = 0;
>> +
>> *off = 0;
>> *len = 0;
>>
>> /* error reqeusts do not have data area */
>> if (hdr->Status && hdr->Status != STATUS_MORE_PROCESSING_REQUIRED
>> &&
>> (((struct smb2_err_rsp *)hdr)->StructureSize) ==
>> SMB2_ERROR_STRUCTURE_SIZE2_LE)
>> - return NULL;
>> + return ret;
>>
>> /*
>> * Following commands have data areas so we have to get the
>> location
>> @@ -165,69 +168,52 @@ static char *smb2_get_data_area_len(int *off, int
>> *len, struct smb2_hdr *hdr)
>> case SMB2_IOCTL:
>> *off = le32_to_cpu(((struct smb2_ioctl_req
>> *)hdr)->InputOffset);
>> *len = le32_to_cpu(((struct smb2_ioctl_req
>> *)hdr)->InputCount);
>> -
>> break;
>> default:
>> ksmbd_debug(SMB, "no length check for command\n");
>> break;
>> }
>>
>> - /*
>> - * Invalid length or offset probably means data area is invalid,
>> but
>> - * we have little choice but to ignore the data area in this
>> case.
>> - */
>> if (*off > 4096) {
>> - ksmbd_debug(SMB, "offset %d too large, data area
>> ignored\n",
>> - *off);
>> - *len = 0;
>> - *off = 0;
>> - } else if (*off < 0) {
>> - ksmbd_debug(SMB,
>> - "negative offset %d to data invalid ignore
>> data area\n",
>> - *off);
>> - *off = 0;
>> - *len = 0;
>> - } else if (*len < 0) {
>> - ksmbd_debug(SMB,
>> - "negative data length %d invalid, data area
>> ignored\n",
>> - *len);
>> - *len = 0;
>> - } else if (*len > 128 * 1024) {
>> - ksmbd_debug(SMB, "data area larger than 128K: %d\n",
>> *len);
>> - *len = 0;
>> + ksmbd_debug(SMB, "offset %d too large\n", *off);
>> + ret = -EINVAL;
>> + } else if ((u64)*off + *len > MAX_STREAM_PROT_LEN) {
>> + ksmbd_debug(SMB, "Request is larger than maximum stream
>> protocol length(%u): %llu\n",
>> + MAX_STREAM_PROT_LEN, (u64)*off + *len);
>> + ret = -EINVAL;
>> }
>>
>> - /* return pointer to beginning of data area, ie offset from SMB
>> start */
>> - if ((*off != 0) && (*len != 0))
>> - return (char *)hdr + *off;
>> - else
>> - return NULL;
>> + return ret;
>> }
>>
>> /*
>> * Calculate the size of the SMB message based on the fixed header
>> * portion, the number of word parameters and the data portion of the
>> message.
>> */
>> -static unsigned int smb2_calc_size(void *buf)
>> +static int smb2_calc_size(void *buf, unsigned int *len)
>> {
>> struct smb2_pdu *pdu = (struct smb2_pdu *)buf;
>> struct smb2_hdr *hdr = &pdu->hdr;
>> - int offset; /* the offset from the beginning of SMB to data area
>> */
>> - int data_length; /* the length of the variable length data area
>> */
>> + unsigned int offset; /* the offset from the beginning of SMB to
>> data area */
>> + unsigned int data_length; /* the length of the variable length
>> data area */
>> + int ret;
>> +
>> /* Structure Size has already been checked to make sure it is 64
>> */
>> - int len = le16_to_cpu(hdr->StructureSize);
>> + *len = le16_to_cpu(hdr->StructureSize);
>>
>> /*
>> * StructureSize2, ie length of fixed parameter area has already
>> * been checked to make sure it is the correct length.
>> */
>> - len += le16_to_cpu(pdu->StructureSize2);
>> + *len += le16_to_cpu(pdu->StructureSize2);
>>
>> if (has_smb2_data_area[le16_to_cpu(hdr->Command)] == false)
>> goto calc_size_exit;
>>
>> - smb2_get_data_area_len(&offset, &data_length, hdr);
>> - ksmbd_debug(SMB, "SMB2 data length %d offset %d\n", data_length,
>> + ret = smb2_get_data_area_len(&offset, &data_length, hdr);
>> + if (ret)
>> + return ret;
>> + ksmbd_debug(SMB, "SMB2 data length %u offset %u\n", data_length,
>> offset);
>>
>> if (data_length > 0) {
>> @@ -237,16 +223,19 @@ static unsigned int smb2_calc_size(void *buf)
>> * for some commands, typically those with odd
>> StructureSize,
>> * so we must add one to the calculation.
>> */
>> - if (offset + 1 < len)
>> + if (offset + 1 < *len) {
>> ksmbd_debug(SMB,
>> - "data area offset %d overlaps SMB2
>> header %d\n",
>> - offset + 1, len);
>> - else
>> - len = offset + data_length;
>> + "data area offset %d overlaps SMB2
>> header %u\n",
>> + offset + 1, *len);
>> + return -EINVAL;
>> + }
>> +
>> + *len = offset + data_length;
>> }
>> +
>> calc_size_exit:
>> - ksmbd_debug(SMB, "SMB2 len %d\n", len);
>> - return len;
>> + ksmbd_debug(SMB, "SMB2 len %u\n", *len);
>> + return 0;
>> }
>>
>> static inline int smb2_query_info_req_len(struct smb2_query_info_req *h)
>> @@ -391,9 +380,11 @@ int ksmbd_smb2_check_message(struct ksmbd_work
>> *work)
>> return 1;
>> }
>>
>> - clc_len = smb2_calc_size(hdr);
>> + if (smb2_calc_size(hdr, &clc_len))
>> + return 1;
>> +
>> if (len != clc_len) {
>> - /* server can return one byte more due to implied bcc[0]
>> */
>> + /* client can return one byte more due to implied bcc[0]
>> */
>> if (clc_len == len + 1)
>> return 0;
>>
>> --
>> 2.25.1
>>
>
>
> --
> Thanks,
> Hyunchul
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 5/7] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value
2021-09-24 2:12 [PATCH 0/7] a bunch of patches that have not yet been reviewed Namjae Jeon
` (3 preceding siblings ...)
2021-09-24 2:12 ` [PATCH 4/7] ksmbd: check strictly data area in ksmbd_smb2_check_message() Namjae Jeon
@ 2021-09-24 2:12 ` Namjae Jeon
2021-09-25 8:41 ` Hyunchul Lee
2021-09-24 2:12 ` [PATCH 6/7] ksmbd: fix invalid request buffer access in compound Namjae Jeon
2021-09-24 2:12 ` [PATCH 7/7] ksmbd: add validation in smb2 negotiate Namjae Jeon
6 siblings, 1 reply; 19+ messages in thread
From: Namjae Jeon @ 2021-09-24 2:12 UTC (permalink / raw)
To: linux-cifs
Cc: Namjae Jeon, Tom Talpey, Ronnie Sahlberg, Ralph Böhme,
Steve French, Hyunchul Lee, Sergey Senozhatsky
This patch add MAX_STREAM_PROT_LEN macro and check if stream protocol
length exceeds maximum value. opencode pdu size check in
ksmbd_pdu_size_has_room().
Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
fs/ksmbd/connection.c | 10 ++++++----
fs/ksmbd/smb_common.c | 6 ------
fs/ksmbd/smb_common.h | 4 ++--
3 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c
index af086d35398a..48b18b4ec117 100644
--- a/fs/ksmbd/connection.c
+++ b/fs/ksmbd/connection.c
@@ -296,10 +296,12 @@ int ksmbd_conn_handler_loop(void *p)
pdu_size = get_rfc1002_len(hdr_buf);
ksmbd_debug(CONN, "RFC1002 header %u bytes\n", pdu_size);
- /* make sure we have enough to get to SMB header end */
- if (!ksmbd_pdu_size_has_room(pdu_size)) {
- ksmbd_debug(CONN, "SMB request too short (%u bytes)\n",
- pdu_size);
+ /*
+ * Check if pdu size is valid (min : smb header size,
+ * max : 0x00FFFFFF).
+ */
+ if (pdu_size < __SMB2_HEADER_STRUCTURE_SIZE ||
+ pdu_size > MAX_STREAM_PROT_LEN) {
continue;
}
diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
index 5901b2884c60..20bd5b8e3c0a 100644
--- a/fs/ksmbd/smb_common.c
+++ b/fs/ksmbd/smb_common.c
@@ -21,7 +21,6 @@ static const char basechars[43] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_-!@#$%";
#define MAGIC_CHAR '~'
#define PERIOD '.'
#define mangle(V) ((char)(basechars[(V) % MANGLE_BASE]))
-#define KSMBD_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr))
struct smb_protocol {
int index;
@@ -272,11 +271,6 @@ int ksmbd_init_smb_server(struct ksmbd_work *work)
return 0;
}
-bool ksmbd_pdu_size_has_room(unsigned int pdu)
-{
- return (pdu >= KSMBD_MIN_SUPPORTED_HEADER_SIZE - 4);
-}
-
int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work, int info_level,
struct ksmbd_file *dir,
struct ksmbd_dir_info *d_info,
diff --git a/fs/ksmbd/smb_common.h b/fs/ksmbd/smb_common.h
index 994abede27e9..6e79e7577f6b 100644
--- a/fs/ksmbd/smb_common.h
+++ b/fs/ksmbd/smb_common.h
@@ -48,6 +48,8 @@
#define CIFS_DEFAULT_IOSIZE (64 * 1024)
#define MAX_CIFS_SMALL_BUFFER_SIZE 448 /* big enough for most */
+#define MAX_STREAM_PROT_LEN 0x00FFFFFF
+
/* Responses when opening a file. */
#define F_SUPERSEDED 0
#define F_OPENED 1
@@ -493,8 +495,6 @@ int ksmbd_lookup_dialect_by_id(__le16 *cli_dialects, __le16 dialects_count);
int ksmbd_init_smb_server(struct ksmbd_work *work);
-bool ksmbd_pdu_size_has_room(unsigned int pdu);
-
struct ksmbd_kstat;
int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work,
int info_level,
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 5/7] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value
2021-09-24 2:12 ` [PATCH 5/7] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value Namjae Jeon
@ 2021-09-25 8:41 ` Hyunchul Lee
2021-09-25 9:24 ` Namjae Jeon
0 siblings, 1 reply; 19+ messages in thread
From: Hyunchul Lee @ 2021-09-25 8:41 UTC (permalink / raw)
To: Namjae Jeon
Cc: linux-cifs, Tom Talpey, Ronnie Sahlberg, Ralph Böhme,
Steve French, Sergey Senozhatsky
2021년 9월 24일 (금) 오전 11:13, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>
> This patch add MAX_STREAM_PROT_LEN macro and check if stream protocol
> length exceeds maximum value. opencode pdu size check in
> ksmbd_pdu_size_has_room().
>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Cc: Hyunchul Lee <hyc.lee@gmail.com>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
> fs/ksmbd/connection.c | 10 ++++++----
> fs/ksmbd/smb_common.c | 6 ------
> fs/ksmbd/smb_common.h | 4 ++--
> 3 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c
> index af086d35398a..48b18b4ec117 100644
> --- a/fs/ksmbd/connection.c
> +++ b/fs/ksmbd/connection.c
> @@ -296,10 +296,12 @@ int ksmbd_conn_handler_loop(void *p)
> pdu_size = get_rfc1002_len(hdr_buf);
> ksmbd_debug(CONN, "RFC1002 header %u bytes\n", pdu_size);
>
> - /* make sure we have enough to get to SMB header end */
> - if (!ksmbd_pdu_size_has_room(pdu_size)) {
> - ksmbd_debug(CONN, "SMB request too short (%u bytes)\n",
> - pdu_size);
> + /*
> + * Check if pdu size is valid (min : smb header size,
> + * max : 0x00FFFFFF).
> + */
> + if (pdu_size < __SMB2_HEADER_STRUCTURE_SIZE ||
> + pdu_size > MAX_STREAM_PROT_LEN) {
> continue;
> }
>
> diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
> index 5901b2884c60..20bd5b8e3c0a 100644
> --- a/fs/ksmbd/smb_common.c
> +++ b/fs/ksmbd/smb_common.c
> @@ -21,7 +21,6 @@ static const char basechars[43] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_-!@#$%";
> #define MAGIC_CHAR '~'
> #define PERIOD '.'
> #define mangle(V) ((char)(basechars[(V) % MANGLE_BASE]))
> -#define KSMBD_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr))
>
> struct smb_protocol {
> int index;
> @@ -272,11 +271,6 @@ int ksmbd_init_smb_server(struct ksmbd_work *work)
> return 0;
> }
>
> -bool ksmbd_pdu_size_has_room(unsigned int pdu)
> -{
> - return (pdu >= KSMBD_MIN_SUPPORTED_HEADER_SIZE - 4);
> -}
> -
> int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work, int info_level,
> struct ksmbd_file *dir,
> struct ksmbd_dir_info *d_info,
> diff --git a/fs/ksmbd/smb_common.h b/fs/ksmbd/smb_common.h
> index 994abede27e9..6e79e7577f6b 100644
> --- a/fs/ksmbd/smb_common.h
> +++ b/fs/ksmbd/smb_common.h
> @@ -48,6 +48,8 @@
> #define CIFS_DEFAULT_IOSIZE (64 * 1024)
> #define MAX_CIFS_SMALL_BUFFER_SIZE 448 /* big enough for most */
>
> +#define MAX_STREAM_PROT_LEN 0x00FFFFFF
Do we need to append "SMB" to this macro name?
Looks good to me.
Acked-by: Hyunchul Lee <hyc.lee@gmail.com>
> +
> /* Responses when opening a file. */
> #define F_SUPERSEDED 0
> #define F_OPENED 1
> @@ -493,8 +495,6 @@ int ksmbd_lookup_dialect_by_id(__le16 *cli_dialects, __le16 dialects_count);
>
> int ksmbd_init_smb_server(struct ksmbd_work *work);
>
> -bool ksmbd_pdu_size_has_room(unsigned int pdu);
> -
> struct ksmbd_kstat;
> int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work,
> int info_level,
> --
> 2.25.1
>
--
Thanks,
Hyunchul
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 5/7] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value
2021-09-25 8:41 ` Hyunchul Lee
@ 2021-09-25 9:24 ` Namjae Jeon
0 siblings, 0 replies; 19+ messages in thread
From: Namjae Jeon @ 2021-09-25 9:24 UTC (permalink / raw)
To: Hyunchul Lee
Cc: linux-cifs, Tom Talpey, Ronnie Sahlberg, Ralph Böhme,
Steve French, Sergey Senozhatsky
2021-09-25 17:41 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> 2021년 9월 24일 (금) 오전 11:13, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>>
>> This patch add MAX_STREAM_PROT_LEN macro and check if stream protocol
>> length exceeds maximum value. opencode pdu size check in
>> ksmbd_pdu_size_has_room().
>>
>> Cc: Tom Talpey <tom@talpey.com>
>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> Cc: Ralph Böhme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Cc: Hyunchul Lee <hyc.lee@gmail.com>
>> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>> fs/ksmbd/connection.c | 10 ++++++----
>> fs/ksmbd/smb_common.c | 6 ------
>> fs/ksmbd/smb_common.h | 4 ++--
>> 3 files changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c
>> index af086d35398a..48b18b4ec117 100644
>> --- a/fs/ksmbd/connection.c
>> +++ b/fs/ksmbd/connection.c
>> @@ -296,10 +296,12 @@ int ksmbd_conn_handler_loop(void *p)
>> pdu_size = get_rfc1002_len(hdr_buf);
>> ksmbd_debug(CONN, "RFC1002 header %u bytes\n", pdu_size);
>>
>> - /* make sure we have enough to get to SMB header end */
>> - if (!ksmbd_pdu_size_has_room(pdu_size)) {
>> - ksmbd_debug(CONN, "SMB request too short (%u
>> bytes)\n",
>> - pdu_size);
>> + /*
>> + * Check if pdu size is valid (min : smb header size,
>> + * max : 0x00FFFFFF).
>> + */
>> + if (pdu_size < __SMB2_HEADER_STRUCTURE_SIZE ||
>> + pdu_size > MAX_STREAM_PROT_LEN) {
>> continue;
>> }
>>
>> diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
>> index 5901b2884c60..20bd5b8e3c0a 100644
>> --- a/fs/ksmbd/smb_common.c
>> +++ b/fs/ksmbd/smb_common.c
>> @@ -21,7 +21,6 @@ static const char basechars[43] =
>> "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_-!@#$%";
>> #define MAGIC_CHAR '~'
>> #define PERIOD '.'
>> #define mangle(V) ((char)(basechars[(V) % MANGLE_BASE]))
>> -#define KSMBD_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr))
>>
>> struct smb_protocol {
>> int index;
>> @@ -272,11 +271,6 @@ int ksmbd_init_smb_server(struct ksmbd_work *work)
>> return 0;
>> }
>>
>> -bool ksmbd_pdu_size_has_room(unsigned int pdu)
>> -{
>> - return (pdu >= KSMBD_MIN_SUPPORTED_HEADER_SIZE - 4);
>> -}
>> -
>> int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work, int
>> info_level,
>> struct ksmbd_file *dir,
>> struct ksmbd_dir_info *d_info,
>> diff --git a/fs/ksmbd/smb_common.h b/fs/ksmbd/smb_common.h
>> index 994abede27e9..6e79e7577f6b 100644
>> --- a/fs/ksmbd/smb_common.h
>> +++ b/fs/ksmbd/smb_common.h
>> @@ -48,6 +48,8 @@
>> #define CIFS_DEFAULT_IOSIZE (64 * 1024)
>> #define MAX_CIFS_SMALL_BUFFER_SIZE 448 /* big enough for most */
>>
>> +#define MAX_STREAM_PROT_LEN 0x00FFFFFF
>
> Do we need to append "SMB" to this macro name?
I named it with the name defined in specification. That is, no problem.
> Looks good to me.
> Acked-by: Hyunchul Lee <hyc.lee@gmail.com>
Thanks for your review!
>
>
>> +
>> /* Responses when opening a file. */
>> #define F_SUPERSEDED 0
>> #define F_OPENED 1
>> @@ -493,8 +495,6 @@ int ksmbd_lookup_dialect_by_id(__le16 *cli_dialects,
>> __le16 dialects_count);
>>
>> int ksmbd_init_smb_server(struct ksmbd_work *work);
>>
>> -bool ksmbd_pdu_size_has_room(unsigned int pdu);
>> -
>> struct ksmbd_kstat;
>> int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work,
>> int info_level,
>> --
>> 2.25.1
>>
>
>
> --
> Thanks,
> Hyunchul
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 6/7] ksmbd: fix invalid request buffer access in compound
2021-09-24 2:12 [PATCH 0/7] a bunch of patches that have not yet been reviewed Namjae Jeon
` (4 preceding siblings ...)
2021-09-24 2:12 ` [PATCH 5/7] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value Namjae Jeon
@ 2021-09-24 2:12 ` Namjae Jeon
2021-09-25 9:41 ` Hyunchul Lee
2021-09-24 2:12 ` [PATCH 7/7] ksmbd: add validation in smb2 negotiate Namjae Jeon
6 siblings, 1 reply; 19+ messages in thread
From: Namjae Jeon @ 2021-09-24 2:12 UTC (permalink / raw)
To: linux-cifs
Cc: Namjae Jeon, Tom Talpey, Ronnie Sahlberg, Ralph Böhme,
Steve French, Hyunchul Lee, Sergey Senozhatsky
Ronnie reported invalid request buffer access in chained command when
inserting garbage value to NextCommand of compound request.
This patch add validation check to avoid this issue.
Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
fs/ksmbd/smb2pdu.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index a930838fd6ac..4f7b5e18a7b9 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -459,13 +459,22 @@ static void init_chained_smb2_rsp(struct ksmbd_work *work)
bool is_chained_smb2_message(struct ksmbd_work *work)
{
struct smb2_hdr *hdr = work->request_buf;
- unsigned int len;
+ unsigned int len, next_cmd;
if (hdr->ProtocolId != SMB2_PROTO_NUMBER)
return false;
hdr = ksmbd_req_buf_next(work);
- if (le32_to_cpu(hdr->NextCommand) > 0) {
+ next_cmd = le32_to_cpu(hdr->NextCommand);
+ if (next_cmd > 0) {
+ if ((u64)work->next_smb2_rcv_hdr_off + next_cmd +
+ __SMB2_HEADER_STRUCTURE_SIZE >
+ get_rfc1002_len(work->request_buf)) {
+ pr_err("next command(%u) offset exceeds smb msg size\n",
+ next_cmd);
+ return false;
+ }
+
ksmbd_debug(SMB, "got SMB2 chained command\n");
init_chained_smb2_rsp(work);
return true;
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 6/7] ksmbd: fix invalid request buffer access in compound
2021-09-24 2:12 ` [PATCH 6/7] ksmbd: fix invalid request buffer access in compound Namjae Jeon
@ 2021-09-25 9:41 ` Hyunchul Lee
0 siblings, 0 replies; 19+ messages in thread
From: Hyunchul Lee @ 2021-09-25 9:41 UTC (permalink / raw)
To: Namjae Jeon
Cc: linux-cifs, Tom Talpey, Ronnie Sahlberg, Ralph Böhme,
Steve French, Sergey Senozhatsky
Looks good to me.
Acked-by: Hyunchul Lee <hyc.lee@gmail.com>
2021년 9월 24일 (금) 오전 11:13, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>
> Ronnie reported invalid request buffer access in chained command when
> inserting garbage value to NextCommand of compound request.
> This patch add validation check to avoid this issue.
>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Cc: Hyunchul Lee <hyc.lee@gmail.com>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
> fs/ksmbd/smb2pdu.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index a930838fd6ac..4f7b5e18a7b9 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -459,13 +459,22 @@ static void init_chained_smb2_rsp(struct ksmbd_work *work)
> bool is_chained_smb2_message(struct ksmbd_work *work)
> {
> struct smb2_hdr *hdr = work->request_buf;
> - unsigned int len;
> + unsigned int len, next_cmd;
>
> if (hdr->ProtocolId != SMB2_PROTO_NUMBER)
> return false;
>
> hdr = ksmbd_req_buf_next(work);
> - if (le32_to_cpu(hdr->NextCommand) > 0) {
> + next_cmd = le32_to_cpu(hdr->NextCommand);
> + if (next_cmd > 0) {
> + if ((u64)work->next_smb2_rcv_hdr_off + next_cmd +
> + __SMB2_HEADER_STRUCTURE_SIZE >
> + get_rfc1002_len(work->request_buf)) {
> + pr_err("next command(%u) offset exceeds smb msg size\n",
> + next_cmd);
> + return false;
> + }
> +
> ksmbd_debug(SMB, "got SMB2 chained command\n");
> init_chained_smb2_rsp(work);
> return true;
> --
> 2.25.1
>
--
Thanks,
Hyunchul
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 7/7] ksmbd: add validation in smb2 negotiate
2021-09-24 2:12 [PATCH 0/7] a bunch of patches that have not yet been reviewed Namjae Jeon
` (5 preceding siblings ...)
2021-09-24 2:12 ` [PATCH 6/7] ksmbd: fix invalid request buffer access in compound Namjae Jeon
@ 2021-09-24 2:12 ` Namjae Jeon
2021-09-24 4:58 ` Namjae Jeon
6 siblings, 1 reply; 19+ messages in thread
From: Namjae Jeon @ 2021-09-24 2:12 UTC (permalink / raw)
To: linux-cifs
Cc: Namjae Jeon, Tom Talpey, Ronnie Sahlberg, Ralph Böhme,
Steve French, Hyunchul Lee, Sergey Senozhatsky
This patch add validation to check request buffer check in smb2
negotiate.
Cc: Tom Talpey <tom@talpey.com>
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Cc: Hyunchul Lee <hyc.lee@gmail.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
fs/ksmbd/smb2pdu.c | 39 +++++++++++++++++++++++++++++++++++++++
fs/ksmbd/smb_common.c | 22 ++++++++++++++++++++--
2 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 4f7b5e18a7b9..9f45779c5f95 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -1081,6 +1081,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work)
struct smb2_negotiate_req *req = work->request_buf;
struct smb2_negotiate_rsp *rsp = work->response_buf;
int rc = 0;
+ unsigned int smb2_buf_len, smb2_neg_size;
__le32 status;
ksmbd_debug(SMB, "Received negotiate request\n");
@@ -1098,6 +1099,44 @@ int smb2_handle_negotiate(struct ksmbd_work *work)
goto err_out;
}
+ smb2_buf_len = get_rfc1002_len(work->request_buf);
+ smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects) - 4;
+ if (conn->dialect == SMB311_PROT_ID) {
+ unsigned int nego_ctxt_off = le32_to_cpu(req->NegotiateContextOffset);
+
+ if (smb2_buf_len < nego_ctxt_off) {
+ rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+ rc = -EINVAL;
+ goto err_out;
+ }
+
+ if (smb2_neg_size > nego_ctxt_off) {
+ rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+ rc = -EINVAL;
+ goto err_out;
+ }
+
+ if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) >
+ nego_ctxt_off) {
+ rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+ rc = -EINVAL;
+ goto err_out;
+ }
+ } else {
+ if (smb2_neg_size > smb2_buf_len) {
+ rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+ rc = -EINVAL;
+ goto err_out;
+ }
+
+ if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) >
+ smb2_buf_len) {
+ rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+ rc = -EINVAL;
+ goto err_out;
+ }
+ }
+
conn->cli_cap = le32_to_cpu(req->Capabilities);
switch (conn->dialect) {
case SMB311_PROT_ID:
diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
index 20bd5b8e3c0a..85937d497dcb 100644
--- a/fs/ksmbd/smb_common.c
+++ b/fs/ksmbd/smb_common.c
@@ -234,13 +234,22 @@ int ksmbd_lookup_dialect_by_id(__le16 *cli_dialects, __le16 dialects_count)
static int ksmbd_negotiate_smb_dialect(void *buf)
{
- __le32 proto;
+ int smb_buf_length = get_rfc1002_len(buf);
+ __le32 proto = ((struct smb2_hdr *)buf)->ProtocolId;
- proto = ((struct smb2_hdr *)buf)->ProtocolId;
if (proto == SMB2_PROTO_NUMBER) {
struct smb2_negotiate_req *req;
+ int smb2_neg_size =
+ offsetof(struct smb2_negotiate_req, Dialects) - 4;
req = (struct smb2_negotiate_req *)buf;
+ if (smb2_neg_size > smb_buf_length)
+ goto err_out;
+
+ if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) >
+ smb_buf_length)
+ goto err_out;
+
return ksmbd_lookup_dialect_by_id(req->Dialects,
req->DialectCount);
}
@@ -250,10 +259,19 @@ static int ksmbd_negotiate_smb_dialect(void *buf)
struct smb_negotiate_req *req;
req = (struct smb_negotiate_req *)buf;
+ if (le16_to_cpu(req->ByteCount) < 2)
+ goto err_out;
+
+ if (offsetof(struct smb_negotiate_req, DialectsArray) - 4 +
+ le16_to_cpu(req->ByteCount) > smb_buf_length) {
+ goto err_out;
+ }
+
return ksmbd_lookup_dialect_by_name(req->DialectsArray,
req->ByteCount);
}
+err_out:
return BAD_PROT_ID;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 7/7] ksmbd: add validation in smb2 negotiate
2021-09-24 2:12 ` [PATCH 7/7] ksmbd: add validation in smb2 negotiate Namjae Jeon
@ 2021-09-24 4:58 ` Namjae Jeon
0 siblings, 0 replies; 19+ messages in thread
From: Namjae Jeon @ 2021-09-24 4:58 UTC (permalink / raw)
To: linux-cifs
Cc: Namjae Jeon, Tom Talpey, Ronnie Sahlberg, Ralph Böhme,
Steve French, Hyunchul Lee, Sergey Senozhatsky
I found null dereferencing kenel oops while testing and updated this patch.
Please review the below patch, thanks!
From 85e1ce56bb0c37e8ea2083cd0e62e160f2bf6677 Mon Sep 17 00:00:00 2001
From: Namjae Jeon <linkinjeon@kernel.org>
Date: Fri, 24 Sep 2021 13:07:17 +0900
Subject: [PATCH] ksmbd: add validation in smb2 negotiate
This patch add validation to check request buffer check in smb2
negotiate and fix null pointer dereferencing oops in smb3_preauth_hash_rsp()
that found from manual test.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
fs/ksmbd/smb2pdu.c | 42 +++++++++++++++++++++++++++++++++++++++++-
fs/ksmbd/smb_common.c | 22 ++++++++++++++++++++--
2 files changed, 61 insertions(+), 3 deletions(-)
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 4c7e7aaba525..9eb8b86ddb27 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -1081,6 +1081,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work)
struct smb2_negotiate_req *req = work->request_buf;
struct smb2_negotiate_rsp *rsp = work->response_buf;
int rc = 0;
+ unsigned int smb2_buf_len, smb2_neg_size;
__le32 status;
ksmbd_debug(SMB, "Received negotiate request\n");
@@ -1098,6 +1099,44 @@ int smb2_handle_negotiate(struct ksmbd_work *work)
goto err_out;
}
+ smb2_buf_len = get_rfc1002_len(work->request_buf);
+ smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects) - 4;
+ if (conn->dialect == SMB311_PROT_ID) {
+ unsigned int nego_ctxt_off = le32_to_cpu(req->NegotiateContextOffset);
+
+ if (smb2_buf_len < nego_ctxt_off) {
+ rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+ rc = -EINVAL;
+ goto err_out;
+ }
+
+ if (smb2_neg_size > nego_ctxt_off) {
+ rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+ rc = -EINVAL;
+ goto err_out;
+ }
+
+ if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) >
+ nego_ctxt_off) {
+ rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+ rc = -EINVAL;
+ goto err_out;
+ }
+ } else {
+ if (smb2_neg_size > smb2_buf_len) {
+ rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+ rc = -EINVAL;
+ goto err_out;
+ }
+
+ if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) >
+ smb2_buf_len) {
+ rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+ rc = -EINVAL;
+ goto err_out;
+ }
+ }
+
conn->cli_cap = le32_to_cpu(req->Capabilities);
switch (conn->dialect) {
case SMB311_PROT_ID:
@@ -8335,7 +8374,8 @@ void smb3_preauth_hash_rsp(struct ksmbd_work *work)
WORK_BUFFERS(work, req, rsp);
- if (le16_to_cpu(req->Command) == SMB2_NEGOTIATE_HE)
+ if (le16_to_cpu(req->Command) == SMB2_NEGOTIATE_HE &&
+ conn->preauth_info)
ksmbd_gen_preauth_integrity_hash(conn, (char *)rsp,
conn->preauth_info->Preauth_HashValue);
diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
index 20bd5b8e3c0a..85937d497dcb 100644
--- a/fs/ksmbd/smb_common.c
+++ b/fs/ksmbd/smb_common.c
@@ -234,13 +234,22 @@ int ksmbd_lookup_dialect_by_id(__le16
*cli_dialects, __le16 dialects_count)
static int ksmbd_negotiate_smb_dialect(void *buf)
{
- __le32 proto;
+ int smb_buf_length = get_rfc1002_len(buf);
+ __le32 proto = ((struct smb2_hdr *)buf)->ProtocolId;
- proto = ((struct smb2_hdr *)buf)->ProtocolId;
if (proto == SMB2_PROTO_NUMBER) {
struct smb2_negotiate_req *req;
+ int smb2_neg_size =
+ offsetof(struct smb2_negotiate_req, Dialects) - 4;
req = (struct smb2_negotiate_req *)buf;
+ if (smb2_neg_size > smb_buf_length)
+ goto err_out;
+
+ if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) >
+ smb_buf_length)
+ goto err_out;
+
return ksmbd_lookup_dialect_by_id(req->Dialects,
req->DialectCount);
}
@@ -250,10 +259,19 @@ static int ksmbd_negotiate_smb_dialect(void *buf)
struct smb_negotiate_req *req;
req = (struct smb_negotiate_req *)buf;
+ if (le16_to_cpu(req->ByteCount) < 2)
+ goto err_out;
+
+ if (offsetof(struct smb_negotiate_req, DialectsArray) - 4 +
+ le16_to_cpu(req->ByteCount) > smb_buf_length) {
+ goto err_out;
+ }
+
return ksmbd_lookup_dialect_by_name(req->DialectsArray,
req->ByteCount);
}
+err_out:
return BAD_PROT_ID;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread