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

Tom Talpey
2021-09-28T14:46:43Z

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.