[PATCH] smb/server: fix SMB2_MIN_SUPPORTED_HEADER_SIZE value

[PATCH] smb/server: fix SMB2_MIN_SUPPORTED_HEADER_SIZE value

[chenxiaosong.chenxiaosong]

From: ChenXiaoSong <chenxiaosong@kylinos.cn>

See RFC1002 4.3.1.

The LENGTH field is the number of bytes following the LENGTH
field.  In other words, LENGTH is the combined size of the
TRAILER field(s).

Link: https://lore.kernel.org/linux-cifs/e4fbcbad-459a-412c-918c-0279ec890353@linux.dev/
Reported-by: David Howells <dhowells@redhat.com>
Signed-off-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
---
 fs/smb/server/connection.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/smb/server/connection.c b/fs/smb/server/connection.c
index b6b4f1286b9c..da6dfd0d80c2 100644
--- a/fs/smb/server/connection.c
+++ b/fs/smb/server/connection.c
@@ -296,7 +296,7 @@ bool ksmbd_conn_alive(struct ksmbd_conn *conn)
 }
 
 #define SMB1_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb_hdr))
-#define SMB2_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr) + 4)
+#define SMB2_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr))
 
 /**
  * ksmbd_conn_handler_loop() - session thread to listen on new smb requests
-- 
2.43.0

Re: [PATCH] smb/server: fix SMB2_MIN_SUPPORTED_HEADER_SIZE value

[Namjae Jeon]

On Fri, Dec 19, 2025 at 2:11 AM <chenxiaosong.chenxiaosong@linux.dev> wrote:
>
> From: ChenXiaoSong <chenxiaosong@kylinos.cn>
>
> See RFC1002 4.3.1.
>
> The LENGTH field is the number of bytes following the LENGTH
> field.  In other words, LENGTH is the combined size of the
> TRAILER field(s).
>
> Link: https://lore.kernel.org/linux-cifs/e4fbcbad-459a-412c-918c-0279ec890353@linux.dev/
> Reported-by: David Howells <dhowells@redhat.com>
> Signed-off-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
> ---
>  fs/smb/server/connection.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/smb/server/connection.c b/fs/smb/server/connection.c
> index b6b4f1286b9c..da6dfd0d80c2 100644
> --- a/fs/smb/server/connection.c
> +++ b/fs/smb/server/connection.c
> @@ -296,7 +296,7 @@ bool ksmbd_conn_alive(struct ksmbd_conn *conn)
>  }
>
>  #define SMB1_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb_hdr))
> -#define SMB2_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr) + 4)
> +#define SMB2_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr))
+4 is needed to validate the ByteCount field of the smb1 request and
the StructureSize2 field of the smb2 request. Let's change the macro
name from HEADER_SIZE to PDU_SIZE and add +4 to
SMB1_MIN_SUPPORTED_PDU_SIZE.
>
>  /**
>   * ksmbd_conn_handler_loop() - session thread to listen on new smb requests
> --
> 2.43.0
>

Re: [PATCH] smb/server: fix SMB2_MIN_SUPPORTED_HEADER_SIZE value

[ChenXiaoSong]

Hi Namjae,

We should rename them to `SMB1_MIN_SUPPORTED_PDU_SIZE` and 
`SMB2_MIN_SUPPORTED_PDU_SIZE`.

But we "should not" add "+4" to them.

The `ksmbd_conn_handler_loop()` function is as follows:

ksmbd_conn_handler_loop()
{
   ...
   pdu_size = get_rfc1002_len(hdr_buf);
   ...
   if (pdu_size < SMB1_MIN_SUPPORTED_HEADER_SIZE)
   ...
   if (pdu_size < SMB2_MIN_SUPPORTED_HEADER_SIZE)
   ...
}

Thanks,
ChenXiaoSong.

On 12/19/25 08:16, Namjae Jeon wrote:
> On Fri, Dec 19, 2025 at 2:11 AM <chenxiaosong.chenxiaosong@linux.dev> wrote:
>>
>> From: ChenXiaoSong <chenxiaosong@kylinos.cn>
>>
>> See RFC1002 4.3.1.
>>
>> The LENGTH field is the number of bytes following the LENGTH
>> field.  In other words, LENGTH is the combined size of the
>> TRAILER field(s).
>>
>> Link: https://lore.kernel.org/linux-cifs/e4fbcbad-459a-412c-918c-0279ec890353@linux.dev/
>> Reported-by: David Howells <dhowells@redhat.com>
>> Signed-off-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
>> ---
>>   fs/smb/server/connection.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/smb/server/connection.c b/fs/smb/server/connection.c
>> index b6b4f1286b9c..da6dfd0d80c2 100644
>> --- a/fs/smb/server/connection.c
>> +++ b/fs/smb/server/connection.c
>> @@ -296,7 +296,7 @@ bool ksmbd_conn_alive(struct ksmbd_conn *conn)
>>   }
>>
>>   #define SMB1_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb_hdr))
>> -#define SMB2_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr) + 4)
>> +#define SMB2_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr))
> +4 is needed to validate the ByteCount field of the smb1 request and
> the StructureSize2 field of the smb2 request. Let's change the macro
> name from HEADER_SIZE to PDU_SIZE and add +4 to
> SMB1_MIN_SUPPORTED_PDU_SIZE.
>>
>>   /**
>>    * ksmbd_conn_handler_loop() - session thread to listen on new smb requests
>> --
>> 2.43.0
>>

Re: [PATCH] smb/server: fix SMB2_MIN_SUPPORTED_HEADER_SIZE value

[Namjae Jeon]

On Fri, Dec 19, 2025 at 10:00 AM ChenXiaoSong
<chenxiaosong.chenxiaosong@linux.dev> wrote:
>
> Hi Namjae,
>
> We should rename them to `SMB1_MIN_SUPPORTED_PDU_SIZE` and
> `SMB2_MIN_SUPPORTED_PDU_SIZE`.
>
> But we "should not" add "+4" to them.
Not adding the +4 will trigger a slab-out-of-bounds issue.
You should check ksmbd_smb2_check_message() and
ksmbd_negotiate_smb_dialect() as well as ksmbd_conn_handler_loop().
>
> The `ksmbd_conn_handler_loop()` function is as follows:
>
> ksmbd_conn_handler_loop()
> {
>    ...
>    pdu_size = get_rfc1002_len(hdr_buf);
>    ...
>    if (pdu_size < SMB1_MIN_SUPPORTED_HEADER_SIZE)
>    ...
>    if (pdu_size < SMB2_MIN_SUPPORTED_HEADER_SIZE)
>    ...
> }
>
> Thanks,
> ChenXiaoSong.
>
> On 12/19/25 08:16, Namjae Jeon wrote:
> > On Fri, Dec 19, 2025 at 2:11 AM <chenxiaosong.chenxiaosong@linux.dev> wrote:
> >>
> >> From: ChenXiaoSong <chenxiaosong@kylinos.cn>
> >>
> >> See RFC1002 4.3.1.
> >>
> >> The LENGTH field is the number of bytes following the LENGTH
> >> field.  In other words, LENGTH is the combined size of the
> >> TRAILER field(s).
> >>
> >> Link: https://lore.kernel.org/linux-cifs/e4fbcbad-459a-412c-918c-0279ec890353@linux.dev/
> >> Reported-by: David Howells <dhowells@redhat.com>
> >> Signed-off-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
> >> ---
> >>   fs/smb/server/connection.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/fs/smb/server/connection.c b/fs/smb/server/connection.c
> >> index b6b4f1286b9c..da6dfd0d80c2 100644
> >> --- a/fs/smb/server/connection.c
> >> +++ b/fs/smb/server/connection.c
> >> @@ -296,7 +296,7 @@ bool ksmbd_conn_alive(struct ksmbd_conn *conn)
> >>   }
> >>
> >>   #define SMB1_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb_hdr))
> >> -#define SMB2_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr) + 4)
> >> +#define SMB2_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr))
> > +4 is needed to validate the ByteCount field of the smb1 request and
> > the StructureSize2 field of the smb2 request. Let's change the macro
> > name from HEADER_SIZE to PDU_SIZE and add +4 to
> > SMB1_MIN_SUPPORTED_PDU_SIZE.
> >>
> >>   /**
> >>    * ksmbd_conn_handler_loop() - session thread to listen on new smb requests
> >> --
> >> 2.43.0
> >>
>

Re: [PATCH] smb/server: fix SMB2_MIN_SUPPORTED_HEADER_SIZE value

[ChenXiaoSong]

Hi Namjae,

`SMB1_MIN_SUPPORTED_HEADER_SIZE` and `SMB2_MIN_SUPPORTED_HEADER_SIZE` 
are only used in `ksmbd_conn_handler_loop()` to check the PDU size, and 
seems not to cause slab-out-of-bounds issues.

Thanks,
ChenXiaoSong.

On 12/19/25 09:16, Namjae Jeon wrote:
> On Fri, Dec 19, 2025 at 10:00 AM ChenXiaoSong
> <chenxiaosong.chenxiaosong@linux.dev> wrote:
>>
>> Hi Namjae,
>>
>> We should rename them to `SMB1_MIN_SUPPORTED_PDU_SIZE` and
>> `SMB2_MIN_SUPPORTED_PDU_SIZE`.
>>
>> But we "should not" add "+4" to them.
> Not adding the +4 will trigger a slab-out-of-bounds issue.
> You should check ksmbd_smb2_check_message() and
> ksmbd_negotiate_smb_dialect() as well as ksmbd_conn_handler_loop().
>>
>> The `ksmbd_conn_handler_loop()` function is as follows:
>>
>> ksmbd_conn_handler_loop()
>> {
>>     ...
>>     pdu_size = get_rfc1002_len(hdr_buf);
>>     ...
>>     if (pdu_size < SMB1_MIN_SUPPORTED_HEADER_SIZE)
>>     ...
>>     if (pdu_size < SMB2_MIN_SUPPORTED_HEADER_SIZE)
>>     ...
>> }
>>
>> Thanks,
>> ChenXiaoSong.
>>

Re: [PATCH] smb/server: fix SMB2_MIN_SUPPORTED_HEADER_SIZE value

[Namjae Jeon]

On Fri, Dec 19, 2025 at 10:31 AM ChenXiaoSong
<chenxiaosong.chenxiaosong@linux.dev> wrote:
>
> Hi Namjae,
>
> `SMB1_MIN_SUPPORTED_HEADER_SIZE` and `SMB2_MIN_SUPPORTED_HEADER_SIZE`
> are only used in `ksmbd_conn_handler_loop()` to check the PDU size, and
> seems not to cause slab-out-of-bounds issues.
Okay, I explained it but you didn't listen... So I can not ACK this patch.
>
> Thanks,
> ChenXiaoSong.
>
> On 12/19/25 09:16, Namjae Jeon wrote:
> > On Fri, Dec 19, 2025 at 10:00 AM ChenXiaoSong
> > <chenxiaosong.chenxiaosong@linux.dev> wrote:
> >>
> >> Hi Namjae,
> >>
> >> We should rename them to `SMB1_MIN_SUPPORTED_PDU_SIZE` and
> >> `SMB2_MIN_SUPPORTED_PDU_SIZE`.
> >>
> >> But we "should not" add "+4" to them.
> > Not adding the +4 will trigger a slab-out-of-bounds issue.
> > You should check ksmbd_smb2_check_message() and
> > ksmbd_negotiate_smb_dialect() as well as ksmbd_conn_handler_loop().
> >>
> >> The `ksmbd_conn_handler_loop()` function is as follows:
> >>
> >> ksmbd_conn_handler_loop()
> >> {
> >>     ...
> >>     pdu_size = get_rfc1002_len(hdr_buf);
> >>     ...
> >>     if (pdu_size < SMB1_MIN_SUPPORTED_HEADER_SIZE)
> >>     ...
> >>     if (pdu_size < SMB2_MIN_SUPPORTED_HEADER_SIZE)
> >>     ...
> >> }
> >>
> >> Thanks,
> >> ChenXiaoSong.
> >>

Re: [PATCH] smb/server: fix SMB2_MIN_SUPPORTED_HEADER_SIZE value

[ChenXiaoSong]

I will analyze the code in more detail. Thanks for your review.

Thanks,
ChenXiaoSong.

On 12/19/25 09:42, Namjae Jeon wrote:
> On Fri, Dec 19, 2025 at 10:31 AM ChenXiaoSong
> <chenxiaosong.chenxiaosong@linux.dev> wrote:
>>
>> Hi Namjae,
>>
>> `SMB1_MIN_SUPPORTED_HEADER_SIZE` and `SMB2_MIN_SUPPORTED_HEADER_SIZE`
>> are only used in `ksmbd_conn_handler_loop()` to check the PDU size, and
>> seems not to cause slab-out-of-bounds issues.
> Okay, I explained it but you didn't listen... So I can not ACK this patch.
>>
>> Thanks,
>> ChenXiaoSong.
>>
>> On 12/19/25 09:16, Namjae Jeon wrote:
>>> On Fri, Dec 19, 2025 at 10:00 AM ChenXiaoSong
>>> <chenxiaosong.chenxiaosong@linux.dev> wrote:
>>>>
>>>> Hi Namjae,
>>>>
>>>> We should rename them to `SMB1_MIN_SUPPORTED_PDU_SIZE` and
>>>> `SMB2_MIN_SUPPORTED_PDU_SIZE`.
>>>>
>>>> But we "should not" add "+4" to them.
>>> Not adding the +4 will trigger a slab-out-of-bounds issue.
>>> You should check ksmbd_smb2_check_message() and
>>> ksmbd_negotiate_smb_dialect() as well as ksmbd_conn_handler_loop().
>>>>
>>>> The `ksmbd_conn_handler_loop()` function is as follows:
>>>>
>>>> ksmbd_conn_handler_loop()
>>>> {
>>>>      ...
>>>>      pdu_size = get_rfc1002_len(hdr_buf);
>>>>      ...
>>>>      if (pdu_size < SMB1_MIN_SUPPORTED_HEADER_SIZE)
>>>>      ...
>>>>      if (pdu_size < SMB2_MIN_SUPPORTED_HEADER_SIZE)
>>>>      ...
>>>> }
>>>>
>>>> Thanks,
>>>> ChenXiaoSong.
>>>>

Re: [PATCH] smb/server: fix SMB2_MIN_SUPPORTED_HEADER_SIZE value

[David Howells]

Namjae Jeon <linkinjeon@kernel.org> wrote:

> > We should rename them to `SMB1_MIN_SUPPORTED_PDU_SIZE` and
> > `SMB2_MIN_SUPPORTED_PDU_SIZE`.
> >
> > But we "should not" add "+4" to them.
> Not adding the +4 will trigger a slab-out-of-bounds issue.
> You should check ksmbd_smb2_check_message() and
> ksmbd_negotiate_smb_dialect() as well as ksmbd_conn_handler_loop().
> ...
> >    pdu_size = get_rfc1002_len(hdr_buf);
> >    ...
> >    if (pdu_size < SMB1_MIN_SUPPORTED_HEADER_SIZE)
> >    ...
> >    if (pdu_size < SMB2_MIN_SUPPORTED_HEADER_SIZE)

As previously mentioned, the problem I have with the +4 accounting for the
RFC1002 header is that the size value in pdu_size does not include the size of
the RFC1002 header, so the comparison seems to be allowing an overlong header.

However, I think the +4 actually makes sense for SMB2/3 - assuming the +4
isn't actually for the RFC1002 size, but is rather for the StructureSize of
the operation header that follows immediately after the smb2_hdr.

If that's the case, I would guess that the SMB1 variant might want a +2 to
allow for the BCC field...  but according to the code in cifs side that I've
looked at, some servers may only provide half a BCC field - or maybe even
forget to put it in entirely.

David

Re: [PATCH] smb/server: fix SMB2_MIN_SUPPORTED_HEADER_SIZE value

[Namjae Jeon]

On Fri, Dec 19, 2025 at 5:18 PM David Howells <dhowells@redhat.com> wrote:
>
> Namjae Jeon <linkinjeon@kernel.org> wrote:
>
> > > We should rename them to `SMB1_MIN_SUPPORTED_PDU_SIZE` and
> > > `SMB2_MIN_SUPPORTED_PDU_SIZE`.
> > >
> > > But we "should not" add "+4" to them.
> > Not adding the +4 will trigger a slab-out-of-bounds issue.
> > You should check ksmbd_smb2_check_message() and
> > ksmbd_negotiate_smb_dialect() as well as ksmbd_conn_handler_loop().
> > ...
> > >    pdu_size = get_rfc1002_len(hdr_buf);
> > >    ...
> > >    if (pdu_size < SMB1_MIN_SUPPORTED_HEADER_SIZE)
> > >    ...
> > >    if (pdu_size < SMB2_MIN_SUPPORTED_HEADER_SIZE)
>
> As previously mentioned, the problem I have with the +4 accounting for the
> RFC1002 header is that the size value in pdu_size does not include the size of
> the RFC1002 header, so the comparison seems to be allowing an overlong header.
>
> However, I think the +4 actually makes sense for SMB2/3 - assuming the +4
> isn't actually for the RFC1002 size, but is rather for the StructureSize of
> the operation header that follows immediately after the smb2_hdr.
Right.
>
> If that's the case, I would guess that the SMB1 variant might want a +2 to
> allow for the BCC field...  but according to the code in cifs side that I've
> looked at, some servers may only provide half a BCC field - or maybe even
> forget to put it in entirely.
ksmbd only handles SMB1 negotiate requests in smb1 protocol, And the
BCC (Byte Count) field of smb1 negotiate request must be greater than
or equal to 2. This is why I originally treated any SMB1 request
smaller than smb_hdr + 4 as an invalid packet. However, even if we add
+2, it will be no problem because ksmbd_negotiate_smb_dialect() checks
to verify if the BCC field is less than 2.

ChenXiaoSong, If you agree with this, Can you make the v2 patch ?

Thanks.
>
> David
>

Re: [PATCH] smb/server: fix SMB2_MIN_SUPPORTED_HEADER_SIZE value

[ChenXiaoSong]

OK, I will send v2 soon.
Thanks,
ChenXiaoSong.

On December 19, 2025 6:32:18 PM GMT+08:00, Namjae Jeon <linkinjeon@kernel.org> wrote:
>On Fri, Dec 19, 2025 at 5:18 PM David Howells <dhowells@redhat.com> wrote:
>>
>> Namjae Jeon <linkinjeon@kernel.org> wrote:
>>
>> > > We should rename them to `SMB1_MIN_SUPPORTED_PDU_SIZE` and
>> > > `SMB2_MIN_SUPPORTED_PDU_SIZE`.
>> > >
>> > > But we "should not" add "+4" to them.
>> > Not adding the +4 will trigger a slab-out-of-bounds issue.
>> > You should check ksmbd_smb2_check_message() and
>> > ksmbd_negotiate_smb_dialect() as well as ksmbd_conn_handler_loop().
>> > ...
>> > >    pdu_size = get_rfc1002_len(hdr_buf);
>> > >    ...
>> > >    if (pdu_size < SMB1_MIN_SUPPORTED_HEADER_SIZE)
>> > >    ...
>> > >    if (pdu_size < SMB2_MIN_SUPPORTED_HEADER_SIZE)
>>
>> As previously mentioned, the problem I have with the +4 accounting for the
>> RFC1002 header is that the size value in pdu_size does not include the size of
>> the RFC1002 header, so the comparison seems to be allowing an overlong header.
>>
>> However, I think the +4 actually makes sense for SMB2/3 - assuming the +4
>> isn't actually for the RFC1002 size, but is rather for the StructureSize of
>> the operation header that follows immediately after the smb2_hdr.
>Right.
>>
>> If that's the case, I would guess that the SMB1 variant might want a +2 to
>> allow for the BCC field...  but according to the code in cifs side that I've
>> looked at, some servers may only provide half a BCC field - or maybe even
>> forget to put it in entirely.
>ksmbd only handles SMB1 negotiate requests in smb1 protocol, And the
>BCC (Byte Count) field of smb1 negotiate request must be greater than
>or equal to 2. This is why I originally treated any SMB1 request
>smaller than smb_hdr + 4 as an invalid packet. However, even if we add
>+2, it will be no problem because ksmbd_negotiate_smb_dialect() checks
>to verify if the BCC field is less than 2.
>
>ChenXiaoSong, If you agree with this, Can you make the v2 patch ?
>
>Thanks.
>>
>> David
>>

Re: [PATCH] smb/server: fix SMB2_MIN_SUPPORTED_HEADER_SIZE value

[David Howells]

Namjae Jeon <linkinjeon@kernel.org> wrote:

> ChenXiaoSong, If you agree with this, Can you make the v2 patch ?

Can I suggest adding a comment to indicate what the +4 represents in the
SMB2/3 case?

David

Re: [PATCH] smb/server: fix SMB2_MIN_SUPPORTED_HEADER_SIZE value

[Namjae Jeon]

On Fri, Dec 19, 2025 at 7:50 PM David Howells <dhowells@redhat.com> wrote:
>
> Namjae Jeon <linkinjeon@kernel.org> wrote:
>
> > ChenXiaoSong, If you agree with this, Can you make the v2 patch ?
>
> Can I suggest adding a comment to indicate what the +4 represents in the
> SMB2/3 case?
He will add comments for both SMB1 and SMB2/3 cases to clarify why the
+2 or +4 is added.
>
> David
>

Re: [PATCH] smb/server: fix SMB2_MIN_SUPPORTED_HEADER_SIZE value

[ChenXiaoSong]

Hi Namjae and David,

Should SMB2_MIN_SUPPORTED_HEADER_SIZE(will rename to 
SMB_MIN_SUPPORTED_PDU_SIZE) be sizeof(struct smb2_pdu), i.e. 
sizeof(struct smb2_hdr) + 2?

If my understanding is incorrect, please let me know.

Thanks,
ChenXiaoSong.

On 12/19/25 7:52 PM, Namjae Jeon wrote:
> On Fri, Dec 19, 2025 at 7:50 PM David Howells <dhowells@redhat.com> wrote:
>>
>> Namjae Jeon <linkinjeon@kernel.org> wrote:
>>
>>> ChenXiaoSong, If you agree with this, Can you make the v2 patch ?
>>
>> Can I suggest adding a comment to indicate what the +4 represents in the
>> SMB2/3 case?
> He will add comments for both SMB1 and SMB2/3 cases to clarify why the
> +2 or +4 is added.
>>
>> David
>>