netdev - Re: [PATCH net-next] net: enable usercopy for skb_small_head

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACSApvaEcGvU+k7PabwbdvyZVJBHZjwdgNJogyOKVUqsqUYFMw@mail.gmail.com>
Date:   Wed, 8 Feb 2023 09:27:26 -0500
From:   Soheil Hassas Yeganeh <soheil@...gle.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        eric.dumazet@...il.com, Alexander Duyck <alexanderduyck@...com>,
        syzbot <syzkaller@...glegroups.com>
Subject: Re: [PATCH net-next] net: enable usercopy for skb_small_head_cache

On Wed, Feb 8, 2023 at 9:25 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> syzbot and other bots reported that we have to enable
> user copy to/from skb->head. [1]
>
> We can prevent access to skb_shared_info, which is a nice
> improvement over standard kmem_cache.
>
> Layout of these kmem_cache objects is:
>
> < SKB_SMALL_HEAD_HEADROOM >< struct skb_shared_info >
>
> usercopy: Kernel memory overwrite attempt detected to SLUB object 'skbuff_small_head' (offset 32, size 20)!
> ------------[ cut here ]------------
> kernel BUG at mm/usercopy.c:102 !
> invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.2.0-rc6-syzkaller-01425-gcb6b2e11a42d #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/12/2023
> RIP: 0010:usercopy_abort+0xbd/0xbf mm/usercopy.c:102
> Code: e8 ee ad ba f7 49 89 d9 4d 89 e8 4c 89 e1 41 56 48 89 ee 48 c7 c7 20 2b 5b 8a ff 74 24 08 41 57 48 8b 54 24 20 e8 7a 17 fe ff <0f> 0b e8 c2 ad ba f7 e8 7d fb 08 f8 48 8b 0c 24 49 89 d8 44 89 ea
> RSP: 0000:ffffc90000067a48 EFLAGS: 00010286
> RAX: 000000000000006b RBX: ffffffff8b5b6ea0 RCX: 0000000000000000
> RDX: ffff8881401c0000 RSI: ffffffff8166195c RDI: fffff5200000cf3b
> RBP: ffffffff8a5b2a60 R08: 000000000000006b R09: 0000000000000000
> R10: 0000000080000000 R11: 0000000000000000 R12: ffffffff8bf2a925
> R13: ffffffff8a5b29a0 R14: 0000000000000014 R15: ffffffff8a5b2960
> FS: 0000000000000000(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 000000000c48e000 CR4: 00000000003506e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> __check_heap_object+0xdd/0x110 mm/slub.c:4761
> check_heap_object mm/usercopy.c:196 [inline]
> __check_object_size mm/usercopy.c:251 [inline]
> __check_object_size+0x1da/0x5a0 mm/usercopy.c:213
> check_object_size include/linux/thread_info.h:199 [inline]
> check_copy_size include/linux/thread_info.h:235 [inline]
> copy_from_iter include/linux/uio.h:186 [inline]
> copy_from_iter_full include/linux/uio.h:194 [inline]
> memcpy_from_msg include/linux/skbuff.h:3977 [inline]
> qrtr_sendmsg+0x65f/0x970 net/qrtr/af_qrtr.c:965
> sock_sendmsg_nosec net/socket.c:722 [inline]
> sock_sendmsg+0xde/0x190 net/socket.c:745
> say_hello+0xf6/0x170 net/qrtr/ns.c:325
> qrtr_ns_init+0x220/0x2b0 net/qrtr/ns.c:804
> qrtr_proto_init+0x59/0x95 net/qrtr/af_qrtr.c:1296
> do_one_initcall+0x141/0x790 init/main.c:1306
> do_initcall_level init/main.c:1379 [inline]
> do_initcalls init/main.c:1395 [inline]
> do_basic_setup init/main.c:1414 [inline]
> kernel_init_freeable+0x6f9/0x782 init/main.c:1634
> kernel_init+0x1e/0x1d0 init/main.c:1522
> ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308
> </TASK>
>
> Fixes: bf9f1baa279f ("net: add dedicated kmem_cache for typical/small skb->head")
> Reported-by: syzbot <syzkaller@...glegroups.com>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>

Acked-by: Soheil Hassas Yeganeh <soheil@...gle.com>

Thank you for the quick fix!

> ---
>  net/core/skbuff.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index bdb1e015e32b9386139e9ad73acd6efb3c357118..70a6088e832682efccf081fa3e6a97cbdeb747ac 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4690,10 +4690,16 @@ void __init skb_init(void)
>                                                 SLAB_HWCACHE_ALIGN|SLAB_PANIC,
>                                                 NULL);
>  #ifdef HAVE_SKB_SMALL_HEAD_CACHE
> -       skb_small_head_cache = kmem_cache_create("skbuff_small_head",
> +       /* usercopy should only access first SKB_SMALL_HEAD_HEADROOM bytes.
> +        * struct skb_shared_info is located at the end of skb->head,
> +        * and should not be copied to/from user.
> +        */
> +       skb_small_head_cache = kmem_cache_create_usercopy("skbuff_small_head",
>                                                 SKB_SMALL_HEAD_CACHE_SIZE,
>                                                 0,
>                                                 SLAB_HWCACHE_ALIGN | SLAB_PANIC,
> +                                               0,
> +                                               SKB_SMALL_HEAD_HEADROOM,
>                                                 NULL);
>  #endif
>         skb_extensions_init();
> --
> 2.39.1.519.gcb327c4b5f-goog
>