From patchwork Tue May 17 18:09:15 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nithin Raju X-Patchwork-Id: 623231 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (archives.nicira.com [96.126.127.54]) by ozlabs.org (Postfix) with ESMTP id 3r8QMQ3KCKz9t4F for ; Wed, 18 May 2016 04:09:22 +1000 (AEST) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id A0BB4108B2; Tue, 17 May 2016 11:09:21 -0700 (PDT) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx3v3.cudamail.com (mx3.cudamail.com [64.34.241.5]) by archives.nicira.com (Postfix) with ESMTPS id C0F47108AE for ; Tue, 17 May 2016 11:09:19 -0700 (PDT) Received: from bar6.cudamail.com (localhost [127.0.0.1]) by mx3v3.cudamail.com (Postfix) with ESMTPS id 5A94016281E for ; Tue, 17 May 2016 12:09:19 -0600 (MDT) X-ASG-Debug-ID: 1463508558-0b32372f4009770001-byXFYA Received: from mx1-pf1.cudamail.com ([192.168.24.1]) by bar6.cudamail.com with ESMTP id ubGzwQlneU93Bp79 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO) for ; Tue, 17 May 2016 12:09:18 -0600 (MDT) X-Barracuda-Envelope-From: nithin@vmware.com X-Barracuda-RBL-Trusted-Forwarder: 192.168.24.1 Received: from unknown (HELO smtp-outbound-1.vmware.com) (208.91.2.12) by mx1-pf1.cudamail.com with ESMTPS (DHE-RSA-AES256-SHA encrypted); 17 May 2016 18:09:17 -0000 Received-SPF: error (mx1-pf1.cudamail.com: error in processing during lookup of vmware.com: DNS problem) X-Barracuda-Apparent-Source-IP: 208.91.2.12 X-Barracuda-RBL-IP: 208.91.2.12 Received: from sc9-mailhost1.vmware.com (sc9-mailhost1.vmware.com [10.113.161.71]) by smtp-outbound-1.vmware.com (Postfix) with ESMTP id 448E198511; Tue, 17 May 2016 11:09:16 -0700 (PDT) Received: from EX13-CAS-008.vmware.com (ex13-cas-008.vmware.com [10.113.191.58]) by sc9-mailhost1.vmware.com (Postfix) with ESMTP id B5D071868F; Tue, 17 May 2016 11:09:16 -0700 (PDT) Received: from EX13-MBX-034.vmware.com (10.113.191.74) by EX13-MBX-012.vmware.com (10.113.191.32) with Microsoft SMTP Server (TLS) id 15.0.1156.6; Tue, 17 May 2016 11:09:16 -0700 Received: from EX13-MBX-034.vmware.com ([fe80::284e:d09d:74f7:17de]) by EX13-MBX-034.vmware.com ([fe80::284e:d09d:74f7:17de%15]) with mapi id 15.00.1156.000; Tue, 17 May 2016 11:09:16 -0700 X-CudaMail-Envelope-Sender: nithin@vmware.com From: Nithin Raju To: Paul Boca , "dev@openvswitch.org" X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-E1-516063531 X-CudaMail-DTE: 051716 X-CudaMail-Originating-IP: 208.91.2.12 Thread-Topic: [ovs-dev] [PATCH V2] datapath-windows: Validate netlink packets integrity X-ASG-Orig-Subj: [##CM-E1-516063531##]Re: [ovs-dev] [PATCH V2] datapath-windows: Validate netlink packets integrity Thread-Index: AQHRsGc5nK3RKEL+uE2DOmHoYhPD5A== Date: Tue, 17 May 2016 18:09:15 +0000 Message-ID: References: <1461744348-11756-1-git-send-email-pboca@cloudbasesolutions.com> In-Reply-To: <1461744348-11756-1-git-send-email-pboca@cloudbasesolutions.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: user-agent: Microsoft-MacOutlook/14.5.8.151023 x-ms-exchange-messagesentrepresentingtype: 1 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.113.160.246] MIME-Version: 1.0 X-Barracuda-Connect: UNKNOWN[192.168.24.1] X-Barracuda-Start-Time: 1463508558 X-Barracuda-Encrypted: ECDHE-RSA-AES256-GCM-SHA384 X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-ASG-Whitelist: Header =?UTF-8?B?eFwtY3VkYW1haWxcLXdoaXRlbGlzdFwtdG8=?= X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 X-ASG-Whitelist: EmailCat (corporate) X-Content-Filtered-By: Mailman/MimeDel 2.1.16 Subject: Re: [ovs-dev] [PATCH V2] datapath-windows: Validate netlink packets integrity X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@openvswitch.org Sender: "dev" Just a couple of more comments. -----Original Message----- From: dev > on behalf of Paul Boca > Date: Wednesday, April 27, 2016 at 1:05 AM To: "dev@openvswitch.org" > Subject: [ovs-dev] [PATCH V2] datapath-windows: Validate netlink packets integrity Solved access violation when trying to acces netling message - obtained with forged IOCTLs Signed-off-by: Paul-Daniel Boca > Acked-by: Alin Gabriel Serdean > --- V2: Fixed alignement problems --- datapath-windows/ovsext/Datapath.c | 45 ++++++++++++++++++--- datapath-windows/ovsext/Flow.c | 42 +++++++++++--------- datapath-windows/ovsext/Netlink/Netlink.c | 66 ++++++++++++++++++++++++++----- datapath-windows/ovsext/Netlink/Netlink.h | 13 ++++-- datapath-windows/ovsext/User.c | 5 ++- datapath-windows/ovsext/Vport.c | 34 ++++++++-------- lib/netlink-socket.c | 2 + 7 files changed, 152 insertions(+), 55 deletions(-) } ovsMsg = inputBuffer; + ovsMsgLength = inputBufferLen; devOp = OVS_TRANSACTION_DEV_OP; break; @@ -808,6 +811,7 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject, OVS_CTRL_CMD_EVENT_NOTIFY : OVS_CTRL_CMD_READ_NOTIFY; ovsMsg->genlMsg.version = nlControlFamilyOps.version; + ovsMsgLength = outputBufferLen; devOp = OVS_READ_DEV_OP; break; @@ -853,6 +857,7 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject, /* Create an NL message for consumption. */ ovsMsg = &ovsMsgReadOp; + ovsMsgLength = sizeof (ovsMsgReadOp); devOp = OVS_READ_DEV_OP; break; @@ -864,7 +869,21 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject, goto done; } + /* + * Output buffer not mandatory but map it in case we have something + * to return to requester. + */ + if (outputBufferLen != 0) { + status = MapIrpOutputBuffer(irp, outputBufferLen, + sizeof *ovsMsg, &outputBuffer); + if (status != STATUS_SUCCESS) { + goto done; + } + ASSERT(outputBuffer); + } + ovsMsg = inputBuffer; + ovsMsgLength = inputBufferLen; devOp = OVS_WRITE_DEV_OP; The contract between userspace and kernel is that for "Write" operations, the output buffer will not be used. Under what context will the kernel code use the output buffer here? If it does, then we are violating the contract. No? Also, this is the code in userspace that calls into WRITE_OP (in lib/netlink-socket.c): if (!DeviceIoControl(sock->handle, OVS_IOCTL_WRITE, msg->data, msg->size, NULL, 0, &bytes, NULL)) { retval = -1; /* XXX: Map to a more appropriate error based on GetLastError(). */ errno = EINVAL; VLOG_DBG_RL(&rl, "fatal driver failure in write: %s", ovs_lasterror_to_string()); } As you can see, we are passing NULL for output buffer. How will your patch work with this code? -- Nithin diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-windows/ovsext/Datapath.c index 06f99b3..1f89964 100644 --- a/datapath-windows/ovsext/Datapath.c +++ b/datapath-windows/ovsext/Datapath.c @@ -307,6 +307,7 @@ static NTSTATUS MapIrpOutputBuffer(PIRP irp, static NTSTATUS ValidateNetlinkCmd(UINT32 devOp, POVS_OPEN_INSTANCE instance, POVS_MESSAGE ovsMsg, + UINT32 ovsMgsLength, NETLINK_FAMILY *nlFamilyOps); static NTSTATUS InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, NETLINK_FAMILY *nlFamilyOps, @@ -694,6 +695,7 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject, UINT32 devOp; OVS_MESSAGE ovsMsgReadOp; POVS_MESSAGE ovsMsg; + UINT32 ovsMsgLength = 0; NETLINK_FAMILY *nlFamilyOps; OVS_USER_PARAMS_CONTEXT usrParamsCtx; @@ -774,6 +776,7 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject,