[PATCH net v2] netfilter: ebtables: ebt_802_3: check the LLC fields before reading them

[PATCH net v2] netfilter: ebtables: ebt_802_3: check the LLC fields before reading them

[Guo Zihao]

ebt_802_3_mt() derives the frame pointer from skb_mac_header() and reads
the LLC fields through it without making sure that those bytes are
available at all. ebtables guarantees that the Ethernet header is linear,
but nothing between the receive path and this match pulls the LLC header,
so a frame that carries nothing past the 14-byte Ethernet header is read
past the linear area.

Also, ebt_802_3_mt() does not need to look at every LLC field for every
rule. A SAP-only rule reads only dsap and ssap, and a rule without any
SAP or TYPE constraint selects frames by their 802.3 length field alone,
which ebt_basic_match() has already handled. Reading the type field there
was wasted work and could not be checked against anything.

Go through skb_header_pointer() with the mac header offset, which handles
frames whose LLC header is not in the linear area, and require only the
bytes each rule actually examines:

 - no SAP or TYPE constraint: nothing to look at, match right away;
 - SAP-only rule: dsap/ssap, at machdr + 14..15;
 - TYPE rule: the whole union, at machdr + 14..23, which also covers the
   control byte that selects between the ui and ni views of the union.

A frame too short to hold the fields a rule examines now fails to match
that rule. The previous hotdrop approach was dropped because ebtables
legacy only honors hotdrop when every match of the rule so far has
matched, so returning false is the only behaviour both front-ends agree
on. Users who want truncated 802.3 frames dropped can match 802.3 alone.

Discovered by a static-analysis scan of net/; confirmed by code
inspection of the receive path and the ebtables core. Not triggered in
testing; no runtime report is available.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reviewed-by: Liu Chao <liuc63@xiaopeng.com>
Signed-off-by: Guo Zihao <guozh23@xiaopeng.com>
---
 net/bridge/netfilter/ebt_802_3.c | 47 +++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/net/bridge/netfilter/ebt_802_3.c b/net/bridge/netfilter/ebt_802_3.c
index 68c2519bd..1a67376d1 100644
--- a/net/bridge/netfilter/ebt_802_3.c
+++ b/net/bridge/netfilter/ebt_802_3.c
@@ -9,32 +9,59 @@
  *
  */
 #include <linux/module.h>
+#include <linux/stddef.h>
 #include <linux/netfilter/x_tables.h>
 #include <linux/netfilter_bridge/ebtables.h>
 #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;
+	unsigned int hlen;
+	__be16 type;
+
+	/* Without SAP or TYPE constraints the match only selects 802.3
+	 * frames, which ebt_basic_match() has already decided from the
+	 * ethertype/length field. Nothing in the LLC header is read.
+	 */
+	if (!(info->bitmask & (EBT_802_3_SAP | EBT_802_3_TYPE)))
+		return true;
+
+	/* Only the dsap/ssap pair is examined for a SAP-only rule; the
+	 * type field (and the control byte that selects between the ui
+	 * and ni views of the union) is needed for a TYPE rule alone.
+	 */
+	hlen = sizeof(_frame);
+	if (!(info->bitmask & EBT_802_3_TYPE))
+		hlen = offsetof(struct ebt_802_3_hdr, llc.ui.ctrl);
+
+	/* skb->data sits past the Ethernet header on the receive path, so
+	 * the frame starts at a negative offset from skb->data. Pass the
+	 * mac header offset so the check follows the frame wherever the
+	 * hook reset the mac header, and covers paged frames too. A frame
+	 * too short to hold the LLC fields simply does not match; users
+	 * who want such frames dropped can match 802.3 alone.
+	 */
+	frame = skb_header_pointer(skb, skb_mac_offset(skb), hlen, &_frame);
+	if (!frame)
+		return false;
 
 	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))
+		type = frame->llc.ui.ctrl & IS_UI ? frame->llc.ui.type :
+						    frame->llc.ni.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 net v2] netfilter: ebtables: ebt_802_3: check the LLC fields before reading them

