* Re: [PATCH net] net/sched: act_ife: validate metadata length before decoding
2026-09-20 7:42 [PATCH net] net/sched: act_ife: validate metadata length before decoding Fang Xieyan
@ 2026-09-21 7:45 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-09-21 7:45 UTC (permalink / raw)
To: fangxy
Cc: netdev, jhs, jiri, davem, edumazet, kuba, pabeni, horms,
xiyou.wangcong
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
^ permalink raw reply [flat|nested] 2+ messages in thread