| Message ID | 1456993371-9902-2-git-send-email-joe@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On 3 March 2016 at 00:22, Joe Stringer <joe@ovn.org> wrote: > 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@ovn.org> With Jarno's ack from patch #4, I pushed this to master and branches 2.1-2.5.
Hi Joe, On Thu, Mar 3, 2016 at 12:22 AM, Joe Stringer <joe@ovn.org> wrote: > 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 > 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); > > I think we need to add another bundle = ofpacts->header; right after ofpact_finish(ofpacts, &bundle->ofpact); Since ofpact_finish could potentially call ofpbuf_put_zero and realloc buffer to new address. Regards, William > if (!error) { > -- > 2.1.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
On 5 March 2016 at 09:13, William Tu <u9012063@gmail.com> wrote: > Hi Joe, > > On Thu, Mar 3, 2016 at 12:22 AM, Joe Stringer <joe@ovn.org> wrote: >> >> 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 >> 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); >> > > I think we need to add another > bundle = ofpacts->header; > right after > ofpact_finish(ofpacts, &bundle->ofpact); > > Since ofpact_finish could potentially call ofpbuf_put_zero and realloc > buffer to new address. I agree, though fortunately this only affects master. There's also another case in CT action. I can follow up on this, thanks for pointing it out.
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) {
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@ovn.org> --- lib/ofp-actions.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)