[ovs-dev] [PATCH V2] datapath-windows: Validate netlink packets integrity

Nithin Raju nithin at vmware.com
Tue May 17 18:09:15 UTC 2016


Just a couple of more comments.

-----Original Message-----
From: dev <dev-bounces at openvswitch.org<mailto:dev-bounces at openvswitch.org>> on behalf of Paul Boca <pboca at cloudbasesolutions.com<mailto:pboca at cloudbasesolutions.com>>
Date: Wednesday, April 27, 2016 at 1:05 AM
To: "dev at openvswitch.org<mailto:dev at openvswitch.org>" <dev at openvswitch.org<mailto:dev at 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 <pboca at cloudbasesolutions.com<mailto:pboca at cloudbasesolutions.com>>
Acked-by: Alin Gabriel Serdean <aserdean at cloudbasesolutions.com<mailto:aserdean at cloudbasesolutions.com>>
---
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(-)

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,
         }
         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



More information about the dev mailing list