| Message ID | 20210927124748.5614-1-linkinjeon@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ksmbd: remove the leftover of smb2.0 dialect support | expand |
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.
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. >
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; @@ -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); 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; @@ -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; 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); }
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(-)