Re: [PATCH 1/5] net: Fix potential deadloop in skb_copy_ubufs()

Re: [PATCH 1/5] net: Fix potential deadloop in skb_copy_ubufs()

[linmiaohe]

Eric Dumazet <edumazet@google.com> wrote:
>On Fri, Aug 14, 2020 at 12:14 AM linmiaohe <linmiaohe@huawei.com> wrote:
>>
>> Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
>>
>> Since we always call skb_orphan_frags in skb_clone, is it unnecessary to call skb_orphan_frags in pskb_expand_head when skb is cloned ?
>
>Please give us a real case.
>
>I fear that your patches are coming directly from some kind of automated tool, that really misses how the code is really used from _current_ code base, not _hypothetical_ one.
>
>This is very time consuming. Please provide evidence first.
>
>Thank you.

I'am sorry about it. I do this mainly through code review and do some test code. So the problem codepath may not exist from current code base, but may
happen when we do not take care of it in the future use. We may forget same assumption. And make these assumptions clear seems not a bad thing.
But I'am going to just drop this patch. I believe you all can handle the things correctly.

Many thanks.

Re: [PATCH 1/5] net: Fix potential deadloop in skb_copy_ubufs()

[Eric Dumazet]

On Fri, Aug 14, 2020 at 12:14 AM linmiaohe <linmiaohe@huawei.com> wrote:
>
> Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> >On Thu, Aug 6, 2020 at 1:48 PM linmiaohe <linmiaohe@huawei.com> wrote:
> >>
> >> From: Miaohe Lin <linmiaohe@huawei.com>
> >>
> >> We could be trapped in deadloop when we try to copy userspace skb
> >> frags buffers to kernel with a cloned skb:
> >> Reproduce code snippet:
> >>         skb = alloc_skb(UBUF_DATA_LEN, GFP_ATOMIC);
> >>         clone = skb_clone(skb, GFP_ATOMIC);
> >>         skb_zcopy_set_nouarg(clone, NULL);
> >>         pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
> >>
> >> Catch this unexpected case and return -EINVAL in skb_orphan_frags()
> >> before we call skb_copy_ubufs() to fix it.
> >
> >Is this a hypothetical codepath?
> >
> >skb zerocopy carefully tracks clone calls where necessary. See the call to skb_orphan_frags in skb_clone, and the implementation of that callee.
> >
> >The only caller of skb zerocopy with nouarg is tpacket_fill_skb, as of commit 5cd8d46ea156 ("packet: copy user buffers before orphan or clone").
> >
> >As the commit subject indicates, this sets skb_zcopy_set_nouarg exactly to be sure that any clone will trigger a copy of "zerocopy"
> >user data to private kernel memory.
> >
> >No clone must happen between alloc_skb and skb_zcopy_set_nouarg, indeed. But AFAIK, none exists.
>
> Since we always call skb_orphan_frags in skb_clone, is it unnecessary to call skb_orphan_frags in pskb_expand_head when skb is cloned ?

Please give us a real case.

I fear that your patches are coming directly from some kind of
automated tool, that really misses how the code is really used
from _current_ code base, not _hypothetical_ one.

This is very time consuming. Please provide evidence first.

Thank you.

Re: [PATCH 1/5] net: Fix potential deadloop in skb_copy_ubufs()

