RE: [Patch v2 2/6] cifs: Allocate validate negotiation request through kmalloc

-----Original Message----- From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- owner@vger.kernel.org] On Behalf Of Long Li Sent: Tuesday, April 17, 2018 2:17 PM To: Steve French sfrench@samba.org; linux-cifs@vger.kernel.org; samba- technical@lists.samba.org; linux-kernel@vger.kernel.org; linux- rdma@vger.kernel.org Cc: longli longli@microsoft.com; stable@vger.kernel.org Subject: [Patch v2 2/6] cifs: Allocate validate negotiation request through kmalloc

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.

Fixes: ff1c038addc4f205d5f1ede449426c7d316c0eed "Check SMB3 dialects against downgrade attacks"

Format is: Fixes: ff1c038addc4 ("Check SMB3 dialects against downgrade attacks") It should be right above Signed-off signature.

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.

Signed-off-by: Long Li longli@microsoft.com Cc: stable@vger.kernel.org

---

fs/cifs/smb2pdu.c | 57 ++++++++++++++++++++++++++++++------------------------

1 file changed, 31 insertions(+), 26 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 0f044c4..41625e4 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,53

+767,53 @@ 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,

Please check if strncmp() should be used or not.

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(struct validate_negotiate_info_req),
  

Use sizeof(*pneg_inbuf)

(char **)&pneg_rsp, &rsplen);

• if (rc != 0) {

• cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", rc);
  

• return -EIO;
  

• if (ret != 0) {

if (ret) is fine.

• cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", ret);
  

• goto out_free_inbuf;
  

  }

  if (rsplen != sizeof(struct validate_negotiate_info_rsp)) { @@ -820,7

if (rsplen != sizeof(*pneg_rsp))