diff mbox series

smb3: remove ->validate_negotiate server op

Message ID 20220830235119.16991-1-ematsumiya@suse.de
State New
Headers show
Series smb3: remove ->validate_negotiate server op | expand

Commit Message

Enzo Matsumiya Aug. 30, 2022, 11:51 p.m. UTC
Since only SMB 3.0 and 3.0.2 uses it, and they use the same operations
struct, remove the ->validate_negotiate server op and just check for
server->dialect on the only caller (SMB2_tcon()).

- rename smb3_validate_negotiate() to smb30_validate_negotiate() to be
  more explict
- remove check for SMB311_PROT_ID since it's unreachable anyway
- simplify dialect counting by using a counter

Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
---
 fs/cifs/cifsglob.h  |  1 -
 fs/cifs/smb2ops.c   |  2 --
 fs/cifs/smb2pdu.c   | 62 ++++++++++++++++++++++-----------------------
 fs/cifs/smb2proto.h |  1 -
 4 files changed, 30 insertions(+), 36 deletions(-)

Comments

Enzo Matsumiya Aug. 31, 2022, 1:29 p.m. UTC | #1
On 08/30, Enzo Matsumiya wrote:
>Since only SMB 3.0 and 3.0.2 uses it, and they use the same operations
>struct, remove the ->validate_negotiate server op and just check for
>server->dialect on the only caller (SMB2_tcon()).
>
>- rename smb3_validate_negotiate() to smb30_validate_negotiate() to be
>  more explict
>- remove check for SMB311_PROT_ID since it's unreachable anyway
>- simplify dialect counting by using a counter
>
>Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
>---
> fs/cifs/cifsglob.h  |  1 -
> fs/cifs/smb2ops.c   |  2 --
> fs/cifs/smb2pdu.c   | 62 ++++++++++++++++++++++-----------------------
> fs/cifs/smb2proto.h |  1 -
> 4 files changed, 30 insertions(+), 36 deletions(-)
> 
> ... snip ...
> 
>@@ -1143,35 +1145,30 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
> 	else
> 		pneg_inbuf->SecurityMode = 0;
>
>-
>-	if (strcmp(server->vals->version_string,
>-		SMB3ANY_VERSION_STRING) == 0) {
>-		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
>-		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
>-		pneg_inbuf->Dialects[2] = cpu_to_le16(SMB311_PROT_ID);
>-		pneg_inbuf->DialectCount = cpu_to_le16(3);
>-		/* SMB 2.1 not included so subtract one dialect from len */
>-		inbuflen = sizeof(*pneg_inbuf) -
>-				(sizeof(pneg_inbuf->Dialects[0]));
>-	} else if (strcmp(server->vals->version_string,
>-		SMBDEFAULT_VERSION_STRING) == 0) {
>-		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
>-		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
>-		pneg_inbuf->Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
>-		pneg_inbuf->Dialects[3] = cpu_to_le16(SMB311_PROT_ID);
>-		pneg_inbuf->DialectCount = cpu_to_le16(4);
>+	i = 0;
>+	if (!strcmp(server->vals->version_string, SMB3ANY_VERSION_STRING)) {
>+		pneg_inbuf->Dialects[i++] = cpu_to_le16(SMB30_PROT_ID);
>+		pneg_inbuf->Dialects[i++] = cpu_to_le16(SMB302_PROT_ID);
>+		pneg_inbuf->Dialects[i++] = cpu_to_le16(SMB311_PROT_ID);
>+		/* SMB 2.1 not included */
>+	} else if (!strcmp(server->vals->version_string, SMBDEFAULT_VERSION_STRING)) {
>+		pneg_inbuf->Dialects[i++] = cpu_to_le16(SMB21_PROT_ID);
>+		pneg_inbuf->Dialects[i++] = cpu_to_le16(SMB30_PROT_ID);
>+		pneg_inbuf->Dialects[i++] = cpu_to_le16(SMB302_PROT_ID);
>+		pneg_inbuf->Dialects[i++] = cpu_to_le16(SMB311_PROT_ID);
> 		/* structure is big enough for 4 dialects */
>-		inbuflen = sizeof(*pneg_inbuf);
> 	} else {
> 		/* otherwise specific dialect was requested */
>-		pneg_inbuf->Dialects[0] =
>-			cpu_to_le16(server->vals->protocol_id);
>-		pneg_inbuf->DialectCount = cpu_to_le16(1);
>-		/* structure is big enough for 3 dialects, sending only 1 */
>-		inbuflen = sizeof(*pneg_inbuf) -
>-				sizeof(pneg_inbuf->Dialects[0]) * 2;
>+		pneg_inbuf->Dialects[i++] = cpu_to_le16(server->vals->protocol_id);
> 	}
>
>+	pneg_inbuf->DialectCount = cpu_to_le16(i);
>+	/*
>+	 * The structure holds 4 dialects at most, so subtract the number of dialects not added,
>+	 * if any.
>+	 */
>+	inbuflen = sizeof(*pneg_inbuf) - (sizeof(pneg_inbuf->Dialects[0]) * (4 - i));
>+
> 	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
> 		FSCTL_VALIDATE_NEGOTIATE_INFO,
> 		(char *)pneg_inbuf, inbuflen, CIFSMaxBufSize,

The hunk above will conflict with Zhang's patch
"[PATCH v2 1/3] cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message",
but this way is, IMHO, safer for future modifications because it doesn't use
hardcoded values.

It might actually make sense to even define a "SMB2_MAX_DIALECTS" to 4
here. Let me know what you think.


Cheers,

Enzo
diff mbox series

Patch

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index ae7f571a7dba..69067111ec66 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -474,7 +474,6 @@  struct smb_version_operations {
 	int (*duplicate_extents)(const unsigned int, struct cifsFileInfo *src,
 			struct cifsFileInfo *target_file, u64 src_off, u64 len,
 			u64 dest_off);
-	int (*validate_negotiate)(const unsigned int, struct cifs_tcon *);
 	ssize_t (*query_all_EAs)(const unsigned int, struct cifs_tcon *,
 			const unsigned char *, const unsigned char *, char *,
 			size_t, struct cifs_sb_info *);
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index f1d36b70cef7..4c5e3866a952 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -5479,7 +5479,6 @@  struct smb_version_operations smb30_operations = {
 	.parse_lease_buf = smb3_parse_lease_buf,
 	.copychunk_range = smb2_copychunk_range,
 	.duplicate_extents = smb2_duplicate_extents,
-	.validate_negotiate = smb3_validate_negotiate,
 	.wp_retry_size = smb2_wp_retry_size,
 	.dir_needs_close = smb2_dir_needs_close,
 	.fallocate = smb3_fallocate,
@@ -5593,7 +5592,6 @@  struct smb_version_operations smb311_operations = {
 	.parse_lease_buf = smb3_parse_lease_buf,
 	.copychunk_range = smb2_copychunk_range,
 	.duplicate_extents = smb2_duplicate_extents,
-/*	.validate_negotiate = smb3_validate_negotiate, */ /* not used in 3.11 */
 	.wp_retry_size = smb2_wp_retry_size,
 	.dir_needs_close = smb2_dir_needs_close,
 	.fallocate = smb3_fallocate,
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 128e44e57528..bf6460c4c8bf 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1089,9 +1089,15 @@  SMB2_negotiate(const unsigned int xid,
 	return rc;
 }
 
-int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
+/*
+ * Validate negotiate is only supported in SMB 3.0 and 3.0.2.
+ * In SMB 3.1.1, preauth integrity supersedes it.
+ *
+ * See MS-SMB2 2.2.31.4
+ */
+static int smb30_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 {
-	int rc;
+	int i, rc;
 	struct validate_negotiate_info_req *pneg_inbuf;
 	struct validate_negotiate_info_rsp *pneg_rsp = NULL;
 	u32 rsplen;
@@ -1100,10 +1106,6 @@  int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 
 	cifs_dbg(FYI, "validate negotiate\n");
 
-	/* In SMB3.11 preauth integrity supersedes validate negotiate */
-	if (server->dialect == SMB311_PROT_ID)
-		return 0;
-
 	/*
 	 * validation ioctl must be signed, so no point sending this if we
 	 * can not sign it (ie are not known user).  Even if signing is not
@@ -1143,35 +1145,30 @@  int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 	else
 		pneg_inbuf->SecurityMode = 0;
 
-
-	if (strcmp(server->vals->version_string,
-		SMB3ANY_VERSION_STRING) == 0) {
-		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
-		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
-		pneg_inbuf->Dialects[2] = cpu_to_le16(SMB311_PROT_ID);
-		pneg_inbuf->DialectCount = cpu_to_le16(3);
-		/* SMB 2.1 not included so subtract one dialect from len */
-		inbuflen = sizeof(*pneg_inbuf) -
-				(sizeof(pneg_inbuf->Dialects[0]));
-	} else if (strcmp(server->vals->version_string,
-		SMBDEFAULT_VERSION_STRING) == 0) {
-		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
-		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
-		pneg_inbuf->Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
-		pneg_inbuf->Dialects[3] = cpu_to_le16(SMB311_PROT_ID);
-		pneg_inbuf->DialectCount = cpu_to_le16(4);
+	i = 0;
+	if (!strcmp(server->vals->version_string, SMB3ANY_VERSION_STRING)) {
+		pneg_inbuf->Dialects[i++] = cpu_to_le16(SMB30_PROT_ID);
+		pneg_inbuf->Dialects[i++] = cpu_to_le16(SMB302_PROT_ID);
+		pneg_inbuf->Dialects[i++] = cpu_to_le16(SMB311_PROT_ID);
+		/* SMB 2.1 not included */
+	} else if (!strcmp(server->vals->version_string, SMBDEFAULT_VERSION_STRING)) {
+		pneg_inbuf->Dialects[i++] = cpu_to_le16(SMB21_PROT_ID);
+		pneg_inbuf->Dialects[i++] = cpu_to_le16(SMB30_PROT_ID);
+		pneg_inbuf->Dialects[i++] = cpu_to_le16(SMB302_PROT_ID);
+		pneg_inbuf->Dialects[i++] = cpu_to_le16(SMB311_PROT_ID);
 		/* structure is big enough for 4 dialects */
-		inbuflen = sizeof(*pneg_inbuf);
 	} else {
 		/* otherwise specific dialect was requested */
-		pneg_inbuf->Dialects[0] =
-			cpu_to_le16(server->vals->protocol_id);
-		pneg_inbuf->DialectCount = cpu_to_le16(1);
-		/* structure is big enough for 3 dialects, sending only 1 */
-		inbuflen = sizeof(*pneg_inbuf) -
-				sizeof(pneg_inbuf->Dialects[0]) * 2;
+		pneg_inbuf->Dialects[i++] = cpu_to_le16(server->vals->protocol_id);
 	}
 
+	pneg_inbuf->DialectCount = cpu_to_le16(i);
+	/*
+	 * The structure holds 4 dialects at most, so subtract the number of dialects not added,
+	 * if any.
+	 */
+	inbuflen = sizeof(*pneg_inbuf) - (sizeof(pneg_inbuf->Dialects[0]) * (4 - i));
+
 	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
 		FSCTL_VALIDATE_NEGOTIATE_INFO,
 		(char *)pneg_inbuf, inbuflen, CIFSMaxBufSize,
@@ -1939,8 +1936,9 @@  SMB2_tcon(const unsigned int xid, struct cifs_ses *ses, const char *tree,
 		cifs_tcon_dbg(VFS, "Encryption is requested but not supported\n");
 
 	init_copy_chunk_defaults(tcon);
-	if (server->ops->validate_negotiate)
-		rc = server->ops->validate_negotiate(xid, tcon);
+
+	if (server->dialect == SMB30_PROT_ID || server->dialect == SMB302_PROT_ID)
+		rc = smb30_validate_negotiate(xid, tcon);
 tcon_exit:
 
 	free_rsp_buf(resp_buftype, rsp);
diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
index 3f740f24b96a..da498d044d8b 100644
--- a/fs/cifs/smb2proto.h
+++ b/fs/cifs/smb2proto.h
@@ -248,7 +248,6 @@  extern int smb2_lockv(const unsigned int xid, struct cifs_tcon *tcon,
 		      struct smb2_lock_element *buf);
 extern int SMB2_lease_break(const unsigned int xid, struct cifs_tcon *tcon,
 			    __u8 *lease_key, const __le32 lease_state);
-extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *);
 
 extern enum securityEnum smb2_select_sectype(struct TCP_Server_Info *,
 					enum securityEnum);