netdev - Re: [PATCH] net: Fix IP_UNICAST_IF option behavior for connected sockets

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <7be18dc0-4d2c-283d-eedb-123ab99197d3@kernel.org>
Date:   Wed, 6 Jul 2022 09:14:17 -0600
From:   David Ahern <dsahern@...nel.org>
To:     Richard Gobert <richardbgobert@...il.com>, davem@...emloft.net,
        yoshfuji@...ux-ipv6.org, kuba@...nel.org, pabeni@...hat.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH] net: Fix IP_UNICAST_IF option behavior for connected
 sockets

On 6/27/22 2:52 AM, Richard Gobert wrote:
> The IP_UNICAST_IF socket option is used to set the outgoing interface for
> outbound packets.
> The IP_UNICAST_IF socket option was added as it was needed by the Wine
> project, since no other existing option (SO_BINDTODEVICE socket option,
> IP_PKTINFO socket option or the bind function) provided the needed
> characteristics needed by the IP_UNICAST_IF socket option. [1]
> The IP_UNICAST_IF socket option works well for unconnected sockets, that
> is, the interface specified by the IP_UNICAST_IF socket option is taken
> into consideration in the route lookup process when a packet is being
> sent.
> However, for connected sockets, the outbound interface is chosen when
> connecting the socket, and in the route lookup process which is done when
> a packet is being sent, the interface specified by the IP_UNICAST_IF
> socket option is being ignored.
> 
> This inconsistent behavior was reported and discussed in an issue opened
> on systemd's GitHub project [2]. Also, a bug report was submitted in the
> kernel's bugzilla [3].
> 
> To understand the problem in more detail, we can look at what happens
> for UDP packets over IPv4 (The same analysis was done separately in
> the referenced systemd issue).
> When a UDP packet is sent the udp_sendmsg function gets called and the
> following happens:
> 
> 1. The oif member of the struct ipcm_cookie ipc (which stores the output
> interface of the packet) is initialized by the ipcm_init_sk function to
> inet->sk.sk_bound_dev_if (the device set by the SO_BINDTODEVICE socket
> option).
> 
> 2. If the IP_PKTINFO socket option was set, the oif member gets overridden
> by the call to the ip_cmsg_send function.
> 
> 3. If no output interface was selected yet, the interface specified by the
> IP_UNICAST_IF socket option is used.
> 
> 4. If the socket is connected and no destination address is specified in
> the send function, the struct ipcm_cookie ipc is not taken into
> consideration and the cached route, that was calculated in the connect
> function is being used.
> 
> Thus, for a connected socket, the IP_UNICAST_IF sockopt isn't taken into
> consideration.
> 
> This patch corrects the behavior of the IP_UNICAST_IF socket option for
> connect()ed sockets by taking into consideration the IP_UNICAST_IF sockopt
> when connecting the socket.
> 
> In order to avoid reconnecting the socket, this option is still ignored 
> when applied on an already connected socket until connect() is called
> again by the user.
> 
> Change the __ip4_datagram_connect function, which is called during socket
> connection, to take into consideration the interface set by the
> IP_UNICAST_IF socket option, in a similar way to what is done in the
> udp_sendmsg function.
> 
> [1] https://lore.kernel.org/netdev/1328685717.4736.4.camel@edumazet-laptop/T/
> [2] https://github.com/systemd/systemd/issues/11935#issuecomment-618691018
> [3] https://bugzilla.kernel.org/show_bug.cgi?id=210255
> 
> Signed-off-by: Richard Gobert <richardbgobert@...il.com>
> ---
>  net/ipv4/datagram.c | 2 ++
>  1 file changed, 2 insertions(+)
> 

Reviewed-by: David Ahern <dsahern@...nel.org>

if the maintainers decide to pick it up.