2021-09-28 23:46 GMT+09:00, Tom Talpey <tom@xxxxxxxxxx>:
> 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@xxxxxxxxxx>
>> Cc: Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx>
>> Cc: Ralph Böhme <slow@xxxxxxxxx>
>> Cc: Steve French <smfrench@xxxxxxxxx>
>> Cc: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
>> Cc: Hyunchul Lee <hyc.lee@xxxxxxxxx>
>> Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx>
>> ---
>> 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.
>
