[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-cifs
Subject:    Re: [PATCH 1/2] ksmbd: remove the leftover of smb2.0 dialect support
From:       Namjae Jeon <linkinjeon () kernel ! org>
Date:       2021-09-29 1:40:38
Message-ID: CAKYAXd_5fK8c09N0G6fu=-LXFhGBES4FgWgBWONcCh=zXuDdOw () mail ! gmail ! com
[Download RAW message or body]

2021-09-28 23:46 GMT+09:00, 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;
>
Hi Tom,

> 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.
Will fix.
>
>
>> @@ -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?
Yep, Will remove this condition.
>
>>   		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???
Will remove it.
>
>> @@ -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.
Will remove it.
>
>> 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?
Will use this helper.

Thanks for your review!
>
> Tom.
>
[prev in list] [next in list] [prev in thread] [next in thread]