Hi Namjae, thanks! One nitpick below. Am 19.09.21 um 04:13 schrieb Namjae Jeon:
From: Hyunchul Lee <hyc.lee@xxxxxxxxx> Add buffer validation for SMB2_CREATE_CONTEXT. Cc: Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx> Cc: Ralph Böhme <slow@xxxxxxxxx> Cc: Steve French <smfrench@xxxxxxxxx> Signed-off-by: Hyunchul Lee <hyc.lee@xxxxxxxxx> Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx> --- fs/ksmbd/oplock.c | 35 +++++++++++++++++++++++++---------- fs/ksmbd/smb2pdu.c | 25 ++++++++++++++++++++++++- fs/ksmbd/smbacl.c | 9 ++++++++- 3 files changed, 57 insertions(+), 12 deletions(-) diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c index 16b6236d1bd2..3fd2713f2282 100644 --- a/fs/ksmbd/oplock.c +++ b/fs/ksmbd/oplock.c @@ -1451,26 +1451,41 @@ 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 ((char *)cc + name_off + name_len > data_end || + (value_len && (char *)cc + value_off + value_len > data_end)) + return ERR_PTR(-EINVAL); + else if (next && (next < name_off + name_len || + (value_len && next < value_off + value_len))) + return ERR_PTR(-EINVAL);
The else is a bit confusing and not needed. Also, Samba has a few additional checks, I wonder whether we should add those two:
if ((next & 0x7) != 0 ||
next > remaining ||
name_offset != 16 ||
name_length < 4 ||
name_offset + name_length > remaining ||
(data_offset & 0x7) != 0 ||
(data_offset && (data_offset < name_offset +
name_length)) ||
(data_offset > remaining) ||
(data_offset + (uint64_t)data_length > remaining)) {
return NT_STATUS_INVALID_PARAMETER;
}
Other then that lgtm.
Thanks!
-slow
--
Ralph Boehme, Samba Team https://samba.org/
SerNet Samba Team Lead https://sernet.de/en/team-samba
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
