[PATCH v2 1/3] ksmbd: remove RFC1002 check in smb2 request

[PATCH v2 1/3] ksmbd: remove RFC1002 check in smb2 request

[Namjae Jeon]

From: Ronnie Sahlberg <lsahlber@redhat.com>

In smb_common.c you have this function :   ksmbd_smb_request() which
is called from connection.c once you have read the initial 4 bytes for
the next length+smb2 blob.

It checks the first byte of this 4 byte preamble for valid values,
i.e. a NETBIOSoverTCP SESSION_MESSAGE or a SESSION_KEEP_ALIVE.

We don't need to check this for ksmbd since it only implements SMB2
over TCP port 445.
The netbios stuff was only used in very old servers when SMB ran over
TCP port 139.
Now that we run over TCP port 445, this is actually not a NB header anymore
and you can just treat it as a 4 byte length field that must be less
than 16Mbyte. and remove the references to the RFC1002 constants that no
longer applies.

Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/smb_common.c | 15 +--------------
 fs/ksmbd/smb_common.h |  8 --------
 2 files changed, 1 insertion(+), 22 deletions(-)

diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
index 43d3123d8b62..1da67217698d 100644
--- a/fs/ksmbd/smb_common.c
+++ b/fs/ksmbd/smb_common.c
@@ -149,20 +149,7 @@ int ksmbd_verify_smb_message(struct ksmbd_work *work)
  */
 bool ksmbd_smb_request(struct ksmbd_conn *conn)
 {
-	int type = *(char *)conn->request_buf;
-
-	switch (type) {
-	case RFC1002_SESSION_MESSAGE:
-		/* Regular SMB request */
-		return true;
-	case RFC1002_SESSION_KEEP_ALIVE:
-		ksmbd_debug(SMB, "RFC 1002 session keep alive\n");
-		break;
-	default:
-		ksmbd_debug(SMB, "RFC 1002 unknown request type 0x%x\n", type);
-	}
-
-	return false;
+	return conn->request_buf[0] == 0;
 }
 
 static bool supported_protocol(int idx)
diff --git a/fs/ksmbd/smb_common.h b/fs/ksmbd/smb_common.h
index 57c667c1be06..d7df19c97c4c 100644
--- a/fs/ksmbd/smb_common.h
+++ b/fs/ksmbd/smb_common.h
@@ -48,14 +48,6 @@
 #define CIFS_DEFAULT_IOSIZE	(64 * 1024)
 #define MAX_CIFS_SMALL_BUFFER_SIZE 448 /* big enough for most */
 
-/* RFC 1002 session packet types */
-#define RFC1002_SESSION_MESSAGE			0x00
-#define RFC1002_SESSION_REQUEST			0x81
-#define RFC1002_POSITIVE_SESSION_RESPONSE	0x82
-#define RFC1002_NEGATIVE_SESSION_RESPONSE	0x83
-#define RFC1002_RETARGET_SESSION_RESPONSE	0x84
-#define RFC1002_SESSION_KEEP_ALIVE		0x85
-
 /* Responses when opening a file. */
 #define F_SUPERSEDED	0
 #define F_OPENED	1
-- 
2.25.1

[PATCH v2 2/3] ksmbd: add validation in smb2 negotiate

[Namjae Jeon]

This patch add validation to check request buffer check in smb2
negotiate.

Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 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;
 	__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) {
+			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);
+	__le32 proto = ((struct smb2_hdr *)buf)->ProtocolId;
 
-	proto = ((struct smb2_hdr *)buf)->ProtocolId;
 	if (proto == SMB2_PROTO_NUMBER) {
 		struct smb2_negotiate_req *req;
+		int smb2_neg_size =
+			offsetof(struct smb2_negotiate_req, Dialects) - 4;
 
 		req = (struct smb2_negotiate_req *)buf;
+		if (smb2_neg_size > smb_buf_length)
+			goto err_out;
+
+		if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) >
+		    smb_buf_length)
+			goto err_out;
+
 		return ksmbd_lookup_dialect_by_id(req->Dialects,
 						  req->DialectCount);
 	}
@@ -245,10 +254,19 @@ static int ksmbd_negotiate_smb_dialect(void *buf)
 		struct smb_negotiate_req *req;
 
 		req = (struct smb_negotiate_req *)buf;
