Skip to contentSkip to conversation

cifs: Allocate validate negotiation request through kmalloc

PATCH Superseded Long Li 4 participants 6 messages Apr 19, 2018 – Apr 20, 2018

Subsystems (3)
InfiniBand SMB client (CIFS) Kernel SMB3 server (ksmbd)

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Long Li

author
#0 lore raw

PATCH v4 1/1 cifs: Allocate validate negotiation request through kmalloc

From: Long Li <[email protected]>

The data buffer allocated on the stack can't be DMA'ed, ib_dma_map_page will return an invalid DMA address for a buffer on stack. Even worse, this incorrect address can't be detected by ib_dma_mapping_error. Sending data from this address to hardware will not fail, but the remote peer will get junk data.

Fix this by allocating the request on the heap in smb3_validate_negotiate.

Changes in v2:
Removed duplicated code on freeing buffers on function exit. (Thanks to Parav Pandit <[email protected]>) Fixed typo in the patch title.

Changes in v3:
Added "Fixes" to the patch.
Changed several sizeof() to use *pointer in place of struct.

Changes in v4:
Added detailed comments on the failure through RDMA. Allocate request buffer using GPF_NOFS. Fixed possible memory leak.

Fixes ff1c038addc4 ("Check SMB3 dialects against downgrade attacks")
Signed-off-by Long Li <[email protected]>
---
 fs/cifs/smb2pdu.c | 61 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 28 deletions(-)
