Patchwork openvswitch: potential NULL deref in sample()

login
register
mail settings
Submitter Dan Carpenter
Date July 23, 2012, 7:46 a.m.
Message ID <20120723074628.GA30892@elgon.mountain>
Download mbox | patch
Permalink /patch/172564/
State Accepted
Delegated to: David Miller
Headers show

Comments

Dan Carpenter - July 23, 2012, 7:46 a.m.
If there is no OVS_SAMPLE_ATTR_ACTIONS set then "acts_list" is NULL and
it leads to a NULL dereference when we call nla_len(acts_list).  This
is a static checker fix, not something I have seen in testing.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This applies to Linus's tree.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - July 23, 2012, 8 a.m.
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Mon, 23 Jul 2012 10:46:28 +0300

> If there is no OVS_SAMPLE_ATTR_ACTIONS set then "acts_list" is NULL and
> it leads to a NULL dereference when we call nla_len(acts_list).  This
> is a static checker fix, not something I have seen in testing.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied, thanks Dan.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jesse Gross - July 23, 2012, 7:54 p.m.
On Mon, Jul 23, 2012 at 12:46 AM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> If there is no OVS_SAMPLE_ATTR_ACTIONS set then "acts_list" is NULL and
> it leads to a NULL dereference when we call nla_len(acts_list).  This
> is a static checker fix, not something I have seen in testing.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

This can never happen in practice because the action list is validated
at the time that userspace installs the flow.  There are plenty of
things in this category that would appear to be unsafe because we'd
much rather do sanity checking on a per-flow basis rather than
per-packet.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 48badff..c2351d6 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -325,6 +325,9 @@  static int sample(struct datapath *dp, struct sk_buff *skb,
 		}
 	}
 
+	if (!acts_list)
+		return 0;
+
 	return do_execute_actions(dp, skb, nla_data(acts_list),
 						 nla_len(acts_list), true);
 }