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
