2021-09-23 9:12 GMT+09:00, ronnie sahlberg <ronniesahlberg@xxxxxxxxx>:
> On Thu, Sep 23, 2021 at 9:13 AM Namjae Jeon <linkinjeon@xxxxxxxxxx> wrote:
>>
>> 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.
>
> You should add a check to ksmbd_conn_handler_loop() that the length is
> < 0x01000000 too.
Right, I will! Thanks!
>
>> >> >> __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
>> >
>> >
>
