Re: [PATCH nf] netfilter: arp_tables: fix IEEE1394 ARP payload parsing in arp_packet_match()

[Simon Horman <horms@kernel.org>]

On Wed, Apr 08, 2026 at 03:35:16PM +0800, Weiming Shi wrote:
> 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().
> 
> 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: Weiming Shi <bestswngs@gmail.com>
> ---
>  net/ipv4/netfilter/arp_tables.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
> index 1cdd9c28ab2da..4b2392bdcd0a6 100644
> --- a/net/ipv4/netfilter/arp_tables.c
> +++ b/net/ipv4/netfilter/arp_tables.c
> @@ -86,7 +86,7 @@ static inline int arp_packet_match(const struct arphdr *arphdr,
>  				   const struct arpt_arp *arpinfo)
>  {
>  	const char *arpptr = (char *)(arphdr + 1);
> -	const char *src_devaddr, *tgt_devaddr;
> +	const char *src_devaddr, *tgt_devaddr = NULL;

I think that it's more in keeping with Kernel code practices
to set tgt_devaddr conditionally.

>  	__be32 src_ipaddr, tgt_ipaddr;
>  	long ret;
>  
> @@ -110,13 +110,23 @@ 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;
> +	switch (dev->type) {
> +#if IS_ENABLED(CONFIG_FIREWIRE_NET)
> +	case ARPHRD_IEEE1394:
> +		break;
> +#endif
> +	default:
> +		tgt_devaddr = arpptr;
> +		arpptr += dev->addr_len;
> +		break;
> +	}

While I acknowledge this isn't the approach taken in arp_hdr_len()
I think it would be nicer to use the following construction
which will give build coverage to all paths regardless of if
CONFIG_FIREWIRE_NET is set or not.

	if (IS_ENABLED(CONFIG_FIREWIRE_NET) && dev->type == ARPHRD_IEEE1394) {
		tgt_devaddr = NULL;
	} else {
		tgt_devaddr = arpptr;
		arpptr += dev->addr_len;
	}

Also, I would include a blank line before the if condition.

>  	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)))
> -- 
> 2.43.0
>