[PATCH net] net/sched: act_ife: validate metadata length before decoding

[PATCH net] net/sched: act_ife: validate metadata length before decoding

[Fang Xieyan]

skbmark_decode(), skbprio_decode() and skbtcindex_decode() read fixed-size
values from the TLV payload without validating its length.

A malformed IFE frame can declare a shorter payload, causing the decoders
to consume bytes beyond the declared metadata value:

[TLV type=IFE_META_SKBMARK len=4]
-> dlen == 0, but decode reads 4 bytes

The decoder may therefore set skb metadata from unintended input.

Validate the payload length before decoding and return -EINVAL for
invalid lengths. The caller treats decoder failures as per-TLV errors
and continues decoding the remaining metadata.

Fixes: 084e2f6566d2 ("Support to encoding decoding skb mark on IFE action")
Fixes: 200e10f46936 ("Support to encoding decoding skb prio on IFE action")
Fixes: 408fbc22ef1e ("net sched ife action: Introduce skb tcindex metadata encap decap")
Assisted-by: Hawkeye:GLM-5.3-flash
Assisted-by: Qoder:Qwen3.8-Max
Signed-off-by: Fang Xieyan <fangxy@xiaopeng.com>
---

Found by auditing the IFE decode path at v6.18-rc7. The malformed
metadata cases were reproduced with a userspace sanitizer model of
the decode path, which reported reads of size 4/2 for dlen values
smaller than the expected metadata size.

