| Message ID | 20170613230429.6909-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
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 <jarno@ovn.org <mailto:jarno@ovn.org>> > On Jun 13, 2017, at 4:04 PM, Ben Pfaff <blp@ovn.org> 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 <lrichard@redhat.com> > CC: Jarno Rajahalme <jarno@ovn.org> > Fixes: 506c1ddb3404 ("vconn: Better bundle error management.") > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > 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 >
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 <jarno@ovn.org <mailto:jarno@ovn.org>> > > > On Jun 13, 2017, at 4:04 PM, Ben Pfaff <blp@ovn.org> 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 <lrichard@redhat.com> > > CC: Jarno Rajahalme <jarno@ovn.org> > > Fixes: 506c1ddb3404 ("vconn: Better bundle error management.") > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > 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 > > >
> From: "Ben Pfaff" <blp@ovn.org> > To: "Jarno Rajahalme" <jarno@ovn.org> > Cc: dev@openvswitch.org, "Lance Richardson" <lrichard@redhat.com> > Sent: Tuesday, 13 June, 2017 8:10:22 PM > Subject: Re: [PATCH] ovs-ofctl: Avoid read overrun in ofperr_decode_msg(). > > Thanks. > > Lance, are you OK with this solution? Fine with me, it does seem less error-prone to keep the whole packet. Thanks, Lance
On Tue, Jun 13, 2017 at 10:59:39PM -0400, Lance Richardson wrote: > > From: "Ben Pfaff" <blp@ovn.org> > > To: "Jarno Rajahalme" <jarno@ovn.org> > > Cc: dev@openvswitch.org, "Lance Richardson" <lrichard@redhat.com> > > Sent: Tuesday, 13 June, 2017 8:10:22 PM > > Subject: Re: [PATCH] ovs-ofctl: Avoid read overrun in ofperr_decode_msg(). > > > > Thanks. > > > > Lance, are you OK with this solution? > > Fine with me, it does seem less error-prone to keep the whole packet. Thanks Lance and Jarno, I applied this to master, branch-2.7, and branch-2.6.
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); }
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 <lrichard@redhat.com> CC: Jarno Rajahalme <jarno@ovn.org> Fixes: 506c1ddb3404 ("vconn: Better bundle error management.") Signed-off-by: Ben Pfaff <blp@ovn.org> --- include/openvswitch/vconn.h | 12 ------------ lib/vconn.c | 25 ++++++++----------------- utilities/ovs-ofctl.c | 10 ++++++---- 3 files changed, 14 insertions(+), 33 deletions(-)