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

[Stefan Bader]

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.


> 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 */

-- 
- Stefan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0xE8675DEECBEECEA3.asc
Type: application/pgp-keys
Size: 48643 bytes
Desc: OpenPGP public key
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20240126/8393d194/attachment-0001.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20240126/8393d194/attachment-0001.sig>