[PATCH 1/1] netfilter: ip6t_eui64: validate MAC header before using it

[PATCH 1/1] netfilter: ip6t_eui64: validate MAC header before using it

[Ren Wei]

From: Zhengchuan Liang <zcliangcn@gmail.com>

`eui64_mt6()` derives a modified EUI-64 from the Ethernet source
address and compares it with the low 64 bits of the IPv6 source
address.

The match unconditionally reaches `skb_mac_header()` and `eth_hdr(skb)`
after a guard that only rejects an invalid MAC header when
`par->fragoff != 0`. As a result, non-fragment packets can still reach
`eth_hdr(skb)` even when the skb has no MAC header set, or when the MAC
header does not cover a full Ethernet header.

Fix this by first checking that the MAC header is set and spans a full
Ethernet header before accessing it, then using that validated header
directly for the EUI-64 comparison. Preserve the existing hotdrop
behavior for non-first fragments with an invalid MAC header.

Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2")
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Co-developed-by: Yuan Tan <yuantan098@gmail.com>
Signed-off-by: Yuan Tan <yuantan098@gmail.com>
Suggested-by: Xin Liu <bird@lzu.edu.cn>
Tested-by: Ren Wei <enjou1224z@gmail.com>
Signed-off-by: Zhengchuan Liang <zcliangcn@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
 net/ipv6/netfilter/ip6t_eui64.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/net/ipv6/netfilter/ip6t_eui64.c b/net/ipv6/netfilter/ip6t_eui64.c
