When TEE target mirrors traffic to another interface, sk_buff may
not have enough headroom to be processed correctly.
ip_finish_output2() detect this situation for ipv4 and allocates
new skb with enogh headroom. However ipv6 lacks this logic in
ip_finish_output2 and it leads to skb_under_panic:
skbuff: skb_under_panic: text:ffffffffc0866ad4 len:96 put:24
head:ffff97be85e31800 data:ffff97be85e317f8 tail:0x58 end:0xc0 dev:gre0
------------[ cut here ]------------
kernel BUG at net/core/skbuff.c:110!
invalid opcode: 0000 [#1] SMP PTI
CPU: 2 PID: 393 Comm: kworker/2:2 Tainted: G OE 5.13.0 #13
Hardware name: Virtuozzo KVM, BIOS 1.11.0-2.vz7.4 04/01/2014
Workqueue: ipv6_addrconf addrconf_dad_work
RIP: 0010:skb_panic+0x48/0x4a
Call Trace:
skb_push.cold.111+0x10/0x10
ipgre_header+0x24/0xf0 [ip_gre]
neigh_connected_output+0xae/0xf0
ip6_finish_output2+0x1a8/0x5a0
ip6_output+0x5c/0x110
nf_dup_ipv6+0x158/0x1000 [nf_dup_ipv6]
tee_tg6+0x2e/0x40 [xt_TEE]
ip6t_do_table+0x294/0x470 [ip6_tables]
nf_hook_slow+0x44/0xc0
nf_hook.constprop.34+0x72/0xe0
ndisc_send_skb+0x20d/0x2e0
ndisc_send_ns+0xd1/0x210
addrconf_dad_work+0x3c8/0x540
process_one_work+0x1d1/0x370
worker_thread+0x30/0x390
kthread+0x116/0x130
ret_from_fork+0x22/0x30
Signed-off-by: Vasily Averin <[email protected]>
---
net/ipv6/ip6_output.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index ff4f9eb..0efcb9b 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -60,10 +60,38 @@ static int ip6_finish_output2(struct net *net, struct sock *sk, struct sk_buff *
{
struct dst_entry *dst = skb_dst(skb);
struct net_device *dev = dst->dev;
+ unsigned int hh_len = LL_RESERVED_SPACE(dev);
+ int delta = hh_len - skb_headroom(skb);
const struct in6_addr *nexthop;
struct neighbour *neigh;
int ret;
+ /* Be paranoid, rather than too clever. */
+ if (unlikely(delta > 0) && dev->header_ops) {
+ /* pskb_expand_head() might crash, if skb is shared */
+ if (skb_shared(skb)) {
+ struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
+
+ if (likely(nskb)) {
+ if (skb->sk)
+ skb_set_owner_w(skb, skb->sk);
+ consume_skb(skb);
+ } else {
+ kfree_skb(skb);
+ }
+ skb = nskb;
+ }
+ if (skb &&
+ pskb_expand_head(skb, SKB_DATA_ALIGN(delta), 0, GFP_ATOMIC)) {
+ kfree_skb(skb);
+ skb = NULL;
+ }
+ if (!skb) {
+ IP6_INC_STATS(net, ip6_dst_idev(dst), IPSTATS_MIB_OUTDISCARDS);
+ return -ENOMEM;
+ }
+ }
+
if (ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr)) {
struct inet6_dev *idev = ip6_dst_idev(skb_dst(skb));
--
1.8.3.1
Hello:
This patch was applied to netdev/net.git (refs/heads/master):
On Mon, 12 Jul 2021 09:45:06 +0300 you wrote:
> When TEE target mirrors traffic to another interface, sk_buff may
> not have enough headroom to be processed correctly.
> ip_finish_output2() detect this situation for ipv4 and allocates
> new skb with enogh headroom. However ipv6 lacks this logic in
> ip_finish_output2 and it leads to skb_under_panic:
>
> skbuff: skb_under_panic: text:ffffffffc0866ad4 len:96 put:24
> head:ffff97be85e31800 data:ffff97be85e317f8 tail:0x58 end:0xc0 dev:gre0
>
> [...]
Here is the summary with links:
- [IPV6,v3,1/1] ipv6: allocate enough headroom in ip6_finish_output2()
https://git.kernel.org/netdev/net/c/5796015fa968
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
I've found 2 problems in this patch,
and I'm going to resend new patch version soon.
On 7/12/21 9:45 AM, Vasily Averin wrote:
> index ff4f9eb..0efcb9b 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -60,10 +60,38 @@ static int ip6_finish_output2(struct net *net, struct sock *sk, struct sk_buff *
> {
> struct dst_entry *dst = skb_dst(skb);
> struct net_device *dev = dst->dev;
> + unsigned int hh_len = LL_RESERVED_SPACE(dev);
> + int delta = hh_len - skb_headroom(skb);
> const struct in6_addr *nexthop;
> struct neighbour *neigh;
> int ret;
>
> + /* Be paranoid, rather than too clever. */
> + if (unlikely(delta > 0) && dev->header_ops) {
> + /* pskb_expand_head() might crash, if skb is shared */
> + if (skb_shared(skb)) {
> + struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
> +
> + if (likely(nskb)) {
> + if (skb->sk)
> + skb_set_owner_w(skb, skb->sk);
need to assign sk not to skb but to nskb
> + consume_skb(skb);
> + } else {
> + kfree_skb(skb);
It is quite strange to call consume_skb() on one case and kfree_skb() in another one.
We know that original skb was shared so we should not call kfree_skb here.
Btw I've noticed similar problem in few other cases:
in pptp_xmit, pvc_xmit, ip_vs_prepare_tunneled_skb
they call consume_skb() in case of success and kfree_skb on error path.
It looks like potential bug for me.
> + }
> + skb = nskb;
> + }
> + if (skb &&
> + pskb_expand_head(skb, SKB_DATA_ALIGN(delta), 0, GFP_ATOMIC)) {
> + kfree_skb(skb);
> + skb = NULL;
> + }
> + if (!skb) {
> + IP6_INC_STATS(net, ip6_dst_idev(dst), IPSTATS_MIB_OUTDISCARDS);
> + return -ENOMEM;
> + }
> + }
> +
> if (ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr)) {
> struct inet6_dev *idev = ip6_dst_idev(skb_dst(skb));
>
>
Recently Syzkaller found one more issue on RHEL7-based OpenVz kernels.
During its investigation I've found that upstream is affected too.
TEE target send sbk with small headroom into another interface which requires
an increased headroom.
ipv4 handles this problem in ip_finish_output2() and creates new skb with enough headroom,
though ip6_finish_output2() lacks this logic.
Suzkaller created C reproducer, it can be found in v1 cover-letter
https://lkml.org/lkml/2021/7/7/467
v4 changes:
fixed skb_set_owner_w() call: it should set sk on new nskb
v3 changes:
now I think it's better to separate bugfix itself and creation of new helper.
now bugfix does not create new inline function. Unlike from v1 it creates new skb
only when it is necessary, i.e. for shared skb only.
In case of failure it updates IPSTATS_MIB_OUTDISCARDS counter
Patch set with new helper will be sent separately.
v2 changes:
new helper was created and used in ip6_finish_output2 and in ip6_xmit()
small refactoring in changed functions: commonly used dereferences was replaced by variables
Vasily Averin (1):
ipv6: allocate enough headroom in ip6_finish_output2()
net/ipv6/ip6_output.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
--
1.8.3.1
On 7/13/21 10:46 AM, Vasily Averin wrote:
>> + if (likely(nskb)) {
>> + if (skb->sk)
>> + skb_set_owner_w(skb, skb->sk);
>
> need to assign sk not to skb but to nskb
>
>> + consume_skb(skb);
>> + } else {
>> + kfree_skb(skb);
Please disread, I was wrong here.
> It is quite strange to call consume_skb() on one case and kfree_skb() in another one.
> We know that original skb was shared so we should not call kfree_skb here.
>
> Btw I've noticed similar problem in few other cases:
> in pptp_xmit, pvc_xmit, ip_vs_prepare_tunneled_skb
> they call consume_skb() in case of success and kfree_skb on error path.
> It looks like potential bug for me.