What bug do you see in the flow extract code?
On Sat, Mar 04, 2017 at 10:09:26AM +0100, Bhargava Shastry wrote: > Hi Ben, > > Question regarding patch: Shouldn't the fix be applied in flow extract code > itself? I think the malformedness is evident during flow extraction. Might > save you a few cycles/more secure. > > > On March 4, 2017 6:18:54 AM GMT+01:00, Ben Pfaff <[email protected]> wrote: > >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. > > -- > Sent from my Android device with K-9 Mail. Please excuse my brevity. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
