LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [Patch v4] cifs: Allocate validate negotiation request through kmalloc
@ 2018-04-19 21:38 Long Li
  2018-04-20 14:55 ` Tom Talpey
  0 siblings, 1 reply; 6+ messages in thread
From: Long Li @ 2018-04-19 21:38 UTC (permalink / raw)
  To: Steve French, linux-cifs, samba-technical, linux-kernel, linux-rdma
  Cc: Long Li

From: Long Li <longli@microsoft.com>

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 <parav@mellanox.com>)
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 <longli@microsoft.com>
---
 fs/cifs/smb2pdu.c | 61 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 0f044c4..caa2a1e 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -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
-- 
2.7.4

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Patch v4] cifs: Allocate validate negotiation request through kmalloc
  2018-04-19 21:38 [Patch v4] cifs: Allocate validate negotiation request through kmalloc Long Li
@ 2018-04-20 14:55 ` Tom Talpey
  2018-04-20 17:58   ` Pavel Shilovsky
  2018-04-20 18:41   ` Long Li
  0 siblings, 2 replies; 6+ messages in thread
From: Tom Talpey @ 2018-04-20 14:55 UTC (permalink / raw)
  To: longli, Steve French, linux-cifs, samba-technical, linux-kernel,
	linux-rdma

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

On 4/19/2018 5:38 PM, Long Li wrote:
> From: Long Li <longli@microsoft.com>
> 
> 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 <parav@mellanox.com>)
> 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 <longli@microsoft.com>
> ---
>   fs/cifs/smb2pdu.c | 61 ++++++++++++++++++++++++++++++-------------------------
>   1 file changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 0f044c4..caa2a1e 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -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)

> +	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;

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.

>   	} 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;

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

>   	}
>   
> -	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;
>   	}

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.

>   
> -	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.

