Re: [PATCH] net/sched: act_nat: only rewrite IPv4 packets

[Simon Horman]

From: 'Simon Horman' <horms@xxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net/sched: act_nat: only rewrite IPv4 packets

This commit modifies act_nat to check the packet protocol before parsing and
rewriting the IPv4 header. This prevents the action from modifying non-IPv4
packets based on unrelated bytes at the network header offset.

> diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
> index abb332dee836d..34d6ba2d41432 100644
> --- a/net/sched/act_nat.c
> +++ b/net/sched/act_nat.c
> @@ -142,6 +142,9 @@ TC_INDIRECT_SCOPE int tcf_nat_act(struct sk_buff *skb,
>  	egress = parms->flags & TCA_NAT_FLAG_EGRESS;
>  
>  	noff = skb_network_offset(skb);
> +	if (skb_protocol(skb, true) != htons(ETH_P_IP))
> +		goto out;
> +

[Severity: High]
Does this code parse inline VLAN headers as IPv4 headers?

By explicitly passing true for skip_vlan to skb_protocol(), the check looks
past inline VLAN tags to evaluate the encapsulated inner protocol. If the
inner protocol is IPv4, the check passes, but noff and skb_network_header(skb)
are not advanced past the VLAN tags.

When the action later proceeds to call iph = ip_hdr(skb) at line 151 and
rewrites the IP addresses, won't it silently corrupt the VLAN tag and
adjacent packet bytes instead of the actual IPv4 header?

>  	if (!pskb_may_pull(skb, sizeof(*iph) + noff))
>  		goto drop;
>