diff mbox series

[v3,2/6] cifs: Allocate validate negotiation request through kmalloc

Message ID 20180418003358.25098-1-longli@linuxonhyperv.com
State New
Headers show
Series None | expand

Commit Message

Long Li April 18, 2018, 12:33 a.m. UTC 
From: Long Li <longli@microsoft.com>

The data buffer allocated on the stack can't be DMA'ed, and hence can't send
through RDMA via SMB Direct.

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 sizeof() to use *pointer in place of struct.

Fixes: ff1c038addc4 ("Check SMB3 dialects against downgrade attacks")
Signed-off-by: Long Li <longli@microsoft.com>
Cc: stable@vger.kernel.org
---
 fs/cifs/smb2pdu.c | 59 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 32 insertions(+), 27 deletions(-)

Comments

Tom Talpey April 18, 2018, 11:32 a.m. UTC | #1 
Two comments.

On 4/17/2018 8:33 PM, Long Li wrote:
> From: Long Li <longli@microsoft.com>
> 
> The data buffer allocated on the stack can't be DMA'ed, and hence can't send
> through RDMA via SMB Direct.

This comment is confusing. Any registered memory can be DMA'd, need to
state the reason for the choice here more clearly.

> 
> 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 sizeof() to use *pointer in place of struct.
> 
> Fixes: ff1c038addc4 ("Check SMB3 dialects against downgrade attacks")
> Signed-off-by: Long Li <longli@microsoft.com>
> Cc: stable@vger.kernel.org
> ---
>   fs/cifs/smb2pdu.c | 59 ++++++++++++++++++++++++++++++-------------------------
>   1 file changed, 32 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 0f044c4..5582a02 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,6 +741,9 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
>   	if (tcon->ses->server->rdma)
>   		return 0;
>   #endif
> +	pneg_inbuf = kmalloc(sizeof(*pneg_inbuf), GFP_KERNEL);
> +	if (!pneg_inbuf)
> +		return -ENOMEM;

Why is this a nonblocking allocation? It would seem more robust to
use GFP_NOFS here.

Tom.

>   
>   	/* In SMB3.11 preauth integrity supersedes validate negotiate */
>   	if (tcon->ses->server->dialect == SMB311_PROT_ID)
> @@ -764,63 +767,63 @@ 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->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
> 
--
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
David Laight April 18, 2018, 1:08 p.m. UTC | #2 
From: Tom Talpey
> Sent: 18 April 2018 12:32
...
> On 4/17/2018 8:33 PM, Long Li wrote:
> > From: Long Li <longli@microsoft.com>
> >
> > The data buffer allocated on the stack can't be DMA'ed, and hence can't send
> > through RDMA via SMB Direct.
> 
> This comment is confusing. Any registered memory can be DMA'd, need to
> state the reason for the choice here more clearly.

The stack could be allocated with vmalloc().
In which case the pages might not be physically contiguous and there is no
(sensible) call to get the physical address required by the dma controller
(or other bus master).

	David
Tom Talpey April 18, 2018, 2:07 p.m. UTC | #3 
On 4/18/2018 9:08 AM, David Laight wrote:
> From: Tom Talpey
>> Sent: 18 April 2018 12:32
> ...
>> On 4/17/2018 8:33 PM, Long Li wrote:
>>> From: Long Li <longli@microsoft.com>
>>>
>>> The data buffer allocated on the stack can't be DMA'ed, and hence can't send
>>> through RDMA via SMB Direct.
>>
>> This comment is confusing. Any registered memory can be DMA'd, need to
>> state the reason for the choice here more clearly.
> 
> The stack could be allocated with vmalloc().
> In which case the pages might not be physically contiguous and there is no
> (sensible) call to get the physical address required by the dma controller
> (or other bus master).

Memory registration does not requires pages to be physically contiguous.
RDMA Regions can and do support very large physical page scatter/gather,
and the adapter DMA's them readily. Is this the only reason?

Tom.
--
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
Long Li April 18, 2018, 5:11 p.m. UTC | #4 
> Subject: Re: [Patch v3 2/6] cifs: Allocate validate negotiation request through
> kmalloc
> 
> On 4/18/2018 9:08 AM, David Laight wrote:
> > From: Tom Talpey
> >> Sent: 18 April 2018 12:32
> > ...
> >> On 4/17/2018 8:33 PM, Long Li wrote:
> >>> From: Long Li <longli@microsoft.com>
> >>>
> >>> The data buffer allocated on the stack can't be DMA'ed, and hence
> >>> can't send through RDMA via SMB Direct.
> >>
> >> This comment is confusing. Any registered memory can be DMA'd, need
> >> to state the reason for the choice here more clearly.
> >
> > The stack could be allocated with vmalloc().
> > In which case the pages might not be physically contiguous and there
> > is no
> > (sensible) call to get the physical address required by the dma
> > controller (or other bus master).
> 
> Memory registration does not requires pages to be physically contiguous.
> RDMA Regions can and do support very large physical page scatter/gather,
> and the adapter DMA's them readily. Is this the only reason?

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.

I think it makes sense as stack is dynamic and can shrink as I/O proceeds, so the buffer is gone. Other kernel code use only data on the heap for DMA, e.g. BLK/SCSI layer never use buffer on the stack to send data.