[Pablo Neira Ayuso]

On Thu, Oct 08, 2026 at 10:45:39AM +0800, Guo Zihao wrote:
> ebt_802_3_mt() derives the frame pointer from skb_mac_header() and reads
> the LLC fields through it without making sure that those bytes are
> available at all. ebtables guarantees that the Ethernet header is linear,
> but nothing between the receive path and this match pulls the LLC header,
> so a frame that carries nothing past the 14-byte Ethernet header is read
> past the linear area.
> 
> Also, ebt_802_3_mt() does not need to look at every LLC field for every
> rule. A SAP-only rule reads only dsap and ssap, and a rule without any
> SAP or TYPE constraint selects frames by their 802.3 length field alone,
> which ebt_basic_match() has already handled. Reading the type field there
> was wasted work and could not be checked against anything.
> 
> Go through skb_header_pointer() with the mac header offset, which handles
> frames whose LLC header is not in the linear area, and require only the
> bytes each rule actually examines:
> 
>  - no SAP or TYPE constraint: nothing to look at, match right away;
>  - SAP-only rule: dsap/ssap, at machdr + 14..15;
>  - TYPE rule: the whole union, at machdr + 14..23, which also covers the
>    control byte that selects between the ui and ni views of the union.
> 
> A frame too short to hold the fields a rule examines now fails to match
> that rule. The previous hotdrop approach was dropped because ebtables
> legacy only honors hotdrop when every match of the rule so far has
> matched, so returning false is the only behaviour both front-ends agree
> on. Users who want truncated 802.3 frames dropped can match 802.3 alone.
> 
> Discovered by a static-analysis scan of net/; confirmed by code
> inspection of the receive path and the ebtables core. Not triggered in
> testing; no runtime report is available.

If no reproducible crash, then this has to be targeted at nf-next.

And llc support has been removed from the kernel IIRC.

> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reviewed-by: Liu Chao <liuc63@xiaopeng.com>
> Signed-off-by: Guo Zihao <guozh23@xiaopeng.com>
> ---
>  net/bridge/netfilter/ebt_802_3.c | 47 +++++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/net/bridge/netfilter/ebt_802_3.c b/net/bridge/netfilter/ebt_802_3.c
> index 68c2519bd..1a67376d1 100644
> --- a/net/bridge/netfilter/ebt_802_3.c
> +++ b/net/bridge/netfilter/ebt_802_3.c
> @@ -9,32 +9,59 @@
>   *
>   */
>  #include <linux/module.h>
> +#include <linux/stddef.h>
>  #include <linux/netfilter/x_tables.h>
>  #include <linux/netfilter_bridge/ebtables.h>
>  #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;
> +	unsigned int hlen;
> +	__be16 type;
> +
> +	/* Without SAP or TYPE constraints the match only selects 802.3
> +	 * frames, which ebt_basic_match() has already decided from the
> +	 * ethertype/length field. Nothing in the LLC header is read.
> +	 */
> +	if (!(info->bitmask & (EBT_802_3_SAP | EBT_802_3_TYPE)))
> +		return true;
> +
> +	/* Only the dsap/ssap pair is examined for a SAP-only rule; the
> +	 * type field (and the control byte that selects between the ui
> +	 * and ni views of the union) is needed for a TYPE rule alone.
> +	 */
> +	hlen = sizeof(_frame);
> +	if (!(info->bitmask & EBT_802_3_TYPE))
> +		hlen = offsetof(struct ebt_802_3_hdr, llc.ui.ctrl);
> +
> +	/* skb->data sits past the Ethernet header on the receive path, so
> +	 * the frame starts at a negative offset from skb->data. Pass the
> +	 * mac header offset so the check follows the frame wherever the
> +	 * hook reset the mac header, and covers paged frames too. A frame
> +	 * too short to hold the LLC fields simply does not match; users
> +	 * who want such frames dropped can match 802.3 alone.
> +	 */

