| Message ID | 20180418003358.25098-1-longli@linuxonhyperv.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
> 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.
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
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
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
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
> 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 --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