[ovs-dev] [PATCH] nx-match: Only store significant bytes to stack.
Ben Pfaff
blp at ovn.org
Fri Jan 6 00:48:25 UTC 2017
On Thu, Jan 05, 2017 at 04:03:17PM -0800, Jarno Rajahalme wrote:
>
> > On Jan 4, 2017, at 11:03 PM, Ben Pfaff <blp at ovn.org> wrote:
> >
> > On Wed, Jan 04, 2017 at 07:21:44PM -0800, Jarno Rajahalme wrote:
> >>
> >>> On Dec 21, 2016, at 2:36 PM, Ben Pfaff <blp at ovn.org> wrote:
> >>>
> >>> On Wed, Dec 07, 2016 at 04:49:00PM -0800, Jarno Rajahalme wrote:
> >>> I'd be more comfortable if nx_stack_pop() had assertions to check for
> >>> underflow.
> >>
> >> I don’t think OVS should assert fail if controller issues one pop
> >> too many? Do you mean that current users of nx_stack_pop() do not
> >> check for NULL return? I had a look and think that setting “*bytes”
> >> to zero when returning NULL should be enough for all users.
> >
> > It appears to me that if stack->size is greater than 0 but less than the
> > number of bytes indicated by its last byte, then it will corrupt the
> > ofpbuf size and that this will later cause some kind of failure that
> > will be harder to debug than an assertion failure.
> >
>
> OK, now i got it. This is just to guard against (future) bugs in OVS itself.
Yes.
> >>> In ofputil_decode_packet_in_private(), it's probably worth checking the
> >>> format of the stack we pull from the payload, since a badly formatted
> >>> stack can segfault us (if we leave out assertions) or assert-fail us (if
> >>> we include them).
> >>>
> >>
> >> What do you mean with badly formatted stack? Zero-sized property? IMO
> >> even that would be properly pushed and popped from the stack, storing
> >> only the length (of zero) in the stack.
> >
> > I mean that if the property contains, for example, a single byte with
> > value 0xff, then it's badly formatted because we can pop off a length
> > (255) but then popping off that number of bytes will underflow.
>
> I did not change the encoding of the stack as properties, so each
> value in the stack is still encoded as a separate property, where the
> (aligned) value length is used as the property length.
I guess I forgot that.
Thanks, that's fine then.
> ofpprop_pull() does the length checking for the properties and the
> current code in ofputil_decode_packet_in_private() assert fails on any
> error, which is not good, as a controller bug would crash OVS?
That's bad. Maybe the fix is as simple as this, though.
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 156d8d2..421b9d7 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -4061,7 +4061,9 @@ ofputil_decode_packet_in_private(const struct ofp_header *oh, bool loose,
uint64_t type;
error = ofpprop_pull(&continuation, &payload, &type);
- ovs_assert(!error);
+ if (error) {
+ break;
+ }
switch (type) {
case NXCPT_BRIDGE:
@@ -4124,7 +4126,7 @@ ofputil_decode_packet_in_private(const struct ofp_header *oh, bool loose,
ofputil_packet_in_private_destroy(pin);
}
- return 0;
+ return error;
}
/* Frees data in 'pin' that is dynamically allocated by
More information about the dev
mailing list