[linmiaohe]

Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
>On Thu, Aug 6, 2020 at 1:48 PM linmiaohe <linmiaohe@huawei.com> wrote:
>>
>> From: Miaohe Lin <linmiaohe@huawei.com>
>>
>> We could be trapped in deadloop when we try to copy userspace skb 
>> frags buffers to kernel with a cloned skb:
>> Reproduce code snippet:
>>         skb = alloc_skb(UBUF_DATA_LEN, GFP_ATOMIC);
>>         clone = skb_clone(skb, GFP_ATOMIC);
>>         skb_zcopy_set_nouarg(clone, NULL);
>>         pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
>>
>> Catch this unexpected case and return -EINVAL in skb_orphan_frags() 
>> before we call skb_copy_ubufs() to fix it.
>
>Is this a hypothetical codepath?
>
>skb zerocopy carefully tracks clone calls where necessary. See the call to skb_orphan_frags in skb_clone, and the implementation of that callee.
>
>The only caller of skb zerocopy with nouarg is tpacket_fill_skb, as of commit 5cd8d46ea156 ("packet: copy user buffers before orphan or clone").
>
>As the commit subject indicates, this sets skb_zcopy_set_nouarg exactly to be sure that any clone will trigger a copy of "zerocopy"
>user data to private kernel memory.
>
>No clone must happen between alloc_skb and skb_zcopy_set_nouarg, indeed. But AFAIK, none exists.

Since we always call skb_orphan_frags in skb_clone, is it unnecessary to call skb_orphan_frags in pskb_expand_head when skb is cloned ?

Thanks.

Re: [PATCH 1/5] net: Fix potential deadloop in skb_copy_ubufs()

[linmiaohe]

Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
>On Thu, Aug 6, 2020 at 1:48 PM linmiaohe <linmiaohe@huawei.com> wrote:
>>
>> From: Miaohe Lin <linmiaohe@huawei.com>
>>
>> We could be trapped in deadloop when we try to copy userspace skb 
>> frags buffers to kernel with a cloned skb:
>
>> Catch this unexpected case and return -EINVAL in skb_orphan_frags() 
>> before we call skb_copy_ubufs() to fix it.
>
>Is this a hypothetical codepath?
>
>skb zerocopy carefully tracks clone calls where necessary. See the call to skb_orphan_frags in skb_clone, and the implementation of that callee.
>
>The only caller of skb zerocopy with nouarg is tpacket_fill_skb, as of commit 5cd8d46ea156 ("packet: copy user buffers before orphan or clone").
>
>As the commit subject indicates, this sets skb_zcopy_set_nouarg exactly to be sure that any clone will trigger a copy of "zerocopy"
>user data to private kernel memory.
>
>No clone must happen between alloc_skb and skb_zcopy_set_nouarg, indeed. But AFAIK, none exists.

Many thanks for your reply and explaination. As you say, this is a hypothetical codepath. This would not be triggerd normally.
I catch this suspicious patch just in case.

Re: [PATCH 1/5] net: Fix potential deadloop in skb_copy_ubufs()

[Willem de Bruijn]

On Thu, Aug 6, 2020 at 1:48 PM linmiaohe <linmiaohe@huawei.com> wrote:
>
> From: Miaohe Lin <linmiaohe@huawei.com>
>
> We could be trapped in deadloop when we try to copy userspace skb frags
> buffers to kernel with a cloned skb:
>
> [kbox] catch panic event, panic reason:kernel stack overflow
> [kbox] catch panic event, start logging.
> CPU: 3 PID: 4083 Comm: insmod Kdump: loaded Tainted: G       OE  4.19 #6
> Hardware name: linux,dummy-virt (DT)
> Call trace:
>         dump_backtrace+0x0/0x198
>         show_stack+0x24/0x30
>         dump_stack+0xa4/0xcc
>         kbox_panic_notifier_callback+0x1d0/0x310 [kbox]
>         notifier_call_chain+0x5c/0xa0
>         atomic_notifier_call_chain+0x3c/0x50
>         panic+0x164/0x314
>         __stack_chk_fail+0x0/0x28
>         handle_bad_stack+0xfc/0x108
>         __bad_stack+0x90/0x94
>         pskb_expand_head+0x0/0x2c8
>         pskb_expand_head+0x290/0x2c8
>         skb_copy_ubufs+0x3cc/0x520
>         pskb_expand_head+0x290/0x2c8
>         skb_copy_ubufs+0x3cc/0x520
>         pskb_expand_head+0x290/0x2c8
>         skb_copy_ubufs+0x3cc/0x520
>         pskb_expand_head+0x290/0x2c8
>         skb_copy_ubufs+0x3cc/0x520
>         ...
>         pskb_expand_head+0x290/0x2c8
>         skb_copy_ubufs+0x3cc/0x520
>         ...
>
> Reproduce code snippet:
>         skb = alloc_skb(UBUF_DATA_LEN, GFP_ATOMIC);
>         clone = skb_clone(skb, GFP_ATOMIC);
>         skb_zcopy_set_nouarg(clone, NULL);
>         pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
>
> Catch this unexpected case and return -EINVAL in skb_orphan_frags() before
> we call skb_copy_ubufs() to fix it.

