Commit 0bdf8e23 authored by Joe Stringer's avatar Joe Stringer

ofp-actions: Fix use-after-free in bundle action.

Upstream commit 19b58f3c.

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: f25d0cf3 ("Introduce ofpacts, an abstraction of OpenFlow actions.")
Signed-off-by: default avatarJoe Stringer <joe@ovn.org>
Acked-by: default avatarJarno Rajahalme <jarno@ovn.org>
parent 03e87149
......@@ -967,9 +967,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_update_len(ofpacts, &bundle->ofpact);
if (!error) {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment