[PATCH] netfilter: ebtables: ebt_802_3: drop short frames before the match reads LLC

[PATCH] netfilter: ebtables: ebt_802_3: drop short frames before the match reads LLC

[Guo Zihao]

ebt_802_3_mt() takes the frame pointer from skb_mac_header() and reads
the LLC union fields (dsap, ssap, ctrl, type) without making sure that
those bytes are in the linear area. For an 802.3 frame (length field
below 1536) the payload is LLC, but only ETH_HLEN bytes are guaranteed
linear by the time the bridge hooks run, and neither ebt_do_table() nor
ebt_basic_match() pulls more.

A frame that carries nothing past the 14 byte Ethernet header therefore
reads into the skb tailroom. Check that sizeof(struct ebt_802_3_hdr)
bytes are actually linear before reading, and drop the packet through
par->hotdrop when they are not, the same signalling other ebt matches
use for a packet they cannot examine.

No Fixes tag. The unchecked skb_mac_header() dereference comes from the
initial ebtables merge and has not been touched since.

Reviewed-by: Liu Chao <liuc63@xiaopeng.com>
Signed-off-by: Guo Zihao <guozh23@xiaopeng.com>
---
 net/bridge/netfilter/ebt_802_3.c | 36 +++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/net/bridge/netfilter/ebt_802_3.c b/net/bridge/netfilter/ebt_802_3.c
index 68c2519bd..533f0d36e 100644
--- a/net/bridge/netfilter/ebt_802_3.c
+++ b/net/bridge/netfilter/ebt_802_3.c
@@ -14,27 +14,43 @@
 #include <linux/skbuff.h>
 #include <uapi/linux/netfilter_bridge/ebt_802_3.h>
 
-static struct ebt_802_3_hdr *ebt_802_3_hdr(const struct sk_buff *skb)
-{
-	return (struct ebt_802_3_hdr *)skb_mac_header(skb);
-}
-
 static bool
 ebt_802_3_mt(const struct sk_buff *skb, struct xt_action_param *par)
 {
 	const struct ebt_802_3_info *info = par->matchinfo;
-	const struct ebt_802_3_hdr *hdr = ebt_802_3_hdr(skb);
-	__be16 type = hdr->llc.ui.ctrl & IS_UI ? hdr->llc.ui.type : hdr->llc.ni.type;
+	struct ebt_802_3_hdr _frame;
+	const struct ebt_802_3_hdr *frame;
+	int mac_offset = skb_mac_offset(skb);
+	__be16 type;
+
+	/* skb->data sits past the Ethernet header when the match runs, so the
+	 * frame starts before skb->data. skb_header_pointer() takes an offset
+	 * relative to skb->data and cannot express that, so check the linear
+	 * area against the mac header ourselves and use skb_mac_header()
+	 * directly once the check has passed.
+	 */
+	if (mac_offset < 0)
+		mac_offset = -mac_offset;
+	if (mac_offset + sizeof(_frame) > skb_headlen(skb)) {
+		/* The frame is too short to hold an LLC header. */
+		par->hotdrop = true;
+		return false;
+	}
+
+	frame = (const struct ebt_802_3_hdr *)skb_mac_header(skb);
+	type = frame->llc.ui.ctrl & IS_UI ? frame->llc.ui.type :
+					    frame->llc.ni.type;
 
 	if (info->bitmask & EBT_802_3_SAP) {
-		if (NF_INVF(info, EBT_802_3_SAP, info->sap != hdr->llc.ui.ssap))
+		if (NF_INVF(info, EBT_802_3_SAP, info->sap != frame->llc.ui.ssap))
 			return false;
-		if (NF_INVF(info, EBT_802_3_SAP, info->sap != hdr->llc.ui.dsap))
+		if (NF_INVF(info, EBT_802_3_SAP, info->sap != frame->llc.ui.dsap))
 			return false;
 	}
 
 	if (info->bitmask & EBT_802_3_TYPE) {
-		if (!(hdr->llc.ui.dsap == CHECK_TYPE && hdr->llc.ui.ssap == CHECK_TYPE))
+		if (!(frame->llc.ui.dsap == CHECK_TYPE &&
+		      frame->llc.ui.ssap == CHECK_TYPE))
 			return false;
 		if (NF_INVF(info, EBT_802_3_TYPE, info->type != type))
 			return false;
-- 
2.50.1

Re: [PATCH] netfilter: ebtables: ebt_802_3: drop short frames before the match reads LLC

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH] netfilter: ebtables: ebt_802_3: drop short frames before the match reads LLC

[Guo Zihao]

Hi,

Replying to the bot's two questions:

1. How the issue was discovered

Manual code inspection, combined with a static-analysis scan of net/
that flagged the unchecked skb_mac_header() dereference in
ebt_802_3_mt(). After the scan report I read the surrounding code by
hand (ebtables.c ebt_do_table()/ebt_basic_match(), the bridge RX path,
and eth_type_trans()) to confirm that nothing between the RX entry and
this match guarantees more than ETH_HLEN (14) linear bytes past the mac
header is available, and that no pskb_may_pull() runs in between.

2. Whether the issue was actually triggered

Not triggered. This is theoretical, found by code inspection only.
There is no runtime report, stack trace, or crash log on my side. The
reasoning chain is:

 - ebt_802_3_mt() casts skb_mac_header(skb) directly to
   struct ebt_802_3_hdr * and dereferences fields up to machdr + 22
   (the ni branch of the LLC union).

 - The ebtables core never calls pskb_may_pull() before running the
   per-rule matches.

 - So an 802.3 frame (length field below 1536) whose skb carries
   nothing past the 14-byte Ethernet header reaches the match with
   machdr + 22 pointing into tailroom.

The fix in the patch deliberately chooses a conservative predicate:
it drops the packet whenever fewer than (ETH_HLEN + ebt 802.3 header
size) bytes are linear, which can also drop frames whose LLC union is
technically still inside the linear area but shorter than that. In
practice the minimum Ethernet frame is 60 bytes, so skb_headlen under
37 only occurs on truncated or malformed frames, where dropping is
the reasonable response. Fixing a potential OOB read is not worth
adding a second rule for.

If a runtime reproducer is needed, I can build one with a raw socket
and a veth pair, but I think the code-level analysis is conclusive.

Guo Zihao

Re: [PATCH] netfilter: ebtables: ebt_802_3: drop short frames before the match reads LLC

[netdev-bot+sashiko]

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

Critical: 0 · High: 3 · Medium: 1 · Low: 1

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).