Is this a hypothetical codepath?

skb zerocopy carefully tracks clone calls where necessary. See the
call to skb_orphan_frags in skb_clone, and the implementation of that
callee.

The only caller of skb zerocopy with nouarg is tpacket_fill_skb, as
of commit 5cd8d46ea156 ("packet: copy user buffers before orphan or
clone").

As the commit subject indicates, this sets skb_zcopy_set_nouarg
exactly to be sure that any clone will trigger a copy of "zerocopy"
user data to private kernel memory.

No clone must happen between alloc_skb and
skb_zcopy_set_nouarg, indeed. But AFAIK, none exists.

[PATCH 1/5] net: Fix potential deadloop in skb_copy_ubufs()

[linmiaohe]

From: Miaohe Lin <linmiaohe@huawei.com>

We could be trapped in deadloop when we try to copy userspace skb frags
buffers to kernel with a cloned skb:

[kbox] catch panic event, panic reason:kernel stack overflow
[kbox] catch panic event, start logging.
CPU: 3 PID: 4083 Comm: insmod Kdump: loaded Tainted: G       OE  4.19 #6
Hardware name: linux,dummy-virt (DT)
Call trace:
	dump_backtrace+0x0/0x198
	show_stack+0x24/0x30
	dump_stack+0xa4/0xcc
	kbox_panic_notifier_callback+0x1d0/0x310 [kbox]
	notifier_call_chain+0x5c/0xa0
	atomic_notifier_call_chain+0x3c/0x50
	panic+0x164/0x314
	__stack_chk_fail+0x0/0x28
	handle_bad_stack+0xfc/0x108
	__bad_stack+0x90/0x94
	pskb_expand_head+0x0/0x2c8
	pskb_expand_head+0x290/0x2c8
	skb_copy_ubufs+0x3cc/0x520
	pskb_expand_head+0x290/0x2c8
	skb_copy_ubufs+0x3cc/0x520
	pskb_expand_head+0x290/0x2c8
	skb_copy_ubufs+0x3cc/0x520
	pskb_expand_head+0x290/0x2c8
	skb_copy_ubufs+0x3cc/0x520
	...
	pskb_expand_head+0x290/0x2c8
	skb_copy_ubufs+0x3cc/0x520
	...

Reproduce code snippet:
	skb = alloc_skb(UBUF_DATA_LEN, GFP_ATOMIC);
	clone = skb_clone(skb, GFP_ATOMIC);
	skb_zcopy_set_nouarg(clone, NULL);
	pskb_expand_head(skb, 0, 0, GFP_ATOMIC);

Catch this unexpected case and return -EINVAL in skb_orphan_frags() before
we call skb_copy_ubufs() to fix it.

Fixes: a6686f2f382b ("skbuff: skb supports zero-copy buffers")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 include/linux/skbuff.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 0c0377fc00c2..167c8f4cb6e3 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2753,6 +2753,9 @@ static inline int skb_orphan_frags(struct sk_buff *skb, gfp_t gfp_mask)
 	if (!skb_zcopy_is_nouarg(skb) &&
 	    skb_uarg(skb)->callback == sock_zerocopy_callback)
 		return 0;
+	/* If the skb is cloned, return error here or we will be trapped in deadloop. */
+	if (unlikely(skb_cloned(skb)))
+		return -EINVAL;
 	return skb_copy_ubufs(skb, gfp_mask);
 }
 
-- 
2.19.1