vconn_add_bundle_error() stores a maximum of 64 bytes of an
OpenFlow packet, however ofperr_decode_msg() assumes that the
entire packet is present. This leads to a buffer read overrun
when the the packet is copied to another buffer using the full
packet size.
Fix by adding a parameter to ofperr_decode_msg() indicating the
size of the buffer containing the OpenFlow packet.
Found via gcc's address sanitizer.
Fixes: 506c1ddb3404 ("vconn: Better bundle error management.")
Signed-off-by: Lance Richardson <[email protected]>
---
include/openvswitch/ofp-errors.h | 1 +
lib/ofp-errors.c | 6 ++++--
lib/ofp-print.c | 2 +-
lib/vconn.c | 2 +-
ovn/controller/ofctrl.c | 5 +++--
utilities/ovs-ofctl.c | 3 ++-
6 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/include/openvswitch/ofp-errors.h b/include/openvswitch/ofp-errors.h
index aeb58e0..1b28084 100644
--- a/include/openvswitch/ofp-errors.h
+++ b/include/openvswitch/ofp-errors.h
@@ -809,6 +809,7 @@ bool ofperr_is_valid(enum ofperr);
enum ofperr ofperr_from_name(const char *);
enum ofperr ofperr_decode_msg(const struct ofp_header *,
+ size_t ofp_header_buf_len,
struct ofpbuf *payload);
struct ofpbuf *ofperr_encode_reply(enum ofperr, const struct ofp_header *);
struct ofpbuf *ofperr_encode_hello(enum ofperr, enum ofp_version ofp_version,
diff --git a/lib/ofp-errors.c b/lib/ofp-errors.c
index adc9295..8f65c51 100644
--- a/lib/ofp-errors.c
+++ b/lib/ofp-errors.c
@@ -282,7 +282,8 @@ ofperr_get_code(enum ofperr error, enum ofp_version version)
* aligned). The caller must free the payload (with ofpbuf_uninit()) when it
* is no longer needed. On failure, '*payload' is cleared. */
enum ofperr
-ofperr_decode_msg(const struct ofp_header *oh, struct ofpbuf *payload)
+ofperr_decode_msg(const struct ofp_header *oh, size_t oh_buflen,
+ struct ofpbuf *payload)
{
const struct ofp_error_msg *oem;
enum ofpraw raw;
@@ -294,7 +295,8 @@ ofperr_decode_msg(const struct ofp_header *oh, struct
ofpbuf *payload)
}
/* Pull off the error message. */
- struct ofpbuf b = ofpbuf_const_initializer(oh, ntohs(oh->length));
+ size_t oh_size = MIN(oh_buflen, ntohs(oh->length));
+ struct ofpbuf b = ofpbuf_const_initializer(oh, oh_size);
enum ofperr error = ofpraw_pull(&raw, &b);
if (error) {
return 0;
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 423df31..a341ce0 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -1596,7 +1596,7 @@ ofp_print_error_msg(struct ds *string, const struct
ofp_header *oh,
enum ofperr error;
char *s;
- error = ofperr_decode_msg(oh, &payload);
+ error = ofperr_decode_msg(oh, len, &payload);
if (!error) {
ds_put_cstr(string, "***decode error***");
ds_put_hex_dump(string, oh + 1, len - sizeof *oh, 0, true);
diff --git a/lib/vconn.c b/lib/vconn.c
index 6997eaa..32cb80d 100644
--- a/lib/vconn.c
+++ b/lib/vconn.c
@@ -1079,7 +1079,7 @@ vconn_bundle_reply_validate(struct ofpbuf *reply,
if (type == OFPTYPE_ERROR) {
vconn_add_bundle_error(oh, errors);
- return ofperr_decode_msg(oh, NULL);
+ return ofperr_decode_msg(oh, ntohs(oh->length), NULL);
}
if (type != OFPTYPE_BUNDLE_CONTROL) {
return OFPERR_OFPBRC_BAD_TYPE;
diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 277d3d7..b9988d1 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -283,8 +283,9 @@ recv_S_TLV_TABLE_REQUESTED(const struct ofp_header *oh,
enum ofptype type,
ofperr_to_string(error));
}
} else if (type == OFPTYPE_ERROR) {
+ enum ofperr error = ofperr_decode_msg(oh, ntohs(oh->length), NULL);
VLOG_ERR("switch refused to allocate Geneve option (%s)",
- ofperr_to_string(ofperr_decode_msg(oh, NULL)));
+ ofperr_to_string(error));
} else {
char *s = ofp_to_string(oh, ntohs(oh->length), NULL, 1);
VLOG_ERR("unexpected reply to TLV table request (%s)", s);
@@ -327,7 +328,7 @@ recv_S_TLV_TABLE_MOD_SENT(const struct ofp_header *oh, enum
ofptype type,
} else if (oh->xid == xid2 && type == OFPTYPE_BARRIER_REPLY) {
state = S_CLEAR_FLOWS;
} else if (oh->xid == xid && type == OFPTYPE_ERROR) {
- enum ofperr error = ofperr_decode_msg(oh, NULL);
+ enum ofperr error = ofperr_decode_msg(oh, ntohs(oh->length), NULL);
if (error == OFPERR_NXTTMFC_ALREADY_MAPPED ||
error == OFPERR_NXTTMFC_DUP_ENTRY) {
VLOG_INFO("raced with another controller adding "
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index dca9be3..d60e5d5 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -708,7 +708,8 @@ bundle_print_errors(struct ovs_list *errors, struct
ovs_list *requests,
enum ofperr ofperr;
struct ofpbuf payload;
- ofperr = ofperr_decode_msg(&error->ofp_msg, &payload);
+ ofperr = ofperr_decode_msg(&error->ofp_msg, sizeof error->ofp_msg_data,
+ &payload);
if (!ofperr) {
fprintf(stderr, "***decode error***");
} else {
--
2.9.4
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
