On Fri, Mar 03, 2017 at 05:00:38PM -0800, Daniele Di Proietto wrote: > 2017-03-03 14:08 GMT-08:00 Ben Pfaff <[email protected]>: > > Otherwise a malformed packet could cause a read up to about 40 bytes past > > the end of the packet. The packet would still likely be dropped because > > of checksum verification. > > > > Reported-by: Bhargava Shastry <[email protected]> > > Signed-off-by: Ben Pfaff <[email protected]> > > Oops, thanks for the fix, Ben > > Fixes: a489b16854b5("conntrack: New userspace connection tracker.") > > One minor comment below, > > Acked-by: Daniele Di Proietto <[email protected]> > > > > --- > > lib/conntrack.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/lib/conntrack.c b/lib/conntrack.c > > index 9bea3d93e4ad..9c1dd63648b8 100644 > > --- a/lib/conntrack.c > > +++ b/lib/conntrack.c > > @@ -568,6 +568,10 @@ extract_l3_ipv6(struct conn_key *key, const void > > *data, size_t size, > > const char **new_data) > > { > > const struct ovs_16aligned_ip6_hdr *ip6 = data; > > + if (size < sizeof *ip6) { > > + return false; > > + } > > + > > We can read 'ip6->ip6_nxt' even though there's not enough data. It > cannot happen > for regular TCP and UDP packets (those are covered my > miniflow_extract), but only > when parsing the nested l3 header in an ICMP error message. > > The code has the same check two lines below, maybe we can reuse that. > Technically > the check is necessary only if new_data != NULL, as explained by the comment > above, but perhaps it's more clear to always perform it.
Thanks for pointing out the duplicate check. I guess that I did not read the code carefully enough. Usually, I would argue to always do the check, but I prefer to make this a minimal change. I will post a v2. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
