Re: [PATCH RFC net 1/5] arp: discard sha bcast/null (bcast ARP poison)

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 22/12/2025 21:37, Marc Sune wrote:
> Missatge de Vadim Fedorenko <vadim.fedorenko@linux.dev> del dia dl.,
> 22 de des. 2025 a les 10:47:
>>
>> On 21/12/2025 21:19, Marc Suñé wrote:
>>>
>>>    /*
>>> + *   For Ethernet devices, Broadcast/Multicast and zero MAC addresses should
>>> + *   never be announced and accepted as sender HW address (prevent BCAST MAC
>>> + *   and NULL ARP poisoning attack).
>>> + */
>>> +     if (dev->addr_len == ETH_ALEN &&
>>
>> dev_type == ARPHRD_ETHER ?
> 
> This is discussed in the cover letter, comments section d). I would
> think more dev_types than that need to check this, at least:
> 
> +       case ARPHRD_ETHER:
> +       case ARPHRD_EETHER:
> +       case ARPHRD_FDDI:
> +       case ARPHRD_IEEE802:
> +       case ARPHRD_IEEE80211:
> 
> but as said, I _think_ it's sufficient to check for HW addrlen == ETH_ALEN.

ARPHRD_EETHER and ARPHRD_IEEE80211 are not really used in the kernel.
For other 3 we already have such case a bit earlier in arp_process(),
it's fine to be aligned with the existing code.

> 
>>
>>
>>> +         (is_broadcast_ether_addr(sha) || is_zero_ether_addr(sha)))
>>
>> RFC says that neither broadcast, nor multicast must be believed. You
>> check for broadcast only. The better check would be:
>>
>> !is_unicast_ether_addr(sha)
> 
> This is discussed in the cover letter, comments section b). In short,
> some NLBs announce MCAST MAC addresses.
> 
> Mind the context there, but I think it's safe. This is applicable to
> ARP and NDP, so I would suggest to follow up there.
> 
> Btw, the is_zero_ether_addr(addr) check is still needed.
> is_unicast_ether_addr(addr) is implemented as
> !is_multicast_ether_addr(addr), and the NULL mac (00:00:00:00:00:00)
> doesn't have the "MCAST bit" set to 1.

Yeah, "!is_valid_ether_addr(sha)" is better in this case.

> 
>>
>>> +             goto out_free_skb;
>>> +
>>> + /*
>>>     *     Special case: We must set Frame Relay source Q.922 address
>>>     */
>>>        if (dev_type == ARPHRD_DLCI)
>>