> 
> Tom.
Long Li April 18, 2018, 5:16 p.m. UTC | #5 
PiBTdWJqZWN0OiBSZTogW1BhdGNoIHYzIDIvNl0gY2lmczogQWxsb2NhdGUgdmFsaWRhdGUgbmVn
b3RpYXRpb24gcmVxdWVzdCB0aHJvdWdoDQo+IGttYWxsb2MNCj4gDQo+IFR3byBjb21tZW50cy4N
Cj4gDQo+IE9uIDQvMTcvMjAxOCA4OjMzIFBNLCBMb25nIExpIHdyb3RlOg0KPiA+IEZyb206IExv
bmcgTGkgPGxvbmdsaUBtaWNyb3NvZnQuY29tPg0KPiA+DQo+ID4gVGhlIGRhdGEgYnVmZmVyIGFs
bG9jYXRlZCBvbiB0aGUgc3RhY2sgY2FuJ3QgYmUgRE1BJ2VkLCBhbmQgaGVuY2UNCj4gPiBjYW4n
dCBzZW5kIHRocm91Z2ggUkRNQSB2aWEgU01CIERpcmVjdC4NCj4gDQo+IFRoaXMgY29tbWVudCBp
cyBjb25mdXNpbmcuIEFueSByZWdpc3RlcmVkIG1lbW9yeSBjYW4gYmUgRE1BJ2QsIG5lZWQgdG8N
Cj4gc3RhdGUgdGhlIHJlYXNvbiBmb3IgdGhlIGNob2ljZSBoZXJlIG1vcmUgY2xlYXJseS4NCj4g
DQo+ID4NCj4gPiBGaXggdGhpcyBieSBhbGxvY2F0aW5nIHRoZSByZXF1ZXN0IG9uIHRoZSBoZWFw
IGluIHNtYjNfdmFsaWRhdGVfbmVnb3RpYXRlLg0KPiA+DQo+ID4gQ2hhbmdlcyBpbiB2MjoNCj4g
PiBSZW1vdmVkIGR1cGxpY2F0ZWQgY29kZSBvbiBmcmVlaW5nIGJ1ZmZlcnMgb24gZnVuY3Rpb24g
ZXhpdC4NCj4gPiAoVGhhbmtzIHRvIFBhcmF2IFBhbmRpdCA8cGFyYXZAbWVsbGFub3guY29tPikg
Rml4ZWQgdHlwbyBpbiB0aGUgcGF0Y2gNCj4gPiB0aXRsZS4NCj4gPg0KPiA+IENoYW5nZXMgaW4g
djM6DQo+ID4gQWRkZWQgIkZpeGVzIiB0byB0aGUgcGF0Y2guDQo+ID4gQ2hhbmdlZCBzaXplb2Yo
KSB0byB1c2UgKnBvaW50ZXIgaW4gcGxhY2Ugb2Ygc3RydWN0Lg0KPiA+DQo+ID4gRml4ZXM6IGZm
MWMwMzhhZGRjNCAoIkNoZWNrIFNNQjMgZGlhbGVjdHMgYWdhaW5zdCBkb3duZ3JhZGUgYXR0YWNr
cyIpDQo+ID4gU2lnbmVkLW9mZi1ieTogTG9uZyBMaSA8bG9uZ2xpQG1pY3Jvc29mdC5jb20+DQo+
ID4gQ2M6IHN0YWJsZUB2Z2VyLmtlcm5lbC5vcmcNCj4gPiAtLS0NCj4gPiAgIGZzL2NpZnMvc21i
MnBkdS5jIHwgNTkgKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrLS0tLS0tLS0tLS0tLS0N
Cj4gLS0tLS0tLS0tLS0NCj4gPiAgIDEgZmlsZSBjaGFuZ2VkLCAzMiBpbnNlcnRpb25zKCspLCAy
NyBkZWxldGlvbnMoLSkNCj4gPg0KPiA+IGRpZmYgLS1naXQgYS9mcy9jaWZzL3NtYjJwZHUuYyBi
L2ZzL2NpZnMvc21iMnBkdS5jIGluZGV4DQo+ID4gMGYwNDRjNC4uNTU4MmEwMiAxMDA2NDQNCj4g
PiAtLS0gYS9mcy9jaWZzL3NtYjJwZHUuYw0KPiA+ICsrKyBiL2ZzL2NpZnMvc21iMnBkdS5jDQo+
ID4gQEAgLTcyOSw4ICs3MjksOCBAQCBTTUIyX25lZ290aWF0ZShjb25zdCB1bnNpZ25lZCBpbnQg
eGlkLCBzdHJ1Y3QNCj4gPiBjaWZzX3NlcyAqc2VzKQ0KPiA+DQo+ID4gICBpbnQgc21iM192YWxp
ZGF0ZV9uZWdvdGlhdGUoY29uc3QgdW5zaWduZWQgaW50IHhpZCwgc3RydWN0IGNpZnNfdGNvbiAq
dGNvbikNCj4gPiAgIHsNCj4gPiAtCWludCByYyA9IDA7DQo+ID4gLQlzdHJ1Y3QgdmFsaWRhdGVf
bmVnb3RpYXRlX2luZm9fcmVxIHZuZWdfaW5idWY7DQo+ID4gKwlpbnQgcmV0LCByYyA9IC1FSU87
DQo+ID4gKwlzdHJ1Y3QgdmFsaWRhdGVfbmVnb3RpYXRlX2luZm9fcmVxICpwbmVnX2luYnVmOw0K
PiA+ICAgCXN0cnVjdCB2YWxpZGF0ZV9uZWdvdGlhdGVfaW5mb19yc3AgKnBuZWdfcnNwID0gTlVM
TDsNCj4gPiAgIAl1MzIgcnNwbGVuOw0KPiA+ICAgCXUzMiBpbmJ1ZmxlbjsgLyogbWF4IG9mIDQg
ZGlhbGVjdHMgKi8gQEAgLTc0MSw2ICs3NDEsOSBAQCBpbnQNCj4gPiBzbWIzX3ZhbGlkYXRlX25l
Z290aWF0ZShjb25zdCB1bnNpZ25lZCBpbnQgeGlkLCBzdHJ1Y3QgY2lmc190Y29uICp0Y29uKQ0K
PiA+ICAgCWlmICh0Y29uLT5zZXMtPnNlcnZlci0+cmRtYSkNCj4gPiAgIAkJcmV0dXJuIDA7DQo+
ID4gICAjZW5kaWYNCj4gPiArCXBuZWdfaW5idWYgPSBrbWFsbG9jKHNpemVvZigqcG5lZ19pbmJ1
ZiksIEdGUF9LRVJORUwpOw0KPiA+ICsJaWYgKCFwbmVnX2luYnVmKQ0KPiA+ICsJCXJldHVybiAt
RU5PTUVNOw0KPiANCj4gV2h5IGlzIHRoaXMgYSBub25ibG9ja2luZyBhbGxvY2F0aW9uPyBJdCB3
b3VsZCBzZWVtIG1vcmUgcm9idXN0IHRvIHVzZQ0KPiBHRlBfTk9GUyBoZXJlLg0KDQpJIGFncmVl
IGl0IG1ha2VzIHNlbnNlIHRvIHVzZSBHRlBfTk9GUy4gDQoNClRoZSBjaG9pY2UgaGVyZSBpcyBt
YWRlIGNvbnNpc3RlbnQgd2l0aCBhbGwgdGhlIHJlc3QgQ0lGUyBjb2RlIGFsbG9jYXRpbmcgcHJv
dG9jb2wgcmVxdWVzdCBidWZmZXJzLiBNYXliZSB3ZSBzaG91bGQgZG8gYW5vdGhlciBwYXRjaCB0
byBjbGVhbnVwIGFsbCB0aG9zZSBjb2RlLg0KDQo+IA0KPiBUb20uDQo+IA0KPiA+DQo+ID4gICAJ
LyogSW4gU01CMy4xMSBwcmVhdXRoIGludGVncml0eSBzdXBlcnNlZGVzIHZhbGlkYXRlIG5lZ290
aWF0ZSAqLw0KPiA+ICAgCWlmICh0Y29uLT5zZXMtPnNlcnZlci0+ZGlhbGVjdCA9PSBTTUIzMTFf
UFJPVF9JRCkgQEAgLTc2NCw2Mw0KPiA+ICs3NjcsNjMgQEAgaW50IHNtYjNfdmFsaWRhdGVfbmVn
b3RpYXRlKGNvbnN0IHVuc2lnbmVkIGludCB4aWQsIHN0cnVjdA0KPiBjaWZzX3Rjb24gKnRjb24p
DQo+ID4gICAJaWYgKHRjb24tPnNlcy0+c2Vzc2lvbl9mbGFncyAmIFNNQjJfU0VTU0lPTl9GTEFH
X0lTX05VTEwpDQo+ID4gICAJCWNpZnNfZGJnKFZGUywgIlVuZXhwZWN0ZWQgbnVsbCB1c2VyIChh
bm9ueW1vdXMpIGF1dGggZmxhZw0KPiBzZW50IGJ5DQo+ID4gc2VydmVyXG4iKTsNCj4gPg0KPiA+
IC0Jdm5lZ19pbmJ1Zi5DYXBhYmlsaXRpZXMgPQ0KPiA+ICsJcG5lZ19pbmJ1Zi0+Q2FwYWJpbGl0
aWVzID0NCj4gPiAgIAkJCWNwdV90b19sZTMyKHRjb24tPnNlcy0+c2VydmVyLT52YWxzLQ0KPiA+
cmVxX2NhcGFiaWxpdGllcyk7DQo+ID4gLQltZW1jcHkodm5lZ19pbmJ1Zi5HdWlkLCB0Y29uLT5z
ZXMtPnNlcnZlci0+Y2xpZW50X2d1aWQsDQo+ID4gKwltZW1jcHkocG5lZ19pbmJ1Zi0+R3VpZCwg
dGNvbi0+c2VzLT5zZXJ2ZXItPmNsaWVudF9ndWlkLA0KPiA+ICAgCQkJCQlTTUIyX0NMSUVOVF9H
VUlEX1NJWkUpOw0KPiA+DQo+ID4gICAJaWYgKHRjb24tPnNlcy0+c2lnbikNCj4gPiAtCQl2bmVn
X2luYnVmLlNlY3VyaXR5TW9kZSA9DQo+ID4gKwkJcG5lZ19pbmJ1Zi0+U2VjdXJpdHlNb2RlID0N
Cj4gPg0KPiAJY3B1X3RvX2xlMTYoU01CMl9ORUdPVElBVEVfU0lHTklOR19SRVFVSVJFRCk7DQo+
ID4gICAJZWxzZSBpZiAoZ2xvYmFsX3NlY2ZsYWdzICYgQ0lGU1NFQ19NQVlfU0lHTikNCj4gPiAt
CQl2bmVnX2luYnVmLlNlY3VyaXR5TW9kZSA9DQo+ID4gKwkJcG5lZ19pbmJ1Zi0+U2VjdXJpdHlN
b2RlID0NCj4gPg0KPiAJY3B1X3RvX2xlMTYoU01CMl9ORUdPVElBVEVfU0lHTklOR19FTkFCTEVE
KTsNCj4gPiAgIAllbHNlDQo+ID4gLQkJdm5lZ19pbmJ1Zi5TZWN1cml0eU1vZGUgPSAwOw0KPiA+
ICsJCXBuZWdfaW5idWYtPlNlY3VyaXR5TW9kZSA9IDA7DQo+ID4NCj4gPg0KPiA+ICAgCWlmIChz
dHJjbXAodGNvbi0+c2VzLT5zZXJ2ZXItPnZhbHMtPnZlcnNpb25fc3RyaW5nLA0KPiA+ICAgCQlT
TUIzQU5ZX1ZFUlNJT05fU1RSSU5HKSA9PSAwKSB7DQo+ID4gLQkJdm5lZ19pbmJ1Zi5EaWFsZWN0
c1swXSA9IGNwdV90b19sZTE2KFNNQjMwX1BST1RfSUQpOw0KPiA+IC0JCXZuZWdfaW5idWYuRGlh
bGVjdHNbMV0gPSBjcHVfdG9fbGUxNihTTUIzMDJfUFJPVF9JRCk7DQo+ID4gLQkJdm5lZ19pbmJ1
Zi5EaWFsZWN0Q291bnQgPSBjcHVfdG9fbGUxNigyKTsNCj4gPiArCQlwbmVnX2luYnVmLT5EaWFs
ZWN0c1swXSA9IGNwdV90b19sZTE2KFNNQjMwX1BST1RfSUQpOw0KPiA+ICsJCXBuZWdfaW5idWYt
PkRpYWxlY3RzWzFdID0gY3B1X3RvX2xlMTYoU01CMzAyX1BST1RfSUQpOw0KPiA+ICsJCXBuZWdf
aW5idWYtPkRpYWxlY3RDb3VudCA9IGNwdV90b19sZTE2KDIpOw0KPiA+ICAgCQkvKiBzdHJ1Y3R1
cmUgaXMgYmlnIGVub3VnaCBmb3IgMyBkaWFsZWN0cywgc2VuZGluZyBvbmx5IDIgKi8NCj4gPiAg
IAkJaW5idWZsZW4gPSBzaXplb2Yoc3RydWN0IHZhbGlkYXRlX25lZ290aWF0ZV9pbmZvX3JlcSkg
LSAyOw0KPiA+ICAgCX0gZWxzZSBpZiAoc3RyY21wKHRjb24tPnNlcy0+c2VydmVyLT52YWxzLT52
ZXJzaW9uX3N0cmluZywNCj4gPiAgIAkJU01CREVGQVVMVF9WRVJTSU9OX1NUUklORykgPT0gMCkg
ew0KPiA+IC0JCXZuZWdfaW5idWYuRGlhbGVjdHNbMF0gPSBjcHVfdG9fbGUxNihTTUIyMV9QUk9U
X0lEKTsNCj4gPiAtCQl2bmVnX2luYnVmLkRpYWxlY3RzWzFdID0gY3B1X3RvX2xlMTYoU01CMzBf
UFJPVF9JRCk7DQo+ID4gLQkJdm5lZ19pbmJ1Zi5EaWFsZWN0c1syXSA9IGNwdV90b19sZTE2KFNN
QjMwMl9QUk9UX0lEKTsNCj4gPiAtCQl2bmVnX2luYnVmLkRpYWxlY3RDb3VudCA9IGNwdV90b19s
ZTE2KDMpOw0KPiA+ICsJCXBuZWdfaW5idWYtPkRpYWxlY3RzWzBdID0gY3B1X3RvX2xlMTYoU01C
MjFfUFJPVF9JRCk7DQo+ID4gKwkJcG5lZ19pbmJ1Zi0+RGlhbGVjdHNbMV0gPSBjcHVfdG9fbGUx
NihTTUIzMF9QUk9UX0lEKTsNCj4gPiArCQlwbmVnX2luYnVmLT5EaWFsZWN0c1syXSA9IGNwdV90
b19sZTE2KFNNQjMwMl9QUk9UX0lEKTsNCj4gPiArCQlwbmVnX2luYnVmLT5EaWFsZWN0Q291bnQg
PSBjcHVfdG9fbGUxNigzKTsNCj4gPiAgIAkJLyogc3RydWN0dXJlIGlzIGJpZyBlbm91Z2ggZm9y
IDMgZGlhbGVjdHMgKi8NCj4gPiAgIAkJaW5idWZsZW4gPSBzaXplb2Yoc3RydWN0IHZhbGlkYXRl
X25lZ290aWF0ZV9pbmZvX3JlcSk7DQo+ID4gICAJfSBlbHNlIHsNCj4gPiAgIAkJLyogb3RoZXJ3
aXNlIHNwZWNpZmljIGRpYWxlY3Qgd2FzIHJlcXVlc3RlZCAqLw0KPiA+IC0JCXZuZWdfaW5idWYu
RGlhbGVjdHNbMF0gPQ0KPiA+ICsJCXBuZWdfaW5idWYtPkRpYWxlY3RzWzBdID0NCj4gPiAgIAkJ
CWNwdV90b19sZTE2KHRjb24tPnNlcy0+c2VydmVyLT52YWxzLT5wcm90b2NvbF9pZCk7DQo+ID4g
LQkJdm5lZ19pbmJ1Zi5EaWFsZWN0Q291bnQgPSBjcHVfdG9fbGUxNigxKTsNCj4gPiArCQlwbmVn
X2luYnVmLT5EaWFsZWN0Q291bnQgPSBjcHVfdG9fbGUxNigxKTsNCj4gPiAgIAkJLyogc3RydWN0
dXJlIGlzIGJpZyBlbm91Z2ggZm9yIDMgZGlhbGVjdHMsIHNlbmRpbmcgb25seSAxICovDQo+ID4g
ICAJCWluYnVmbGVuID0gc2l6ZW9mKHN0cnVjdCB2YWxpZGF0ZV9uZWdvdGlhdGVfaW5mb19yZXEp
IC0gNDsNCj4gPiAgIAl9DQo+ID4NCj4gPiAtCXJjID0gU01CMl9pb2N0bCh4aWQsIHRjb24sIE5P
X0ZJTEVfSUQsIE5PX0ZJTEVfSUQsDQo+ID4gKwlyZXQgPSBTTUIyX2lvY3RsKHhpZCwgdGNvbiwg
Tk9fRklMRV9JRCwgTk9fRklMRV9JRCwNCj4gPiAgIAkJRlNDVExfVkFMSURBVEVfTkVHT1RJQVRF
X0lORk8sIHRydWUgLyogaXNfZnNjdGwgKi8sDQo+ID4gLQkJKGNoYXIgKikmdm5lZ19pbmJ1Ziwg
c2l6ZW9mKHN0cnVjdA0KPiB2YWxpZGF0ZV9uZWdvdGlhdGVfaW5mb19yZXEpLA0KPiA+ICsJCShj
aGFyICopcG5lZ19pbmJ1Ziwgc2l6ZW9mKCpwbmVnX2luYnVmKSwNCj4gPiAgIAkJKGNoYXIgKiop
JnBuZWdfcnNwLCAmcnNwbGVuKTsNCj4gPg0KPiA+IC0JaWYgKHJjICE9IDApIHsNCj4gPiAtCQlj
aWZzX2RiZyhWRlMsICJ2YWxpZGF0ZSBwcm90b2NvbCBuZWdvdGlhdGUgZmFpbGVkOiAlZFxuIiwg
cmMpOw0KPiA+IC0JCXJldHVybiAtRUlPOw0KPiA+ICsJaWYgKHJldCkgew0KPiA+ICsJCWNpZnNf
ZGJnKFZGUywgInZhbGlkYXRlIHByb3RvY29sIG5lZ290aWF0ZSBmYWlsZWQ6ICVkXG4iLCByZXQp
Ow0KPiA+ICsJCWdvdG8gb3V0X2ZyZWVfaW5idWY7DQo+ID4gICAJfQ0KPiA+DQo+ID4gLQlpZiAo
cnNwbGVuICE9IHNpemVvZihzdHJ1Y3QgdmFsaWRhdGVfbmVnb3RpYXRlX2luZm9fcnNwKSkgew0K
PiA+ICsJaWYgKHJzcGxlbiAhPSBzaXplb2YoKnBuZWdfcnNwKSkgew0KPiA+ICAgCQljaWZzX2Ri
ZyhWRlMsICJpbnZhbGlkIHByb3RvY29sIG5lZ290aWF0ZSByZXNwb25zZQ0KPiBzaXplOiAlZFxu
IiwNCj4gPiAgIAkJCSByc3BsZW4pOw0KPiA+DQo+ID4gICAJCS8qIHJlbGF4IGNoZWNrIHNpbmNl
IE1hYyByZXR1cm5zIG1heCBidWZzaXplIGFsbG93ZWQgb24gaW9jdGwNCj4gKi8NCj4gPiAgIAkJ
aWYgKChyc3BsZW4gPiBDSUZTTWF4QnVmU2l6ZSkNCj4gPiAgIAkJICAgICB8fCAocnNwbGVuIDwg
c2l6ZW9mKHN0cnVjdCB2YWxpZGF0ZV9uZWdvdGlhdGVfaW5mb19yc3ApKSkNCj4gPiAtCQkJZ290
byBlcnJfcnNwX2ZyZWU7DQo+ID4gKwkJCWdvdG8gb3V0X2ZyZWVfcnNwOw0KPiA+ICAgCX0NCj4g
Pg0KPiA+ICAgCS8qIGNoZWNrIHZhbGlkYXRlIG5lZ290aWF0ZSBpbmZvIHJlc3BvbnNlIG1hdGNo
ZXMgd2hhdCB3ZSBnb3QNCj4gPiBlYXJsaWVyICovIEBAIC04MzgsMTQgKzg0MSwxNiBAQCBpbnQg
c21iM192YWxpZGF0ZV9uZWdvdGlhdGUoY29uc3QNCj4gPiB1bnNpZ25lZCBpbnQgeGlkLCBzdHJ1
Y3QgY2lmc190Y29uICp0Y29uKQ0KPiA+DQo+ID4gICAJLyogdmFsaWRhdGUgbmVnb3RpYXRlIHN1
Y2Nlc3NmdWwgKi8NCj4gPiAgIAljaWZzX2RiZyhGWUksICJ2YWxpZGF0ZSBuZWdvdGlhdGUgaW5m
byBzdWNjZXNzZnVsXG4iKTsNCj4gPiAtCWtmcmVlKHBuZWdfcnNwKTsNCj4gPiAtCXJldHVybiAw
Ow0KPiA+ICsJcmMgPSAwOw0KPiA+ICsJZ290byBvdXRfZnJlZV9yc3A7DQo+ID4NCj4gPiAgIHZu
ZWdfb3V0Og0KPiA+ICAgCWNpZnNfZGJnKFZGUywgInByb3RvY29sIHJldmFsaWRhdGlvbiAtIHNl
Y3VyaXR5IHNldHRpbmdzDQo+ID4gbWlzbWF0Y2hcbiIpOw0KPiA+IC1lcnJfcnNwX2ZyZWU6DQo+
ID4gK291dF9mcmVlX3JzcDoNCj4gPiAgIAlrZnJlZShwbmVnX3JzcCk7DQo+ID4gLQlyZXR1cm4g
LUVJTzsNCj4gPiArb3V0X2ZyZWVfaW5idWY6DQo+ID4gKwlrZnJlZShwbmVnX2luYnVmKTsNCj4g
PiArCXJldHVybiByYzsNCj4gPiAgIH0NCj4gPg0KPiA+ICAgZW51bSBzZWN1cml0eUVudW0NCj4g
Pg0K
--
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
Tom Talpey April 18, 2018, 5:40 p.m. UTC | #6 
On 4/18/2018 1:11 PM, Long Li wrote:
>> Subject: Re: [Patch v3 2/6] cifs: Allocate validate negotiation request through
>> kmalloc
>>
>> On 4/18/2018 9:08 AM, David Laight wrote:
>>> From: Tom Talpey
>>>> Sent: 18 April 2018 12:32
>>> ...
>>>> On 4/17/2018 8:33 PM, Long Li wrote:
>>>>> From: Long Li <longli@microsoft.com>
>>>>>
>>>>> The data buffer allocated on the stack can't be DMA'ed, and hence
>>>>> can't send through RDMA via SMB Direct.
>>>>
>>>> This comment is confusing. Any registered memory can be DMA'd, need
>>>> to state the reason for the choice here more clearly.
>>>
>>> The stack could be allocated with vmalloc().
>>> In which case the pages might not be physically contiguous and there
>>> is no
>>> (sensible) call to get the physical address required by the dma
>>> controller (or other bus master).
>>
>> Memory registration does not requires pages to be physically contiguous.
>> RDMA Regions can and do support very large physical page scatter/gather,
>> and the adapter DMA's them readily. Is this the only reason?
> 
> 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.
> 
> I think it makes sense as stack is dynamic and can shrink as I/O proceeds, so the buffer is gone. Other kernel code use only data on the heap for DMA, e.g. BLK/SCSI layer never use buffer on the stack to send data.

