netdev - Re: [PATCH net-next] net: xdp: Update pkt_type if generic XDP changes unicast MAC

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]

Date:   Mon, 19 Apr 2021 16:53:20 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Martin Willi <martin@...ongswan.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH net-next] net: xdp: Update pkt_type if generic XDP
 changes unicast MAC

Martin Willi <martin@...ongswan.org> writes:

> If a generic XDP program changes the destination MAC address from/to
> multicast/broadcast, the skb->pkt_type is updated to properly handle
> the packet when passed up the stack. When changing the MAC from/to
> the NICs MAC, PACKET_HOST/OTHERHOST is not updated, though, making
> the behavior different from that of native XDP.
>
> Remember the PACKET_HOST/OTHERHOST state before calling the program
> in generic XDP, and update pkt_type accordingly if the destination
> MAC address has changed. As eth_type_trans() assumes a default
> pkt_type of PACKET_HOST, restore that before calling it.
>
> The use case for this is when a XDP program wants to push received
> packets up the stack by rewriting the MAC to the NICs MAC, for
> example by cluster nodes sharing MAC addresses.
>
> Fixes: 297249569932 ("net: fix generic XDP to handle if eth header was mangled")
> Signed-off-by: Martin Willi <martin@...ongswan.org>
> ---
>  net/core/dev.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d9bf63dbe4fd..eed028aec6a4 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4723,10 +4723,10 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
>  	void *orig_data, *orig_data_end, *hard_start;
>  	struct netdev_rx_queue *rxqueue;
>  	u32 metalen, act = XDP_DROP;
> +	bool orig_bcast, orig_host;
>  	u32 mac_len, frame_sz;
>  	__be16 orig_eth_type;
>  	struct ethhdr *eth;
> -	bool orig_bcast;
>  	int off;
>  
>  	/* Reinjected packets coming from act_mirred or similar should
> @@ -4773,6 +4773,7 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
>  	orig_data_end = xdp->data_end;
>  	orig_data = xdp->data;
>  	eth = (struct ethhdr *)xdp->data;
> +	orig_host = ether_addr_equal_64bits(eth->h_dest, skb->dev->dev_addr);

ether_addr_equal_64bits() seems to assume that the addresses passed to
it are padded to be 8 bytes long, which is not the case for eth->h_dest.
AFAICT the only reason the _64bits variant works for multicast is that
it happens to be only checking the top-most bit, but unless I'm missing
something you'll have to use the boring old ether_addr_equal() here, no?

>  	orig_bcast = is_multicast_ether_addr_64bits(eth->h_dest);
>  	orig_eth_type = eth->h_proto;
>  
> @@ -4800,8 +4801,11 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
>  	/* check if XDP changed eth hdr such SKB needs update */
>  	eth = (struct ethhdr *)xdp->data;
>  	if ((orig_eth_type != eth->h_proto) ||
> +	    (orig_host != ether_addr_equal_64bits(eth->h_dest,
> +						  skb->dev->dev_addr)) ||
>  	    (orig_bcast != is_multicast_ether_addr_64bits(eth->h_dest))) {
>  		__skb_push(skb, ETH_HLEN);
> +		skb->pkt_type = PACKET_HOST;
>  		skb->protocol = eth_type_trans(skb, skb->dev);
>  	}

Okay, so this was a bit confusing to me at fist glance: eth_type_trans()
will reset the type, but not back to PACKET_HOST. So this works, just a
bit confusing :)

-Toke