Thanks. Lance, are you OK with this solution?
On Tue, Jun 13, 2017 at 04:23:01PM -0700, Jarno Rajahalme wrote: > 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
