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: <77c9a31ba08bcc472617c08c0542cd82f7959a58.camel@redhat.com>
Date:   Wed, 06 Jul 2022 18:21:05 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     David Ahern <dsahern@...nel.org>,
        Richard Gobert <richardbgobert@...il.com>, davem@...emloft.net,
        yoshfuji@...ux-ipv6.org, kuba@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH] net: Fix IP_UNICAST_IF option behavior for connected
 sockets

On Wed, 2022-07-06 at 09:14 -0600, David Ahern wrote:
> 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.

I think your reasoning is correct, and I'm now ok with the patch. Jakub
noted it does not apply cleanly, so a repost will be needed.
Additionally it would be great to include some self-tests.

It looks like the feature (even the original one, I mean) is IPv4
specific, don't you need an IPv6 counter-part?

Thanks!

Paolo