I don't think it is worth to add so many comments in the code for
this, the skb_header_pointer() call plus what you can retrieve via
'git annotate' is sufficient to document why this
skb_header_pointer() is needed.

> +	frame = skb_header_pointer(skb, skb_mac_offset(skb), hlen, &_frame);
> +	if (!frame)
> +		return false;
>  
>  	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))
> +		type = frame->llc.ui.ctrl & IS_UI ? frame->llc.ui.type :
> +						    frame->llc.ni.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 net v2] netfilter: ebtables: ebt_802_3: check the LLC fields before reading them

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (2) · 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).

- [Medium] In ebt_802_3_mt() (net/bridge/netfilter/ebt_802_3.c), the
  patch makes every rule with EBT_802_3_TYPE require sizeof(struct…
- [Low] The new comment in ebt_802_3_mt()
  (net/bridge/netfilter/ebt_802_3.c, lines 27-30), and the matching…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but it is kept
  here because the concern requires validation.
- [Medium] This problem wasn't introduced by this patch, but it is kept
  here because the concern requires validation.

--

netfilter: ebtables: ebt_802_3: check the LLC fields before reading them

This patch changes ebt_802_3_mt() to read the LLC header with
skb_header_pointer() at the mac header offset instead of dereferencing
skb_mac_header() directly. Rules without SAP or TYPE constraints now return
early, and the fetch length depends on which fields the rule examines.
Frames too short for the requested fields now fail to match.

> diff --git a/net/bridge/netfilter/ebt_802_3.c b/net/bridge/netfilter/ebt_802_3.c
> index 68c2519bdc528..1a67376d146f8 100644
> --- a/net/bridge/netfilter/ebt_802_3.c
> +++ b/net/bridge/netfilter/ebt_802_3.c

[ ... ]

>  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;
> +	unsigned int hlen;
> +	__be16 type;
> +
> +	/* Without SAP or TYPE constraints the match only selects 802.3
> +	 * frames, which ebt_basic_match() has already decided from the
> +	 * ethertype/length field. Nothing in the LLC header is read.
> +	 */

[Severity: Low]
Is this comment accurate? The commit message makes the same claim:

  "a rule without any SAP or TYPE constraint selects frames by their
  802.3 length field alone, which ebt_basic_match() has already handled."

ebt_basic_match() only checks the 802.3 length when the entry has
EBT_802_3 set:

net/bridge/netfilter/ebtables.c:ebt_basic_match() {
    ...
	if (e->bitmask & EBT_802_3) {
		if (NF_INVF(e, EBT_IPROTO, eth_proto_is_802_3(ethproto)))
			return 1;
	} else if (!(e->bitmask & EBT_NOPROTO) &&
    ...
}

ebt_802_3_mt_check() only checks info->bitmask and info->invflags
against EBT_802_3_MASK. It never looks at the entry, so the kernel
accepts this match on a rule with any protocol. Only the ebtables
userspace tool enforces -p Length.

On the nft_compat bridge path, nft_match_set_mtchk_param() fills in only
ebt.ethproto and ebt.invflags. __nft_match_eval() then calls
match->match() directly and never runs ebt_basic_match().

The early return behaves the same as the old code. Could the comment be
reworded so it doesn't imply that SAP/TYPE rules only ever see 802.3/LLC
frames? As it stands, they can read Ethernet II payload bytes as
dsap/ssap/type.

> +	if (!(info->bitmask & (EBT_802_3_SAP | EBT_802_3_TYPE)))
> +		return true;
> +
> +	/* Only the dsap/ssap pair is examined for a SAP-only rule; the
> +	 * type field (and the control byte that selects between the ui
> +	 * and ni views of the union) is needed for a TYPE rule alone.
> +	 */
> +	hlen = sizeof(_frame);
> +	if (!(info->bitmask & EBT_802_3_TYPE))
> +		hlen = offsetof(struct ebt_802_3_hdr, llc.ui.ctrl);

[ ... ]

> +	frame = skb_header_pointer(skb, skb_mac_offset(skb), hlen, &_frame);
> +	if (!frame)
> +		return false;

[Severity: Medium]
Can this reject well-formed SNAP UI frames for TYPE rules?

For any rule with EBT_802_3_TYPE, hlen is sizeof(struct ebt_802_3_hdr),
which is 24 bytes. That size comes from the hdr_ni member. Its type field
sits at frame bytes 22..23, after a padding byte, which makes the union
10 bytes.

For a UI frame (ctrl & IS_UI), the TYPE branch only reads dsap, ssap and
ctrl (frame bytes 14..16) and llc.ui.type (frame bytes 20..21). That is
22 bytes in total.

Take a complete SNAP UI frame (dsap = ssap = 0xaa, ctrl = 0x03) that is
22 or 23 bytes long from the mac header. For that frame,
skb_header_pointer() returns NULL here, and the match returns false
whether or not the rule is inverted. Before the patch, every byte read
for such a frame was inside the frame.

Physical NICs pad frames to 60 bytes. Tap, veth and AF_PACKET senders on
a bridge port can still inject frames this short, and the bridge does not
pad them. A rule like "--802_3-type 0x0800 -j ACCEPT" no longer matches
these frames, and "--802_3-type ! X -j DROP" no longer drops them.

This also seems at odds with the commit message, which says the patch
will:

  "require only the bytes each rule actually examines"

Would it work to fetch up to and including ctrl first? The code could
then require offsetof(struct ebt_802_3_hdr, llc.ui.type) + 2 for UI, or
the end of the ni type for NI. A second skb_header_pointer() for just
the selected type field would also work.

>  
>  	if (info->bitmask & EBT_802_3_SAP) {

[ ... ]

>  	if (info->bitmask & EBT_802_3_TYPE) {
> -		if (!(hdr->llc.ui.dsap == CHECK_TYPE && hdr->llc.ui.ssap == CHECK_TYPE))
> +		type = frame->llc.ui.ctrl & IS_UI ? frame->llc.ui.type :
> +						    frame->llc.ni.type;

[Severity: Medium]
This isn't a bug introduced by this patch, but does this choose the
right view of the union for S-format and I-format frames?

frame->llc.ui.ctrl & IS_UI is true when either of the two low bits is
set. The uAPI header says the control field is one byte only when the
first byte is 0x03. That matches LLC_PDU_TYPE_IS_U() in
include/net/llc_pdu.h:

#define LLC_PDU_TYPE_IS_U(pdu) \
	(((pdu->ctrl_1 & LLC_PDU_TYPE_U_MASK) == LLC_PDU_TYPE_U) ? 1 : 0)

Control bytes 0x01 (S-format) and 0x02 (I-format) both have two-byte
control fields, but here they select the ui view. With
dsap = ssap = 0xaa, a TYPE rule then compares frame bytes 20..21 instead
of the ni type location. That can produce false matches or false
non-matches.

Should the test be (frame->llc.ui.ctrl & IS_UI) == IS_UI?

[Severity: Medium]
This is also a pre-existing issue, but does frame->llc.ni.type read the
SNAP type from the right offset?

struct hdr_ni in include/uapi/linux/netfilter_bridge/ebt_802_3.h is not
packed:

struct hdr_ni {
	__u8 dsap;
	__u8 ssap;
	__be16 ctrl;
	__u8  orig[3];
	__be16 type;
};

Natural alignment adds a padding byte after orig[3], so type ends up at
LLC offset 8 (frame bytes 22..23). On the wire, a header with a 2-byte
control field and a 3-byte OUI has the type at LLC offset 7 (frame bytes
21..22).

So whenever the ni view is picked, the comparison uses the second type
byte plus the next payload byte, not the real SNAP type. The same padding
is what makes sizeof(struct ebt_802_3_hdr) 24, which feeds into the hlen
question above.

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