From: Miaohe Lin <[email protected]>
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 <[email protected]>
---
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
Willem de Bruijn <[email protected]> wrote:
>On Thu, Aug 6, 2020 at 1:48 PM linmiaohe <[email protected]> wrote:
>>
>> From: Miaohe Lin <[email protected]>
>>
>> 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.
Willem de Bruijn <[email protected]> wrote:
>On Thu, Aug 6, 2020 at 1:48 PM linmiaohe <[email protected]> wrote:
>>
>> From: Miaohe Lin <[email protected]>
>>
>> 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.
On Fri, Aug 14, 2020 at 12:14 AM linmiaohe <[email protected]> wrote:
>
> Willem de Bruijn <[email protected]> wrote:
> >On Thu, Aug 6, 2020 at 1:48 PM linmiaohe <[email protected]> wrote:
> >>
> >> From: Miaohe Lin <[email protected]>
> >>
> >> 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.
Eric Dumazet <[email protected]> wrote:
>On Fri, Aug 14, 2020 at 12:14 AM linmiaohe <[email protected]> wrote:
>>
>> Willem de Bruijn <[email protected]> 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.