vconn_add_bundle_error() was keeping at most 64 bytes of an OpenFlow error message, then it was passing it to ofperr_decode_msg(), which assumed that the full message was available. This led to a buffer overread. There's no good reason why it was only keeping the first 64 bytes, so this commit changes it to keep the whole error message, sidestepping the problem.
struct vconn_bundle_error only existed for this special case, so remove it in favor of a chain of ofpbufs. Found via gcc's address sanitizer. Reported-by: Lance Richardson <[email protected]> CC: Jarno Rajahalme <[email protected]> Fixes: 506c1ddb3404 ("vconn: Better bundle error management.") Signed-off-by: Ben Pfaff <[email protected]> --- include/openvswitch/vconn.h | 12 ------------ lib/vconn.c | 25 ++++++++----------------- utilities/ovs-ofctl.c | 10 ++++++---- 3 files changed, 14 insertions(+), 33 deletions(-) diff --git a/include/openvswitch/vconn.h b/include/openvswitch/vconn.h index 40ca9edfe868..90f9bad2c1c9 100644 --- a/include/openvswitch/vconn.h +++ b/include/openvswitch/vconn.h @@ -61,18 +61,6 @@ int vconn_dump_flows(struct vconn *, const struct ofputil_flow_stats_request *, enum ofputil_protocol, struct ofputil_flow_stats **fsesp, size_t *n_fsesp); -/* Bundle errors must be free()d by the caller. */ -struct vconn_bundle_error { - struct ovs_list list_node; - - /* OpenFlow header and some of the message contents for error reporting. */ - union { - struct ofp_header ofp_msg; - uint8_t ofp_msg_data[64]; - }; -}; - -/* Bundle errors must be free()d by the caller. */ int vconn_bundle_transact(struct vconn *, struct ovs_list *requests, uint16_t bundle_flags, struct ovs_list *errors); diff --git a/lib/vconn.c b/lib/vconn.c index 6997eaa96e2c..8a9f0ca8fa96 100644 --- a/lib/vconn.c +++ b/lib/vconn.c @@ -744,18 +744,6 @@ vconn_recv_block(struct vconn *vconn, struct ofpbuf **msgp) return retval; } -static void -vconn_add_bundle_error(const struct ofp_header *oh, struct ovs_list *errors) -{ - if (errors) { - struct vconn_bundle_error *err = xmalloc(sizeof *err); - size_t len = ntohs(oh->length); - - memcpy(err->ofp_msg_data, oh, MIN(len, sizeof err->ofp_msg_data)); - ovs_list_push_back(errors, &err->list_node); - } -} - static int vconn_recv_xid__(struct vconn *vconn, ovs_be32 xid, struct ofpbuf **replyp, struct ovs_list *errors) @@ -781,13 +769,13 @@ vconn_recv_xid__(struct vconn *vconn, ovs_be32 xid, struct ofpbuf **replyp, error = ofptype_decode(&type, oh); if (!error && type == OFPTYPE_ERROR) { - vconn_add_bundle_error(oh, errors); + ovs_list_push_back(errors, &reply->list_node); } else { VLOG_DBG_RL(&bad_ofmsg_rl, "%s: received reply with xid %08"PRIx32 " != expected %08"PRIx32, vconn->name, ntohl(recv_xid), ntohl(xid)); + ofpbuf_delete(reply); } - ofpbuf_delete(reply); } } @@ -1078,7 +1066,8 @@ vconn_bundle_reply_validate(struct ofpbuf *reply, } if (type == OFPTYPE_ERROR) { - vconn_add_bundle_error(oh, errors); + struct ofpbuf *copy = ofpbuf_clone(reply); + ovs_list_push_back(errors, ©->list_node); return ofperr_decode_msg(oh, NULL); } if (type != OFPTYPE_BUNDLE_CONTROL) { @@ -1150,13 +1139,13 @@ vconn_recv_error(struct vconn *vconn, struct ovs_list *errors) oh = reply->data; ofperr = ofptype_decode(&type, oh); if (!ofperr && type == OFPTYPE_ERROR) { - vconn_add_bundle_error(oh, errors); + ovs_list_push_back(errors, &reply->list_node); } else { VLOG_DBG_RL(&bad_ofmsg_rl, "%s: received unexpected reply with xid %08"PRIx32, vconn->name, ntohl(oh->xid)); + ofpbuf_delete(reply); } - ofpbuf_delete(reply); } } while (!error); } @@ -1209,6 +1198,8 @@ vconn_bundle_add_msg(struct vconn *vconn, struct ofputil_bundle_ctrl_msg *bc, return error; } +/* Appends ofpbufs for received errors, if any, to 'errors'. The caller must + * free the received errors. */ int vconn_bundle_transact(struct vconn *vconn, struct ovs_list *requests, uint16_t flags, struct ovs_list *errors) diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index dca9be3a5995..95989eb11d16 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -699,16 +699,18 @@ static void bundle_print_errors(struct ovs_list *errors, struct ovs_list *requests, const char *vconn_name) { - struct vconn_bundle_error *error, *next; + struct ofpbuf *error, *next; struct ofpbuf *bmsg; INIT_CONTAINER(bmsg, requests, list_node); LIST_FOR_EACH_SAFE (error, next, list_node, errors) { + const struct ofp_header *error_oh = error->data; + ovs_be32 error_xid = error_oh->xid; enum ofperr ofperr; struct ofpbuf payload; - ofperr = ofperr_decode_msg(&error->ofp_msg, &payload); + ofperr = ofperr_decode_msg(error_oh, &payload); if (!ofperr) { fprintf(stderr, "***decode error***"); } else { @@ -724,7 +726,7 @@ bundle_print_errors(struct ovs_list *errors, struct ovs_list *requests, LIST_FOR_EACH_CONTINUE (bmsg, list_node, requests) { const struct ofp_header *oh = bmsg->data; - if (oh->xid == error->ofp_msg.xid) { + if (oh->xid == error_xid) { ofp_msg = oh; msg_len = bmsg->size; break; @@ -735,7 +737,7 @@ bundle_print_errors(struct ovs_list *errors, struct ovs_list *requests, verbosity + 1); } ofpbuf_uninit(&payload); - free(error); + ofpbuf_delete(error); } fflush(stderr); } -- 2.10.2 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
