* [PATCH 3/3] tomoyo: Check address length before reading address family
From: Tetsuo Handa @ 2019-04-12 10:59 UTC (permalink / raw)
To: linux-security-module; +Cc: Tetsuo Handa
In-Reply-To: <1555066776-9758-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp>
KMSAN will complain if valid address length passed to bind()/connect()/
sendmsg() is shorter than sizeof("struct sockaddr"->sa_family) bytes.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
security/tomoyo/network.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/security/tomoyo/network.c b/security/tomoyo/network.c
index 9094f4b3b367..f9ff121d7e1e 100644
--- a/security/tomoyo/network.c
+++ b/security/tomoyo/network.c
@@ -505,6 +505,8 @@ static int tomoyo_check_inet_address(const struct sockaddr *addr,
{
struct tomoyo_inet_addr_info *i = &address->inet;
+ if (addr_len < offsetofend(struct sockaddr, sa_family))
+ return 0;
switch (addr->sa_family) {
case AF_INET6:
if (addr_len < SIN6_LEN_RFC2133)
@@ -594,6 +596,8 @@ static int tomoyo_check_unix_address(struct sockaddr *addr,
{
struct tomoyo_unix_addr_info *u = &address->unix0;
+ if (addr_len < offsetofend(struct sockaddr, sa_family))
+ return 0;
if (addr->sa_family != AF_UNIX)
return 0;
u->addr = ((struct sockaddr_un *) addr)->sun_path;
--
2.16.5
^ permalink raw reply related
* [PATCH 2/3] smack: Check address length before reading address family
From: Tetsuo Handa @ 2019-04-12 10:59 UTC (permalink / raw)
To: linux-security-module; +Cc: Tetsuo Handa
In-Reply-To: <1555066776-9758-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp>
KMSAN will complain if valid address length passed to bind()/connect()/
sendmsg() is shorter than sizeof("struct sockaddr"->sa_family) bytes.
Also, since smk_ipv6_port_label()/smack_netlabel_send()/
smack_ipv6host_label()/smk_ipv6_check()/smk_ipv6_port_check() are not
checking valid address length and/or address family, make sure we check
both. The minimal valid length in smack_socket_connect() is changed from
sizeof(struct sockaddr_in6) bytes to SIN6_LEN_RFC2133 bytes, for it seems
that Smack is not using "struct sockaddr_in6"->sin6_scope_id field.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
security/smack/smack_lsm.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 5c1613519d5a..4b59e4519e0c 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2805,13 +2805,17 @@ static int smack_socket_socketpair(struct socket *socka,
*
* Records the label bound to a port.
*
- * Returns 0
+ * Returns 0 on success, and error code otherwise
*/
static int smack_socket_bind(struct socket *sock, struct sockaddr *address,
int addrlen)
{
- if (sock->sk != NULL && sock->sk->sk_family == PF_INET6)
+ if (sock->sk != NULL && sock->sk->sk_family == PF_INET6) {
+ if (addrlen < SIN6_LEN_RFC2133 ||
+ address->sa_family != AF_INET6)
+ return -EINVAL;
smk_ipv6_port_label(sock, address);
+ }
return 0;
}
#endif /* SMACK_IPV6_PORT_LABELING */
@@ -2847,12 +2851,13 @@ static int smack_socket_connect(struct socket *sock, struct sockaddr *sap,
switch (sock->sk->sk_family) {
case PF_INET:
- if (addrlen < sizeof(struct sockaddr_in))
+ if (addrlen < sizeof(struct sockaddr_in) ||
+ sap->sa_family != AF_INET)
return -EINVAL;
rc = smack_netlabel_send(sock->sk, (struct sockaddr_in *)sap);
break;
case PF_INET6:
- if (addrlen < sizeof(struct sockaddr_in6))
+ if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family != AF_INET6)
return -EINVAL;
#ifdef SMACK_IPV6_SECMARK_LABELING
rsp = smack_ipv6host_label(sip);
@@ -3682,9 +3687,15 @@ static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg,
switch (sock->sk->sk_family) {
case AF_INET:
+ if (msg->msg_namelen < sizeof(struct sockaddr_in) ||
+ sip->sin_family != AF_INET)
+ return -EINVAL;
rc = smack_netlabel_send(sock->sk, sip);
break;
case AF_INET6:
+ if (msg->msg_namelen < SIN6_LEN_RFC2133 ||
+ sap->sin6_family != AF_INET6)
+ return -EINVAL;
#ifdef SMACK_IPV6_SECMARK_LABELING
rsp = smack_ipv6host_label(sap);
if (rsp != NULL)
--
2.16.5
^ permalink raw reply related
* [PATCH 1/3] selinux: Check address length before reading address family
From: Tetsuo Handa @ 2019-04-12 10:59 UTC (permalink / raw)
To: linux-security-module; +Cc: Tetsuo Handa
KMSAN will complain if valid address length passed to bind()/connect() is
shorter than sizeof("struct sockaddr"->sa_family) bytes.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
security/selinux/hooks.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d5fdcb0d26fe..c61787b15f27 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4512,7 +4512,7 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
struct lsm_network_audit net = {0,};
struct sockaddr_in *addr4 = NULL;
struct sockaddr_in6 *addr6 = NULL;
- u16 family_sa = address->sa_family;
+ u16 family_sa;
unsigned short snum;
u32 sid, node_perm;
@@ -4522,6 +4522,9 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
* need to check address->sa_family as it is possible to have
* sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
*/
+ if (addrlen < offsetofend(struct sockaddr, sa_family))
+ return -EINVAL;
+ family_sa = address->sa_family;
switch (family_sa) {
case AF_UNSPEC:
case AF_INET:
@@ -4654,6 +4657,8 @@ static int selinux_socket_connect_helper(struct socket *sock,
* need to check address->sa_family as it is possible to have
* sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
*/
+ if (addrlen < offsetofend(struct sockaddr, sa_family))
+ return -EINVAL;
switch (address->sa_family) {
case AF_INET:
addr4 = (struct sockaddr_in *)address;
--
2.16.5
^ permalink raw reply related
* Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP
From: Dmitry Vyukov @ 2019-04-12 5:38 UTC (permalink / raw)
To: Kees Cook
Cc: Eric Biggers, Rik van Riel, Matthew Wilcox, linux-crypto,
Herbert Xu, Geert Uytterhoeven, linux-security-module, Linux ARM,
Linux Kernel Mailing List, Laura Abbott
In-Reply-To: <CAGXu5jJ8k7fP5Vb=ygmQ0B45GfrK2PeaV04bPWmcZ6Vb+swgyA@mail.gmail.com>
On Thu, Apr 11, 2019 at 10:32 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Apr 11, 2019 at 12:31 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > From: Eric Biggers <ebiggers@google.com>
> >
> > This is needed so that CONFIG_HARDENED_USERCOPY_PAGESPAN=y doesn't
> > incorrectly report a buffer overflow when the destination of
> > copy_from_iter() spans the page boundary in the 2-page buffer.
> >
> > Fixes: 3f47a03df6e8 ("crypto: testmgr - add testvec_config struct and helper functions")
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> > ---
> > crypto/testmgr.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> > index 0f6bfb6ce6a46..3522c0bed2492 100644
> > --- a/crypto/testmgr.c
> > +++ b/crypto/testmgr.c
> > @@ -156,7 +156,8 @@ static int __testmgr_alloc_buf(char *buf[XBUFSIZE], int order)
> > int i;
> >
> > for (i = 0; i < XBUFSIZE; i++) {
> > - buf[i] = (char *)__get_free_pages(GFP_KERNEL, order);
> > + buf[i] = (char *)__get_free_pages(GFP_KERNEL | __GFP_COMP,
> > + order);
>
> Is there a reason __GFP_COMP isn't automatically included in all page
> allocations? (Or rather, it seems like the exception is when things
> should NOT be considered part of the same allocation, so something
> like __GFP_SINGLE should exist?.)
It would be reasonable if __get_free_pages would automatically mark
consecutive pages as consecutive.
When these should not be considered part of the same allocation? Is it
possible to free them separately? Will that BUG with __GFP_COMP mark?
I understand that there can be a weak "these are actually the same
allocation, but I would like to think about them as separate". But
potentially we can ignore such cases.
> -Kees
>
> > if (!buf[i])
> > goto err_free_buf;
> > }
> > --
> > 2.21.0.392.gf8f6787159e-goog
> >
>
>
> --
> Kees Cook
^ permalink raw reply
* Re: [PATCH v2 1/3] security: Create "kernel hardening" config area
From: Masahiro Yamada @ 2019-04-12 1:38 UTC (permalink / raw)
To: Kees Cook
Cc: Alexander Potapenko, James Morris, Alexander Popov,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Randy Dunlap, Michal Marek, Emese Revfy,
Serge E. Hallyn, Kernel Hardening, linux-security-module,
Linux Kbuild mailing list, Linux Kernel Mailing List
In-Reply-To: <20190411180117.27704-2-keescook@chromium.org>
On Fri, Apr 12, 2019 at 3:01 AM Kees Cook <keescook@chromium.org> wrote:
>
> Right now kernel hardening options are scattered around various Kconfig
> files. This can be a central place to collect these kinds of options
> going forward. This is initially populated with the memory initialization
> options from the gcc-plugins.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> scripts/gcc-plugins/Kconfig | 74 +++--------------------------
> security/Kconfig | 2 +
> security/Kconfig.hardening | 93 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 102 insertions(+), 67 deletions(-)
> create mode 100644 security/Kconfig.hardening
>
> diff --git a/scripts/gcc-plugins/Kconfig b/scripts/gcc-plugins/Kconfig
> index 74271dba4f94..84d471dea2b7 100644
> --- a/scripts/gcc-plugins/Kconfig
> +++ b/scripts/gcc-plugins/Kconfig
> @@ -13,10 +13,11 @@ config HAVE_GCC_PLUGINS
> An arch should select this symbol if it supports building with
> GCC plugins.
>
> -menuconfig GCC_PLUGINS
> - bool "GCC plugins"
> +config GCC_PLUGINS
> + bool
> depends on HAVE_GCC_PLUGINS
> depends on PLUGIN_HOSTCC != ""
> + default y
> help
> GCC plugins are loadable modules that provide extra features to the
> compiler. They are useful for runtime instrumentation and static analysis.
> @@ -25,6 +26,8 @@ menuconfig GCC_PLUGINS
>
> if GCC_PLUGINS
>
> +menu "GCC plugins"
> +
Just a tip to save "if" ... "endif" block.
If you like, you can write like follows:
menu "GCC plugins"
depends on GCC_PLUGINS
<menu body>
endmenu
instead of
if GCC_PLUGINS
menu "GCC plugins"
<menu body>
endmenu
fi
> +menu "Memory initialization"
> +
> +choice
> + prompt "Initialize kernel stack variables at function entry"
> + depends on GCC_PLUGINS
On second thought,
this 'depends on' is unnecessary
because INIT_STACK_NONE should be always visible.
> + default INIT_STACK_NONE
> + help
> + This option enables initialization of stack variables at
> + function entry time. This has the possibility to have the
> + greatest coverage (since all functions can have their
> + variables initialized), but the performance impact depends
> + on the function calling complexity of a given workload's
> + syscalls.
> +
> + This chooses the level of coverage over classes of potentially
> + uninitialized variables. The selected class will be
> + initialized before use in a function.
> +
> + config INIT_STACK_NONE
> + bool "no automatic initialization (weakest)"
> + help
> + Disable automatic stack variable initialization.
> + This leaves the kernel vulnerable to the standard
> + classes of uninitialized stack variable exploits
> + and information exposures.
> +
> + config GCC_PLUGIN_STRUCTLEAK_USER
> + bool "zero-init structs marked for userspace (weak)"
> + depends on GCC_PLUGINS
> + select GCC_PLUGIN_STRUCTLEAK
> + help
> + Zero-initialize any structures on the stack containing
> + a __user attribute. This can prevent some classes of
> + uninitialized stack variable exploits and information
> + exposures, like CVE-2013-2141:
> + https://git.kernel.org/linus/b9e146d8eb3b9eca
> +
> + config GCC_PLUGIN_STRUCTLEAK_BYREF
> + bool "zero-init structs passed by reference (strong)"
> + depends on GCC_PLUGINS
> + select GCC_PLUGIN_STRUCTLEAK
> + help
> + Zero-initialize any structures on the stack that may
> + be passed by reference and had not already been
> + explicitly initialized. This can prevent most classes
> + of uninitialized stack variable exploits and information
> + exposures, like CVE-2017-1000410:
> + https://git.kernel.org/linus/06e7e776ca4d3654
> +
> + config GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
> + bool "zero-init anything passed by reference (very strong)"
> + depends on GCC_PLUGINS
> + select GCC_PLUGIN_STRUCTLEAK
> + help
> + Zero-initialize any stack variables that may be passed
> + by reference and had not already been explicitly
> + initialized. This is intended to eliminate all classes
> + of uninitialized stack variable exploits and information
> + exposures.
> +
> +endchoice
Another behavior change is
GCC_PLUGIN_STRUCTLEAK was previously enabled by all{yes,mod}config,
and in the compile-test coverage.
It will be disabled for all{yes,mod}config with this patch.
This is up to you. Just FYI.
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* [GIT PULL] linux-integrity patches for Linux 5.2
From: Mimi Zohar @ 2019-04-12 1:21 UTC (permalink / raw)
To: James Morris; +Cc: linux-integrity, linux-security-module
Hi James,
This pull request contains just three patches, the remainder are
either included in other pull requests (eg. audit, lockdown) or will
be upstreamed via other subsystems (eg. kselftests, Power). Included
in this pull request is one bug fix, one documentation update, and
extending the x86 IMA arch policy rules to coordinate the different
kernel module signature verification methods.
Thanks,
Mimi
The following changes since commit 8d93e952fba216cd0811247f6360d97e0465d5fc:
LSM: lsm_hooks.h: fix documentation format (2019-03-26 16:46:22 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity-for-james
for you to fetch changes up to 41475a3ebaceb270e47a77356ddc30960354cb00:
doc/kernel-parameters.txt: Deprecate ima_appraise_tcb (2019-04-10 16:41:01 -0400)
----------------------------------------------------------------
Mimi Zohar (2):
x86/ima: require signed kernel modules
x86/ima: add missing include
Petr Vorel (1):
doc/kernel-parameters.txt: Deprecate ima_appraise_tcb
Documentation/admin-guide/kernel-parameters.txt | 5 ++---
arch/x86/kernel/ima_arch.c | 10 +++++++++-
include/linux/module.h | 5 +++++
kernel/module.c | 5 +++++
4 files changed, 21 insertions(+), 4 deletions(-)
^ permalink raw reply
* Re: [PATCH] net: socket: Always initialize family field at move_addr_to_kernel().
From: Tetsuo Handa @ 2019-04-12 0:24 UTC (permalink / raw)
To: Paul Moore
Cc: linux-security-module, David Miller, netdev,
syzbot+0049bebbf3042dbd2e8f, syzbot+915c9f99f3dbc4bd6cd1
In-Reply-To: <CAHC9VhQy=UheDzhpwn7S2SUq51UmZ0MrNM+JtNdkoAi5Juscpw@mail.gmail.com>
Paul Moore wrote:
> > + /*
> > + * Nothing more to do if valid length is too short to check
> > + * address->sa_family.
> > + */
> > + if (addrlen < offsetofend(struct sockaddr, sa_family))
> > + goto out;
>
> SELinux already checks the address length further down in
> selinux_socket_bind() for the address families it care about, although
> it does read sa_family before the address length check so no
> unfortunately it's of no help.
Exactly.
Since we will anyway have to check valid length after sa_family is checked,
reading a stale sa_family is harmless except KMSAN complains uninit-value.
Therefore,
--- a/net/socket.c
+++ b/net/socket.c
@@ -181,6 +181,7 @@ static ssize_t sock_splice_read(struct file *file, loff_t *ppos,
int move_addr_to_kernel(void __user *uaddr, int ulen, struct sockaddr_storage *kaddr)
{
+ kaddr->ss_family = 0;
if (ulen < 0 || ulen > sizeof(struct sockaddr_storage))
return -EINVAL;
if (ulen == 0)
or
--- a/net/socket.c
+++ b/net/socket.c
@@ -181,6 +181,7 @@ static ssize_t sock_splice_read(struct file *file, loff_t *ppos,
int move_addr_to_kernel(void __user *uaddr, int ulen, struct sockaddr_storage *kaddr)
{
+ memset(kaddr, 0, sizeof(struct sockaddr_storage));
if (ulen < 0 || ulen > sizeof(struct sockaddr_storage))
return -EINVAL;
if (ulen == 0)
will avoid adding this "addrlen < offsetofend(struct sockaddr, sa_family)" check to every
LSM module. It is a bit pity that while it is guaranteed that sizeof(struct sockaddr_storage)
bytes are accessible, it is not guaranteed that reading "struct sockaddr_storage"->family is
safe. (Thus, I wanted to hear comments from LSM people.)
>
> I imagine you could move this new length check inside the
> PF_INET/PF_INET6 if-then code block to minimize the impact to other
> address families, but I suppose there is some value in keeping it
> where it is too.
I guess that since PF_INET/PF_INET6/PF_UNIX will be most frequently used protocols,
the impact to non-PF_INET/PF_INET6 protocols is negligible.
^ permalink raw reply
* Re: [PATCH] net: socket: Always initialize family field at move_addr_to_kernel().
From: Paul Moore @ 2019-04-11 22:33 UTC (permalink / raw)
To: Tetsuo Handa
Cc: linux-security-module, David Miller, netdev,
syzbot+0049bebbf3042dbd2e8f, syzbot+915c9f99f3dbc4bd6cd1
In-Reply-To: <4d077355-03de-0b8a-b6fc-51757eafe7eb@i-love.sakura.ne.jp>
On Thu, Apr 11, 2019 at 7:32 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> On 2019/04/04 13:49, David Miller wrote:
> > From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> > Date: Wed, 3 Apr 2019 06:07:40 +0900
> >
> >> On 2019/04/03 5:23, David Miller wrote:
> >>> Please fix RDS and other protocols to examine the length properly
> >>> instead.
> >>
> >> Do you prefer adding branches only for allow reading the family of socket address?
> >
> > If the length is zero, there is no point in reading the family.
> >
>
> (Adding LSM people.)
>
> syzbot is reporting that RDS is not checking valid length of address given from userspace.
> It turned out that there are several users who access "struct sockaddr"->family without
> checking valid length (which will be reported by KMSAN).
>
> Unfortunately, since tipc_bind() in net/tipc/socket.c accepts length == 0 as a valid input,
> we can't reject length < offsetofend(struct sockaddr, sa_family) with -EINVAL at
> move_addr_to_kernel() which are called from bind()/connect() system calls. I proposed
> always setting "struct sockaddr"->family at move_addr_to_kernel() but David does not
> like such trick.
>
> Therefore, LSM modules which checks address and/or port have to check valid length
> before accessing "struct sockaddr"->family in order to determine IPv4 or IPv6 or UNIX.
>
> Below is all-in-one change (for x86_64 allmodconfig). Well, I've just realized that
> move_addr_to_kernel() is also called by sendmsg() system call and for now added changes
> for only security/ directory. Is this change appropriate for you? (I wish we could
> simplify this by automatically initializing "struct sockaddr_storage" with 0 at
> move_addr_to_kernel()...)
> ---
> drivers/isdn/mISDN/socket.c | 4 ++--
> net/bluetooth/sco.c | 4 ++--
> net/core/filter.c | 2 ++
> net/ipv6/udp.c | 2 ++
> net/llc/af_llc.c | 3 +--
> net/netlink/af_netlink.c | 3 ++-
> net/rds/af_rds.c | 3 +++
> net/rds/bind.c | 2 ++
> net/rxrpc/af_rxrpc.c | 3 ++-
> net/sctp/socket.c | 3 ++-
> security/apparmor/lsm.c | 6 ++++++
> security/selinux/hooks.c | 12 ++++++++++++
> security/smack/smack_lsm.c | 39 +++++++++++++++++++++++++++++++++++----
> security/tomoyo/network.c | 12 ++++++++++++
> 14 files changed, 85 insertions(+), 13 deletions(-)
Some minor nits/comments below, but I think this is still okay as-is
from a SELinux perspective.
Acked-by: Paul Moore <paul@paul-moore.com>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index d5fdcb0..710d386 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4503,6 +4503,12 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
> err = sock_has_perm(sk, SOCKET__BIND);
> if (err)
> goto out;
> + /*
> + * Nothing more to do if valid length is too short to check
> + * address->sa_family.
> + */
> + if (addrlen < offsetofend(struct sockaddr, sa_family))
> + goto out;
SELinux already checks the address length further down in
selinux_socket_bind() for the address families it care about, although
it does read sa_family before the address length check so no
unfortunately it's of no help.
I imagine you could move this new length check inside the
PF_INET/PF_INET6 if-then code block to minimize the impact to other
address families, but I suppose there is some value in keeping it
where it is too.
> /* If PF_INET or PF_INET6, check name_bind permission for the port. */
> family = sk->sk_family;
> @@ -4634,6 +4640,12 @@ static int selinux_socket_connect_helper(struct socket *sock,
> err = sock_has_perm(sk, SOCKET__CONNECT);
> if (err)
> return err;
> + /*
> + * Nothing more to do if valid length is too short to check
> + * address->sa_family.
> + */
> + if (addrlen < offsetofend(struct sockaddr, sa_family))
> + return 0;
Similar comments as above, including moving the check inside the if-then block.
> /*
> * If a TCP, DCCP or SCTP socket, check name_connect permission
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages
From: Eric Biggers @ 2019-04-11 20:56 UTC (permalink / raw)
To: Kees Cook
Cc: Dmitry Vyukov, Geert Uytterhoeven, Herbert Xu,
linux-security-module, Linux ARM, Linux Crypto Mailing List,
Linux Kernel Mailing List, Laura Abbott, Rik van Riel
In-Reply-To: <CAGXu5jJo0jXhB5Xy0fryrYPy7zN2RhSsMb0r0DPQ91dNR0zPCA@mail.gmail.com>
On Thu, Apr 11, 2019 at 01:36:37PM -0700, Kees Cook wrote:
> On Thu, Apr 11, 2019 at 12:26 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > Well, I guess I'll just add __GFP_COMP so I at least don't get spammed with
> > useless bug reports.
>
> Thanks, I appreciate it.
>
> > But I don't think it's in any way acceptable to change the semantics of the
> > kernel's page allocator but only under some obscure config option, don't
> > document it anywhere, ignore the known problems for years, say that the option
> > is broken anyway so it shouldn't be used, and have to exchange 15 emails to get
> > anything resembling an explanation.
>
> I understand what you mean, yeah. I'm sorry I wasn't clear about it
> earlier. What do you think might help the situation as far as
> documentation?
>
Explanation in Documentation/core-api/memory-allocation.rst of when to use
__GFP_COMP and why. Where "why" includes the real underlying reason why it's
designed the way it is, not just "you now need to provide this flag in order to
stop the hardened usercopy thing from crashing, even though previously it meant
something else, because that's the way it is."
Also a brief, improved explanation of __GFP_COMP in include/linux/gfp.h.
Also get Documentation/security/self-protection.rst up to date with what's
actually in the kernel. Currently it doesn't mention hardened usercopy at all,
and it's unclear about what's supported now vs. what is future work.
And actually fix the known problems with the pagespan check, not ignore them for
years. If not feasible, remove the option.
- Eric
^ permalink raw reply
* Re: [PATCH v2 09/10] LSM: SafeSetID: verify transitive constrainedness
From: Kees Cook @ 2019-04-11 20:38 UTC (permalink / raw)
To: Micah Morton
Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <20190411201243.167800-1-mortonm@chromium.org>
On Thu, Apr 11, 2019 at 1:12 PM Micah Morton <mortonm@chromium.org> wrote:
>
> From: Jann Horn <jannh@google.com>
>
> Someone might write a ruleset like the following, expecting that it
> securely constrains UID 1 to UIDs 1, 2 and 3:
>
> 1:2
> 1:3
>
> However, because no constraints are applied to UIDs 2 and 3, an attacker
> with UID 1 can simply first switch to UID 2, then switch to any UID from
> there. The secure way to write this ruleset would be:
>
> 1:2
> 1:3
> 2:2
> 3:3
>
> , which uses "transition to self" as a way to inhibit the default-allow
> policy without allowing anything specific.
>
> This is somewhat unintuitive. To make sure that policy authors don't
> accidentally write insecure policies because of this, let the kernel verify
> that a new ruleset does not contain any entries that are constrained, but
> transitively unconstrained.
>
> Signed-off-by: Jann Horn <jannh@google.com>
> Signed-off-by: Micah Morton <mortonm@chromium.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> Changes since the last patch: Instead of failing open when userspace
> configures an unconstrained (and vulnerable) policy, fix up the policy
> to make sure it is safe by restricting the un-constrained UIDs. Return
> EINVAL from the policy write in the case that userspace writes an
> unconstrained policy. Also move hash_add() into a small helper function.
> security/safesetid/securityfs.c | 38 ++++++++++++++++++-
> .../selftests/safesetid/safesetid-test.c | 4 +-
> 2 files changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> index 997b403c6255..d568e17dd773 100644
> --- a/security/safesetid/securityfs.c
> +++ b/security/safesetid/securityfs.c
> @@ -76,6 +76,37 @@ static void release_ruleset(struct setuid_ruleset *pol)
> call_rcu(&pol->rcu, __release_ruleset);
> }
>
> +static void insert_rule(struct setuid_ruleset *pol, struct setuid_rule *rule)
> +{
> + hash_add(pol->rules, &rule->next, __kuid_val(rule->src_uid));
> +}
> +
> +static int verify_ruleset(struct setuid_ruleset *pol)
> +{
> + int bucket;
> + struct setuid_rule *rule, *nrule;
> + int res = 0;
> +
> + hash_for_each(pol->rules, bucket, rule, next) {
> + if (_setuid_policy_lookup(pol, rule->dst_uid, INVALID_UID) ==
> + SIDPOL_DEFAULT) {
> + pr_warn("insecure policy detected: uid %d is constrained but transitively unconstrained through uid %d\n",
> + __kuid_val(rule->src_uid),
> + __kuid_val(rule->dst_uid));
> + res = -EINVAL;
> +
> + /* fix it up */
> + nrule = kmalloc(sizeof(struct setuid_rule), GFP_KERNEL);
> + if (!nrule)
> + return -ENOMEM;
> + nrule->src_uid = rule->dst_uid;
> + nrule->dst_uid = rule->dst_uid;
> + insert_rule(pol, nrule);
> + }
> + }
> + return res;
> +}
> +
> static ssize_t handle_policy_update(struct file *file,
> const char __user *ubuf, size_t len)
> {
> @@ -128,7 +159,7 @@ static ssize_t handle_policy_update(struct file *file,
> goto out_free_rule;
> }
>
> - hash_add(pol->rules, &rule->next, __kuid_val(rule->src_uid));
> + insert_rule(pol, rule);
> p = end + 1;
> continue;
>
> @@ -137,6 +168,11 @@ static ssize_t handle_policy_update(struct file *file,
> goto out_free_buf;
> }
>
> + err = verify_ruleset(pol);
> + /* bogus policy falls through after fixing it up */
> + if (err && err != -EINVAL)
> + goto out_free_buf;
> +
> /*
> * Everything looks good, apply the policy and release the old one.
> * What we really want here is an xchg() wrapper for RCU, but since that
> diff --git a/tools/testing/selftests/safesetid/safesetid-test.c b/tools/testing/selftests/safesetid/safesetid-test.c
> index 4f03813d1911..8f40c6ecdad1 100644
> --- a/tools/testing/selftests/safesetid/safesetid-test.c
> +++ b/tools/testing/selftests/safesetid/safesetid-test.c
> @@ -144,7 +144,9 @@ static void write_policies(void)
> {
> static char *policy_str =
> "1:2\n"
> - "1:3\n";
> + "1:3\n"
> + "2:2\n"
> + "3:3\n";
> ssize_t written;
> int fd;
>
> --
> 2.21.0.392.gf8f6787159e-goog
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 08/10] LSM: SafeSetID: add read handler
From: Kees Cook @ 2019-04-11 20:37 UTC (permalink / raw)
To: Micah Morton
Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <20190411201154.167617-1-mortonm@chromium.org>
On Thu, Apr 11, 2019 at 1:12 PM Micah Morton <mortonm@chromium.org> wrote:
>
> From: Jann Horn <jannh@google.com>
>
> For debugging a running system, it is very helpful to be able to see what
> policy the system is using. Add a read handler that can dump out a copy of
> the loaded policy.
>
> Signed-off-by: Jann Horn <jannh@google.com>
> Signed-off-by: Micah Morton <mortonm@chromium.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> Changes since the last patch set: Instead of doing refcounting, change
> policy_update_lock to a mutex and hold the mutex across the policy read.
> security/safesetid/lsm.h | 1 +
> security/safesetid/securityfs.c | 35 +++++++++++++++++++++++++++++----
> 2 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
> index 4a34f558d964..db6d16e6bbc3 100644
> --- a/security/safesetid/lsm.h
> +++ b/security/safesetid/lsm.h
> @@ -41,6 +41,7 @@ struct setuid_rule {
>
> struct setuid_ruleset {
> DECLARE_HASHTABLE(rules, SETID_HASH_BITS);
> + char *policy_str;
> struct rcu_head rcu;
> };
>
> diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> index 250d59e046c1..997b403c6255 100644
> --- a/security/safesetid/securityfs.c
> +++ b/security/safesetid/securityfs.c
> @@ -19,7 +19,7 @@
>
> #include "lsm.h"
>
> -static DEFINE_SPINLOCK(policy_update_lock);
> +static DEFINE_MUTEX(policy_update_lock);
>
> /*
> * In the case the input buffer contains one or more invalid UIDs, the kuid_t
> @@ -67,6 +67,7 @@ static void __release_ruleset(struct rcu_head *rcu)
>
> hash_for_each_safe(pol->rules, bucket, tmp, rule, next)
> kfree(rule);
> + kfree(pol->policy_str);
> kfree(pol);
> }
>
> @@ -85,6 +86,7 @@ static ssize_t handle_policy_update(struct file *file,
> pol = kmalloc(sizeof(struct setuid_ruleset), GFP_KERNEL);
> if (!pol)
> return -ENOMEM;
> + pol->policy_str = NULL;
> hash_init(pol->rules);
>
> p = buf = memdup_user_nul(ubuf, len);
> @@ -92,6 +94,11 @@ static ssize_t handle_policy_update(struct file *file,
> err = PTR_ERR(buf);
> goto out_free_pol;
> }
> + pol->policy_str = kstrdup(buf, GFP_KERNEL);
> + if (pol->policy_str == NULL) {
> + err = -ENOMEM;
> + goto out_free_buf;
> + }
>
> /* policy lines, including the last one, end with \n */
> while (*p != '\0') {
> @@ -135,10 +142,10 @@ static ssize_t handle_policy_update(struct file *file,
> * What we really want here is an xchg() wrapper for RCU, but since that
> * doesn't currently exist, just use a spinlock for now.
> */
> - spin_lock(&policy_update_lock);
> + mutex_lock(&policy_update_lock);
> rcu_swap_protected(safesetid_setuid_rules, pol,
> lockdep_is_held(&policy_update_lock));
> - spin_unlock(&policy_update_lock);
> + mutex_unlock(&policy_update_lock);
> err = len;
>
> out_free_buf:
> @@ -162,7 +169,27 @@ static ssize_t safesetid_file_write(struct file *file,
> return handle_policy_update(file, buf, len);
> }
>
> +static ssize_t safesetid_file_read(struct file *file, char __user *buf,
> + size_t len, loff_t *ppos)
> +{
> + ssize_t res = 0;
> + struct setuid_ruleset *pol;
> + const char *kbuf;
> +
> + mutex_lock(&policy_update_lock);
> + pol = rcu_dereference_protected(safesetid_setuid_rules,
> + lockdep_is_held(&policy_update_lock));
> + if (pol) {
> + kbuf = pol->policy_str;
> + res = simple_read_from_buffer(buf, len, ppos,
> + kbuf, strlen(kbuf));
> + }
> + mutex_unlock(&policy_update_lock);
> + return res;
> +}
> +
> static const struct file_operations safesetid_file_fops = {
> + .read = safesetid_file_read,
> .write = safesetid_file_write,
> };
>
> @@ -181,7 +208,7 @@ static int __init safesetid_init_securityfs(void)
> goto error;
> }
>
> - policy_file = securityfs_create_file("whitelist_policy", 0200,
> + policy_file = securityfs_create_file("whitelist_policy", 0600,
> policy_dir, NULL, &safesetid_file_fops);
> if (IS_ERR(policy_file)) {
> ret = PTR_ERR(policy_file);
> --
> 2.21.0.392.gf8f6787159e-goog
--
Kees Cook
^ permalink raw reply
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages
From: Kees Cook @ 2019-04-11 20:36 UTC (permalink / raw)
To: Eric Biggers
Cc: Dmitry Vyukov, Geert Uytterhoeven, Herbert Xu,
linux-security-module, Linux ARM, Linux Crypto Mailing List,
Linux Kernel Mailing List, Laura Abbott, Rik van Riel
In-Reply-To: <20190411192607.GD225654@gmail.com>
On Thu, Apr 11, 2019 at 12:26 PM Eric Biggers <ebiggers@kernel.org> wrote:
> Well, I guess I'll just add __GFP_COMP so I at least don't get spammed with
> useless bug reports.
Thanks, I appreciate it.
> But I don't think it's in any way acceptable to change the semantics of the
> kernel's page allocator but only under some obscure config option, don't
> document it anywhere, ignore the known problems for years, say that the option
> is broken anyway so it shouldn't be used, and have to exchange 15 emails to get
> anything resembling an explanation.
I understand what you mean, yeah. I'm sorry I wasn't clear about it
earlier. What do you think might help the situation as far as
documentation?
--
Kees Cook
^ permalink raw reply
* Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP
From: Kees Cook @ 2019-04-11 20:32 UTC (permalink / raw)
To: Eric Biggers, Rik van Riel, Matthew Wilcox
Cc: linux-crypto, Herbert Xu, Kees Cook, Dmitry Vyukov,
Geert Uytterhoeven, linux-security-module, Linux ARM,
Linux Kernel Mailing List, Laura Abbott
In-Reply-To: <20190411192827.72551-1-ebiggers@kernel.org>
On Thu, Apr 11, 2019 at 12:31 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> From: Eric Biggers <ebiggers@google.com>
>
> This is needed so that CONFIG_HARDENED_USERCOPY_PAGESPAN=y doesn't
> incorrectly report a buffer overflow when the destination of
> copy_from_iter() spans the page boundary in the 2-page buffer.
>
> Fixes: 3f47a03df6e8 ("crypto: testmgr - add testvec_config struct and helper functions")
> Signed-off-by: Eric Biggers <ebiggers@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
> ---
> crypto/testmgr.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index 0f6bfb6ce6a46..3522c0bed2492 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -156,7 +156,8 @@ static int __testmgr_alloc_buf(char *buf[XBUFSIZE], int order)
> int i;
>
> for (i = 0; i < XBUFSIZE; i++) {
> - buf[i] = (char *)__get_free_pages(GFP_KERNEL, order);
> + buf[i] = (char *)__get_free_pages(GFP_KERNEL | __GFP_COMP,
> + order);
Is there a reason __GFP_COMP isn't automatically included in all page
allocations? (Or rather, it seems like the exception is when things
should NOT be considered part of the same allocation, so something
like __GFP_SINGLE should exist?.)
-Kees
> if (!buf[i])
> goto err_free_buf;
> }
> --
> 2.21.0.392.gf8f6787159e-goog
>
--
Kees Cook
^ permalink raw reply
* [PATCH v2 09/10] LSM: SafeSetID: verify transitive constrainedness
From: Micah Morton @ 2019-04-11 20:12 UTC (permalink / raw)
To: jmorris, keescook, casey, linux-security-module; +Cc: Jann Horn, Micah Morton
From: Jann Horn <jannh@google.com>
Someone might write a ruleset like the following, expecting that it
securely constrains UID 1 to UIDs 1, 2 and 3:
1:2
1:3
However, because no constraints are applied to UIDs 2 and 3, an attacker
with UID 1 can simply first switch to UID 2, then switch to any UID from
there. The secure way to write this ruleset would be:
1:2
1:3
2:2
3:3
, which uses "transition to self" as a way to inhibit the default-allow
policy without allowing anything specific.
This is somewhat unintuitive. To make sure that policy authors don't
accidentally write insecure policies because of this, let the kernel verify
that a new ruleset does not contain any entries that are constrained, but
transitively unconstrained.
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
Changes since the last patch: Instead of failing open when userspace
configures an unconstrained (and vulnerable) policy, fix up the policy
to make sure it is safe by restricting the un-constrained UIDs. Return
EINVAL from the policy write in the case that userspace writes an
unconstrained policy. Also move hash_add() into a small helper function.
security/safesetid/securityfs.c | 38 ++++++++++++++++++-
.../selftests/safesetid/safesetid-test.c | 4 +-
2 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
index 997b403c6255..d568e17dd773 100644
--- a/security/safesetid/securityfs.c
+++ b/security/safesetid/securityfs.c
@@ -76,6 +76,37 @@ static void release_ruleset(struct setuid_ruleset *pol)
call_rcu(&pol->rcu, __release_ruleset);
}
+static void insert_rule(struct setuid_ruleset *pol, struct setuid_rule *rule)
+{
+ hash_add(pol->rules, &rule->next, __kuid_val(rule->src_uid));
+}
+
+static int verify_ruleset(struct setuid_ruleset *pol)
+{
+ int bucket;
+ struct setuid_rule *rule, *nrule;
+ int res = 0;
+
+ hash_for_each(pol->rules, bucket, rule, next) {
+ if (_setuid_policy_lookup(pol, rule->dst_uid, INVALID_UID) ==
+ SIDPOL_DEFAULT) {
+ pr_warn("insecure policy detected: uid %d is constrained but transitively unconstrained through uid %d\n",
+ __kuid_val(rule->src_uid),
+ __kuid_val(rule->dst_uid));
+ res = -EINVAL;
+
+ /* fix it up */
+ nrule = kmalloc(sizeof(struct setuid_rule), GFP_KERNEL);
+ if (!nrule)
+ return -ENOMEM;
+ nrule->src_uid = rule->dst_uid;
+ nrule->dst_uid = rule->dst_uid;
+ insert_rule(pol, nrule);
+ }
+ }
+ return res;
+}
+
static ssize_t handle_policy_update(struct file *file,
const char __user *ubuf, size_t len)
{
@@ -128,7 +159,7 @@ static ssize_t handle_policy_update(struct file *file,
goto out_free_rule;
}
- hash_add(pol->rules, &rule->next, __kuid_val(rule->src_uid));
+ insert_rule(pol, rule);
p = end + 1;
continue;
@@ -137,6 +168,11 @@ static ssize_t handle_policy_update(struct file *file,
goto out_free_buf;
}
+ err = verify_ruleset(pol);
+ /* bogus policy falls through after fixing it up */
+ if (err && err != -EINVAL)
+ goto out_free_buf;
+
/*
* Everything looks good, apply the policy and release the old one.
* What we really want here is an xchg() wrapper for RCU, but since that
diff --git a/tools/testing/selftests/safesetid/safesetid-test.c b/tools/testing/selftests/safesetid/safesetid-test.c
index 4f03813d1911..8f40c6ecdad1 100644
--- a/tools/testing/selftests/safesetid/safesetid-test.c
+++ b/tools/testing/selftests/safesetid/safesetid-test.c
@@ -144,7 +144,9 @@ static void write_policies(void)
{
static char *policy_str =
"1:2\n"
- "1:3\n";
+ "1:3\n"
+ "2:2\n"
+ "3:3\n";
ssize_t written;
int fd;
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* [PATCH v2 08/10] LSM: SafeSetID: add read handler
From: Micah Morton @ 2019-04-11 20:11 UTC (permalink / raw)
To: jmorris, keescook, casey, linux-security-module; +Cc: Jann Horn, Micah Morton
From: Jann Horn <jannh@google.com>
For debugging a running system, it is very helpful to be able to see what
policy the system is using. Add a read handler that can dump out a copy of
the loaded policy.
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
Changes since the last patch set: Instead of doing refcounting, change
policy_update_lock to a mutex and hold the mutex across the policy read.
security/safesetid/lsm.h | 1 +
security/safesetid/securityfs.c | 35 +++++++++++++++++++++++++++++----
2 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
index 4a34f558d964..db6d16e6bbc3 100644
--- a/security/safesetid/lsm.h
+++ b/security/safesetid/lsm.h
@@ -41,6 +41,7 @@ struct setuid_rule {
struct setuid_ruleset {
DECLARE_HASHTABLE(rules, SETID_HASH_BITS);
+ char *policy_str;
struct rcu_head rcu;
};
diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
index 250d59e046c1..997b403c6255 100644
--- a/security/safesetid/securityfs.c
+++ b/security/safesetid/securityfs.c
@@ -19,7 +19,7 @@
#include "lsm.h"
-static DEFINE_SPINLOCK(policy_update_lock);
+static DEFINE_MUTEX(policy_update_lock);
/*
* In the case the input buffer contains one or more invalid UIDs, the kuid_t
@@ -67,6 +67,7 @@ static void __release_ruleset(struct rcu_head *rcu)
hash_for_each_safe(pol->rules, bucket, tmp, rule, next)
kfree(rule);
+ kfree(pol->policy_str);
kfree(pol);
}
@@ -85,6 +86,7 @@ static ssize_t handle_policy_update(struct file *file,
pol = kmalloc(sizeof(struct setuid_ruleset), GFP_KERNEL);
if (!pol)
return -ENOMEM;
+ pol->policy_str = NULL;
hash_init(pol->rules);
p = buf = memdup_user_nul(ubuf, len);
@@ -92,6 +94,11 @@ static ssize_t handle_policy_update(struct file *file,
err = PTR_ERR(buf);
goto out_free_pol;
}
+ pol->policy_str = kstrdup(buf, GFP_KERNEL);
+ if (pol->policy_str == NULL) {
+ err = -ENOMEM;
+ goto out_free_buf;
+ }
/* policy lines, including the last one, end with \n */
while (*p != '\0') {
@@ -135,10 +142,10 @@ static ssize_t handle_policy_update(struct file *file,
* What we really want here is an xchg() wrapper for RCU, but since that
* doesn't currently exist, just use a spinlock for now.
*/
- spin_lock(&policy_update_lock);
+ mutex_lock(&policy_update_lock);
rcu_swap_protected(safesetid_setuid_rules, pol,
lockdep_is_held(&policy_update_lock));
- spin_unlock(&policy_update_lock);
+ mutex_unlock(&policy_update_lock);
err = len;
out_free_buf:
@@ -162,7 +169,27 @@ static ssize_t safesetid_file_write(struct file *file,
return handle_policy_update(file, buf, len);
}
+static ssize_t safesetid_file_read(struct file *file, char __user *buf,
+ size_t len, loff_t *ppos)
+{
+ ssize_t res = 0;
+ struct setuid_ruleset *pol;
+ const char *kbuf;
+
+ mutex_lock(&policy_update_lock);
+ pol = rcu_dereference_protected(safesetid_setuid_rules,
+ lockdep_is_held(&policy_update_lock));
+ if (pol) {
+ kbuf = pol->policy_str;
+ res = simple_read_from_buffer(buf, len, ppos,
+ kbuf, strlen(kbuf));
+ }
+ mutex_unlock(&policy_update_lock);
+ return res;
+}
+
static const struct file_operations safesetid_file_fops = {
+ .read = safesetid_file_read,
.write = safesetid_file_write,
};
@@ -181,7 +208,7 @@ static int __init safesetid_init_securityfs(void)
goto error;
}
- policy_file = securityfs_create_file("whitelist_policy", 0200,
+ policy_file = securityfs_create_file("whitelist_policy", 0600,
policy_dir, NULL, &safesetid_file_fops);
if (IS_ERR(policy_file)) {
ret = PTR_ERR(policy_file);
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP
From: Eric Biggers @ 2019-04-11 19:28 UTC (permalink / raw)
To: linux-crypto, Herbert Xu
Cc: Kees Cook, Dmitry Vyukov, Geert Uytterhoeven,
linux-security-module, Linux ARM, Linux Kernel Mailing List,
Laura Abbott, Rik van Riel
In-Reply-To: <20190411192607.GD225654@gmail.com>
From: Eric Biggers <ebiggers@google.com>
This is needed so that CONFIG_HARDENED_USERCOPY_PAGESPAN=y doesn't
incorrectly report a buffer overflow when the destination of
copy_from_iter() spans the page boundary in the 2-page buffer.
Fixes: 3f47a03df6e8 ("crypto: testmgr - add testvec_config struct and helper functions")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
crypto/testmgr.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 0f6bfb6ce6a46..3522c0bed2492 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -156,7 +156,8 @@ static int __testmgr_alloc_buf(char *buf[XBUFSIZE], int order)
int i;
for (i = 0; i < XBUFSIZE; i++) {
- buf[i] = (char *)__get_free_pages(GFP_KERNEL, order);
+ buf[i] = (char *)__get_free_pages(GFP_KERNEL | __GFP_COMP,
+ order);
if (!buf[i])
goto err_free_buf;
}
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages
From: Eric Biggers @ 2019-04-11 19:26 UTC (permalink / raw)
To: Kees Cook
Cc: Dmitry Vyukov, Geert Uytterhoeven, Herbert Xu,
linux-security-module, Linux ARM, Linux Crypto Mailing List,
Linux Kernel Mailing List, Laura Abbott, Rik van Riel
In-Reply-To: <CAGXu5j+u3f3qUVkQO+zYLN7BDh3r7w-W3k3aa+qdAP11k7A1dw@mail.gmail.com>
On Thu, Apr 11, 2019 at 11:33:04AM -0700, Kees Cook wrote:
> On Thu, Apr 11, 2019 at 10:58 AM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Wed, Apr 10, 2019 at 04:27:28PM -0700, Kees Cook wrote:
> > > On Wed, Apr 10, 2019 at 4:12 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > You've explained *what* it does again, but not *why*. *Why* do you want
> > > > hardened usercopy to detect copies across page boundaries, when there is no
> > > > actual buffer overflow?
> > >
> > > But that *is* how it determines it was a buffer overflow: "if you
> > > cross page boundaries (of a non-compound allocation), it *is* a buffer
> > > overflow". This assertion, however, is flawed because many contiguous
> > > allocations are not marked as being grouped together when it reality
> > > they were. It was an attempt to get allocation size information out of
> > > the page allocator, similar to how slab can be queries about
> > > allocation size. I'm open to improvements here, since it's obviously
> > > broken in its current state. :)
> > >
> > > --
> > > Kees Cook
> >
> > Well, I'm still at a loss as to whether I'm actually supposed to "fix" this by
> > adding __GFP_COMP, or whether you're saying the option is broken anyway so I
>
> I would love it if you could fix it, yes.
>
> > shouldn't bother doing anything. IIUC, even the kernel stack is still not
> > marked __GFP_COMP, so copies to/from the stack can trigger this too, despite
> > this being reported over 2 years ago
> > (http://lkml.iu.edu/hypermail/linux/kernel/1701.2/01450.html)?
> > CONFIG_HARDENED_USERCOPY_PAGESPAN is even disabled in syzbot because you already
> > said the option is broken and should not be used.
>
> stacks are checked before PAGESPAN, so that particular problem should
> no longer be present since commit 7bff3c069973 ("mm/usercopy.c: no
> check page span for stack objects").
>
> > I worry that people will enable all the hardened usercopy options "because
> > security", then when the pagespan check breaks something they will disable all
> > hardened usercopy options, because they don't understand the individual options.
> > Providing broken options is actively harmful, IMO.
>
> It's behind EXPERT, default-n, and says:
>
> When a multi-page allocation is done without __GFP_COMP,
> hardened usercopy will reject attempts to copy it. There are,
> however, several cases of this in the kernel that have not all
> been removed. This config is intended to be used only while
> trying to find such users.
>
> I'd rather leave it since it's still useful.
>
> Perhaps it could be switched to WARN by default and we reenable it in
> syzbot to improve its utility there?
>
> diff --git a/mm/usercopy.c b/mm/usercopy.c
> index 14faadcedd06..6e7e28fe062b 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -208,8 +208,13 @@ static inline void check_page_span(const void
> *ptr, unsigned long n,
> */
> is_reserved = PageReserved(page);
> is_cma = is_migrate_cma_page(page);
> - if (!is_reserved && !is_cma)
> - usercopy_abort("spans multiple pages", NULL, to_user, 0, n);
> + if (!is_reserved && !is_cma) {
> + usercopy_warn("spans multiple pages without __GFP_COMP",
> + NULL, to_user,
> + (unsigned long)ptr & (unsigned long)PAGE_MASK,
> + n);
> + return;
> + }
>
> for (ptr += PAGE_SIZE; ptr <= end; ptr += PAGE_SIZE) {
> page = virt_to_head_page(ptr);
>
>
> --
> Kees Cook
Well, I guess I'll just add __GFP_COMP so I at least don't get spammed with
useless bug reports.
But I don't think it's in any way acceptable to change the semantics of the
kernel's page allocator but only under some obscure config option, don't
document it anywhere, ignore the known problems for years, say that the option
is broken anyway so it shouldn't be used, and have to exchange 15 emails to get
anything resembling an explanation.
- Eric
^ permalink raw reply
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages
From: Kees Cook @ 2019-04-11 18:33 UTC (permalink / raw)
To: Eric Biggers, Dmitry Vyukov
Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM,
Linux Crypto Mailing List, Linux Kernel Mailing List,
Laura Abbott, Rik van Riel
In-Reply-To: <20190411175823.GC225654@gmail.com>
On Thu, Apr 11, 2019 at 10:58 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Wed, Apr 10, 2019 at 04:27:28PM -0700, Kees Cook wrote:
> > On Wed, Apr 10, 2019 at 4:12 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > You've explained *what* it does again, but not *why*. *Why* do you want
> > > hardened usercopy to detect copies across page boundaries, when there is no
> > > actual buffer overflow?
> >
> > But that *is* how it determines it was a buffer overflow: "if you
> > cross page boundaries (of a non-compound allocation), it *is* a buffer
> > overflow". This assertion, however, is flawed because many contiguous
> > allocations are not marked as being grouped together when it reality
> > they were. It was an attempt to get allocation size information out of
> > the page allocator, similar to how slab can be queries about
> > allocation size. I'm open to improvements here, since it's obviously
> > broken in its current state. :)
> >
> > --
> > Kees Cook
>
> Well, I'm still at a loss as to whether I'm actually supposed to "fix" this by
> adding __GFP_COMP, or whether you're saying the option is broken anyway so I
I would love it if you could fix it, yes.
> shouldn't bother doing anything. IIUC, even the kernel stack is still not
> marked __GFP_COMP, so copies to/from the stack can trigger this too, despite
> this being reported over 2 years ago
> (http://lkml.iu.edu/hypermail/linux/kernel/1701.2/01450.html)?
> CONFIG_HARDENED_USERCOPY_PAGESPAN is even disabled in syzbot because you already
> said the option is broken and should not be used.
stacks are checked before PAGESPAN, so that particular problem should
no longer be present since commit 7bff3c069973 ("mm/usercopy.c: no
check page span for stack objects").
> I worry that people will enable all the hardened usercopy options "because
> security", then when the pagespan check breaks something they will disable all
> hardened usercopy options, because they don't understand the individual options.
> Providing broken options is actively harmful, IMO.
It's behind EXPERT, default-n, and says:
When a multi-page allocation is done without __GFP_COMP,
hardened usercopy will reject attempts to copy it. There are,
however, several cases of this in the kernel that have not all
been removed. This config is intended to be used only while
trying to find such users.
I'd rather leave it since it's still useful.
Perhaps it could be switched to WARN by default and we reenable it in
syzbot to improve its utility there?
diff --git a/mm/usercopy.c b/mm/usercopy.c
index 14faadcedd06..6e7e28fe062b 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -208,8 +208,13 @@ static inline void check_page_span(const void
*ptr, unsigned long n,
*/
is_reserved = PageReserved(page);
is_cma = is_migrate_cma_page(page);
- if (!is_reserved && !is_cma)
- usercopy_abort("spans multiple pages", NULL, to_user, 0, n);
+ if (!is_reserved && !is_cma) {
+ usercopy_warn("spans multiple pages without __GFP_COMP",
+ NULL, to_user,
+ (unsigned long)ptr & (unsigned long)PAGE_MASK,
+ n);
+ return;
+ }
for (ptr += PAGE_SIZE; ptr <= end; ptr += PAGE_SIZE) {
page = virt_to_head_page(ptr);
--
Kees Cook
^ permalink raw reply related
* [PATCH v2 1/3] security: Create "kernel hardening" config area
From: Kees Cook @ 2019-04-11 18:01 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Kees Cook, Masahiro Yamada, James Morris, Alexander Popov,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Randy Dunlap, Michal Marek, Emese Revfy,
Serge E. Hallyn, kernel-hardening, linux-security-module,
linux-kbuild, linux-kernel
In-Reply-To: <20190411180117.27704-1-keescook@chromium.org>
Right now kernel hardening options are scattered around various Kconfig
files. This can be a central place to collect these kinds of options
going forward. This is initially populated with the memory initialization
options from the gcc-plugins.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
scripts/gcc-plugins/Kconfig | 74 +++--------------------------
security/Kconfig | 2 +
security/Kconfig.hardening | 93 +++++++++++++++++++++++++++++++++++++
3 files changed, 102 insertions(+), 67 deletions(-)
create mode 100644 security/Kconfig.hardening
diff --git a/scripts/gcc-plugins/Kconfig b/scripts/gcc-plugins/Kconfig
index 74271dba4f94..84d471dea2b7 100644
--- a/scripts/gcc-plugins/Kconfig
+++ b/scripts/gcc-plugins/Kconfig
@@ -13,10 +13,11 @@ config HAVE_GCC_PLUGINS
An arch should select this symbol if it supports building with
GCC plugins.
-menuconfig GCC_PLUGINS
- bool "GCC plugins"
+config GCC_PLUGINS
+ bool
depends on HAVE_GCC_PLUGINS
depends on PLUGIN_HOSTCC != ""
+ default y
help
GCC plugins are loadable modules that provide extra features to the
compiler. They are useful for runtime instrumentation and static analysis.
@@ -25,6 +26,8 @@ menuconfig GCC_PLUGINS
if GCC_PLUGINS
+menu "GCC plugins"
+
config GCC_PLUGIN_CYC_COMPLEXITY
bool "Compute the cyclomatic complexity of a function" if EXPERT
depends on !COMPILE_TEST # too noisy
@@ -66,71 +69,6 @@ config GCC_PLUGIN_LATENT_ENTROPY
* https://grsecurity.net/
* https://pax.grsecurity.net/
-config GCC_PLUGIN_STRUCTLEAK
- bool "Zero initialize stack variables"
- help
- While the kernel is built with warnings enabled for any missed
- stack variable initializations, this warning is silenced for
- anything passed by reference to another function, under the
- occasionally misguided assumption that the function will do
- the initialization. As this regularly leads to exploitable
- flaws, this plugin is available to identify and zero-initialize
- such variables, depending on the chosen level of coverage.
-
- This plugin was originally ported from grsecurity/PaX. More
- information at:
- * https://grsecurity.net/
- * https://pax.grsecurity.net/
-
-choice
- prompt "Coverage"
- depends on GCC_PLUGIN_STRUCTLEAK
- default GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
- help
- This chooses the level of coverage over classes of potentially
- uninitialized variables. The selected class will be
- zero-initialized before use.
-
- config GCC_PLUGIN_STRUCTLEAK_USER
- bool "structs marked for userspace"
- help
- Zero-initialize any structures on the stack containing
- a __user attribute. This can prevent some classes of
- uninitialized stack variable exploits and information
- exposures, like CVE-2013-2141:
- https://git.kernel.org/linus/b9e146d8eb3b9eca
-
- config GCC_PLUGIN_STRUCTLEAK_BYREF
- bool "structs passed by reference"
- help
- Zero-initialize any structures on the stack that may
- be passed by reference and had not already been
- explicitly initialized. This can prevent most classes
- of uninitialized stack variable exploits and information
- exposures, like CVE-2017-1000410:
- https://git.kernel.org/linus/06e7e776ca4d3654
-
- config GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
- bool "anything passed by reference"
- help
- Zero-initialize any stack variables that may be passed
- by reference and had not already been explicitly
- initialized. This is intended to eliminate all classes
- of uninitialized stack variable exploits and information
- exposures.
-
-endchoice
-
-config GCC_PLUGIN_STRUCTLEAK_VERBOSE
- bool "Report forcefully initialized variables"
- depends on GCC_PLUGIN_STRUCTLEAK
- depends on !COMPILE_TEST # too noisy
- help
- This option will cause a warning to be printed each time the
- structleak plugin finds a variable it thinks needs to be
- initialized. Since not all existing initializers are detected
- by the plugin, this can produce false positive warnings.
-
config GCC_PLUGIN_RANDSTRUCT
bool "Randomize layout of sensitive kernel structures"
select MODVERSIONS if MODULES
@@ -226,4 +164,6 @@ config GCC_PLUGIN_ARM_SSP_PER_TASK
bool
depends on GCC_PLUGINS && ARM
+endmenu
+
endif
diff --git a/security/Kconfig b/security/Kconfig
index 1d6463fb1450..7aec8d094ce2 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -249,5 +249,7 @@ config LSM
If unsure, leave this as the default.
+source "security/Kconfig.hardening"
+
endmenu
diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
new file mode 100644
index 000000000000..01a119437dfc
--- /dev/null
+++ b/security/Kconfig.hardening
@@ -0,0 +1,93 @@
+menu "Kernel hardening options"
+
+config GCC_PLUGIN_STRUCTLEAK
+ bool
+ help
+ While the kernel is built with warnings enabled for any missed
+ stack variable initializations, this warning is silenced for
+ anything passed by reference to another function, under the
+ occasionally misguided assumption that the function will do
+ the initialization. As this regularly leads to exploitable
+ flaws, this plugin is available to identify and zero-initialize
+ such variables, depending on the chosen level of coverage.
+
+ This plugin was originally ported from grsecurity/PaX. More
+ information at:
+ * https://grsecurity.net/
+ * https://pax.grsecurity.net/
+
+menu "Memory initialization"
+
+choice
+ prompt "Initialize kernel stack variables at function entry"
+ depends on GCC_PLUGINS
+ default INIT_STACK_NONE
+ help
+ This option enables initialization of stack variables at
+ function entry time. This has the possibility to have the
+ greatest coverage (since all functions can have their
+ variables initialized), but the performance impact depends
+ on the function calling complexity of a given workload's
+ syscalls.
+
+ This chooses the level of coverage over classes of potentially
+ uninitialized variables. The selected class will be
+ initialized before use in a function.
+
+ config INIT_STACK_NONE
+ bool "no automatic initialization (weakest)"
+ help
+ Disable automatic stack variable initialization.
+ This leaves the kernel vulnerable to the standard
+ classes of uninitialized stack variable exploits
+ and information exposures.
+
+ config GCC_PLUGIN_STRUCTLEAK_USER
+ bool "zero-init structs marked for userspace (weak)"
+ depends on GCC_PLUGINS
+ select GCC_PLUGIN_STRUCTLEAK
+ help
+ Zero-initialize any structures on the stack containing
+ a __user attribute. This can prevent some classes of
+ uninitialized stack variable exploits and information
+ exposures, like CVE-2013-2141:
+ https://git.kernel.org/linus/b9e146d8eb3b9eca
+
+ config GCC_PLUGIN_STRUCTLEAK_BYREF
+ bool "zero-init structs passed by reference (strong)"
+ depends on GCC_PLUGINS
+ select GCC_PLUGIN_STRUCTLEAK
+ help
+ Zero-initialize any structures on the stack that may
+ be passed by reference and had not already been
+ explicitly initialized. This can prevent most classes
+ of uninitialized stack variable exploits and information
+ exposures, like CVE-2017-1000410:
+ https://git.kernel.org/linus/06e7e776ca4d3654
+
+ config GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
+ bool "zero-init anything passed by reference (very strong)"
+ depends on GCC_PLUGINS
+ select GCC_PLUGIN_STRUCTLEAK
+ help
+ Zero-initialize any stack variables that may be passed
+ by reference and had not already been explicitly
+ initialized. This is intended to eliminate all classes
+ of uninitialized stack variable exploits and information
+ exposures.
+
+endchoice
+
+config GCC_PLUGIN_STRUCTLEAK_VERBOSE
+ bool "Report forcefully initialized variables"
+ depends on GCC_PLUGIN_STRUCTLEAK
+ depends on !COMPILE_TEST # too noisy
+ help
+ This option will cause a warning to be printed each time the
+ structleak plugin finds a variable it thinks needs to be
+ initialized. Since not all existing initializers are detected
+ by the plugin, this can produce false positive warnings.
+
+endmenu
+
+endmenu
--
2.17.1
^ permalink raw reply related
* [PATCH v2 2/3] security: Move stackleak config to Kconfig.hardening
From: Kees Cook @ 2019-04-11 18:01 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Kees Cook, Masahiro Yamada, James Morris, Alexander Popov,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Randy Dunlap, Michal Marek, Emese Revfy,
Serge E. Hallyn, kernel-hardening, linux-security-module,
linux-kbuild, linux-kernel
In-Reply-To: <20190411180117.27704-1-keescook@chromium.org>
This moves the stackleak plugin options to Kconfig.hardening's memory
initialization menu.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
scripts/gcc-plugins/Kconfig | 51 ---------------------------------
security/Kconfig.hardening | 57 +++++++++++++++++++++++++++++++++++++
2 files changed, 57 insertions(+), 51 deletions(-)
diff --git a/scripts/gcc-plugins/Kconfig b/scripts/gcc-plugins/Kconfig
index 84d471dea2b7..e4cb58d5a73f 100644
--- a/scripts/gcc-plugins/Kconfig
+++ b/scripts/gcc-plugins/Kconfig
@@ -109,57 +109,6 @@ config GCC_PLUGIN_RANDSTRUCT_PERFORMANCE
in structures. This reduces the performance hit of RANDSTRUCT
at the cost of weakened randomization.
-config GCC_PLUGIN_STACKLEAK
- bool "Erase the kernel stack before returning from syscalls"
- depends on GCC_PLUGINS
- depends on HAVE_ARCH_STACKLEAK
- help
- This option makes the kernel erase the kernel stack before
- returning from system calls. That reduces the information which
- kernel stack leak bugs can reveal and blocks some uninitialized
- stack variable attacks.
-
- The tradeoff is the performance impact: on a single CPU system kernel
- compilation sees a 1% slowdown, other systems and workloads may vary
- and you are advised to test this feature on your expected workload
- before deploying it.
-
- This plugin was ported from grsecurity/PaX. More information at:
- * https://grsecurity.net/
- * https://pax.grsecurity.net/
-
-config STACKLEAK_TRACK_MIN_SIZE
- int "Minimum stack frame size of functions tracked by STACKLEAK"
- default 100
- range 0 4096
- depends on GCC_PLUGIN_STACKLEAK
- help
- The STACKLEAK gcc plugin instruments the kernel code for tracking
- the lowest border of the kernel stack (and for some other purposes).
- It inserts the stackleak_track_stack() call for the functions with
- a stack frame size greater than or equal to this parameter.
- If unsure, leave the default value 100.
-
-config STACKLEAK_METRICS
- bool "Show STACKLEAK metrics in the /proc file system"
- depends on GCC_PLUGIN_STACKLEAK
- depends on PROC_FS
- help
- If this is set, STACKLEAK metrics for every task are available in
- the /proc file system. In particular, /proc/<pid>/stack_depth
- shows the maximum kernel stack consumption for the current and
- previous syscalls. Although this information is not precise, it
- can be useful for estimating the STACKLEAK performance impact for
- your workloads.
-
-config STACKLEAK_RUNTIME_DISABLE
- bool "Allow runtime disabling of kernel stack erasing"
- depends on GCC_PLUGIN_STACKLEAK
- help
- This option provides 'stack_erasing' sysctl, which can be used in
- runtime to control kernel stack erasing for kernels built with
- CONFIG_GCC_PLUGIN_STACKLEAK.
-
config GCC_PLUGIN_ARM_SSP_PER_TASK
bool
depends on GCC_PLUGINS && ARM
diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
index 01a119437dfc..3dd7a28c3822 100644
--- a/security/Kconfig.hardening
+++ b/security/Kconfig.hardening
@@ -88,6 +88,63 @@ config GCC_PLUGIN_STRUCTLEAK_VERBOSE
initialized. Since not all existing initializers are detected
by the plugin, this can produce false positive warnings.
+config GCC_PLUGIN_STACKLEAK
+ bool "Poison kernel stack before returning from syscalls"
+ depends on GCC_PLUGINS
+ depends on HAVE_ARCH_STACKLEAK
+ help
+ This option makes the kernel erase the kernel stack before
+ returning from system calls. This has the effect of leaving
+ the stack initialized to the poison value, which both reduces
+ the lifetime of any sensitive stack contents and reduces
+ potential for uninitialized stack variable exploits or information
+ exposures (it does not cover functions reaching the same stack
+ depth as prior functions during the same syscall). This blocks
+ most uninitialized stack variable attacks, with the performance
+ impact being driven by the depth of the stack usage, rather than
+ the function calling complexity.
+
+ The performance impact on a single CPU system kernel compilation
+ sees a 1% slowdown, other systems and workloads may vary and you
+ are advised to test this feature on your expected workload before
+ deploying it.
+
+ This plugin was ported from grsecurity/PaX. More information at:
+ * https://grsecurity.net/
+ * https://pax.grsecurity.net/
+
+config STACKLEAK_TRACK_MIN_SIZE
+ int "Minimum stack frame size of functions tracked by STACKLEAK"
+ default 100
+ range 0 4096
+ depends on GCC_PLUGIN_STACKLEAK
+ help
+ The STACKLEAK gcc plugin instruments the kernel code for tracking
+ the lowest border of the kernel stack (and for some other purposes).
+ It inserts the stackleak_track_stack() call for the functions with
+ a stack frame size greater than or equal to this parameter.
+ If unsure, leave the default value 100.
+
+config STACKLEAK_METRICS
+ bool "Show STACKLEAK metrics in the /proc file system"
+ depends on GCC_PLUGIN_STACKLEAK
+ depends on PROC_FS
+ help
+ If this is set, STACKLEAK metrics for every task are available in
+ the /proc file system. In particular, /proc/<pid>/stack_depth
+ shows the maximum kernel stack consumption for the current and
+ previous syscalls. Although this information is not precise, it
+ can be useful for estimating the STACKLEAK performance impact for
+ your workloads.
+
+config STACKLEAK_RUNTIME_DISABLE
+ bool "Allow runtime disabling of kernel stack erasing"
+ depends on GCC_PLUGIN_STACKLEAK
+ help
+ This option provides 'stack_erasing' sysctl, which can be used in
+ runtime to control kernel stack erasing for kernels built with
+ CONFIG_GCC_PLUGIN_STACKLEAK.
+
endmenu
endmenu
--
2.17.1
^ permalink raw reply related
* [PATCH v2 3/3] security: Implement Clang's stack initialization
From: Kees Cook @ 2019-04-11 18:01 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Kees Cook, Masahiro Yamada, James Morris, Alexander Popov,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Randy Dunlap, Michal Marek, Emese Revfy,
Serge E. Hallyn, kernel-hardening, linux-security-module,
linux-kbuild, linux-kernel
In-Reply-To: <20190411180117.27704-1-keescook@chromium.org>
CONFIG_INIT_STACK_ALL turns on stack initialization based on
-ftrivial-auto-var-init in Clang builds, which has greater coverage
than CONFIG_GCC_PLUGINS_STRUCTLEAK_BYREF_ALL.
-ftrivial-auto-var-init Clang option provides trivial initializers for
uninitialized local variables, variable fields and padding.
It has three possible values:
pattern - uninitialized locals are filled with a fixed pattern
(mostly 0xAA on 64-bit platforms, see https://reviews.llvm.org/D54604
for more details, but 0x000000AA for 32-bit pointers) likely to cause
crashes when uninitialized value is used;
zero (it's still debated whether this flag makes it to the official
Clang release) - uninitialized locals are filled with zeroes;
uninitialized (default) - uninitialized locals are left intact.
This patch uses only the "pattern" mode when CONFIG_INIT_STACK_ALL is
enabled.
Developers have the possibility to opt-out of this feature on a
per-variable basis by using __attribute__((uninitialized)), but such
use should be well justified in comments.
Co-developed-by: Alexander Potapenko <glider@google.com>
Signed-off-by: Alexander Potapenko <glider@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
Makefile | 5 +++++
security/Kconfig.hardening | 15 ++++++++++++++-
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index c0a34064c574..a7d9c6cd0267 100644
--- a/Makefile
+++ b/Makefile
@@ -745,6 +745,11 @@ KBUILD_CFLAGS += -fomit-frame-pointer
endif
endif
+# Initialize all stack variables with a pattern, if desired.
+ifdef CONFIG_INIT_STACK_ALL
+KBUILD_CFLAGS += -ftrivial-auto-var-init=pattern
+endif
+
DEBUG_CFLAGS := $(call cc-option, -fno-var-tracking-assignments)
ifdef CONFIG_DEBUG_INFO
diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
index 3dd7a28c3822..5dd61770d3f0 100644
--- a/security/Kconfig.hardening
+++ b/security/Kconfig.hardening
@@ -18,9 +18,12 @@ config GCC_PLUGIN_STRUCTLEAK
menu "Memory initialization"
+config CC_HAS_AUTO_VAR_INIT
+ def_bool $(cc-option,-ftrivial-auto-var-init=pattern)
+
choice
prompt "Initialize kernel stack variables at function entry"
- depends on GCC_PLUGINS
+ depends on CC_HAS_AUTO_VAR_INIT || GCC_PLUGINS
default INIT_STACK_NONE
help
This option enables initialization of stack variables at
@@ -76,6 +79,16 @@ choice
of uninitialized stack variable exploits and information
exposures.
+ config INIT_STACK_ALL
+ bool "0xAA-init everything on the stack (strongest)"
+ depends on CC_HAS_AUTO_VAR_INIT
+ help
+ Initializes everything on the stack with a 0xAA
+ pattern. This is intended to eliminate all classes
+ of uninitialized stack variable exploits and information
+ exposures, even variables that were warned to have been
+ left uninitialized.
+
endchoice
config GCC_PLUGIN_STRUCTLEAK_VERBOSE
--
2.17.1
^ permalink raw reply related
* [PATCH v2 0/3] Refactor memory initialization hardening
From: Kees Cook @ 2019-04-11 18:01 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Kees Cook, Masahiro Yamada, James Morris, Alexander Popov,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Randy Dunlap, Michal Marek, Emese Revfy,
Serge E. Hallyn, kernel-hardening, linux-security-module,
linux-kbuild, linux-kernel
This is a proposed alternative for the memory initialization series,
which refactoring the existing gcc plugins into a separate Kconfig
file and collects all the related options together with some more
language to describe their differences. The last patch adds the
Clang auto init option, as done by Alexander Potapenko.
Since there isn't really a good way to "select" with dependencies,
I've left out CONFIG_INIT_ALL_MEMORY for the moment...
I intend to carry this in the gcc-plugins tree, but I'd really like
to get Acks from Masahiro (Kconfig changes, Makefile change), and
from James (adding the new Kconfig.hardening to security/Kconfig).
Thanks!
-Kees
v2:
- add plugin menu (masahiro)
- adjust patch subject prefixes (masahiro)
- drop redundent "depends" (masahiro)
- fixed early use of CC_HAS_AUTO_VAR_INIT (masahiro)
- dropped default-enabled for STACK_INIT_ALL (masahiro)
Kees Cook (3):
security: Create "kernel hardening" config area
security: Move stackleak config to Kconfig.hardening
security: Implement Clang's stack initialization
Makefile | 5 ++
scripts/gcc-plugins/Kconfig | 125 ++-------------------------
security/Kconfig | 2 +
security/Kconfig.hardening | 163 ++++++++++++++++++++++++++++++++++++
4 files changed, 177 insertions(+), 118 deletions(-)
create mode 100644 security/Kconfig.hardening
--
2.17.1
^ permalink raw reply
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages
From: Eric Biggers @ 2019-04-11 17:58 UTC (permalink / raw)
To: Kees Cook
Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM,
Linux Crypto Mailing List, Linux Kernel Mailing List,
Laura Abbott, Rik van Riel
In-Reply-To: <CAGXu5jKg+oN7yUY=N69U93afOVHBPgwz6Osf=eMDPfiwbzEqmw@mail.gmail.com>
On Wed, Apr 10, 2019 at 04:27:28PM -0700, Kees Cook wrote:
> On Wed, Apr 10, 2019 at 4:12 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > You've explained *what* it does again, but not *why*. *Why* do you want
> > hardened usercopy to detect copies across page boundaries, when there is no
> > actual buffer overflow?
>
> But that *is* how it determines it was a buffer overflow: "if you
> cross page boundaries (of a non-compound allocation), it *is* a buffer
> overflow". This assertion, however, is flawed because many contiguous
> allocations are not marked as being grouped together when it reality
> they were. It was an attempt to get allocation size information out of
> the page allocator, similar to how slab can be queries about
> allocation size. I'm open to improvements here, since it's obviously
> broken in its current state. :)
>
> --
> Kees Cook
Well, I'm still at a loss as to whether I'm actually supposed to "fix" this by
adding __GFP_COMP, or whether you're saying the option is broken anyway so I
shouldn't bother doing anything. IIUC, even the kernel stack is still not
marked __GFP_COMP, so copies to/from the stack can trigger this too, despite
this being reported over 2 years ago
(http://lkml.iu.edu/hypermail/linux/kernel/1701.2/01450.html)?
CONFIG_HARDENED_USERCOPY_PAGESPAN is even disabled in syzbot because you already
said the option is broken and should not be used.
I worry that people will enable all the hardened usercopy options "because
security", then when the pagespan check breaks something they will disable all
hardened usercopy options, because they don't understand the individual options.
Providing broken options is actively harmful, IMO.
- Eric
^ permalink raw reply
* Re: [PATCH v4 2/3] initmem: introduce CONFIG_INIT_ALL_HEAP
From: Alexander Potapenko @ 2019-04-11 17:40 UTC (permalink / raw)
To: Kees Cook
Cc: Masahiro Yamada, James Morris, Serge E. Hallyn,
linux-security-module, linux-kbuild, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Kernel Hardening
In-Reply-To: <CAGXu5jLEGmMVTJEJHFM3z0EVBAryK3x8Uz4K+mMaOJhfuL9Acg@mail.gmail.com>
On Thu, Apr 11, 2019 at 7:29 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Apr 11, 2019 at 1:39 AM Alexander Potapenko <glider@google.com> wrote:
> >
> > On Wed, Apr 10, 2019 at 6:09 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Wed, Apr 10, 2019 at 6:18 AM Alexander Potapenko <glider@google.com> wrote:
> > > >
> > > > This config option adds the possibility to initialize newly allocated
> > > > pages and heap objects with a 0xAA pattern.
> > > > There's already a number of places where allocations are initialized
> > > > based on the presence of __GFP_ZERO flag. We just change this code so
> > > > that under CONFIG_INIT_ALL_HEAP these allocations are always initialized
> > > > with either 0x00 or 0xAA depending on the __GFP_ZERO.
> > >
> > > Why not just make __GFP_ZERO unconditional instead? This looks like
> > > it'd be simpler and not need arch-specific implementation?
> >
> > Right, but it would mean we can only initialize with 0x00 pattern.
> > I believe that for testing purposes a nonzero pattern is better,
>
> Can it be implemented in a way that isn't arch-specific? I'd really
> like to have a general solution that works immediately for all
> architectures. (Can't everything just use a memset?)
>
> > because it'll not only assure the execution is deterministic, but will
> > also uncover logic bugs earlier (see the discussion at
> > https://reviews.llvm.org/D54604?id=174471)
> > For hardening purposes the pattern shouldn't matter much.
>
> So, for hardening, it actually does matter but only in certain cases.
> On 64-bit, a 0xAA... pointer will have the high bit set, so it'll land
> in the non-canonical memory range, which is good. For 32-bit, 0xAA...
> will be in userspace (TASK_SIZE is 0xC0000000). In the above URL I see
> now that 32-bit pointer init gets 0x000000AA, which is good, but for
> heap init, types aren't known. So perhaps use 0x000000AA for 32-bit
> and 0xAA... for 64-bit heap init? (0xAA... has stronger properties
> since there have been NULL page mapping bypass flaws in the (recent!)
> past, so I could see keeping that for 64-bit instead of just using
> 0-init everywhere.)
>
> > If you think arch-specific code is too much of a trouble, we could
> > implement clear_page_pattern() using memset() on every architecture,
> > but allow the user to choose between slow (0xAA) and production (0x00)
> > modes.
>
> How about 32-bit use 0x00, 64-bit use 0xAA (and provide per-arch
> speed-ups with a generic "slow" version for all the other
> architectures?)
Might be easier to start with a generic 0x00 version and add
improvements on top of that :)
I'll send an updated patch.
> -Kees
>
> --
> Kees Cook
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: [PATCH v4 2/3] initmem: introduce CONFIG_INIT_ALL_HEAP
From: Kees Cook @ 2019-04-11 17:29 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Masahiro Yamada, James Morris, Serge E. Hallyn,
linux-security-module, linux-kbuild, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Kernel Hardening
In-Reply-To: <CAG_fn=Vkod40bxxP8PU-+zXs2u4arqAB_bYxqei1bb_tgsg8GA@mail.gmail.com>
On Thu, Apr 11, 2019 at 1:39 AM Alexander Potapenko <glider@google.com> wrote:
>
> On Wed, Apr 10, 2019 at 6:09 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Wed, Apr 10, 2019 at 6:18 AM Alexander Potapenko <glider@google.com> wrote:
> > >
> > > This config option adds the possibility to initialize newly allocated
> > > pages and heap objects with a 0xAA pattern.
> > > There's already a number of places where allocations are initialized
> > > based on the presence of __GFP_ZERO flag. We just change this code so
> > > that under CONFIG_INIT_ALL_HEAP these allocations are always initialized
> > > with either 0x00 or 0xAA depending on the __GFP_ZERO.
> >
> > Why not just make __GFP_ZERO unconditional instead? This looks like
> > it'd be simpler and not need arch-specific implementation?
>
> Right, but it would mean we can only initialize with 0x00 pattern.
> I believe that for testing purposes a nonzero pattern is better,
Can it be implemented in a way that isn't arch-specific? I'd really
like to have a general solution that works immediately for all
architectures. (Can't everything just use a memset?)
> because it'll not only assure the execution is deterministic, but will
> also uncover logic bugs earlier (see the discussion at
> https://reviews.llvm.org/D54604?id=174471)
> For hardening purposes the pattern shouldn't matter much.
So, for hardening, it actually does matter but only in certain cases.
On 64-bit, a 0xAA... pointer will have the high bit set, so it'll land
in the non-canonical memory range, which is good. For 32-bit, 0xAA...
will be in userspace (TASK_SIZE is 0xC0000000). In the above URL I see
now that 32-bit pointer init gets 0x000000AA, which is good, but for
heap init, types aren't known. So perhaps use 0x000000AA for 32-bit
and 0xAA... for 64-bit heap init? (0xAA... has stronger properties
since there have been NULL page mapping bypass flaws in the (recent!)
past, so I could see keeping that for 64-bit instead of just using
0-init everywhere.)
> If you think arch-specific code is too much of a trouble, we could
> implement clear_page_pattern() using memset() on every architecture,
> but allow the user to choose between slow (0xAA) and production (0x00)
> modes.
How about 32-bit use 0x00, 64-bit use 0xAA (and provide per-arch
speed-ups with a generic "slow" version for all the other
architectures?)
-Kees
--
Kees Cook
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox