On Fri, Jan 06, 2017 at 01:10:14PM -0800, Jarno Rajahalme wrote:
>
> > On Jan 5, 2017, at 9:06 PM, Ben Pfaff <b...@ovn.org> wrote:
> >
> > On Thu, Jan 05, 2017 at 05:45:29PM -0800, Jarno Rajahalme wrote:
> >>
> >>> On Jan 5, 2017, at 4:48 PM, Ben Pfaff <b...@ovn.org> wrote:
> >>>
> >>> On Thu, Jan 05, 2017 at 04:03:17PM -0800, Jarno Rajahalme wrote:
> >>>>
> >>>>> On Jan 4, 2017, at 11:03 PM, Ben Pfaff <b...@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 <b...@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
> >>>
> >>
> >> I folded this in to v3.
> >
> > This bug is in 2.6, isn't it? If so then we should fix it in a separate
> > patch, for backporting purposes.
>
> Right, I’ll separate it out. What is the most proper way to attribute it to
> you? "Suggested-by”, or something else?
That's fine.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
