Re: [PATCH] net: Whitelist the skbuff_head_cache "cb" field
From: Kees Cook Date: Fri, 9 Feb 2018 08:01:12 +1100 > Cool, thanks. And just to be clear, if it's not already obvious, this > patch needs kmem_cache_create_usercopy() which just landed in Linus's > tree last week, in case you've not merged yet. Understood, and 'net' has it.
Re: [PATCH] net: Whitelist the skbuff_head_cache "cb" field
On Fri, Feb 9, 2018 at 7:16 AM, David Miller wrote: > From: Kees Cook > Date: Wed, 7 Feb 2018 17:44:38 -0800 > >> Most callers of put_cmsg() use a "sizeof(foo)" for the length argument. >> Within put_cmsg(), a copy_to_user() call is made with a dynamic size, as a >> result of the cmsg header calculations. This means that hardened usercopy >> will examine the copy, even though it was technically a fixed size and >> should be implicitly whitelisted. All the put_cmsg() calls being built >> from values in skbuff_head_cache are coming out of the protocol-defined >> "cb" field, so whitelist this field entirely instead of creating per-use >> bounce buffers, for which there are concerns about performance. >> >> Original report was: > ... >> Reported-by: syzbot+e2d6cfb305e9f3911...@syzkaller.appspotmail.com >> Fixes: 6d07d1cd300f ("usercopy: Restrict non-usercopy caches to size 0") >> Signed-off-by: Kees Cook >> --- >> I tried the inlining, it was awful. Splitting put_cmsg() was awful. So, >> instead, whitelist the "cb" field as the least bad option if bounce >> buffers are unacceptable. Dave, do you want to take this through net, or >> should I take it through the usercopy tree? > > Thanks Kees, I'll take this through my 'net' tree. Cool, thanks. And just to be clear, if it's not already obvious, this patch needs kmem_cache_create_usercopy() which just landed in Linus's tree last week, in case you've not merged yet. -Kees -- Kees Cook Pixel Security
Re: [PATCH] net: Whitelist the skbuff_head_cache "cb" field
From: Kees Cook Date: Wed, 7 Feb 2018 17:44:38 -0800 > Most callers of put_cmsg() use a "sizeof(foo)" for the length argument. > Within put_cmsg(), a copy_to_user() call is made with a dynamic size, as a > result of the cmsg header calculations. This means that hardened usercopy > will examine the copy, even though it was technically a fixed size and > should be implicitly whitelisted. All the put_cmsg() calls being built > from values in skbuff_head_cache are coming out of the protocol-defined > "cb" field, so whitelist this field entirely instead of creating per-use > bounce buffers, for which there are concerns about performance. > > Original report was: ... > Reported-by: syzbot+e2d6cfb305e9f3911...@syzkaller.appspotmail.com > Fixes: 6d07d1cd300f ("usercopy: Restrict non-usercopy caches to size 0") > Signed-off-by: Kees Cook > --- > I tried the inlining, it was awful. Splitting put_cmsg() was awful. So, > instead, whitelist the "cb" field as the least bad option if bounce > buffers are unacceptable. Dave, do you want to take this through net, or > should I take it through the usercopy tree? Thanks Kees, I'll take this through my 'net' tree.
[PATCH] net: Whitelist the skbuff_head_cache "cb" field
Most callers of put_cmsg() use a "sizeof(foo)" for the length argument. Within put_cmsg(), a copy_to_user() call is made with a dynamic size, as a result of the cmsg header calculations. This means that hardened usercopy will examine the copy, even though it was technically a fixed size and should be implicitly whitelisted. All the put_cmsg() calls being built from values in skbuff_head_cache are coming out of the protocol-defined "cb" field, so whitelist this field entirely instead of creating per-use bounce buffers, for which there are concerns about performance. Original report was: Bad or missing usercopy whitelist? Kernel memory exposure attempt detected from SLAB object 'skbuff_head_cache' (offset 64, size 16)! WARNING: CPU: 0 PID: 3663 at mm/usercopy.c:81 usercopy_warn+0xdb/0x100 mm/usercopy.c:76 ... __check_heap_object+0x89/0xc0 mm/slab.c:4426 check_heap_object mm/usercopy.c:236 [inline] __check_object_size+0x272/0x530 mm/usercopy.c:259 check_object_size include/linux/thread_info.h:112 [inline] check_copy_size include/linux/thread_info.h:143 [inline] copy_to_user include/linux/uaccess.h:154 [inline] put_cmsg+0x233/0x3f0 net/core/scm.c:242 sock_recv_errqueue+0x200/0x3e0 net/core/sock.c:2913 packet_recvmsg+0xb2e/0x17a0 net/packet/af_packet.c:3296 sock_recvmsg_nosec net/socket.c:803 [inline] sock_recvmsg+0xc9/0x110 net/socket.c:810 ___sys_recvmsg+0x2a4/0x640 net/socket.c:2179 __sys_recvmmsg+0x2a9/0xaf0 net/socket.c:2287 SYSC_recvmmsg net/socket.c:2368 [inline] SyS_recvmmsg+0xc4/0x160 net/socket.c:2352 entry_SYSCALL_64_fastpath+0x29/0xa0 Reported-by: syzbot+e2d6cfb305e9f3911...@syzkaller.appspotmail.com Fixes: 6d07d1cd300f ("usercopy: Restrict non-usercopy caches to size 0") Signed-off-by: Kees Cook --- I tried the inlining, it was awful. Splitting put_cmsg() was awful. So, instead, whitelist the "cb" field as the least bad option if bounce buffers are unacceptable. Dave, do you want to take this through net, or should I take it through the usercopy tree? --- net/core/skbuff.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 6b0ff396fa9d..201b96c8f414 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -3889,10 +3889,12 @@ EXPORT_SYMBOL_GPL(skb_gro_receive); void __init skb_init(void) { - skbuff_head_cache = kmem_cache_create("skbuff_head_cache", + skbuff_head_cache = kmem_cache_create_usercopy("skbuff_head_cache", sizeof(struct sk_buff), 0, SLAB_HWCACHE_ALIGN|SLAB_PANIC, + offsetof(struct sk_buff, cb), + sizeof_field(struct sk_buff, cb), NULL); skbuff_fclone_cache = kmem_cache_create("skbuff_fclone_cache", sizeof(struct sk_buff_fclones), -- 2.7.4 -- Kees Cook Pixel Security