Re: [ovs-dev] [PATCH] odp-util: extend usage of limit for parse functions

[Ben Pfaff]

On Wed, May 08, 2019 at 09:22:54AM -0700, Toms Atteka wrote:
> This fixes stack overflow issues for odp_actions_from_string.
> As well moved depth check inside parse_odp_key_mask_attr to beginning.
> Added some missing depth reductions.
> 
> Basic manual testing was performed.
> 
> Reported-by:
> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=13808
> Signed-off-by: Toms Atteka 

Thanks for fixing this!

Above, as minor points, please don't word-wrap the tag across two lines,
and please use Reported-at instead of Reported-by for a URL reference.

In the patch itself, this approach requires every "return" within
parse_odp_action() to decrement the depth counter.  This is error-prone.
I would prefer to rename parse_odp_action() to parse_odp_action__() and
make a new parse_odp_action() as a wrapper around parse_odp_action__()
that just increments and decrements the depth.  Another approach that
avoids so many decrements would also be OK.

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] odp-util: extend usage of limit for parse functions

[Toms Atteka]

This fixes stack overflow issues for odp_actions_from_string.
As well moved depth check inside parse_odp_key_mask_attr to beginning.
Added some missing depth reductions.

Basic manual testing was performed.

Reported-by:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=13808
Signed-off-by: Toms Atteka 
---
 lib/odp-util.c | 89 --
 1 file changed, 65 insertions(+), 24 deletions(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 1b2347d6f..fc85c6123 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -90,7 +90,7 @@ static void format_u128(struct ds *d, const 
ovs_32aligned_u128 *key,
 const ovs_32aligned_u128 *mask, bool verbose);
 static int scan_u128(const char *s, ovs_u128 *value, ovs_u128 *mask);
 
-static int parse_odp_action(const char *s, const struct simap *port_names,
+static int parse_odp_action(struct parse_odp_context *context, const char *s,
 struct ofpbuf *actions);
 
 /* Returns one the following for the action with the given OVS_ACTION_ATTR_*
@@ -2183,7 +2183,7 @@ out:
 }
 
 static int
-parse_action_list(const char *s, const struct simap *port_names,
+parse_action_list(struct parse_odp_context *context, const char *s,
   struct ofpbuf *actions)
 {
 int n = 0;
@@ -2195,7 +2195,7 @@ parse_action_list(const char *s, const struct simap 
*port_names,
 if (s[n] == ')') {
 break;
 }
-retval = parse_odp_action(s + n, port_names, actions);
+retval = parse_odp_action(context, s + n, actions);
 if (retval < 0) {
 return retval;
 }
@@ -2210,15 +2210,21 @@ parse_action_list(const char *s, const struct simap 
*port_names,
 }
 
 static int
-parse_odp_action(const char *s, const struct simap *port_names,
+parse_odp_action(struct parse_odp_context *context, const char *s,
  struct ofpbuf *actions)
 {
+if (context->depth + 1 == MAX_ODP_NESTED) {
+return -EINVAL;
+}
+context->depth++;
+
 {
 uint32_t port;
 int n;
 
 if (ovs_scan(s, "%"SCNi32"%n", &port, &n)) {
 nl_msg_put_u32(actions, OVS_ACTION_ATTR_OUTPUT, port);
+context->depth--;
 return n;
 }
 }
@@ -2233,17 +2239,19 @@ parse_odp_action(const char *s, const struct simap 
*port_names,
 trunc = nl_msg_put_unspec_uninit(actions,
  OVS_ACTION_ATTR_TRUNC, sizeof *trunc);
 trunc->max_len = max_len;
+context->depth--;
 return n;
 }
 }
 
-if (port_names) {
+if (context->port_names) {
 int len = strcspn(s, delimiters);
 struct simap_node *node;
 
-node = simap_find_len(port_names, s, len);
+node = simap_find_len(context->port_names, s, len);
 if (node) {
 nl_msg_put_u32(actions, OVS_ACTION_ATTR_OUTPUT, node->data);
+context->depth--;
 return len;
 }
 }
@@ -2254,11 +2262,13 @@ parse_odp_action(const char *s, const struct simap 
*port_names,
 
 if (ovs_scan(s, "recirc(%"PRIu32")%n", &recirc_id, &n)) {
 nl_msg_put_u32(actions, OVS_ACTION_ATTR_RECIRC, recirc_id);
+context->depth--;
 return n;
 }
 }
 
 if (!strncmp(s, "userspace(", 10)) {
+context->depth--;
 return parse_odp_userspace_action(s, actions);
 }
 
@@ -2269,18 +2279,17 @@ parse_odp_action(const char *s, const struct simap 
*port_names,
 struct ofpbuf maskbuf = OFPBUF_STUB_INITIALIZER(mask);
 struct nlattr *nested, *key;
 size_t size;
-struct parse_odp_context context = (struct parse_odp_context) {
-.port_names = port_names,
-};
 
 start_ofs = nl_msg_start_nested(actions, OVS_ACTION_ATTR_SET);
-retval = parse_odp_key_mask_attr(&context, s + 4, actions, &maskbuf);
+retval = parse_odp_key_mask_attr(context, s + 4, actions, &maskbuf);
 if (retval < 0) {
 ofpbuf_uninit(&maskbuf);
+context->depth--;
 return retval;
 }
 if (s[retval + 4] != ')') {
 ofpbuf_uninit(&maskbuf);
+context->depth--;
 return -EINVAL;
 }
 
@@ -2311,6 +2320,7 @@ parse_odp_action(const char *s, const struct simap 
*port_names,
 ofpbuf_uninit(&maskbuf);
 
 nl_msg_end_nested(actions, start_ofs);
+context->depth--;
 return retval + 5;
 }
 
@@ -2330,6 +2340,7 @@ parse_odp_action(const char *s, const struct simap 
*port_names,
 &tpid, &vid, &pcp, &cfi, &n)) {
 if ((vid & ~(VLAN_VID_MASK >> VLAN_VID_SHIFT)) != 0
 || (pcp & ~(VLAN_PCP_MASK >> VLAN_PCP_SHIFT)) != 0) {
+context->depth--;
 return -EINVAL;
 }
 push.vlan_tpid = htons(tpid);
@@ -2339,12 +2350,14 @@ parse_odp_action(