Message ID | 20180714203316.25373-1-jpettit@ovn.org |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev,branch-2.9] Revert "flow: Fix buffer overread for crafted IPv6 packets." | expand |
Thanks Justin, In networking-ovn (the OVN driver for OpenStack Neutron) we are seem an IPv6 related test failure right now [0] and I can confirm that after I've applied this patch locally and re-ran the test it works again. [0] http://logs.openstack.org/59/570459/2/check/networking-ovn-tempest-dsvm-ovs-release/4b5bb1d/logs/testr_results.html.gz (the link will eventually expire) Acked-By: Lucas Alvares Gomes <lucasagomes@gmail.com> On Sat, Jul 14, 2018 at 9:33 PM Justin Pettit <jpettit@ovn.org> wrote: > > This reverts commit 0760bd61a666e9fa866fcb5ed67f48f34895d2f6. > > This patch was a cherry-pick from a bug fix in the master branch that > fixed an overread for IPv6 packets. However, the backport introduced a > problem in older branches, since the code path is different. In the > master branch, this check is done on the raw packet data, which starts > at the beginning of the IPv6 packet. In older branches, this check is > done after a call to data_pull(), which subtracts the IPv6 header length > from the 'size' variable. This means that valid IPv6 packets aren't > being processed since the check thinks they are too long. > > CC: Ben Pfaff <blp@ovn.org> > Fixes: 0760bd61a66 ("flow: Fix buffer overread for crafted IPv6 packets.") > Signed-off-by: Justin Pettit <jpettit@ovn.org> > > --- > This patch should be backported to older branches starting with branch-2.9. > --- > lib/flow.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/flow.c b/lib/flow.c > index c78f46d6c15a..f9d7c2a74007 100644 > --- a/lib/flow.c > +++ b/lib/flow.c > @@ -804,7 +804,7 @@ miniflow_extract(struct dp_packet *packet, struct miniflow *dst) > nh = data_pull(&data, &size, sizeof *nh); > > plen = ntohs(nh->ip6_plen); > - if (OVS_UNLIKELY(plen + IPV6_HEADER_LEN > size)) { > + if (OVS_UNLIKELY(plen > size)) { > goto out; > } > /* Jumbo Payload option not supported yet. */ > -- > 2.17.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
It looks like the code flow ordering was changed recently by 62b0859dd89d(flow: Introduce IP packet sanity checks). - if (OVS_UNLIKELY(size < sizeof *nh)) { + if (OVS_UNLIKELY(!ipv6_sanity_check(nh, size))) { goto out; } - nh = data_pull(&data, &size, sizeof *nh); + data_pull(&data, &size, sizeof *nh); plen = ntohs(nh->ip6_plen); - if (OVS_UNLIKELY(plen > size)) { - goto out; - } - /* Jumbo Payload option not supported yet. */ - if (OVS_UNLIKELY(size - plen > UINT8_MAX)) { - goto out; - } On 7/16/18, 6:35 AM, "ovs-dev-bounces@openvswitch.org on behalf of Lucas Alvares Gomes" <ovs-dev-bounces@openvswitch.org on behalf of lucasagomes@gmail.com> wrote: Thanks Justin, In networking-ovn (the OVN driver for OpenStack Neutron) we are seem an IPv6 related test failure right now [0] and I can confirm that after I've applied this patch locally and re-ran the test it works again. [0] https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flogs.openstack.org%2F59%2F570459%2F2%2Fcheck%2Fnetworking-ovn-tempest-dsvm-ovs-release%2F4b5bb1d%2Flogs%2Ftestr_results.html.gz&data=02%7C01%7Cdball%40vmware.com%7Cd5698b8b3b84410609fc08d5eb210c5a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636673449543755379&sdata=0s1hShgwC7nXh21U9hWXg%2FLbtgO3pgUczJQkodKzzw0%3D&reserved=0 (the link will eventually expire) Acked-By: Lucas Alvares Gomes <lucasagomes@gmail.com> On Sat, Jul 14, 2018 at 9:33 PM Justin Pettit <jpettit@ovn.org> wrote: > > This reverts commit 0760bd61a666e9fa866fcb5ed67f48f34895d2f6. > > This patch was a cherry-pick from a bug fix in the master branch that > fixed an overread for IPv6 packets. However, the backport introduced a > problem in older branches, since the code path is different. In the > master branch, this check is done on the raw packet data, which starts > at the beginning of the IPv6 packet. In older branches, this check is > done after a call to data_pull(), which subtracts the IPv6 header length > from the 'size' variable. This means that valid IPv6 packets aren't > being processed since the check thinks they are too long. > > CC: Ben Pfaff <blp@ovn.org> > Fixes: 0760bd61a66 ("flow: Fix buffer overread for crafted IPv6 packets.") > Signed-off-by: Justin Pettit <jpettit@ovn.org> > > --- > This patch should be backported to older branches starting with branch-2.9. > --- > lib/flow.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/flow.c b/lib/flow.c > index c78f46d6c15a..f9d7c2a74007 100644 > --- a/lib/flow.c > +++ b/lib/flow.c > @@ -804,7 +804,7 @@ miniflow_extract(struct dp_packet *packet, struct miniflow *dst) > nh = data_pull(&data, &size, sizeof *nh); > > plen = ntohs(nh->ip6_plen); > - if (OVS_UNLIKELY(plen + IPV6_HEADER_LEN > size)) { > + if (OVS_UNLIKELY(plen > size)) { > goto out; > } > /* Jumbo Payload option not supported yet. */ > -- > 2.17.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Cdball%40vmware.com%7Cd5698b8b3b84410609fc08d5eb210c5a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636673449543755379&sdata=%2FyB6MWLpdYQa2cplcpeMGQakdPAXgS0zHakvZ95gSBY%3D&reserved=0 _______________________________________________ dev mailing list dev@openvswitch.org https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Cdball%40vmware.com%7Cd5698b8b3b84410609fc08d5eb210c5a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636673449543755379&sdata=%2FyB6MWLpdYQa2cplcpeMGQakdPAXgS0zHakvZ95gSBY%3D&reserved=0
> On Jul 16, 2018, at 6:34 AM, Lucas Alvares Gomes <lucasagomes@gmail.com> wrote: > > Thanks Justin, > > In networking-ovn (the OVN driver for OpenStack Neutron) we are seem > an IPv6 related test failure right now [0] and I can confirm that > after I've applied this patch locally and re-ran the test it works > again. > > [0] http://logs.openstack.org/59/570459/2/check/networking-ovn-tempest-dsvm-ovs-release/4b5bb1d/logs/testr_results.html.gz > (the link will eventually expire) > > Acked-By: Lucas Alvares Gomes <lucasagomes@gmail.com> Thanks for the confirmation. I've pushed this to branches 2.4 through 2.9. --Justin
diff --git a/lib/flow.c b/lib/flow.c index c78f46d6c15a..f9d7c2a74007 100644 --- a/lib/flow.c +++ b/lib/flow.c @@ -804,7 +804,7 @@ miniflow_extract(struct dp_packet *packet, struct miniflow *dst) nh = data_pull(&data, &size, sizeof *nh); plen = ntohs(nh->ip6_plen); - if (OVS_UNLIKELY(plen + IPV6_HEADER_LEN > size)) { + if (OVS_UNLIKELY(plen > size)) { goto out; } /* Jumbo Payload option not supported yet. */
This reverts commit 0760bd61a666e9fa866fcb5ed67f48f34895d2f6. This patch was a cherry-pick from a bug fix in the master branch that fixed an overread for IPv6 packets. However, the backport introduced a problem in older branches, since the code path is different. In the master branch, this check is done on the raw packet data, which starts at the beginning of the IPv6 packet. In older branches, this check is done after a call to data_pull(), which subtracts the IPv6 header length from the 'size' variable. This means that valid IPv6 packets aren't being processed since the check thinks they are too long. CC: Ben Pfaff <blp@ovn.org> Fixes: 0760bd61a66 ("flow: Fix buffer overread for crafted IPv6 packets.") Signed-off-by: Justin Pettit <jpettit@ovn.org> --- This patch should be backported to older branches starting with branch-2.9. --- lib/flow.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)