2021-09-22 23:17 GMT+09:00, Ralph Boehme <slow@xxxxxxxxx>:
> Hi Namjae
>
> patch looks great! Few nitpicks below.
>
> Am 22.09.21 um 00:51 schrieb Namjae Jeon:
>> This patch add validation to check request buffer check in smb2
>> negotiate.
>>
>> Cc: Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx>
>> Cc: Ralph Böhme <slow@xxxxxxxxx>
>> Cc: Steve French <smfrench@xxxxxxxxx>
>> Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx>
>> ---
>> fs/ksmbd/smb2pdu.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>> fs/ksmbd/smb_common.c | 22 ++++++++++++++++++++--
>> 2 files changed, 60 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index baf7ce31d557..1fe37ad4e5bc 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -1071,7 +1071,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work)
>> struct ksmbd_conn *conn = work->conn;
>> struct smb2_negotiate_req *req = work->request_buf;
>> struct smb2_negotiate_rsp *rsp = work->response_buf;
>> - int rc = 0;
>> + int rc = 0, smb2_buf_len, smb2_neg_size;
>
> I guess all len variables should use unsigned types to facilitate well
> defined overflow checks.
As Ronnie pointed out, if checking max stream size, will be no problem.
I'll fix it though.
>
>> __le32 status;
>>
>> ksmbd_debug(SMB, "Received negotiate request\n");
>> @@ -1089,6 +1089,45 @@ 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) {
>> + int nego_ctxt_off = le32_to_cpu(req->NegotiateContextOffset);
>> + int nego_ctxt_count = le16_to_cpu(req->NegotiateContextCount);
>> +
>> + if (smb2_buf_len < nego_ctxt_off + nego_ctxt_count) {
>
> overflow check needed for 32 bit arch?
Okay, will fix it on v3.
Thanks!
>
>> + 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 1da67217698d..da17b21ac685 100644
>> --- a/fs/ksmbd/smb_common.c
>> +++ b/fs/ksmbd/smb_common.c
>> @@ -229,13 +229,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);
>
> unsigned
>
> Thanks!
> -slow
>
> --
> Ralph Boehme, Samba Team https://samba.org/
> SerNet Samba Team Lead https://sernet.de/en/team-samba
>
>
