* [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 related [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@vger.kernel.org, samba-technical@lists.samba.org, linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org > 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@vger.kernel.org, samba-technical@lists.samba.org, linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org 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@vger.kernel.org, samba-technical@lists.samba.org, linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org > 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