[ovs-dev] [PATCH] ofpbuf: Fix use-after-free in bundle parse.

[William Tu]

Hi Joe,

Thanks for your reply. The reason is that line 181:
    ofpact_finish(ofpacts, &bundle->ofpact);
https://github.com/openvswitch/ovs/blob/34abaa3deaa430ca0b50453865d2e042a5132165/lib/bundle.c#L181

Could also call into ofpbuf_put_zero, which frees the memory pointed by
bundle.

Regards,
William

On Fri, Mar 4, 2016 at 6:12 PM, Joe Stringer <joe at ovn.org> wrote:

> On 4 March 2016 at 18:00, William Tu <u9012063 at gmail.com> wrote:
> > Address pointed by bundle could be obsolete/free'd when
> > realloc, called from ofpbuf_put_zero(), returns new address.
> > Reported by Valgrind 367: ovs-ofctl parse-flows (NXM)
> >
> > Invalid write of size 4
> >     bundle_parse__ (bundle.c:200)
> >     bundle_parse_load (bundle.c:272)
> >     parse_bundle_load (ofp-actions.c:1324)
> >     ofpacts_parse__ (ofp-actions.c:7484)
> >     ofpacts_parse (ofp-actions.c:7540)
> >     ofpacts_parse_copy (ofp-actions.c:7558)
> >     parse_ofp_str__ (ofp-parse.c:491)
> >     parse_ofp_str (ofp-parse.c:544)
> >     parse_ofp_flow_mod_str (ofp-parse.c:870)
> >
> > Address 0x7a4e96c is 12 bytes inside a block of size 64 free'd
> >     free (vg_replace_malloc.c:530)
> >     ofpbuf_resize__ (ofpbuf.c:246) (purposely add to force using new buf)
> >     ofpbuf_put_zeros (ofpbuf.c:375)
> >     bundle_parse__ (bundle.c:181)
> >     bundle_parse_load (bundle.c:272)
> >     parse_bundle_load (ofp-actions.c:1324)
> >     ofpacts_parse__ (ofp-actions.c:7484)
> >     ofpacts_parse (ofp-actions.c:7540)
> >     ofpacts_parse_copy (ofp-actions.c:7558)
> >
> > Signed-off-by: William Tu <u9012063 at gmail.com>
> > ---
> >  lib/bundle.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/bundle.c b/lib/bundle.c
> > index 871a724..1451e92 100644
> > --- a/lib/bundle.c
> > +++ b/lib/bundle.c
> > @@ -180,6 +180,7 @@ bundle_parse__(const char *s, char **save_ptr,
> >      }
> >      ofpact_finish(ofpacts, &bundle->ofpact);
> >
> > +    bundle = ofpacts->header;
> >      bundle->basis = atoi(basis);
>
> I don't understand how this would trigger (or how this would fix the
> issue); the same line is also done directly after ofpbuf_put() inside
> the loop above:
>
> https://github.com/openvswitch/ovs/blob/34abaa3deaa430ca0b50453865d2e042a5132165/lib/bundle.c#L178
>
> Can you explain it?
>