netdev - Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7543fa0f-a053-8387-1862-d78ffadba688@stressinduktion.org>
Date:   Tue, 15 Nov 2016 15:45:29 +0100
From:   Hannes Frederic Sowa <hannes@...essinduktion.org>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     David Ahern <dsa@...ulusnetworks.com>,
        Netdev <netdev@...r.kernel.org>,
        WireGuard mailing list <wireguard@...ts.zx2c4.com>,
        LKML <linux-kernel@...r.kernel.org>,
        YOSHIFUJI Hideaki <hideaki.yoshifuji@...aclelinux.com>
Subject: Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to
 device

Hey Jason,

On 15.11.2016 01:45, Jason A. Donenfeld wrote:
> I'll have a better look at this. Perhaps this should be modeled on
> what we currently do for userspace, which might amount to something
> more or less like:

Cool, thanks!

> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 6001e78..0721915 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -925,6 +925,7 @@ static int ip6_dst_lookup_tail(struct net *net,
> const struct sock *sk,
>  #endif
>          int err;
>          int flags = 0;
> +        int addr_type, bind_to_dev;
> 
>          /* The correct way to handle this would be to do
>           * ip6_route_get_saddr, and then ip6_route_output; however,
> @@ -1012,6 +1013,16 @@ static int ip6_dst_lookup_tail(struct net *net,
> const struct sock *sk,
>          }
>  #endif
> 
> +        addr_type = ipv6_addr_type(&fl6->saddr);
> +        if (addr_type == IPv6_ADDR_ANY)
> +                return 0;
> +
> +        err = -EINVAL;
> +        bind_to_dev = __ipv6_addr_src_scope(addr_type) <=
> IPV6_ADDR_SCOPE_LINKLOCAL;
> +        if (!ipv6_chk_addr(net, &fl6->saddr, bind_to_dev ?
> (*dst)->dev : NULL, 0) &&
> +            !ipv6_chk_acast_addr_src(net, (*dst)->dev, &fl6->saddr))
> +                goto out_err_release;
> +
>          return 0;
> 
>  out_err_release:
> 

We should not use (*dst)->dev, as this is the resulting device after the
lookup and not necessarily corresponds to the device the user asked for.
Thus you need to pass in fl6.flowi6_oif. Thus to kill the necessary
ifindex->net_device lookup, I would suggest to move
ipv6_chk_addr_and_flags to use ifindex instead of net_device (0
corresponds to the net_device == NULL case). It seems to me this would
make the code easier. ipv6_chk_addr can simply pass down dev->ifindex to
ipv6_chk_addr.

Probably for checking anycast address you need to look up the
net_device, thus use dev_get_by_index_rcu. But probably the unicast
filter will already hit thus the whole traversing of anycast addresses
won't happen in normal cases. This could be separated to its own function.

In the non-strict case we don't necessarily need bind_to_dev?

Bye,
Hannes