fs/cifs/smb2pdu.c +33 -28
index 0f044c4..caa2a1e 100644
@@ -729,8 +729,8 @@ SMB2_negotiate(const unsigned int xid, struct cifs_ses *ses)
 
 int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 {
-	int rc = 0;
-	struct validate_negotiate_info_req vneg_inbuf;
+	int ret, rc = -EIO;
+	struct validate_negotiate_info_req *pneg_inbuf;
 	struct validate_negotiate_info_rsp *pneg_rsp = NULL;
 	u32 rsplen;
 	u32 inbuflen; /* max of 4 dialects */

@@ -741,7 +741,6 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 	if (tcon->ses->server->rdma)
 		return 0;
 #endif
-
 	/* In SMB3.11 preauth integrity supersedes validate negotiate */
 	if (tcon->ses->server->dialect == SMB311_PROT_ID)
 		return 0;

@@ -764,63 +763,67 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 	if (tcon->ses->session_flags & SMB2_SESSION_FLAG_IS_NULL)
 		cifs_dbg(VFS, "Unexpected null user (anonymous) auth flag sent by server\n");
 
-	vneg_inbuf.Capabilities =
+	pneg_inbuf = kmalloc(sizeof(*pneg_inbuf), GFP_NOFS);
+	if (!pneg_inbuf)
+		return -ENOMEM;
+
+	pneg_inbuf->Capabilities =
 			cpu_to_le32(tcon->ses->server->vals->req_capabilities);
-	memcpy(vneg_inbuf.Guid, tcon->ses->server->client_guid,
+	memcpy(pneg_inbuf->Guid, tcon->ses->server->client_guid,
 					SMB2_CLIENT_GUID_SIZE);
 
 	if (tcon->ses->sign)
-		vneg_inbuf.SecurityMode =
+		pneg_inbuf->SecurityMode =
 			cpu_to_le16(SMB2_NEGOTIATE_SIGNING_REQUIRED);
 	else if (global_secflags & CIFSSEC_MAY_SIGN)
-		vneg_inbuf.SecurityMode =
+		pneg_inbuf->SecurityMode =
 			cpu_to_le16(SMB2_NEGOTIATE_SIGNING_ENABLED);
 	else
-		vneg_inbuf.SecurityMode = 0;
+		pneg_inbuf->SecurityMode = 0;
 
 
 	if (strcmp(tcon->ses->server->vals->version_string,
 		SMB3ANY_VERSION_STRING) == 0) {
-		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
-		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
-		vneg_inbuf.DialectCount = cpu_to_le16(2);
+		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
+		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
+		pneg_inbuf->DialectCount = cpu_to_le16(2);
 		/* structure is big enough for 3 dialects, sending only 2 */
 		inbuflen = sizeof(struct validate_negotiate_info_req) - 2;
 	} else if (strcmp(tcon->ses->server->vals->version_string,
 		SMBDEFAULT_VERSION_STRING) == 0) {
-		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
-		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
-		vneg_inbuf.Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
-		vneg_inbuf.DialectCount = cpu_to_le16(3);
+		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->DialectCount = cpu_to_le16(3);
 		/* structure is big enough for 3 dialects */
 		inbuflen = sizeof(struct validate_negotiate_info_req);
 	} else {
 		/* otherwise specific dialect was requested */
-		vneg_inbuf.Dialects[0] =
+		pneg_inbuf->Dialects[0] =
 			cpu_to_le16(tcon->ses->server->vals->protocol_id);
-		vneg_inbuf.DialectCount = cpu_to_le16(1);
+		pneg_inbuf->DialectCount = cpu_to_le16(1);
 		/* structure is big enough for 3 dialects, sending only 1 */
 		inbuflen = sizeof(struct validate_negotiate_info_req) - 4;
 	}
 
-	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
+	ret = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
 		FSCTL_VALIDATE_NEGOTIATE_INFO, true /* is_fsctl */,
-		(char *)&vneg_inbuf, sizeof(struct validate_negotiate_info_req),
+		(char *)pneg_inbuf, sizeof(*pneg_inbuf),
 		(char **)&pneg_rsp, &rsplen);
 
-	if (rc != 0) {
-		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", rc);
-		return -EIO;
+	if (ret) {
+		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", ret);
+		goto out_free_inbuf;
 	}
 
-	if (rsplen != sizeof(struct validate_negotiate_info_rsp)) {
+	if (rsplen != sizeof(*pneg_rsp)) {
 		cifs_dbg(VFS, "invalid protocol negotiate response size: %d\n",
 			 rsplen);
 
 		/* relax check since Mac returns max bufsize allowed on ioctl */
 		if ((rsplen > CIFSMaxBufSize)
 		     || (rsplen < sizeof(struct validate_negotiate_info_rsp)))
-			goto err_rsp_free;
+			goto out_free_rsp;
 	}
 
 	/* check validate negotiate info response matches what we got earlier */

@@ -838,14 +841,16 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 
 	/* validate negotiate successful */
 	cifs_dbg(FYI, "validate negotiate info successful\n");
-	kfree(pneg_rsp);
-	return 0;
+	rc = 0;
+	goto out_free_rsp;
 
 vneg_out:
 	cifs_dbg(VFS, "protocol revalidation - security settings mismatch\n");
-err_rsp_free:
+out_free_rsp:
 	kfree(pneg_rsp);
-	return -EIO;
+out_free_inbuf:
+	kfree(pneg_inbuf);
+	return rc;
 }
 
 enum securityEnum
Signature · 1 line
2.7.4
reviewer
#1 lore raw

Looks good, but I have two possibly style-related comments.

Quoted 35 lines · Long Li

From: Long Li <[email protected]>

The data buffer allocated on the stack can't be DMA'ed, ib_dma_map_page will return an invalid DMA address for a buffer on stack. Even worse, this incorrect address can't be detected by ib_dma_mapping_error. Sending data from this address to hardware will not fail, but the remote peer will get junk data.

Fix this by allocating the request on the heap in smb3_validate_negotiate.

Changes in v2:
Removed duplicated code on freeing buffers on function exit. (Thanks to Parav Pandit <[email protected]>) Fixed typo in the patch title.

Changes in v3:
Added "Fixes" to the patch.
Changed several sizeof() to use *pointer in place of struct.

Changes in v4:
Added detailed comments on the failure through RDMA. Allocate request buffer using GPF_NOFS. Fixed possible memory leak.

Fixes ff1c038addc4 ("Check SMB3 dialects against downgrade attacks")
Signed-off-by Long Li <[email protected]>
---
  fs/cifs/smb2pdu.c | 61 ++++++++++++++++++++++++++++++-------------------------
  1 file changed, 33 insertions(+), 28 deletions(-)
fs/cifs/smb2pdu.c +0 -0
index 0f044c4..caa2a1e 100644

fs/cifs/smb2pdu.c SMB2_negotiate(const unsigned int xid, struct cifs_ses *ses) · line 732

Quoted 6 lines
fs/cifs/smb2pdu.c +1 -2
@@ -729,8 +729,8 @@ SMB2_negotiate(const unsigned int xid, struct cifs_ses *ses)
 
  int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
  {
-	int rc = 0;
-	struct validate_negotiate_info_req vneg_inbuf;
+	int ret, rc = -EIO;

Seems awkward to have "rc" and "ret", and based on the code below I don't see why two variables are needed. Simplify? (see later comment)

Quoted 40 lines
fs/cifs/smb2pdu.c +10 -6
+	struct validate_negotiate_info_req *pneg_inbuf;
  	struct validate_negotiate_info_rsp *pneg_rsp = NULL;
  	u32 rsplen;
  	u32 inbuflen; /* max of 4 dialects */

@@ -741,7 +741,6 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
  	if (tcon->ses->server->rdma)
  		return 0;
  #endif
-
  	/* In SMB3.11 preauth integrity supersedes validate negotiate */
  	if (tcon->ses->server->dialect == SMB311_PROT_ID)
  		return 0;

@@ -764,63 +763,67 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
  	if (tcon->ses->session_flags & SMB2_SESSION_FLAG_IS_NULL)
  		cifs_dbg(VFS, "Unexpected null user (anonymous) auth flag sent by server\n");
 
-	vneg_inbuf.Capabilities =
+	pneg_inbuf = kmalloc(sizeof(*pneg_inbuf), GFP_NOFS);
+	if (!pneg_inbuf)
+		return -ENOMEM;
+
+	pneg_inbuf->Capabilities =
  			cpu_to_le32(tcon->ses->server->vals->req_capabilities);
-	memcpy(vneg_inbuf.Guid, tcon->ses->server->client_guid,
+	memcpy(pneg_inbuf->Guid, tcon->ses->server->client_guid,
  					SMB2_CLIENT_GUID_SIZE);
 
  	if (tcon->ses->sign)
-		vneg_inbuf.SecurityMode =
+		pneg_inbuf->SecurityMode =
  			cpu_to_le16(SMB2_NEGOTIATE_SIGNING_REQUIRED);
  	else if (global_secflags & CIFSSEC_MAY_SIGN)
-		vneg_inbuf.SecurityMode =
+		pneg_inbuf->SecurityMode =
  			cpu_to_le16(SMB2_NEGOTIATE_SIGNING_ENABLED);
  	else
-		vneg_inbuf.SecurityMode = 0;
+		pneg_inbuf->SecurityMode = 0;

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 791

Quoted 10 lines
fs/cifs/smb2pdu.c +3 -3
  	if (strcmp(tcon->ses->server->vals->version_string,
  		SMB3ANY_VERSION_STRING) == 0) {
-		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
-		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
-		vneg_inbuf.DialectCount = cpu_to_le16(2);
+		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
+		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
+		pneg_inbuf->DialectCount = cpu_to_le16(2);
  		/* structure is big enough for 3 dialects, sending only 2 */
  		inbuflen = sizeof(struct validate_negotiate_info_req) - 2;

The "- 2" is a little confusing here. This was existing code, but I suggest you change this to a sizeof (u16) construct for consistency. It's reducing by the length of a single Dialects[n] entry.

Quoted 11 lines
fs/cifs/smb2pdu.c +4 -4
  	} else if (strcmp(tcon->ses->server->vals->version_string,
  		SMBDEFAULT_VERSION_STRING) == 0) {
-		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
-		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
-		vneg_inbuf.Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
-		vneg_inbuf.DialectCount = cpu_to_le16(3);
+		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->DialectCount = cpu_to_le16(3);
  		/* structure is big enough for 3 dialects */

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 806

Quoted 10 lines
fs/cifs/smb2pdu.c +2 -2
  		inbuflen = sizeof(struct validate_negotiate_info_req);
  	} else {
  		/* otherwise specific dialect was requested */
-		vneg_inbuf.Dialects[0] =
+		pneg_inbuf->Dialects[0] =
  			cpu_to_le16(tcon->ses->server->vals->protocol_id);
-		vneg_inbuf.DialectCount = cpu_to_le16(1);
+		pneg_inbuf->DialectCount = cpu_to_le16(1);
  		/* structure is big enough for 3 dialects, sending only 1 */
  		inbuflen = sizeof(struct validate_negotiate_info_req) - 4;

Ditto previous sizeof (u16) comment, with a *2 this case.

Quoted 6 lines
fs/cifs/smb2pdu.c +1 -2
  	}
 
-	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
+	ret = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
  		FSCTL_VALIDATE_NEGOTIATE_INFO, true /* is_fsctl */,
-		(char *)&vneg_inbuf, sizeof(struct validate_negotiate_info_req),

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 817

Quoted 10 lines
fs/cifs/smb2pdu.c +4 -3
+		(char *)pneg_inbuf, sizeof(*pneg_inbuf),
  		(char **)&pneg_rsp, &rsplen);
 
-	if (rc != 0) {
-		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", rc);
-		return -EIO;
+	if (ret) {
+		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", ret);
+		goto out_free_inbuf;
  	}

Why not leave "rc" alone, and set its value to -EIO before the goto if the ioctl fails? That will simplify and make the code much more readable IMO.

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 827

Quoted 12 lines
fs/cifs/smb2pdu.c +2 -2
-	if (rsplen != sizeof(struct validate_negotiate_info_rsp)) {
+	if (rsplen != sizeof(*pneg_rsp)) {
  		cifs_dbg(VFS, "invalid protocol negotiate response size: %d\n",
  			 rsplen);
 
  		/* relax check since Mac returns max bufsize allowed on ioctl */
  		if ((rsplen > CIFSMaxBufSize)
  		     || (rsplen < sizeof(struct validate_negotiate_info_rsp)))
-			goto err_rsp_free;
+			goto out_free_rsp;
  	}

Would need an rc = -EIO here too if above comment is accepted.

Tom.

Quoted 24 lines
fs/cifs/smb2pdu.c +6 -4
  	/* check validate negotiate info response matches what we got earlier */

@@ -838,14 +841,16 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 
  	/* validate negotiate successful */
  	cifs_dbg(FYI, "validate negotiate info successful\n");
-	kfree(pneg_rsp);
-	return 0;
+	rc = 0;
+	goto out_free_rsp;
 
  vneg_out:
  	cifs_dbg(VFS, "protocol revalidation - security settings mismatch\n");
-err_rsp_free:
+out_free_rsp:
  	kfree(pneg_rsp);
-	return -EIO;
+out_free_inbuf:
+	kfree(pneg_inbuf);
+	return rc;
  }
 
  enum securityEnum
#2 lore raw

2018-04-20 7:55 GMT-07:00 Tom Talpey <[email protected]>:

Quoted 103 lines

Looks good, but I have two possibly style-related comments.

Quoted 38 lines · Long Li

From: Long Li <[email protected]>

The data buffer allocated on the stack can't be DMA'ed, ib_dma_map_page will
return an invalid DMA address for a buffer on stack. Even worse, this incorrect address can't be detected by ib_dma_mapping_error. Sending data from this address to hardware will not fail, but the remote peer will get junk data.

Fix this by allocating the request on the heap in smb3_validate_negotiate.

Changes in v2:
Removed duplicated code on freeing buffers on function exit. (Thanks to Parav Pandit <[email protected]>) Fixed typo in the patch title.

Changes in v3:
Added "Fixes" to the patch.
Changed several sizeof() to use *pointer in place of struct.

Changes in v4:
Added detailed comments on the failure through RDMA. Allocate request buffer using GPF_NOFS. Fixed possible memory leak.

Fixes ff1c038addc4 ("Check SMB3 dialects against downgrade attacks")
Signed-off-by Long Li <[email protected]>
---
  fs/cifs/smb2pdu.c | 61
++++++++++++++++++++++++++++++-------------------------
  1 file changed, 33 insertions(+), 28 deletions(-)
fs/cifs/smb2pdu.c +0 -0
index 0f044c4..caa2a1e 100644

fs/cifs/smb2pdu.c SMB2_negotiate(const unsigned int xid, struct cifs_ses · line 733

Quoted 7 lines
fs/cifs/smb2pdu.c +1 -2
@@ -729,8 +729,8 @@ SMB2_negotiate(const unsigned int xid, struct cifs_ses
 *ses)
    int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon
 *tcon)
  {
-       int rc = 0;
-       struct validate_negotiate_info_req vneg_inbuf;
+       int ret, rc = -EIO;

Seems awkward to have "rc" and "ret", and based on the code below I don't see why two variables are needed. Simplify? (see later comment)

Quoted 48 lines
fs/cifs/smb2pdu.c +1 -0
+       struct validate_negotiate_info_req *pneg_inbuf;
        struct validate_negotiate_info_rsp *pneg_rsp = NULL;
        u32 rsplen;

u32 inbuflen; /* max of 4 dialects */

fs/cifs/smb2pdu.c +0 -1
@@ -741,7 +741,6 @@ int smb3_validate_negotiate(const unsigned int xid,
 struct cifs_tcon *tcon)
        if (tcon->ses->server->rdma)
                return 0;
  #endif
-
        /* In SMB3.11 preauth integrity supersedes validate negotiate */
        if (tcon->ses->server->dialect == SMB311_PROT_ID)

                return 0;
fs/cifs/smb2pdu.c +12 -7
@@ -764,63 +763,67 @@ int smb3_validate_negotiate(const unsigned int xid,
 struct cifs_tcon *tcon)
        if (tcon->ses->session_flags & SMB2_SESSION_FLAG_IS_NULL)
                cifs_dbg(VFS, "Unexpected null user (anonymous) auth flag
 sent by server\n");
  -     vneg_inbuf.Capabilities =
+       pneg_inbuf = kmalloc(sizeof(*pneg_inbuf), GFP_NOFS);
+       if (!pneg_inbuf)
+               return -ENOMEM;
+
+       pneg_inbuf->Capabilities =
 
 cpu_to_le32(tcon->ses->server->vals->req_capabilities);
-       memcpy(vneg_inbuf.Guid, tcon->ses->server->client_guid,
+       memcpy(pneg_inbuf->Guid, tcon->ses->server->client_guid,
                                        SMB2_CLIENT_GUID_SIZE);
        if (tcon->ses->sign)
-               vneg_inbuf.SecurityMode =
+               pneg_inbuf->SecurityMode =
                        cpu_to_le16(SMB2_NEGOTIATE_SIGNING_REQUIRED);
        else if (global_secflags & CIFSSEC_MAY_SIGN)
-               vneg_inbuf.SecurityMode =
+               pneg_inbuf->SecurityMode =
                        cpu_to_le16(SMB2_NEGOTIATE_SIGNING_ENABLED);
        else
-               vneg_inbuf.SecurityMode = 0;
+               pneg_inbuf->SecurityMode = 0;
        if (strcmp(tcon->ses->server->vals->version_string,
                SMB3ANY_VERSION_STRING) == 0) {
-               vneg_inbuf.Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
-               vneg_inbuf.Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
-               vneg_inbuf.DialectCount = cpu_to_le16(2);
+               pneg_inbuf->Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
+               pneg_inbuf->Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
+               pneg_inbuf->DialectCount = cpu_to_le16(2);
Quoted 8 lines

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, · line 792

Quoted 3 lines
fs/cifs/smb2pdu.c +0 -0
                /* structure is big enough for 3 dialects, sending only 2
 */
                inbuflen = sizeof(struct validate_negotiate_info_req) - 2;

The "- 2" is a little confusing here. This was existing code, but I suggest you change this to a sizeof (u16) construct for consistency. It's reducing by the length of a single Dialects[n] entry.

I would suggest to make it "- sizeof(pneg_inbuf->Dialects[0])"...

Quoted 26 lines
Quoted 13 lines
fs/cifs/smb2pdu.c +4 -4
        } else if (strcmp(tcon->ses->server->vals->version_string,
                SMBDEFAULT_VERSION_STRING) == 0) {
-               vneg_inbuf.Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
-               vneg_inbuf.Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
-               vneg_inbuf.Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
-               vneg_inbuf.DialectCount = cpu_to_le16(3);
+               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->DialectCount = cpu_to_le16(3);
                /* structure is big enough for 3 dialects */
                inbuflen = sizeof(struct validate_negotiate_info_req);

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 806

Quoted 10 lines
fs/cifs/smb2pdu.c +2 -2
        } else {
                /* otherwise specific dialect was requested */
-               vneg_inbuf.Dialects[0] =
+               pneg_inbuf->Dialects[0] =
                        cpu_to_le16(tcon->ses->server->vals->protocol_id);
-               vneg_inbuf.DialectCount = cpu_to_le16(1);
+               pneg_inbuf->DialectCount = cpu_to_le16(1);
                /* structure is big enough for 3 dialects, sending only 1
 */
                inbuflen = sizeof(struct validate_negotiate_info_req) - 4;
Quoted 1 line

Ditto previous sizeof (u16) comment, with a *2 this case.

... and "- 2 * sizeof(pneg_inbuf->Dialects[0])" respectively.

Quoted 70 lines
Quoted 8 lines
fs/cifs/smb2pdu.c +2 -1
        }
  -     rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
+       ret = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
                FSCTL_VALIDATE_NEGOTIATE_INFO, true /* is_fsctl */,
-               (char *)&vneg_inbuf, sizeof(struct
 validate_negotiate_info_req),
+               (char *)pneg_inbuf, sizeof(*pneg_inbuf),

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 817

Quoted 10 lines
fs/cifs/smb2pdu.c +3 -2
                (char **)&pneg_rsp, &rsplen);
  -     if (rc != 0) {
-               cifs_dbg(VFS, "validate protocol negotiate failed: %d\n",
 rc);
-               return -EIO;
+       if (ret) {
+               cifs_dbg(VFS, "validate protocol negotiate failed: %d\n",
 ret);
+               goto out_free_inbuf;
        }

Why not leave "rc" alone, and set its value to -EIO before the goto if the ioctl fails? That will simplify and make the code much more readable IMO.

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 827

Quoted 13 lines
fs/cifs/smb2pdu.c +1 -0
  -     if (rsplen != sizeof(struct validate_negotiate_info_rsp)) {
+       if (rsplen != sizeof(*pneg_rsp)) {
                cifs_dbg(VFS, "invalid protocol negotiate response size:
 %d\n",
                         rsplen);
                /* relax check since Mac returns max bufsize allowed on

ioctl */
if ((rsplen > CIFSMaxBufSize)
|| (rsplen < sizeof(struct
validate_negotiate_info_rsp)))

fs/cifs/smb2pdu.c +1 -1
-                       goto err_rsp_free;
+                       goto out_free_rsp;
        }

Would need an rc = -EIO here too if above comment is accepted.

Tom.

Quoted 13 lines
        /* check validate negotiate info response matches what we got

earlier */

fs/cifs/smb2pdu.c +2 -2
@@ -838,14 +841,16 @@ int smb3_validate_negotiate(const unsigned int xid,
 struct cifs_tcon *tcon)
        /* validate negotiate successful */
        cifs_dbg(FYI, "validate negotiate info successful\n");
-       kfree(pneg_rsp);
-       return 0;
+       rc = 0;
+       goto out_free_rsp;
    vneg_out:
        cifs_dbg(VFS, "protocol revalidation - security settings
 mismatch\n");

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, · line 856

Quoted 10 lines
fs/cifs/smb2pdu.c +4 -2
-err_rsp_free:
+out_free_rsp:
        kfree(pneg_rsp);
-       return -EIO;
+out_free_inbuf:
+       kfree(pneg_inbuf);
+       return rc;
  }
    enum securityEnum

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
In reply to Tom Talpey
Quoted 44 lines

Subject: Re: [Patch v4] cifs: Allocate validate negotiation request through kmalloc

Looks good, but I have two possibly style-related comments.

Quoted 24 lines · Long Li

From: Long Li <[email protected]>

The data buffer allocated on the stack can't be DMA'ed, ib_dma_map_page will return an invalid DMA address for a buffer on stack. Even worse, this incorrect address can't be detected by ib_dma_mapping_error. Sending data from this address to hardware will not fail, but the remote peer will get junk data.

Fix this by allocating the request on the heap in smb3_validate_negotiate.

Changes in v2:
Removed duplicated code on freeing buffers on function exit. (Thanks to Parav Pandit <[email protected]>) Fixed typo in the patch title.

Changes in v3:
Added "Fixes" to the patch.
Changed several sizeof() to use *pointer in place of struct.

Changes in v4:
Added detailed comments on the failure through RDMA. Allocate request buffer using GPF_NOFS. Fixed possible memory leak.

Quoted 4 lines
Fixes ff1c038addc4 ("Check SMB3 dialects against downgrade attacks")
Signed-off-by Long Li <[email protected]>
---
  fs/cifs/smb2pdu.c | 61 ++++++++++++++++++++++++++++++--------------

-----------

Quoted 9 lines
  1 file changed, 33 insertions(+), 28 deletions(-)
fs/cifs/smb2pdu.c index renamed from fs/cifs/smb2pdu.c +0 -0

0f044c4..caa2a1e 100644

fs/cifs/smb2pdu.c +0 -0
@@ -729,8 +729,8 @@ SMB2_negotiate(const unsigned int xid, struct
 cifs_ses *ses)
Quoted 8 lines

fs/cifs/smb2pdu.c SMB2_negotiate(const unsigned int xid, struct cifs_ses *ses) · line 732

Quoted 5 lines
fs/cifs/smb2pdu.c +1 -2
  int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
  {
-	int rc = 0;
-	struct validate_negotiate_info_req vneg_inbuf;
+	int ret, rc = -EIO;

Seems awkward to have "rc" and "ret", and based on the code below I don't see why two variables are needed. Simplify? (see later comment)

This is for addressing a prior comment to reduce duplicate code. All the failure paths (after issuing I/O) returning -EIO, there are 5 of them. Set rc to -EIO at the beginning so we don’t need to set it multiple times.

Quoted 152 lines

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int

Quoted 14 lines
fs/cifs/smb2pdu.c +1 -0
+	struct validate_negotiate_info_req *pneg_inbuf;
  	struct validate_negotiate_info_rsp *pneg_rsp = NULL;
  	u32 rsplen;

u32 inbuflen; /* max of 4 dialects */ @@ -741,7 +741,6 @@ int
smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
if (tcon->ses->server->rdma)
return 0;
#endif

fs/cifs/smb2pdu.c +0 -1
-
  	/* In SMB3.11 preauth integrity supersedes validate negotiate */
  	if (tcon->ses->server->dialect == SMB311_PROT_ID)
  		return 0;

@@ -764,63 +763,67 @@ int smb3_validate_negotiate(const unsigned int

xid, struct cifs_tcon *tcon)

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int · line 764

Quoted 2 lines
fs/cifs/smb2pdu.c +0 -0
  	if (tcon->ses->session_flags & SMB2_SESSION_FLAG_IS_NULL)
  		cifs_dbg(VFS, "Unexpected null user (anonymous) auth flag

sent by

Quoted 8 lines
fs/cifs/smb2pdu.c +5 -1
 server\n");
 
-	vneg_inbuf.Capabilities =
+	pneg_inbuf = kmalloc(sizeof(*pneg_inbuf), GFP_NOFS);
+	if (!pneg_inbuf)
+		return -ENOMEM;
+
+	pneg_inbuf->Capabilities =

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 783

Quoted 10 lines
fs/cifs/smb2pdu.c +2 -2
  			cpu_to_le32(tcon->ses->server->vals-
 req_capabilities);
-	memcpy(vneg_inbuf.Guid, tcon->ses->server->client_guid,
+	memcpy(pneg_inbuf->Guid, tcon->ses->server->client_guid,
  					SMB2_CLIENT_GUID_SIZE);
 
  	if (tcon->ses->sign)
-		vneg_inbuf.SecurityMode =
+		pneg_inbuf->SecurityMode =
 

	cpu_to_le16(SMB2_NEGOTIATE_SIGNING_REQUIRED);

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 783

Quoted 4 lines
fs/cifs/smb2pdu.c +1 -1
  	else if (global_secflags & CIFSSEC_MAY_SIGN)
-		vneg_inbuf.SecurityMode =
+		pneg_inbuf->SecurityMode =
 

	cpu_to_le16(SMB2_NEGOTIATE_SIGNING_ENABLED);
Quoted 5 lines
fs/cifs/smb2pdu.c +1 -1
  	else
-		vneg_inbuf.SecurityMode = 0;
+		pneg_inbuf->SecurityMode = 0;

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 791

Quoted 10 lines
fs/cifs/smb2pdu.c +3 -3
  	if (strcmp(tcon->ses->server->vals->version_string,
  		SMB3ANY_VERSION_STRING) == 0) {
-		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
-		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
-		vneg_inbuf.DialectCount = cpu_to_le16(2);
+		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
+		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
+		pneg_inbuf->DialectCount = cpu_to_le16(2);
  		/* structure is big enough for 3 dialects, sending only 2 */
  		inbuflen = sizeof(struct validate_negotiate_info_req) - 2;

The "- 2" is a little confusing here. This was existing code, but I suggest you change this to a sizeof (u16) construct for consistency.
It's reducing by the length of a single Dialects[n] entry.

Quoted 11 lines
fs/cifs/smb2pdu.c +4 -4
  	} else if (strcmp(tcon->ses->server->vals->version_string,
  		SMBDEFAULT_VERSION_STRING) == 0) {
-		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
-		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
-		vneg_inbuf.Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
-		vneg_inbuf.DialectCount = cpu_to_le16(3);
+		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->DialectCount = cpu_to_le16(3);
  		/* structure is big enough for 3 dialects */

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 806

Quoted 10 lines
fs/cifs/smb2pdu.c +2 -2
  		inbuflen = sizeof(struct validate_negotiate_info_req);
  	} else {
  		/* otherwise specific dialect was requested */
-		vneg_inbuf.Dialects[0] =
+		pneg_inbuf->Dialects[0] =
  			cpu_to_le16(tcon->ses->server->vals->protocol_id);
-		vneg_inbuf.DialectCount = cpu_to_le16(1);
+		pneg_inbuf->DialectCount = cpu_to_le16(1);
  		/* structure is big enough for 3 dialects, sending only 1 */
  		inbuflen = sizeof(struct validate_negotiate_info_req) - 4;

Ditto previous sizeof (u16) comment, with a *2 this case.

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)

Quoted 6 lines
fs/cifs/smb2pdu.c +1 -2
  	}
 
-	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
+	ret = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
  		FSCTL_VALIDATE_NEGOTIATE_INFO, true /* is_fsctl */,
-		(char *)&vneg_inbuf, sizeof(struct

validate_negotiate_info_req),

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 817

Quoted 10 lines
fs/cifs/smb2pdu.c +4 -3
+		(char *)pneg_inbuf, sizeof(*pneg_inbuf),
  		(char **)&pneg_rsp, &rsplen);
 
-	if (rc != 0) {
-		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", rc);
-		return -EIO;
+	if (ret) {
+		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", ret);
+		goto out_free_inbuf;
  	}

Why not leave "rc" alone, and set its value to -EIO before the goto if the ioctl fails? That will simplify and make the code much more readable IMO.

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)

Quoted 4 lines
fs/cifs/smb2pdu.c +1 -1
-	if (rsplen != sizeof(struct validate_negotiate_info_rsp)) {
+	if (rsplen != sizeof(*pneg_rsp)) {
  		cifs_dbg(VFS, "invalid protocol negotiate response

size: %d\n",

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)

Quoted 3 lines
fs/cifs/smb2pdu.c +0 -0
  			 rsplen);
 
  		/* relax check since Mac returns max bufsize allowed on ioctl

*/

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 827

Quoted 5 lines
fs/cifs/smb2pdu.c +1 -1
  		if ((rsplen > CIFSMaxBufSize)
  		     || (rsplen < sizeof(struct validate_negotiate_info_rsp)))
-			goto err_rsp_free;
+			goto out_free_rsp;
  	}

Would need an rc = -EIO here too if above comment is accepted.

Tom.

Quoted 26 lines
fs/cifs/smb2pdu.c +0 -0
  	/* check validate negotiate info response matches what we got

earlier */ @@ -838,14 +841,16 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)

  	/* validate negotiate successful */
  	cifs_dbg(FYI, "validate negotiate info successful\n");
fs/cifs/smb2pdu.c +2 -2
-	kfree(pneg_rsp);
-	return 0;
+	rc = 0;
+	goto out_free_rsp;
 
  vneg_out:
  	cifs_dbg(VFS, "protocol revalidation - security settings

mismatch\n");

fs/cifs/smb2pdu.c +4 -2
-err_rsp_free:
+out_free_rsp:
  	kfree(pneg_rsp);
-	return -EIO;
+out_free_inbuf:
+	kfree(pneg_inbuf);
+	return rc;
  }
 
  enum securityEnum
reviewer
#4 lore raw
Quoted 53 lines · Long Li
Quoted 44 lines

Subject: Re: [Patch v4] cifs: Allocate validate negotiation request through kmalloc

Looks good, but I have two possibly style-related comments.

Quoted 24 lines · Long Li

From: Long Li <[email protected]>

The data buffer allocated on the stack can't be DMA'ed, ib_dma_map_page will return an invalid DMA address for a buffer on stack. Even worse, this incorrect address can't be detected by ib_dma_mapping_error. Sending data from this address to hardware will not fail, but the remote peer will get junk data.

Fix this by allocating the request on the heap in smb3_validate_negotiate.

Changes in v2:
Removed duplicated code on freeing buffers on function exit. (Thanks to Parav Pandit <[email protected]>) Fixed typo in the patch title.

Changes in v3:
Added "Fixes" to the patch.
Changed several sizeof() to use *pointer in place of struct.

Changes in v4:
Added detailed comments on the failure through RDMA. Allocate request buffer using GPF_NOFS. Fixed possible memory leak.

Quoted 4 lines
Fixes ff1c038addc4 ("Check SMB3 dialects against downgrade attacks")
Signed-off-by Long Li <[email protected]>
---
   fs/cifs/smb2pdu.c | 61 ++++++++++++++++++++++++++++++--------------

-----------

Quoted 9 lines
   1 file changed, 33 insertions(+), 28 deletions(-)
fs/cifs/smb2pdu.c index renamed from fs/cifs/smb2pdu.c +0 -0

0f044c4..caa2a1e 100644

fs/cifs/smb2pdu.c +0 -0
@@ -729,8 +729,8 @@ SMB2_negotiate(const unsigned int xid, struct
 cifs_ses *ses)
Quoted 8 lines

fs/cifs/smb2pdu.c SMB2_negotiate(const unsigned int xid, struct cifs_ses *ses) · line 732

Quoted 5 lines
fs/cifs/smb2pdu.c +1 -2
   int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
   {
-	int rc = 0;
-	struct validate_negotiate_info_req vneg_inbuf;
+	int ret, rc = -EIO;

Seems awkward to have "rc" and "ret", and based on the code below I don't see why two variables are needed. Simplify? (see later comment)

Quoted 3 lines

This is for addressing a prior comment to reduce duplicate code. All the failure paths (after issuing I/O) returning -EIO, there are 5 of them. Set rc to -EIO at the beginning so we don’t need to set it multiple times.

Well, ok but now there are semi-duplicate and rather confusing "rc" and "ret" local variables, only one of which is actually returned.

How about a "goto err" with unconditonal -EIO, and just leave the "return 0" path alone, like it was? That would be much clearer IMO.

As-is, I needed to read it several times to convince myself the right rc was returned.

Tom,

Quoted 151 lines
Quoted 151 lines

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int

Quoted 15 lines
fs/cifs/smb2pdu.c +1 -0
+	struct validate_negotiate_info_req *pneg_inbuf;
   	struct validate_negotiate_info_rsp *pneg_rsp = NULL;
   	u32 rsplen;

   	u32 inbuflen; /* max of 4 dialects */ @@ -741,7 +741,6 @@ int
smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
   	if (tcon->ses->server->rdma)
   		return 0;
   #endif
fs/cifs/smb2pdu.c +0 -1
-
   	/* In SMB3.11 preauth integrity supersedes validate negotiate */
   	if (tcon->ses->server->dialect == SMB311_PROT_ID)
   		return 0;

@@ -764,63 +763,67 @@ int smb3_validate_negotiate(const unsigned int

xid, struct cifs_tcon *tcon)

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int · line 764

Quoted 2 lines
fs/cifs/smb2pdu.c +0 -0
   	if (tcon->ses->session_flags & SMB2_SESSION_FLAG_IS_NULL)
   		cifs_dbg(VFS, "Unexpected null user (anonymous) auth flag

sent by

Quoted 8 lines
fs/cifs/smb2pdu.c +5 -1
 server\n");
 
-	vneg_inbuf.Capabilities =
+	pneg_inbuf = kmalloc(sizeof(*pneg_inbuf), GFP_NOFS);
+	if (!pneg_inbuf)
+		return -ENOMEM;
+
+	pneg_inbuf->Capabilities =

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 783

Quoted 10 lines
fs/cifs/smb2pdu.c +2 -2
   			cpu_to_le32(tcon->ses->server->vals-
 req_capabilities);
-	memcpy(vneg_inbuf.Guid, tcon->ses->server->client_guid,
+	memcpy(pneg_inbuf->Guid, tcon->ses->server->client_guid,
   					SMB2_CLIENT_GUID_SIZE);
 
   	if (tcon->ses->sign)
-		vneg_inbuf.SecurityMode =
+		pneg_inbuf->SecurityMode =
 

	cpu_to_le16(SMB2_NEGOTIATE_SIGNING_REQUIRED);

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 783

Quoted 4 lines
fs/cifs/smb2pdu.c +1 -1
   	else if (global_secflags & CIFSSEC_MAY_SIGN)
-		vneg_inbuf.SecurityMode =
+		pneg_inbuf->SecurityMode =
 

	cpu_to_le16(SMB2_NEGOTIATE_SIGNING_ENABLED);
Quoted 5 lines
fs/cifs/smb2pdu.c +1 -1
   	else
-		vneg_inbuf.SecurityMode = 0;
+		pneg_inbuf->SecurityMode = 0;

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 791

Quoted 10 lines
fs/cifs/smb2pdu.c +3 -3
   	if (strcmp(tcon->ses->server->vals->version_string,
   		SMB3ANY_VERSION_STRING) == 0) {
-		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
-		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
-		vneg_inbuf.DialectCount = cpu_to_le16(2);
+		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
+		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
+		pneg_inbuf->DialectCount = cpu_to_le16(2);
   		/* structure is big enough for 3 dialects, sending only 2 */
   		inbuflen = sizeof(struct validate_negotiate_info_req) - 2;

The "- 2" is a little confusing here. This was existing code, but I suggest you change this to a sizeof (u16) construct for consistency.
It's reducing by the length of a single Dialects[n] entry.

Quoted 11 lines
fs/cifs/smb2pdu.c +4 -4
   	} else if (strcmp(tcon->ses->server->vals->version_string,
   		SMBDEFAULT_VERSION_STRING) == 0) {
-		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
-		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
-		vneg_inbuf.Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
-		vneg_inbuf.DialectCount = cpu_to_le16(3);
+		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->DialectCount = cpu_to_le16(3);
   		/* structure is big enough for 3 dialects */

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 806

Quoted 10 lines
fs/cifs/smb2pdu.c +2 -2
   		inbuflen = sizeof(struct validate_negotiate_info_req);
   	} else {
   		/* otherwise specific dialect was requested */
-		vneg_inbuf.Dialects[0] =
+		pneg_inbuf->Dialects[0] =
   			cpu_to_le16(tcon->ses->server->vals->protocol_id);
-		vneg_inbuf.DialectCount = cpu_to_le16(1);
+		pneg_inbuf->DialectCount = cpu_to_le16(1);
   		/* structure is big enough for 3 dialects, sending only 1 */
   		inbuflen = sizeof(struct validate_negotiate_info_req) - 4;

Ditto previous sizeof (u16) comment, with a *2 this case.

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)

Quoted 6 lines
fs/cifs/smb2pdu.c +1 -2
   	}
 
-	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
+	ret = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
   		FSCTL_VALIDATE_NEGOTIATE_INFO, true /* is_fsctl */,
-		(char *)&vneg_inbuf, sizeof(struct

validate_negotiate_info_req),

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 817

Quoted 10 lines
fs/cifs/smb2pdu.c +4 -3
+		(char *)pneg_inbuf, sizeof(*pneg_inbuf),
   		(char **)&pneg_rsp, &rsplen);
 
-	if (rc != 0) {
-		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", rc);
-		return -EIO;
+	if (ret) {
+		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", ret);
+		goto out_free_inbuf;
   	}

Why not leave "rc" alone, and set its value to -EIO before the goto if the ioctl fails? That will simplify and make the code much more readable IMO.

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)

Quoted 4 lines
fs/cifs/smb2pdu.c +1 -1
-	if (rsplen != sizeof(struct validate_negotiate_info_rsp)) {
+	if (rsplen != sizeof(*pneg_rsp)) {
   		cifs_dbg(VFS, "invalid protocol negotiate response

size: %d\n",

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)

Quoted 3 lines
fs/cifs/smb2pdu.c +0 -0
   			 rsplen);
 
   		/* relax check since Mac returns max bufsize allowed on ioctl

*/

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 827

Quoted 5 lines
fs/cifs/smb2pdu.c +1 -1
   		if ((rsplen > CIFSMaxBufSize)
   		     || (rsplen < sizeof(struct validate_negotiate_info_rsp)))
-			goto err_rsp_free;
+			goto out_free_rsp;
   	}

Would need an rc = -EIO here too if above comment is accepted.

Tom.

Quoted 24 lines
fs/cifs/smb2pdu.c +0 -0
   	/* check validate negotiate info response matches what we got

earlier */ @@ -838,14 +841,16 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)

   	/* validate negotiate successful */
   	cifs_dbg(FYI, "validate negotiate info successful\n");
fs/cifs/smb2pdu.c +2 -2
-	kfree(pneg_rsp);
-	return 0;
+	rc = 0;
+	goto out_free_rsp;
 
   vneg_out:
   	cifs_dbg(VFS, "protocol revalidation - security settings

mismatch\n");

fs/cifs/smb2pdu.c +4 -2
-err_rsp_free:
+out_free_rsp:
   	kfree(pneg_rsp);
-	return -EIO;
+out_free_inbuf:
+	kfree(pneg_inbuf);
+	return rc;
   }
Quoted 3 lines
Quoted 2 lines
Quoted 2 lines

enum securityEnum

N�����r��y���b�X��ǧv�^�)޺{.n�+����{��ٚ�{ay�

ʇڙ�,j��f���h���z�
�w���
���j:+v���w�j�m��������zZ+�����ݢj"��!tml=

Quoted 1 line
#5 lore raw
Quoted 64 lines

Subject: Re: [Patch v4] cifs: Allocate validate negotiation request through kmalloc

Quoted 8 lines · Long Li
Quoted 8 lines

Subject: Re: [Patch v4] cifs: Allocate validate negotiation request through kmalloc

Looks good, but I have two possibly style-related comments.

Quoted 2 lines · Long Li

From: Long Li <[email protected]>

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

The data buffer allocated on the stack can't be DMA'ed, ib_dma_map_page will return an invalid DMA address for a buffer on stack. Even worse, this incorrect address can't be detected by ib_dma_mapping_error. Sending data from this address to hardware will not fail, but the remote peer will get junk data.

Fix this by allocating the request on the heap in

smb3_validate_negotiate.

Quoted 24 lines
Quoted 24 lines
Quoted 15 lines

Changes in v2:
Removed duplicated code on freeing buffers on function exit. (Thanks to Parav Pandit <[email protected]>) Fixed typo in the patch title.

Changes in v3:
Added "Fixes" to the patch.
Changed several sizeof() to use *pointer in place of struct.

Changes in v4:
Added detailed comments on the failure through RDMA. Allocate request buffer using GPF_NOFS. Fixed possible memory leak.

Quoted 6 lines
Fixes ff1c038addc4 ("Check SMB3 dialects against downgrade

attacks")

Signed-off-by Long Li <[email protected]>
---
   fs/cifs/smb2pdu.c | 61

++++++++++++++++++++++++++++++--------------

-----------

Quoted 2 lines
   1 file changed, 33 insertions(+), 28 deletions(-)
Quoted 8 lines
Quoted 8 lines

fs/cifs/smb2pdu.c SMB2_negotiate(const unsigned int xid, struct · line 731

Quoted 8 lines
fs/cifs/smb2pdu.c index renamed from fs/cifs/smb2pdu.c +0 -0

0f044c4..caa2a1e 100644

fs/cifs/smb2pdu.c +0 -0
@@ -729,8 +729,8 @@ SMB2_negotiate(const unsigned int xid, struct
 cifs_ses *ses)
 
   int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon

*tcon)

Quoted 11 lines
Quoted 7 lines

fs/cifs/smb2pdu.c SMB2_negotiate(const unsigned int xid, struct cifs_ses *ses) · line 732

Quoted 4 lines
fs/cifs/smb2pdu.c +1 -2
   {
-	int rc = 0;
-	struct validate_negotiate_info_req vneg_inbuf;
+	int ret, rc = -EIO;

Seems awkward to have "rc" and "ret", and based on the code below I don't see why two variables are needed. Simplify? (see later comment)

This is for addressing a prior comment to reduce duplicate code. All the failure paths (after issuing I/O) returning -EIO, there are 5 of them. Set rc to -EIO at the beginning so we don’t need to set it multiple

Quoted 7 lines

times.

Well, ok but now there are semi-duplicate and rather confusing "rc" and "ret" local variables, only one of which is actually returned.

How about a "goto err" with unconditonal -EIO, and just leave the "return 0" path alone, like it was? That would be much clearer IMO.

That means we'll have to call kfree(pneg_rsp) and kfree(pneg_inbuf) on the return 0 path, as well as the return -EIO path.

I'm happy to do that if there is no objection.

Quoted 175 lines

As-is, I needed to read it several times to convince myself the right rc was returned.

Tom,

Quoted 15 lines
Quoted 15 lines

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned

Quoted 15 lines
fs/cifs/smb2pdu.c +1 -0
+	struct validate_negotiate_info_req *pneg_inbuf;
   	struct validate_negotiate_info_rsp *pneg_rsp = NULL;
   	u32 rsplen;

   	u32 inbuflen; /* max of 4 dialects */ @@ -741,7 +741,6 @@ int
smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
   	if (tcon->ses->server->rdma)
   		return 0;
   #endif
fs/cifs/smb2pdu.c +0 -1
-
   	/* In SMB3.11 preauth integrity supersedes validate negotiate */
   	if (tcon->ses->server->dialect == SMB311_PROT_ID)
   		return 0;

@@ -764,63 +763,67 @@ int smb3_validate_negotiate(const unsigned

int

Quoted 82 lines
Quoted 82 lines

xid, struct cifs_tcon *tcon)

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned · line 764

Quoted 2 lines
fs/cifs/smb2pdu.c +0 -0
   	if (tcon->ses->session_flags & SMB2_SESSION_FLAG_IS_NULL)
   		cifs_dbg(VFS, "Unexpected null user (anonymous) auth flag

sent by

Quoted 8 lines
fs/cifs/smb2pdu.c +5 -1
 server\n");
 
-	vneg_inbuf.Capabilities =
+	pneg_inbuf = kmalloc(sizeof(*pneg_inbuf), GFP_NOFS);
+	if (!pneg_inbuf)
+		return -ENOMEM;
+
+	pneg_inbuf->Capabilities =

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 783

Quoted 10 lines
fs/cifs/smb2pdu.c +2 -2
   			cpu_to_le32(tcon->ses->server->vals-
 req_capabilities);
-	memcpy(vneg_inbuf.Guid, tcon->ses->server->client_guid,
+	memcpy(pneg_inbuf->Guid, tcon->ses->server->client_guid,
   					SMB2_CLIENT_GUID_SIZE);
 
   	if (tcon->ses->sign)
-		vneg_inbuf.SecurityMode =
+		pneg_inbuf->SecurityMode =
 

	cpu_to_le16(SMB2_NEGOTIATE_SIGNING_REQUIRED);

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 783

Quoted 4 lines
fs/cifs/smb2pdu.c +1 -1
   	else if (global_secflags & CIFSSEC_MAY_SIGN)
-		vneg_inbuf.SecurityMode =
+		pneg_inbuf->SecurityMode =
 

	cpu_to_le16(SMB2_NEGOTIATE_SIGNING_ENABLED);
Quoted 5 lines
fs/cifs/smb2pdu.c +1 -1
   	else
-		vneg_inbuf.SecurityMode = 0;
+		pneg_inbuf->SecurityMode = 0;

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 791

Quoted 10 lines
fs/cifs/smb2pdu.c +3 -3
   	if (strcmp(tcon->ses->server->vals->version_string,
   		SMB3ANY_VERSION_STRING) == 0) {
-		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
-		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
-		vneg_inbuf.DialectCount = cpu_to_le16(2);
+		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
+		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
+		pneg_inbuf->DialectCount = cpu_to_le16(2);
   		/* structure is big enough for 3 dialects, sending only 2 */
   		inbuflen = sizeof(struct validate_negotiate_info_req) - 2;

The "- 2" is a little confusing here. This was existing code, but I suggest you change this to a sizeof (u16) construct for consistency. It's reducing by the length of a single Dialects[n] entry.

Quoted 11 lines
fs/cifs/smb2pdu.c +4 -4
   	} else if (strcmp(tcon->ses->server->vals->version_string,
   		SMBDEFAULT_VERSION_STRING) == 0) {
-		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
-		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
-		vneg_inbuf.Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
-		vneg_inbuf.DialectCount = cpu_to_le16(3);
+		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->DialectCount = cpu_to_le16(3);
   		/* structure is big enough for 3 dialects */

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 806

Quoted 10 lines
fs/cifs/smb2pdu.c +2 -2
   		inbuflen = sizeof(struct validate_negotiate_info_req);
   	} else {
   		/* otherwise specific dialect was requested */
-		vneg_inbuf.Dialects[0] =
+		pneg_inbuf->Dialects[0] =
   			cpu_to_le16(tcon->ses->server->vals->protocol_id);
-		vneg_inbuf.DialectCount = cpu_to_le16(1);
+		pneg_inbuf->DialectCount = cpu_to_le16(1);
   		/* structure is big enough for 3 dialects, sending only 1 */
   		inbuflen = sizeof(struct validate_negotiate_info_req) - 4;

Ditto previous sizeof (u16) comment, with a *2 this case.

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)

Quoted 6 lines
fs/cifs/smb2pdu.c +1 -2
   	}
 
-	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
+	ret = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
   		FSCTL_VALIDATE_NEGOTIATE_INFO, true /* is_fsctl */,
-		(char *)&vneg_inbuf, sizeof(struct

validate_negotiate_info_req),

Quoted 3 lines
fs/cifs/smb2pdu.c +1 -0
+		(char *)pneg_inbuf, sizeof(*pneg_inbuf),
   		(char **)&pneg_rsp, &rsplen);
Quoted 10 lines
Quoted 10 lines

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 817

Quoted 7 lines
fs/cifs/smb2pdu.c +3 -3
-	if (rc != 0) {
-		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", rc);
-		return -EIO;
+	if (ret) {
+		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", ret);
+		goto out_free_inbuf;
   	}

Why not leave "rc" alone, and set its value to -EIO before the goto if the ioctl fails? That will simplify and make the code much more readable

IMO.

Quoted 44 lines
Quoted 44 lines

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)

Quoted 5 lines
fs/cifs/smb2pdu.c +1 -1
-	if (rsplen != sizeof(struct validate_negotiate_info_rsp)) {
+	if (rsplen != sizeof(*pneg_rsp)) {
   		cifs_dbg(VFS, "invalid protocol negotiate response

size: %d\n",

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)

Quoted 3 lines
fs/cifs/smb2pdu.c +0 -0
   			 rsplen);
 
   		/* relax check since Mac returns max bufsize allowed on ioctl

*/

fs/cifs/smb2pdu.c int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) · line 827

Quoted 5 lines
fs/cifs/smb2pdu.c +1 -1
   		if ((rsplen > CIFSMaxBufSize)
   		     || (rsplen < sizeof(struct validate_negotiate_info_rsp)))
-			goto err_rsp_free;
+			goto out_free_rsp;
   	}

Would need an rc = -EIO here too if above comment is accepted.

Tom.

Quoted 24 lines
fs/cifs/smb2pdu.c +0 -0
   	/* check validate negotiate info response matches what we got

earlier */ @@ -838,14 +841,16 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)

   	/* validate negotiate successful */
   	cifs_dbg(FYI, "validate negotiate info successful\n");
fs/cifs/smb2pdu.c +2 -2
-	kfree(pneg_rsp);
-	return 0;
+	rc = 0;
+	goto out_free_rsp;
 
   vneg_out:
   	cifs_dbg(VFS, "protocol revalidation - security settings

mismatch\n");

fs/cifs/smb2pdu.c +4 -2
-err_rsp_free:
+out_free_rsp:
   	kfree(pneg_rsp);
-	return -EIO;
+out_free_inbuf:
+	kfree(pneg_inbuf);
+	return rc;
   }
Quoted 3 lines
Quoted 2 lines
Quoted 2 lines

enum securityEnum

N r y b X ǧv ^ )޺{.n + { ٚ {ay ʇڙ ,j f h z  w

j:+v w j m zZ+ ݢj" !tml=

Quoted 1 line

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at
https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.ke
rnel.org%2Fmajordomo- info.html&data=02%7C01%7Clongli%40microsoft.com%7C713a34e4889147ee d4fd08d5a6ef951e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63 6598470290134086&sdata=RfS2Gs9cqoxkofoFtqcMTtSquLOZD09ffLhdlWCj2S 4%3D&reserved=0