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:   Tue, 20 Apr 2021 11:35:54 +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:

> Hi,
>
> Thanks for your comments.
>
>> >  	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?
>
> This is what eth_type_trans() uses below, so I assumed it is safe to
> use. Isn't that working on the same data?
>
> Also, the destination address in Ethernet is followed by the source
> address, so two extra bytes in the source are used as padding. These
> are then shifted out by ether_addr_equal_64bits(), no?

Ohh, you're right, it's shifting off the two extra bytes afterwards.
Clever! I obviously missed that, but yeah, that means it just needs the
two extra bytes to not be out-of-bounds reads, so this usage should be
fine :)

>> > +		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 :)
>
> Indeed. I considered changing eth_type_trans() to always reset
> pkt_type, but I didn't want to take the risk for any side effects.

Hmm, yeah, it does seem there are quite a few call sites to audit if you
were to change the behaviour. I guess we'll have to live with the slight
confusion, then :)

-Toke


Given the above:

Acked-by: Toke Høiland-Jørgensen <toke@...hat.com>