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.
> >> >> __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
> >
> >
