* [PATCH v3] ksmbd: add buffer validation for SMB2_CREATE_CONTEXT
@ 2021-09-22 1:26 Hyunchul Lee
2021-09-22 13:08 ` Ralph Boehme
0 siblings, 1 reply; 5+ messages in thread
From: Hyunchul Lee @ 2021-09-22 1:26 UTC (permalink / raw)
To: linux-cifs
Cc: Hyunchul Lee, Ronnie Sahlberg, Ralph Boehme, Steve French, Namjae Jeon
Add buffer validation for SMB2_CREATE_CONTEXT.
Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Boehme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
Changes from v2:
- check struct create_context's fields more in smb2_find_context_vals
(suggested by Ralph Boehme).
fs/ksmbd/oplock.c | 34 ++++++++++++++++++++++++----------
fs/ksmbd/smb2pdu.c | 25 ++++++++++++++++++++++++-
fs/ksmbd/smbacl.c | 9 ++++++++-
3 files changed, 56 insertions(+), 12 deletions(-)
diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c
index 16b6236d1bd2..8f743913b1cf 100644
--- a/fs/ksmbd/oplock.c
+++ b/fs/ksmbd/oplock.c
@@ -1451,26 +1451,40 @@ struct lease_ctx_info *parse_lease_state(void *open_req)
*/
struct create_context *smb2_find_context_vals(void *open_req, const char *tag)
{
- char *data_offset;
+ struct smb2_create_req *req = (struct smb2_create_req *)open_req;
struct create_context *cc;
- unsigned int next = 0;
+ char *data_offset, *data_end;
char *name;
- struct smb2_create_req *req = (struct smb2_create_req *)open_req;
+ unsigned int next = 0;
+ unsigned int name_off, name_len, value_off, value_len;
data_offset = (char *)req + 4 + le32_to_cpu(req->CreateContextsOffset);
+ data_end = data_offset + le32_to_cpu(req->CreateContextsLength);
cc = (struct create_context *)data_offset;
do {
- int val;
-
cc = (struct create_context *)((char *)cc + next);
- name = le16_to_cpu(cc->NameOffset) + (char *)cc;
- val = le16_to_cpu(cc->NameLength);
- if (val < 4)
+ if ((char *)cc + offsetof(struct create_context, Buffer) >
+ data_end)
return ERR_PTR(-EINVAL);
- if (memcmp(name, tag, val) == 0)
- return cc;
next = le32_to_cpu(cc->Next);
+ name_off = le16_to_cpu(cc->NameOffset);
+ name_len = le16_to_cpu(cc->NameLength);
+ value_off = le16_to_cpu(cc->DataOffset);
+ value_len = le32_to_cpu(cc->DataLength);
+
+ if ((next & 0x7) != 0 ||
+ name_off != 16 ||
+ name_len < 4 ||
+ (char *)cc + name_off + name_len > data_end ||
+ (value_off & 0x7) != 0 ||
+ (value_off && value_off < name_off + name_len) ||
+ (char *)cc + value_off + value_len > data_end)
+ return ERR_PTR(-EINVAL);
+
+ name = (char *)cc + name_off;
+ if (memcmp(name, tag, name_len) == 0)
+ return cc;
} while (next != 0);
return NULL;
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index c86164dc70bb..976490bfd93c 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -2377,6 +2377,10 @@ static int smb2_create_sd_buffer(struct ksmbd_work *work,
ksmbd_debug(SMB,
"Set ACLs using SMB2_CREATE_SD_BUFFER context\n");
sd_buf = (struct create_sd_buf_req *)context;
+ if (le16_to_cpu(context->DataOffset) +
+ le32_to_cpu(context->DataLength) <
+ sizeof(struct create_sd_buf_req))
+ return -EINVAL;
return set_info_sec(work->conn, work->tcon, path, &sd_buf->ntsd,
le32_to_cpu(sd_buf->ccontext.DataLength), true);
}
@@ -2577,6 +2581,12 @@ int smb2_open(struct ksmbd_work *work)
goto err_out1;
} else if (context) {
ea_buf = (struct create_ea_buf_req *)context;
+ if (le16_to_cpu(context->DataOffset) +
+ le32_to_cpu(context->DataLength) <
+ sizeof(struct create_ea_buf_req)) {
+ rc = -EINVAL;
+ goto err_out1;
+ }
if (req->CreateOptions & FILE_NO_EA_KNOWLEDGE_LE) {
rsp->hdr.Status = STATUS_ACCESS_DENIED;
rc = -EACCES;
@@ -2615,6 +2625,12 @@ int smb2_open(struct ksmbd_work *work)
} else if (context) {
struct create_posix *posix =
(struct create_posix *)context;
+ if (le16_to_cpu(context->DataOffset) +
+ le32_to_cpu(context->DataLength) <
+ sizeof(struct create_posix)) {
+ rc = -EINVAL;
+ goto err_out1;
+ }
ksmbd_debug(SMB, "get posix context\n");
posix_mode = le32_to_cpu(posix->Mode);
@@ -3019,9 +3035,16 @@ int smb2_open(struct ksmbd_work *work)
rc = PTR_ERR(az_req);
goto err_out;
} else if (az_req) {
- loff_t alloc_size = le64_to_cpu(az_req->AllocationSize);
+ loff_t alloc_size;
int err;
+ if (le16_to_cpu(az_req->ccontext.DataOffset) +
+ le32_to_cpu(az_req->ccontext.DataLength) <
+ sizeof(struct create_alloc_size_req)) {
+ rc = -EINVAL;
+ goto err_out;
+ }
+ alloc_size = le64_to_cpu(az_req->AllocationSize);
ksmbd_debug(SMB,
"request smb2 create allocate size : %llu\n",
alloc_size);
diff --git a/fs/ksmbd/smbacl.c b/fs/ksmbd/smbacl.c
index 0a95cdec8c80..f67567e1e178 100644
--- a/fs/ksmbd/smbacl.c
+++ b/fs/ksmbd/smbacl.c
@@ -392,7 +392,7 @@ static void parse_dacl(struct user_namespace *user_ns,
return;
/* validate that we do not go past end of acl */
- if (end_of_acl <= (char *)pdacl ||
+ if (end_of_acl < (char *)pdacl + sizeof(struct smb_acl) ||
end_of_acl < (char *)pdacl + le16_to_cpu(pdacl->size)) {
pr_err("ACL too small to parse DACL\n");
return;
@@ -434,6 +434,10 @@ static void parse_dacl(struct user_namespace *user_ns,
ppace[i] = (struct smb_ace *)(acl_base + acl_size);
acl_base = (char *)ppace[i];
acl_size = le16_to_cpu(ppace[i]->size);
+
+ if (acl_base + acl_size > end_of_acl)
+ break;
+
ppace[i]->access_req =
smb_map_generic_desired_access(ppace[i]->access_req);
@@ -807,6 +811,9 @@ int parse_sec_desc(struct user_namespace *user_ns, struct smb_ntsd *pntsd,
if (!pntsd)
return -EIO;
+ if (acl_len < sizeof(struct smb_ntsd))
+ return -EINVAL;
+
owner_sid_ptr = (struct smb_sid *)((char *)pntsd +
le32_to_cpu(pntsd->osidoffset));
group_sid_ptr = (struct smb_sid *)((char *)pntsd +
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3] ksmbd: add buffer validation for SMB2_CREATE_CONTEXT
2021-09-22 1:26 [PATCH v3] ksmbd: add buffer validation for SMB2_CREATE_CONTEXT Hyunchul Lee
@ 2021-09-22 13:08 ` Ralph Boehme
2021-09-22 18:44 ` Tom Talpey
2021-09-22 21:39 ` ronnie sahlberg
0 siblings, 2 replies; 5+ messages in thread
From: Ralph Boehme @ 2021-09-22 13:08 UTC (permalink / raw)
To: Hyunchul Lee, linux-cifs; +Cc: Ronnie Sahlberg, Steve French, Namjae Jeon
[-- Attachment #1.1: Type: text/plain, Size: 6821 bytes --]
Hi Hyunchul,
patch looks excellent, few more nitpicks below.
Am 22.09.21 um 03:26 schrieb Hyunchul Lee:
> Add buffer validation for SMB2_CREATE_CONTEXT.
>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Boehme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
> Changes from v2:
> - check struct create_context's fields more in smb2_find_context_vals
> (suggested by Ralph Boehme).
>
> fs/ksmbd/oplock.c | 34 ++++++++++++++++++++++++----------
> fs/ksmbd/smb2pdu.c | 25 ++++++++++++++++++++++++-
> fs/ksmbd/smbacl.c | 9 ++++++++-
> 3 files changed, 56 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c
> index 16b6236d1bd2..8f743913b1cf 100644
> --- a/fs/ksmbd/oplock.c
> +++ b/fs/ksmbd/oplock.c
> @@ -1451,26 +1451,40 @@ struct lease_ctx_info *parse_lease_state(void *open_req)
> */
> struct create_context *smb2_find_context_vals(void *open_req, const char *tag)
> {
> - char *data_offset;
> + struct smb2_create_req *req = (struct smb2_create_req *)open_req;
> struct create_context *cc;
> - unsigned int next = 0;
> + char *data_offset, *data_end;
> char *name;
> - struct smb2_create_req *req = (struct smb2_create_req *)open_req;
this line is only moved, not changed. Can we remove this change from the
diff?
> + unsigned int next = 0;
> + unsigned int name_off, name_len, value_off, value_len;
>
> data_offset = (char *)req + 4 + le32_to_cpu(req->CreateContextsOffset);
> + data_end = data_offset + le32_to_cpu(req->CreateContextsLength);
do we need overflow checks here? At least on 32-bit arch this could
easily overflow.
> cc = (struct create_context *)data_offset;
> do {
> - int val;
> -
> cc = (struct create_context *)((char *)cc + next);
> - name = le16_to_cpu(cc->NameOffset) + (char *)cc;
> - val = le16_to_cpu(cc->NameLength);
> - if (val < 4)
> + if ((char *)cc + offsetof(struct create_context, Buffer) >
> + data_end)
> return ERR_PTR(-EINVAL);
>
> - if (memcmp(name, tag, val) == 0)
> - return cc;
> next = le32_to_cpu(cc->Next);
> + name_off = le16_to_cpu(cc->NameOffset);
> + name_len = le16_to_cpu(cc->NameLength);
> + value_off = le16_to_cpu(cc->DataOffset);
> + value_len = le32_to_cpu(cc->DataLength);
same here: possible overflow checks needed?
> +
> + if ((next & 0x7) != 0 ||
> + name_off != 16 ||
> + name_len < 4 ||
> + (char *)cc + name_off + name_len > data_end ||
> + (value_off & 0x7) != 0 ||
> + (value_off && value_off < name_off + name_len) ||
I guess this must be
(value_off && (value_off < name_off + name_len)) ||
> + (char *)cc + value_off + value_len > data_end)
> + return ERR_PTR(-EINVAL);
> +
> + name = (char *)cc + name_off;
> + if (memcmp(name, tag, name_len) == 0)
> + return cc;
> } while (next != 0);
>
> return NULL;
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index c86164dc70bb..976490bfd93c 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -2377,6 +2377,10 @@ static int smb2_create_sd_buffer(struct ksmbd_work *work,
> ksmbd_debug(SMB,
> "Set ACLs using SMB2_CREATE_SD_BUFFER context\n");
> sd_buf = (struct create_sd_buf_req *)context;
> + if (le16_to_cpu(context->DataOffset) +
> + le32_to_cpu(context->DataLength) <
> + sizeof(struct create_sd_buf_req))
> + return -EINVAL;
> return set_info_sec(work->conn, work->tcon, path, &sd_buf->ntsd,
> le32_to_cpu(sd_buf->ccontext.DataLength), true);
> }
> @@ -2577,6 +2581,12 @@ int smb2_open(struct ksmbd_work *work)
> goto err_out1;
> } else if (context) {
> ea_buf = (struct create_ea_buf_req *)context;
> + if (le16_to_cpu(context->DataOffset) +
> + le32_to_cpu(context->DataLength) <
> + sizeof(struct create_ea_buf_req)) {
> + rc = -EINVAL;
> + goto err_out1;
> + }
> if (req->CreateOptions & FILE_NO_EA_KNOWLEDGE_LE) {
> rsp->hdr.Status = STATUS_ACCESS_DENIED;
> rc = -EACCES;
> @@ -2615,6 +2625,12 @@ int smb2_open(struct ksmbd_work *work)
> } else if (context) {
> struct create_posix *posix =
> (struct create_posix *)context;
> + if (le16_to_cpu(context->DataOffset) +
> + le32_to_cpu(context->DataLength) <
> + sizeof(struct create_posix)) {
> + rc = -EINVAL;
> + goto err_out1;
> + }
> ksmbd_debug(SMB, "get posix context\n");
>
> posix_mode = le32_to_cpu(posix->Mode);
> @@ -3019,9 +3035,16 @@ int smb2_open(struct ksmbd_work *work)
> rc = PTR_ERR(az_req);
> goto err_out;
> } else if (az_req) {
> - loff_t alloc_size = le64_to_cpu(az_req->AllocationSize);
> + loff_t alloc_size;
> int err;
>
> + if (le16_to_cpu(az_req->ccontext.DataOffset) +
> + le32_to_cpu(az_req->ccontext.DataLength) <
> + sizeof(struct create_alloc_size_req)) {
> + rc = -EINVAL;
> + goto err_out;
> + }
> + alloc_size = le64_to_cpu(az_req->AllocationSize);
> ksmbd_debug(SMB,
> "request smb2 create allocate size : %llu\n",
> alloc_size);
> diff --git a/fs/ksmbd/smbacl.c b/fs/ksmbd/smbacl.c
> index 0a95cdec8c80..f67567e1e178 100644
> --- a/fs/ksmbd/smbacl.c
> +++ b/fs/ksmbd/smbacl.c
> @@ -392,7 +392,7 @@ static void parse_dacl(struct user_namespace *user_ns,
> return;
>
> /* validate that we do not go past end of acl */
> - if (end_of_acl <= (char *)pdacl ||
> + if (end_of_acl < (char *)pdacl + sizeof(struct smb_acl) ||
> end_of_acl < (char *)pdacl + le16_to_cpu(pdacl->size)) {
> pr_err("ACL too small to parse DACL\n");
> return;
> @@ -434,6 +434,10 @@ static void parse_dacl(struct user_namespace *user_ns,
> ppace[i] = (struct smb_ace *)(acl_base + acl_size);
> acl_base = (char *)ppace[i];
> acl_size = le16_to_cpu(ppace[i]->size);
overflow check needed?
> +
> + if (acl_base + acl_size > end_of_acl)
> + break;
> +
> ppace[i]->access_req =
> smb_map_generic_desired_access(ppace[i]->access_req);
>
> @@ -807,6 +811,9 @@ int parse_sec_desc(struct user_namespace *user_ns, struct smb_ntsd *pntsd,
> if (!pntsd)
> return -EIO;
>
> + if (acl_len < sizeof(struct smb_ntsd))
> + return -EINVAL;
> +
> owner_sid_ptr = (struct smb_sid *)((char *)pntsd +
> le32_to_cpu(pntsd->osidoffset));
> group_sid_ptr = (struct smb_sid *)((char *)pntsd +
>
Thanks!
-slow
--
Ralph Boehme, Samba Team https://samba.org/
SerNet Samba Team Lead https://sernet.de/en/team-samba
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] ksmbd: add buffer validation for SMB2_CREATE_CONTEXT
2021-09-22 13:08 ` Ralph Boehme
@ 2021-09-22 18:44 ` Tom Talpey
2021-09-22 21:39 ` ronnie sahlberg
1 sibling, 0 replies; 5+ messages in thread
From: Tom Talpey @ 2021-09-22 18:44 UTC (permalink / raw)
To: Ralph Boehme, Hyunchul Lee, linux-cifs
Cc: Ronnie Sahlberg, Steve French, Namjae Jeon
Ralph's overflow check comments are spot-on, and should
be addressed even though he adds "?" to them. :)
Tom.
On 9/22/2021 9:08 AM, Ralph Boehme wrote:
> Hi Hyunchul,
>
> patch looks excellent, few more nitpicks below.
>
> Am 22.09.21 um 03:26 schrieb Hyunchul Lee:
>> Add buffer validation for SMB2_CREATE_CONTEXT.
>>
>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> Cc: Ralph Boehme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>> Changes from v2:
>> - check struct create_context's fields more in smb2_find_context_vals
>> (suggested by Ralph Boehme).
>>
>> fs/ksmbd/oplock.c | 34 ++++++++++++++++++++++++----------
>> fs/ksmbd/smb2pdu.c | 25 ++++++++++++++++++++++++-
>> fs/ksmbd/smbacl.c | 9 ++++++++-
>> 3 files changed, 56 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c
>> index 16b6236d1bd2..8f743913b1cf 100644
>> --- a/fs/ksmbd/oplock.c
>> +++ b/fs/ksmbd/oplock.c
>> @@ -1451,26 +1451,40 @@ struct lease_ctx_info *parse_lease_state(void
>> *open_req)
>> */
>> struct create_context *smb2_find_context_vals(void *open_req, const
>> char *tag)
>> {
>> - char *data_offset;
>> + struct smb2_create_req *req = (struct smb2_create_req *)open_req;
>> struct create_context *cc;
>> - unsigned int next = 0;
>> + char *data_offset, *data_end;
>> char *name;
>> - struct smb2_create_req *req = (struct smb2_create_req *)open_req;
>
> this line is only moved, not changed. Can we remove this change from the
> diff?
>
>> + unsigned int next = 0;
>> + unsigned int name_off, name_len, value_off, value_len;
>> data_offset = (char *)req + 4 +
>> le32_to_cpu(req->CreateContextsOffset);
>> + data_end = data_offset + le32_to_cpu(req->CreateContextsLength);
>
> do we need overflow checks here? At least on 32-bit arch this could
> easily overflow.
>
>> cc = (struct create_context *)data_offset;
>> do {
>> - int val;
>> -
>> cc = (struct create_context *)((char *)cc + next);
>> - name = le16_to_cpu(cc->NameOffset) + (char *)cc;
>> - val = le16_to_cpu(cc->NameLength);
>> - if (val < 4)
>> + if ((char *)cc + offsetof(struct create_context, Buffer) >
>> + data_end)
>> return ERR_PTR(-EINVAL);
>> - if (memcmp(name, tag, val) == 0)
>> - return cc;
>> next = le32_to_cpu(cc->Next);
>> + name_off = le16_to_cpu(cc->NameOffset);
>> + name_len = le16_to_cpu(cc->NameLength);
>> + value_off = le16_to_cpu(cc->DataOffset);
>> + value_len = le32_to_cpu(cc->DataLength);
>
> same here: possible overflow checks needed?
>
>> +
>> + if ((next & 0x7) != 0 ||
>> + name_off != 16 ||
>> + name_len < 4 ||
>> + (char *)cc + name_off + name_len > data_end ||
>> + (value_off & 0x7) != 0 ||
>> + (value_off && value_off < name_off + name_len) ||
>
> I guess this must be
>
> (value_off && (value_off < name_off + name_len)) ||
>
>> + (char *)cc + value_off + value_len > data_end)
>> + return ERR_PTR(-EINVAL);
>> +
>> + name = (char *)cc + name_off;
>> + if (memcmp(name, tag, name_len) == 0)
>> + return cc;
>> } while (next != 0);
>> return NULL;
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index c86164dc70bb..976490bfd93c 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -2377,6 +2377,10 @@ static int smb2_create_sd_buffer(struct
>> ksmbd_work *work,
>> ksmbd_debug(SMB,
>> "Set ACLs using SMB2_CREATE_SD_BUFFER context\n");
>> sd_buf = (struct create_sd_buf_req *)context;
>> + if (le16_to_cpu(context->DataOffset) +
>> + le32_to_cpu(context->DataLength) <
>> + sizeof(struct create_sd_buf_req))
>> + return -EINVAL;
>> return set_info_sec(work->conn, work->tcon, path, &sd_buf->ntsd,
>> le32_to_cpu(sd_buf->ccontext.DataLength), true);
>> }
>> @@ -2577,6 +2581,12 @@ int smb2_open(struct ksmbd_work *work)
>> goto err_out1;
>> } else if (context) {
>> ea_buf = (struct create_ea_buf_req *)context;
>> + if (le16_to_cpu(context->DataOffset) +
>> + le32_to_cpu(context->DataLength) <
>> + sizeof(struct create_ea_buf_req)) {
>> + rc = -EINVAL;
>> + goto err_out1;
>> + }
>> if (req->CreateOptions & FILE_NO_EA_KNOWLEDGE_LE) {
>> rsp->hdr.Status = STATUS_ACCESS_DENIED;
>> rc = -EACCES;
>> @@ -2615,6 +2625,12 @@ int smb2_open(struct ksmbd_work *work)
>> } else if (context) {
>> struct create_posix *posix =
>> (struct create_posix *)context;
>> + if (le16_to_cpu(context->DataOffset) +
>> + le32_to_cpu(context->DataLength) <
>> + sizeof(struct create_posix)) {
>> + rc = -EINVAL;
>> + goto err_out1;
>> + }
>> ksmbd_debug(SMB, "get posix context\n");
>> posix_mode = le32_to_cpu(posix->Mode);
>> @@ -3019,9 +3035,16 @@ int smb2_open(struct ksmbd_work *work)
>> rc = PTR_ERR(az_req);
>> goto err_out;
>> } else if (az_req) {
>> - loff_t alloc_size = le64_to_cpu(az_req->AllocationSize);
>> + loff_t alloc_size;
>> int err;
>> + if (le16_to_cpu(az_req->ccontext.DataOffset) +
>> + le32_to_cpu(az_req->ccontext.DataLength) <
>> + sizeof(struct create_alloc_size_req)) {
>> + rc = -EINVAL;
>> + goto err_out;
>> + }
>> + alloc_size = le64_to_cpu(az_req->AllocationSize);
>> ksmbd_debug(SMB,
>> "request smb2 create allocate size : %llu\n",
>> alloc_size);
>> diff --git a/fs/ksmbd/smbacl.c b/fs/ksmbd/smbacl.c
>> index 0a95cdec8c80..f67567e1e178 100644
>> --- a/fs/ksmbd/smbacl.c
>> +++ b/fs/ksmbd/smbacl.c
>> @@ -392,7 +392,7 @@ static void parse_dacl(struct user_namespace
>> *user_ns,
>> return;
>> /* validate that we do not go past end of acl */
>> - if (end_of_acl <= (char *)pdacl ||
>> + if (end_of_acl < (char *)pdacl + sizeof(struct smb_acl) ||
>> end_of_acl < (char *)pdacl + le16_to_cpu(pdacl->size)) {
>> pr_err("ACL too small to parse DACL\n");
>> return;
>> @@ -434,6 +434,10 @@ static void parse_dacl(struct user_namespace
>> *user_ns,
>> ppace[i] = (struct smb_ace *)(acl_base + acl_size);
>> acl_base = (char *)ppace[i];
>> acl_size = le16_to_cpu(ppace[i]->size);
>
> overflow check needed?
>
>> +
>> + if (acl_base + acl_size > end_of_acl)
>> + break;
>> +
>> ppace[i]->access_req =
>> smb_map_generic_desired_access(ppace[i]->access_req);
>> @@ -807,6 +811,9 @@ int parse_sec_desc(struct user_namespace *user_ns,
>> struct smb_ntsd *pntsd,
>> if (!pntsd)
>> return -EIO;
>> + if (acl_len < sizeof(struct smb_ntsd))
>> + return -EINVAL;
>> +
>> owner_sid_ptr = (struct smb_sid *)((char *)pntsd +
>> le32_to_cpu(pntsd->osidoffset));
>> group_sid_ptr = (struct smb_sid *)((char *)pntsd +
>>
>
> Thanks!
> -slow
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] ksmbd: add buffer validation for SMB2_CREATE_CONTEXT
2021-09-22 13:08 ` Ralph Boehme
2021-09-22 18:44 ` Tom Talpey
@ 2021-09-22 21:39 ` ronnie sahlberg
2021-09-22 23:26 ` Namjae Jeon
1 sibling, 1 reply; 5+ messages in thread
From: ronnie sahlberg @ 2021-09-22 21:39 UTC (permalink / raw)
To: Ralph Boehme; +Cc: Hyunchul Lee, linux-cifs, Steve French, Namjae Jeon
On Wed, Sep 22, 2021 at 11:08 PM Ralph Boehme <slow@samba.org> wrote:
>
> Hi Hyunchul,
>
> patch looks excellent, few more nitpicks below.
>
> Am 22.09.21 um 03:26 schrieb Hyunchul Lee:
> > Add buffer validation for SMB2_CREATE_CONTEXT.
> >
> > Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> > Cc: Ralph Boehme <slow@samba.org>
> > Cc: Steve French <smfrench@gmail.com>
> > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
> > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> > ---
> > Changes from v2:
> > - check struct create_context's fields more in smb2_find_context_vals
> > (suggested by Ralph Boehme).
> >
> > fs/ksmbd/oplock.c | 34 ++++++++++++++++++++++++----------
> > fs/ksmbd/smb2pdu.c | 25 ++++++++++++++++++++++++-
> > fs/ksmbd/smbacl.c | 9 ++++++++-
> > 3 files changed, 56 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c
> > index 16b6236d1bd2..8f743913b1cf 100644
> > --- a/fs/ksmbd/oplock.c
> > +++ b/fs/ksmbd/oplock.c
> > @@ -1451,26 +1451,40 @@ struct lease_ctx_info *parse_lease_state(void *open_req)
> > */
> > struct create_context *smb2_find_context_vals(void *open_req, const char *tag)
> > {
> > - char *data_offset;
> > + struct smb2_create_req *req = (struct smb2_create_req *)open_req;
> > struct create_context *cc;
> > - unsigned int next = 0;
> > + char *data_offset, *data_end;
> > char *name;
> > - struct smb2_create_req *req = (struct smb2_create_req *)open_req;
>
> this line is only moved, not changed. Can we remove this change from the
> diff?
>
> > + unsigned int next = 0;
> > + unsigned int name_off, name_len, value_off, value_len;
> >
> > data_offset = (char *)req + 4 + le32_to_cpu(req->CreateContextsOffset);
> > + data_end = data_offset + le32_to_cpu(req->CreateContextsLength);
>
> do we need overflow checks here? At least on 32-bit arch this could
> easily overflow.
>
> > cc = (struct create_context *)data_offset;
> > do {
> > - int val;
> > -
> > cc = (struct create_context *)((char *)cc + next);
> > - name = le16_to_cpu(cc->NameOffset) + (char *)cc;
> > - val = le16_to_cpu(cc->NameLength);
> > - if (val < 4)
> > + if ((char *)cc + offsetof(struct create_context, Buffer) >
> > + data_end)
> > return ERR_PTR(-EINVAL);
> >
> > - if (memcmp(name, tag, val) == 0)
> > - return cc;
> > next = le32_to_cpu(cc->Next);
> > + name_off = le16_to_cpu(cc->NameOffset);
> > + name_len = le16_to_cpu(cc->NameLength);
> > + value_off = le16_to_cpu(cc->DataOffset);
> > + value_len = le32_to_cpu(cc->DataLength);
>
> same here: possible overflow checks needed?
>
> > +
> > + if ((next & 0x7) != 0 ||
> > + name_off != 16 ||
> > + name_len < 4 ||
> > + (char *)cc + name_off + name_len > data_end ||
> > + (value_off & 0x7) != 0 ||
> > + (value_off && value_off < name_off + name_len) ||
>
> I guess this must be
>
> (value_off && (value_off < name_off + name_len)) ||
>
> > + (char *)cc + value_off + value_len > data_end)
> > + return ERR_PTR(-EINVAL);
> > +
> > + name = (char *)cc + name_off;
> > + if (memcmp(name, tag, name_len) == 0)
> > + return cc;
> > } while (next != 0);
> >
> > return NULL;
> > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> > index c86164dc70bb..976490bfd93c 100644
> > --- a/fs/ksmbd/smb2pdu.c
> > +++ b/fs/ksmbd/smb2pdu.c
> > @@ -2377,6 +2377,10 @@ static int smb2_create_sd_buffer(struct ksmbd_work *work,
> > ksmbd_debug(SMB,
> > "Set ACLs using SMB2_CREATE_SD_BUFFER context\n");
> > sd_buf = (struct create_sd_buf_req *)context;
> > + if (le16_to_cpu(context->DataOffset) +
> > + le32_to_cpu(context->DataLength) <
> > + sizeof(struct create_sd_buf_req))
> > + return -EINVAL;
> > return set_info_sec(work->conn, work->tcon, path, &sd_buf->ntsd,
> > le32_to_cpu(sd_buf->ccontext.DataLength), true);
> > }
> > @@ -2577,6 +2581,12 @@ int smb2_open(struct ksmbd_work *work)
> > goto err_out1;
> > } else if (context) {
> > ea_buf = (struct create_ea_buf_req *)context;
> > + if (le16_to_cpu(context->DataOffset) +
> > + le32_to_cpu(context->DataLength) <
> > + sizeof(struct create_ea_buf_req)) {
> > + rc = -EINVAL;
> > + goto err_out1;
> > + }
> > if (req->CreateOptions & FILE_NO_EA_KNOWLEDGE_LE) {
> > rsp->hdr.Status = STATUS_ACCESS_DENIED;
> > rc = -EACCES;
> > @@ -2615,6 +2625,12 @@ int smb2_open(struct ksmbd_work *work)
> > } else if (context) {
> > struct create_posix *posix =
> > (struct create_posix *)context;
> > + if (le16_to_cpu(context->DataOffset) +
> > + le32_to_cpu(context->DataLength) <
> > + sizeof(struct create_posix)) {
> > + rc = -EINVAL;
> > + goto err_out1;
> > + }
> > ksmbd_debug(SMB, "get posix context\n");
> >
> > posix_mode = le32_to_cpu(posix->Mode);
> > @@ -3019,9 +3035,16 @@ int smb2_open(struct ksmbd_work *work)
> > rc = PTR_ERR(az_req);
> > goto err_out;
> > } else if (az_req) {
> > - loff_t alloc_size = le64_to_cpu(az_req->AllocationSize);
> > + loff_t alloc_size;
> > int err;
> >
> > + if (le16_to_cpu(az_req->ccontext.DataOffset) +
> > + le32_to_cpu(az_req->ccontext.DataLength) <
> > + sizeof(struct create_alloc_size_req)) {
> > + rc = -EINVAL;
> > + goto err_out;
> > + }
> > + alloc_size = le64_to_cpu(az_req->AllocationSize);
> > ksmbd_debug(SMB,
> > "request smb2 create allocate size : %llu\n",
> > alloc_size);
> > diff --git a/fs/ksmbd/smbacl.c b/fs/ksmbd/smbacl.c
> > index 0a95cdec8c80..f67567e1e178 100644
> > --- a/fs/ksmbd/smbacl.c
> > +++ b/fs/ksmbd/smbacl.c
> > @@ -392,7 +392,7 @@ static void parse_dacl(struct user_namespace *user_ns,
> > return;
> >
> > /* validate that we do not go past end of acl */
> > - if (end_of_acl <= (char *)pdacl ||
> > + if (end_of_acl < (char *)pdacl + sizeof(struct smb_acl) ||
> > end_of_acl < (char *)pdacl + le16_to_cpu(pdacl->size)) {
> > pr_err("ACL too small to parse DACL\n");
> > return;
> > @@ -434,6 +434,10 @@ static void parse_dacl(struct user_namespace *user_ns,
> > ppace[i] = (struct smb_ace *)(acl_base + acl_size);
> > acl_base = (char *)ppace[i];
> > acl_size = le16_to_cpu(ppace[i]->size);
>
> overflow check needed?
>
> > +
> > + if (acl_base + acl_size > end_of_acl)
> > + break;
> > +
> > ppace[i]->access_req =
> > smb_map_generic_desired_access(ppace[i]->access_req);
> >
> > @@ -807,6 +811,9 @@ int parse_sec_desc(struct user_namespace *user_ns, struct smb_ntsd *pntsd,
> > if (!pntsd)
> > return -EIO;
> >
> > + if (acl_len < sizeof(struct smb_ntsd))
> > + return -EINVAL;
> > +
> > owner_sid_ptr = (struct smb_sid *)((char *)pntsd +
> > le32_to_cpu(pntsd->osidoffset));
> > group_sid_ptr = (struct smb_sid *)((char *)pntsd +
> >
Agree with Ralph.
Overflow checks needs in all these places. But there are several ways
to do them.
Note that the maximum stream length is 0x00ffffff which means that
every Offset or Length must be
< 0x01000000
If you check every single Length and Offset for < 0x01000000 then
overflow can not happen.
(well, at least Offset + Length can not overflow)
>
> Thanks!
> -slow
>
> --
> Ralph Boehme, Samba Team https://samba.org/
> SerNet Samba Team Lead https://sernet.de/en/team-samba
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] ksmbd: add buffer validation for SMB2_CREATE_CONTEXT
2021-09-22 21:39 ` ronnie sahlberg
@ 2021-09-22 23:26 ` Namjae Jeon
0 siblings, 0 replies; 5+ messages in thread
From: Namjae Jeon @ 2021-09-22 23:26 UTC (permalink / raw)
To: ronnie sahlberg; +Cc: Ralph Boehme, Hyunchul Lee, linux-cifs, Steve French
2021-09-23 6:39 GMT+09:00, ronnie sahlberg <ronniesahlberg@gmail.com>:
> On Wed, Sep 22, 2021 at 11:08 PM Ralph Boehme <slow@samba.org> wrote:
>>
>> Hi Hyunchul,
>>
>> patch looks excellent, few more nitpicks below.
>>
>> Am 22.09.21 um 03:26 schrieb Hyunchul Lee:
>> > Add buffer validation for SMB2_CREATE_CONTEXT.
>> >
>> > Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> > Cc: Ralph Boehme <slow@samba.org>
>> > Cc: Steve French <smfrench@gmail.com>
>> > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
>> > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> > ---
>> > Changes from v2:
>> > - check struct create_context's fields more in smb2_find_context_vals
>> > (suggested by Ralph Boehme).
>> >
>> > fs/ksmbd/oplock.c | 34 ++++++++++++++++++++++++----------
>> > fs/ksmbd/smb2pdu.c | 25 ++++++++++++++++++++++++-
>> > fs/ksmbd/smbacl.c | 9 ++++++++-
>> > 3 files changed, 56 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c
>> > index 16b6236d1bd2..8f743913b1cf 100644
>> > --- a/fs/ksmbd/oplock.c
>> > +++ b/fs/ksmbd/oplock.c
>> > @@ -1451,26 +1451,40 @@ struct lease_ctx_info *parse_lease_state(void
>> > *open_req)
>> > */
>> > struct create_context *smb2_find_context_vals(void *open_req, const
>> > char *tag)
>> > {
>> > - char *data_offset;
>> > + struct smb2_create_req *req = (struct smb2_create_req *)open_req;
>> > struct create_context *cc;
>> > - unsigned int next = 0;
>> > + char *data_offset, *data_end;
>> > char *name;
>> > - struct smb2_create_req *req = (struct smb2_create_req *)open_req;
>>
>> this line is only moved, not changed. Can we remove this change from the
>> diff?
>>
>> > + unsigned int next = 0;
>> > + unsigned int name_off, name_len, value_off, value_len;
>> >
>> > data_offset = (char *)req + 4 +
>> > le32_to_cpu(req->CreateContextsOffset);
>> > + data_end = data_offset + le32_to_cpu(req->CreateContextsLength);
>>
>> do we need overflow checks here? At least on 32-bit arch this could
>> easily overflow.
>>
>> > cc = (struct create_context *)data_offset;
>> > do {
>> > - int val;
>> > -
>> > cc = (struct create_context *)((char *)cc + next);
>> > - name = le16_to_cpu(cc->NameOffset) + (char *)cc;
>> > - val = le16_to_cpu(cc->NameLength);
>> > - if (val < 4)
>> > + if ((char *)cc + offsetof(struct create_context, Buffer)
>> > >
>> > + data_end)
>> > return ERR_PTR(-EINVAL);
>> >
>> > - if (memcmp(name, tag, val) == 0)
>> > - return cc;
>> > next = le32_to_cpu(cc->Next);
>> > + name_off = le16_to_cpu(cc->NameOffset);
>> > + name_len = le16_to_cpu(cc->NameLength);
>> > + value_off = le16_to_cpu(cc->DataOffset);
>> > + value_len = le32_to_cpu(cc->DataLength);
>>
>> same here: possible overflow checks needed?
>>
>> > +
>> > + if ((next & 0x7) != 0 ||
>> > + name_off != 16 ||
>> > + name_len < 4 ||
>> > + (char *)cc + name_off + name_len > data_end ||
>> > + (value_off & 0x7) != 0 ||
>> > + (value_off && value_off < name_off + name_len) ||
>>
>> I guess this must be
>>
>> (value_off && (value_off < name_off + name_len)) ||
>>
>> > + (char *)cc + value_off + value_len > data_end)
>> > + return ERR_PTR(-EINVAL);
>> > +
>> > + name = (char *)cc + name_off;
>> > + if (memcmp(name, tag, name_len) == 0)
>> > + return cc;
>> > } while (next != 0);
>> >
>> > return NULL;
>> > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> > index c86164dc70bb..976490bfd93c 100644
>> > --- a/fs/ksmbd/smb2pdu.c
>> > +++ b/fs/ksmbd/smb2pdu.c
>> > @@ -2377,6 +2377,10 @@ static int smb2_create_sd_buffer(struct
>> > ksmbd_work *work,
>> > ksmbd_debug(SMB,
>> > "Set ACLs using SMB2_CREATE_SD_BUFFER context\n");
>> > sd_buf = (struct create_sd_buf_req *)context;
>> > + if (le16_to_cpu(context->DataOffset) +
>> > + le32_to_cpu(context->DataLength) <
>> > + sizeof(struct create_sd_buf_req))
>> > + return -EINVAL;
>> > return set_info_sec(work->conn, work->tcon, path, &sd_buf->ntsd,
>> > le32_to_cpu(sd_buf->ccontext.DataLength),
>> > true);
>> > }
>> > @@ -2577,6 +2581,12 @@ int smb2_open(struct ksmbd_work *work)
>> > goto err_out1;
>> > } else if (context) {
>> > ea_buf = (struct create_ea_buf_req *)context;
>> > + if (le16_to_cpu(context->DataOffset) +
>> > + le32_to_cpu(context->DataLength) <
>> > + sizeof(struct create_ea_buf_req)) {
>> > + rc = -EINVAL;
>> > + goto err_out1;
>> > + }
>> > if (req->CreateOptions & FILE_NO_EA_KNOWLEDGE_LE)
>> > {
>> > rsp->hdr.Status = STATUS_ACCESS_DENIED;
>> > rc = -EACCES;
>> > @@ -2615,6 +2625,12 @@ int smb2_open(struct ksmbd_work *work)
>> > } else if (context) {
>> > struct create_posix *posix =
>> > (struct create_posix *)context;
>> > + if (le16_to_cpu(context->DataOffset) +
>> > + le32_to_cpu(context->DataLength) <
>> > + sizeof(struct create_posix)) {
>> > + rc = -EINVAL;
>> > + goto err_out1;
>> > + }
>> > ksmbd_debug(SMB, "get posix context\n");
>> >
>> > posix_mode = le32_to_cpu(posix->Mode);
>> > @@ -3019,9 +3035,16 @@ int smb2_open(struct ksmbd_work *work)
>> > rc = PTR_ERR(az_req);
>> > goto err_out;
>> > } else if (az_req) {
>> > - loff_t alloc_size =
>> > le64_to_cpu(az_req->AllocationSize);
>> > + loff_t alloc_size;
>> > int err;
>> >
>> > + if (le16_to_cpu(az_req->ccontext.DataOffset) +
>> > + le32_to_cpu(az_req->ccontext.DataLength) <
>> > + sizeof(struct create_alloc_size_req)) {
>> > + rc = -EINVAL;
>> > + goto err_out;
>> > + }
>> > + alloc_size = le64_to_cpu(az_req->AllocationSize);
>> > ksmbd_debug(SMB,
>> > "request smb2 create allocate size :
>> > %llu\n",
>> > alloc_size);
>> > diff --git a/fs/ksmbd/smbacl.c b/fs/ksmbd/smbacl.c
>> > index 0a95cdec8c80..f67567e1e178 100644
>> > --- a/fs/ksmbd/smbacl.c
>> > +++ b/fs/ksmbd/smbacl.c
>> > @@ -392,7 +392,7 @@ static void parse_dacl(struct user_namespace
>> > *user_ns,
>> > return;
>> >
>> > /* validate that we do not go past end of acl */
>> > - if (end_of_acl <= (char *)pdacl ||
>> > + if (end_of_acl < (char *)pdacl + sizeof(struct smb_acl) ||
>> > end_of_acl < (char *)pdacl + le16_to_cpu(pdacl->size)) {
>> > pr_err("ACL too small to parse DACL\n");
>> > return;
>> > @@ -434,6 +434,10 @@ static void parse_dacl(struct user_namespace
>> > *user_ns,
>> > ppace[i] = (struct smb_ace *)(acl_base + acl_size);
>> > acl_base = (char *)ppace[i];
>> > acl_size = le16_to_cpu(ppace[i]->size);
>>
>> overflow check needed?
>>
>> > +
>> > + if (acl_base + acl_size > end_of_acl)
>> > + break;
>> > +
>> > ppace[i]->access_req =
>> >
>> > smb_map_generic_desired_access(ppace[i]->access_req);
>> >
>> > @@ -807,6 +811,9 @@ int parse_sec_desc(struct user_namespace *user_ns,
>> > struct smb_ntsd *pntsd,
>> > if (!pntsd)
>> > return -EIO;
>> >
>> > + if (acl_len < sizeof(struct smb_ntsd))
>> > + return -EINVAL;
>> > +
>> > owner_sid_ptr = (struct smb_sid *)((char *)pntsd +
>> > le32_to_cpu(pntsd->osidoffset));
>> > group_sid_ptr = (struct smb_sid *)((char *)pntsd +
>> >
>
> Agree with Ralph.
> Overflow checks needs in all these places. But there are several ways
> to do them.
> Note that the maximum stream length is 0x00ffffff which means that
> every Offset or Length must be
> < 0x01000000
> If you check every single Length and Offset for < 0x01000000 then
> overflow can not happen.
> (well, at least Offset + Length can not overflow)
Thanks for detailed explanation!
>
>
>>
>> Thanks!
>> -slow
>>
>> --
>> Ralph Boehme, Samba Team https://samba.org/
>> SerNet Samba Team Lead https://sernet.de/en/team-samba
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-09-22 23:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-22 1:26 [PATCH v3] ksmbd: add buffer validation for SMB2_CREATE_CONTEXT Hyunchul Lee
2021-09-22 13:08 ` Ralph Boehme
2021-09-22 18:44 ` Tom Talpey
2021-09-22 21:39 ` ronnie sahlberg
2021-09-22 23:26 ` Namjae Jeon
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.