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...@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 > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev