diff mbox series

[ovs-dev,branch-2.9] Revert "flow: Fix buffer overread for crafted IPv6 packets."

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

Commit Message

Justin Pettit July 14, 2018, 8:33 p.m. UTC 
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(-)

Comments

Lucas Alvares Gomes July 16, 2018, 1:34 p.m. UTC | #1 
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
Darrell Ball July 16, 2018, 2:57 p.m. UTC | #2 
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&amp;data=02%7C01%7Cdball%40vmware.com%7Cd5698b8b3b84410609fc08d5eb210c5a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636673449543755379&amp;sdata=0s1hShgwC7nXh21U9hWXg%2FLbtgO3pgUczJQkodKzzw0%3D&amp;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&amp;data=02%7C01%7Cdball%40vmware.com%7Cd5698b8b3b84410609fc08d5eb210c5a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636673449543755379&amp;sdata=%2FyB6MWLpdYQa2cplcpeMGQakdPAXgS0zHakvZ95gSBY%3D&amp;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&amp;data=02%7C01%7Cdball%40vmware.com%7Cd5698b8b3b84410609fc08d5eb210c5a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636673449543755379&amp;sdata=%2FyB6MWLpdYQa2cplcpeMGQakdPAXgS0zHakvZ95gSBY%3D&amp;reserved=0
Justin Pettit July 16, 2018, 7:28 p.m. UTC | #3 
> 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 mbox series

Patch

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. */