+		if (le16_to_cpu(req->ByteCount) < 2)
+			goto err_out;
+
+		if (offsetof(struct smb_negotiate_req, DialectsArray) - 4 +
+			le16_to_cpu(req->ByteCount) > smb_buf_length) {
+			goto err_out;
+		}
+
 		return ksmbd_lookup_dialect_by_name(req->DialectsArray,
 						    req->ByteCount);
 	}
 
+err_out:
 	return BAD_PROT_ID;
 }
 
-- 
2.25.1

[PATCH v2 3/3] ksmbd: fix invalid request buffer access in compound request

[Namjae Jeon]

Ronnie reported invalid request buffer access in chained command when
inserting garbage value to NextCommand of compound request.
This patch add validation check to avoid this issue.

Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: Ralph Böhme <slow@samba.org>
Cc: Steve French <smfrench@gmail.com>
Reported-by: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 v2:
  - fix integer overflow from work->next_smb2_rcv_hdr_off.

 fs/ksmbd/smb2pdu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 1fe37ad4e5bc..cae796ea1148 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -466,6 +466,13 @@ bool is_chained_smb2_message(struct ksmbd_work *work)
 
 	hdr = ksmbd_req_buf_next(work);
 	if (le32_to_cpu(hdr->NextCommand) > 0) {
+		if ((u64)work->next_smb2_rcv_hdr_off + le32_to_cpu(hdr->NextCommand) >
+		    get_rfc1002_len(work->request_buf)) {
+			pr_err("next command(%u) offset exceeds smb msg size\n",
+			       hdr->NextCommand);
+			return false;
+		}
+
 		ksmbd_debug(SMB, "got SMB2 chained command\n");
 		init_chained_smb2_rsp(work);
 		return true;
-- 
2.25.1

Re: [PATCH v2 3/3] ksmbd: fix invalid request buffer access in compound request

[ronnie sahlberg]

On Wed, Sep 22, 2021 at 8:51 AM Namjae Jeon <linkinjeon@kernel.org> wrote:
>
> Ronnie reported invalid request buffer access in chained command when
> inserting garbage value to NextCommand of compound request.
> This patch add validation check to avoid this issue.
>
> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Reported-by: Ronnie Sahlberg <lsahlber@redhat.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>  v2:
>   - fix integer overflow from work->next_smb2_rcv_hdr_off.
>
>  fs/ksmbd/smb2pdu.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 1fe37ad4e5bc..cae796ea1148 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -466,6 +466,13 @@ bool is_chained_smb2_message(struct ksmbd_work *work)
>
>         hdr = ksmbd_req_buf_next(work);
>         if (le32_to_cpu(hdr->NextCommand) > 0) {
> +               if ((u64)work->next_smb2_rcv_hdr_off + le32_to_cpu(hdr->NextCommand) >
> +                   get_rfc1002_len(work->request_buf)) {
> +                       pr_err("next command(%u) offset exceeds smb msg size\n",
> +                              hdr->NextCommand);
> +                       return false;
> +               }
> +
>                 ksmbd_debug(SMB, "got SMB2 chained command\n");
>                 init_chained_smb2_rsp(work);
>                 return true;

Very good, reviewed by me.
The conditional though, since you know there will be at least a full
smb2 header there you could already check that change it to
> +               if ((u64)work->next_smb2_rcv_hdr_off + le32_to_cpu(hdr->NextCommand) >
> +                   get_rfc1002_len(work->request_buf) +  64) {


Which leads to another question.  Where do you check that the buffer
contains enough data to hold the smb2 header and the full fixed part
of the request?
There is a check that you have enough space for the smb2 header in
ksmbd_conn_handler_loop()
that there is enough space for the smb2 header
(ksmbd_pdu_size_has_room()) but that function assumes that the smb2
header always start at the head of the buffer.
So if you have a compound chain, this functrion only checks the first pdu.


I know that the buffer handling is copied from the cifs client.  It
used to also do these "just pass a buffer around and the first 4 bytes
is the size" (and still does for smb1)  and there was a lot of
terrible +4 or -4 to all sort of casts and conditionals.
I changed that in cifs.ko to remove the 4 byte length completely from
the buffer.
I also changed it as part of the compounding to pass an array of
requests (each containing an iovector) to the functions instead of
just one large byte array.
That made things a lot easier to manage since you could then assume
that the SMB2 header would always start at offset 0 in the
corresponding iovector, even for compounded commands since they all
had their own private vector.
And since an iovector contains both a pointer and a length there is no
need anymore to read the first 4 bytes/validate them/and covnert into
a length all the time.

I think that would help, but it would be a MAJOR amount of work, so
maybe that should wait until later.
That approach is very nice since it completely avoids keeping track of
offset-to-where-this-pdu-starts which makes all checks and
conditionals so much more complex.


regards
ronnie sahlberg


> --
> 2.25.1
>

Re: [PATCH v2 3/3] ksmbd: fix invalid request buffer access in compound request

[Namjae Jeon]

2021-09-22 9:39 GMT+09:00, ronnie sahlberg <ronniesahlberg@gmail.com>:
> On Wed, Sep 22, 2021 at 8:51 AM Namjae Jeon <linkinjeon@kernel.org> wrote:
>>
>> Ronnie reported invalid request buffer access in chained command when
>> inserting garbage value to NextCommand of compound request.
>> This patch add validation check to avoid this issue.
>>
>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> Cc: Ralph Böhme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Reported-by: Ronnie Sahlberg <lsahlber@redhat.com>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>>  v2:
>>   - fix integer overflow from work->next_smb2_rcv_hdr_off.
>>
>>  fs/ksmbd/smb2pdu.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index 1fe37ad4e5bc..cae796ea1148 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -466,6 +466,13 @@ bool is_chained_smb2_message(struct ksmbd_work
>> *work)
>>
>>         hdr = ksmbd_req_buf_next(work);
>>         if (le32_to_cpu(hdr->NextCommand) > 0) {
>> +               if ((u64)work->next_smb2_rcv_hdr_off +
>> le32_to_cpu(hdr->NextCommand) >
>> +                   get_rfc1002_len(work->request_buf)) {
>> +                       pr_err("next command(%u) offset exceeds smb msg
>> size\n",
>> +                              hdr->NextCommand);
>> +                       return false;
>> +               }
>> +
>>                 ksmbd_debug(SMB, "got SMB2 chained command\n");
>>                 init_chained_smb2_rsp(work);
>>                 return true;
>
> Very good, reviewed by me.
Sorry for late response, Thanks for your review!
> The conditional though, since you know there will be at least a full
> smb2 header there you could already check that change it to
>> +               if ((u64)work->next_smb2_rcv_hdr_off +
>> le32_to_cpu(hdr->NextCommand) >
>> +                   get_rfc1002_len(work->request_buf) +  64) {
Ah, I didn't understand why we should add + 64(smb2 hdr size)...
As I know, NextCommand offset included smb2 header size..
>
>
> Which leads to another question.  Where do you check that the buffer
> contains enough data to hold the smb2 header and the full fixed part
> of the request?
ksmbd_smb2_check_message() in smb2misc.c should check it.

> There is a check that you have enough space for the smb2 header in
> ksmbd_conn_handler_loop()
> that there is enough space for the smb2 header
> (ksmbd_pdu_size_has_room()) but that function assumes that the smb2
> header always start at the head of the buffer.
> So if you have a compound chain, this functrion only checks the first pdu.
I think that is_chained_smb2_message() will check all pdu as well as first pdu.
there is loop do { } while (is_chained_smb2_message(work)); in server.c
>
>
> I know that the buffer handling is copied from the cifs client.  It
> used to also do these "just pass a buffer around and the first 4 bytes
> is the size" (and still does for smb1)  and there was a lot of
> terrible +4 or -4 to all sort of casts and conditionals.
> I changed that in cifs.ko to remove the 4 byte length completely from
> the buffer.
> I also changed it as part of the compounding to pass an array of
> requests (each containing an iovector) to the functions instead of
> just one large byte array.
> That made things a lot easier to manage since you could then assume
> that the SMB2 header would always start at offset 0 in the
> corresponding iovector, even for compounded commands since they all
> had their own private vector.
> And since an iovector contains both a pointer and a length there is no
> need anymore to read the first 4 bytes/validate them/and covnert into
> a length all the time.
Right. fully agreed.

>
> I think that would help, but it would be a MAJOR amount of work, so
> maybe that should wait until later.
Agreed, I will do that after fixing current urgent issues first!

> That approach is very nice since it completely avoids keeping track of
> offset-to-where-this-pdu-starts which makes all checks and
> conditionals so much more complex.
Thanks!
>
>
> regards
> ronnie sahlberg
>
>
>> --
>> 2.25.1
>>
>

Re: [PATCH v2 3/3] ksmbd: fix invalid request buffer access in compound request

[ronnie sahlberg]

On Wed, Sep 22, 2021 at 2:35 PM Namjae Jeon <linkinjeon@kernel.org> wrote:
>
> 2021-09-22 9:39 GMT+09:00, ronnie sahlberg <ronniesahlberg@gmail.com>:
> > On Wed, Sep 22, 2021 at 8:51 AM Namjae Jeon <linkinjeon@kernel.org> wrote:
> >>
> >> Ronnie reported invalid request buffer access in chained command when
> >> inserting garbage value to NextCommand of compound request.
> >> This patch add validation check to avoid this issue.
> >>
> >> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> >> Cc: Ralph Böhme <slow@samba.org>
> >> Cc: Steve French <smfrench@gmail.com>
> >> Reported-by: Ronnie Sahlberg <lsahlber@redhat.com>
> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> >> ---
> >>  v2:
> >>   - fix integer overflow from work->next_smb2_rcv_hdr_off.
> >>
> >>  fs/ksmbd/smb2pdu.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> >> index 1fe37ad4e5bc..cae796ea1148 100644
> >> --- a/fs/ksmbd/smb2pdu.c
> >> +++ b/fs/ksmbd/smb2pdu.c
> >> @@ -466,6 +466,13 @@ bool is_chained_smb2_message(struct ksmbd_work
> >> *work)
> >>
> >>         hdr = ksmbd_req_buf_next(work);
> >>         if (le32_to_cpu(hdr->NextCommand) > 0) {
> >> +               if ((u64)work->next_smb2_rcv_hdr_off +
> >> le32_to_cpu(hdr->NextCommand) >
> >> +                   get_rfc1002_len(work->request_buf)) {
> >> +                       pr_err("next command(%u) offset exceeds smb msg
> >> size\n",
> >> +                              hdr->NextCommand);
> >> +                       return false;
> >> +               }
> >> +
> >>                 ksmbd_debug(SMB, "got SMB2 chained command\n");
> >>                 init_chained_smb2_rsp(work);
> >>                 return true;
> >
> > Very good, reviewed by me.
> Sorry for late response, Thanks for your review!
> > The conditional though, since you know there will be at least a full
> > smb2 header there you could already check that change it to
> >> +               if ((u64)work->next_smb2_rcv_hdr_off +
> >> le32_to_cpu(hdr->NextCommand) >
> >> +                   get_rfc1002_len(work->request_buf) +  64) {
> Ah, I didn't understand why we should add + 64(smb2 hdr size)...
> As I know, NextCommand offset included smb2 header size..

This is what I meant.
+               if ((u64)work->next_smb2_rcv_hdr_off +
le32_to_cpu(hdr->NextCommand) + 64 >
+                   get_rfc1002_len(work->request_buf)) {

It could just be an early check that what hdr->NextCommand points to
has at least 64 bytes.
I.e. an early test that "does the next PDU have at least a full smb2 header?"

I mean, since you already test that NextCommand is valid,  you could
at the same time also
test that the next pdu is at least 64 bytes.

> >
> >
> > Which leads to another question.  Where do you check that the buffer
> > contains enough data to hold the smb2 header and the full fixed part
> > of the request?
> ksmbd_smb2_check_message() in smb2misc.c should check it.
>
> > There is a check that you have enough space for the smb2 header in
> > ksmbd_conn_handler_loop()
> > that there is enough space for the smb2 header
> > (ksmbd_pdu_size_has_room()) but that function assumes that the smb2
> > header always start at the head of the buffer.
> > So if you have a compound chain, this functrion only checks the first pdu.
> I think that is_chained_smb2_message() will check all pdu as well as first pdu.
> there is loop do { } while (is_chained_smb2_message(work)); in server.c
> >
> >
> > I know that the buffer handling is copied from the cifs client.  It
> > used to also do these "just pass a buffer around and the first 4 bytes
> > is the size" (and still does for smb1)  and there was a lot of
> > terrible +4 or -4 to all sort of casts and conditionals.
> > I changed that in cifs.ko to remove the 4 byte length completely from
> > the buffer.
> > I also changed it as part of the compounding to pass an array of
> > requests (each containing an iovector) to the functions instead of
> > just one large byte array.
> > That made things a lot easier to manage since you could then assume
> > that the SMB2 header would always start at offset 0 in the
> > corresponding iovector, even for compounded commands since they all
> > had their own private vector.
> > And since an iovector contains both a pointer and a length there is no
> > need anymore to read the first 4 bytes/validate them/and covnert into
> > a length all the time.
> Right. fully agreed.
>
> >
> > I think that would help, but it would be a MAJOR amount of work, so
> > maybe that should wait until later.
> Agreed, I will do that after fixing current urgent issues first!
>
> > That approach is very nice since it completely avoids keeping track of
> > offset-to-where-this-pdu-starts which makes all checks and
> > conditionals so much more complex.
> Thanks!
> >
> >
> > regards
> > ronnie sahlberg
> >
> >
> >> --
> >> 2.25.1
> >>
> >

Re: [PATCH v2 3/3] ksmbd: fix invalid request buffer access in compound request

[Namjae Jeon]

2021-09-22 13:56 GMT+09:00, ronnie sahlberg <ronniesahlberg@gmail.com>:
> On Wed, Sep 22, 2021 at 2:35 PM Namjae Jeon <linkinjeon@kernel.org> wrote:
>>
>> 2021-09-22 9:39 GMT+09:00, ronnie sahlberg <ronniesahlberg@gmail.com>:
>> > On Wed, Sep 22, 2021 at 8:51 AM Namjae Jeon <linkinjeon@kernel.org>
>> > wrote:
>> >>
>> >> Ronnie reported invalid request buffer access in chained command when
>> >> inserting garbage value to NextCommand of compound request.
>> >> This patch add validation check to avoid this issue.
>> >>
>> >> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> >> Cc: Ralph Böhme <slow@samba.org>
>> >> Cc: Steve French <smfrench@gmail.com>
>> >> Reported-by: Ronnie Sahlberg <lsahlber@redhat.com>
>> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> >> ---
>> >>  v2:
>> >>   - fix integer overflow from work->next_smb2_rcv_hdr_off.
>> >>
>> >>  fs/ksmbd/smb2pdu.c | 7 +++++++
>> >>  1 file changed, 7 insertions(+)
>> >>
>> >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> >> index 1fe37ad4e5bc..cae796ea1148 100644
>> >> --- a/fs/ksmbd/smb2pdu.c
>> >> +++ b/fs/ksmbd/smb2pdu.c
>> >> @@ -466,6 +466,13 @@ bool is_chained_smb2_message(struct ksmbd_work
>> >> *work)
>> >>
>> >>         hdr = ksmbd_req_buf_next(work);
>> >>         if (le32_to_cpu(hdr->NextCommand) > 0) {
>> >> +               if ((u64)work->next_smb2_rcv_hdr_off +
>> >> le32_to_cpu(hdr->NextCommand) >
>> >> +                   get_rfc1002_len(work->request_buf)) {
>> >> +                       pr_err("next command(%u) offset exceeds smb
>> >> msg
>> >> size\n",
>> >> +                              hdr->NextCommand);
>> >> +                       return false;
>> >> +               }
>> >> +
>> >>                 ksmbd_debug(SMB, "got SMB2 chained command\n");
>> >>                 init_chained_smb2_rsp(work);
>> >>                 return true;
>> >
>> > Very good, reviewed by me.
>> Sorry for late response, Thanks for your review!
>> > The conditional though, since you know there will be at least a full
>> > smb2 header there you could already check that change it to
>> >> +               if ((u64)work->next_smb2_rcv_hdr_off +
>> >> le32_to_cpu(hdr->NextCommand) >
>> >> +                   get_rfc1002_len(work->request_buf) +  64) {
>> Ah, I didn't understand why we should add + 64(smb2 hdr size)...
>> As I know, NextCommand offset included smb2 header size..
>
> This is what I meant.
> +               if ((u64)work->next_smb2_rcv_hdr_off +
> le32_to_cpu(hdr->NextCommand) + 64 >
> +                   get_rfc1002_len(work->request_buf)) {
>
> It could just be an early check that what hdr->NextCommand points to
> has at least 64 bytes.
> I.e. an early test that "does the next PDU have at least a full smb2
> header?"
>
> I mean, since you already test that NextCommand is valid,  you could
> at the same time also
> test that the next pdu is at least 64 bytes.
Understood, I will update it on v3.

Thanks!
>
>> >
>> >
>> > Which leads to another question.  Where do you check that the buffer
>> > contains enough data to hold the smb2 header and the full fixed part
>> > of the request?
>> ksmbd_smb2_check_message() in smb2misc.c should check it.
>>
>> > There is a check that you have enough space for the smb2 header in
>> > ksmbd_conn_handler_loop()
>> > that there is enough space for the smb2 header
>> > (ksmbd_pdu_size_has_room()) but that function assumes that the smb2
>> > header always start at the head of the buffer.
>> > So if you have a compound chain, this functrion only checks the first
>> > pdu.
>> I think that is_chained_smb2_message() will check all pdu as well as first
>> pdu.
>> there is loop do { } while (is_chained_smb2_message(work)); in server.c
>> >
>> >
>> > I know that the buffer handling is copied from the cifs client.  It
>> > used to also do these "just pass a buffer around and the first 4 bytes
>> > is the size" (and still does for smb1)  and there was a lot of
>> > terrible +4 or -4 to all sort of casts and conditionals.
>> > I changed that in cifs.ko to remove the 4 byte length completely from
>> > the buffer.
>> > I also changed it as part of the compounding to pass an array of
>> > requests (each containing an iovector) to the functions instead of
>> > just one large byte array.
>> > That made things a lot easier to manage since you could then assume
>> > that the SMB2 header would always start at offset 0 in the
>> > corresponding iovector, even for compounded commands since they all
>> > had their own private vector.
>> > And since an iovector contains both a pointer and a length there is no
>> > need anymore to read the first 4 bytes/validate them/and covnert into
>> > a length all the time.
>> Right. fully agreed.
>>
>> >
>> > I think that would help, but it would be a MAJOR amount of work, so
>> > maybe that should wait until later.
>> Agreed, I will do that after fixing current urgent issues first!
>>
>> > That approach is very nice since it completely avoids keeping track of
>> > offset-to-where-this-pdu-starts which makes all checks and
>> > conditionals so much more complex.
>> Thanks!
>> >
>> >
>> > regards
>> > ronnie sahlberg
>> >
>> >
>> >> --
>> >> 2.25.1
>> >>
>> >
>

Re: [PATCH v2 2/3] ksmbd: add validation in smb2 negotiate

[Ralph Boehme]

[-- Attachment #1.1: Type: text/plain, Size: 3243 bytes --]

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@gmail.com>
> Cc: Ralph Böhme <slow@samba.org>
> Cc: Steve French <smfrench@gmail.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>   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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 2/3] ksmbd: add validation in smb2 negotiate

[Namjae Jeon]

2021-09-22 23:17 GMT+09:00, Ralph Boehme <slow@samba.org>:
> 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@gmail.com>
>> Cc: Ralph Böhme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>>   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
>
>

Re: [PATCH v2 2/3] ksmbd: add validation in smb2 negotiate

[ronnie sahlberg]

On Thu, Sep 23, 2021 at 9:13 AM Namjae Jeon <linkinjeon@kernel.org> wrote:
>
> 2021-09-22 23:17 GMT+09:00, Ralph Boehme <slow@samba.org>:
> > 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@gmail.com>
> >> Cc: Ralph Böhme <slow@samba.org>
> >> Cc: Steve French <smfrench@gmail.com>
> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> >> ---
> >>   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
> >
> >

Re: [PATCH v2 2/3] ksmbd: add validation in smb2 negotiate

[Namjae Jeon]

2021-09-23 9:12 GMT+09:00, ronnie sahlberg <ronniesahlberg@gmail.com>:
> On Thu, Sep 23, 2021 at 9:13 AM Namjae Jeon <linkinjeon@kernel.org> wrote:
>>
>> 2021-09-22 23:17 GMT+09:00, Ralph Boehme <slow@samba.org>:
>> > 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@gmail.com>
>> >> Cc: Ralph Böhme <slow@samba.org>
>> >> Cc: Steve French <smfrench@gmail.com>
>> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> >> ---
>> >>   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
>> >
>> >
>