Re: [PATCH nf,v5] netfilter: arp_tables: fix IEEE1394 ARP payload parsing

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Florian,

I am tossing this approach and getting back to the approach that
checks skb->dev, arp_process comes _after_ NF_HOOK_ARP_IN.

On Tue, Apr 21, 2026 at 08:35:14PM +0200, Pablo Neira Ayuso wrote:
> Weiming Shi says:
> 
> "arp_packet_match() unconditionally parses the ARP payload assuming two
> hardware addresses are present (source and target). However,
> IPv4-over-IEEE1394 ARP (RFC 2734) omits the target hardware address
> field, and arp_hdr_len() already accounts for this by returning a
> shorter length for ARPHRD_IEEE1394 devices.
> 
> As a result, on IEEE1394 interfaces arp_packet_match() advances past a
> nonexistent target hardware address and reads the wrong bytes for both
> the target device address comparison and the target IP address. This
> causes arptables rules to match against garbage data, leading to
> incorrect filtering decisions: packets that should be accepted may be
> dropped and vice versa.
> 
> The ARP stack in net/ipv4/arp.c (arp_create and arp_process) already
> handles this correctly by skipping the target hardware address for
> ARPHRD_IEEE1394. Apply the same pattern to arp_packet_match()."
> 
> This patch always returns 0 (no match) in case user matches on the target
> hardware address which is never present in IEEE1394.
> 
> Note that this returns 0 (no match) for either normal and inverse match
> because matching in the target hardware address in ARPHRD_IEEE1394 has
> never been supported by arptables. This is intentional, matching on the
> target hardware address should never evaluate true for ARPHRD_IEEE1394.
> 
> Moreover, adjust arpt_mangle to drop the packet if user tries to mangle
> target hardware and IP address in IEEE1394, this has never been
> supported.
> 
> Fixes: 6752c8db8e0c ("firewire net, ipv4 arp: Extend hardware address and remove driver-level packet inspection.")
> Reported-by: Xiang Mei <xmei5@asu.edu>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> v5: check for arphdr->ar_hrd == htons(ARPHRD_IEEE1394) in
>     arp_packet_match() too.
> 
>  net/ipv4/netfilter/arp_tables.c  | 19 ++++++++++++++++---
>  net/ipv4/netfilter/arpt_mangle.c |  8 ++++++++
>  2 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
> index 1cdd9c28ab2d..e4b2106d0456 100644
> --- a/net/ipv4/netfilter/arp_tables.c
> +++ b/net/ipv4/netfilter/arp_tables.c
> @@ -110,13 +110,26 @@ static inline int arp_packet_match(const struct arphdr *arphdr,
>  	arpptr += dev->addr_len;
>  	memcpy(&src_ipaddr, arpptr, sizeof(u32));
>  	arpptr += sizeof(u32);
> -	tgt_devaddr = arpptr;
> -	arpptr += dev->addr_len;
> +
> +	if (IS_ENABLED(CONFIG_FIREWIRE_NET) &&
> +	    arphdr->ar_hrd == htons(ARPHRD_IEEE1394)) {
> +		if (unlikely(memchr_inv(arpinfo->tgt_devaddr.mask, 0,
> +					sizeof(arpinfo->tgt_devaddr.mask))))
> +			return 0;
> +
> +		tgt_devaddr = NULL;
> +	} else {
> +		tgt_devaddr = arpptr;
> +		arpptr += dev->addr_len;
> +	}
>  	memcpy(&tgt_ipaddr, arpptr, sizeof(u32));
>  
>  	if (NF_INVF(arpinfo, ARPT_INV_SRCDEVADDR,
>  		    arp_devaddr_compare(&arpinfo->src_devaddr, src_devaddr,
> -					dev->addr_len)) ||
> +					dev->addr_len)))
> +		return 0;
> +
> +	if (tgt_devaddr &&
>  	    NF_INVF(arpinfo, ARPT_INV_TGTDEVADDR,
>  		    arp_devaddr_compare(&arpinfo->tgt_devaddr, tgt_devaddr,
>  					dev->addr_len)))
> diff --git a/net/ipv4/netfilter/arpt_mangle.c b/net/ipv4/netfilter/arpt_mangle.c
> index a4e07e5e9c11..285b1123b05c 100644
> --- a/net/ipv4/netfilter/arpt_mangle.c
> +++ b/net/ipv4/netfilter/arpt_mangle.c
> @@ -40,6 +40,10 @@ target(struct sk_buff *skb, const struct xt_action_param *par)
>  	}
>  	arpptr += pln;
>  	if (mangle->flags & ARPT_MANGLE_TDEV) {
> +		if (IS_ENABLED(CONFIG_FIREWIRE_NET) &&
> +		    arp->ar_hrd == htons(ARPHRD_IEEE1394))
> +			return NF_DROP;
> +
>  		if (ARPT_DEV_ADDR_LEN_MAX < hln ||
>  		   (arpptr + hln > skb_tail_pointer(skb)))
>  			return NF_DROP;
> @@ -47,6 +51,10 @@ target(struct sk_buff *skb, const struct xt_action_param *par)
>  	}
>  	arpptr += hln;
>  	if (mangle->flags & ARPT_MANGLE_TIP) {
> +		if (IS_ENABLED(CONFIG_FIREWIRE_NET) &&
> +		    arp->ar_hrd == htons(ARPHRD_IEEE1394))
> +			return NF_DROP;
> +
>  		if (ARPT_MANGLE_ADDR_LEN_MAX < pln ||
>  		   (arpptr + pln > skb_tail_pointer(skb)))
>  			return NF_DROP;
> -- 
> 2.47.3
> 
>