index d704f7ed300c2..dbf64948d72c0 100644
--- a/net/ipv6/netfilter/ip6t_eui64.c
+++ b/net/ipv6/netfilter/ip6t_eui64.c
@@ -19,21 +19,30 @@ MODULE_AUTHOR("Andras Kis-Szabo <kisza@sch.bme.hu>");
 static bool
 eui64_mt6(const struct sk_buff *skb, struct xt_action_param *par)
 {
+	const unsigned char *mac;
+	const struct ethhdr *eth;
 	unsigned char eui64[8];
 
-	if (!(skb_mac_header(skb) >= skb->head &&
-	      skb_mac_header(skb) + ETH_HLEN <= skb->data) &&
-	    par->fragoff != 0) {
-		par->hotdrop = true;
+	if (!skb_mac_header_was_set(skb)) {
+		if (par->fragoff != 0)
+			par->hotdrop = true;
 		return false;
 	}
 
+	mac = skb_mac_header(skb);
+	if (mac < skb->head || mac + ETH_HLEN > skb->data) {
+		if (par->fragoff != 0)
+			par->hotdrop = true;
+		return false;
+	}
+	eth = (const struct ethhdr *)mac;
+
 	memset(eui64, 0, sizeof(eui64));
 
-	if (eth_hdr(skb)->h_proto == htons(ETH_P_IPV6)) {
+	if (eth->h_proto == htons(ETH_P_IPV6)) {
 		if (ipv6_hdr(skb)->version == 0x6) {
-			memcpy(eui64, eth_hdr(skb)->h_source, 3);
-			memcpy(eui64 + 5, eth_hdr(skb)->h_source + 3, 3);
+			memcpy(eui64, eth->h_source, 3);
+			memcpy(eui64 + 5, eth->h_source + 3, 3);
 			eui64[3] = 0xff;
 			eui64[4] = 0xfe;
 			eui64[0] ^= 0x02;
-- 
2.43.0

Re: [PATCH 1/1] netfilter: ip6t_eui64: validate MAC header before using it

[Florian Westphal]

Ren Wei <n05ec@lzu.edu.cn> wrote:
> From: Zhengchuan Liang <zcliangcn@gmail.com>
> 
> `eui64_mt6()` derives a modified EUI-64 from the Ethernet source
> address and compares it with the low 64 bits of the IPv6 source
> address.
> 
> The match unconditionally reaches `skb_mac_header()` and `eth_hdr(skb)`
> after a guard that only rejects an invalid MAC header when
> `par->fragoff != 0`. As a result, non-fragment packets can still reach
> `eth_hdr(skb)` even when the skb has no MAC header set, or when the MAC
> header does not cover a full Ethernet header.
> 
> Fix this by first checking that the MAC header is set and spans a full
> Ethernet header before accessing it, then using that validated header
> directly for the EUI-64 comparison. Preserve the existing hotdrop
> behavior for non-first fragments with an invalid MAC header.

I find this rather confusing.  E.g. why is net/netfilter/xt_mac.c safe?
I get the feeling that this patch is not sufficient?

> +	if (!skb_mac_header_was_set(skb)) {

Makes sense to me.

> +	mac = skb_mac_header(skb);
> +	if (mac < skb->head || mac + ETH_HLEN > skb->data) {
> +		if (par->fragoff != 0)
> +			par->hotdrop = true;
> +		return false;

Why do we still need this stunt?  Why not:

mac_len = skb_mac_header_len(skb);
if (mac_len < ETH_HLEN) {
	par->hotdrop = true;
	return false;
}

?

I also feel there should be a check for ethernet, i.e.
        if (skb->dev == NULL || skb->dev->type != ARPHRD_ETHER)

... like in xt_mac.c.

Re: [PATCH 1/1] netfilter: ip6t_eui64: validate MAC header before using it

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Ren Wei <n05ec@lzu.edu.cn> wrote:
> > From: Zhengchuan Liang <zcliangcn@gmail.com>
> > 
> > `eui64_mt6()` derives a modified EUI-64 from the Ethernet source
> > address and compares it with the low 64 bits of the IPv6 source
> > address.
> > 
> > The match unconditionally reaches `skb_mac_header()` and `eth_hdr(skb)`
> > after a guard that only rejects an invalid MAC header when
> > `par->fragoff != 0`. As a result, non-fragment packets can still reach
> > `eth_hdr(skb)` even when the skb has no MAC header set, or when the MAC
> > header does not cover a full Ethernet header.
> > 
> > Fix this by first checking that the MAC header is set and spans a full
> > Ethernet header before accessing it, then using that validated header
> > directly for the EUI-64 comparison. Preserve the existing hotdrop
> > behavior for non-first fragments with an invalid MAC header.
> 
> I find this rather confusing.  E.g. why is net/netfilter/xt_mac.c safe?
> I get the feeling that this patch is not sufficient?
> 
> > +	if (!skb_mac_header_was_set(skb)) {
> 
> Makes sense to me.
> 
> > +	mac = skb_mac_header(skb);
> > +	if (mac < skb->head || mac + ETH_HLEN > skb->data) {
> > +		if (par->fragoff != 0)
> > +			par->hotdrop = true;
> > +		return false;
> 
> Why do we still need this stunt?  Why not:
> 
> mac_len = skb_mac_header_len(skb);
> if (mac_len < ETH_HLEN) {
> 	par->hotdrop = true;
> 	return false;
> }
> 
> ?
> 
> I also feel there should be a check for ethernet, i.e.
>         if (skb->dev == NULL || skb->dev->type != ARPHRD_ETHER)
> 
> ... like in xt_mac.c.

There are other suspicious spots, e.g. in nf_log_syslog.c and in ipset.

Will you make a patch to cover all of these?

Re: [PATCH 1/1] netfilter: ip6t_eui64: validate MAC header before using it

[Florian Westphal]

[ Trimming Cc list ]

Florian Westphal <fw@strlen.de> wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > Ren Wei <n05ec@lzu.edu.cn> wrote:
> > > From: Zhengchuan Liang <zcliangcn@gmail.com>
> > > 
> > > `eui64_mt6()` derives a modified EUI-64 from the Ethernet source
> > > address and compares it with the low 64 bits of the IPv6 source
> > > address.
> > > 
> > > The match unconditionally reaches `skb_mac_header()` and `eth_hdr(skb)`
> > > after a guard that only rejects an invalid MAC header when
> > > `par->fragoff != 0`. As a result, non-fragment packets can still reach
> > > `eth_hdr(skb)` even when the skb has no MAC header set, or when the MAC
> > > header does not cover a full Ethernet header.
> > > 
> > > Fix this by first checking that the MAC header is set and spans a full
> > > Ethernet header before accessing it, then using that validated header
> > > directly for the EUI-64 comparison. Preserve the existing hotdrop
> > > behavior for non-first fragments with an invalid MAC header.
> > 
> > I find this rather confusing.  E.g. why is net/netfilter/xt_mac.c safe?
> > I get the feeling that this patch is not sufficient?
> > 
> > > +	if (!skb_mac_header_was_set(skb)) {
> > 
> > Makes sense to me.
> > 
> > > +	mac = skb_mac_header(skb);
> > > +	if (mac < skb->head || mac + ETH_HLEN > skb->data) {
> > > +		if (par->fragoff != 0)
> > > +			par->hotdrop = true;
> > > +		return false;
> > 
> > Why do we still need this stunt?  Why not:
> > 
> > mac_len = skb_mac_header_len(skb);
> > if (mac_len < ETH_HLEN) {
> > 	par->hotdrop = true;
> > 	return false;
> > }
> > 
> > ?
> > 
> > I also feel there should be a check for ethernet, i.e.
> >         if (skb->dev == NULL || skb->dev->type != ARPHRD_ETHER)
> > 
> > ... like in xt_mac.c.
> 
> There are other suspicious spots, e.g. in nf_log_syslog.c and in ipset.
> 
> Will you make a patch to cover all of these?

Ping.  Will you followup or do you expect us to take over?

Re: [PATCH 1/1] netfilter: ip6t_eui64: validate MAC header before using it

[Yuan Tan]

On 4/1/26 12:14, Florian Westphal wrote:
> [ Trimming Cc list ]
>
> Florian Westphal <fw@strlen.de> wrote:
>> Florian Westphal <fw@strlen.de> wrote:
>>> Ren Wei <n05ec@lzu.edu.cn> wrote:
>>>> From: Zhengchuan Liang <zcliangcn@gmail.com>
>>>>
>>>> `eui64_mt6()` derives a modified EUI-64 from the Ethernet source
>>>> address and compares it with the low 64 bits of the IPv6 source
>>>> address.
>>>>
>>>> The match unconditionally reaches `skb_mac_header()` and `eth_hdr(skb)`
>>>> after a guard that only rejects an invalid MAC header when
>>>> `par->fragoff != 0`. As a result, non-fragment packets can still reach
>>>> `eth_hdr(skb)` even when the skb has no MAC header set, or when the MAC
>>>> header does not cover a full Ethernet header.
>>>>
>>>> Fix this by first checking that the MAC header is set and spans a full
>>>> Ethernet header before accessing it, then using that validated header
>>>> directly for the EUI-64 comparison. Preserve the existing hotdrop
>>>> behavior for non-first fragments with an invalid MAC header.
>>> I find this rather confusing.  E.g. why is net/netfilter/xt_mac.c safe?
>>> I get the feeling that this patch is not sufficient?
>>>
>>>> +	if (!skb_mac_header_was_set(skb)) {
>>> Makes sense to me.
>>>
>>>> +	mac = skb_mac_header(skb);
>>>> +	if (mac < skb->head || mac + ETH_HLEN > skb->data) {
>>>> +		if (par->fragoff != 0)
>>>> +			par->hotdrop = true;
>>>> +		return false;
>>> Why do we still need this stunt?  Why not:
>>>
>>> mac_len = skb_mac_header_len(skb);
>>> if (mac_len < ETH_HLEN) {
>>> 	par->hotdrop = true;
>>> 	return false;
>>> }
>>>
>>> ?
>>>
>>> I also feel there should be a check for ethernet, i.e.
>>>          if (skb->dev == NULL || skb->dev->type != ARPHRD_ETHER)
>>>
>>> ... like in xt_mac.c.
>> There are other suspicious spots, e.g. in nf_log_syslog.c and in ipset.
>>
>> Will you make a patch to cover all of these?
> Ping.  Will you followup or do you expect us to take over?
Sorry for our late reply. We will try to send version 2 in 2 days.

[PATCH nf v2 1/2] netfilter: ip6t_eui64: reject invalid MAC header for all packets

[Ren Wei]

From: Zhengchuan Liang <zcliangcn@gmail.com>

`eui64_mt6()` derives a modified EUI-64 from the Ethernet source
address and compares it with the low 64 bits of the IPv6 source
address.

The existing guard only rejects an invalid MAC header when
`par->fragoff != 0`. For packets with `par->fragoff == 0`,
`eui64_mt6()` can still reach `eth_hdr(skb)` even when the MAC header
is not valid.

Fix this by removing the `par->fragoff != 0` condition so that packets
with an invalid MAC header are rejected before accessing `eth_hdr(skb)`.

Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2")
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Co-developed-by: Yuan Tan <yuantan098@gmail.com>
Signed-off-by: Yuan Tan <yuantan098@gmail.com>
Suggested-by: Xin Liu <bird@lzu.edu.cn>
Tested-by: Ren Wei <enjou1224z@gmail.com>
Signed-off-by: Zhengchuan Liang <zcliangcn@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
Changes in v2:
This patch specifically fixes the memory-safety issue in `eui64_mt6()`:
the old guard only rejected invalid MAC headers for non-first fragments,
while our PoC triggers the KASAN report with a non-fragment packet
(`par->fragoff == 0`). This patch is sufficient enough to fix the
memory-safety issue, and we do not find other `eth_hdr(skb)` users with
similar use-after-free or out-of-bound issues.

 net/ipv6/netfilter/ip6t_eui64.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/ipv6/netfilter/ip6t_eui64.c b/net/ipv6/netfilter/ip6t_eui64.c
index d704f7ed300c2..da69a27e8332c 100644
--- a/net/ipv6/netfilter/ip6t_eui64.c
+++ b/net/ipv6/netfilter/ip6t_eui64.c
@@ -22,8 +22,7 @@ eui64_mt6(const struct sk_buff *skb, struct xt_action_param *par)
 	unsigned char eui64[8];
 
 	if (!(skb_mac_header(skb) >= skb->head &&
-	      skb_mac_header(skb) + ETH_HLEN <= skb->data) &&
-	    par->fragoff != 0) {
+	      skb_mac_header(skb) + ETH_HLEN <= skb->data)) {
 		par->hotdrop = true;
 		return false;
 	}
-- 
2.43.0

[PATCH nf v2 2/2] netfilter: require Ethernet MAC header before using eth_hdr()

[Ren Wei]

From: Zhengchuan Liang <zcliangcn@gmail.com>

`ip6t_eui64`, `xt_mac`, the `bitmap:ip,mac`, `hash:ip,mac`, and
`hash:mac` ipset types, and `nf_log_syslog` access `eth_hdr(skb)`
after either assuming that the skb is associated with an Ethernet
device or checking only that the `ETH_HLEN` bytes at
`skb_mac_header(skb)` lie between `skb->head` and `skb->data`.

Make these paths first verify that the skb is associated with an
Ethernet device, that the MAC header was set, and that it spans at
least a full Ethernet header before accessing `eth_hdr(skb)`.

Suggested-by: Florian Westphal <fw@strlen.de>
Tested-by: Ren Wei <enjou1224z@gmail.com>
Signed-off-by: Zhengchuan Liang <zcliangcn@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
Changes in v2:
This patch addresses suspicious `eth_hdr(skb)` users by requiring an
Ethernet device together with a valid MAC header before accessing it.

 net/ipv6/netfilter/ip6t_eui64.c           | 7 +++++--
 net/netfilter/ipset/ip_set_bitmap_ipmac.c | 5 +++--
 net/netfilter/ipset/ip_set_hash_ipmac.c   | 9 +++++----
 net/netfilter/ipset/ip_set_hash_mac.c     | 5 +++--
 net/netfilter/nf_log_syslog.c             | 7 ++++++-
 net/netfilter/xt_mac.c                    | 4 +---
 6 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/net/ipv6/netfilter/ip6t_eui64.c b/net/ipv6/netfilter/ip6t_eui64.c
index da69a27e8332c..bbb684f9964c0 100644
--- a/net/ipv6/netfilter/ip6t_eui64.c
+++ b/net/ipv6/netfilter/ip6t_eui64.c
@@ -7,6 +7,7 @@
 #include <linux/module.h>
 #include <linux/skbuff.h>
 #include <linux/ipv6.h>
+#include <linux/if_arp.h>
 #include <linux/if_ether.h>
 
 #include <linux/netfilter/x_tables.h>
@@ -21,8 +22,10 @@ eui64_mt6(const struct sk_buff *skb, struct xt_action_param *par)
 {
 	unsigned char eui64[8];
 
-	if (!(skb_mac_header(skb) >= skb->head &&
-	      skb_mac_header(skb) + ETH_HLEN <= skb->data)) {
+	if (!skb->dev || skb->dev->type != ARPHRD_ETHER)
+		return false;
+
+	if (!skb_mac_header_was_set(skb) || skb_mac_header_len(skb) < ETH_HLEN) {
 		par->hotdrop = true;
 		return false;
 	}
diff --git a/net/netfilter/ipset/ip_set_bitmap_ipmac.c b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
index 2c625e0f49ec0..752f59ef87442 100644
--- a/net/netfilter/ipset/ip_set_bitmap_ipmac.c
+++ b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
@@ -11,6 +11,7 @@
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
 #include <linux/errno.h>
+#include <linux/if_arp.h>
 #include <linux/if_ether.h>
 #include <linux/netlink.h>
 #include <linux/jiffies.h>
@@ -220,8 +221,8 @@ bitmap_ipmac_kadt(struct ip_set *set, const struct sk_buff *skb,
 		return -IPSET_ERR_BITMAP_RANGE;
 
 	/* Backward compatibility: we don't check the second flag */
-	if (skb_mac_header(skb) < skb->head ||
-	    (skb_mac_header(skb) + ETH_HLEN) > skb->data)
+	if (!skb->dev || skb->dev->type != ARPHRD_ETHER ||
+	    !skb_mac_header_was_set(skb) || skb_mac_header_len(skb) < ETH_HLEN)
 		return -EINVAL;
 
 	e.id = ip_to_id(map, ip);
diff --git a/net/netfilter/ipset/ip_set_hash_ipmac.c b/net/netfilter/ipset/ip_set_hash_ipmac.c
index 467c59a83c0ab..b9a2681e24888 100644
--- a/net/netfilter/ipset/ip_set_hash_ipmac.c
+++ b/net/netfilter/ipset/ip_set_hash_ipmac.c
@@ -11,6 +11,7 @@
 #include <linux/skbuff.h>
 #include <linux/errno.h>
 #include <linux/random.h>
+#include <linux/if_arp.h>
 #include <linux/if_ether.h>
 #include <net/ip.h>
 #include <net/ipv6.h>
@@ -89,8 +90,8 @@ hash_ipmac4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	struct hash_ipmac4_elem e = { .ip = 0, { .foo[0] = 0, .foo[1] = 0 } };
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
 
-	if (skb_mac_header(skb) < skb->head ||
-	    (skb_mac_header(skb) + ETH_HLEN) > skb->data)
+	if (!skb->dev || skb->dev->type != ARPHRD_ETHER ||
+	    !skb_mac_header_was_set(skb) || skb_mac_header_len(skb) < ETH_HLEN)
 		return -EINVAL;
 
 	if (opt->flags & IPSET_DIM_TWO_SRC)
@@ -205,8 +206,8 @@ hash_ipmac6_kadt(struct ip_set *set, const struct sk_buff *skb,
 	};
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
 
-	if (skb_mac_header(skb) < skb->head ||
-	    (skb_mac_header(skb) + ETH_HLEN) > skb->data)
+	if (!skb->dev || skb->dev->type != ARPHRD_ETHER ||
+	    !skb_mac_header_was_set(skb) || skb_mac_header_len(skb) < ETH_HLEN)
 		return -EINVAL;
 
 	if (opt->flags & IPSET_DIM_TWO_SRC)
diff --git a/net/netfilter/ipset/ip_set_hash_mac.c b/net/netfilter/ipset/ip_set_hash_mac.c
index 718814730acf6..41a122591fe24 100644
--- a/net/netfilter/ipset/ip_set_hash_mac.c
+++ b/net/netfilter/ipset/ip_set_hash_mac.c
@@ -8,6 +8,7 @@
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
 #include <linux/errno.h>
+#include <linux/if_arp.h>
 #include <linux/if_ether.h>
 #include <net/netlink.h>
 
@@ -77,8 +78,8 @@ hash_mac4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	struct hash_mac4_elem e = { { .foo[0] = 0, .foo[1] = 0 } };
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
 
-	if (skb_mac_header(skb) < skb->head ||
-	    (skb_mac_header(skb) + ETH_HLEN) > skb->data)
+	if (!skb->dev || skb->dev->type != ARPHRD_ETHER ||
+	    !skb_mac_header_was_set(skb) || skb_mac_header_len(skb) < ETH_HLEN)
 		return -EINVAL;
 
 	if (opt->flags & IPSET_DIM_ONE_SRC)
diff --git a/net/netfilter/nf_log_syslog.c b/net/netfilter/nf_log_syslog.c
index 41503847d9d7f..f62049d429653 100644
--- a/net/netfilter/nf_log_syslog.c
+++ b/net/netfilter/nf_log_syslog.c
@@ -78,7 +78,9 @@ dump_arp_packet(struct nf_log_buf *m,
 	else
 		logflags = NF_LOG_DEFAULT_MASK;
 
-	if (logflags & NF_LOG_MACDECODE) {
+	if ((logflags & NF_LOG_MACDECODE) &&
+	    skb->dev && skb->dev->type == ARPHRD_ETHER &&
+	    skb_mac_header_was_set(skb) &&
+	    skb_mac_header_len(skb) >= ETH_HLEN) {
 		nf_log_buf_add(m, "MACSRC=%pM MACDST=%pM ",
 			       eth_hdr(skb)->h_source, eth_hdr(skb)->h_dest);
 		nf_log_dump_vlan(m, skb);
@@ -789,6 +791,9 @@ static void dump_mac_header(struct nf_log_buf *m,
 
 	switch (dev->type) {
 	case ARPHRD_ETHER:
+		if (!skb_mac_header_was_set(skb) || skb_mac_header_len(skb) < ETH_HLEN)
+			return;
+
 		nf_log_buf_add(m, "MACSRC=%pM MACDST=%pM ",
 			       eth_hdr(skb)->h_source, eth_hdr(skb)->h_dest);
 		nf_log_dump_vlan(m, skb);
diff --git a/net/netfilter/xt_mac.c b/net/netfilter/xt_mac.c
index 81649da57ba5d..4798cd2ca26ed 100644
--- a/net/netfilter/xt_mac.c
+++ b/net/netfilter/xt_mac.c
@@ -29,9 +29,7 @@ static bool mac_mt(const struct sk_buff *skb, struct xt_action_param *par)
 
 	if (skb->dev == NULL || skb->dev->type != ARPHRD_ETHER)
 		return false;
-	if (skb_mac_header(skb) < skb->head)
-		return false;
-	if (skb_mac_header(skb) + ETH_HLEN > skb->data)
+	if (!skb_mac_header_was_set(skb) || skb_mac_header_len(skb) < ETH_HLEN)
 		return false;
 	ret  = ether_addr_equal(eth_hdr(skb)->h_source, info->srcaddr);
 	ret ^= info->invert;
-- 
2.43.0