socket: Provide put_cmsg_whitelist() for constant size copies
Subsystems (5)
PATCH v2 1/1 socket: Provide put_cmsg_whitelist() for constant size copies
Most callers of put_cmsg() use a "sizeof(foo)" for the length argument. But within put_cmsg(), the copy_to_user() call is made with a dynamic length, 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.
Most callers of put_cmsg() are copying out of stack or kmalloc, so these cases aren't a problem for hardened usercopy. However, some try to copy out of the skbuff_head_cache slab, including the "cb" region. Since whitelisting the slab area would leave other protocol definition of the "cb" region exposed to usercopy bugs, this creates put_cmsg_whitelist(), which internally uses sizeof() to provide a constant-sized length and a stack bounce buffer, in order to explicitly whitelist an otherwise disallowed slab region.
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
--- include/linux/socket.h | 8 ++++++++ net/core/sock.c | 4 +--- net/iucv/af_iucv.c | 5 ++--- net/netlink/af_netlink.c | 4 ++-- net/socket.c | 4 ++-- 5 files changed, 15 insertions(+), 10 deletions(-)
include/linux/socket.h +8 -0
@@ -343,6 +343,14 @@ struct ucred {extern int move_addr_to_kernel (void __user *uaddr,int ulen,struct sockaddr_storage *kaddr);extern int put_cmsg (struct msghdr *,int level,int type,int len,void *data); +/* +* Provide a bounce buffer for copying cmsg data to userspace when the +* target memory isn't already whitelisted for hardened usercopy. +*/ +#define put_cmsg_whitelist (_msg, _level, _type, _ptr) ({ \ + typeof(*(_ptr)) _val = *(_ptr); \ + put_cmsg(_msg, _level, _type, sizeof(_val), &_val); \ + })struct timespec ;
net/core/sock.c +1 -3
@@ -2879,7 +2879,6 @@ void sock_enable_timestamp(struct sock *sk, int flag)int sock_recv_errqueue (struct sock *sk,struct msghdr *msg,int len,int level,int type) { -struct sock_exterr_skb *serr;struct sk_buff *skb;int copied, err;
@@ -2899,8 +2898,7 @@ int sock_recv_errqueue(struct sock *sk, struct msghdr *msg, int len,sock_recv_timestamp (msg, sk, skb); - serr =SKB_EXT_ERR (skb); -put_cmsg (msg, level, type,sizeof (serr->ee), &serr->ee); +put_cmsg_whitelist (msg, level, type, &SKB_EXT_ERR (skb)->ee); msg->msg_flags |=MSG_ERRQUEUE ; err = copied;
net/iucv/af_iucv.c +2 -3
@@ -1407,9 +1407,8 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,/* create control message to store iucv msg target class: * get the trgcls from the control buffer of the skb due to * fragmentation of original iucv message. */ - err =put_cmsg (msg,SOL_IUCV ,SCM_IUCV_TRGCLS , -sizeof (IUCV_SKB_CB (skb)->class), - (void *)&IUCV_SKB_CB (skb)->class); + err =put_cmsg_whitelist (msg,SOL_IUCV ,SCM_IUCV_TRGCLS , + &IUCV_SKB_CB (skb)->class);if (err) {if (!(flags &MSG_PEEK ))skb_queue_head (&sk->sk_receive_queue, skb);
net/netlink/af_netlink.c +2 -2
@@ -1781,8 +1781,8 @@ static void netlink_cmsg_listen_all_nsid(struct sock *sk, struct msghdr *msg,if (!NETLINK_CB (skb).nsid_is_set)return ; -put_cmsg (msg,SOL_NETLINK ,NETLINK_LISTEN_ALL_NSID ,sizeof (int ), - &NETLINK_CB (skb).nsid); +put_cmsg_whitelist (msg,SOL_NETLINK ,NETLINK_LISTEN_ALL_NSID , + &NETLINK_CB (skb).nsid); }static int netlink_sendmsg (struct socket *sock,struct msghdr *msg,size_t len)
net/socket.c +2 -2
@@ -787,8 +787,8 @@ static inline void sock_recv_drops(struct msghdr *msg, struct sock *sk, struct sk_buff *skb) {if (sock_flag (sk,SOCK_RXQ_OVFL ) && skb &&SOCK_SKB_CB (skb)->dropcount) -put_cmsg (msg,SOL_SOCKET ,SO_RXQ_OVFL , -sizeof (__u32), &SOCK_SKB_CB (skb)->dropcount); +put_cmsg_whitelist (msg,SOL_SOCKET ,SO_RXQ_OVFL , + &SOCK_SKB_CB (skb)->dropcount); } void __sock_recv_ts_and_drops(struct msghdr *msg, struct sock *sk,
Signature · 12 lines
2.7.4 -- Kees Cook Pixel Security -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
David Miller
maintainerFrom: Kees Cook <[email protected]>
Date: Fri, 2 Feb 2018 02:27:49 -0800
include/linux/socket.h struct ucred { · line 353
Quoted 12 lines
include/linux/socket.h +8
-0@@ -343,6 +343,14 @@ struct ucred {extern int move_addr_to_kernel (void __user *uaddr,int ulen,struct sockaddr_storage *kaddr);extern int put_cmsg (struct msghdr *,int level,int type,int len,void *data); +/* +* Provide a bounce buffer for copying cmsg data to userspace when the +* target memory isn't already whitelisted for hardened usercopy. +*/ +#define put_cmsg_whitelist (_msg, _level, _type, _ptr) ({ \ + typeof(*(_ptr)) _val = *(_ptr); \ + put_cmsg(_msg, _level, _type, sizeof(_val), &_val); \ + })
I understand what you are trying to achieve, but it's at a real cost here. Some of these objects are structures, for example the struct sock_extended_err is 16 bytes.
And now we're going to copy it twice, once into the on-stack copy, and then once again into the CMSG blob.
Please find a way to make hardened user copy happy without adding new overhead.
Thank you.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 5 lines · David Miller
From: Kees Cook <[email protected]>
Date: Fri, 2 Feb 2018 02:27:49 -0800Quoted 2 lines
(quoted diff) +0
-0@@ -343,6 +343,14 @@ struct ucred {
Quoted 14 lines
include/linux/socket.h struct ucred { · line 353
Quoted 10 lines
include/linux/socket.h +8
-0extern int move_addr_to_kernel (void __user *uaddr,int ulen,struct sockaddr_storage *kaddr);extern int put_cmsg (struct msghdr *,int level,int type,int len,void *data); +/* +* Provide a bounce buffer for copying cmsg data to userspace when the +* target memory isn't already whitelisted for hardened usercopy. +*/ +#define put_cmsg_whitelist (_msg, _level, _type, _ptr) ({ \ + typeof(*(_ptr)) _val = *(_ptr); \ + put_cmsg(_msg, _level, _type, sizeof(_val), &_val); \ + })I understand what you are trying to achieve, but it's at a real cost here. Some of these objects are structures, for example the struct
sock_extended_erris 16 bytes.
It didn't look like put_cmsg() was on a fast path, so it seemed like a bounce buffer was the best solution here (and it's not without precedent).
Quoted 5 lines
And now we're going to copy it twice, once into the on-stack copy, and then once again into the CMSG blob.
Please find a way to make hardened user copy happy without adding new overhead.
Another idea would be breaking put_cmsg() up into a macro with helper functions, rearrange the arguments to avoid the math, and leaving the copy_to_user() inline to see the const-ness, but that seemed way uglier to me.
I'll think about it some more, but I think having put_cmsg_whitelist() called only in a few places is reasonable here.
-Kees
Signature · 8 lines
Kees Cook Pixel Security -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
David Miller
maintainerFrom: Kees Cook <[email protected]>
Date: Tue, 6 Feb 2018 04:31:50 +1100
Quoted 21 lines · David Miller
Quoted 5 lines
From: Kees Cook <[email protected]>
Date: Fri, 2 Feb 2018 02:27:49 -0800Quoted 2 lines
(quoted diff) +0
-0@@ -343,6 +343,14 @@ struct ucred {Quoted 14 lines
include/linux/socket.h struct ucred { · line 353
Quoted 10 lines
include/linux/socket.h +8
-0extern int move_addr_to_kernel (void __user *uaddr,int ulen,struct sockaddr_storage *kaddr);extern int put_cmsg (struct msghdr *,int level,int type,int len,void *data); +/* +* Provide a bounce buffer for copying cmsg data to userspace when the +* target memory isn't already whitelisted for hardened usercopy. +*/ +#define put_cmsg_whitelist (_msg, _level, _type, _ptr) ({ \ + typeof(*(_ptr)) _val = *(_ptr); \ + put_cmsg(_msg, _level, _type, sizeof(_val), &_val); \ + })I understand what you are trying to achieve, but it's at a real cost here. Some of these objects are structures, for example the struct
sock_extended_erris 16 bytes.
Quoted 3 lines
It didn't look like
put_cmsg()was on a fast path, so it seemed like a bounce buffer was the best solution here (and it's not without precedent).
For some things like timestamps it can be important.
Quoted 24 lines · David Miller
From: Kees Cook <[email protected]>
Date: Tue, 6 Feb 2018 04:31:50 +1100Quoted 21 lines · David Miller
Quoted 19 lines
From: Kees Cook <[email protected]>
Date: Fri, 2 Feb 2018 02:27:49 -0800include/linux/socket.h struct ucred { · line 353
Quoted 12 lines
include/linux/socket.h +8
-0@@ -343,6 +343,14 @@ struct ucred {extern int move_addr_to_kernel (void __user *uaddr,int ulen,struct sockaddr_storage *kaddr);extern int put_cmsg (struct msghdr *,int level,int type,int len,void *data); +/* +* Provide a bounce buffer for copying cmsg data to userspace when the +* target memory isn't already whitelisted for hardened usercopy. +*/ +#define put_cmsg_whitelist (_msg, _level, _type, _ptr) ({ \ + typeof(*(_ptr)) _val = *(_ptr); \ + put_cmsg(_msg, _level, _type, sizeof(_val), &_val); \ + })I understand what you are trying to achieve, but it's at a real cost here. Some of these objects are structures, for example the struct
sock_extended_erris 16 bytes.
Quoted 5 lines
Quoted 3 lines
It didn't look like
put_cmsg()was on a fast path, so it seemed like a bounce buffer was the best solution here (and it's not without precedent).For some things like timestamps it can be important.
Making put_cmsg() inline would help quite a bit with tracking the builtin_const-ness, and that could speed things up a little bit too. Would you be opposed to inlining?
-Kees
Signature · 8 lines
Kees Cook Pixel Security -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
David Miller
maintainerFrom: Kees Cook <[email protected]>
Date: Wed, 7 Feb 2018 05:36:02 +1100
Quoted 3 lines
Making
put_cmsg()inline would help quite a bit with tracking the builtin_const-ness, and that could speed things up a little bit too. Would you be opposed to inlining?
Nope.