| Message ID | 1549931037-73112-1-git-send-email-cpp.code.lv@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] netlink: added check to prevent netlink attribute overflow | expand |
On Mon, Feb 11, 2019 at 04:23:57PM -0800, Toms Atteka wrote: > If enough large input is passed to odp_actions_from_string it can > cause netlink attribute to overflow. > ovs_assert was added just before the problematic code so it could > be debugged faster in similar cases if they would arise. Check > for buffer size was added to prevent entering this function and > returning appropriate error code. > > Basic manual testing was performed. > > Reported-by: > https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12231 > Signed-off-by: Toms Atteka <cpp.code.lv@gmail.com> Thank you for the fix! An nlattr's size is a uint16_t. I am more comfortable using UINT16_MAX as the maximum value for this type than I am USHRT_MAX, since the latter can in theory be bigger than 16 bits. I am not sure why the assertion in nl_msg_end_nested() is sure to be correct. It seems risky to me. Can you explain? Thanks, Ben. > --- > lib/netlink.c | 1 + > lib/odp-util.c | 4 ++++ > 2 files changed, 5 insertions(+) > > diff --git a/lib/netlink.c b/lib/netlink.c > index de3ebcd..c91c868 100644 > --- a/lib/netlink.c > +++ b/lib/netlink.c > @@ -498,6 +498,7 @@ void > nl_msg_end_nested(struct ofpbuf *msg, size_t offset) > { > struct nlattr *attr = ofpbuf_at_assert(msg, offset, sizeof *attr); > + ovs_assert(msg->size - offset <= USHRT_MAX); > attr->nla_len = msg->size - offset; > } > > diff --git a/lib/odp-util.c b/lib/odp-util.c > index e893f46..9f637ca 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -2161,6 +2161,10 @@ parse_action_list(const char *s, const struct simap *port_names, > n += retval; > } > > + if (actions->size > USHRT_MAX) { > + return -EFBIG; > + } > + > return n; > } > > -- > 2.7.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/netlink.c b/lib/netlink.c index de3ebcd..c91c868 100644 --- a/lib/netlink.c +++ b/lib/netlink.c @@ -498,6 +498,7 @@ void nl_msg_end_nested(struct ofpbuf *msg, size_t offset) { struct nlattr *attr = ofpbuf_at_assert(msg, offset, sizeof *attr); + ovs_assert(msg->size - offset <= USHRT_MAX); attr->nla_len = msg->size - offset; } diff --git a/lib/odp-util.c b/lib/odp-util.c index e893f46..9f637ca 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -2161,6 +2161,10 @@ parse_action_list(const char *s, const struct simap *port_names, n += retval; } + if (actions->size > USHRT_MAX) { + return -EFBIG; + } + return n; }
If enough large input is passed to odp_actions_from_string it can cause netlink attribute to overflow. ovs_assert was added just before the problematic code so it could be debugged faster in similar cases if they would arise. Check for buffer size was added to prevent entering this function and returning appropriate error code. Basic manual testing was performed. Reported-by: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12231 Signed-off-by: Toms Atteka <cpp.code.lv@gmail.com> --- lib/netlink.c | 1 + lib/odp-util.c | 4 ++++ 2 files changed, 5 insertions(+)