netdev - Re: [PATCH net] net: diag: Fix swapped src/dst in udp_dump

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 for Android: free password hash cracker in your pocket

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

Message-Id: <20180914.083038.452831271321088318.davem@davemloft.net>
Date:   Fri, 14 Sep 2018 08:30:38 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     lorenzo@...gle.com
Cc:     netdev@...r.kernel.org, zenczykowski@...il.com, dsahern@...il.com,
        jeffv@...gle.com
Subject: Re: [PATCH net] net: diag: Fix swapped src/dst in udp_dump_one.

From: Lorenzo Colitti <lorenzo@...gle.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@...gle.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.