diff mbox

[ovs-dev] nx-match: Fix use-after-free.

Message ID CALDO+SZFZLZa6zz32kg2HtqnAOtY+0iRNb3opSx4-2ACJ3Za4g@mail.gmail.com
State Superseded
Headers show

Commit Message

William Tu March 7, 2016, 7:11 p.m. UTC 
Hi Jarno,

Thanks for the feedback. I forgot to mention that this issue is found by
changing the ofpbuf code to make each put reallocate the memory. I patched
the code with:


Which forces to increase the headroom and causes allocating new buffer.
Then I re-run the testsuite with valgrind testcase, and a couple of related
issues show up as "Invalid write...".
Another idea is to modify glibc so that realloc will always use newly
allocated memory. In this way we don't need to change ofpbuf code. I'm
still experimenting this approach.

Regards,
William

On Mon, Mar 7, 2016 at 10:46 AM, Jarno Rajahalme <jarno@ovn.org> wrote:

> It might be super slow, but how about running the test suite with valgrind
> and ofpbuf code changed so that each put reallocates the memory? That way
> we would not have to be lucky about the timing/placement of reallocations
> to find these bugs?
>
>   Jarno
>
> > On Mar 4, 2016, at 5:35 PM, William Tu <u9012063@gmail.com> wrote:
> >
> > Address pointed by header_ptr might be free'd due to realloc
> > happened at ofpbuf_put_uninit() and ofpbuf_put_hex(). Reported
> > by valgrind 379: check TCP flags expression in OXM and NXM.
> >
> > Invalid write of size 4
> >    nx_match_from_string_raw (nx-match.c:1510)
> >    nx_match_from_string (nx-match.c:1538)
> >    ofctl_parse_nxm__ (ovs-ofctl.c:3325)
> >    ovs_cmdl_run_command (command-line.c:121)
> >    main (ovs-ofctl.c:137)
> >
> > Address 0x7a2cc40 is 0 bytes inside a block of size 64 free'd
> >    free (vg_replace_malloc.c:530)
> >    ofpbuf_resize__ (ofpbuf.c:246)
> >    ofpbuf_put (ofpbuf.c:386)
> >    ofpbuf_put_hex (ofpbuf.c:414)
> >    nx_match_from_string_raw (nx-match.c:1488)
> >    nx_match_from_string (nx-match.c:1538)
> >    ofctl_parse_nxm__ (ovs-ofctl.c:3325)
> >
> > Signed-off-by: William Tu <u9012063@gmail.com>
> > ---
> > lib/nx-match.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/lib/nx-match.c b/lib/nx-match.c
> > index 4999b1a..2615f8c 100644
> > --- a/lib/nx-match.c
> > +++ b/lib/nx-match.c
> > @@ -1470,6 +1470,7 @@ nx_match_from_string_raw(const char *s, struct
> ofpbuf *b)
> >         ovs_be64 *header_ptr;
> >         int name_len;
> >         size_t n;
> > +        ptrdiff_t header_ptr_offset;
> >
> >         name = s;
> >         name_len = strcspn(s, "(");
> > @@ -1485,6 +1486,7 @@ nx_match_from_string_raw(const char *s, struct
> ofpbuf *b)
> >         s += name_len + 1;
> >
> >         header_ptr = ofpbuf_put_uninit(b, nxm_header_len(header));
> > +        header_ptr_offset = (char *)header_ptr - (char *)b->data;
> >         s = ofpbuf_put_hex(b, s, &n);
> >         if (n != nxm_field_bytes(header)) {
> >             const struct mf_field *field = mf_from_oxm_header(header);
> > @@ -1507,6 +1509,10 @@ nx_match_from_string_raw(const char *s, struct
> ofpbuf *b)
> >             }
> >         }
> >         nw_header = htonll(header);
> > +
> > +        /* header_ptr might be free'd due to
> > +         * ofpbuf_put_uninit() and ofpbuf_put_hex(). */
> > +        header_ptr = (ovs_be64 *)((char *)b->data + header_ptr_offset);
> >         memcpy(header_ptr, &nw_header, nxm_header_len(header));
> >
> >         if (nxm_hasmask(header)) {
> > --
> > 2.5.0
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
>
diff mbox

Patch

--- a/lib/ofpbuf.c
+++ b/lib/ofpbuf.c
@@ -383,6 +383,7 @@  ofpbuf_put_zeros(struct ofpbuf *b, size_t size)
 void *
 ofpbuf_put(struct ofpbuf *b, const void *p, size_t size)
 {
+    ofpbuf_resize__(b, ofpbuf_headroom(b)+64, ofpbuf_tailroom(b));
     void *dst = ofpbuf_put_uninit(b, size);
     memcpy(dst, p, size);
     return dst;