diff mbox

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

Message ID D360AAB1.3CC7A%nithin@vmware.com
State Not Applicable
Headers show

Commit Message

Nithin Raju May 17, 2016, 6:09 p.m. UTC 
Just a couple of more comments.

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

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

Patch

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,