From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Cyrus-Session-Id: sloti22d1t05-909730-1524073394-2-8789698520885484362 X-Sieve: CMU Sieve 3.0 X-Spam-known-sender: no X-Spam-score: 0.0 X-Spam-hits: BAYES_00 -1.9, HEADER_FROM_DIFFERENT_DOMAINS 0.25, MAILING_LIST_MULTI -1, ME_NOAUTH 0.01, RCVD_IN_DNSWL_HI -5, LANGUAGES unknown, BAYES_USED global, SA_VERSION 3.4.0 X-Spam-source: IP='209.132.180.67', Host='vger.kernel.org', Country='US', FromHeader='com', MailFrom='org' X-Spam-charsets: plain='utf-8' X-Resolved-to: greg@kroah.com X-Delivered-to: greg@kroah.com X-Mail-from: stable-owner@vger.kernel.org ARC-Seal: i=1; a=rsa-sha256; cv=none; d=messagingengine.com; s=fm2; t= 1524073393; b=ucnhBtKc9dRgz0MFTUFeq5XRFa1g+s/DASfeLtabbEWk/agKOw it/oKT+SuAC/Fwt125W8Qzpt2SoZ72ocSM2MN9SBOiRVj10bBUDOZOuT/LQGuFDq +hGiMms80DNn4R6Y4Yvz3zz7j/EIG8vyVlzG3LMesj+0dQ9SYlEwo84e9dr/nm2s ndJhmeSHDnbxyInGNWPO2e2E66MAKAzGGVtKFDrgRCsCKb/B7emGFPmAKpwgN4Zi ob5OD2L6PNvkiPFmF6ynunI5kXm4N2i9C/RlNxmC/hSX14YWt2nNAfexgYZ419Vg rrx53gzvSC60sQfaKNdw3+e/brxzNXnf2egg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=subject:to:cc:references:from:message-id :date:mime-version:in-reply-to:content-type :content-transfer-encoding:sender:list-id; s=fm2; t=1524073393; bh=RcMtlKM/teT9DMqDkSY/tj7fwBJk7jac4IH5//dzmD8=; b=PZXEPtkZRV4L 7LnVy4rVywdAmVTFPzZv3SZf754UWSltATUUws4kz6wZCnGJLG24mTGpvAZh00ND CUGZ1zSZjDO2I40ysoCbrT7biVVOFirlc/sLjhUP7C0+ImucGZGixY3HIhswFqgT +V2v2QqpIQ7SN46Ar1eVoXUd1PgDrKKMc8pnsYPzqoUw1wDPYHJ/2y2wTW7d2vs5 8qhT5jgGIL2yeymFK6IUVvJS3AVU5Y5Xi3/11yMjWxkt9UMpHU58XI+/pG5Snvq2 bNkBB7El2Sy//9Rv0CwtZKD333zahhpvCKTZHjK2s8oVRNSjfxW24zq/wRHGRYIi Q+upK82nNA== ARC-Authentication-Results: i=1; mx3.messagingengine.com; arc=none (no signatures found); dkim=none (no signatures found); dmarc=none (p=none,has-list-id=yes,d=none) header.from=talpey.com; iprev=pass policy.iprev=209.132.180.67 (vger.kernel.org); spf=none smtp.mailfrom=stable-owner@vger.kernel.org smtp.helo=vger.kernel.org; x-aligned-from=fail; x-cm=none score=0; x-ptr=pass x-ptr-helo=vger.kernel.org x-ptr-lookup=vger.kernel.org; x-return-mx=pass smtp.domain=vger.kernel.org smtp.result=pass smtp_org.domain=kernel.org smtp_org.result=pass smtp_is_org_domain=no header.domain=talpey.com header.result=pass header_is_org_domain=yes; x-vs=clean score=-100 state=0 Authentication-Results: mx3.messagingengine.com; arc=none (no signatures found); dkim=none (no signatures found); dmarc=none (p=none,has-list-id=yes,d=none) header.from=talpey.com; iprev=pass policy.iprev=209.132.180.67 (vger.kernel.org); spf=none smtp.mailfrom=stable-owner@vger.kernel.org smtp.helo=vger.kernel.org; x-aligned-from=fail; x-cm=none score=0; x-ptr=pass x-ptr-helo=vger.kernel.org x-ptr-lookup=vger.kernel.org; x-return-mx=pass smtp.domain=vger.kernel.org smtp.result=pass smtp_org.domain=kernel.org smtp_org.result=pass smtp_is_org_domain=no header.domain=talpey.com header.result=pass header_is_org_domain=yes; x-vs=clean score=-100 state=0 X-ME-VSCategory: clean X-CM-Envelope: MS4wfIT1opJisPeDzYoUq2CBcSCaa2GCq31yiyWT9/LuFnNUJ+Mi3d+jqgu1NegX+j9CeBxX0uhAMOmhxPvgkO2V2Ydi7M6rza5XThFJTq+jF3cp3p1SUNvk uy6mNKPTbeDzC5dzGsig/Phe9kNvgZ+AHaBk0DkJS7lJuM9g90y5fqBhykGqgwmUnJybfHv5PHqMBuuxrhV6yApD3EmWef+seNf30395EAkZvbHf/cXOGcrp X-CM-Analysis: v=2.3 cv=Tq3Iegfh c=1 sm=1 tr=0 a=UK1r566ZdBxH71SXbqIOeA==:117 a=UK1r566ZdBxH71SXbqIOeA==:17 a=h6HYFqFZYC0A:10 a=IkcTkHD0fZMA:10 a=Kd1tUaAdevIA:10 a=yMhMjlubAAAA:8 a=CbDCq_QkAAAA:8 a=VwQbUJbxAAAA:8 a=BOB88Glp3cpl7VA-vX8A:9 a=QEXdDO2ut3YA:10 a=1qrBK16LubpBFNPVNq2M:22 a=AjGcO6oz07-iQ99wixmX:22 X-ME-CMScore: 0 X-ME-CMCategory: none Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751692AbeDRRmu (ORCPT ); Wed, 18 Apr 2018 13:42:50 -0400 Received: from p3plsmtpa11-05.prod.phx3.secureserver.net ([68.178.252.106]:43368 "EHLO p3plsmtpa11-05.prod.phx3.secureserver.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752337AbeDRRmr (ORCPT ); Wed, 18 Apr 2018 13:42:47 -0400 Subject: Re: [Patch v3 2/6] cifs: Allocate validate negotiation request through kmalloc 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" Cc: "stable@vger.kernel.org" References: <20180418003358.25098-1-longli@linuxonhyperv.com> From: Tom Talpey Message-ID: Date: Wed, 18 Apr 2018 13:42:44 -0400 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-CMAE-Envelope: MS4wfPjs3boyOoqXtBLrK6fF3hWYorRfqSwJle7iwUzxVJZqdvBa4P2j1848Pig2XprAkVBV04Zerqg43KFyIeBsPra+E24saMrwSUOgGOfk9MNCa/hxeqWq vRiGLGhgbnD6w3KhcRqGw225g6Jnklyd/GRplqyCpNFnFK7aCeESqRcpaS3j2CxWULl4O5Kwl9FM1mkzlNVIQBDcnLV/V09UapwpEM3WtPnTA+bqCK15NBvj xeIsGmQzSUvaPSx4tbjJv8qeRO0fTRtkVpTXewHwF4oUKsRu7ikt8L55zkwSdcUgQ32MF7uvwlog9F9HfmpMVF1Wm+D8P9LkQYs+Nd5xYh1woN8mPMIwun3J 1zRFJp9+42g8fpErzsNNn/4wTzdmTOPB5lrLjwniL19XKEJ7ja0= Sender: stable-owner@vger.kernel.org X-Mailing-List: stable@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-Mailing-List: linux-kernel@vger.kernel.org List-ID: 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 >>> >>> 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 ) 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 >>> 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= >