From 19b58f3cbcb3191432eefba3a504376399cc07a7 Mon Sep 17 00:00:00 2001 From: Joe Stringer Date: Thu, 3 Mar 2016 21:22:49 +1300 Subject: [PATCH] ofp-actions: Fix use-after-free in bundle action. If the actions list in an incoming flow mod is long enough, and there is a bundle() action with 3 or more slaves, then it is possible for a reallocation to occur after placing the ofpact_bundle into the ofpacts buffer, while slave ports into the buffer. If the memory freed by this reallocation is then passed to another thread, then that thread may modify the value that bundle->n_slaves points to. If this occurs quickly enough before the main thread finishes copying all of the slaves, then the iteration may continue beyond the originally intended number of slaves, copying (and swapping) an undetermined number of 2-byte chunks from the openflow message. Finally, the length of the ofpact will be updated based on how much data was written to the buffer, which may be significantly longer than intended. In many cases, the freed memory may not be allocated to another thread and be left untouched. In some milder bug cases, this will lead to 'bundle' actions using more memory than required. In more serious cases, this length may then exceed the maximum length of an OpenFlow action, which is then stored (truncated) into the 16-bit length field in the ofpact header. Later execution of ofpacts_verify() would then use this length to iterate through the ofpacts, and may dereference memory in unintended ways, causing crashes or infinite loops by attempting to parse/validate arbitrary data as ofpact objects. Fix the issue by updating 'bundle' within the iteration, immediately after (potentially) expanding the bundle. Thanks to Jarno Rajahalme for his keen pair of eyes on finding this issue. VMWare-BZ: #1614715 Fixes: f25d0cf3c366 ("Introduce ofpacts, an abstraction of OpenFlow actions.") Signed-off-by: Joe Stringer Acked-by: Jarno Rajahalme --- lib/ofp-actions.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index ae961f6cc..fe1424f13 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -1253,9 +1253,9 @@ decode_bundle(bool load, const struct nx_action_bundle *nab, for (i = 0; i < bundle->n_slaves; i++) { uint16_t ofp_port = ntohs(((ovs_be16 *)(nab + 1))[i]); ofpbuf_put(ofpacts, &ofp_port, sizeof ofp_port); + bundle = ofpacts->header; } - bundle = ofpacts->header; ofpact_finish(ofpacts, &bundle->ofpact); if (!error) { -- 2.20.1