I totally agree that registering the stack is a bad idea. I mainly
suggest that you capture these fundamental ib_dma* reasons in the
commit. There's no other practical reason why the original approach
would not work.

Tom.
--
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
Tom Talpey April 18, 2018, 5:42 p.m. UTC | #7 
On 4/18/2018 1:16 PM, Long Li wrote:
>> Subject: Re: [Patch v3 2/6] cifs: Allocate validate negotiation request through
>> kmalloc
>>
>> Two comments.
>>
>> On 4/17/2018 8:33 PM, Long Li wrote:
>>> From: Long Li <longli@microsoft.com>
>>>
>>> The data buffer allocated on the stack can't be DMA'ed, and hence
>>> can't send through RDMA via SMB Direct.
>>
>> This comment is confusing. Any registered memory can be DMA'd, need to
>> state the reason for the choice here more clearly.
>>
>>>
>>> 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 sizeof() to use *pointer in place of struct.
>>>
>>> Fixes: ff1c038addc4 ("Check SMB3 dialects against downgrade attacks")
>>> Signed-off-by: Long Li <longli@microsoft.com>
>>> Cc: stable@vger.kernel.org
>>> ---
>>>    fs/cifs/smb2pdu.c | 59 ++++++++++++++++++++++++++++++--------------
>> -----------
>>>    1 file changed, 32 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index
>>> 0f044c4..5582a02 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,6 +741,9 @@ int
>>> smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
>>>    	if (tcon->ses->server->rdma)
>>>    		return 0;
>>>    #endif
>>> +	pneg_inbuf = kmalloc(sizeof(*pneg_inbuf), GFP_KERNEL);
>>> +	if (!pneg_inbuf)
>>> +		return -ENOMEM;
>>
>> Why is this a nonblocking allocation? It would seem more robust to use
>> GFP_NOFS here.
> 
> I agree it makes sense to use GFP_NOFS.
> 
> The choice here is made consistent with all the rest CIFS code allocating protocol request buffers. Maybe we should do another patch to cleanup all those code.

