From: Joe Stringer Date: Thu, 28 Apr 2016 21:13:38 +0000 (-0700) Subject: ofp-actions: Fix use-after-free in decode_NOTE. X-Git-Url: http://git.cascardo.eti.br/?a=commitdiff_plain;h=ace39a6f63d4c28344ca0e2a2c4233ddbc16b07c;p=cascardo%2Fovs.git ofp-actions: Fix use-after-free in decode_NOTE. 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 Acked-by: Ansis Atteka --- diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index 39b6fbca5..10ef3ea80 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); return 0; diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 9ac2e2aff..e7445ac6e 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