netdev - Re: [PATCH net] net: loopback: Avoid sending IP packets without an Ethernet header

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: <CANn89iKnRBQipCOLtFq-rVPOoaZ7M6tYVo+Fm1aYgCZnyqW=eg@mail.gmail.com>
Date: Thu, 20 Feb 2025 11:40:07 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Ido Schimmel <idosch@...dia.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org, 
	pabeni@...hat.com, andrew+netdev@...n.ch, maheshb@...gle.com, 
	lucien.xin@...il.com, fmei@....com
Subject: Re: [PATCH net] net: loopback: Avoid sending IP packets without an
 Ethernet header

On Thu, Feb 20, 2025 at 8:26 AM Ido Schimmel <idosch@...dia.com> wrote:
>
> After commit 22600596b675 ("ipv4: give an IPv4 dev to blackhole_netdev")
> IPv4 neighbors can be constructed on the blackhole net device, but they
> are constructed with an output function (neigh_direct_output()) that
> simply calls dev_queue_xmit(). The latter will transmit packets via
> 'skb->dev' which might not be the blackhole net device if dst_dev_put()
> switched 'dst->dev' to the blackhole net device while another CPU was
> using the dst entry in ip_output(), but after it already initialized
> 'skb->dev' from 'dst->dev'.
>
> Specifically, the following can happen:
>
>     CPU1                                      CPU2
>
> udp_sendmsg(sk1)                          udp_sendmsg(sk2)
> udp_send_skb()                            [...]
> ip_output()
>     skb->dev = skb_dst(skb)->dev
>                                           dst_dev_put()
>                                               dst->dev = blackhole_netdev
> ip_finish_output2()
>     resolves neigh on dst->dev
> neigh_output()
> neigh_direct_output()
> dev_queue_xmit()
>
> This will result in IPv4 packets being sent without an Ethernet header
> via a valid net device:
>
> tcpdump: verbose output suppressed, use -v[v]... for full protocol decode
> listening on enp9s0, link-type EN10MB (Ethernet), snapshot length 262144 bytes
> 22:07:02.329668 20:00:40:11:18:fb > 45:00:00:44:f4:94, ethertype Unknown
> (0x58c6), length 68:
>         0x0000:  8dda 74ca f1ae ca6c ca6c 0098 969c 0400  ..t....l.l......
>         0x0010:  0000 4730 3f18 6800 0000 0000 0000 9971  ..G0?.h........q
>         0x0020:  c4c9 9055 a157 0a70 9ead bf83 38ca ab38  ...U.W.p....8..8
>         0x0030:  8add ab96 e052                           .....R
>
> Fix by making sure that neighbors are constructed on top of the
> blackhole net device with an output function that simply consumes the
> packets, in a similar fashion to dst_discard_out() and
> blackhole_netdev_xmit().
>
> Fixes: 8d7017fd621d ("blackhole_netdev: use blackhole_netdev to invalidate dst entries")
> Fixes: 22600596b675 ("ipv4: give an IPv4 dev to blackhole_netdev")
> Reported-by: Florian Meister <fmei@....com>
> Closes: https://lore.kernel.org/netdev/20250210084931.23a5c2e4@hermes.local/
> Signed-off-by: Ido Schimmel <idosch@...dia.com>
> ---
>  drivers/net/loopback.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
> index c8840c3b9a1b..f1d68153987e 100644
> --- a/drivers/net/loopback.c
> +++ b/drivers/net/loopback.c
> @@ -244,8 +244,22 @@ static netdev_tx_t blackhole_netdev_xmit(struct sk_buff *skb,
>         return NETDEV_TX_OK;
>  }
>
> +static int blackhole_neigh_output(struct neighbour *n, struct sk_buff *skb)
> +{
> +       kfree_skb(skb);

If there is any risk of this being hit often, I would probably use the
recent SKB_DROP_REASON_BLACKHOLE

(feel free to resubmit
https://lore.kernel.org/netdev/20250212164323.2183023-1-edumazet@google.com/T/#mbb8d4b0779cb8f0654a382772c943af5389606ea
?)

Otherwise, this looks good to me, thanks !

Reviewed-by: Eric Dumazet <edumazet@...gle.com>