[ovs-dev] [PATCH 2/4] ofp-actions: Fix use-after-free in bundle action.

[Joe Stringer]

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 appending all of the slaves into the buffer. If the buffer
freed by this reallocation is then passed to another thread, then it 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 action will be
updated based on how much data was written to the buffer, which may be
significantly longer than intended.

In the milder 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.

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 <joe at ovn.org>
---
 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 ae961f6cc5bb..fe1424f137a1 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.1.4