- [High] The new length check in ebt_802_3_mt()
  (net/bridge/netfilter/ebt_802_3.c:32-34) negates the signed MAC offset…
- [High] The new comment in ebt_802_3_mt() (ebt_802_3.c:26-31) says
  skb_header_pointer() 'takes an offset relative to skb->data and cannot…
- [High] ebt_802_3_mt() sets par->hotdrop and also returns false.
- [Medium] ebt_802_3_mt() requires the full sizeof(struct ebt_802_3_hdr)
  (24 bytes from the MAC header) before looking at info->bitmask.
- [Low] The commit fixes a real read of skb data past skb_headlen() in
  ebt_802_3_mt(), but it explicitly leaves out the Fixes: tag because…

--

netfilter: ebtables: ebt_802_3: drop short frames before the match reads LLC

This commit adds a check to ebt_802_3_mt(). Before the LLC fields are read
through skb_mac_header(), it checks that sizeof(struct ebt_802_3_hdr) bytes
starting at the mac header are in the linear area. If they are not, the match
sets par->hotdrop and returns false.

> No Fixes tag. The unchecked skb_mac_header() dereference comes from the
> initial ebtables merge and has not been touched since.

[Severity: Low]
This isn't a bug, but bugs that predate git history are usually tagged in
netdev and netfilter with:

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

Could that be added so stable tooling and backporters can track this fix?

> diff --git a/net/bridge/netfilter/ebt_802_3.c b/net/bridge/netfilter/ebt_802_3.c
> index 68c2519bdc528..533f0d36e1716 100644
> --- a/net/bridge/netfilter/ebt_802_3.c
> +++ b/net/bridge/netfilter/ebt_802_3.c
> @@ -14,27 +14,43 @@

[ ... ]

