Seems like I leaped from the fact that error message’s payload must contain at least 64 bytes of the message causing the error (or, less, if the message length was less than 64), to the erroneous notion that the whole error message would only need 64 bytes of storage. Thanks for fixing this.
Acked-by: Jarno Rajahlame <[email protected] <mailto:[email protected]>> > On Jun 13, 2017, at 4:04 PM, Ben Pfaff <[email protected]> wrote: > > 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
