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

From	Hannes Frederic Sowa <hannes@stressinduktion.org>
Newsgroups	linux.kernel
Subject	Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to device
Date	2016-11-14 18:10 +0100
Message-ID	<sDsVz-1pb-9@gated-at.bofh.it> (permalink)
References	<sD9SV-5kR-17@gated-at.bofh.it> <sDcnM-73M-9@gated-at.bofh.it> <sDsCe-12v-21@gated-at.bofh.it> <sDsLU-16h-21@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

On 14.11.2016 17:55, David Ahern wrote:
> On 11/14/16 9:44 AM, Hannes Frederic Sowa wrote:
>> On Mon, Nov 14, 2016, at 00:28, Jason A. Donenfeld wrote:
>>> This puts the IPv6 routing functions in parity with the IPv4 routing
>>> functions. Namely, we now check in v6 that if a flowi6 requests an
>>> saddr, the returned dst actually corresponds to a net device that has
>>> that saddr. This mirrors the v4 logic with __ip_dev_find in
>>> __ip_route_output_key_hash. In the event that the returned dst is not
>>> for a dst with a dev that has the saddr, we return -EINVAL, just like
>>> v4; this makes it easy to use the same error handlers for both cases.
>>>
>>> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>>> Cc: David Ahern <dsa@cumulusnetworks.com>
>>> ---
>>> Changes from v2:
>>>     It turns out ipv6_chk_addr already has the device enumeration
>>>     logic that we need by simply passing NULL.
>>>
>>>  net/ipv6/ip6_output.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
>>> index 6001e78..b3b5cb6 100644
>>> --- a/net/ipv6/ip6_output.c
>>> +++ b/net/ipv6/ip6_output.c
>>> @@ -926,6 +926,10 @@ static int ip6_dst_lookup_tail(struct net *net,
>>> const struct sock *sk,
>>>  	int err;
>>>  	int flags = 0;
>>>  
>>> +       if (!ipv6_addr_any(&fl6->saddr) &&
>>> +           !ipv6_chk_addr(net, &fl6->saddr, NULL, 1))
>>> +               return -EINVAL;
>>
>> Hmm, this check is too permissive, no?
>>
>> E.g. what happens if you move a link local address from one interface to
>> another? In this case this code would still allow the saddr to be used.
> 
> This check -- like the ipv4 variant -- only verifies the saddr is locally assigned. If the address moves interfaces it should be fine.

But in this case we should actually bail out, no?

Let's say, user assumes we are on ifindex eth0 with LL address from
eth0. Suddenly the LL address from eth0 is moved to eth1, we can't
accept this source address anymore and need to return -EINVAL, too.

>> I just also quickly read up on the history (sorry was travelling last
>> week) and wonder if you ever saw a user space facing bug or if this is
>> basically some difference you saw while writing out of tree code?
> 
> I checked the userspace API this morning. bind and cmsg for example check that the address is valid with calls to ipv6_chk_addr.

Hmm, so it fixes no real bug.

Because of translations of flowi6_oif we actually can't do a correct
check of source address for cases like the one I outlined above? Hmm,
maybe we should simply depend on user space checks.

Bye,
Hannes

Thread

Re: Source address fib invalidation on IPv6 David Ahern <dsa@cumulusnetworks.com> - 2016-11-11 23:30 +0100
  Re: Source address fib invalidation on IPv6 "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-11-12 03:20 +0100
    Re: Source address fib invalidation on IPv6 "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-11-12 16:50 +0100
      Re: Source address fib invalidation on IPv6 David Ahern <dsa@cumulusnetworks.com> - 2016-11-12 19:20 +0100
        Re: Source address fib invalidation on IPv6 "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-11-12 20:10 +0100
          Re: Source address fib invalidation on IPv6 "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-11-13 01:50 +0100
            Re: Source address fib invalidation on IPv6 Hannes Frederic Sowa <hannes@stressinduktion.org> - 2016-11-13 02:00 +0100
              Re: Source address fib invalidation on IPv6 "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-11-13 02:10 +0100
                [PATCH] ip6_output: ensure flow saddr actually belongs to device "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-11-13 14:40 +0100
                Re: [PATCH] ip6_output: ensure flow saddr actually belongs to device David Ahern <dsa@cumulusnetworks.com> - 2016-11-13 17:40 +0100
                [PATCH v2] ip6_output: ensure flow saddr actually belongs to device "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-11-13 20:10 +0100
                Re: [PATCH v2] ip6_output: ensure flow saddr actually belongs to  device David Ahern <dsa@cumulusnetworks.com> - 2016-11-13 21:50 +0100
                [PATCH v3] ip6_output: ensure flow saddr actually belongs to device "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-11-14 00:30 +0100
                Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to  device David Ahern <dsa@cumulusnetworks.com> - 2016-11-14 17:20 +0100
                Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to device Hannes Frederic Sowa <hannes@stressinduktion.org> - 2016-11-14 17:50 +0100
                Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to  device David Ahern <dsa@cumulusnetworks.com> - 2016-11-14 18:00 +0100
                Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to  device Hannes Frederic Sowa <hannes@stressinduktion.org> - 2016-11-14 18:10 +0100
                Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to  device David Ahern <dsa@cumulusnetworks.com> - 2016-11-14 18:20 +0100
                Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to  device Hannes Frederic Sowa <hannes@stressinduktion.org> - 2016-11-14 18:40 +0100
                Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to  device David Ahern <dsa@cumulusnetworks.com> - 2016-11-14 18:50 +0100
                Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to device Hannes Frederic Sowa <hannes@stressinduktion.org> - 2016-11-14 19:40 +0100
                Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to device "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-11-15 01:50 +0100
                Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to  device Hannes Frederic Sowa <hannes@stressinduktion.org> - 2016-11-15 15:50 +0100
                Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to  device David Ahern <dsa@cumulusnetworks.com> - 2016-11-15 16:30 +0100
                Re: [PATCH] ip6_output: ensure flow saddr actually belongs to device "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-11-13 21:30 +0100
                Re: [PATCH] ip6_output: ensure flow saddr actually belongs to device David Ahern <dsa@cumulusnetworks.com> - 2016-11-13 21:40 +0100
            Re: Source address fib invalidation on IPv6 "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-11-13 02:00 +0100

csiph-web