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

From: Namjae Jeon <linkinjeon@kernel.org>
To: Ralph Boehme <slow@samba.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: Sun, 3 Oct 2021 10:18:39 +0900	[thread overview]
Message-ID: <CAKYAXd8X_8nAvLXxna8kAHfce1jxz4-TH1j1h6g3ufjc7WpYXw@mail.gmail.com> (raw)
In-Reply-To: <20211002131212.130629-2-slow@samba.org>

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