Compile-tested on x86_64 with defconfig and NET_ACT_IFE=y: act_ife.o
and the three act_meta_*.o build warning-free.

 net/sched/act_meta_mark.c       | 6 +++++-
 net/sched/act_meta_skbprio.c    | 6 +++++-
 net/sched/act_meta_skbtcindex.c | 6 +++++-
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/net/sched/act_meta_mark.c b/net/sched/act_meta_mark.c
index ea0573cb8b2d..3a7d568df0da 100644
--- a/net/sched/act_meta_mark.c
+++ b/net/sched/act_meta_mark.c
@@ -28,8 +28,12 @@ static int skbmark_encode(struct sk_buff *skb, void *skbdata,
 
 static int skbmark_decode(struct sk_buff *skb, void *data, u16 len)
 {
-	u32 ifemark = *(u32 *)data;
+	u32 ifemark;
 
+	if (len != sizeof(ifemark))
+		return -EINVAL;
+
+	ifemark = *(u32 *)data;
 	skb->mark = ntohl(ifemark);
 	return 0;
 }
diff --git a/net/sched/act_meta_skbprio.c b/net/sched/act_meta_skbprio.c
index 2df3133ce5ad..fa4ba4269ca0 100644
--- a/net/sched/act_meta_skbprio.c
+++ b/net/sched/act_meta_skbprio.c
@@ -33,8 +33,12 @@ static int skbprio_encode(struct sk_buff *skb, void *skbdata,
 
 static int skbprio_decode(struct sk_buff *skb, void *data, u16 len)
 {
-	u32 ifeprio = *(u32 *)data;
+	u32 ifeprio;
 
+	if (len != sizeof(ifeprio))
+		return -EINVAL;
+
+	ifeprio = *(u32 *)data;
 	skb->priority = ntohl(ifeprio);
 	return 0;
 }
diff --git a/net/sched/act_meta_skbtcindex.c b/net/sched/act_meta_skbtcindex.c
index 44547caead46..48f001ada8f6 100644
--- a/net/sched/act_meta_skbtcindex.c
+++ b/net/sched/act_meta_skbtcindex.c
@@ -28,8 +28,12 @@ static int skbtcindex_encode(struct sk_buff *skb, void *skbdata,
 
 static int skbtcindex_decode(struct sk_buff *skb, void *data, u16 len)
 {
-	u16 ifetc_index = *(u16 *)data;
+	u16 ifetc_index;
 
+	if (len != sizeof(ifetc_index))
+		return -EINVAL;
+
+	ifetc_index = *(u16 *)data;
 	skb->tc_index = ntohs(ifetc_index);
 	return 0;
 }
-- 
2.50.1

Re: [PATCH net] net/sched: act_ife: validate metadata length before decoding

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] New error class from the decode callbacks is misreported by
  the unchanged caller.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the patch
  rewrites exactly the offending load statements and keeps the unaligned…

--

net/sched: act_ife: validate metadata length before decoding

This adds a length check to skbmark_decode(), skbprio_decode() and
skbtcindex_decode() so that a TLV whose payload is shorter than the
expected metadata size is rejected with -EINVAL instead of being read
past the declared value.  The caller keeps decoding the remaining
metadata after a decoder failure.

No Link: tags present in the commit header.

> diff --git a/net/sched/act_meta_mark.c b/net/sched/act_meta_mark.c
> index ea0573cb8b2d..3a7d568df0da 100644
> --- a/net/sched/act_meta_mark.c
> +++ b/net/sched/act_meta_mark.c
> @@ -28,8 +28,12 @@ static int skbmark_encode(struct sk_buff *skb, void *skbdata,
>  
>  static int skbmark_decode(struct sk_buff *skb, void *data, u16 len)
>  {
> -	u32 ifemark = *(u32 *)data;
> +	u32 ifemark;
>  
> +	if (len != sizeof(ifemark))
> +		return -EINVAL;
> +

[Severity: Medium]
Is the new -EINVAL return reported correctly by the caller?  Before this
patch all three decoders returned 0 unconditionally, so a non-zero return
from find_decode_metaid() could only mean -ENOENT:

net/sched/act_ife.c:find_decode_metaid() {
	list_for_each_entry_rcu(e, &p->metalist, metalist) {
		if (metaid == e->metaid) {
			if (e->ops) {
				/* We check for decode presence already */
				return e->ops->decode(skb, mdata, mlen);
			}
		}
	}

	return -ENOENT;
}

The decoder return value is passed through verbatim, so tcf_ife_decode()
cannot tell a length error apart from an unconfigured metaid:

net/sched/act_ife.c:tcf_ife_decode() {
		if (find_decode_metaid(skb, p, mtype, dlen, curr_data)) {
			/* abuse overlimits to count when we receive metadata
			 * but dont have an ops for it
			 */
			pr_info_ratelimited("Unknown metaid %d dlen %d\n",
					    mtype, dlen);
			qstats_cpu_overlimit_inc(ife->common.cpu_qstats);
		}
}

So a frame carrying IFE_META_SKBMARK with len == 4 (dlen == 0) on an
action where SKBMARK is configured now logs "Unknown metaid 1" and bumps
the overlimits qstat, even though the metaid is registered and
configured.

Would it make sense to special case -ENOENT in tcf_ife_decode(), give
the length failure its own message, and update the comment so the
in-tree text and the operator-visible diagnostic still match what the
code does?  That log line is what an operator would look at while
triaging exactly the malformed-frame flood this patch hardens against.

> +	ifemark = *(u32 *)data;
>  	skb->mark = ntohl(ifemark);
>  	return 0;
>  }

[Severity: Medium]
This isn't a regression introduced by this patch, but since the load
statement is being rewritten anyway: is the plain u32 cast safe on
strict-alignment architectures?

The pointer comes straight from the on-wire frame layout.  ife_decode()
returns &ifehdr->tlv_data with ifehdr at skb->data + ETH_HLEN, and
struct ifeheadr starts with a __be16, so the first TLV sits at
mac_header + 16.  ife_tlv_meta_decode() then returns:

net/ife/ife.c:ife_tlv_meta_decode() {
	...
	return skbdata + sizeof(struct meta_tlvhdr);
}

which is mac_header + 20.  With the default NET_IP_ALIGN == 2 in
include/linux/skbuff.h (only x86, arm64 and powerpc override it to 0),
the MAC header is at an address congruent to 2 mod 4, so data is 2-byte
but not 4-byte aligned, and following TLVs inherit the same offset since
they are only NLA_ALIGN() spaced from that base.

That makes the two u32 loads in skbmark_decode() and skbprio_decode()
misaligned on the strict-alignment architectures (ARM32, MIPS, RISC-V,
sparc32), where the result is either an incorrect skb->mark /
skb->priority or a per-packet alignment fault fixup.  The u16 load in
skbtcindex_decode() happens to be aligned, but uses the same idiom.

Would get_unaligned_be32(data) / get_unaligned_be16(data) be preferable
here?  That also drops the now redundant ntohl()/ntohs().

> diff --git a/net/sched/act_meta_skbprio.c b/net/sched/act_meta_skbprio.c
> index 2df3133ce5ad..fa4ba4269ca0 100644
> --- a/net/sched/act_meta_skbprio.c
> +++ b/net/sched/act_meta_skbprio.c
> @@ -33,8 +33,12 @@ static int skbprio_encode(struct sk_buff *skb, void *skbdata,
>  
>  static int skbprio_decode(struct sk_buff *skb, void *data, u16 len)
>  {
> -	u32 ifeprio = *(u32 *)data;
> +	u32 ifeprio;
>  
> +	if (len != sizeof(ifeprio))
> +		return -EINVAL;
> +
> +	ifeprio = *(u32 *)data;
>  	skb->priority = ntohl(ifeprio);
>  	return 0;
>  }
> diff --git a/net/sched/act_meta_skbtcindex.c b/net/sched/act_meta_skbtcindex.c
> index 44547caead46..48f001ada8f6 100644
> --- a/net/sched/act_meta_skbtcindex.c
> +++ b/net/sched/act_meta_skbtcindex.c
> @@ -28,8 +28,12 @@ static int skbtcindex_encode(struct sk_buff *skb, void *skbdata,
>  
>  static int skbtcindex_decode(struct sk_buff *skb, void *data, u16 len)
>  {
> -	u16 ifetc_index = *(u16 *)data;
> +	u16 ifetc_index;
>  
> +	if (len != sizeof(ifetc_index))
> +		return -EINVAL;
> +
> +	ifetc_index = *(u16 *)data;
>  	skb->tc_index = ntohs(ifetc_index);
>  	return 0;
>  }

The same two questions apply to these two hunks.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260920074245.79965-1-fangxy%40xiaopeng.com