>   
>   	/* 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
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Patch v4] cifs: Allocate validate negotiation request through kmalloc
  2018-04-20 14:55 ` Tom Talpey
@ 2018-04-20 17:58   ` Pavel Shilovsky
  2018-04-20 18:41   ` Long Li
  1 sibling, 0 replies; 6+ messages in thread
From: Pavel Shilovsky @ 2018-04-20 17:58 UTC (permalink / raw)
  To: Tom Talpey
  Cc: Long Li, Steve French, linux-cifs, samba-technical,
	Kernel Mailing List, linux-rdma

2018-04-20 7:55 GMT-07:00 Tom Talpey <tom@talpey.com>:
> Looks good, but I have two possibly style-related comments.
>
>
> On 4/19/2018 5:38 PM, Long Li wrote:
>>
>> From: Long Li <longli@microsoft.com>
>>
>> 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 <parav@mellanox.com>)
>> 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 <longli@microsoft.com>
>> ---
>>   fs/cifs/smb2pdu.c | 61
>> ++++++++++++++++++++++++++++++-------------------------
>>   1 file changed, 33 insertions(+), 28 deletions(-)
>>
>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
>> index 0f044c4..caa2a1e 100644
>> --- a/fs/cifs/smb2pdu.c
>> +++ b/fs/cifs/smb2pdu.c
>> @@ -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)
>
>
>> +       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;
>
>
> 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])"...

>
>>         } 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;
>
>
>
> Ditto previous sizeof (u16) comment, with a *2 this case.

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

>
>>         }
>>   -     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;
>>         }
>
>
> 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.
>
>>   -     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.
>
>>         /* 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
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [Patch v4] cifs: Allocate validate negotiation request through kmalloc
  2018-04-20 14:55 ` Tom Talpey
  2018-04-20 17:58   ` Pavel Shilovsky
@ 2018-04-20 18:41   ` Long Li
  2018-04-20 18:50     ` Tom Talpey
  1 sibling, 1 reply; 6+ messages in thread
From: Long Li @ 2018-04-20 18:41 UTC (permalink / raw)
  To: Tom Talpey, Steve French, linux-cifs, samba-technical,
	linux-kernel, linux-rdma

> Subject: Re: [Patch v4] cifs: Allocate validate negotiation request through
> kmalloc
> 
> Looks good, but I have two possibly style-related comments.
> 
> On 4/19/2018 5:38 PM, Long Li wrote:
> > From: Long Li <longli@microsoft.com>
> >
> > 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 <parav@mellanox.com>) 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 <longli@microsoft.com>
> > ---
> >   fs/cifs/smb2pdu.c | 61 ++++++++++++++++++++++++++++++--------------
> -----------
> >   1 file changed, 33 insertions(+), 28 deletions(-)
> >
> > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index
> > 0f044c4..caa2a1e 100644
> > --- a/fs/cifs/smb2pdu.c
> > +++ b/fs/cifs/smb2pdu.c
> > @@ -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)

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.

> 
> > +	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;
> 
> 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.
> 
> >   	} 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;
> 
> Ditto previous sizeof (u16) comment, with a *2 this case.
> 
> >   	}
> >
> > -	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;
> >   	}
> 
> 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.
> 
> >
> > -	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.
> 
> >
> >   	/* 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
> >

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Patch v4] cifs: Allocate validate negotiation request through kmalloc
  2018-04-20 18:41   ` Long Li
@ 2018-04-20 18:50     ` Tom Talpey
  2018-04-20 19:15       ` Long Li
  0 siblings, 1 reply; 6+ messages in thread
From: Tom Talpey @ 2018-04-20 18:50 UTC (permalink / raw)
  To: Long Li, Steve French, linux-cifs, samba-technical, linux-kernel,
	linux-rdma

On 4/20/2018 2:41 PM, Long Li wrote:
>> Subject: Re: [Patch v4] cifs: Allocate validate negotiation request through
>> kmalloc
>>
>> Looks good, but I have two possibly style-related comments.
>>
>> On 4/19/2018 5:38 PM, Long Li wrote:
>>> From: Long Li <longli@microsoft.com>
>>>
>>> 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 <parav@mellanox.com>) 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 <longli@microsoft.com>
>>> ---
>>>    fs/cifs/smb2pdu.c | 61 ++++++++++++++++++++++++++++++--------------
>> -----------
>>>    1 file changed, 33 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index
>>> 0f044c4..caa2a1e 100644
>>> --- a/fs/cifs/smb2pdu.c
>>> +++ b/fs/cifs/smb2pdu.c
>>> @@ -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)
> 
> 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,


> 
>>
>>> +	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;
>>
>> 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.
>>
>>>    	} 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;
>>
>> Ditto previous sizeof (u16) comment, with a *2 this case.
>>
>>>    	}
>>>
>>> -	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;
>>>    	}
>>
>> 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.
>>
>>>
>>> -	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.
>>
>>>
>>>    	/* 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
>>>
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{��ٚ�{ay�\x1dʇڙ�,j\a��f���h���z�\x1e�w���\f���j:+v���w�j�m����\a����zZ+�����ݢj"��!tml=
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [Patch v4] cifs: Allocate validate negotiation request through kmalloc
  2018-04-20 18:50     ` Tom Talpey
@ 2018-04-20 19:15       ` Long Li
  0 siblings, 0 replies; 6+ messages in thread
From: Long Li @ 2018-04-20 19:15 UTC (permalink / raw)
  To: Tom Talpey, Steve French, linux-cifs, samba-technical,
	linux-kernel, linux-rdma

> Subject: Re: [Patch v4] cifs: Allocate validate negotiation request through
> kmalloc
> 
> On 4/20/2018 2:41 PM, Long Li wrote:
> >> Subject: Re: [Patch v4] cifs: Allocate validate negotiation request
> >> through kmalloc
> >>
> >> Looks good, but I have two possibly style-related comments.
> >>
> >> On 4/19/2018 5:38 PM, Long Li wrote:
> >>> From: Long Li <longli@microsoft.com>
> >>>
> >>> 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 <parav@mellanox.com>) 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 <longli@microsoft.com>
> >>> ---
> >>>    fs/cifs/smb2pdu.c | 61
> >>> ++++++++++++++++++++++++++++++--------------
> >> -----------
> >>>    1 file changed, 33 insertions(+), 28 deletions(-)
> >>>
> >>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index
> >>> 0f044c4..caa2a1e 100644
> >>> --- a/fs/cifs/smb2pdu.c
> >>> +++ b/fs/cifs/smb2pdu.c
> >>> @@ -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)
> >
> > 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.

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.

> 
> As-is, I needed to read it several times to convince myself the right rc was
> returned.
> 
> Tom,
> 
> 
> >
> >>
> >>> +	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;
> >>
> >> 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.
> >>
> >>>    	} 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;
> >>
> >> Ditto previous sizeof (u16) comment, with a *2 this case.
> >>
> >>>    	}
> >>>
> >>> -	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;
> >>>    	}
> >>
> >> 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.
> >>
> >>>
> >>> -	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.
> >>
> >>>
> >>>    	/* 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
> >>>
> > N     r  y   b X  ǧv ^ )޺{.n +    {  ٚ {ay \x1dʇڙ ,j   f   h   z \x1e w
> 
>    j:+v   w j m         zZ+     ݢj"  !tml=
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the
> body of a message to majordomo@vger.kernel.org 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-04-20 19:15 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-19 21:38 [Patch v4] cifs: Allocate validate negotiation request through kmalloc Long Li
2018-04-20 14:55 ` Tom Talpey
2018-04-20 17:58   ` Pavel Shilovsky
2018-04-20 18:41   ` Long Li
2018-04-20 18:50     ` Tom Talpey
2018-04-20 19:15       ` Long Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).