Xiao Liang <shaw.leon@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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?
