Cmt: [SRU][Jammy][PATCH 1/1] ksmbd: check the validation of pdu_size in ksmbd_conn_handler_loop

[Roxana Nicolescu]

On 13/02/2024 10:36, Stefan Bader wrote:
> On 26.01.24 10:55, Stefan Bader wrote:
>> On 25.01.24 22:44, Bethany Jamison wrote:
>>> From: Namjae Jeon <linkinjeon at kernel.org>
>>>
>>> The length field of netbios header must be greater than the SMB header
>>> sizes(smb1 or smb2 header), otherwise the packet is an invalid SMB 
>>> packet.
>>>
>>> If `pdu_size` is 0, ksmbd allocates a 4 bytes chunk to 
>>> `conn->request_buf`.
>>> In the function `get_smb2_cmd_val` ksmbd will read cmd from
>>> `rcv_hdr->Command`, which is `conn->request_buf + 12`, causing the 
>>> KASAN
>>> detector to print the following error message:
>>>
>>> [    7.205018] BUG: KASAN: slab-out-of-bounds in 
>>> get_smb2_cmd_val+0x45/0x60
>>> [    7.205423] Read of size 2 at addr ffff8880062d8b50 by task 
>>> ksmbd:42632/248
>>> ...
>>> [    7.207125]  <TASK>
>>> [    7.209191]  get_smb2_cmd_val+0x45/0x60
>>> [    7.209426]  ksmbd_conn_enqueue_request+0x3a/0x100
>>> [    7.209712]  ksmbd_server_process_request+0x72/0x160
>>> [    7.210295]  ksmbd_conn_handler_loop+0x30c/0x550
>>> [    7.212280]  kthread+0x160/0x190
>>> [    7.212762]  ret_from_fork+0x1f/0x30
>>> [    7.212981]  </TASK>
>>>
>>> Cc: stable at vger.kernel.org
>>> Reported-by: Chih-Yen Chang <cc85nod at gmail.com>
>>> Signed-off-by: Namjae Jeon <linkinjeon at kernel.org>
>>> Signed-off-by: Steve French <stfrench at microsoft.com>
>>> (backported from commit 368ba06881c395f1c9a7ba22203cf8d78b4addc0)
>>> [bjamison: Jammy was missing the definition of smb2_get_msg function 
>>> which was
>>> used in the fix commit - function definition was very simple so I 
>>> added it]
>>
>> I do not want to say NACK right away but are you sure this works? The 
>> commit which added this functions was
>>
>> commit cb4517201b8acdb5fd5314494aaf86c267f22345
>> Author: Namjae Jeon <linkinjeon at kernel.org>
>> Date:   Wed Nov 3 08:08:44 2021 +0900
>>
>>      ksmbd: remove smb2_buf_length in smb2_hdr
>>
>> Which also did a lot of rework in the structures... When I looks at 
>> one of the places where the smb2_get_msg gets added:
>>
>> diff --git a/fs/ksmbd/oplock.c b/fs/ksmbd/oplock.c
>> index f9dae6ef2115..ce0e85552da9 100644
>> --- a/fs/ksmbd/oplock.c
>> +++ b/fs/ksmbd/oplock.c
>> @@ -629,10 +629,10 @@ static void __smb2_oplock_break_noti(struct 
>> work_struct *wk)
>>                  return;
>>          }
>>
>> -       rsp_hdr = work->response_buf;
>> +       rsp_hdr = smb2_get_msg(work->response_buf);
>>
>> This looks like Jammy code would still expect the rsp_hdr to be where 
>> work->reponse_buf points to. While the access function uses a +4 offset.
>>
>
> Hi Bethany,
>
> this request is in a bit of a fuzzy state. Did we figure out whether 
> my concern was valid or not. If yes, could you nack this, if no, maybe 
> you could explain me why I am wrong.
>>
>>> CVE-2023-38431
>>> Signed-off-by: Bethany Jamison <bethany.jamison at canonical.com>
>>> ---
>>>   fs/ksmbd/connection.c | 12 ++++++++++++
>>>   fs/ksmbd/smb2pdu.h    |  9 +++++++++
>>>   2 files changed, 21 insertions(+)
>>>
>>> diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c
>>> index 3c4b7a96919ce..33d9f6ade7004 100644
>>> --- a/fs/ksmbd/connection.c
>>> +++ b/fs/ksmbd/connection.c
>>> @@ -263,6 +263,9 @@ bool ksmbd_conn_alive(struct ksmbd_conn *conn)
>>>       return true;
>>>   }
>>> +#define SMB1_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb_hdr))
>>> +#define SMB2_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr) + 4)
>>> +
>>>   /**
>>>    * ksmbd_conn_handler_loop() - session thread to listen on new smb 
>>> requests
>>>    * @p:        connection instance
>>> @@ -319,6 +322,9 @@ int ksmbd_conn_handler_loop(void *p)
>>>           if (pdu_size > MAX_STREAM_PROT_LEN)
>>>               break;
>>> +        if (pdu_size < SMB1_MIN_SUPPORTED_HEADER_SIZE)
>>> +            break;
>>> +
>>>           /* 4 for rfc1002 length field */
>>>           /* 1 for implied bcc[0] */
>>>           size = pdu_size + 4 + 1;
>>> @@ -346,6 +352,12 @@ int ksmbd_conn_handler_loop(void *p)
>>>               continue;
>>>           }
>>> +        if (((struct smb2_hdr 
>>> *)smb2_get_msg(conn->request_buf))->ProtocolId ==
>>
>> Maybe that should be
>>
>> if ( ((struct smb2_hdr *) conn->request_buf )->ProtocolId == 
>> SMB2_PROTO_NUMBER) {
>>
>>
>>> +            SMB2_PROTO_NUMBER) {
>>> +            if (pdu_size < SMB2_MIN_SUPPORTED_HEADER_SIZE)
>>> +                break;
>>> +        }
>>> +
>>>           if (!default_conn_ops.process_fn) {
>>>               pr_err("No connection request callback\n");
>>>               break;
>>> diff --git a/fs/ksmbd/smb2pdu.h b/fs/ksmbd/smb2pdu.h
>>> index fa1cd556ab7ac..c8cc1083d3c81 100644
>>> --- a/fs/ksmbd/smb2pdu.h
>>> +++ b/fs/ksmbd/smb2pdu.h
>>> @@ -1707,4 +1707,13 @@ int smb2_ioctl(struct ksmbd_work *work);
>>>   int smb2_oplock_break(struct ksmbd_work *work);
>>>   int smb2_notify(struct ksmbd_work *ksmbd_work);
>>> +/*
>>> + * Get the body of the smb2 message excluding the 4 byte rfc1002 
>>> headers
>>> + * from request/response buffer.
>>> + */
>>> +static inline void *smb2_get_msg(void *buf)
>>> +{
>>> +    return buf + 4;
>>> +}
>>> +
>>>   #endif    /* _SMB2PDU_H */
>>
>>
> Thanks,
> - Stefan
>
>
Hi Bethany,

Could you address Stefan's concerns? This patch has been sitting here 
for a while.

Roxana