| Message ID | 1461744348-11756-1-git-send-email-pboca@cloudbasesolutions.com |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, Apr 27, 2016 at 08:05:47AM +0000, Paul Boca wrote: > Solved access violation when trying to acces netling message - obtained with forged IOCTLs > > Signed-off-by: Paul-Daniel Boca <pboca@cloudbasesolutions.com> > Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com> > --- > V2: Fixed alignement problems Applied, thanks!
Hi Paul,
Can you point out the particular code that fixed the access violation?
I looked at the code, and code such as the following is redundant:
+ // We need to ensure we have enough data to process
+ if (NlMsgSize(&ovsMsg->nlMsg) > ovsMsgLength) {
+ status = STATUS_INVALID_PARAMETER;
+ goto done;
+ }
We already validate the minimum required length when we call into
MapIrpOutputBuffer().
Also, it would be helpful for review if we limited each patch to address
one problem. I see fixes in Flow.c and Datpath.c. Are they all related to
the same problem you are trying to address?
-- Nithin
-----Original Message-----
From: dev <dev-bounces@openvswitch.org> on behalf of Paul Boca
<pboca@cloudbasesolutions.com>
Date: Wednesday, April 27, 2016 at 1:05 AM
To: "dev@openvswitch.org" <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>
>Acked-by: Alin Gabriel Serdean <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(-)
>
>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;
> break;
>
>@@ -903,7 +922,8 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject,
> * "artificial" or was copied from a previously validated 'ovsMsg'.
> */
> if (devOp != OVS_READ_DEV_OP) {
>- status = ValidateNetlinkCmd(devOp, instance, ovsMsg,
>nlFamilyOps);
>+ status = ValidateNetlinkCmd(devOp, instance, ovsMsg,
>+ ovsMsgLength, nlFamilyOps);
> if (status != STATUS_SUCCESS) {
> goto done;
> }
>@@ -938,11 +958,18 @@ static NTSTATUS
> ValidateNetlinkCmd(UINT32 devOp,
> POVS_OPEN_INSTANCE instance,
> POVS_MESSAGE ovsMsg,
>+ UINT32 ovsMsgLength,
> NETLINK_FAMILY *nlFamilyOps)
> {
> NTSTATUS status = STATUS_INVALID_PARAMETER;
> UINT16 i;
>
>+ // We need to ensure we have enough data to process
>+ if (NlMsgSize(&ovsMsg->nlMsg) > ovsMsgLength) {
>+ status = STATUS_INVALID_PARAMETER;
>+ goto done;
>+ }
>+
> for (i = 0; i < nlFamilyOps->opsCount; i++) {
> if (nlFamilyOps->cmds[i].cmd == ovsMsg->genlMsg.cmd) {
> /* Validate if the command is valid for the device
>operation. */
>@@ -977,6 +1004,14 @@ ValidateNetlinkCmd(UINT32 devOp,
> }
> }
>
>+ // validate all NlAttrs
>+ if (!NlValidateAllAttrs(&ovsMsg->nlMsg, sizeof(*ovsMsg),
>+ NlMsgAttrsLen((PNL_MSG_HDR)ovsMsg),
>+ NULL, 0)) {
>+ status = STATUS_INVALID_PARAMETER;
>+ goto done;
>+ }
>+
> done:
> return status;
> }
>@@ -1028,6 +1063,7 @@ InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
> POVS_MESSAGE msgIn = NULL;
> POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
> usrParamsCtx->outputBuffer;
>+ UINT32 msgErrorLen = usrParamsCtx->outputLength;
>
> if (usrParamsCtx->ovsMsg->genlMsg.cmd ==
>OVS_CTRL_CMD_EVENT_NOTIFY ||
> usrParamsCtx->ovsMsg->genlMsg.cmd ==
>OVS_CTRL_CMD_READ_NOTIFY) {
>@@ -1043,8 +1079,7 @@ InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
>
> ASSERT(msgIn);
> ASSERT(msgError);
>- NlBuildErrorMsg(msgIn, msgError, nlError);
>- *replyLen = msgError->nlMsg.nlmsgLen;
>+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError,
>replyLen);
> }
>
> if (*replyLen != 0) {
>@@ -1416,9 +1451,9 @@ cleanup:
> if (nlError != NL_ERROR_SUCCESS) {
> POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
> usrParamsCtx->outputBuffer;
>+ UINT32 msgErrorLen = usrParamsCtx->outputLength;
>
>- NlBuildErrorMsg(msgIn, msgError, nlError);
>- *replyLen = msgError->nlMsg.nlmsgLen;
>+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen);
> }
>
> return STATUS_SUCCESS;
>diff --git a/datapath-windows/ovsext/Flow.c
>b/datapath-windows/ovsext/Flow.c
>index a49a60c..792b614 100644
>--- a/datapath-windows/ovsext/Flow.c
>+++ b/datapath-windows/ovsext/Flow.c
>@@ -285,20 +285,10 @@ OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
> goto done;
> }
>
>- /* Get all the top level Flow attributes */
>- if ((NlAttrParse(nlMsgHdr, attrOffset, NlMsgAttrsLen(nlMsgHdr),
>- nlFlowPolicy, ARRAY_SIZE(nlFlowPolicy),
>- flowAttrs, ARRAY_SIZE(flowAttrs)))
>- != TRUE) {
>- OVS_LOG_ERROR("Attr Parsing failed for msg: %p",
>- nlMsgHdr);
>- rc = STATUS_INVALID_PARAMETER;
>- goto done;
>- }
>-
>- /* FLOW_DEL command w/o any key input is a flush case. */
>+ /* FLOW_DEL command w/o any key input is a flush case.
>+ If we don't have any attr, we treat this as a flush command*/
> if ((genlMsgHdr->cmd == OVS_FLOW_CMD_DEL) &&
>- (!(flowAttrs[OVS_FLOW_ATTR_KEY]))) {
>+ (!NlMsgAttrsLen(nlMsgHdr))) {
>
> rc = OvsFlushFlowIoctl(ovsHdr->dp_ifindex);
>
>@@ -323,6 +313,17 @@ OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
> goto done;
> }
>
>+ /* Get all the top level Flow attributes */
>+ if ((NlAttrParse(nlMsgHdr, attrOffset, NlMsgAttrsLen(nlMsgHdr),
>+ nlFlowPolicy, ARRAY_SIZE(nlFlowPolicy),
>+ flowAttrs, ARRAY_SIZE(flowAttrs)))
>+ != TRUE) {
>+ OVS_LOG_ERROR("Attr Parsing failed for msg: %p",
>+ nlMsgHdr);
>+ rc = STATUS_INVALID_PARAMETER;
>+ goto done;
>+ }
>+
> if (flowAttrs[OVS_FLOW_ATTR_PROBE]) {
> rc = OvsProbeSupportedFeature(msgIn,
>flowAttrs[OVS_FLOW_ATTR_KEY]);
> if (rc != STATUS_SUCCESS) {
>@@ -399,8 +400,9 @@ done:
> if (nlError != NL_ERROR_SUCCESS) {
> POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
> usrParamsCtx->outputBuffer;
>- NlBuildErrorMsg(msgIn, msgError, nlError);
>- *replyLen = msgError->nlMsg.nlmsgLen;
>+ UINT32 msgErrorLen = usrParamsCtx->outputLength;
>+
>+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen);
> rc = STATUS_SUCCESS;
> }
>
>@@ -568,8 +570,9 @@ done:
> if (nlError != NL_ERROR_SUCCESS) {
> POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
> usrParamsCtx->outputBuffer;
>- NlBuildErrorMsg(msgIn, msgError, nlError);
>- *replyLen = msgError->nlMsg.nlmsgLen;
>+ UINT32 msgErrorLen = usrParamsCtx->outputLength;
>+
>+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen);
> rc = STATUS_SUCCESS;
> }
>
>@@ -706,8 +709,9 @@ done:
> if (nlError != NL_ERROR_SUCCESS) {
> POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
> usrParamsCtx->outputBuffer;
>- NlBuildErrorMsg(msgIn, msgError, nlError);
>- *replyLen = msgError->nlMsg.nlmsgLen;
>+ UINT32 msgErrorLen = usrParamsCtx->outputLength;
>+
>+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen);
> rc = STATUS_SUCCESS;
> }
>
>diff --git a/datapath-windows/ovsext/Netlink/Netlink.c
>b/datapath-windows/ovsext/Netlink/Netlink.c
>index 27dcd4f..dc1e78c 100644
>--- a/datapath-windows/ovsext/Netlink/Netlink.c
>+++ b/datapath-windows/ovsext/Netlink/Netlink.c
>@@ -108,12 +108,18 @@ NlFillNlHdr(PNL_BUFFER nlBuf, UINT16 nlmsgType,
> *
>--------------------------------------------------------------------------
>-
> */
> VOID
>-NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR msgError, UINT
>errorCode)
>+NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR msgError,
>+ UINT32 msgErrorLen,
>+ UINT errorCode, UINT32 *msgLen)
> {
> NL_BUFFER nlBuffer;
>
> ASSERT(errorCode != NL_ERROR_PENDING);
>
>+ if ((msgError == NULL) || (msgErrorLen < sizeof *msgError)) {
>+ return;
>+ }
>+
> NlBufInit(&nlBuffer, (PCHAR)msgError, sizeof *msgError);
> NlFillNlHdr(&nlBuffer, NLMSG_ERROR, 0,
> msgIn->nlMsg.nlmsgSeq, msgIn->nlMsg.nlmsgPid);
>@@ -121,6 +127,10 @@ NlBuildErrorMsg(POVS_MESSAGE msgIn,
>POVS_MESSAGE_ERROR msgError, UINT errorCode)
> msgError->errorMsg.error = errorCode;
> msgError->errorMsg.nlMsg = msgIn->nlMsg;
> msgError->nlMsg.nlmsgLen = sizeof(OVS_MESSAGE_ERROR);
>+
>+ if (NULL != msgLen) {
>+ *msgLen = msgError->nlMsg.nlmsgLen;
>+ }
> }
>
> /*
>@@ -1006,7 +1016,7 @@ NlAttrFind__(const PNL_ATTR attrs, UINT32 size,
>UINT16 type)
> {
> PNL_ATTR iter = NULL;
> PNL_ATTR ret = NULL;
>- UINT32 left;
>+ INT left;
>
> NL_ATTR_FOR_EACH (iter, left, attrs, size) {
> if (NlAttrType(iter) == type) {
>@@ -1036,6 +1046,49 @@ NlAttrFindNested(const PNL_ATTR nla, UINT16 type)
>
> /*
>
>*-------------------------------------------------------------------------
>---
>+ * Traverses all attributes in received buffer in order to insure all
>are valid
>+
>*-------------------------------------------------------------------------
>---
>+ */
>+BOOLEAN NlValidateAllAttrs(const PNL_MSG_HDR nlMsg, UINT32 attrOffset,
>+ UINT32 totalAttrLen,
>+ const NL_POLICY policy[], const UINT32
>numPolicy)
>+{
>+ PNL_ATTR nla;
>+ INT left;
>+ BOOLEAN ret = TRUE;
>+
>+ if ((NlMsgSize(nlMsg) < attrOffset)) {
>+ OVS_LOG_WARN("No attributes in nlMsg: %p at offset: %d",
>+ nlMsg, attrOffset);
>+ ret = FALSE;
>+ goto done;
>+ }
>+
>+ NL_ATTR_FOR_EACH_UNSAFE(nla, left, NlMsgAt(nlMsg, attrOffset),
>+ totalAttrLen)
>+ {
>+ if (!NlAttrIsValid(nla, left)) {
>+ ret = FALSE;
>+ goto done;
>+ }
>+
>+ UINT16 type = NlAttrType(nla);
>+ if (type < numPolicy && policy[type].type != NL_A_NO_ATTR) {
>+ /* Typecasting to keep the compiler happy */
>+ const PNL_POLICY e = (const PNL_POLICY)(&policy[type]);
>+ if (!NlAttrValidate(nla, e)) {
>+ ret = FALSE;
>+ goto done;
>+ }
>+ }
>+ }
>+
>+done:
>+ return ret;
>+}
>+
>+/*
>+
>*-------------------------------------------------------------------------
>---
> * Parses the netlink message at a given offset (attrOffset)
> * as a series of attributes. A pointer to the attribute with type
> * 'type' is stored in attrs at index 'type'. policy is used to define
>the
>@@ -1052,20 +1105,13 @@ NlAttrParse(const PNL_MSG_HDR nlMsg, UINT32
>attrOffset,
> PNL_ATTR attrs[], UINT32 numAttrs)
> {
> PNL_ATTR nla;
>- UINT32 left;
>+ INT left;
> UINT32 iter;
> BOOLEAN ret = FALSE;
> UINT32 numPolicyAttr = MIN(numPolicy, numAttrs);
>
> RtlZeroMemory(attrs, numAttrs * sizeof *attrs);
>
>-
>- /* There is nothing to parse */
>- if (!(NlMsgAttrsLen(nlMsg))) {
>- ret = TRUE;
>- goto done;
>- }
>-
> if ((NlMsgSize(nlMsg) < attrOffset)) {
> OVS_LOG_WARN("No attributes in nlMsg: %p at offset: %d",
> nlMsg, attrOffset);
>diff --git a/datapath-windows/ovsext/Netlink/Netlink.h
>b/datapath-windows/ovsext/Netlink/Netlink.h
>index 8f6a5be..63164c7 100644
>--- a/datapath-windows/ovsext/Netlink/Netlink.h
>+++ b/datapath-windows/ovsext/Netlink/Netlink.h
>@@ -72,6 +72,7 @@ typedef struct _NL_POLICY
> /* This macro is careful to check for attributes with bad lengths. */
> #define NL_ATTR_FOR_EACH(ITER, LEFT, ATTRS, ATTRS_LEN) \
> for ((ITER) = (ATTRS), (LEFT) = (ATTRS_LEN); \
>+ ((INT)LEFT) >= (INT)NLA_ALIGN(sizeof(NL_ATTR)) && \
> NlAttrIsValid(ITER, LEFT); \
> (LEFT) -= NlAttrLenPad(ITER, LEFT), (ITER) = NlAttrNext(ITER))
>
>@@ -80,7 +81,7 @@ typedef struct _NL_POLICY
> * already been validated (e.g. with NL_ATTR_FOR_EACH). */
> #define NL_ATTR_FOR_EACH_UNSAFE(ITER, LEFT, ATTRS, ATTRS_LEN) \
> for ((ITER) = (ATTRS), (LEFT) = (ATTRS_LEN); \
>- (LEFT) > 0; \
>+ ((INT)LEFT) >= (INT)NLA_ALIGN(sizeof(NL_ATTR)); \
> (LEFT) -= NLA_ALIGN((ITER)->nlaLen), (ITER) = NlAttrNext(ITER))
>
> #define NL_ATTR_GET_AS(NLA, TYPE) \
>@@ -94,8 +95,9 @@ BOOLEAN NlFillNlHdr(PNL_BUFFER nlBuf,
> UINT16 nlmsgType, UINT16 nlmsgFlags,
> UINT32 nlmsgSeq, UINT32 nlmsgPid);
>
>-VOID NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR msgOut,
>- UINT errorCode);
>+VOID NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR msgError,
>+ UINT32 msgErrorLen,
>+ UINT errorCode, UINT32 *msgLen);
>
> /* Netlink message accessing the payload */
> PVOID NlMsgAt(const PNL_MSG_HDR nlh, UINT32 offset);
>@@ -187,6 +189,11 @@ static __inline NlAttrIsLast(const PNL_ATTR nla, int
>rem)
> /* Netlink attribute validation */
> BOOLEAN NlAttrValidate(const PNL_ATTR, const PNL_POLICY);
>
>+/* Netlink attribute stream validation */
>+BOOLEAN NlValidateAllAttrs(const PNL_MSG_HDR nlMsg, UINT32 attrOffset,
>+ UINT32 totalAttrLen,
>+ const NL_POLICY policy[], const UINT32 numPolicy);
>+
> /* Put APis */
> BOOLEAN NlMsgPutNlHdr(PNL_BUFFER buf, PNL_MSG_HDR nlMsg);
> BOOLEAN NlMsgPutGenlHdr(PNL_BUFFER buf, PGENL_MSG_HDR genlMsg);
>diff --git a/datapath-windows/ovsext/User.c
>b/datapath-windows/ovsext/User.c
>index 33cbd89..bc010c7 100644
>--- a/datapath-windows/ovsext/User.c
>+++ b/datapath-windows/ovsext/User.c
>@@ -345,8 +345,9 @@ OvsNlExecuteCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
>
> POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
> usrParamsCtx->outputBuffer;
>- NlBuildErrorMsg(msgIn, msgError, nlError);
>- *replyLen = msgError->nlMsg.nlmsgLen;
>+ UINT32 msgErrorLen = usrParamsCtx->outputLength;
>+
>+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError,
>replyLen);
> status = STATUS_SUCCESS;
> goto done;
> }
>diff --git a/datapath-windows/ovsext/Vport.c
>b/datapath-windows/ovsext/Vport.c
>index d04b12b..4299169 100644
>--- a/datapath-windows/ovsext/Vport.c
>+++ b/datapath-windows/ovsext/Vport.c
>@@ -1729,9 +1729,9 @@ cleanup:
> if (nlError != NL_ERROR_SUCCESS) {
> POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
> usrParamsCtx->outputBuffer;
>+ UINT32 msgErrorLen = usrParamsCtx->outputLength;
>
>- NlBuildErrorMsg(msgIn, msgError, nlError);
>- *replyLen = msgError->nlMsg.nlmsgLen;
>+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen);
> }
>
> return STATUS_SUCCESS;
>@@ -2088,9 +2088,9 @@ Cleanup:
> if (nlError != NL_ERROR_SUCCESS) {
> POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
> usrParamsCtx->outputBuffer;
>+ UINT32 msgErrorLen = usrParamsCtx->outputLength;
>
>- NlBuildErrorMsg(msgIn, msgError, nlError);
>- *replyLen = msgError->nlMsg.nlmsgLen;
>+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen);
> }
>
> return STATUS_SUCCESS;
>@@ -2324,6 +2324,7 @@ Cleanup:
> if ((nlError != NL_ERROR_SUCCESS) && (nlError != NL_ERROR_PENDING)) {
> POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
> usrParamsCtx->outputBuffer;
>+ UINT32 msgErrorLen = usrParamsCtx->outputLength;
>
> if (vport && vportAllocated == TRUE) {
> if (vportInitialized == TRUE) {
>@@ -2343,8 +2344,7 @@ Cleanup:
> OvsFreeMemoryWithTag(vport, OVS_VPORT_POOL_TAG);
> }
>
>- NlBuildErrorMsg(msgIn, msgError, nlError);
>- *replyLen = msgError->nlMsg.nlmsgLen;
>+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen);
> }
>
> return (status == STATUS_PENDING) ? STATUS_PENDING : STATUS_SUCCESS;
>@@ -2452,9 +2452,9 @@ Cleanup:
> if (nlError != NL_ERROR_SUCCESS) {
> POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
> usrParamsCtx->outputBuffer;
>+ UINT32 msgErrorLen = usrParamsCtx->outputLength;
>
>- NlBuildErrorMsg(msgIn, msgError, nlError);
>- *replyLen = msgError->nlMsg.nlmsgLen;
>+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen);
> }
>
> return STATUS_SUCCESS;
>@@ -2544,9 +2544,9 @@ Cleanup:
> if ((nlError != NL_ERROR_SUCCESS) && (nlError != NL_ERROR_PENDING)) {
> POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
> usrParamsCtx->outputBuffer;
>+ UINT32 msgErrorLen = usrParamsCtx->outputLength;
>
>- NlBuildErrorMsg(msgIn, msgError, nlError);
>- *replyLen = msgError->nlMsg.nlmsgLen;
>+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen);
> }
>
> return (status == STATUS_PENDING) ? STATUS_PENDING : STATUS_SUCCESS;
>@@ -2579,10 +2579,11 @@ OvsTunnelVportPendingRemove(PVOID context,
>
> *replyLen = msgOut->nlMsg.nlmsgLen;
> } else {
>- POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)msgOut;
>+ POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
>+ tunnelContext->outputBuffer;
>+ UINT32 msgErrorLen = tunnelContext->outputLength;
>
>- NlBuildErrorMsg(msgIn, msgError, nlError);
>- *replyLen = msgError->nlMsg.nlmsgLen;
>+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError,
>replyLen);
> }
> }
>
>@@ -2721,12 +2722,13 @@ OvsTunnelVportPendingInit(PVOID context,
> } while (error);
>
> if (error) {
>- POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) msgOut;
>+ POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
>+ tunnelContext->outputBuffer;
>+ UINT32 msgErrorLen = tunnelContext->outputLength;
>
> OvsCleanupVxlanTunnel(NULL, vport, NULL, NULL);
> OvsFreeMemory(vport);
>
>- NlBuildErrorMsg(msgIn, msgError, nlError);
>- *replyLen = msgError->nlMsg.nlmsgLen;
>+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen);
> }
> }
>diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c
>index cad9490..32b0cc3 100644
>--- a/lib/netlink-socket.c
>+++ b/lib/netlink-socket.c
>@@ -127,6 +127,7 @@ nl_sock_create(int protocol, struct nl_sock **sockp)
> sock = xmalloc(sizeof *sock);
>
> #ifdef _WIN32
>+ sock->overlapped.hEvent = NULL;
> sock->handle = CreateFile(OVS_DEVICE_NAME_USER,
> GENERIC_READ | GENERIC_WRITE,
> FILE_SHARE_READ | FILE_SHARE_WRITE,
>@@ -1191,6 +1192,7 @@ pend_io_request(struct nl_sock *sock)
>
> ovs_header = ofpbuf_put_uninit(&request, sizeof *ovs_header);
> ovs_header->dp_ifindex = 0;
>+ nlmsg->nlmsg_len = request.size;
>
> if (!DeviceIoControl(sock->handle, OVS_IOCTL_WRITE,
> request.data, request.size,
>--
>2.7.2.windows.1
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailma
>n_listinfo_dev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=pN
>HQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=GabGAlmcY24uVSoAQCaiRYFIYSfiDS
>JZomM6llWwZmA&s=N-QN8hqnSsvjfhoygdktRzwFa6jdc2QCMZelWxlsT3g&e=
Hi Nithin! This patch doesn't solve an existing access violation; it adds the necessary checks to avoid crashes in case the driver receives a corrupted IOCTL. Please see inlined comments. Regards, Paul > -----Original Message----- > From: Nithin Raju [mailto:nithin@vmware.com] > Sent: Tuesday, May 17, 2016 8:42 PM > To: Paul Boca; dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH V2] datapath-windows: Validate netlink packets > integrity > > Hi Paul, > Can you point out the particular code that fixed the access violation? > > I looked at the code, and code such as the following is redundant: > + // We need to ensure we have enough data to process > + if (NlMsgSize(&ovsMsg->nlMsg) > ovsMsgLength) { [Paul Boca] NlMsgSize returns the length from received IOCTL buffer, if the buffer is corrupted in any way we will access random memory. You can use input/outputBufferLen if you expect that all data is valid and the sender is trustworthy. Some of checks may seem redundant but they are necessary given the fact that we are parsing a netlink message with one total length defined in inputBufferLength, one in ovsMsg->nlMsg->nlmsgLen and a total length composed by the sum of all nlAttrs length; the only one that we can trust is inputBufferLength. > + status = STATUS_INVALID_PARAMETER; > + goto done; > + } > > We already validate the minimum required length when we call into > MapIrpOutputBuffer(). > > Also, it would be helpful for review if we limited each patch to address > one problem. I see fixes in Flow.c and Datpath.c. Are they all related to > the same problem you are trying to address? [Paul Boca] They are related to the same issue, I tried to cover all the situations where an invalid IOCTL buffer is received, that's why I made changes both in Flow.c and in Datapath.c. I did this with a small tool which sends almost random data to the driver, fine-tuned to bypass almost any check regarding in/out buffer received from USER-MODE. > > -- Nithin > > > -----Original Message----- > From: dev <dev-bounces@openvswitch.org> on behalf of Paul Boca > <pboca@cloudbasesolutions.com> > Date: Wednesday, April 27, 2016 at 1:05 AM > To: "dev@openvswitch.org" <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> > >Acked-by: Alin Gabriel Serdean <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(-) > > > >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); [Paul Boca] Because we didn't have this buffer mapped, in case of an error, there was a NULL pointer dereference when trying to copy the error message to the USER-MODE buffer > >+ if (status != STATUS_SUCCESS) { > >+ goto done; > >+ } > >+ ASSERT(outputBuffer); > >+ } > >+ > > ovsMsg = inputBuffer; > >+ ovsMsgLength = inputBufferLen; > > devOp = OVS_WRITE_DEV_OP; > > break; > > > >@@ -903,7 +922,8 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject, > > * "artificial" or was copied from a previously validated 'ovsMsg'. > > */ > > if (devOp != OVS_READ_DEV_OP) { > >- status = ValidateNetlinkCmd(devOp, instance, ovsMsg, > >nlFamilyOps); > >+ status = ValidateNetlinkCmd(devOp, instance, ovsMsg, > >+ ovsMsgLength, nlFamilyOps); > > if (status != STATUS_SUCCESS) { > > goto done; > > } > >@@ -938,11 +958,18 @@ static NTSTATUS > > ValidateNetlinkCmd(UINT32 devOp, > > POVS_OPEN_INSTANCE instance, > > POVS_MESSAGE ovsMsg, > >+ UINT32 ovsMsgLength, > > NETLINK_FAMILY *nlFamilyOps) > > { > > NTSTATUS status = STATUS_INVALID_PARAMETER; > > UINT16 i; > > > >+ // We need to ensure we have enough data to process > >+ if (NlMsgSize(&ovsMsg->nlMsg) > ovsMsgLength) { > >+ status = STATUS_INVALID_PARAMETER; > >+ goto done; > >+ } > >+ > > for (i = 0; i < nlFamilyOps->opsCount; i++) { > > if (nlFamilyOps->cmds[i].cmd == ovsMsg->genlMsg.cmd) { > > /* Validate if the command is valid for the device > >operation. */ > >@@ -977,6 +1004,14 @@ ValidateNetlinkCmd(UINT32 devOp, > > } > > } > > > >+ // validate all NlAttrs > >+ if (!NlValidateAllAttrs(&ovsMsg->nlMsg, sizeof(*ovsMsg), > >+ NlMsgAttrsLen((PNL_MSG_HDR)ovsMsg), > >+ NULL, 0)) { > >+ status = STATUS_INVALID_PARAMETER; > >+ goto done; > >+ } > >+ > > done: > > return status; > > } > >@@ -1028,6 +1063,7 @@ > InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT > >usrParamsCtx, > > POVS_MESSAGE msgIn = NULL; > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > > > > if (usrParamsCtx->ovsMsg->genlMsg.cmd == > >OVS_CTRL_CMD_EVENT_NOTIFY || > > usrParamsCtx->ovsMsg->genlMsg.cmd == > >OVS_CTRL_CMD_READ_NOTIFY) { > >@@ -1043,8 +1079,7 @@ > InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT > >usrParamsCtx, > > > > ASSERT(msgIn); > > ASSERT(msgError); > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, > >replyLen); [Paul Boca] NlBuildErrorMsg receives msgErrorLen to avoid buffer overflow in case the outputBufferLength received from UM is less than the error message length. > > } > > > > if (*replyLen != 0) { > >@@ -1416,9 +1451,9 @@ cleanup: > > if (nlError != NL_ERROR_SUCCESS) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > > > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > } > > > > return STATUS_SUCCESS; > >diff --git a/datapath-windows/ovsext/Flow.c > >b/datapath-windows/ovsext/Flow.c > >index a49a60c..792b614 100644 > >--- a/datapath-windows/ovsext/Flow.c > >+++ b/datapath-windows/ovsext/Flow.c > >@@ -285,20 +285,10 @@ > OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT > >usrParamsCtx, > > goto done; > > } > > > >- /* Get all the top level Flow attributes */ > >- if ((NlAttrParse(nlMsgHdr, attrOffset, NlMsgAttrsLen(nlMsgHdr), > >- nlFlowPolicy, ARRAY_SIZE(nlFlowPolicy), > >- flowAttrs, ARRAY_SIZE(flowAttrs))) > >- != TRUE) { > >- OVS_LOG_ERROR("Attr Parsing failed for msg: %p", > >- nlMsgHdr); > >- rc = STATUS_INVALID_PARAMETER; > >- goto done; > >- } > >- > >- /* FLOW_DEL command w/o any key input is a flush case. */ [Paul Boca] Due to the validations over nlAttrs I had to move the flush command before parsing Nlattrs because this doesn't complies the nlFlowPolicy . > >+ /* FLOW_DEL command w/o any key input is a flush case. > >+ If we don't have any attr, we treat this as a flush command*/ > > if ((genlMsgHdr->cmd == OVS_FLOW_CMD_DEL) && > >- (!(flowAttrs[OVS_FLOW_ATTR_KEY]))) { > >+ (!NlMsgAttrsLen(nlMsgHdr))) { > > > > rc = OvsFlushFlowIoctl(ovsHdr->dp_ifindex); > > > >@@ -323,6 +313,17 @@ > OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT > >usrParamsCtx, > > goto done; > > } > > > >+ /* Get all the top level Flow attributes */ > >+ if ((NlAttrParse(nlMsgHdr, attrOffset, NlMsgAttrsLen(nlMsgHdr), > >+ nlFlowPolicy, ARRAY_SIZE(nlFlowPolicy), > >+ flowAttrs, ARRAY_SIZE(flowAttrs))) > >+ != TRUE) { > >+ OVS_LOG_ERROR("Attr Parsing failed for msg: %p", > >+ nlMsgHdr); > >+ rc = STATUS_INVALID_PARAMETER; > >+ goto done; > >+ } > >+ > > if (flowAttrs[OVS_FLOW_ATTR_PROBE]) { > > rc = OvsProbeSupportedFeature(msgIn, > >flowAttrs[OVS_FLOW_ATTR_KEY]); > > if (rc != STATUS_SUCCESS) { > >@@ -399,8 +400,9 @@ done: > > if (nlError != NL_ERROR_SUCCESS) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > >+ > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > rc = STATUS_SUCCESS; > > } > > > >@@ -568,8 +570,9 @@ done: > > if (nlError != NL_ERROR_SUCCESS) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > >+ > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > rc = STATUS_SUCCESS; > > } > > > >@@ -706,8 +709,9 @@ done: > > if (nlError != NL_ERROR_SUCCESS) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > >+ > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > rc = STATUS_SUCCESS; > > } > > > >diff --git a/datapath-windows/ovsext/Netlink/Netlink.c > >b/datapath-windows/ovsext/Netlink/Netlink.c > >index 27dcd4f..dc1e78c 100644 > >--- a/datapath-windows/ovsext/Netlink/Netlink.c > >+++ b/datapath-windows/ovsext/Netlink/Netlink.c > >@@ -108,12 +108,18 @@ NlFillNlHdr(PNL_BUFFER nlBuf, UINT16 nlmsgType, > > * > >-------------------------------------------------------------------------- > >- > > */ > > VOID > >-NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR msgError, > UINT > >errorCode) > >+NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR msgError, > >+ UINT32 msgErrorLen, > >+ UINT errorCode, UINT32 *msgLen) > > { > > NL_BUFFER nlBuffer; > > > > ASSERT(errorCode != NL_ERROR_PENDING); > > > >+ if ((msgError == NULL) || (msgErrorLen < sizeof *msgError)) { [Paul Boca] In case outputBuffer was NULL, this leads to an access violation when trying to access msgError fields. > >+ return; > >+ } > >+ > > NlBufInit(&nlBuffer, (PCHAR)msgError, sizeof *msgError); > > NlFillNlHdr(&nlBuffer, NLMSG_ERROR, 0, > > msgIn->nlMsg.nlmsgSeq, msgIn->nlMsg.nlmsgPid); > >@@ -121,6 +127,10 @@ NlBuildErrorMsg(POVS_MESSAGE msgIn, > >POVS_MESSAGE_ERROR msgError, UINT errorCode) > > msgError->errorMsg.error = errorCode; > > msgError->errorMsg.nlMsg = msgIn->nlMsg; > > msgError->nlMsg.nlmsgLen = sizeof(OVS_MESSAGE_ERROR); > >+ > >+ if (NULL != msgLen) { > >+ *msgLen = msgError->nlMsg.nlmsgLen; > >+ } > > } > > > > /* > >@@ -1006,7 +1016,7 @@ NlAttrFind__(const PNL_ATTR attrs, UINT32 size, > >UINT16 type) > > { > > PNL_ATTR iter = NULL; > > PNL_ATTR ret = NULL; > >- UINT32 left; > >+ INT left; > > > > NL_ATTR_FOR_EACH (iter, left, attrs, size) { > > if (NlAttrType(iter) == type) { > >@@ -1036,6 +1046,49 @@ NlAttrFindNested(const PNL_ATTR nla, UINT16 > type) > > > > /* > > > >*------------------------------------------------------------------------- > >--- > >+ * Traverses all attributes in received buffer in order to insure all > >are valid > >+ > >*------------------------------------------------------------------------- > >--- > >+ */ > >+BOOLEAN NlValidateAllAttrs(const PNL_MSG_HDR nlMsg, UINT32 attrOffset, > >+ UINT32 totalAttrLen, > >+ const NL_POLICY policy[], const UINT32 > >numPolicy) > >+{ > >+ PNL_ATTR nla; > >+ INT left; > >+ BOOLEAN ret = TRUE; > >+ > >+ if ((NlMsgSize(nlMsg) < attrOffset)) { > >+ OVS_LOG_WARN("No attributes in nlMsg: %p at offset: %d", > >+ nlMsg, attrOffset); > >+ ret = FALSE; > >+ goto done; > >+ } > >+ > >+ NL_ATTR_FOR_EACH_UNSAFE(nla, left, NlMsgAt(nlMsg, attrOffset), > >+ totalAttrLen) > >+ { > >+ if (!NlAttrIsValid(nla, left)) { > >+ ret = FALSE; > >+ goto done; > >+ } > >+ > >+ UINT16 type = NlAttrType(nla); > >+ if (type < numPolicy && policy[type].type != NL_A_NO_ATTR) { > >+ /* Typecasting to keep the compiler happy */ > >+ const PNL_POLICY e = (const PNL_POLICY)(&policy[type]); > >+ if (!NlAttrValidate(nla, e)) { > >+ ret = FALSE; > >+ goto done; > >+ } > >+ } > >+ } > >+ > >+done: > >+ return ret; > >+} > >+ > >+/* > >+ > >*------------------------------------------------------------------------- > >--- > > * Parses the netlink message at a given offset (attrOffset) > > * as a series of attributes. A pointer to the attribute with type > > * 'type' is stored in attrs at index 'type'. policy is used to define > >the > >@@ -1052,20 +1105,13 @@ NlAttrParse(const PNL_MSG_HDR nlMsg, UINT32 > >attrOffset, > > PNL_ATTR attrs[], UINT32 numAttrs) > > { > > PNL_ATTR nla; > >- UINT32 left; > >+ INT left; > > UINT32 iter; > > BOOLEAN ret = FALSE; > > UINT32 numPolicyAttr = MIN(numPolicy, numAttrs); > > > > RtlZeroMemory(attrs, numAttrs * sizeof *attrs); > > > >- > >- /* There is nothing to parse */ > >- if (!(NlMsgAttrsLen(nlMsg))) { > >- ret = TRUE; > >- goto done; > >- } > >- > > if ((NlMsgSize(nlMsg) < attrOffset)) { > > OVS_LOG_WARN("No attributes in nlMsg: %p at offset: %d", > > nlMsg, attrOffset); > >diff --git a/datapath-windows/ovsext/Netlink/Netlink.h > >b/datapath-windows/ovsext/Netlink/Netlink.h > >index 8f6a5be..63164c7 100644 > >--- a/datapath-windows/ovsext/Netlink/Netlink.h > >+++ b/datapath-windows/ovsext/Netlink/Netlink.h > >@@ -72,6 +72,7 @@ typedef struct _NL_POLICY > > /* This macro is careful to check for attributes with bad lengths. */ > > #define NL_ATTR_FOR_EACH(ITER, LEFT, ATTRS, ATTRS_LEN) \ > > for ((ITER) = (ATTRS), (LEFT) = (ATTRS_LEN); \ > >+ ((INT)LEFT) >= (INT)NLA_ALIGN(sizeof(NL_ATTR)) && \ [Paul Boca] We must ensure that we don't have a positive value all the time if the sum of all attr lenghts don't match exactly the total length. > > NlAttrIsValid(ITER, LEFT); \ > > (LEFT) -= NlAttrLenPad(ITER, LEFT), (ITER) = NlAttrNext(ITER)) > > > >@@ -80,7 +81,7 @@ typedef struct _NL_POLICY > > * already been validated (e.g. with NL_ATTR_FOR_EACH). */ > > #define NL_ATTR_FOR_EACH_UNSAFE(ITER, LEFT, ATTRS, ATTRS_LEN) \ > > for ((ITER) = (ATTRS), (LEFT) = (ATTRS_LEN); \ > >- (LEFT) > 0; \ > >+ ((INT)LEFT) >= (INT)NLA_ALIGN(sizeof(NL_ATTR)); \ > > (LEFT) -= NLA_ALIGN((ITER)->nlaLen), (ITER) = NlAttrNext(ITER)) > > > > #define NL_ATTR_GET_AS(NLA, TYPE) \ > >@@ -94,8 +95,9 @@ BOOLEAN NlFillNlHdr(PNL_BUFFER nlBuf, > > UINT16 nlmsgType, UINT16 nlmsgFlags, > > UINT32 nlmsgSeq, UINT32 nlmsgPid); > > > >-VOID NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR > msgOut, > >- UINT errorCode); > >+VOID NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR > msgError, > >+ UINT32 msgErrorLen, > >+ UINT errorCode, UINT32 *msgLen); > > > > /* Netlink message accessing the payload */ > > PVOID NlMsgAt(const PNL_MSG_HDR nlh, UINT32 offset); > >@@ -187,6 +189,11 @@ static __inline NlAttrIsLast(const PNL_ATTR nla, int > >rem) > > /* Netlink attribute validation */ > > BOOLEAN NlAttrValidate(const PNL_ATTR, const PNL_POLICY); > > > >+/* Netlink attribute stream validation */ > >+BOOLEAN NlValidateAllAttrs(const PNL_MSG_HDR nlMsg, UINT32 attrOffset, > >+ UINT32 totalAttrLen, > >+ const NL_POLICY policy[], const UINT32 numPolicy); > >+ > > /* Put APis */ > > BOOLEAN NlMsgPutNlHdr(PNL_BUFFER buf, PNL_MSG_HDR nlMsg); > > BOOLEAN NlMsgPutGenlHdr(PNL_BUFFER buf, PGENL_MSG_HDR genlMsg); > >diff --git a/datapath-windows/ovsext/User.c > >b/datapath-windows/ovsext/User.c > >index 33cbd89..bc010c7 100644 > >--- a/datapath-windows/ovsext/User.c > >+++ b/datapath-windows/ovsext/User.c > >@@ -345,8 +345,9 @@ > OvsNlExecuteCmdHandler(POVS_USER_PARAMS_CONTEXT > >usrParamsCtx, > > > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > >+ > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, > >replyLen); > > status = STATUS_SUCCESS; > > goto done; > > } > >diff --git a/datapath-windows/ovsext/Vport.c > >b/datapath-windows/ovsext/Vport.c > >index d04b12b..4299169 100644 > >--- a/datapath-windows/ovsext/Vport.c > >+++ b/datapath-windows/ovsext/Vport.c > >@@ -1729,9 +1729,9 @@ cleanup: > > if (nlError != NL_ERROR_SUCCESS) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > > > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > } > > > > return STATUS_SUCCESS; > >@@ -2088,9 +2088,9 @@ Cleanup: > > if (nlError != NL_ERROR_SUCCESS) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > > > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > } > > > > return STATUS_SUCCESS; > >@@ -2324,6 +2324,7 @@ Cleanup: > > if ((nlError != NL_ERROR_SUCCESS) && (nlError != NL_ERROR_PENDING)) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > > > > if (vport && vportAllocated == TRUE) { > > if (vportInitialized == TRUE) { > >@@ -2343,8 +2344,7 @@ Cleanup: > > OvsFreeMemoryWithTag(vport, OVS_VPORT_POOL_TAG); > > } > > > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > } > > > > return (status == STATUS_PENDING) ? STATUS_PENDING : > STATUS_SUCCESS; > >@@ -2452,9 +2452,9 @@ Cleanup: > > if (nlError != NL_ERROR_SUCCESS) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > > > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > } > > > > return STATUS_SUCCESS; > >@@ -2544,9 +2544,9 @@ Cleanup: > > if ((nlError != NL_ERROR_SUCCESS) && (nlError != NL_ERROR_PENDING)) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > > > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > } > > > > return (status == STATUS_PENDING) ? STATUS_PENDING : > STATUS_SUCCESS; > >@@ -2579,10 +2579,11 @@ OvsTunnelVportPendingRemove(PVOID context, > > > > *replyLen = msgOut->nlMsg.nlmsgLen; > > } else { > >- POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)msgOut; > >+ POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > >+ tunnelContext->outputBuffer; > >+ UINT32 msgErrorLen = tunnelContext->outputLength; > > > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, > >replyLen); > > } > > } > > > >@@ -2721,12 +2722,13 @@ OvsTunnelVportPendingInit(PVOID context, > > } while (error); > > > > if (error) { > >- POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) msgOut; > >+ POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > >+ tunnelContext->outputBuffer; > >+ UINT32 msgErrorLen = tunnelContext->outputLength; > > > > OvsCleanupVxlanTunnel(NULL, vport, NULL, NULL); > > OvsFreeMemory(vport); > > > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > } > > } > >diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c > >index cad9490..32b0cc3 100644 > >--- a/lib/netlink-socket.c > >+++ b/lib/netlink-socket.c > >@@ -127,6 +127,7 @@ nl_sock_create(int protocol, struct nl_sock **sockp) > > sock = xmalloc(sizeof *sock); > > > > #ifdef _WIN32 > >+ sock->overlapped.hEvent = NULL; > > sock->handle = CreateFile(OVS_DEVICE_NAME_USER, > > GENERIC_READ | GENERIC_WRITE, > > FILE_SHARE_READ | FILE_SHARE_WRITE, > >@@ -1191,6 +1192,7 @@ pend_io_request(struct nl_sock *sock) > > > > ovs_header = ofpbuf_put_uninit(&request, sizeof *ovs_header); > > ovs_header->dp_ifindex = 0; > >+ nlmsg->nlmsg_len = request.size; > > > > if (!DeviceIoControl(sock->handle, OVS_IOCTL_WRITE, > > request.data, request.size, > >-- > >2.7.2.windows.1 > >_______________________________________________ > >dev mailing list > >dev@openvswitch.org > >https://urldefense.proofpoint.com/v2/url?u=http- > 3A__openvswitch.org_mailma > >n_listinfo_dev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt- > uEs&r=pN > >HQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=GabGAlmcY24uVSoAQCa > iRYFIYSfiDS > >JZomM6llWwZmA&s=N-QN8hqnSsvjfhoygdktRzwFa6jdc2QCMZelWxlsT3g&e=
Hi Paul, I looked at the change in detail and it is definitely in the right spirit to harden the kernel datapath code. However, I thought a few things could be simplified a little. I will be sending out a couple of simple reviews on top of your patch (that is already submitted). Pls. take a look. Other than that the patch looks good. Thanks, -- Nithin -----Original Message----- From: dev <dev-bounces@openvswitch.org> on behalf of Paul Boca <pboca@cloudbasesolutions.com> Date: Wednesday, April 27, 2016 at 1:05 AM To: "dev@openvswitch.org" <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> >Acked-by: Alin Gabriel Serdean <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(-) > >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; > break; > >@@ -903,7 +922,8 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject, > * "artificial" or was copied from a previously validated 'ovsMsg'. > */ > if (devOp != OVS_READ_DEV_OP) { >- status = ValidateNetlinkCmd(devOp, instance, ovsMsg, >nlFamilyOps); >+ status = ValidateNetlinkCmd(devOp, instance, ovsMsg, >+ ovsMsgLength, nlFamilyOps); > if (status != STATUS_SUCCESS) { > goto done; > } >@@ -938,11 +958,18 @@ static NTSTATUS > ValidateNetlinkCmd(UINT32 devOp, > POVS_OPEN_INSTANCE instance, > POVS_MESSAGE ovsMsg, >+ UINT32 ovsMsgLength, > NETLINK_FAMILY *nlFamilyOps) > { > NTSTATUS status = STATUS_INVALID_PARAMETER; > UINT16 i; > >+ // We need to ensure we have enough data to process >+ if (NlMsgSize(&ovsMsg->nlMsg) > ovsMsgLength) { >+ status = STATUS_INVALID_PARAMETER; >+ goto done; >+ } Sure, this can be a useful check for integrity. We have not relied on ovsMsg->nlMsg anytime, but it is a general goodness check. >+ > for (i = 0; i < nlFamilyOps->opsCount; i++) { > if (nlFamilyOps->cmds[i].cmd == ovsMsg->genlMsg.cmd) { > /* Validate if the command is valid for the device >operation. */ >@@ -977,6 +1004,14 @@ ValidateNetlinkCmd(UINT32 devOp, > } > } > >+ // validate all NlAttrs >+ if (!NlValidateAllAttrs(&ovsMsg->nlMsg, sizeof(*ovsMsg), >+ NlMsgAttrsLen((PNL_MSG_HDR)ovsMsg), >+ NULL, 0)) { >+ status = STATUS_INVALID_PARAMETER; >+ goto done; >+ } I am not fully convinced this check will buy us anything. The reason is that we do the same check later in each of the handler functions along with the appropriate policy. In fact, the new code parses attributes twice for no additional benefit. It only contributes to increased CPU usage, but I see your point that we¹ll have a centralized location to parse all of the attributes. Maybe useful. Let me think about it more. >+ > done: > return status; > } >@@ -1028,6 +1063,7 @@ InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT >usrParamsCtx, > POVS_MESSAGE msgIn = NULL; > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > usrParamsCtx->outputBuffer; >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > > if (usrParamsCtx->ovsMsg->genlMsg.cmd == >OVS_CTRL_CMD_EVENT_NOTIFY || > usrParamsCtx->ovsMsg->genlMsg.cmd == >OVS_CTRL_CMD_READ_NOTIFY) { >@@ -1043,8 +1079,7 @@ InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT >usrParamsCtx, > > ASSERT(msgIn); > ASSERT(msgError); >- NlBuildErrorMsg(msgIn, msgError, nlError); >- *replyLen = msgError->nlMsg.nlmsgLen; >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, >replyLen); > } > > if (*replyLen != 0) { >@@ -1416,9 +1451,9 @@ cleanup: > if (nlError != NL_ERROR_SUCCESS) { > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > usrParamsCtx->outputBuffer; >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > >- NlBuildErrorMsg(msgIn, msgError, nlError); >- *replyLen = msgError->nlMsg.nlmsgLen; >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > } > > return STATUS_SUCCESS; >diff --git a/datapath-windows/ovsext/Flow.c >b/datapath-windows/ovsext/Flow.c >index a49a60c..792b614 100644 >--- a/datapath-windows/ovsext/Flow.c >+++ b/datapath-windows/ovsext/Flow.c >@@ -285,20 +285,10 @@ OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT >usrParamsCtx, > goto done; > } > >- /* Get all the top level Flow attributes */ >- if ((NlAttrParse(nlMsgHdr, attrOffset, NlMsgAttrsLen(nlMsgHdr), >- nlFlowPolicy, ARRAY_SIZE(nlFlowPolicy), >- flowAttrs, ARRAY_SIZE(flowAttrs))) >- != TRUE) { >- OVS_LOG_ERROR("Attr Parsing failed for msg: %p", >- nlMsgHdr); >- rc = STATUS_INVALID_PARAMETER; >- goto done; >- } >- >- /* FLOW_DEL command w/o any key input is a flush case. */ >+ /* FLOW_DEL command w/o any key input is a flush case. >+ If we don't have any attr, we treat this as a flush command*/ > if ((genlMsgHdr->cmd == OVS_FLOW_CMD_DEL) && >- (!(flowAttrs[OVS_FLOW_ATTR_KEY]))) { >+ (!NlMsgAttrsLen(nlMsgHdr))) { > > rc = OvsFlushFlowIoctl(ovsHdr->dp_ifindex); > >@@ -323,6 +313,17 @@ OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT >usrParamsCtx, > goto done; > } > >+ /* Get all the top level Flow attributes */ >+ if ((NlAttrParse(nlMsgHdr, attrOffset, NlMsgAttrsLen(nlMsgHdr), >+ nlFlowPolicy, ARRAY_SIZE(nlFlowPolicy), >+ flowAttrs, ARRAY_SIZE(flowAttrs))) >+ != TRUE) { >+ OVS_LOG_ERROR("Attr Parsing failed for msg: %p", >+ nlMsgHdr); >+ rc = STATUS_INVALID_PARAMETER; >+ goto done; >+ } >+ > if (flowAttrs[OVS_FLOW_ATTR_PROBE]) { > rc = OvsProbeSupportedFeature(msgIn, >flowAttrs[OVS_FLOW_ATTR_KEY]); > if (rc != STATUS_SUCCESS) { >@@ -399,8 +400,9 @@ done: > if (nlError != NL_ERROR_SUCCESS) { > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > usrParamsCtx->outputBuffer; >- NlBuildErrorMsg(msgIn, msgError, nlError); >- *replyLen = msgError->nlMsg.nlmsgLen; >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; >+ >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > rc = STATUS_SUCCESS; > } > >@@ -568,8 +570,9 @@ done: > if (nlError != NL_ERROR_SUCCESS) { > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > usrParamsCtx->outputBuffer; >- NlBuildErrorMsg(msgIn, msgError, nlError); >- *replyLen = msgError->nlMsg.nlmsgLen; >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; >+ >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > rc = STATUS_SUCCESS; > } > >@@ -706,8 +709,9 @@ done: > if (nlError != NL_ERROR_SUCCESS) { > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > usrParamsCtx->outputBuffer; >- NlBuildErrorMsg(msgIn, msgError, nlError); >- *replyLen = msgError->nlMsg.nlmsgLen; >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; >+ >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > rc = STATUS_SUCCESS; > } > >diff --git a/datapath-windows/ovsext/Netlink/Netlink.c >b/datapath-windows/ovsext/Netlink/Netlink.c >index 27dcd4f..dc1e78c 100644 >--- a/datapath-windows/ovsext/Netlink/Netlink.c >+++ b/datapath-windows/ovsext/Netlink/Netlink.c >@@ -108,12 +108,18 @@ NlFillNlHdr(PNL_BUFFER nlBuf, UINT16 nlmsgType, > * >-------------------------------------------------------------------------- >- > */ > VOID >-NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR msgError, UINT >errorCode) >+NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR msgError, >+ UINT32 msgErrorLen, >+ UINT errorCode, UINT32 *msgLen) > { > NL_BUFFER nlBuffer; > > ASSERT(errorCode != NL_ERROR_PENDING); > >+ if ((msgError == NULL) || (msgErrorLen < sizeof *msgError)) { >+ return; >+ } I am not sure if it is a good idea to silently return here. If ŒmsgError¹ is NULL, it is a contract violation, and must be caught. This is why we have ASSERTs for msgError before the call to NlBuildErrorMsg. Yes, ASSERTs are compiled out in !DBG code, but I am ok with taking a BSOD and fixing an issue rather than silently not report an error. For checking of msgError, I¹d rather increase the amount of memory we map in MapIrpOutputBuffer(), and bail out there itself. >+ > NlBufInit(&nlBuffer, (PCHAR)msgError, sizeof *msgError); > NlFillNlHdr(&nlBuffer, NLMSG_ERROR, 0, > msgIn->nlMsg.nlmsgSeq, msgIn->nlMsg.nlmsgPid); >@@ -121,6 +127,10 @@ NlBuildErrorMsg(POVS_MESSAGE msgIn, >POVS_MESSAGE_ERROR msgError, UINT errorCode) > msgError->errorMsg.error = errorCode; > msgError->errorMsg.nlMsg = msgIn->nlMsg; > msgError->nlMsg.nlmsgLen = sizeof(OVS_MESSAGE_ERROR); >+ >+ if (NULL != msgLen) { >+ *msgLen = msgError->nlMsg.nlmsgLen; >+ } Sure, this is a good cleanup to set replyLen here itself rather than outside of the function. > } > > /* >@@ -1006,7 +1016,7 @@ NlAttrFind__(const PNL_ATTR attrs, UINT32 size, >UINT16 type) > { > PNL_ATTR iter = NULL; > PNL_ATTR ret = NULL; >- UINT32 left; >+ INT left; > > NL_ATTR_FOR_EACH (iter, left, attrs, size) { > if (NlAttrType(iter) == type) { >@@ -1036,6 +1046,49 @@ NlAttrFindNested(const PNL_ATTR nla, UINT16 type) > > /* > >*------------------------------------------------------------------------- >--- >+ * Traverses all attributes in received buffer in order to insure all >are valid >+ >*------------------------------------------------------------------------- >--- >+ */ >+BOOLEAN NlValidateAllAttrs(const PNL_MSG_HDR nlMsg, UINT32 attrOffset, >+ UINT32 totalAttrLen, >+ const NL_POLICY policy[], const UINT32 >numPolicy) >+{ >+ PNL_ATTR nla; >+ INT left; >+ BOOLEAN ret = TRUE; >+ >+ if ((NlMsgSize(nlMsg) < attrOffset)) { >+ OVS_LOG_WARN("No attributes in nlMsg: %p at offset: %d", >+ nlMsg, attrOffset); >+ ret = FALSE; >+ goto done; >+ } >+ >+ NL_ATTR_FOR_EACH_UNSAFE(nla, left, NlMsgAt(nlMsg, attrOffset), >+ totalAttrLen) >+ { >+ if (!NlAttrIsValid(nla, left)) { >+ ret = FALSE; >+ goto done; >+ } >+ >+ UINT16 type = NlAttrType(nla); >+ if (type < numPolicy && policy[type].type != NL_A_NO_ATTR) { >+ /* Typecasting to keep the compiler happy */ >+ const PNL_POLICY e = (const PNL_POLICY)(&policy[type]); >+ if (!NlAttrValidate(nla, e)) { >+ ret = FALSE; >+ goto done; >+ } >+ } >+ } >+ >+done: >+ return ret; >+} >+ >+/* >+ >*------------------------------------------------------------------------- >--- > * Parses the netlink message at a given offset (attrOffset) > * as a series of attributes. A pointer to the attribute with type > * 'type' is stored in attrs at index 'type'. policy is used to define >the >@@ -1052,20 +1105,13 @@ NlAttrParse(const PNL_MSG_HDR nlMsg, UINT32 >attrOffset, > PNL_ATTR attrs[], UINT32 numAttrs) > { > PNL_ATTR nla; >- UINT32 left; >+ INT left; > UINT32 iter; > BOOLEAN ret = FALSE; > UINT32 numPolicyAttr = MIN(numPolicy, numAttrs); > > RtlZeroMemory(attrs, numAttrs * sizeof *attrs); > >- >- /* There is nothing to parse */ >- if (!(NlMsgAttrsLen(nlMsg))) { >- ret = TRUE; >- goto done; >- } >- > if ((NlMsgSize(nlMsg) < attrOffset)) { > OVS_LOG_WARN("No attributes in nlMsg: %p at offset: %d", > nlMsg, attrOffset); >diff --git a/datapath-windows/ovsext/Netlink/Netlink.h >b/datapath-windows/ovsext/Netlink/Netlink.h >index 8f6a5be..63164c7 100644 >--- a/datapath-windows/ovsext/Netlink/Netlink.h >+++ b/datapath-windows/ovsext/Netlink/Netlink.h >@@ -72,6 +72,7 @@ typedef struct _NL_POLICY > /* This macro is careful to check for attributes with bad lengths. */ > #define NL_ATTR_FOR_EACH(ITER, LEFT, ATTRS, ATTRS_LEN) \ > for ((ITER) = (ATTRS), (LEFT) = (ATTRS_LEN); \ >+ ((INT)LEFT) >= (INT)NLA_ALIGN(sizeof(NL_ATTR)) && \ > NlAttrIsValid(ITER, LEFT); \ > (LEFT) -= NlAttrLenPad(ITER, LEFT), (ITER) = NlAttrNext(ITER)) > >@@ -80,7 +81,7 @@ typedef struct _NL_POLICY > * already been validated (e.g. with NL_ATTR_FOR_EACH). */ > #define NL_ATTR_FOR_EACH_UNSAFE(ITER, LEFT, ATTRS, ATTRS_LEN) \ > for ((ITER) = (ATTRS), (LEFT) = (ATTRS_LEN); \ >- (LEFT) > 0; \ >+ ((INT)LEFT) >= (INT)NLA_ALIGN(sizeof(NL_ATTR)); \ > (LEFT) -= NLA_ALIGN((ITER)->nlaLen), (ITER) = NlAttrNext(ITER)) > > #define NL_ATTR_GET_AS(NLA, TYPE) \ >@@ -94,8 +95,9 @@ BOOLEAN NlFillNlHdr(PNL_BUFFER nlBuf, > UINT16 nlmsgType, UINT16 nlmsgFlags, > UINT32 nlmsgSeq, UINT32 nlmsgPid); > >-VOID NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR msgOut, >- UINT errorCode); >+VOID NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR msgError, >+ UINT32 msgErrorLen, >+ UINT errorCode, UINT32 *msgLen); > > /* Netlink message accessing the payload */ > PVOID NlMsgAt(const PNL_MSG_HDR nlh, UINT32 offset); >@@ -187,6 +189,11 @@ static __inline NlAttrIsLast(const PNL_ATTR nla, int >rem) > /* Netlink attribute validation */ > BOOLEAN NlAttrValidate(const PNL_ATTR, const PNL_POLICY); > >+/* Netlink attribute stream validation */ >+BOOLEAN NlValidateAllAttrs(const PNL_MSG_HDR nlMsg, UINT32 attrOffset, >+ UINT32 totalAttrLen, >+ const NL_POLICY policy[], const UINT32 numPolicy); >+ > /* Put APis */ > BOOLEAN NlMsgPutNlHdr(PNL_BUFFER buf, PNL_MSG_HDR nlMsg); > BOOLEAN NlMsgPutGenlHdr(PNL_BUFFER buf, PGENL_MSG_HDR genlMsg); >diff --git a/datapath-windows/ovsext/User.c >b/datapath-windows/ovsext/User.c >index 33cbd89..bc010c7 100644 >--- a/datapath-windows/ovsext/User.c >+++ b/datapath-windows/ovsext/User.c >@@ -345,8 +345,9 @@ OvsNlExecuteCmdHandler(POVS_USER_PARAMS_CONTEXT >usrParamsCtx, > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > usrParamsCtx->outputBuffer; >- NlBuildErrorMsg(msgIn, msgError, nlError); >- *replyLen = msgError->nlMsg.nlmsgLen; >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; >+ >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, >replyLen); > status = STATUS_SUCCESS; > goto done; > } >diff --git a/datapath-windows/ovsext/Vport.c >b/datapath-windows/ovsext/Vport.c >index d04b12b..4299169 100644 >--- a/datapath-windows/ovsext/Vport.c >+++ b/datapath-windows/ovsext/Vport.c >@@ -1729,9 +1729,9 @@ cleanup: > if (nlError != NL_ERROR_SUCCESS) { > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > usrParamsCtx->outputBuffer; >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > >- NlBuildErrorMsg(msgIn, msgError, nlError); >- *replyLen = msgError->nlMsg.nlmsgLen; >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > } > > return STATUS_SUCCESS; >@@ -2088,9 +2088,9 @@ Cleanup: > if (nlError != NL_ERROR_SUCCESS) { > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > usrParamsCtx->outputBuffer; >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > >- NlBuildErrorMsg(msgIn, msgError, nlError); >- *replyLen = msgError->nlMsg.nlmsgLen; >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > } > > return STATUS_SUCCESS; >@@ -2324,6 +2324,7 @@ Cleanup: > if ((nlError != NL_ERROR_SUCCESS) && (nlError != NL_ERROR_PENDING)) { > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > usrParamsCtx->outputBuffer; >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > > if (vport && vportAllocated == TRUE) { > if (vportInitialized == TRUE) { >@@ -2343,8 +2344,7 @@ Cleanup: > OvsFreeMemoryWithTag(vport, OVS_VPORT_POOL_TAG); > } > >- NlBuildErrorMsg(msgIn, msgError, nlError); >- *replyLen = msgError->nlMsg.nlmsgLen; >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > } > > return (status == STATUS_PENDING) ? STATUS_PENDING : STATUS_SUCCESS; >@@ -2452,9 +2452,9 @@ Cleanup: > if (nlError != NL_ERROR_SUCCESS) { > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > usrParamsCtx->outputBuffer; >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > >- NlBuildErrorMsg(msgIn, msgError, nlError); >- *replyLen = msgError->nlMsg.nlmsgLen; >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > } > > return STATUS_SUCCESS; >@@ -2544,9 +2544,9 @@ Cleanup: > if ((nlError != NL_ERROR_SUCCESS) && (nlError != NL_ERROR_PENDING)) { > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > usrParamsCtx->outputBuffer; >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > >- NlBuildErrorMsg(msgIn, msgError, nlError); >- *replyLen = msgError->nlMsg.nlmsgLen; >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > } > > return (status == STATUS_PENDING) ? STATUS_PENDING : STATUS_SUCCESS; >@@ -2579,10 +2579,11 @@ OvsTunnelVportPendingRemove(PVOID context, > > *replyLen = msgOut->nlMsg.nlmsgLen; > } else { >- POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)msgOut; >+ POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) >+ tunnelContext->outputBuffer; >+ UINT32 msgErrorLen = tunnelContext->outputLength; > >- NlBuildErrorMsg(msgIn, msgError, nlError); >- *replyLen = msgError->nlMsg.nlmsgLen; >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, >replyLen); > } > } > >@@ -2721,12 +2722,13 @@ OvsTunnelVportPendingInit(PVOID context, > } while (error); > > if (error) { >- POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) msgOut; >+ POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) >+ tunnelContext->outputBuffer; >+ UINT32 msgErrorLen = tunnelContext->outputLength; > > OvsCleanupVxlanTunnel(NULL, vport, NULL, NULL); > OvsFreeMemory(vport); > >- NlBuildErrorMsg(msgIn, msgError, nlError); >- *replyLen = msgError->nlMsg.nlmsgLen; >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > } > } >diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c >index cad9490..32b0cc3 100644 >--- a/lib/netlink-socket.c >+++ b/lib/netlink-socket.c >@@ -127,6 +127,7 @@ nl_sock_create(int protocol, struct nl_sock **sockp) > sock = xmalloc(sizeof *sock); > > #ifdef _WIN32 >+ sock->overlapped.hEvent = NULL; Good catch. > sock->handle = CreateFile(OVS_DEVICE_NAME_USER, > GENERIC_READ | GENERIC_WRITE, > FILE_SHARE_READ | FILE_SHARE_WRITE, >@@ -1191,6 +1192,7 @@ pend_io_request(struct nl_sock *sock) > > ovs_header = ofpbuf_put_uninit(&request, sizeof *ovs_header); > ovs_header->dp_ifindex = 0; >+ nlmsg->nlmsg_len = request.size; Good catch. > > if (!DeviceIoControl(sock->handle, OVS_IOCTL_WRITE, > request.data, request.size,
Hi Nithin! Thanks for your review and changes! Regards, Paul > -----Original Message----- > From: Nithin Raju [mailto:nithin@vmware.com] > Sent: Friday, May 20, 2016 1:31 AM > To: Paul Boca; dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH V2] datapath-windows: Validate netlink packets > integrity > > Hi Paul, > I looked at the change in detail and it is definitely in the right spirit > to harden the kernel datapath code. > > However, I thought a few things could be simplified a little. I will be > sending out a couple of simple reviews on top of your patch (that is > already submitted). Pls. take a look. > > Other than that the patch looks good. > > Thanks, > -- Nithin > > -----Original Message----- > From: dev <dev-bounces@openvswitch.org> on behalf of Paul Boca > <pboca@cloudbasesolutions.com> > Date: Wednesday, April 27, 2016 at 1:05 AM > To: "dev@openvswitch.org" <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> > >Acked-by: Alin Gabriel Serdean <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(-) > > > >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; > > break; > > > >@@ -903,7 +922,8 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject, > > * "artificial" or was copied from a previously validated 'ovsMsg'. > > */ > > if (devOp != OVS_READ_DEV_OP) { > >- status = ValidateNetlinkCmd(devOp, instance, ovsMsg, > >nlFamilyOps); > >+ status = ValidateNetlinkCmd(devOp, instance, ovsMsg, > >+ ovsMsgLength, nlFamilyOps); > > if (status != STATUS_SUCCESS) { > > goto done; > > } > >@@ -938,11 +958,18 @@ static NTSTATUS > > ValidateNetlinkCmd(UINT32 devOp, > > POVS_OPEN_INSTANCE instance, > > POVS_MESSAGE ovsMsg, > >+ UINT32 ovsMsgLength, > > NETLINK_FAMILY *nlFamilyOps) > > { > > NTSTATUS status = STATUS_INVALID_PARAMETER; > > UINT16 i; > > > >+ // We need to ensure we have enough data to process > >+ if (NlMsgSize(&ovsMsg->nlMsg) > ovsMsgLength) { > >+ status = STATUS_INVALID_PARAMETER; > >+ goto done; > >+ } > > Sure, this can be a useful check for integrity. We have not relied on > ovsMsg->nlMsg anytime, but it is a general goodness check. > > >+ > > for (i = 0; i < nlFamilyOps->opsCount; i++) { > > if (nlFamilyOps->cmds[i].cmd == ovsMsg->genlMsg.cmd) { > > /* Validate if the command is valid for the device > >operation. */ > >@@ -977,6 +1004,14 @@ ValidateNetlinkCmd(UINT32 devOp, > > } > > } > > > >+ // validate all NlAttrs > >+ if (!NlValidateAllAttrs(&ovsMsg->nlMsg, sizeof(*ovsMsg), > >+ NlMsgAttrsLen((PNL_MSG_HDR)ovsMsg), > >+ NULL, 0)) { > >+ status = STATUS_INVALID_PARAMETER; > >+ goto done; > >+ } > > I am not fully convinced this check will buy us anything. The reason is > that we do the same check later in each of the handler functions along > with the appropriate policy. In fact, the new code parses attributes twice > for no additional benefit. It only contributes to increased CPU usage, but > I see your point that we¹ll have a centralized location to parse all of > the attributes. Maybe useful. Let me think about it more. > > >+ > > done: > > return status; > > } > >@@ -1028,6 +1063,7 @@ > InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT > >usrParamsCtx, > > POVS_MESSAGE msgIn = NULL; > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > > > > if (usrParamsCtx->ovsMsg->genlMsg.cmd == > >OVS_CTRL_CMD_EVENT_NOTIFY || > > usrParamsCtx->ovsMsg->genlMsg.cmd == > >OVS_CTRL_CMD_READ_NOTIFY) { > >@@ -1043,8 +1079,7 @@ > InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT > >usrParamsCtx, > > > > ASSERT(msgIn); > > ASSERT(msgError); > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, > >replyLen); > > } > > > > if (*replyLen != 0) { > >@@ -1416,9 +1451,9 @@ cleanup: > > if (nlError != NL_ERROR_SUCCESS) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > > > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > } > > > > return STATUS_SUCCESS; > >diff --git a/datapath-windows/ovsext/Flow.c > >b/datapath-windows/ovsext/Flow.c > >index a49a60c..792b614 100644 > >--- a/datapath-windows/ovsext/Flow.c > >+++ b/datapath-windows/ovsext/Flow.c > >@@ -285,20 +285,10 @@ > OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT > >usrParamsCtx, > > goto done; > > } > > > >- /* Get all the top level Flow attributes */ > >- if ((NlAttrParse(nlMsgHdr, attrOffset, NlMsgAttrsLen(nlMsgHdr), > >- nlFlowPolicy, ARRAY_SIZE(nlFlowPolicy), > >- flowAttrs, ARRAY_SIZE(flowAttrs))) > >- != TRUE) { > >- OVS_LOG_ERROR("Attr Parsing failed for msg: %p", > >- nlMsgHdr); > >- rc = STATUS_INVALID_PARAMETER; > >- goto done; > >- } > >- > >- /* FLOW_DEL command w/o any key input is a flush case. */ > >+ /* FLOW_DEL command w/o any key input is a flush case. > >+ If we don't have any attr, we treat this as a flush command*/ > > if ((genlMsgHdr->cmd == OVS_FLOW_CMD_DEL) && > >- (!(flowAttrs[OVS_FLOW_ATTR_KEY]))) { > >+ (!NlMsgAttrsLen(nlMsgHdr))) { > > > > rc = OvsFlushFlowIoctl(ovsHdr->dp_ifindex); > > > >@@ -323,6 +313,17 @@ > OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT > >usrParamsCtx, > > goto done; > > } > > > >+ /* Get all the top level Flow attributes */ > >+ if ((NlAttrParse(nlMsgHdr, attrOffset, NlMsgAttrsLen(nlMsgHdr), > >+ nlFlowPolicy, ARRAY_SIZE(nlFlowPolicy), > >+ flowAttrs, ARRAY_SIZE(flowAttrs))) > >+ != TRUE) { > >+ OVS_LOG_ERROR("Attr Parsing failed for msg: %p", > >+ nlMsgHdr); > >+ rc = STATUS_INVALID_PARAMETER; > >+ goto done; > >+ } > >+ > > if (flowAttrs[OVS_FLOW_ATTR_PROBE]) { > > rc = OvsProbeSupportedFeature(msgIn, > >flowAttrs[OVS_FLOW_ATTR_KEY]); > > if (rc != STATUS_SUCCESS) { > >@@ -399,8 +400,9 @@ done: > > if (nlError != NL_ERROR_SUCCESS) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > >+ > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > rc = STATUS_SUCCESS; > > } > > > >@@ -568,8 +570,9 @@ done: > > if (nlError != NL_ERROR_SUCCESS) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > >+ > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > rc = STATUS_SUCCESS; > > } > > > >@@ -706,8 +709,9 @@ done: > > if (nlError != NL_ERROR_SUCCESS) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > >+ > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > rc = STATUS_SUCCESS; > > } > > > >diff --git a/datapath-windows/ovsext/Netlink/Netlink.c > >b/datapath-windows/ovsext/Netlink/Netlink.c > >index 27dcd4f..dc1e78c 100644 > >--- a/datapath-windows/ovsext/Netlink/Netlink.c > >+++ b/datapath-windows/ovsext/Netlink/Netlink.c > >@@ -108,12 +108,18 @@ NlFillNlHdr(PNL_BUFFER nlBuf, UINT16 nlmsgType, > > * > >-------------------------------------------------------------------------- > >- > > */ > > VOID > >-NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR msgError, > UINT > >errorCode) > >+NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR msgError, > >+ UINT32 msgErrorLen, > >+ UINT errorCode, UINT32 *msgLen) > > { > > NL_BUFFER nlBuffer; > > > > ASSERT(errorCode != NL_ERROR_PENDING); > > > >+ if ((msgError == NULL) || (msgErrorLen < sizeof *msgError)) { > >+ return; > >+ } > > I am not sure if it is a good idea to silently return here. If ŒmsgError¹ > is NULL, it is a contract violation, and must be caught. This is why we > have ASSERTs for msgError before the call to NlBuildErrorMsg. Yes, ASSERTs > are compiled out in !DBG code, but I am ok with taking a BSOD and fixing > an issue rather than silently not report an error. > > For checking of msgError, I¹d rather increase the amount of memory we map > in MapIrpOutputBuffer(), and bail out there itself. > > >+ > > NlBufInit(&nlBuffer, (PCHAR)msgError, sizeof *msgError); > > NlFillNlHdr(&nlBuffer, NLMSG_ERROR, 0, > > msgIn->nlMsg.nlmsgSeq, msgIn->nlMsg.nlmsgPid); > >@@ -121,6 +127,10 @@ NlBuildErrorMsg(POVS_MESSAGE msgIn, > >POVS_MESSAGE_ERROR msgError, UINT errorCode) > > msgError->errorMsg.error = errorCode; > > msgError->errorMsg.nlMsg = msgIn->nlMsg; > > msgError->nlMsg.nlmsgLen = sizeof(OVS_MESSAGE_ERROR); > >+ > >+ if (NULL != msgLen) { > >+ *msgLen = msgError->nlMsg.nlmsgLen; > >+ } > > Sure, this is a good cleanup to set replyLen here itself rather than > outside of the function. > > > } > > > > /* > >@@ -1006,7 +1016,7 @@ NlAttrFind__(const PNL_ATTR attrs, UINT32 size, > >UINT16 type) > > { > > PNL_ATTR iter = NULL; > > PNL_ATTR ret = NULL; > >- UINT32 left; > >+ INT left; > > > > NL_ATTR_FOR_EACH (iter, left, attrs, size) { > > if (NlAttrType(iter) == type) { > >@@ -1036,6 +1046,49 @@ NlAttrFindNested(const PNL_ATTR nla, UINT16 > type) > > > > /* > > > >*------------------------------------------------------------------------- > >--- > >+ * Traverses all attributes in received buffer in order to insure all > >are valid > >+ > >*------------------------------------------------------------------------- > >--- > >+ */ > >+BOOLEAN NlValidateAllAttrs(const PNL_MSG_HDR nlMsg, UINT32 attrOffset, > >+ UINT32 totalAttrLen, > >+ const NL_POLICY policy[], const UINT32 > >numPolicy) > >+{ > >+ PNL_ATTR nla; > >+ INT left; > >+ BOOLEAN ret = TRUE; > >+ > >+ if ((NlMsgSize(nlMsg) < attrOffset)) { > >+ OVS_LOG_WARN("No attributes in nlMsg: %p at offset: %d", > >+ nlMsg, attrOffset); > >+ ret = FALSE; > >+ goto done; > >+ } > >+ > >+ NL_ATTR_FOR_EACH_UNSAFE(nla, left, NlMsgAt(nlMsg, attrOffset), > >+ totalAttrLen) > >+ { > >+ if (!NlAttrIsValid(nla, left)) { > >+ ret = FALSE; > >+ goto done; > >+ } > >+ > >+ UINT16 type = NlAttrType(nla); > >+ if (type < numPolicy && policy[type].type != NL_A_NO_ATTR) { > >+ /* Typecasting to keep the compiler happy */ > >+ const PNL_POLICY e = (const PNL_POLICY)(&policy[type]); > >+ if (!NlAttrValidate(nla, e)) { > >+ ret = FALSE; > >+ goto done; > >+ } > >+ } > >+ } > >+ > >+done: > >+ return ret; > >+} > >+ > >+/* > >+ > >*------------------------------------------------------------------------- > >--- > > * Parses the netlink message at a given offset (attrOffset) > > * as a series of attributes. A pointer to the attribute with type > > * 'type' is stored in attrs at index 'type'. policy is used to define > >the > >@@ -1052,20 +1105,13 @@ NlAttrParse(const PNL_MSG_HDR nlMsg, UINT32 > >attrOffset, > > PNL_ATTR attrs[], UINT32 numAttrs) > > { > > PNL_ATTR nla; > >- UINT32 left; > >+ INT left; > > UINT32 iter; > > BOOLEAN ret = FALSE; > > UINT32 numPolicyAttr = MIN(numPolicy, numAttrs); > > > > RtlZeroMemory(attrs, numAttrs * sizeof *attrs); > > > >- > >- /* There is nothing to parse */ > >- if (!(NlMsgAttrsLen(nlMsg))) { > >- ret = TRUE; > >- goto done; > >- } > >- > > if ((NlMsgSize(nlMsg) < attrOffset)) { > > OVS_LOG_WARN("No attributes in nlMsg: %p at offset: %d", > > nlMsg, attrOffset); > >diff --git a/datapath-windows/ovsext/Netlink/Netlink.h > >b/datapath-windows/ovsext/Netlink/Netlink.h > >index 8f6a5be..63164c7 100644 > >--- a/datapath-windows/ovsext/Netlink/Netlink.h > >+++ b/datapath-windows/ovsext/Netlink/Netlink.h > >@@ -72,6 +72,7 @@ typedef struct _NL_POLICY > > /* This macro is careful to check for attributes with bad lengths. */ > > #define NL_ATTR_FOR_EACH(ITER, LEFT, ATTRS, ATTRS_LEN) \ > > for ((ITER) = (ATTRS), (LEFT) = (ATTRS_LEN); \ > >+ ((INT)LEFT) >= (INT)NLA_ALIGN(sizeof(NL_ATTR)) && \ > > NlAttrIsValid(ITER, LEFT); \ > > (LEFT) -= NlAttrLenPad(ITER, LEFT), (ITER) = NlAttrNext(ITER)) > > > >@@ -80,7 +81,7 @@ typedef struct _NL_POLICY > > * already been validated (e.g. with NL_ATTR_FOR_EACH). */ > > #define NL_ATTR_FOR_EACH_UNSAFE(ITER, LEFT, ATTRS, ATTRS_LEN) \ > > for ((ITER) = (ATTRS), (LEFT) = (ATTRS_LEN); \ > >- (LEFT) > 0; \ > >+ ((INT)LEFT) >= (INT)NLA_ALIGN(sizeof(NL_ATTR)); \ > > (LEFT) -= NLA_ALIGN((ITER)->nlaLen), (ITER) = NlAttrNext(ITER)) > > > > #define NL_ATTR_GET_AS(NLA, TYPE) \ > >@@ -94,8 +95,9 @@ BOOLEAN NlFillNlHdr(PNL_BUFFER nlBuf, > > UINT16 nlmsgType, UINT16 nlmsgFlags, > > UINT32 nlmsgSeq, UINT32 nlmsgPid); > > > >-VOID NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR > msgOut, > >- UINT errorCode); > >+VOID NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR > msgError, > >+ UINT32 msgErrorLen, > >+ UINT errorCode, UINT32 *msgLen); > > > > /* Netlink message accessing the payload */ > > PVOID NlMsgAt(const PNL_MSG_HDR nlh, UINT32 offset); > >@@ -187,6 +189,11 @@ static __inline NlAttrIsLast(const PNL_ATTR nla, int > >rem) > > /* Netlink attribute validation */ > > BOOLEAN NlAttrValidate(const PNL_ATTR, const PNL_POLICY); > > > >+/* Netlink attribute stream validation */ > >+BOOLEAN NlValidateAllAttrs(const PNL_MSG_HDR nlMsg, UINT32 attrOffset, > >+ UINT32 totalAttrLen, > >+ const NL_POLICY policy[], const UINT32 numPolicy); > >+ > > /* Put APis */ > > BOOLEAN NlMsgPutNlHdr(PNL_BUFFER buf, PNL_MSG_HDR nlMsg); > > BOOLEAN NlMsgPutGenlHdr(PNL_BUFFER buf, PGENL_MSG_HDR genlMsg); > >diff --git a/datapath-windows/ovsext/User.c > >b/datapath-windows/ovsext/User.c > >index 33cbd89..bc010c7 100644 > >--- a/datapath-windows/ovsext/User.c > >+++ b/datapath-windows/ovsext/User.c > >@@ -345,8 +345,9 @@ > OvsNlExecuteCmdHandler(POVS_USER_PARAMS_CONTEXT > >usrParamsCtx, > > > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > >+ > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, > >replyLen); > > status = STATUS_SUCCESS; > > goto done; > > } > >diff --git a/datapath-windows/ovsext/Vport.c > >b/datapath-windows/ovsext/Vport.c > >index d04b12b..4299169 100644 > >--- a/datapath-windows/ovsext/Vport.c > >+++ b/datapath-windows/ovsext/Vport.c > >@@ -1729,9 +1729,9 @@ cleanup: > > if (nlError != NL_ERROR_SUCCESS) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > > > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > } > > > > return STATUS_SUCCESS; > >@@ -2088,9 +2088,9 @@ Cleanup: > > if (nlError != NL_ERROR_SUCCESS) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > > > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > } > > > > return STATUS_SUCCESS; > >@@ -2324,6 +2324,7 @@ Cleanup: > > if ((nlError != NL_ERROR_SUCCESS) && (nlError != NL_ERROR_PENDING)) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > > > > if (vport && vportAllocated == TRUE) { > > if (vportInitialized == TRUE) { > >@@ -2343,8 +2344,7 @@ Cleanup: > > OvsFreeMemoryWithTag(vport, OVS_VPORT_POOL_TAG); > > } > > > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > } > > > > return (status == STATUS_PENDING) ? STATUS_PENDING : > STATUS_SUCCESS; > >@@ -2452,9 +2452,9 @@ Cleanup: > > if (nlError != NL_ERROR_SUCCESS) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > > > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > } > > > > return STATUS_SUCCESS; > >@@ -2544,9 +2544,9 @@ Cleanup: > > if ((nlError != NL_ERROR_SUCCESS) && (nlError != NL_ERROR_PENDING)) { > > POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > > usrParamsCtx->outputBuffer; > >+ UINT32 msgErrorLen = usrParamsCtx->outputLength; > > > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > } > > > > return (status == STATUS_PENDING) ? STATUS_PENDING : > STATUS_SUCCESS; > >@@ -2579,10 +2579,11 @@ OvsTunnelVportPendingRemove(PVOID context, > > > > *replyLen = msgOut->nlMsg.nlmsgLen; > > } else { > >- POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)msgOut; > >+ POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > >+ tunnelContext->outputBuffer; > >+ UINT32 msgErrorLen = tunnelContext->outputLength; > > > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, > >replyLen); > > } > > } > > > >@@ -2721,12 +2722,13 @@ OvsTunnelVportPendingInit(PVOID context, > > } while (error); > > > > if (error) { > >- POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) msgOut; > >+ POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) > >+ tunnelContext->outputBuffer; > >+ UINT32 msgErrorLen = tunnelContext->outputLength; > > > > OvsCleanupVxlanTunnel(NULL, vport, NULL, NULL); > > OvsFreeMemory(vport); > > > >- NlBuildErrorMsg(msgIn, msgError, nlError); > >- *replyLen = msgError->nlMsg.nlmsgLen; > >+ NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); > > } > > } > >diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c > >index cad9490..32b0cc3 100644 > >--- a/lib/netlink-socket.c > >+++ b/lib/netlink-socket.c > >@@ -127,6 +127,7 @@ nl_sock_create(int protocol, struct nl_sock **sockp) > > sock = xmalloc(sizeof *sock); > > > > #ifdef _WIN32 > >+ sock->overlapped.hEvent = NULL; > > Good catch. > > > sock->handle = CreateFile(OVS_DEVICE_NAME_USER, > > GENERIC_READ | GENERIC_WRITE, > > FILE_SHARE_READ | FILE_SHARE_WRITE, > >@@ -1191,6 +1192,7 @@ pend_io_request(struct nl_sock *sock) > > > > ovs_header = ofpbuf_put_uninit(&request, sizeof *ovs_header); > > ovs_header->dp_ifindex = 0; > >+ nlmsg->nlmsg_len = request.size; > > Good catch. > > > > > if (!DeviceIoControl(sock->handle, OVS_IOCTL_WRITE, > > request.data, request.size,
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; break; @@ -903,7 +922,8 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject, * "artificial" or was copied from a previously validated 'ovsMsg'. */ if (devOp != OVS_READ_DEV_OP) { - status = ValidateNetlinkCmd(devOp, instance, ovsMsg, nlFamilyOps); + status = ValidateNetlinkCmd(devOp, instance, ovsMsg, + ovsMsgLength, nlFamilyOps); if (status != STATUS_SUCCESS) { goto done; } @@ -938,11 +958,18 @@ static NTSTATUS ValidateNetlinkCmd(UINT32 devOp, POVS_OPEN_INSTANCE instance, POVS_MESSAGE ovsMsg, + UINT32 ovsMsgLength, NETLINK_FAMILY *nlFamilyOps) { NTSTATUS status = STATUS_INVALID_PARAMETER; UINT16 i; + // We need to ensure we have enough data to process + if (NlMsgSize(&ovsMsg->nlMsg) > ovsMsgLength) { + status = STATUS_INVALID_PARAMETER; + goto done; + } + for (i = 0; i < nlFamilyOps->opsCount; i++) { if (nlFamilyOps->cmds[i].cmd == ovsMsg->genlMsg.cmd) { /* Validate if the command is valid for the device operation. */ @@ -977,6 +1004,14 @@ ValidateNetlinkCmd(UINT32 devOp, } } + // validate all NlAttrs + if (!NlValidateAllAttrs(&ovsMsg->nlMsg, sizeof(*ovsMsg), + NlMsgAttrsLen((PNL_MSG_HDR)ovsMsg), + NULL, 0)) { + status = STATUS_INVALID_PARAMETER; + goto done; + } + done: return status; } @@ -1028,6 +1063,7 @@ InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, POVS_MESSAGE msgIn = NULL; POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) usrParamsCtx->outputBuffer; + UINT32 msgErrorLen = usrParamsCtx->outputLength; if (usrParamsCtx->ovsMsg->genlMsg.cmd == OVS_CTRL_CMD_EVENT_NOTIFY || usrParamsCtx->ovsMsg->genlMsg.cmd == OVS_CTRL_CMD_READ_NOTIFY) { @@ -1043,8 +1079,7 @@ InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, ASSERT(msgIn); ASSERT(msgError); - NlBuildErrorMsg(msgIn, msgError, nlError); - *replyLen = msgError->nlMsg.nlmsgLen; + NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); } if (*replyLen != 0) { @@ -1416,9 +1451,9 @@ cleanup: if (nlError != NL_ERROR_SUCCESS) { POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) usrParamsCtx->outputBuffer; + UINT32 msgErrorLen = usrParamsCtx->outputLength; - NlBuildErrorMsg(msgIn, msgError, nlError); - *replyLen = msgError->nlMsg.nlmsgLen; + NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); } return STATUS_SUCCESS; diff --git a/datapath-windows/ovsext/Flow.c b/datapath-windows/ovsext/Flow.c index a49a60c..792b614 100644 --- a/datapath-windows/ovsext/Flow.c +++ b/datapath-windows/ovsext/Flow.c @@ -285,20 +285,10 @@ OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, goto done; } - /* Get all the top level Flow attributes */ - if ((NlAttrParse(nlMsgHdr, attrOffset, NlMsgAttrsLen(nlMsgHdr), - nlFlowPolicy, ARRAY_SIZE(nlFlowPolicy), - flowAttrs, ARRAY_SIZE(flowAttrs))) - != TRUE) { - OVS_LOG_ERROR("Attr Parsing failed for msg: %p", - nlMsgHdr); - rc = STATUS_INVALID_PARAMETER; - goto done; - } - - /* FLOW_DEL command w/o any key input is a flush case. */ + /* FLOW_DEL command w/o any key input is a flush case. + If we don't have any attr, we treat this as a flush command*/ if ((genlMsgHdr->cmd == OVS_FLOW_CMD_DEL) && - (!(flowAttrs[OVS_FLOW_ATTR_KEY]))) { + (!NlMsgAttrsLen(nlMsgHdr))) { rc = OvsFlushFlowIoctl(ovsHdr->dp_ifindex); @@ -323,6 +313,17 @@ OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, goto done; } + /* Get all the top level Flow attributes */ + if ((NlAttrParse(nlMsgHdr, attrOffset, NlMsgAttrsLen(nlMsgHdr), + nlFlowPolicy, ARRAY_SIZE(nlFlowPolicy), + flowAttrs, ARRAY_SIZE(flowAttrs))) + != TRUE) { + OVS_LOG_ERROR("Attr Parsing failed for msg: %p", + nlMsgHdr); + rc = STATUS_INVALID_PARAMETER; + goto done; + } + if (flowAttrs[OVS_FLOW_ATTR_PROBE]) { rc = OvsProbeSupportedFeature(msgIn, flowAttrs[OVS_FLOW_ATTR_KEY]); if (rc != STATUS_SUCCESS) { @@ -399,8 +400,9 @@ done: if (nlError != NL_ERROR_SUCCESS) { POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) usrParamsCtx->outputBuffer; - NlBuildErrorMsg(msgIn, msgError, nlError); - *replyLen = msgError->nlMsg.nlmsgLen; + UINT32 msgErrorLen = usrParamsCtx->outputLength; + + NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); rc = STATUS_SUCCESS; } @@ -568,8 +570,9 @@ done: if (nlError != NL_ERROR_SUCCESS) { POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) usrParamsCtx->outputBuffer; - NlBuildErrorMsg(msgIn, msgError, nlError); - *replyLen = msgError->nlMsg.nlmsgLen; + UINT32 msgErrorLen = usrParamsCtx->outputLength; + + NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); rc = STATUS_SUCCESS; } @@ -706,8 +709,9 @@ done: if (nlError != NL_ERROR_SUCCESS) { POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) usrParamsCtx->outputBuffer; - NlBuildErrorMsg(msgIn, msgError, nlError); - *replyLen = msgError->nlMsg.nlmsgLen; + UINT32 msgErrorLen = usrParamsCtx->outputLength; + + NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); rc = STATUS_SUCCESS; } diff --git a/datapath-windows/ovsext/Netlink/Netlink.c b/datapath-windows/ovsext/Netlink/Netlink.c index 27dcd4f..dc1e78c 100644 --- a/datapath-windows/ovsext/Netlink/Netlink.c +++ b/datapath-windows/ovsext/Netlink/Netlink.c @@ -108,12 +108,18 @@ NlFillNlHdr(PNL_BUFFER nlBuf, UINT16 nlmsgType, * --------------------------------------------------------------------------- */ VOID -NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR msgError, UINT errorCode) +NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR msgError, + UINT32 msgErrorLen, + UINT errorCode, UINT32 *msgLen) { NL_BUFFER nlBuffer; ASSERT(errorCode != NL_ERROR_PENDING); + if ((msgError == NULL) || (msgErrorLen < sizeof *msgError)) { + return; + } + NlBufInit(&nlBuffer, (PCHAR)msgError, sizeof *msgError); NlFillNlHdr(&nlBuffer, NLMSG_ERROR, 0, msgIn->nlMsg.nlmsgSeq, msgIn->nlMsg.nlmsgPid); @@ -121,6 +127,10 @@ NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR msgError, UINT errorCode) msgError->errorMsg.error = errorCode; msgError->errorMsg.nlMsg = msgIn->nlMsg; msgError->nlMsg.nlmsgLen = sizeof(OVS_MESSAGE_ERROR); + + if (NULL != msgLen) { + *msgLen = msgError->nlMsg.nlmsgLen; + } } /* @@ -1006,7 +1016,7 @@ NlAttrFind__(const PNL_ATTR attrs, UINT32 size, UINT16 type) { PNL_ATTR iter = NULL; PNL_ATTR ret = NULL; - UINT32 left; + INT left; NL_ATTR_FOR_EACH (iter, left, attrs, size) { if (NlAttrType(iter) == type) { @@ -1036,6 +1046,49 @@ NlAttrFindNested(const PNL_ATTR nla, UINT16 type) /* *---------------------------------------------------------------------------- + * Traverses all attributes in received buffer in order to insure all are valid + *---------------------------------------------------------------------------- + */ +BOOLEAN NlValidateAllAttrs(const PNL_MSG_HDR nlMsg, UINT32 attrOffset, + UINT32 totalAttrLen, + const NL_POLICY policy[], const UINT32 numPolicy) +{ + PNL_ATTR nla; + INT left; + BOOLEAN ret = TRUE; + + if ((NlMsgSize(nlMsg) < attrOffset)) { + OVS_LOG_WARN("No attributes in nlMsg: %p at offset: %d", + nlMsg, attrOffset); + ret = FALSE; + goto done; + } + + NL_ATTR_FOR_EACH_UNSAFE(nla, left, NlMsgAt(nlMsg, attrOffset), + totalAttrLen) + { + if (!NlAttrIsValid(nla, left)) { + ret = FALSE; + goto done; + } + + UINT16 type = NlAttrType(nla); + if (type < numPolicy && policy[type].type != NL_A_NO_ATTR) { + /* Typecasting to keep the compiler happy */ + const PNL_POLICY e = (const PNL_POLICY)(&policy[type]); + if (!NlAttrValidate(nla, e)) { + ret = FALSE; + goto done; + } + } + } + +done: + return ret; +} + +/* + *---------------------------------------------------------------------------- * Parses the netlink message at a given offset (attrOffset) * as a series of attributes. A pointer to the attribute with type * 'type' is stored in attrs at index 'type'. policy is used to define the @@ -1052,20 +1105,13 @@ NlAttrParse(const PNL_MSG_HDR nlMsg, UINT32 attrOffset, PNL_ATTR attrs[], UINT32 numAttrs) { PNL_ATTR nla; - UINT32 left; + INT left; UINT32 iter; BOOLEAN ret = FALSE; UINT32 numPolicyAttr = MIN(numPolicy, numAttrs); RtlZeroMemory(attrs, numAttrs * sizeof *attrs); - - /* There is nothing to parse */ - if (!(NlMsgAttrsLen(nlMsg))) { - ret = TRUE; - goto done; - } - if ((NlMsgSize(nlMsg) < attrOffset)) { OVS_LOG_WARN("No attributes in nlMsg: %p at offset: %d", nlMsg, attrOffset); diff --git a/datapath-windows/ovsext/Netlink/Netlink.h b/datapath-windows/ovsext/Netlink/Netlink.h index 8f6a5be..63164c7 100644 --- a/datapath-windows/ovsext/Netlink/Netlink.h +++ b/datapath-windows/ovsext/Netlink/Netlink.h @@ -72,6 +72,7 @@ typedef struct _NL_POLICY /* This macro is careful to check for attributes with bad lengths. */ #define NL_ATTR_FOR_EACH(ITER, LEFT, ATTRS, ATTRS_LEN) \ for ((ITER) = (ATTRS), (LEFT) = (ATTRS_LEN); \ + ((INT)LEFT) >= (INT)NLA_ALIGN(sizeof(NL_ATTR)) && \ NlAttrIsValid(ITER, LEFT); \ (LEFT) -= NlAttrLenPad(ITER, LEFT), (ITER) = NlAttrNext(ITER)) @@ -80,7 +81,7 @@ typedef struct _NL_POLICY * already been validated (e.g. with NL_ATTR_FOR_EACH). */ #define NL_ATTR_FOR_EACH_UNSAFE(ITER, LEFT, ATTRS, ATTRS_LEN) \ for ((ITER) = (ATTRS), (LEFT) = (ATTRS_LEN); \ - (LEFT) > 0; \ + ((INT)LEFT) >= (INT)NLA_ALIGN(sizeof(NL_ATTR)); \ (LEFT) -= NLA_ALIGN((ITER)->nlaLen), (ITER) = NlAttrNext(ITER)) #define NL_ATTR_GET_AS(NLA, TYPE) \ @@ -94,8 +95,9 @@ BOOLEAN NlFillNlHdr(PNL_BUFFER nlBuf, UINT16 nlmsgType, UINT16 nlmsgFlags, UINT32 nlmsgSeq, UINT32 nlmsgPid); -VOID NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR msgOut, - UINT errorCode); +VOID NlBuildErrorMsg(POVS_MESSAGE msgIn, POVS_MESSAGE_ERROR msgError, + UINT32 msgErrorLen, + UINT errorCode, UINT32 *msgLen); /* Netlink message accessing the payload */ PVOID NlMsgAt(const PNL_MSG_HDR nlh, UINT32 offset); @@ -187,6 +189,11 @@ static __inline NlAttrIsLast(const PNL_ATTR nla, int rem) /* Netlink attribute validation */ BOOLEAN NlAttrValidate(const PNL_ATTR, const PNL_POLICY); +/* Netlink attribute stream validation */ +BOOLEAN NlValidateAllAttrs(const PNL_MSG_HDR nlMsg, UINT32 attrOffset, + UINT32 totalAttrLen, + const NL_POLICY policy[], const UINT32 numPolicy); + /* Put APis */ BOOLEAN NlMsgPutNlHdr(PNL_BUFFER buf, PNL_MSG_HDR nlMsg); BOOLEAN NlMsgPutGenlHdr(PNL_BUFFER buf, PGENL_MSG_HDR genlMsg); diff --git a/datapath-windows/ovsext/User.c b/datapath-windows/ovsext/User.c index 33cbd89..bc010c7 100644 --- a/datapath-windows/ovsext/User.c +++ b/datapath-windows/ovsext/User.c @@ -345,8 +345,9 @@ OvsNlExecuteCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) usrParamsCtx->outputBuffer; - NlBuildErrorMsg(msgIn, msgError, nlError); - *replyLen = msgError->nlMsg.nlmsgLen; + UINT32 msgErrorLen = usrParamsCtx->outputLength; + + NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); status = STATUS_SUCCESS; goto done; } diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c index d04b12b..4299169 100644 --- a/datapath-windows/ovsext/Vport.c +++ b/datapath-windows/ovsext/Vport.c @@ -1729,9 +1729,9 @@ cleanup: if (nlError != NL_ERROR_SUCCESS) { POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) usrParamsCtx->outputBuffer; + UINT32 msgErrorLen = usrParamsCtx->outputLength; - NlBuildErrorMsg(msgIn, msgError, nlError); - *replyLen = msgError->nlMsg.nlmsgLen; + NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); } return STATUS_SUCCESS; @@ -2088,9 +2088,9 @@ Cleanup: if (nlError != NL_ERROR_SUCCESS) { POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) usrParamsCtx->outputBuffer; + UINT32 msgErrorLen = usrParamsCtx->outputLength; - NlBuildErrorMsg(msgIn, msgError, nlError); - *replyLen = msgError->nlMsg.nlmsgLen; + NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); } return STATUS_SUCCESS; @@ -2324,6 +2324,7 @@ Cleanup: if ((nlError != NL_ERROR_SUCCESS) && (nlError != NL_ERROR_PENDING)) { POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) usrParamsCtx->outputBuffer; + UINT32 msgErrorLen = usrParamsCtx->outputLength; if (vport && vportAllocated == TRUE) { if (vportInitialized == TRUE) { @@ -2343,8 +2344,7 @@ Cleanup: OvsFreeMemoryWithTag(vport, OVS_VPORT_POOL_TAG); } - NlBuildErrorMsg(msgIn, msgError, nlError); - *replyLen = msgError->nlMsg.nlmsgLen; + NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); } return (status == STATUS_PENDING) ? STATUS_PENDING : STATUS_SUCCESS; @@ -2452,9 +2452,9 @@ Cleanup: if (nlError != NL_ERROR_SUCCESS) { POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) usrParamsCtx->outputBuffer; + UINT32 msgErrorLen = usrParamsCtx->outputLength; - NlBuildErrorMsg(msgIn, msgError, nlError); - *replyLen = msgError->nlMsg.nlmsgLen; + NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); } return STATUS_SUCCESS; @@ -2544,9 +2544,9 @@ Cleanup: if ((nlError != NL_ERROR_SUCCESS) && (nlError != NL_ERROR_PENDING)) { POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) usrParamsCtx->outputBuffer; + UINT32 msgErrorLen = usrParamsCtx->outputLength; - NlBuildErrorMsg(msgIn, msgError, nlError); - *replyLen = msgError->nlMsg.nlmsgLen; + NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); } return (status == STATUS_PENDING) ? STATUS_PENDING : STATUS_SUCCESS; @@ -2579,10 +2579,11 @@ OvsTunnelVportPendingRemove(PVOID context, *replyLen = msgOut->nlMsg.nlmsgLen; } else { - POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)msgOut; + POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) + tunnelContext->outputBuffer; + UINT32 msgErrorLen = tunnelContext->outputLength; - NlBuildErrorMsg(msgIn, msgError, nlError); - *replyLen = msgError->nlMsg.nlmsgLen; + NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); } } @@ -2721,12 +2722,13 @@ OvsTunnelVportPendingInit(PVOID context, } while (error); if (error) { - POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) msgOut; + POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR) + tunnelContext->outputBuffer; + UINT32 msgErrorLen = tunnelContext->outputLength; OvsCleanupVxlanTunnel(NULL, vport, NULL, NULL); OvsFreeMemory(vport); - NlBuildErrorMsg(msgIn, msgError, nlError); - *replyLen = msgError->nlMsg.nlmsgLen; + NlBuildErrorMsg(msgIn, msgError, msgErrorLen, nlError, replyLen); } } diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c index cad9490..32b0cc3 100644 --- a/lib/netlink-socket.c +++ b/lib/netlink-socket.c @@ -127,6 +127,7 @@ nl_sock_create(int protocol, struct nl_sock **sockp) sock = xmalloc(sizeof *sock); #ifdef _WIN32 + sock->overlapped.hEvent = NULL; sock->handle = CreateFile(OVS_DEVICE_NAME_USER, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, @@ -1191,6 +1192,7 @@ pend_io_request(struct nl_sock *sock) ovs_header = ofpbuf_put_uninit(&request, sizeof *ovs_header); ovs_header->dp_ifindex = 0; + nlmsg->nlmsg_len = request.size; if (!DeviceIoControl(sock->handle, OVS_IOCTL_WRITE, request.data, request.size,