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 > Cc: Ralph Böhme > Cc: Steve French > Signed-off-by: Namjae Jeon > --- > 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. > __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? > + 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