Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1519992 > unrolled thread
| Started by | "Jason A. Donenfeld" <Jason@zx2c4.com> |
|---|---|
| First post | 2016-11-11 20:40 +0100 |
| Last post | 2016-11-13 02:00 +0100 |
| Articles | 20 on this page of 28 — 3 participants |
Back to article view | Back to linux.kernel
Source address fib invalidation on IPv6 "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-11-11 20:40 +0100
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
Page 1 of 2 [1] 2 Next page →
| From | "Jason A. Donenfeld" <Jason@zx2c4.com> |
|---|---|
| Date | 2016-11-11 20:40 +0100 |
| Subject | Source address fib invalidation on IPv6 |
| Message-ID | <sCpQ6-5Y-21@gated-at.bofh.it> |
Hi folks,
If I'm replying to a UDP packet, I generally want to use a source
address that's the same as the destination address of the packet to
which I'm replying. For example:
Peer A sends packet: src = 10.0.0.1, dst = 10.0.0.3
Peer B replies with: src = 10.0.0.3, dst = 10.0.0.1
But let's complicate things. Let's say Peer B has multiple IPs on an
interface: 10.0.0.2, 10.0.0.3. The default route uses 10.0.0.2. In
this case what do you think should happen?
Case 1:
Peer A sends packet: src = 10.0.0.1, dst = 10.0.0.3
Peer B replies with: src = 10.0.0.2, dst = 10.0.0.1
Case 2:
Peer A sends packet: src = 10.0.0.1, dst = 10.0.0.3
Peer B replies with: src = 10.0.0.3, dst = 10.0.0.1
Intuition tells me the answer is "Case 2". If you agree, keep reading.
If you disagree, stop reading here, and instead correct my poor
intuition.
So, assuming "Case 2", when Peer B receives the first packet, he notes
that packet's destination address, so that he can use it as a source
address next. When replying, Peer B sets the stored source address and
calls the routing function:
struct flowi4 fl = {
.saddr = from_daddr_of_previous_packet,
.daddr = from_saddr_of_previous_packet,
};
rt = ip_route_output_flow(sock_net(sock), &fl, sock);
What if, however, by the time Peer B chooses to reply, his interface
no longer has that source address? No problem, because
ip_route_output_flow will return -EINVAL in that case. So, we can do
this:
struct flowi4 fl = {
.saddr = from_daddr_of_previous_packet,
.daddr = from_saddr_of_previous_packet,
};
rt = ip_route_output_flow(sock_net(sock), &fl, sock);
if (unlikely(IS_ERR(rt))) {
fl.saddr = 0;
rt = ip_route_output_flow(sock_net(sock), &fl, sock);
}
And then all is good in the neighborhood. This solution works. Done.
But what about IPv6? That's where we get into trouble:
struct flowi6 fl = {
.saddr = from_daddr_of_previous_packet,
.daddr = from_saddr_of_previous_packet,
};
ret = ipv6_stub->ipv6_dst_lookup(sock_net(sock), sock, &dst, &fl);
In this case, IPv6 returns a valid dst, when no interface has the
source address anymore! So, there's no way to know whether or not the
source address for replying has gone stale. We don't have a means of
falling back to inaddr_any for the source address.
Primary question: is this behavior a bug? Or is this some consequence
of a fundamental IPv6 difference with v4? Or is something else
happening here?
Thanks,
Jason
[toc] | [next] | [standalone]
| From | David Ahern <dsa@cumulusnetworks.com> |
|---|---|
| Date | 2016-11-11 23:30 +0100 |
| Message-ID | <sCsuC-1Y1-37@gated-at.bofh.it> |
| In reply to | #1519992 |
On 11/11/16 12:29 PM, Jason A. Donenfeld wrote:
> Hi folks,
>
> If I'm replying to a UDP packet, I generally want to use a source
> address that's the same as the destination address of the packet to
> which I'm replying. For example:
>
> Peer A sends packet: src = 10.0.0.1, dst = 10.0.0.3
> Peer B replies with: src = 10.0.0.3, dst = 10.0.0.1
>
> But let's complicate things. Let's say Peer B has multiple IPs on an
> interface: 10.0.0.2, 10.0.0.3. The default route uses 10.0.0.2. In
> this case what do you think should happen?
>
> Case 1:
> Peer A sends packet: src = 10.0.0.1, dst = 10.0.0.3
> Peer B replies with: src = 10.0.0.2, dst = 10.0.0.1
>
> Case 2:
> Peer A sends packet: src = 10.0.0.1, dst = 10.0.0.3
> Peer B replies with: src = 10.0.0.3, dst = 10.0.0.1
>
> Intuition tells me the answer is "Case 2". If you agree, keep reading.
> If you disagree, stop reading here, and instead correct my poor
> intuition.
>
> So, assuming "Case 2", when Peer B receives the first packet, he notes
> that packet's destination address, so that he can use it as a source
> address next. When replying, Peer B sets the stored source address and
> calls the routing function:
>
> struct flowi4 fl = {
> .saddr = from_daddr_of_previous_packet,
> .daddr = from_saddr_of_previous_packet,
> };
> rt = ip_route_output_flow(sock_net(sock), &fl, sock);
>
> What if, however, by the time Peer B chooses to reply, his interface
> no longer has that source address? No problem, because
> ip_route_output_flow will return -EINVAL in that case. So, we can do
> this:
>
> struct flowi4 fl = {
> .saddr = from_daddr_of_previous_packet,
> .daddr = from_saddr_of_previous_packet,
> };
> rt = ip_route_output_flow(sock_net(sock), &fl, sock);
> if (unlikely(IS_ERR(rt))) {
> fl.saddr = 0;
> rt = ip_route_output_flow(sock_net(sock), &fl, sock);
> }
>
> And then all is good in the neighborhood. This solution works. Done.
>
> But what about IPv6? That's where we get into trouble:
>
> struct flowi6 fl = {
> .saddr = from_daddr_of_previous_packet,
> .daddr = from_saddr_of_previous_packet,
> };
> ret = ipv6_stub->ipv6_dst_lookup(sock_net(sock), sock, &dst, &fl);
>
> In this case, IPv6 returns a valid dst, when no interface has the
> source address anymore! So, there's no way to know whether or not the
> source address for replying has gone stale. We don't have a means of
> falling back to inaddr_any for the source address.
What do you mean by 'valid dst'? ipv6 returns net->ipv6.ip6_null_entry on lookup failures so yes dst is non-NULL but that does not mean the lookup succeeded.
For example take a look at ip6_dst_lookup_tail():
if (!*dst)
*dst = ip6_route_output_flags(net, sk, fl6, flags);
err = (*dst)->error;
if (err)
goto out_err_release;
perhaps I should add dst->error to the fib tracepoints ...
>
> Primary question: is this behavior a bug? Or is this some consequence
> of a fundamental IPv6 difference with v4? Or is something else
> happening here?
>
> Thanks,
> Jason
>
[toc] | [prev] | [next] | [standalone]
| From | "Jason A. Donenfeld" <Jason@zx2c4.com> |
|---|---|
| Date | 2016-11-12 03:20 +0100 |
| Message-ID | <sCw5c-4hn-5@gated-at.bofh.it> |
| In reply to | #1520076 |
Hi David, On Fri, Nov 11, 2016 at 11:14 PM, David Ahern <dsa@cumulusnetworks.com> wrote: > What do you mean by 'valid dst'? ipv6 returns net->ipv6.ip6_null_entry on lookup failures so yes dst is non-NULL but that does not mean the lookup succeeded. What I mean is that it returns an ordinary dst, as if that souce address _hadn't_ been removed from the interface, even though I just removed it. Is this buggy behavior? If so, let me know and I'll try to track it down. The expected behavior, as far as I can see, would be the same that ip_route_output_flow has -- returning -EINVAL when the saddr isn't valid. At the moment, when the saddr is invalid, ipv6_stub->ipv6_dst_lookup returns 0 and &dst contains a real entry. Regards, Jason
[toc] | [prev] | [next] | [standalone]
| From | "Jason A. Donenfeld" <Jason@zx2c4.com> |
|---|---|
| Date | 2016-11-12 16:50 +0100 |
| Message-ID | <sCIJ3-3SS-3@gated-at.bofh.it> |
| In reply to | #1520155 |
Hi again,
I've done some pretty in depth debugging now to determine exactly what
the behavior of ipv6_stub->ipv6_dst_lookup is. First I'll start with
ip_route_output_flow, which I believe to be well behaved, and then
I'll show ipv6_stub->ipv6_dst_lookup, which seems ill-behaved:
Userspace:
ip addr add 192.168.1.2/24 dev eth0
Kernelspace:
struct flowi4 fl = {
.saddr = 192.168.1.2,
.daddr = 192.168.1.99,
};
rt = ip_route_output_flow(sock_net(sock), &fl, sock);
// rt returns valid rt for routing to 192.168.1.99 from
192.168.1.2 using eth0
Userspace:
ip addr add 192.168.1.3/24 dev eth0
ip addr del 192.168.1.2/24 dev eth0
Kernelspace:
struct flowi4 fl = {
.saddr = 192.168.1.2,
.daddr = 192.168.1.99,
};
rt = ip_route_output_flow(sock_net(sock), &fl, sock);
// PTR_ERR(rt) == -EINVAL
This seems correct behavior to me, since no interface has 192.168.1.2
as a source address.
Now for the incorrect IPv6 behavior:
Userspace:
ip -6 addr add abcd::2/96 dev eth0
Kernelspace:
struct flowi6 fl = {
.saddr = abcd::2,
.daddr = abcd::99,
};
ret = ipv6_stub->ipv6_dst_lookup(sock_net(sock), sock, &dst, &fl);
// ret is 0, and dst is a non-null dst routing to abcd::99 from
abcd::2 using eth0
Userspace:
ip -6 addr add abcd::3/96 dev eth0
ip -6 addr del abcd::2/96 dev eth0
Kernelspace:
struct flowi6 fl = {
.saddr = abcd::2,
.daddr = abcd::99,
};
ret = ipv6_stub->ipv6_dst_lookup(sock_net(sock), sock, &dst, &fl);
// ret is 0, and dst is a non-null dst routing to abcd::99 from
abcd::2 using eth0 **INCORRECT BEHAVIOR!**
This seems *INCORRECT* behavior to me, since no interface has abcd::2
as a source address.
So, to summarize, the problem is that ipv6_dst_lookup will happily
return a dst even though the source IP has been removed from the
interface.
I hope this clarifies things. I await your response.
Regards,
Jason
[toc] | [prev] | [next] | [standalone]
| From | David Ahern <dsa@cumulusnetworks.com> |
|---|---|
| Date | 2016-11-12 19:20 +0100 |
| Message-ID | <sCL4e-5H5-7@gated-at.bofh.it> |
| In reply to | #1520257 |
On 11/12/16 8:40 AM, Jason A. Donenfeld wrote:
> Hi again,
>
> I've done some pretty in depth debugging now to determine exactly what
> the behavior of ipv6_stub->ipv6_dst_lookup is. First I'll start with
> ip_route_output_flow, which I believe to be well behaved, and then
> I'll show ipv6_stub->ipv6_dst_lookup, which seems ill-behaved:
>
> Userspace:
> ip addr add 192.168.1.2/24 dev eth0
> Kernelspace:
> struct flowi4 fl = {
> .saddr = 192.168.1.2,
> .daddr = 192.168.1.99,
> };
> rt = ip_route_output_flow(sock_net(sock), &fl, sock);
> // rt returns valid rt for routing to 192.168.1.99 from
> 192.168.1.2 using eth0
> Userspace:
> ip addr add 192.168.1.3/24 dev eth0
> ip addr del 192.168.1.2/24 dev eth0
> Kernelspace:
> struct flowi4 fl = {
> .saddr = 192.168.1.2,
> .daddr = 192.168.1.99,
> };
> rt = ip_route_output_flow(sock_net(sock), &fl, sock);
> // PTR_ERR(rt) == -EINVAL
I believe that is coming from __ip_route_output_key_hash(), line 2232 with __ip_dev_find not finding a device with that address.
Not applicable for your use case, but __ip_dev_find does not have any checks on which L3 domain the device belongs to so the check does not handle VRF for example. I'll take a look at fixing this next week.
>
> This seems correct behavior to me, since no interface has 192.168.1.2
> as a source address.
>
> Now for the incorrect IPv6 behavior:
>
> Userspace:
> ip -6 addr add abcd::2/96 dev eth0
> Kernelspace:
> struct flowi6 fl = {
> .saddr = abcd::2,
> .daddr = abcd::99,
> };
> ret = ipv6_stub->ipv6_dst_lookup(sock_net(sock), sock, &dst, &fl);
> // ret is 0, and dst is a non-null dst routing to abcd::99 from
> abcd::2 using eth0
> Userspace:
> ip -6 addr add abcd::3/96 dev eth0
> ip -6 addr del abcd::2/96 dev eth0
> Kernelspace:
> struct flowi6 fl = {
> .saddr = abcd::2,
> .daddr = abcd::99,
> };
> ret = ipv6_stub->ipv6_dst_lookup(sock_net(sock), sock, &dst, &fl);
> // ret is 0, and dst is a non-null dst routing to abcd::99 from
> abcd::2 using eth0 **INCORRECT BEHAVIOR!**
>
> This seems *INCORRECT* behavior to me, since no interface has abcd::2
> as a source address.
Gotcha. I don't see any checks that the saddr is valid similar to what IPv4 does.
I think the right place to add a check is in ip6_dst_lookup_tail():
if (!ipv6_addr_any(&fl6->saddr)) {
// saddr is valid for L3 domain
}
[toc] | [prev] | [next] | [standalone]
| From | "Jason A. Donenfeld" <Jason@zx2c4.com> |
|---|---|
| Date | 2016-11-12 20:10 +0100 |
| Message-ID | <sCLQB-6es-3@gated-at.bofh.it> |
| In reply to | #1520324 |
Hi David,
On Sat, Nov 12, 2016 at 7:14 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
> I believe that is coming from __ip_route_output_key_hash(), line 2232 with __ip_dev_find not finding a device with that address.
It's possible we simply are looking at different source trees, but I
have the -EINVAL return in 4.8 route.c sources happening due to the
assignment on line 2175 and the jump on line 2220.
> Not applicable for your use case, but __ip_dev_find does not have any checks on which L3 domain the device belongs to so the check does not handle VRF for example. I'll take a look at fixing this next week.
Interesting.
>
> Gotcha. I don't see any checks that the saddr is valid similar to what IPv4 does.
>
> I think the right place to add a check is in ip6_dst_lookup_tail():
> if (!ipv6_addr_any(&fl6->saddr)) {
> // saddr is valid for L3 domain
> }
Right. It should probably do the check here, and return
ERR_PTR(-EINVAL), the same as the v4 version, so that ret codes can be
checked consistently.
Thanks for looking into this. If you're backed up and would like me to
submit a patch, just let me know, and I'll give it my best shot.
Regards,
Jason
[toc] | [prev] | [next] | [standalone]
| From | "Jason A. Donenfeld" <Jason@zx2c4.com> |
|---|---|
| Date | 2016-11-13 01:50 +0100 |
| Message-ID | <sCR9D-1fA-3@gated-at.bofh.it> |
| In reply to | #1520355 |
On Sat, Nov 12, 2016 at 8:08 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> Gotcha. I don't see any checks that the saddr is valid similar to what IPv4 does.
>>
>> I think the right place to add a check is in ip6_dst_lookup_tail():
>> if (!ipv6_addr_any(&fl6->saddr)) {
>> // saddr is valid for L3 domain
>> }
>
> Right. It should probably do the check here, and return
> ERR_PTR(-EINVAL), the same as the v4 version, so that ret codes can be
> checked consistently.
>
> Thanks for looking into this. If you're backed up and would like me to
> submit a patch, just let me know, and I'll give it my best shot.
In perusing through the v6 FIB code, I don't even see an analog of
__ip_dev_find... Hm?
[toc] | [prev] | [next] | [standalone]
| From | Hannes Frederic Sowa <hannes@stressinduktion.org> |
|---|---|
| Date | 2016-11-13 02:00 +0100 |
| Message-ID | <sCRjj-1iT-5@gated-at.bofh.it> |
| In reply to | #1520444 |
On Sun, Nov 13, 2016, at 01:43, Jason A. Donenfeld wrote:
> On Sat, Nov 12, 2016 at 8:08 PM, Jason A. Donenfeld <Jason@zx2c4.com>
> wrote:
> >> Gotcha. I don't see any checks that the saddr is valid similar to what IPv4 does.
> >>
> >> I think the right place to add a check is in ip6_dst_lookup_tail():
> >> if (!ipv6_addr_any(&fl6->saddr)) {
> >> // saddr is valid for L3 domain
> >> }
> >
> > Right. It should probably do the check here, and return
> > ERR_PTR(-EINVAL), the same as the v4 version, so that ret codes can be
> > checked consistently.
> >
> > Thanks for looking into this. If you're backed up and would like me to
> > submit a patch, just let me know, and I'll give it my best shot.
>
> In perusing through the v6 FIB code, I don't even see an analog of
> __ip_dev_find... Hm?
You probably need some combination of ipv6_chk_addr and/or
ipv6_check_addr_and_flags (where dev can also be NULL). Be careful if a
IFA_HOST or IFA_LINK address switches from one interface to another.
Bye,
Hannes
[toc] | [prev] | [next] | [standalone]
| From | "Jason A. Donenfeld" <Jason@zx2c4.com> |
|---|---|
| Date | 2016-11-13 02:10 +0100 |
| Message-ID | <sCRt0-1B4-3@gated-at.bofh.it> |
| In reply to | #1520445 |
Hi Hannes, On Sun, Nov 13, 2016 at 1:51 AM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > You probably need some combination of ipv6_chk_addr and/or > ipv6_check_addr_and_flags (where dev can also be NULL). Be careful if a > IFA_HOST or IFA_LINK address switches from one interface to another. I can confirm this trick works beautifully: https://git.zx2c4.com/WireGuard/commit/?id=eb65810fc6350c50b42abedd1291b12337d3dc3d I'll see if I can fold this into the routing function so that it behaves the same as v4, unless David gets there first. Regards, Jason
[toc] | [prev] | [next] | [standalone]
| From | "Jason A. Donenfeld" <Jason@zx2c4.com> |
|---|---|
| Date | 2016-11-13 14:40 +0100 |
| Subject | [PATCH] ip6_output: ensure flow saddr actually belongs to device |
| Message-ID | <sD3aN-Px-17@gated-at.bofh.it> |
| In reply to | #1520447 |
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>
---
net/ipv6/ip6_output.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 6001e78..a834129 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1011,6 +1011,11 @@ static int ip6_dst_lookup_tail(struct net *net, const struct sock *sk,
}
}
#endif
+ if (!ipv6_addr_any(&fl6->saddr) &&
+ !ipv6_chk_addr(net, &fl6->saddr, (*dst)->dev, 1)) {
+ err = -EINVAL;
+ goto out_err_release;
+ }
return 0;
--
2.10.2
[toc] | [prev] | [next] | [standalone]
| From | David Ahern <dsa@cumulusnetworks.com> |
|---|---|
| Date | 2016-11-13 17:40 +0100 |
| Subject | Re: [PATCH] ip6_output: ensure flow saddr actually belongs to device |
| Message-ID | <sD5Z5-2JN-35@gated-at.bofh.it> |
| In reply to | #1520602 |
On 11/13/16 6:23 AM, 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>
> ---
> net/ipv6/ip6_output.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 6001e78..a834129 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -1011,6 +1011,11 @@ static int ip6_dst_lookup_tail(struct net *net, const struct sock *sk,
> }
> }
> #endif
> + if (!ipv6_addr_any(&fl6->saddr) &&
> + !ipv6_chk_addr(net, &fl6->saddr, (*dst)->dev, 1)) {
> + err = -EINVAL;
> + goto out_err_release;
> + }
>
> return 0;
You can't require the address to be on the dst device. e.g., it can be an address from the loopback/vrf device.
This block needs to be done at function entry, and pass dev as NULL to mean is the address assigned to any interface. That gets you the equivalency of the IPv4 check.
[toc] | [prev] | [next] | [standalone]
| From | "Jason A. Donenfeld" <Jason@zx2c4.com> |
|---|---|
| Date | 2016-11-13 20:10 +0100 |
| Subject | [PATCH v2] ip6_output: ensure flow saddr actually belongs to device |
| Message-ID | <sD8k9-4t2-3@gated-at.bofh.it> |
| In reply to | #1520622 |
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 v1:
This moves the check to the top and now sees if it's a valid address
on _any_ device, not just the one in dst.
include/net/ipv6.h | 2 ++
net/ipv6/ip6_output.c | 28 ++++++++++++++++++++++++++++
2 files changed, 30 insertions(+)
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 8fed1cd..e5dc14f 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -914,6 +914,8 @@ struct dst_entry *ip6_sk_dst_lookup_flow(struct sock *sk, struct flowi6 *fl6,
const struct in6_addr *final_dst);
struct dst_entry *ip6_blackhole_route(struct net *net,
struct dst_entry *orig_dst);
+struct net_device *__ip6_dev_find(struct net *net, struct in6_addr *addr,
+ bool devref);
/*
* skb processing functions
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 6001e78..371170b 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -916,6 +916,30 @@ static struct dst_entry *ip6_sk_dst_check(struct sock *sk,
return dst;
}
+/**
+ * __ip6_dev_find - find the first device with a given source address.
+ * @net: the net namespace
+ * @addr: the source address
+ * @devref: if true, take a reference on the found device
+ *
+ * If a caller uses devref=false, it should be protected by RCU, or RTNL
+ */
+struct net_device *__ip6_dev_find(struct net *net, struct in6_addr *addr, bool devref)
+{
+ struct net_device *result;
+
+ rcu_read_lock();
+ for_each_netdev_rcu(net, result) {
+ if (ipv6_chk_addr(net, addr, result, 1))
+ break;
+ }
+ if (result && devref)
+ dev_hold(result);
+ rcu_read_unlock();
+ return result;
+}
+EXPORT_SYMBOL(__ip6_dev_find);
+
static int ip6_dst_lookup_tail(struct net *net, const struct sock *sk,
struct dst_entry **dst, struct flowi6 *fl6)
{
@@ -926,6 +950,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) &&
+ !__ip6_dev_find(net, &fl6->saddr, false))
+ 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
[toc] | [prev] | [next] | [standalone]
| From | David Ahern <dsa@cumulusnetworks.com> |
|---|---|
| Date | 2016-11-13 21:50 +0100 |
| Subject | Re: [PATCH v2] ip6_output: ensure flow saddr actually belongs to device |
| Message-ID | <sD9SV-5kR-17@gated-at.bofh.it> |
| In reply to | #1520647 |
On 11/13/16 12:02 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 <Jason@zx2c4.com>
> Cc: David Ahern <dsa@cumulusnetworks.com>
> ---
> Changes from v1:
> This moves the check to the top and now sees if it's a valid address
> on _any_ device, not just the one in dst.
>
> include/net/ipv6.h | 2 ++
> net/ipv6/ip6_output.c | 28 ++++++++++++++++++++++++++++
> 2 files changed, 30 insertions(+)
>
> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> index 8fed1cd..e5dc14f 100644
> --- a/include/net/ipv6.h
> +++ b/include/net/ipv6.h
> @@ -914,6 +914,8 @@ struct dst_entry *ip6_sk_dst_lookup_flow(struct sock *sk, struct flowi6 *fl6,
> const struct in6_addr *final_dst);
> struct dst_entry *ip6_blackhole_route(struct net *net,
> struct dst_entry *orig_dst);
> +struct net_device *__ip6_dev_find(struct net *net, struct in6_addr *addr,
> + bool devref);
>
> /*
> * skb processing functions
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 6001e78..371170b 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -916,6 +916,30 @@ static struct dst_entry *ip6_sk_dst_check(struct sock *sk,
> return dst;
> }
>
> +/**
> + * __ip6_dev_find - find the first device with a given source address.
> + * @net: the net namespace
> + * @addr: the source address
> + * @devref: if true, take a reference on the found device
> + *
> + * If a caller uses devref=false, it should be protected by RCU, or RTNL
> + */
> +struct net_device *__ip6_dev_find(struct net *net, struct in6_addr *addr, bool devref)
> +{
> + struct net_device *result;
> +
> + rcu_read_lock();
> + for_each_netdev_rcu(net, result) {
> + if (ipv6_chk_addr(net, addr, result, 1))
> + break;
> + }
> + if (result && devref)
> + dev_hold(result);
> + rcu_read_unlock();
> + return result;
> +}
> +EXPORT_SYMBOL(__ip6_dev_find);
You don't need a new function to walk all interfaces; just use ipv6_chk_addr with a dev arg of NULL. IPv6 has a hash table with all unicast addresses -- inet6_addr_lst. ipv6_chk_addr is checking that list for the address in question. The actual device is not relevant for verifying the address is a valid local one (though the device can be returned from ifp->idev->dev if ever needed).
So drop the above ...
> +
> static int ip6_dst_lookup_tail(struct net *net, const struct sock *sk,
> struct dst_entry **dst, struct flowi6 *fl6)
> {
> @@ -926,6 +950,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) &&
> + !__ip6_dev_find(net, &fl6->saddr, false))
... and just use ipv6_chk_addr here.
> + 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
>
[toc] | [prev] | [next] | [standalone]
| From | "Jason A. Donenfeld" <Jason@zx2c4.com> |
|---|---|
| Date | 2016-11-14 00:30 +0100 |
| Subject | [PATCH v3] ip6_output: ensure flow saddr actually belongs to device |
| Message-ID | <sDcnM-73M-9@gated-at.bofh.it> |
| In reply to | #1520664 |
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;
+
/* 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
[toc] | [prev] | [next] | [standalone]
| From | David Ahern <dsa@cumulusnetworks.com> |
|---|---|
| Date | 2016-11-14 17:20 +0100 |
| Subject | Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to device |
| Message-ID | <sDs9b-RG-5@gated-at.bofh.it> |
| In reply to | #1520721 |
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 <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; > + > /* 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 <dsa@cumulusnetworks.com>
[toc] | [prev] | [next] | [standalone]
| From | Hannes Frederic Sowa <hannes@stressinduktion.org> |
|---|---|
| Date | 2016-11-14 17:50 +0100 |
| Subject | Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to device |
| Message-ID | <sDsCe-12v-21@gated-at.bofh.it> |
| In reply to | #1520721 |
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. 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? Thanks, Hannes
[toc] | [prev] | [next] | [standalone]
| From | David Ahern <dsa@cumulusnetworks.com> |
|---|---|
| Date | 2016-11-14 18:00 +0100 |
| Subject | Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to device |
| Message-ID | <sDsLU-16h-21@gated-at.bofh.it> |
| In reply to | #1521841 |
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. > > 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.
[toc] | [prev] | [next] | [standalone]
| From | Hannes Frederic Sowa <hannes@stressinduktion.org> |
|---|---|
| Date | 2016-11-14 18:10 +0100 |
| Subject | Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to device |
| Message-ID | <sDsVz-1pb-9@gated-at.bofh.it> |
| In reply to | #1521860 |
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
[toc] | [prev] | [next] | [standalone]
| From | David Ahern <dsa@cumulusnetworks.com> |
|---|---|
| Date | 2016-11-14 18:20 +0100 |
| Subject | Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to device |
| Message-ID | <sDt5g-1sT-15@gated-at.bofh.it> |
| In reply to | #1521871 |
On 11/14/16 10:04 AM, Hannes Frederic Sowa wrote: > 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. so you mean if rt6_need_strict(&fl6->saddr) then the dev needs to be considered. > >>> 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.
[toc] | [prev] | [next] | [standalone]
| From | Hannes Frederic Sowa <hannes@stressinduktion.org> |
|---|---|
| Date | 2016-11-14 18:40 +0100 |
| Subject | Re: [PATCH v3] ip6_output: ensure flow saddr actually belongs to device |
| Message-ID | <sDtoB-1A0-5@gated-at.bofh.it> |
| In reply to | #1521878 |
On 14.11.2016 18:17, David Ahern wrote: > On 11/14/16 10:04 AM, Hannes Frederic Sowa wrote: >> 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. > > so you mean if rt6_need_strict(&fl6->saddr) then the dev needs to be considered. Exactly, like we do in the user space facing APIs. >>>> 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. ;) Bye, Hannes
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web