[PATCH nf] netfilter: nft_exthdr: Fix non-linear header modification

[PATCH nf] netfilter: nft_exthdr: Fix non-linear header modification

[Xiao Liang]

nft_tcp_header_pointer() may copy TCP header if it's not linear.
In that case, we should modify the packet rather than the buffer, after
proper skb_ensure_writable().

Signed-off-by: Xiao Liang <shaw.leon@gmail.com>
---
 net/netfilter/nft_exthdr.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c
index 7f856ceb3a66..2189ccc1119c 100644
--- a/net/netfilter/nft_exthdr.c
+++ b/net/netfilter/nft_exthdr.c
@@ -254,13 +254,12 @@ static void nft_exthdr_tcp_set_eval(const struct nft_expr *expr,
 			goto err;
 
 		if (skb_ensure_writable(pkt->skb,
-					nft_thoff(pkt) + i + priv->len))
+					nft_thoff(pkt) + i + priv->offset +
+					priv->len))
 			goto err;
 
-		tcph = nft_tcp_header_pointer(pkt, sizeof(buff), buff,
-					      &tcphdr_len);
-		if (!tcph)
-			goto err;
+		tcph = (struct tcphdr *)(pkt->skb->data + nft_thoff(pkt));
+		opt = (u8 *)tcph;
 
 		offset = i + priv->offset;
 
@@ -325,9 +324,9 @@ static void nft_exthdr_tcp_strip_eval(const struct nft_expr *expr,
 	if (skb_ensure_writable(pkt->skb, nft_thoff(pkt) + tcphdr_len))
 		goto drop;
 
-	opt = (u8 *)nft_tcp_header_pointer(pkt, sizeof(buff), buff, &tcphdr_len);
-	if (!opt)
-		goto err;
+	tcph = (struct tcphdr *)(pkt->skb->data + nft_thoff(pkt));
+	opt = (u8 *)tcph;
+
 	for (i = sizeof(*tcph); i < tcphdr_len - 1; i += optl) {
 		unsigned int j;
 
-- 
2.42.0

Re: [PATCH nf] netfilter: nft_exthdr: Fix non-linear header modification

[Florian Westphal]

Xiao Liang <shaw.leon@gmail.com> wrote:
> nft_tcp_header_pointer() may copy TCP header if it's not linear.
> In that case, we should modify the packet rather than the buffer, after
> proper skb_ensure_writable().

Fixes: 99d1712bc41c ("netfilter: exthdr: tcp option set support")

I do not understand this changelog.

The bug is that skb_ensure_writable() size is too small, hence
nft_tcp_header_pointer() may return a pointer to local stack
buffer.

> Signed-off-by: Xiao Liang <shaw.leon@gmail.com>
> ---
>  net/netfilter/nft_exthdr.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c
> index 7f856ceb3a66..2189ccc1119c 100644
> --- a/net/netfilter/nft_exthdr.c
> +++ b/net/netfilter/nft_exthdr.c
> @@ -254,13 +254,12 @@ static void nft_exthdr_tcp_set_eval(const struct nft_expr *expr,
>  			goto err;
>  
>  		if (skb_ensure_writable(pkt->skb,
> -					nft_thoff(pkt) + i + priv->len))
> +					nft_thoff(pkt) + i + priv->offset +
> +					priv->len))

[..]

> -		tcph = nft_tcp_header_pointer(pkt, sizeof(buff), buff,
> -					      &tcphdr_len);
> -		if (!tcph)
> -			goto err;
> +		tcph = (struct tcphdr *)(pkt->skb->data + nft_thoff(pkt));
> +		opt = (u8 *)tcph;

This modification is not related to the bug?

If you think this is better, then please say that the 'do not use
nft_tcp_header_pointer' is an unrelated cleanup in the commit message.

But I would prefer to not mix functional and non-functional changes.
Also, the use of the nft_tcp_header_pointer() helper is the reason why
this doesn't result in memory corruption.

> @@ -325,9 +324,9 @@ static void nft_exthdr_tcp_strip_eval(const struct nft_expr *expr,
>  	if (skb_ensure_writable(pkt->skb, nft_thoff(pkt) + tcphdr_len))

Just use the above in nft_exthdr_tcp_set_eval and place it before the loop?

Re: [PATCH nf] netfilter: nft_exthdr: Fix non-linear header modification

[Xiao Liang]

On Fri, Aug 25, 2023 at 11:11 AM Florian Westphal <fw@strlen.de> wrote:
>
> > @@ -325,9 +324,9 @@ static void nft_exthdr_tcp_strip_eval(const struct nft_expr *expr,
> >       if (skb_ensure_writable(pkt->skb, nft_thoff(pkt) + tcphdr_len))
>
> Just use the above in nft_exthdr_tcp_set_eval and place it before the loop?

Sure. Need to pull the entire TCP header with skb_ensure_writable() then.

Re: [PATCH nf] netfilter: nft_exthdr: Fix non-linear header modification

[Xiao Liang]

On Fri, Aug 25, 2023 at 11:11 AM Florian Westphal <fw@strlen.de> wrote:
> > -             tcph = nft_tcp_header_pointer(pkt, sizeof(buff), buff,
> > -                                           &tcphdr_len);
> > -             if (!tcph)
> > -                     goto err;
> > +             tcph = (struct tcphdr *)(pkt->skb->data + nft_thoff(pkt));
> > +             opt = (u8 *)tcph;
>
> This modification is not related to the bug?
>
> If you think this is better, then please say that the 'do not use
> nft_tcp_header_pointer' is an unrelated cleanup in the commit message.
>
> But I would prefer to not mix functional and non-functional changes.
> Also, the use of the nft_tcp_header_pointer() helper is the reason why
> this doesn't result in memory corruption.

I think this makes it explicit that
    "we are modifying the original packet"
rather than
    "we are modifying the packet because above skb_ensure_writable() is enough"

>
> > @@ -325,9 +324,9 @@ static void nft_exthdr_tcp_strip_eval(const struct nft_expr *expr,
> >       if (skb_ensure_writable(pkt->skb, nft_thoff(pkt) + tcphdr_len))
>
> Just use the above in nft_exthdr_tcp_set_eval and place it before the loop?

In this case, all TCP headers will be pulled even if they don't have
the target option.

Re: [PATCH nf] netfilter: nft_exthdr: Fix non-linear header modification

[Florian Westphal]

Xiao Liang <shaw.leon@gmail.com> wrote:
> > But I would prefer to not mix functional and non-functional changes.
> > Also, the use of the nft_tcp_header_pointer() helper is the reason why
> > this doesn't result in memory corruption.
> 
> I think this makes it explicit that
>     "we are modifying the original packet"
> rather than
>     "we are modifying the packet because above skb_ensure_writable() is enough"

OK, I won't argue here.

> > Just use the above in nft_exthdr_tcp_set_eval and place it before the loop?
> 
> In this case, all TCP headers will be pulled even if they don't have
> the target option.

Keep it simple.