Re: [PATCH v6 01/14] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value

From: Ralph Boehme <slow@samba.org>
To: Namjae Jeon <linkinjeon@kernel.org>
Cc: linux-cifs@vger.kernel.org, Tom Talpey <tom@talpey.com>,
	Ronnie Sahlberg <ronniesahlberg@gmail.com>,
	Steve French <smfrench@gmail.com>,
	Sergey Senozhatsky <senozhatsky@chromium.org>,
	Hyunchul Lee <hyc.lee@gmail.com>
Subject: Re: [PATCH v6 01/14] ksmbd: add the check to vaildate if stream protocol length exceeds maximum value
Date: Tue, 5 Oct 2021 06:24:34 +0200	[thread overview]
Message-ID: <65d4498e-e0c0-2390-a8d3-e3d82e1fe2ec@samba.org> (raw)
In-Reply-To: <CAKYAXd8X_8nAvLXxna8kAHfce1jxz4-TH1j1h6g3ufjc7WpYXw@mail.gmail.com>

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

Am 03.10.21 um 03:18 schrieb Namjae Jeon:
> 2021-10-02 22:11 GMT+09:00, Ralph Boehme <slow@samba.org>:
>> From: Namjae Jeon <linkinjeon@kernel.org>
>>
>> This patch add MAX_STREAM_PROT_LEN macro and check if stream protocol
>> length exceeds maximum value. opencode pdu size check in
>> ksmbd_pdu_size_has_room().
>>
>> Cc: Tom Talpey <tom@talpey.com>
>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> Cc: Ralph Böhme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
>> Acked-by: Hyunchul Lee <hyc.lee@gmail.com>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>>   fs/ksmbd/connection.c | 9 +++++----
>>   fs/ksmbd/smb_common.c | 6 ------
>>   fs/ksmbd/smb_common.h | 4 ++--
>>   3 files changed, 7 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c
>> index af086d35398a..e50353c50661 100644
>> --- a/fs/ksmbd/connection.c
>> +++ b/fs/ksmbd/connection.c
>> @@ -296,10 +296,11 @@ int ksmbd_conn_handler_loop(void *p)
>>   		pdu_size = get_rfc1002_len(hdr_buf);
>>   		ksmbd_debug(CONN, "RFC1002 header %u bytes\n", pdu_size);
>>
>> -		/* make sure we have enough to get to SMB header end */
>> -		if (!ksmbd_pdu_size_has_room(pdu_size)) {
>> -			ksmbd_debug(CONN, "SMB request too short (%u bytes)\n",
>> -				    pdu_size);
>> +		/*
>> +		 * Check if pdu size is valid (min : smb header size,
>> +		 * max : 0x00FFFFFF).
>> +		 */
>> +		if (pdu_size > MAX_STREAM_PROT_LEN) {
> You changed this patch not to check header size check.

yes, on purpose. Because I though that your change was wrong and the 
whole logic should be done differently. Unfortunately I missed the fact 
that there are a bunch of other places where header data is accessed 
early, before reaching ksmbd_smb2_check_message() where I wanted to 
consolidate the checks.

> I think that
> this patch should be backed to original one.
> Because your patches are
> clean-up patches, not fixes.

there are two more invalid reads that the patchset address.

But agreed, I will now simply work my patches on-top of yours.

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 --]