[ovs-dev] [PATCH] datapath-windows: BSOD for a transactional NL cmd w/o dump state

[Sorin Vinturis]

>From user mode an OVS_IOCTL_TRANSACT NL packet with no previous
dump state is sent to the OVS datapath. The packet is handled by
the dispatch routine and the OvsFlowNlGetCmdHandler() NL command
handler is invoked. The OVS datapath crashes when trying to get
the attributes from the NL header of the previous OVS_MESSAGE,
which is NULL.

To fix this, a check is performed in the _FlowNlGetCmdHandler()
function and, if previous dump state is NULL, an error is returned
and an appropriate message is logged.

Also modified OvsFlowNlGetCmdHandler() to handle transactional
errors.

Signed-off-by: Sorin Vinturis <svinturis at cloudbasesolutions.com>
Reported-by: Sorin Vinturis <svinturis at cloudbasesolutions.com>
Reported-at: https://github.com/openvswitch/ovs-issues/issues/54

---
 datapath-windows/ovsext/Datapath.c             |  6 ++----
 datapath-windows/ovsext/Flow.c                 | 23 ++++++++++++++++-------
 datapath-windows/ovsext/Netlink/NetlinkError.h |  3 +++
 3 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-windows/ovsext/Datapath.c
index 1b504a2..1ba96f2 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -774,7 +774,7 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject,
          * state in the instance to indicate the command that started the dump
          * operation. The state can setup 'ovsMsgReadOp' appropriately. Note
          * that 'ovsMsgReadOp' is needed only in this function to call into the
-         * appropraite handler. The handler itself can access the state in the
+         * appropriate handler. The handler itself can access the state in the
          * instance.
          *
          * In the absence of a dump start, return 0 bytes.
@@ -1344,9 +1344,7 @@ OvsSetupDumpStart(POVS_USER_PARAMS_CONTEXT usrParamsCtx)
      * This operation should be setting up the dump state. If there's any
      * previous state, clear it up so as to set it up afresh.
      */
-    if (instance->dumpState.ovsMsg != NULL) {
-        FreeUserDumpState(instance);
-    }
+    FreeUserDumpState(instance);
 
     return InitUserDumpState(instance, msgIn);
 }
diff --git a/datapath-windows/ovsext/Flow.c b/datapath-windows/ovsext/Flow.c
index d2d0ae5..263fb22 100644
--- a/datapath-windows/ovsext/Flow.c
+++ b/datapath-windows/ovsext/Flow.c
@@ -379,10 +379,8 @@ OvsFlowNlGetCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
 
     if (usrParamsCtx->devOp == OVS_TRANSACTION_DEV_OP) {
         status = _FlowNlGetCmdHandler(usrParamsCtx, replyLen);
-
-        /* No trasanctional errors as of now.
-         * If we have something then we need to convert rc to
-         * nlError. */
+        /* Convert rc to nlError. */
+        nlError = NlMapStatusToNlErr(status);
         if ((nlError != NL_ERROR_SUCCESS) &&
             (usrParamsCtx->outputBuffer)) {
             POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
@@ -413,9 +411,9 @@ _FlowNlGetCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
     NTSTATUS rc = STATUS_SUCCESS;
     POVS_OPEN_INSTANCE instance = (POVS_OPEN_INSTANCE)
                                   (usrParamsCtx->ovsInstance);
-    POVS_MESSAGE msgIn = instance->dumpState.ovsMsg;
-    PNL_MSG_HDR nlMsgHdr = &(msgIn->nlMsg);
-    POVS_HDR ovsHdr = &(msgIn->ovsHdr);
+    POVS_MESSAGE msgIn = NULL;
+    PNL_MSG_HDR nlMsgHdr = NULL;
+    POVS_HDR ovsHdr = NULL;
     PNL_MSG_HDR nlMsgOutHdr = NULL;
     UINT32 attrOffset = NLMSG_HDRLEN + GENL_HDRLEN + OVS_HDRLEN;
     PNL_ATTR nlAttrs[__OVS_FLOW_ATTR_MAX];
@@ -433,6 +431,17 @@ _FlowNlGetCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
     UINT32 keyAttrOffset = 0;
     UINT32 tunnelKeyAttrOffset = 0;
 
+    if (instance->dumpState.ovsMsg == NULL) {
+        rc = STATUS_INVALID_DEVICE_STATE;
+        OVS_LOG_ERROR("No previous dump state for instance: %p",
+                      instance);
+        goto done;
+    }
+
+    msgIn = instance->dumpState.ovsMsg;
+    nlMsgHdr = &(msgIn->nlMsg);
+    ovsHdr = &(msgIn->ovsHdr);
+
     if (usrParamsCtx->inputLength > usrParamsCtx->outputLength) {
         /* Should not be the case.
          * We'll be copying the flow keys back from
diff --git a/datapath-windows/ovsext/Netlink/NetlinkError.h b/datapath-windows/ovsext/Netlink/NetlinkError.h
index 827fa8c..56adf73 100644
--- a/datapath-windows/ovsext/Netlink/NetlinkError.h
+++ b/datapath-windows/ovsext/Netlink/NetlinkError.h
@@ -209,6 +209,9 @@ NlMapStatusToNlErr(NTSTATUS status)
     case STATUS_NOT_SUPPORTED:
       ret = NL_ERROR_NOTSUPP;
       break;
+    case STATUS_INVALID_DEVICE_STATE:
+      ret = NL_ERROR_BADMSG;
+      break;
     default:
       break;
     }
-- 
1.9.0.msysgit.0