On 28 April 2016 at 18:29, Ansis Atteka <ansisatt...@gmail.com> wrote: > 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.
Thanks, I applied this to master. It didn't affect any existing releases, as it was introduced by 2bd318dec242 ("ofp-actions: Make composing actions harder to screw up."). > 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. I guess there's a little more 'magic' to the way that ofpact_finish() works in regards to the latter argument which is easy to miss since it's hidden in some macros. The documentation for ofpact_finish_<ENUM> in include/openvswitch/ofp-actions.h describes this behaviour. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev