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

[David Ahern]

On 11/15/16 7:45 AM, Hannes Frederic Sowa wrote:

>> @@ -1012,6 +1013,16 @@ static int ip6_dst_lookup_tail(struct net *net,
>> const struct sock *sk,
>>  }
>>  #endif
>>
>> +addr_type = ipv6_addr_type(>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, >saddr, bind_to_dev ?
>> (*dst)->dev : NULL, 0) &&
>> +!ipv6_chk_acast_addr_src(net, (*dst)->dev, >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.

To be consistent with IPv4 the saddr check is done before the lookup and dst 
and flow oif should not be used. Handling LL addresses are trickier and perhaps 
this is not the right place to enforce that check since it requires a specific 
device which is only really known after lookup. Why not add the if saddr is LL 
verification as part of the route selection? e.g, add something like 
rt6_device_match to ip6_pol_route (the device match call is only used for 
ip6_pol_route_lookup and not ip6_pol_route - why is that?).




___
WireGuard mailing list
WireGuard@lists.zx2c4.com
http://lists.zx2c4.com/mailman/listinfo/wireguard

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

[Hannes Frederic Sowa]

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(>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, >saddr, bind_to_dev ?
> (*dst)->dev : NULL, 0) &&
> +!ipv6_chk_acast_addr_src(net, (*dst)->dev, >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

___
WireGuard mailing list
WireGuard@lists.zx2c4.com
http://lists.zx2c4.com/mailman/listinfo/wireguard

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

[Jason A. Donenfeld]

Hey Hannes, David,

On Mon, Nov 14, 2016 at 7:33 PM, Hannes Frederic Sowa
 wrote:
> I meant to say, we don't require the IPv6 "API" to behave in a similar
> way like the IPv4 one. We do this function pointer trick to allow
> _in-kernel_ tree modules to use the function dynamically, even the
> kernel ipv6 module would be available but is not loaded but don't
> guarante any "API like IPv4" to outside tree modules.

Ultimately I intend to submit my own use case for mainline inclusion.
I have no intention of maintaining my work as exclusively out of tree
and would be very pleased to see this well integrated. Hopefully I'll
meet some of you in person at upcoming conferences and events for
discussion about this.

Were I only concerned about out of tree status, I'd have no hesitation
about just including these particular checks in my own code and
leaving the current in tree code to rot. However, with my final
intentions in mind, it's certainly in my interest to "do things
right", hence this thread. When I noticed that the ipv6_stub routing
function was insufficient for my own project, I examined its usage in
a few other places. And indeed vxlan and geneve seem to also benefit
from this patch. I'd be happy to audit the small handful of other
cases in the kernel that use this API; I suspect they'll also be
improved in a positive way.

> I tried to make the point, that it is still something internal to the
> kernel if compared to out-of-tree function users. And that different
> behavior by itself doesn't count as a bug.

To the extent that other in-tree code uses this function and doesn't
get the saddr check, it seems like a bug. To the extent that this
function becomes more correct, it seems like an improvement. But
whether a bug or a mere improvement, it seems like a net positive.

> We could as well require the users of this function to check for the
> source address before or require to check the source address after the
> ipv6_dst_lookup call.

As said above, I have no qualms about doing this check in my own code.
I was thinking, though, that a handful of other places in the kernel
that use this function would benefit from adding that check too. In
this case, it's usually common practice to move the check into the
shared function, rather than require a flurry of copy-and-paste.

> vxlan currently seems wrong and would impacted by this patch in a better
> way, so I am all in for such a change, but I think we need to check if
> we are also correct scope-wise and not just match for the address on its
> own.

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:


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(>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, >saddr, bind_to_dev ?
(*dst)->dev : NULL, 0) &&
+!ipv6_chk_acast_addr_src(net, (*dst)->dev, >saddr))
+goto out_err_release;
+
 return 0;

 out_err_release:
___
WireGuard mailing list
WireGuard@lists.zx2c4.com
http://lists.zx2c4.com/mailman/listinfo/wireguard

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

[David Ahern]

On 11/14/16 10:33 AM, Hannes Frederic Sowa wrote:
> 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.
>>
>> I believe Jason's case is forwarding path and the ipv6_stub->ipv6_dst_lookup 
>> API.
> 
> It is not a kernel API, because we don't support something like that for
> external kernel modules. We basically exported ipv6_dst_lookup to allow
> some IPv4 code to do ipv6 stunts when the IPv6 module is loaded. ;)

???

ipv6_stub is exported for modules (EXPORT_SYMBOL_GPL(ipv6_stub)).

ipv6_stub->ipv6_dst_lookup is used by several modules -- geneve, tipc, vxlan, 
mpls -- for IPv6 lookups, not IPv4 code do IPv6 stunts.

So how do you say that is not an exported kernel API?
___
WireGuard mailing list
WireGuard@lists.zx2c4.com
http://lists.zx2c4.com/mailman/listinfo/wireguard

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

[David Ahern]

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 
>> Cc: David Ahern 
>> ---
>> 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(>saddr) &&
>> +   !ipv6_chk_addr(net, >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.

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

___
WireGuard mailing list
WireGuard@lists.zx2c4.com
http://lists.zx2c4.com/mailman/listinfo/wireguard

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

[David Ahern]

On 11/13/16 4:28 PM, 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 
> Cc: David Ahern 
> ---
> 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(>saddr) &&
> + !ipv6_chk_addr(net, >saddr, NULL, 1))
> + return -EINVAL;
> +
>   /* The correct way to handle this would be to do
>* ip6_route_get_saddr, and then ip6_route_output; however,
>* the route-specific preferred source forces the
> 

LGTM

Acked-by: David Ahern 
___
WireGuard mailing list
WireGuard@lists.zx2c4.com
http://lists.zx2c4.com/mailman/listinfo/wireguard

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

[Jason A. Donenfeld]

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 
Cc: David Ahern 
---
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(>saddr) &&
+   !ipv6_chk_addr(net, >saddr, NULL, 1))
+   return -EINVAL;
+
/* The correct way to handle this would be to do
 * ip6_route_get_saddr, and then ip6_route_output; however,
 * the route-specific preferred source forces the
-- 
2.10.2

___
WireGuard mailing list
WireGuard@lists.zx2c4.com
http://lists.zx2c4.com/mailman/listinfo/wireguard