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

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

[Samuel Moelius]

act_nat can process packets whose protocol is not IPv4 and then
interpret the payload as an IPv4 header.  Non-IPv4 packets may be
modified based on unrelated bytes at the network header offset.

The action is documented as IPv4 NAT and should leave other protocols
alone.

Check the packet protocol before parsing and rewriting the IPv4 header.

Assisted-by: Codex:gpt-5.5-cyber-preview
Signed-off-by: Samuel Moelius <sam.moelius@trailofbits.com>
---
 net/sched/act_nat.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index abb332dee836..34d6ba2d4143 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;
+
 	if (!pskb_may_pull(skb, sizeof(*iph) + noff))
 		goto drop;
 
-- 
2.43.0

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

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

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;
>