diff mbox series

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

Message ID 20211002131212.130629-2-slow@samba.org (mailing list archive)
State New, archived
Headers show
Series Buffer validation patches | expand

Commit Message

Ralph Boehme Oct. 2, 2021, 1:11 p.m. UTC
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(-)

Comments

Namjae Jeon Oct. 3, 2021, 1:18 a.m. UTC | #1
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. I think that
this patch should be backed to original one. Because your patches are
clean-up patches, not fixes.
So you need to remove header size check here in "ksmbd: check PDU len
is at least header plus body size... ".

>  			continue;
>  		}
>
> diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
> index db8042a173d0..b6c4c7e960fa 100644
> --- a/fs/ksmbd/smb_common.c
> +++ b/fs/ksmbd/smb_common.c
> @@ -21,7 +21,6 @@ static const char basechars[43] =
> "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_-!@#$%";
>  #define MAGIC_CHAR '~'
>  #define PERIOD '.'
>  #define mangle(V) ((char)(basechars[(V) % MANGLE_BASE]))
> -#define KSMBD_MIN_SUPPORTED_HEADER_SIZE	(sizeof(struct smb2_hdr))
>
>  struct smb_protocol {
>  	int		index;
> @@ -294,11 +293,6 @@ int ksmbd_init_smb_server(struct ksmbd_work *work)
>  	return 0;
>  }
>
> -bool ksmbd_pdu_size_has_room(unsigned int pdu)
> -{
> -	return (pdu >= KSMBD_MIN_SUPPORTED_HEADER_SIZE - 4);
> -}
> -
>  int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work, int
> info_level,
>  				      struct ksmbd_file *dir,
>  				      struct ksmbd_dir_info *d_info,
> diff --git a/fs/ksmbd/smb_common.h b/fs/ksmbd/smb_common.h
> index 994abede27e9..6e79e7577f6b 100644
> --- a/fs/ksmbd/smb_common.h
> +++ b/fs/ksmbd/smb_common.h
> @@ -48,6 +48,8 @@
>  #define CIFS_DEFAULT_IOSIZE	(64 * 1024)
>  #define MAX_CIFS_SMALL_BUFFER_SIZE 448 /* big enough for most */
>
> +#define MAX_STREAM_PROT_LEN	0x00FFFFFF
> +
>  /* Responses when opening a file. */
>  #define F_SUPERSEDED	0
>  #define F_OPENED	1
> @@ -493,8 +495,6 @@ int ksmbd_lookup_dialect_by_id(__le16 *cli_dialects,
> __le16 dialects_count);
>
>  int ksmbd_init_smb_server(struct ksmbd_work *work);
>
> -bool ksmbd_pdu_size_has_room(unsigned int pdu);
> -
>  struct ksmbd_kstat;
>  int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work,
>  				      int info_level,
> --
> 2.31.1
>
>
Ralph Boehme Oct. 5, 2021, 4:24 a.m. UTC | #2
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
diff mbox series

Patch

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) {
 			continue;
 		}
 
diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
index db8042a173d0..b6c4c7e960fa 100644
--- a/fs/ksmbd/smb_common.c
+++ b/fs/ksmbd/smb_common.c
@@ -21,7 +21,6 @@  static const char basechars[43] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_-!@#$%";
 #define MAGIC_CHAR '~'
 #define PERIOD '.'
 #define mangle(V) ((char)(basechars[(V) % MANGLE_BASE]))
-#define KSMBD_MIN_SUPPORTED_HEADER_SIZE	(sizeof(struct smb2_hdr))
 
 struct smb_protocol {
 	int		index;
@@ -294,11 +293,6 @@  int ksmbd_init_smb_server(struct ksmbd_work *work)
 	return 0;
 }
 
-bool ksmbd_pdu_size_has_room(unsigned int pdu)
-{
-	return (pdu >= KSMBD_MIN_SUPPORTED_HEADER_SIZE - 4);
-}
-
 int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work, int info_level,
 				      struct ksmbd_file *dir,
 				      struct ksmbd_dir_info *d_info,
diff --git a/fs/ksmbd/smb_common.h b/fs/ksmbd/smb_common.h
index 994abede27e9..6e79e7577f6b 100644
--- a/fs/ksmbd/smb_common.h
+++ b/fs/ksmbd/smb_common.h
@@ -48,6 +48,8 @@ 
 #define CIFS_DEFAULT_IOSIZE	(64 * 1024)
 #define MAX_CIFS_SMALL_BUFFER_SIZE 448 /* big enough for most */
 
+#define MAX_STREAM_PROT_LEN	0x00FFFFFF
+
 /* Responses when opening a file. */
 #define F_SUPERSEDED	0
 #define F_OPENED	1
@@ -493,8 +495,6 @@  int ksmbd_lookup_dialect_by_id(__le16 *cli_dialects, __le16 dialects_count);
 
 int ksmbd_init_smb_server(struct ksmbd_work *work);
 
-bool ksmbd_pdu_size_has_room(unsigned int pdu);
-
 struct ksmbd_kstat;
 int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work,
 				      int info_level,