>  static bool
>  ebt_802_3_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  {
>  	const struct ebt_802_3_info *info = par->matchinfo;
> -	const struct ebt_802_3_hdr *hdr = ebt_802_3_hdr(skb);
> -	__be16 type = hdr->llc.ui.ctrl & IS_UI ? hdr->llc.ui.type : hdr->llc.ni.type;
> +	struct ebt_802_3_hdr _frame;
> +	const struct ebt_802_3_hdr *frame;
> +	int mac_offset = skb_mac_offset(skb);
> +	__be16 type;
> +
> +	/* skb->data sits past the Ethernet header when the match runs, so the
> +	 * frame starts before skb->data. skb_header_pointer() takes an offset
> +	 * relative to skb->data and cannot express that, so check the linear
> +	 * area against the mac header ourselves and use skb_mac_header()
> +	 * directly once the check has passed.
> +	 */

[Severity: High]
Is this comment accurate? skb_header_pointer() takes a signed int offset, and
__skb_header_pointer() handles a negative value:

include/linux/skbuff.h:__skb_header_pointer() {
	if (likely(hlen - offset >= len))
		return (void *)data + offset;

	if (!skb || unlikely(skb_copy_bits(skb, offset, buffer, len) < 0))
	...
}

With offset = skb_mac_offset(skb) = -14, the fast path returns the mac header
when skb_headlen() >= 10. Otherwise, skb_copy_bits() copies the linear bytes
that start before skb->data and then walks the frags.

nft_payload_eval() already relies on this:

	offset = skb_mac_header(skb) - skb->data;

The commit message says only ETH_HLEN bytes are guaranteed linear. A complete
frame whose LLC header sits in page frags would now be hotdropped instead of
matched.

Could this use the following, and return false on NULL the way ebt_stp_mt()
does?

	skb_header_pointer(skb, skb_mac_offset(skb), sizeof(_frame), &_frame)

That would also give _frame a real use. Right now it only appears in sizeof().

> +	if (mac_offset < 0)
> +		mac_offset = -mac_offset;
> +	if (mac_offset + sizeof(_frame) > skb_headlen(skb)) {

[Severity: High]
Does negating mac_offset make this check stricter than intended?
skb_mac_offset() and skb_headlen() are both measured from skb->data. On
NF_BR_PRE_ROUTING, NF_BR_FORWARD and bridge-device NF_BR_LOCAL_OUT, skb->data
is ETH_HLEN past the mac header, so mac_offset is -14.

The 24-byte header ends at mac_header + 24, so skb_headlen() >= 10 is enough
(-14 + 24). After the sign flip, the test becomes 14 + 24 > skb_headlen(skb),
which requires 38 linear bytes past skb->data.

Wouldn't that hotdrop every 802.3 frame with 10 to 37 bytes of linear
payload, even though the whole LLC header is present? Unpadded frames from
veth, tap or virtio guests would hit this.

The threshold also depends on the hook. br_send_bpdu() does:

	skb_reset_mac_header(skb);

	NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_OUT,

On that path mac_offset is 0 and the frame threshold is 24 bytes. On ingress
and forward it works out to 52 bytes of frame. Doesn't this path also
contradict the comment's claim that skb->data always sits past the Ethernet
header?

[Severity: Medium]
Should the length required here depend on info->bitmask? A SAP-only rule
reads only llc.ui.dsap and llc.ui.ssap, at mac offsets 14 and 15. The ctrl
and type bytes are needed only for EBT_802_3_TYPE.

For example, br_send_tcn_bpdu() calls br_send_bpdu(p, buf, 4). That adds the
3-byte LLC UI header and the 14-byte Ethernet header, resets the mac header,
and enters NF_HOOK(NF_BR_LOCAL_OUT) with a 21-byte linear skb and mac_offset
0. The test 0 + 24 > 21 then sets par->hotdrop.

With an OUTPUT rule such as "-p Length --802_3-sap 0x42 -j ACCEPT", will the
bridge's own STP topology change notification BPDUs now be dropped? Before
this patch, dsap and ssap were read from valid bytes in that frame.

> +		/* The frame is too short to hold an LLC header. */
> +		par->hotdrop = true;
> +		return false;
> +	}

[Severity: High]
Is the packet actually dropped here under legacy ebtables? ebt_do_match()
returns !match(), so a false return stops EBT_MATCH_ITERATE. ebt_do_table()
then skips the hotdrop check:

net/bridge/netfilter/ebtables.c:ebt_do_table() {
	...
		if (EBT_MATCH_ITERATE(point, ebt_do_match, skb, &acpar) != 0)
			goto letscontinue;
		if (acpar.hotdrop) {
	...
}

acpar.hotdrop is set once per table walk and never cleared. If no later rule
fully matches, the chain policy decides:

	if (chaininfo->policy == EBT_ACCEPT) {
		read_unlock_bh(&table->lock);
		return NF_ACCEPT;

Take the default ACCEPT policy and a rule like "-p Length --802_3-sap 0x42
-j DROP". Before this patch, a short frame with SAP 0x42 was dropped, because
dsap and ssap were in linear data. Now it looks like it would be forwarded.
Can an untrusted guest on a veth or tap port use this to get past such a rule?

If a later, unrelated rule does fully match, it returns NF_DROP instead of
running its own target, and its counters and watchers are skipped.

ebtables-nft behaves differently, because __nft_match_eval() checks
xt.hotdrop before the match result:

	if (xt.hotdrop) {
		regs->verdict.code = NF_DROP;
		return;
	}

So the two front-ends now give different results. In the broute table,
ebt_broute() treats NF_DROP as "route instead of bridge", so the frame is not
discarded there either.

Would simply returning false, as ebt_stp_mt() does when skb_header_pointer()
fails, match what the commit message describes?

[ ... ]

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