[PATCH] netfilter: nft_exthdr: fix offset with ipv4_find_option()

[PATCH] netfilter: nft_exthdr: fix offset with ipv4_find_option()

[Alexey Kashavkin]

There is an incorrect calculation in the offset variable which causes the nft_skb_copy_to_reg() function to always return -EFAULT. Adding the start variable is redundant. In the __ip_options_compile() function the correct offset is specified when finding the function. There is no need to add the size of the iphdr structure to the offset.

Signed-off-by: Alexey Kashavkin <akashavkin@gmail.com>
---
 net/netfilter/nft_exthdr.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c
index b8d03364566c..c74012c99125 100644
--- a/net/netfilter/nft_exthdr.c
+++ b/net/netfilter/nft_exthdr.c
@@ -85,7 +85,6 @@ static int ipv4_find_option(struct net *net, struct sk_buff *skb,
 	unsigned char optbuf[sizeof(struct ip_options) + 40];
 	struct ip_options *opt = (struct ip_options *)optbuf;
 	struct iphdr *iph, _iph;
-	unsigned int start;
 	bool found = false;
 	__be32 info;
 	int optlen;
@@ -93,7 +92,6 @@ static int ipv4_find_option(struct net *net, struct sk_buff *skb,
 	iph = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
 	if (!iph)
 		return -EBADMSG;
-	start = sizeof(struct iphdr);
 
 	optlen = iph->ihl * 4 - (int)sizeof(struct iphdr);
 	if (optlen <= 0)
@@ -103,7 +101,7 @@ static int ipv4_find_option(struct net *net, struct sk_buff *skb,
 	/* Copy the options since __ip_options_compile() modifies
 	 * the options.
 	 */
-	if (skb_copy_bits(skb, start, opt->__data, optlen))
+	if (skb_copy_bits(skb, sizeof(struct iphdr), opt->__data, optlen))
 		return -EBADMSG;
 	opt->optlen = optlen;
 
@@ -118,18 +116,18 @@ static int ipv4_find_option(struct net *net, struct sk_buff *skb,
 		found = target == IPOPT_SSRR ? opt->is_strictroute :
 					       !opt->is_strictroute;
 		if (found)
-			*offset = opt->srr + start;
+			*offset = opt->srr;
 		break;
 	case IPOPT_RR:
 		if (!opt->rr)
 			break;
-		*offset = opt->rr + start;
+		*offset = opt->rr;
 		found = true;
 		break;
 	case IPOPT_RA:
 		if (!opt->router_alert)
 			break;
-		*offset = opt->router_alert + start;
+		*offset = opt->router_alert;
 		found = true;
 		break;
 	default:
-- 
2.39.2

Re: [PATCH] netfilter: nft_exthdr: fix offset with ipv4_find_option()

[Alexey Kashavkin]

Rules such as the following will always result in the NFT_BREAK verdict code:

# filter input ip option rr ptr 4 counter

Because the function nft_skb_copy_to_reg() returns -EFAULT. This happens because in the skb_copy_bits() function the 'offset > (int)skb->len - len' condition causes a jump to the fault part of the code.

You can verify this with two virtual machines and the python scapy library.

Configure the nftables rule on some virtual machine. From another virtual machine, use scapy to send packet with IP option:

# python3 -m scapy
# >>> send(IP(dst='x.x.x.x', options=IPOption_RR())/ICMP())
# .
# Sent 1 packets.

The 'rr exists counter' rule will show the receiving of one packet, and the 'rr ptr 4 counter' rule will not increment the counter. After applying the patch from the previous email, the 'rr ptr 4 counter' rule will increment the counter. This will happen with other options as well. But for lsrr and ssrr, you must send the packet with the routers parameter filled in. This is due to checks in __ip_options_compile() function.

# send(IP(dst=‘x.x.x.x', options=IPOption_LSRR(routers=[‘x.x.x.x']))/ICMP())

Re: [PATCH] netfilter: nft_exthdr: fix offset with ipv4_find_option()

[Florian Westphal]

Alexey Kashavkin <akashavkin@gmail.com> wrote:
> There is an incorrect calculation in the offset variable which causes the nft_skb_copy_to_reg() function to always return -EFAULT. Adding the start variable is redundant. In the __ip_options_compile() function the correct offset is specified when finding the function. There is no need to add the size of the iphdr structure to the offset.

Fixes: dbb5281a1f84 ("netfilter: nf_tables: add support for matching IPv4 options")

But there are more bugs remaining, see below.

> @@ -85,7 +85,6 @@ static int ipv4_find_option(struct net *net, struct sk_buff *skb,
>  	unsigned char optbuf[sizeof(struct ip_options) + 40];
>  	struct ip_options *opt = (struct ip_options *)optbuf;

This is wrong, the array should be aligned to fit struct
requirements, so u32 is needed, or __aligned annotation is needed
for optbuf.

> -	if (skb_copy_bits(skb, start, opt->__data, optlen))
> +	if (skb_copy_bits(skb, sizeof(struct iphdr), opt->__data, optlen))
>  		return -EBADMSG;

This copies to local on stack buffer.

>  		if (found)
> -			*offset = opt->srr + start;
> +			*offset = opt->srr;

This returns a pointer to local on stack buffer.
But that buffer isn't valid anymore when the function
returns.

It might work in case compiler inlines, but better
not rely on this.

Can you make a second patch that places optbuf in the
stack frame of the calling function instead?

Re: [PATCH] netfilter: nft_exthdr: fix offset with ipv4_find_option()

[Alexey Kashavkin]

> This is wrong, the array should be aligned to fit struct
> requirements, so u32 is needed, or __aligned annotation is needed
> for optbuf.

This is the old common case of initialising the variable structure ip_options, as in ip_sockglue.c or cipso_ipv4.c. But I don't understand how best to do it, because if we change the optbuf type to u32, it might be redundant if we don't change the array size, and therefore I have no idea what boundary to align it on.

> Can you make a second patch that places optbuf in the
> stack frame of the calling function instead?

Into the ipv4_find_option() function?

Re: [PATCH] netfilter: nft_exthdr: fix offset with ipv4_find_option()

[Florian Westphal]

Alexey Kashavkin <akashavkin@gmail.com> wrote:
> > This is wrong, the array should be aligned to fit struct
> > requirements, so u32 is needed, or __aligned annotation is needed
> > for optbuf.
> 
> This is the old common case of initialising the variable structure ip_options, as in ip_sockglue.c or cipso_ipv4.c. But I don't understand how best to do it, because if we change the optbuf type to u32, it might be redundant if we don't change the array size, and therefore I have no idea what boundary to align it on.

Then lets leave it as-is.

> > Can you make a second patch that places optbuf in the
> > stack frame of the calling function instead?
> 
> Into the ipv4_find_option() function?

Never mind, its fine, nft_exthdr_ipv4_eval refetches data from skb data.

Re: [PATCH] netfilter: nft_exthdr: fix offset with ipv4_find_option()

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Alexey Kashavkin <akashavkin@gmail.com> wrote:
> > There is an incorrect calculation in the offset variable which causes the nft_skb_copy_to_reg() function to always return -EFAULT. Adding the start variable is redundant. In the __ip_options_compile() function the correct offset is specified when finding the function. There is no need to add the size of the iphdr structure to the offset.
> 
> Fixes: dbb5281a1f84 ("netfilter: nf_tables: add support for matching IPv4 options")

Patch is fine,

Reviewed-by: Florian Westphal <fw@strlen.de>

Re: [PATCH] netfilter: nft_exthdr: fix offset with ipv4_find_option()

[Alexey Kashavkin]

> Rules such as the following will always result in the NFT_BREAK verdict code:
> 
> # filter input ip option rr ptr 4 counter
> 
> Because the function nft_skb_copy_to_reg() returns -EFAULT. This happens because in the skb_copy_bits() function the 'offset > (int)skb->len - len' condition causes a jump to the fault part of the code.
> 

As confirmation, here is a listing from kgdb. Target is Fedora 41 with kernel - 6.11.4-301.fc41.x86_64.

Thread 2 hit Breakpoint 1, nft_exthdr_ipv4_eval (expr=0xffff888004dc1210, regs=0xffffc900001009a0, pkt=0xffffc90000100ab0) at net/netfilter/nft_exthdr.c:163
163		if (nft_skb_copy_to_reg(pkt->skb, offset, dest, priv->len) < 0)
(gdb) p offset
$4 = 42
(gdb) n
167		regs->verdict.code = NFT_BREAK;
(gdb) p $eax
$5 = -14
(gdb) c
Continuing.
[Switching to Thread 27]

Thread 25 hit Breakpoint 1, nft_exthdr_ipv4_eval (expr=0xffff888004dc1210, regs=0xffffc900000eb890, pkt=0xffffc900000eb9a0) at net/netfilter/nft_exthdr.c:163
163		if (nft_skb_copy_to_reg(pkt->skb, offset, dest, priv->len) < 0)
(gdb) s
nft_skb_copy_to_reg (skb=0xffff888009407f00, offset=42, dest=0xffffc900000eb8a0, len=1) at net/netfilter/nft_exthdr.c:40
40		if (len % NFT_REG32_SIZE)
(gdb) set offset -= sizeof(struct iphdr)
(gdb) p offset
$3 = 22
(gdb) n
41			dest[len / NFT_REG32_SIZE] = 0;
(gdb)
43		return skb_copy_bits(skb, offset, dest, len);
(gdb)
nft_do_chain (pkt=0xffffc900000eb9a0, priv=0x0 <fixed_percpu_data>) at net/netfilter/nf_tables_core.c:290
290				if (regs.verdict.code != NFT_CONTINUE)


With the second packet I manually changed the offset value in nft_skb_copy_to_reg() and, as you can see, the jump to err for the resulting NFT_BREAK did not happen.