On 28 April 2016 at 14:13, Joe Stringer <j...@ovn.org> wrote: > When decoding the 'note' action, variable-length data could be pushed to > a buffer immediately prior to calling ofpact_finish_NOTE(). The > ofpbuf_put() could cause reallocation, in which case the finish call > could access freed memory. Fix the issue by updating the local pointer > before passing it to ofpact_finish_NOTE(). > > If the memory was reused, it may trigger an assert in ofpact_finish(): > > assertion ofpact == ofpacts->header failed in ofpact_finish() > > With the included test, make check-valgrind reports: > > Invalid read of size 1 > at 0x500A9F: ofpact_finish_NOTE (ofp-actions.h:988) > by 0x4FE5C1: decode_NXAST_RAW_NOTE (ofp-actions.c:4557) > by 0x4FBC05: ofpact_decode (ofp-actions.inc2:3831) > by 0x4F7E87: ofpacts_decode (ofp-actions.c:5780) > by 0x4F709F: ofpacts_pull_openflow_actions__ (ofp-actions.c:5817) > by 0x4F7856: ofpacts_pull_openflow_instructions (ofp-actions.c:6397) > by 0x52CFF5: ofputil_decode_flow_mod (ofp-util.c:1727) > by 0x5227A9: ofp_print_flow_mod (ofp-print.c:789) > by 0x520823: ofp_to_string__ (ofp-print.c:3235) > by 0x5204F6: ofp_to_string (ofp-print.c:3468) > by 0x5925C8: do_recv (vconn.c:644) > by 0x592372: vconn_recv (vconn.c:598) > by 0x565CEA: rconn_recv (rconn.c:703) > by 0x46CB62: ofconn_run (connmgr.c:1367) > by 0x46C7AD: connmgr_run (connmgr.c:320) > by 0x4224A9: ofproto_run (ofproto.c:1763) > by 0x407C0D: bridge_run__ (bridge.c:2888) > by 0x40767A: bridge_run (bridge.c:2943) > by 0x4161B7: main (ovs-vswitchd.c:120) > > Signed-off-by: Joe Stringer <j...@ovn.org> > --- > lib/ofp-actions.c | 1 + > tests/ofproto-dpif.at | 10 ++++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c > index 39b6fbca531e..10ef3ea808fd 100644 > --- a/lib/ofp-actions.c > +++ b/lib/ofp-actions.c > @@ -4554,6 +4554,7 @@ decode_NXAST_RAW_NOTE(const struct nx_action_note *nan, > note = ofpact_put_NOTE(out); > note->length = length; > ofpbuf_put(out, nan->note, length); > + note = out->header; > ofpact_finish_NOTE(out, ¬e);
What you have looks good to me. I had to stop and think a little bit about the ofpact_finish() function's API. It gives freedom to its caller to specify whatever it wants as second 'ofpact' argument. However, at the end of the day ofpact_finish() asserts if second argument value does not match to the first argument's "->header" value. I think that in theory this function really needs only the first argument to do its job, because second argument is really implied from the first argument. BTW, there are other bugs in similar nature in this same file, for example, the 'bundle' variable is still referenced after invoking ofpact_finish_BUNDLE(). > > return 0; > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 9ac2e2affaeb..e7445ac6e817 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -782,6 +782,16 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > OVS_VSWITCHD_STOP > AT_CLEANUP > > +dnl As of OVS-2.5, a note action after 4 set_field actions are likely to > +dnl trigger ofpbuf reallocation during decode (~1KB into ofpacts buffer). > +dnl Using `make check-valgrind' here checks for use-after-free in this > +dnl codepath. > +AT_SETUP([ofproto-dpif - note action deep inside ofpacts]) > +OVS_VSWITCHD_START > +AT_CHECK([ovs-ofctl add-flow br0 > 'actions=set_field:0x1->metadata,set_field:0x2->metadata,set_field:0x3->metadata,set_field:0x4->metadata,note:00000000000000000000000000000000,note:00000000000000000000000000000000']) > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > AT_SETUP([ofproto-dpif - output, OFPP_NONE ingress port]) > OVS_VSWITCHD_START > add_of_ports br0 1 2 > -- > 2.1.4 > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
