[PATCH net] net: diag: Fix swapped src/dst in udp_dump_one.

[PATCH net] net: diag: Fix swapped src/dst in udp_dump_one.

[Lorenzo Colitti]

Since its inception, udp_dump_one had has a bug where userspace
needs to swap src and dst addresses and ports in order to find
the socket it wants.

This is because udp_dump_one misuses __udp[46]_lib_lookup by
passing the source address as the source address argument.
Unfortunately, those functions are intended to find local sockets
matching received packets, so the order of the arguments is
inverted: the argument that ends up being compared with, e.g.,
sk_daddr is actually saddr, not daddr.

While it's true that this creates a backwards compatibility
problem, this is clearly a bug since inet_diag_sockid is very
clear about which struct elements are the source address and port
and which are the destination address and port. Also, this bug
does not affect TCP sockets, SOCK_DESTROY of UDP sockets, or
finding UDP sockets with NLMSG_DUMP.

Fixes: a925aa00a55 ("udp_diag: Implement the get_exact dumping functionality")
Tested: https://android-review.googlesource.com/c/kernel/tests/+/755889/
Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
---
 net/ipv4/udp_diag.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/udp_diag.c b/net/ipv4/udp_diag.c
index d9ad986c7b..e1c6f90a92 100644
--- a/net/ipv4/udp_diag.c
+++ b/net/ipv4/udp_diag.c
@@ -43,16 +43,16 @@ static int udp_dump_one(struct udp_table *tbl, struct sk_buff *in_skb,
 	rcu_read_lock();
 	if (req->sdiag_family == AF_INET)
 		sk = __udp4_lib_lookup(net,
-				req->id.idiag_src[0], req->id.idiag_sport,
 				req->id.idiag_dst[0], req->id.idiag_dport,
+				req->id.idiag_src[0], req->id.idiag_sport,
 				req->id.idiag_if, 0, tbl, NULL);
 #if IS_ENABLED(CONFIG_IPV6)
 	else if (req->sdiag_family == AF_INET6)
 		sk = __udp6_lib_lookup(net,
-				(struct in6_addr *)req->id.idiag_src,
-				req->id.idiag_sport,
 				(struct in6_addr *)req->id.idiag_dst,
 				req->id.idiag_dport,
+				(struct in6_addr *)req->id.idiag_src,
+				req->id.idiag_sport,
 				req->id.idiag_if, 0, tbl, NULL);
 #endif
 	if (sk && !refcount_inc_not_zero(&sk->sk_refcnt))
-- 
2.19.0.397.gdd90340f6a-goog

Re: [PATCH net] net: diag: Fix swapped src/dst in udp_dump_one.

[David Miller]

From: Lorenzo Colitti <lorenzo@google.com>
Date: Fri, 14 Sep 2018 15:25:53 +0900

> Since its inception, udp_dump_one had has a bug where userspace
> needs to swap src and dst addresses and ports in order to find
> the socket it wants.
> 
> This is because udp_dump_one misuses __udp[46]_lib_lookup by
> passing the source address as the source address argument.
> Unfortunately, those functions are intended to find local sockets
> matching received packets, so the order of the arguments is
> inverted: the argument that ends up being compared with, e.g.,
> sk_daddr is actually saddr, not daddr.
> 
> While it's true that this creates a backwards compatibility
> problem, this is clearly a bug since inet_diag_sockid is very
> clear about which struct elements are the source address and port
> and which are the destination address and port. Also, this bug
> does not affect TCP sockets, SOCK_DESTROY of UDP sockets, or
> finding UDP sockets with NLMSG_DUMP.
> 
> Fixes: a925aa00a55 ("udp_diag: Implement the get_exact dumping functionality")
> Tested: https://android-review.googlesource.com/c/kernel/tests/+/755889/
> Signed-off-by: Lorenzo Colitti <lorenzo@google.com>

Unfortunately I think we are stuck with how things are now.

Indisputably, your patch breaks userland components that have
workarounds in order to work with existing kernels.  People who
wrote such code:

1) Won't get any warnings that things are about to break on them

2) Will have limited options to have their code work on all kernels,
   ones that have this change and ones that do not.

Maybe if this got introduced 1 or 2 releases ago we could consider
doing this, but all the way back to v3.3?  No way.

I cannot apply this, sorry.

Re: [PATCH net] net: diag: Fix swapped src/dst in udp_dump_one.

[Lorenzo Colitti]

On Fri, Sep 14, 2018 at 11:30 PM David Miller <davem@davemloft.net> wrote:
> Unfortunately I think we are stuck with how things are now.
>
> Indisputably, your patch breaks userland components that have
> workarounds in order to work with existing kernels.  [...]
> I cannot apply this, sorry.

Understood. We're about to start using this UDP codepath on Android,
so I wanted to see if this would be fixed before we commit to the
current behaviour in our conformance tests, at which point we won't
ever be able to change it either.

Would you take a patch to add a one-line comment saying that this is
the way it is for backwards compatibility? If that comment were there
anyone else who finds this will not spend time debugging it and
immediately know what's going on. The fact that the ports are inverted
is not easy to spot on casual inspection.

Re: [PATCH net] net: diag: Fix swapped src/dst in udp_dump_one.

[David Miller]

From: Lorenzo Colitti <lorenzo@google.com>
Date: Fri, 21 Sep 2018 18:46:25 +0800

> Would you take a patch to add a one-line comment saying that this is
> the way it is for backwards compatibility? If that comment were there
> anyone else who finds this will not spend time debugging it and
> immediately know what's going on. The fact that the ports are inverted
> is not easy to spot on casual inspection.

Sure.