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

[PATCH v2] 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.
Do not look through inline VLAN tags, because act_nat does not adjust
the network header offset before using ip_hdr(skb).

Assisted-by: Codex:gpt-5.5-cyber-preview
Signed-off-by: Samuel Moelius <sam.moelius@trailofbits.com>
---
Changes in v2
  - don't corrupt VLAN tags

 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..cb3e7a415da7 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, false) != htons(ETH_P_IP))
+		goto out;
+
 	if (!pskb_may_pull(skb, sizeof(*iph) + noff))
 		goto drop;
 
-- 
2.43.0

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

[Jakub Kicinski]

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

This commit restricts act_nat to only process IPv4 packets to prevent
non-IPv4 payloads from being incorrectly parsed and modified as IPv4
headers. It adds a protocol check using skb_protocol() to skip inline
VLAN tags.

> diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
> index abb332dee836d..cb3e7a415da74 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, false) != htons(ETH_P_IP))
> +		goto out;
> +

[Severity: High]
Will this check skip valid IPv4 packets that have hardware-accelerated VLAN
tags?

When a packet has a hardware-accelerated VLAN tag, skb_vlan_tag_present(skb)
is true. In this case, skb_protocol(skb, false) evaluates to skb->vlan_proto
(such as ETH_P_8021Q), causing the check to evaluate to true and jump to the
out label.

Since hardware-accelerated VLAN tags are stripped by the MAC driver,
skb_network_offset(skb) correctly points to the inner IPv4 header and
skb->protocol already holds htons(ETH_P_IP). Should this code check
skb->protocol != htons(ETH_P_IP) directly instead to avoid skipping
hardware-VLAN-tagged IPv4 packets?

>  	if (!pskb_may_pull(skb, sizeof(*iph) + noff))
>  		goto drop;
>
-- 
pw-bot: cr