| Message ID | 1461878018-64352-1-git-send-email-joe@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On 28 April 2016 at 14:13, Joe Stringer <joe@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 <joe@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 >
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(). > This one is OK. because when ofpact_finish_BUNDLE(ofpacts, &bundle); the address 'bundle' points to will be updated if reallocation happens. So we need the second argument as a way to update the stale pointer. This is mentioned here: https://patchwork.ozlabs.org/patch/593747/ http://openvswitch.org/pipermail/dev/2016-April/069508.html Regards, William
On 29 April 2016 at 09:53, William Tu <u9012063@gmail.com> wrote: > 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(). > > > This one is OK. because when > ofpact_finish_BUNDLE(ofpacts, &bundle); > > the address 'bundle' points to will be updated if reallocation happens. So > we need the second argument as a way to update the stale pointer. > > This is mentioned here: > https://patchwork.ozlabs.org/patch/593747/ > http://openvswitch.org/pipermail/dev/2016-April/069508.html I think you are right. Thanks for pointing out those patches.
On 28 April 2016 at 18:29, Ansis Atteka <ansisatteka@gmail.com> wrote: > On 28 April 2016 at 14:13, Joe Stringer <joe@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 <joe@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.
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); 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
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 <joe@ovn.org> --- lib/ofp-actions.c | 1 + tests/ofproto-dpif.at | 10 ++++++++++ 2 files changed, 11 insertions(+)