* [PATCH 1/2] ksmbd: do not allow the actual frame length to be smaller than the rfc10024 length
@ 2023-02-08 9:41 Namjae Jeon
2023-02-09 23:48 ` Steve French
2023-02-11 20:53 ` Steve French
0 siblings, 2 replies; 6+ messages in thread
From: Namjae Jeon @ 2023-02-08 9:41 UTC (permalink / raw)
To: linux-cifs; +Cc: smfrench, senozhatsky, tom, Namjae Jeon
ksmbd allowed the actual frame length to be smaller than the rfc1002
length. If allowed, it is possible to allocates a large amount of memory
that can be limited by credit management and can eventually cause memory
exhaustion problem. This patch do not allow it except SMB2 Negotiate
request which will be validated when message handling proceeds.
Also, cifs client pad smb2 tree connect to 2bytes.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
fs/ksmbd/smb2misc.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
index a717aa9b4af8..fc44f08b5939 100644
--- a/fs/ksmbd/smb2misc.c
+++ b/fs/ksmbd/smb2misc.c
@@ -408,20 +408,19 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
goto validate_credit;
/*
- * windows client also pad up to 8 bytes when compounding.
- * If pad is longer than eight bytes, log the server behavior
- * (once), since may indicate a problem but allow it and
- * continue since the frame is parseable.
+ * SMB2 NEGOTIATE request will be validated when message
+ * handling proceeds.
*/
- if (clc_len < len) {
- ksmbd_debug(SMB,
- "cli req padded more than expected. Length %d not %d for cmd:%d mid:%llu\n",
- len, clc_len, command,
- le64_to_cpu(hdr->MessageId));
- goto validate_credit;
- }
+ if (command == SMB2_NEGOTIATE_HE)
+ goto validate_credit;
+
+ /*
+ * cifs client pads smb2 tree connect to 2 bytes.
+ */
+ if (clc_len + 2 == len)
+ goto validate_credit;
- ksmbd_debug(SMB,
+ pr_err_ratelimited(
"cli req too short, len %d not %d. cmd:%d mid:%llu\n",
len, clc_len, command,
le64_to_cpu(hdr->MessageId));
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] ksmbd: do not allow the actual frame length to be smaller than the rfc10024 length
2023-02-08 9:41 [PATCH 1/2] ksmbd: do not allow the actual frame length to be smaller than the rfc10024 length Namjae Jeon
@ 2023-02-09 23:48 ` Steve French
2023-02-10 1:14 ` Namjae Jeon
2023-02-11 20:53 ` Steve French
1 sibling, 1 reply; 6+ messages in thread
From: Steve French @ 2023-02-09 23:48 UTC (permalink / raw)
To: Namjae Jeon; +Cc: linux-cifs, senozhatsky, tom
typically rounding up to 8 byte boundary would be logical to allow
On Wed, Feb 8, 2023 at 3:41 AM Namjae Jeon <linkinjeon@kernel.org> wrote:
>
> ksmbd allowed the actual frame length to be smaller than the rfc1002
> length. If allowed, it is possible to allocates a large amount of memory
> that can be limited by credit management and can eventually cause memory
> exhaustion problem. This patch do not allow it except SMB2 Negotiate
> request which will be validated when message handling proceeds.
> Also, cifs client pad smb2 tree connect to 2bytes.
>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
> fs/ksmbd/smb2misc.c | 23 +++++++++++------------
> 1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
> index a717aa9b4af8..fc44f08b5939 100644
> --- a/fs/ksmbd/smb2misc.c
> +++ b/fs/ksmbd/smb2misc.c
> @@ -408,20 +408,19 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
> goto validate_credit;
>
> /*
> - * windows client also pad up to 8 bytes when compounding.
> - * If pad is longer than eight bytes, log the server behavior
> - * (once), since may indicate a problem but allow it and
> - * continue since the frame is parseable.
> + * SMB2 NEGOTIATE request will be validated when message
> + * handling proceeds.
> */
> - if (clc_len < len) {
> - ksmbd_debug(SMB,
> - "cli req padded more than expected. Length %d not %d for cmd:%d mid:%llu\n",
> - len, clc_len, command,
> - le64_to_cpu(hdr->MessageId));
> - goto validate_credit;
> - }
> + if (command == SMB2_NEGOTIATE_HE)
> + goto validate_credit;
> +
> + /*
> + * cifs client pads smb2 tree connect to 2 bytes.
> + */
> + if (clc_len + 2 == len)
> + goto validate_credit;
>
> - ksmbd_debug(SMB,
> + pr_err_ratelimited(
> "cli req too short, len %d not %d. cmd:%d mid:%llu\n",
> len, clc_len, command,
> le64_to_cpu(hdr->MessageId));
> --
> 2.25.1
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] ksmbd: do not allow the actual frame length to be smaller than the rfc10024 length
2023-02-09 23:48 ` Steve French
@ 2023-02-10 1:14 ` Namjae Jeon
0 siblings, 0 replies; 6+ messages in thread
From: Namjae Jeon @ 2023-02-10 1:14 UTC (permalink / raw)
To: Steve French; +Cc: linux-cifs, senozhatsky, tom
2023-02-10 8:48 GMT+09:00, Steve French <smfrench@gmail.com>:
> typically rounding up to 8 byte boundary would be logical to allow
okay, Will update it on v2.
Thanks!
>
> On Wed, Feb 8, 2023 at 3:41 AM Namjae Jeon <linkinjeon@kernel.org> wrote:
>>
>> ksmbd allowed the actual frame length to be smaller than the rfc1002
>> length. If allowed, it is possible to allocates a large amount of memory
>> that can be limited by credit management and can eventually cause memory
>> exhaustion problem. This patch do not allow it except SMB2 Negotiate
>> request which will be validated when message handling proceeds.
>> Also, cifs client pad smb2 tree connect to 2bytes.
>>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>> fs/ksmbd/smb2misc.c | 23 +++++++++++------------
>> 1 file changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
>> index a717aa9b4af8..fc44f08b5939 100644
>> --- a/fs/ksmbd/smb2misc.c
>> +++ b/fs/ksmbd/smb2misc.c
>> @@ -408,20 +408,19 @@ int ksmbd_smb2_check_message(struct ksmbd_work
>> *work)
>> goto validate_credit;
>>
>> /*
>> - * windows client also pad up to 8 bytes when
>> compounding.
>> - * If pad is longer than eight bytes, log the server
>> behavior
>> - * (once), since may indicate a problem but allow it and
>> - * continue since the frame is parseable.
>> + * SMB2 NEGOTIATE request will be validated when message
>> + * handling proceeds.
>> */
>> - if (clc_len < len) {
>> - ksmbd_debug(SMB,
>> - "cli req padded more than expected.
>> Length %d not %d for cmd:%d mid:%llu\n",
>> - len, clc_len, command,
>> - le64_to_cpu(hdr->MessageId));
>> - goto validate_credit;
>> - }
>> + if (command == SMB2_NEGOTIATE_HE)
>> + goto validate_credit;
>> +
>> + /*
>> + * cifs client pads smb2 tree connect to 2 bytes.
>> + */
>> + if (clc_len + 2 == len)
>> + goto validate_credit;
>>
>> - ksmbd_debug(SMB,
>> + pr_err_ratelimited(
>> "cli req too short, len %d not %d. cmd:%d
>> mid:%llu\n",
>> len, clc_len, command,
>> le64_to_cpu(hdr->MessageId));
>> --
>> 2.25.1
>>
>
>
> --
> Thanks,
>
> Steve
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ksmbd: do not allow the actual frame length to be smaller than the rfc10024 length
2023-02-08 9:41 [PATCH 1/2] ksmbd: do not allow the actual frame length to be smaller than the rfc10024 length Namjae Jeon
2023-02-09 23:48 ` Steve French
@ 2023-02-11 20:53 ` Steve French
2023-02-12 2:01 ` Namjae Jeon
1 sibling, 1 reply; 6+ messages in thread
From: Steve French @ 2023-02-11 20:53 UTC (permalink / raw)
To: Namjae Jeon; +Cc: linux-cifs, senozhatsky, tom
Did you see any examples other than (SMB3) tree connect where the
Linux client padded a request beyond end of SMB/SMB3 frame?
On Wed, Feb 8, 2023 at 3:41 AM Namjae Jeon <linkinjeon@kernel.org> wrote:
>
> ksmbd allowed the actual frame length to be smaller than the rfc1002
> length. If allowed, it is possible to allocates a large amount of memory
> that can be limited by credit management and can eventually cause memory
> exhaustion problem. This patch do not allow it except SMB2 Negotiate
> request which will be validated when message handling proceeds.
> Also, cifs client pad smb2 tree connect to 2bytes.
>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
> fs/ksmbd/smb2misc.c | 23 +++++++++++------------
> 1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
> index a717aa9b4af8..fc44f08b5939 100644
> --- a/fs/ksmbd/smb2misc.c
> +++ b/fs/ksmbd/smb2misc.c
> @@ -408,20 +408,19 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
> goto validate_credit;
>
> /*
> - * windows client also pad up to 8 bytes when compounding.
> - * If pad is longer than eight bytes, log the server behavior
> - * (once), since may indicate a problem but allow it and
> - * continue since the frame is parseable.
> + * SMB2 NEGOTIATE request will be validated when message
> + * handling proceeds.
> */
> - if (clc_len < len) {
> - ksmbd_debug(SMB,
> - "cli req padded more than expected. Length %d not %d for cmd:%d mid:%llu\n",
> - len, clc_len, command,
> - le64_to_cpu(hdr->MessageId));
> - goto validate_credit;
> - }
> + if (command == SMB2_NEGOTIATE_HE)
> + goto validate_credit;
> +
> + /*
> + * cifs client pads smb2 tree connect to 2 bytes.
> + */
> + if (clc_len + 2 == len)
> + goto validate_credit;
>
> - ksmbd_debug(SMB,
> + pr_err_ratelimited(
> "cli req too short, len %d not %d. cmd:%d mid:%llu\n",
> len, clc_len, command,
> le64_to_cpu(hdr->MessageId));
> --
> 2.25.1
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] ksmbd: do not allow the actual frame length to be smaller than the rfc10024 length
2023-02-11 20:53 ` Steve French
@ 2023-02-12 2:01 ` Namjae Jeon
2023-02-12 20:43 ` Steve French
0 siblings, 1 reply; 6+ messages in thread
From: Namjae Jeon @ 2023-02-12 2:01 UTC (permalink / raw)
To: Steve French; +Cc: linux-cifs, senozhatsky, tom
2023-02-12 5:53 GMT+09:00, Steve French <smfrench@gmail.com>:
> Did you see any examples other than (SMB3) tree connect where the
> Linux client padded a request beyond end of SMB/SMB3 frame?
Yes. libsmb, but rfc1002 length is 8byte-aligned frame length. (i.e.
ALIGN(clc_len, 8) == len) but cifs don't do that about smb2 tree
connect, just frame length + 2.
And I can not understand why cifs pad smb2 tree connect to 2bytes.
note that smbclient, windows, MacOs, and Nautilus clients do not pad
smb2 tree connect request.
>
> On Wed, Feb 8, 2023 at 3:41 AM Namjae Jeon <linkinjeon@kernel.org> wrote:
>>
>> ksmbd allowed the actual frame length to be smaller than the rfc1002
>> length. If allowed, it is possible to allocates a large amount of memory
>> that can be limited by credit management and can eventually cause memory
>> exhaustion problem. This patch do not allow it except SMB2 Negotiate
>> request which will be validated when message handling proceeds.
>> Also, cifs client pad smb2 tree connect to 2bytes.
>>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>> fs/ksmbd/smb2misc.c | 23 +++++++++++------------
>> 1 file changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
>> index a717aa9b4af8..fc44f08b5939 100644
>> --- a/fs/ksmbd/smb2misc.c
>> +++ b/fs/ksmbd/smb2misc.c
>> @@ -408,20 +408,19 @@ int ksmbd_smb2_check_message(struct ksmbd_work
>> *work)
>> goto validate_credit;
>>
>> /*
>> - * windows client also pad up to 8 bytes when
>> compounding.
>> - * If pad is longer than eight bytes, log the server
>> behavior
>> - * (once), since may indicate a problem but allow it and
>> - * continue since the frame is parseable.
>> + * SMB2 NEGOTIATE request will be validated when message
>> + * handling proceeds.
>> */
>> - if (clc_len < len) {
>> - ksmbd_debug(SMB,
>> - "cli req padded more than expected.
>> Length %d not %d for cmd:%d mid:%llu\n",
>> - len, clc_len, command,
>> - le64_to_cpu(hdr->MessageId));
>> - goto validate_credit;
>> - }
>> + if (command == SMB2_NEGOTIATE_HE)
>> + goto validate_credit;
>> +
>> + /*
>> + * cifs client pads smb2 tree connect to 2 bytes.
>> + */
>> + if (clc_len + 2 == len)
>> + goto validate_credit;
>>
>> - ksmbd_debug(SMB,
>> + pr_err_ratelimited(
>> "cli req too short, len %d not %d. cmd:%d
>> mid:%llu\n",
>> len, clc_len, command,
>> le64_to_cpu(hdr->MessageId));
>> --
>> 2.25.1
>>
>
>
> --
> Thanks,
>
> Steve
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] ksmbd: do not allow the actual frame length to be smaller than the rfc10024 length
2023-02-12 2:01 ` Namjae Jeon
@ 2023-02-12 20:43 ` Steve French
0 siblings, 0 replies; 6+ messages in thread
From: Steve French @ 2023-02-12 20:43 UTC (permalink / raw)
To: Namjae Jeon; +Cc: linux-cifs, senozhatsky, tom
seems reasonable to merge the client fix - I will doublecheck it later
today or tomorrow and add it to for-next
On Sat, Feb 11, 2023 at 8:01 PM Namjae Jeon <linkinjeon@kernel.org> wrote:
>
> 2023-02-12 5:53 GMT+09:00, Steve French <smfrench@gmail.com>:
> > Did you see any examples other than (SMB3) tree connect where the
> > Linux client padded a request beyond end of SMB/SMB3 frame?
> Yes. libsmb, but rfc1002 length is 8byte-aligned frame length. (i.e.
> ALIGN(clc_len, 8) == len) but cifs don't do that about smb2 tree
> connect, just frame length + 2.
> And I can not understand why cifs pad smb2 tree connect to 2bytes.
>
> note that smbclient, windows, MacOs, and Nautilus clients do not pad
> smb2 tree connect request.
>
> >
> > On Wed, Feb 8, 2023 at 3:41 AM Namjae Jeon <linkinjeon@kernel.org> wrote:
> >>
> >> ksmbd allowed the actual frame length to be smaller than the rfc1002
> >> length. If allowed, it is possible to allocates a large amount of memory
> >> that can be limited by credit management and can eventually cause memory
> >> exhaustion problem. This patch do not allow it except SMB2 Negotiate
> >> request which will be validated when message handling proceeds.
> >> Also, cifs client pad smb2 tree connect to 2bytes.
> >>
> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> >> ---
> >> fs/ksmbd/smb2misc.c | 23 +++++++++++------------
> >> 1 file changed, 11 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
> >> index a717aa9b4af8..fc44f08b5939 100644
> >> --- a/fs/ksmbd/smb2misc.c
> >> +++ b/fs/ksmbd/smb2misc.c
> >> @@ -408,20 +408,19 @@ int ksmbd_smb2_check_message(struct ksmbd_work
> >> *work)
> >> goto validate_credit;
> >>
> >> /*
> >> - * windows client also pad up to 8 bytes when
> >> compounding.
> >> - * If pad is longer than eight bytes, log the server
> >> behavior
> >> - * (once), since may indicate a problem but allow it and
> >> - * continue since the frame is parseable.
> >> + * SMB2 NEGOTIATE request will be validated when message
> >> + * handling proceeds.
> >> */
> >> - if (clc_len < len) {
> >> - ksmbd_debug(SMB,
> >> - "cli req padded more than expected.
> >> Length %d not %d for cmd:%d mid:%llu\n",
> >> - len, clc_len, command,
> >> - le64_to_cpu(hdr->MessageId));
> >> - goto validate_credit;
> >> - }
> >> + if (command == SMB2_NEGOTIATE_HE)
> >> + goto validate_credit;
> >> +
> >> + /*
> >> + * cifs client pads smb2 tree connect to 2 bytes.
> >> + */
> >> + if (clc_len + 2 == len)
> >> + goto validate_credit;
> >>
> >> - ksmbd_debug(SMB,
> >> + pr_err_ratelimited(
> >> "cli req too short, len %d not %d. cmd:%d
> >> mid:%llu\n",
> >> len, clc_len, command,
> >> le64_to_cpu(hdr->MessageId));
> >> --
> >> 2.25.1
> >>
> >
> >
> > --
> > Thanks,
> >
> > Steve
> >
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-02-12 20:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-08 9:41 [PATCH 1/2] ksmbd: do not allow the actual frame length to be smaller than the rfc10024 length Namjae Jeon
2023-02-09 23:48 ` Steve French
2023-02-10 1:14 ` Namjae Jeon
2023-02-11 20:53 ` Steve French
2023-02-12 2:01 ` Namjae Jeon
2023-02-12 20:43 ` Steve French
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox