| Message ID | 20230825021432.6053-1-shaw.leon@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [nf] netfilter: nft_exthdr: Fix non-linear header modification | expand |
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?
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.
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.
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.
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;
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(-)