It'll be required sooner or later. I'm agnostic as to how you apply it,
but I still suggest you change this one now rather than continue the
fragile behavior. It may not be a global search-and-replace since some
allocations may require nonblocking.


> 
>>
>> Tom.
>>
>>>
>>>    	/* In SMB3.11 preauth integrity supersedes validate negotiate */
>>>    	if (tcon->ses->server->dialect == SMB311_PROT_ID) @@ -764,63
>>> +767,63 @@ 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->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
>>>
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{��ٚ�{ay�ʇڙ�,j��f���h���z��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  http://vger.kernel.org/majordomo-info.html
Long Li April 18, 2018, 6:53 p.m. UTC | #8 
PiBTdWJqZWN0OiBSZTogW1BhdGNoIHYzIDIvNl0gY2lmczogQWxsb2NhdGUgdmFsaWRhdGUgbmVn
b3RpYXRpb24gcmVxdWVzdCB0aHJvdWdoDQo+IGttYWxsb2MNCj4gDQo+IE9uIDQvMTgvMjAxOCAx
OjExIFBNLCBMb25nIExpIHdyb3RlOg0KPiA+PiBTdWJqZWN0OiBSZTogW1BhdGNoIHYzIDIvNl0g
Y2lmczogQWxsb2NhdGUgdmFsaWRhdGUgbmVnb3RpYXRpb24NCj4gPj4gcmVxdWVzdCB0aHJvdWdo
IGttYWxsb2MNCj4gPj4NCj4gPj4gT24gNC8xOC8yMDE4IDk6MDggQU0sIERhdmlkIExhaWdodCB3
cm90ZToNCj4gPj4+IEZyb206IFRvbSBUYWxwZXkNCj4gPj4+PiBTZW50OiAxOCBBcHJpbCAyMDE4
IDEyOjMyDQo+ID4+PiAuLi4NCj4gPj4+PiBPbiA0LzE3LzIwMTggODozMyBQTSwgTG9uZyBMaSB3
cm90ZToNCj4gPj4+Pj4gRnJvbTogTG9uZyBMaSA8bG9uZ2xpQG1pY3Jvc29mdC5jb20+DQo+ID4+
Pj4+DQo+ID4+Pj4+IFRoZSBkYXRhIGJ1ZmZlciBhbGxvY2F0ZWQgb24gdGhlIHN0YWNrIGNhbid0
IGJlIERNQSdlZCwgYW5kIGhlbmNlDQo+ID4+Pj4+IGNhbid0IHNlbmQgdGhyb3VnaCBSRE1BIHZp
YSBTTUIgRGlyZWN0Lg0KPiA+Pj4+DQo+ID4+Pj4gVGhpcyBjb21tZW50IGlzIGNvbmZ1c2luZy4g
QW55IHJlZ2lzdGVyZWQgbWVtb3J5IGNhbiBiZSBETUEnZCwNCj4gbmVlZA0KPiA+Pj4+IHRvIHN0
YXRlIHRoZSByZWFzb24gZm9yIHRoZSBjaG9pY2UgaGVyZSBtb3JlIGNsZWFybHkuDQo+ID4+Pg0K
PiA+Pj4gVGhlIHN0YWNrIGNvdWxkIGJlIGFsbG9jYXRlZCB3aXRoIHZtYWxsb2MoKS4NCj4gPj4+
IEluIHdoaWNoIGNhc2UgdGhlIHBhZ2VzIG1pZ2h0IG5vdCBiZSBwaHlzaWNhbGx5IGNvbnRpZ3Vv
dXMgYW5kIHRoZXJlDQo+ID4+PiBpcyBubw0KPiA+Pj4gKHNlbnNpYmxlKSBjYWxsIHRvIGdldCB0
aGUgcGh5c2ljYWwgYWRkcmVzcyByZXF1aXJlZCBieSB0aGUgZG1hDQo+ID4+PiBjb250cm9sbGVy
IChvciBvdGhlciBidXMgbWFzdGVyKS4NCj4gPj4NCj4gPj4gTWVtb3J5IHJlZ2lzdHJhdGlvbiBk
b2VzIG5vdCByZXF1aXJlcyBwYWdlcyB0byBiZSBwaHlzaWNhbGx5IGNvbnRpZ3VvdXMuDQo+ID4+
IFJETUEgUmVnaW9ucyBjYW4gYW5kIGRvIHN1cHBvcnQgdmVyeSBsYXJnZSBwaHlzaWNhbCBwYWdl
DQo+ID4+IHNjYXR0ZXIvZ2F0aGVyLCBhbmQgdGhlIGFkYXB0ZXIgRE1BJ3MgdGhlbSByZWFkaWx5
LiBJcyB0aGlzIHRoZSBvbmx5DQo+IHJlYXNvbj8NCj4gPg0KPiA+IGliX2RtYV9tYXBfcGFnZSB3
aWxsIHJldHVybiBhbiBpbnZhbGlkIERNQSBhZGRyZXNzIGZvciBhIGJ1ZmZlciBvbiBzdGFjay4N
Cj4gRXZlbiB3b3JzZSwgdGhpcyBpbmNvcnJlY3QgYWRkcmVzcyBjYW4ndCBiZSBkZXRlY3RlZCBi
eQ0KPiBpYl9kbWFfbWFwcGluZ19lcnJvci4gU2VuZGluZyBkYXRhIGZyb20gdGhpcyBhZGRyZXNz
IHRvIGhhcmR3YXJlIHdpbGwgbm90DQo+IGZhaWwsIGJ1dCB0aGUgcmVtb3RlIHBlZXIgd2lsbCBn
ZXQganVuayBkYXRhLg0KPiA+DQo+ID4gSSB0aGluayBpdCBtYWtlcyBzZW5zZSBhcyBzdGFjayBp
cyBkeW5hbWljIGFuZCBjYW4gc2hyaW5rIGFzIEkvTyBwcm9jZWVkcywgc28NCj4gdGhlIGJ1ZmZl
ciBpcyBnb25lLiBPdGhlciBrZXJuZWwgY29kZSB1c2Ugb25seSBkYXRhIG9uIHRoZSBoZWFwIGZv
ciBETUEsIGUuZy4NCj4gQkxLL1NDU0kgbGF5ZXIgbmV2ZXIgdXNlIGJ1ZmZlciBvbiB0aGUgc3Rh
Y2sgdG8gc2VuZCBkYXRhLg0KPiANCj4gSSB0b3RhbGx5IGFncmVlIHRoYXQgcmVnaXN0ZXJpbmcg
dGhlIHN0YWNrIGlzIGEgYmFkIGlkZWEuIEkgbWFpbmx5IHN1Z2dlc3QgdGhhdA0KPiB5b3UgY2Fw
dHVyZSB0aGVzZSBmdW5kYW1lbnRhbCBpYl9kbWEqIHJlYXNvbnMgaW4gdGhlIGNvbW1pdC4gVGhl
cmUncyBubw0KPiBvdGhlciBwcmFjdGljYWwgcmVhc29uIHdoeSB0aGUgb3JpZ2luYWwgYXBwcm9h
Y2ggd291bGQgbm90IHdvcmsuDQoNClN1cmUgSSB3aWxsIGZpeCB0aGF0Lg0K
--
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
Long Li April 18, 2018, 6:53 p.m. UTC | #9 
> Subject: Re: [Patch v3 2/6] cifs: Allocate validate negotiation request through
> kmalloc
> 
> On 4/18/2018 1:16 PM, Long Li wrote:
> >> Subject: Re: [Patch v3 2/6] cifs: Allocate validate negotiation
> >> request through kmalloc
> >>
> >> Two comments.
> >>
> >> On 4/17/2018 8:33 PM, Long Li wrote:
> >>> From: Long Li <longli@microsoft.com>
> >>>
> >>> The data buffer allocated on the stack can't be DMA'ed, and hence
> >>> can't send through RDMA via SMB Direct.
> >>
> >> This comment is confusing. Any registered memory can be DMA'd, need
> >> to state the reason for the choice here more clearly.
> >>
> >>>
> >>> 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 sizeof() to use *pointer in place of struct.
> >>>
> >>> Fixes: ff1c038addc4 ("Check SMB3 dialects against downgrade
> >>> attacks")
> >>> Signed-off-by: Long Li <longli@microsoft.com>
> >>> Cc: stable@vger.kernel.org
> >>> ---
> >>>    fs/cifs/smb2pdu.c | 59
> >>> ++++++++++++++++++++++++++++++--------------
> >> -----------
> >>>    1 file changed, 32 insertions(+), 27 deletions(-)
> >>>
> >>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index
> >>> 0f044c4..5582a02 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,6 +741,9 @@ int
> >>> smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
> >>>    	if (tcon->ses->server->rdma)
> >>>    		return 0;
> >>>    #endif
> >>> +	pneg_inbuf = kmalloc(sizeof(*pneg_inbuf), GFP_KERNEL);
> >>> +	if (!pneg_inbuf)
> >>> +		return -ENOMEM;
> >>
> >> Why is this a nonblocking allocation? It would seem more robust to
> >> use GFP_NOFS here.
> >
> > I agree it makes sense to use GFP_NOFS.
> >
> > The choice here is made consistent with all the rest CIFS code allocating
> protocol request buffers. Maybe we should do another patch to cleanup all
> those code.
> 
> It'll be required sooner or later. I'm agnostic as to how you apply it, but I still
> suggest you change this one now rather than continue the fragile behavior. It
> may not be a global search-and-replace since some allocations may require
> nonblocking.

Okay, I will fix this.

> 
> 
> >
> >>
> >> Tom.
> >>
> >>>
> >>>    	/* In SMB3.11 preauth integrity supersedes validate negotiate */
> >>>    	if (tcon->ses->server->dialect == SMB311_PROT_ID) @@ -764,63
> >>> +767,63 @@ 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->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
> >>>
> > N     r  y   b X  ǧv ^ )޺{.n +    {  ٚ {ay ʇڙ ,j   f   h   z  w
> 
>    j:+v   w j m         zZ+     ݢj"  !tml=
> >
diff mbox series

Patch

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 0f044c4..5582a02 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,6 +741,9 @@  int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 	if (tcon->ses->server->rdma)
 		return 0;
 #endif
+	pneg_inbuf = kmalloc(sizeof(*pneg_inbuf), GFP_KERNEL);
+	if (!pneg_inbuf)
+		return -ENOMEM;
 
 	/* In SMB3.11 preauth integrity supersedes validate negotiate */
 	if (tcon->ses->server->dialect == SMB311_PROT_ID)
@@ -764,63 +767,63 @@  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->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