Re: [PATCH 1/2] ksmbd: remove the leftover of smb2.0 dialect support

[Tom Talpey <tom@talpey.com>]

On 9/27/2021 8:47 AM, Namjae Jeon wrote:
> Although ksmbd doesn't send SMB2.0 support in supported dialect list of smb
> negotiate response, There is the leftover of smb2.0 dialect.
> This patch remove it not to support SMB2.0 in ksmbd.
> 
> 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>
> Cc: Hyunchul Lee <hyc.lee@gmail.com>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>   fs/ksmbd/smb2ops.c    |  5 -----
>   fs/ksmbd/smb2pdu.c    | 15 ++++-----------
>   fs/ksmbd/smb2pdu.h    |  1 -
>   fs/ksmbd/smb_common.c |  4 ++--
>   4 files changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/ksmbd/smb2ops.c b/fs/ksmbd/smb2ops.c
> index 197473871aa4..b06456eb587b 100644
> --- a/fs/ksmbd/smb2ops.c
> +++ b/fs/ksmbd/smb2ops.c
> @@ -187,11 +187,6 @@ static struct smb_version_cmds smb2_0_server_cmds[NUMBER_OF_SMB2_COMMANDS] = {
>   	[SMB2_CHANGE_NOTIFY_HE]	=	{ .proc = smb2_notify},
>   };
>   
> -int init_smb2_0_server(struct ksmbd_conn *conn)
> -{
> -	return -EOPNOTSUPP;
> -}
> -
>   /**
>    * init_smb2_1_server() - initialize a smb server connection with smb2.1
>    *			command dispatcher
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 88e94a8e4a15..b7d0406d1a14 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -236,7 +236,7 @@ int init_smb2_neg_rsp(struct ksmbd_work *work)
>   
>   	if (conn->need_neg == false)
>   		return -EINVAL;
> -	if (!(conn->dialect >= SMB20_PROT_ID &&
> +	if (!(conn->dialect >= SMB21_PROT_ID &&
>   	      conn->dialect <= SMB311_PROT_ID))
>   		return -EINVAL;

This would accept any in-between value! That, um, would be bad.

This needs to be a much stronger check, especially since significant
state is being built in the lines that follow this segment.


> @@ -1166,13 +1166,6 @@ int smb2_handle_negotiate(struct ksmbd_work *work)
>   	case SMB21_PROT_ID:
>   		init_smb2_1_server(conn);
>   		break;
> -	case SMB20_PROT_ID:
> -		rc = init_smb2_0_server(conn);
> -		if (rc) {
> -			rsp->hdr.Status = STATUS_NOT_SUPPORTED;
> -			goto err_out;
> -		}
> -		break;
>   	case SMB2X_PROT_ID:
>   	case BAD_PROT_ID:
>   	default:
> @@ -1191,7 +1184,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work)
>   	rsp->MaxReadSize = cpu_to_le32(conn->vals->max_read_size);
>   	rsp->MaxWriteSize = cpu_to_le32(conn->vals->max_write_size);
>   
> -	if (conn->dialect > SMB20_PROT_ID) {
> +	if (conn->dialect >= SMB21_PROT_ID) {
>   		memcpy(conn->ClientGUID, req->ClientGUID,
>   		       SMB2_CLIENT_GUID_SIZE);

If SMB2.1 is now the minimum supported dialect, why is this GUID
insertion made conditional?

>   		conn->cli_sec_mode = le16_to_cpu(req->SecurityMode);
> @@ -1537,7 +1530,7 @@ static int ntlm_authenticate(struct ksmbd_work *work)
>   		}
>   	}
>   
> -	if (conn->dialect > SMB20_PROT_ID) {
> +	if (conn->dialect >= SMB21_PROT_ID) {
>   		if (!ksmbd_conn_lookup_dialect(conn)) {
>   			pr_err("fail to verify the dialect\n");
>   			return -ENOENT;

Why is verifying the dialect *ever* conditional on the dialect value???

> @@ -1623,7 +1616,7 @@ static int krb5_authenticate(struct ksmbd_work *work)
>   		}
>   	}
>   
> -	if (conn->dialect > SMB20_PROT_ID) {
> +	if (conn->dialect >= SMB21_PROT_ID) {
>   		if (!ksmbd_conn_lookup_dialect(conn)) {
>   			pr_err("fail to verify the dialect\n");
>   			return -ENOENT;

Ditto previous comment.

> diff --git a/fs/ksmbd/smb2pdu.h b/fs/ksmbd/smb2pdu.h
> index 261825d06391..a6dec5ec6a54 100644
> --- a/fs/ksmbd/smb2pdu.h
> +++ b/fs/ksmbd/smb2pdu.h
> @@ -1637,7 +1637,6 @@ struct smb2_posix_info {
>   } __packed;
>   
>   /* functions */
> -int init_smb2_0_server(struct ksmbd_conn *conn);
>   void init_smb2_1_server(struct ksmbd_conn *conn);
>   void init_smb3_0_server(struct ksmbd_conn *conn);
>   void init_smb3_02_server(struct ksmbd_conn *conn);
> diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c
> index b6c4c7e960fa..435ca8df590b 100644
> --- a/fs/ksmbd/smb_common.c
> +++ b/fs/ksmbd/smb_common.c
> @@ -88,7 +88,7 @@ unsigned int ksmbd_server_side_copy_max_total_size(void)
>   
>   inline int ksmbd_min_protocol(void)
>   {
> -	return SMB2_PROT;
> +	return SMB21_PROT;
>   }
>   
>   inline int ksmbd_max_protocol(void)
> @@ -427,7 +427,7 @@ int ksmbd_extract_shortname(struct ksmbd_conn *conn, const char *longname,
>   
>   static int __smb2_negotiate(struct ksmbd_conn *conn)
>   {
> -	return (conn->dialect >= SMB20_PROT_ID &&
> +	return (conn->dialect >= SMB21_PROT_ID &&
>   		conn->dialect <= SMB311_PROT_ID);
>   }

Ditto previous comment. And after fixing it, why is this helper not used
everywhere?

Tom.