LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] mm/secretmem: use refcount_t instead of atomic_t
@ 2021-08-20  4:33 Jordy Zomer
  2021-08-20  5:33 ` Kees Cook
  2021-08-20 14:57 ` James Bottomley
  0 siblings, 2 replies; 8+ messages in thread
From: Jordy Zomer @ 2021-08-20  4:33 UTC (permalink / raw)
  To: linux-kernel; +Cc: Kees Cook, Jordy Zomer, Andrew Morton, linux-mm

When a secret memory region is active, memfd_secret disables
hibernation. One of the goals is to keep the secret data from being
written to persistent-storage.

It accomplishes this by maintaining a reference count to
`secretmem_users`. Once this reference is held your system can not be
hibernated due to the check in `hibernation_available()`. However,
because `secretmem_users` is of type `atomic_t`, reference counter
overflows are possible.

As you can see there's an `atomic_inc` for each `memfd` that is opened
in the `memfd_secret` syscall. If a local attacker succeeds to open 2^32
memfd's, the counter will wrap around to 0. This implies that you may
hibernate again, even though there are still regions of this secret
memory, thereby bypassing the security check.

In an attempt to fix this I have used `refcount_t` instead of `atomic_t`
which prevents reference counter overflows.

Signed-off-by: Jordy Zomer <jordy@pwning.systems>
---
 mm/secretmem.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/mm/secretmem.c b/mm/secretmem.c
index 030f02ddc7c1..1fea68b8d5a6 100644
--- a/mm/secretmem.c
+++ b/mm/secretmem.c
@@ -18,6 +18,7 @@
 #include <linux/secretmem.h>
 #include <linux/set_memory.h>
 #include <linux/sched/signal.h>
+#include <linux/refcount.h>
 
 #include <uapi/linux/magic.h>
 
@@ -40,11 +41,11 @@ module_param_named(enable, secretmem_enable, bool, 0400);
 MODULE_PARM_DESC(secretmem_enable,
 		 "Enable secretmem and memfd_secret(2) system call");
 
-static atomic_t secretmem_users;
+static refcount_t secretmem_users;
 
 bool secretmem_active(void)
 {
-	return !!atomic_read(&secretmem_users);
+	return !!refcount_read(&secretmem_users);
 }
 
 static vm_fault_t secretmem_fault(struct vm_fault *vmf)
@@ -103,7 +104,7 @@ static const struct vm_operations_struct secretmem_vm_ops = {
 
 static int secretmem_release(struct inode *inode, struct file *file)
 {
-	atomic_dec(&secretmem_users);
+	refcount_dec(&secretmem_users);
 	return 0;
 }
 
@@ -217,7 +218,7 @@ SYSCALL_DEFINE1(memfd_secret, unsigned int, flags)
 	file->f_flags |= O_LARGEFILE;
 
 	fd_install(fd, file);
-	atomic_inc(&secretmem_users);
+	refcount_inc(&secretmem_users);
 	return fd;
 
 err_put_fd:
-- 
2.27.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] mm/secretmem: use refcount_t instead of atomic_t
  2021-08-20  4:33 [PATCH] mm/secretmem: use refcount_t instead of atomic_t Jordy Zomer
@ 2021-08-20  5:33 ` Kees Cook
  2021-08-24 14:05   ` Mike Rapoport
  2021-08-20 14:57 ` James Bottomley
  1 sibling, 1 reply; 8+ messages in thread
From: Kees Cook @ 2021-08-20  5:33 UTC (permalink / raw)
  To: Jordy Zomer
  Cc: linux-kernel, Andrew Morton, linux-mm, Mike Rapoport, James Bottomley

On Fri, Aug 20, 2021 at 06:33:38AM +0200, Jordy Zomer wrote:
> When a secret memory region is active, memfd_secret disables
> hibernation. One of the goals is to keep the secret data from being
> written to persistent-storage.
> 
> It accomplishes this by maintaining a reference count to
> `secretmem_users`. Once this reference is held your system can not be
> hibernated due to the check in `hibernation_available()`. However,
> because `secretmem_users` is of type `atomic_t`, reference counter
> overflows are possible.

It's an unlikely condition to hit given max-open-fds, etc, but there's
no reason to leave this weakness. Changing this to refcount_t is easy
and better than using atomic_t.

Reviewed-by: Kees Cook <keescook@chromium.org>

> As you can see there's an `atomic_inc` for each `memfd` that is opened
> in the `memfd_secret` syscall. If a local attacker succeeds to open 2^32
> memfd's, the counter will wrap around to 0. This implies that you may
> hibernate again, even though there are still regions of this secret
> memory, thereby bypassing the security check.

IMO, this hibernation check is also buggy, since it looks to be
vulnerable to ToCToU: processes aren't frozen when
hibernation_available() checks secretmem_users(), so a process could add
one and fill it before the process freezer stops it.

And of course, there's still the ptrace hole[1], which is think is quite
serious as it renders the entire defense moot.

-Kees

[1] https://lore.kernel.org/lkml/202105071620.E834B1FA92@keescook/

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] mm/secretmem: use refcount_t instead of atomic_t
  2021-08-20  4:33 [PATCH] mm/secretmem: use refcount_t instead of atomic_t Jordy Zomer
  2021-08-20  5:33 ` Kees Cook
@ 2021-08-20 14:57 ` James Bottomley
  2021-08-20 16:05   ` Kees Cook
  1 sibling, 1 reply; 8+ messages in thread
From: James Bottomley @ 2021-08-20 14:57 UTC (permalink / raw)
  To: Jordy Zomer, linux-kernel
  Cc: Kees Cook, Andrew Morton, linux-mm, Mike Rapoport

On Fri, 2021-08-20 at 06:33 +0200, Jordy Zomer wrote:
> As you can see there's an `atomic_inc` for each `memfd` that is
> opened in the `memfd_secret` syscall. If a local attacker succeeds to
> open 2^32 memfd's, the counter will wrap around to 0. This implies
> that you may hibernate again, even though there are still regions of
> this secret memory, thereby bypassing the security check.

This isn't a possible attack, is it?  secret memory is per process and
each process usually has an open fd limit of 1024.  That's not to say
we shouldn't have overflow protection just in case, but I think today
we don't have a problem.

James



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] mm/secretmem: use refcount_t instead of atomic_t
  2021-08-20 14:57 ` James Bottomley
@ 2021-08-20 16:05   ` Kees Cook
  2021-08-20 16:38     ` Jordy Zomer
  0 siblings, 1 reply; 8+ messages in thread
From: Kees Cook @ 2021-08-20 16:05 UTC (permalink / raw)
  To: James Bottomley
  Cc: Jordy Zomer, linux-kernel, Andrew Morton, linux-mm, Mike Rapoport

On Fri, Aug 20, 2021 at 07:57:25AM -0700, James Bottomley wrote:
> On Fri, 2021-08-20 at 06:33 +0200, Jordy Zomer wrote:
> > As you can see there's an `atomic_inc` for each `memfd` that is
> > opened in the `memfd_secret` syscall. If a local attacker succeeds to
> > open 2^32 memfd's, the counter will wrap around to 0. This implies
> > that you may hibernate again, even though there are still regions of
> > this secret memory, thereby bypassing the security check.
> 
> This isn't a possible attack, is it?  secret memory is per process and
> each process usually has an open fd limit of 1024.  That's not to say
> we shouldn't have overflow protection just in case, but I think today
> we don't have a problem.

But it's a _global_ setting, so it's still possible, though likely
impractical today. But refcount_t mitigates it and is a trivial change.
:)

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] mm/secretmem: use refcount_t instead of atomic_t
  2021-08-20 16:05   ` Kees Cook
@ 2021-08-20 16:38     ` Jordy Zomer
  2021-08-20 19:40       ` James Bottomley
  0 siblings, 1 reply; 8+ messages in thread
From: Jordy Zomer @ 2021-08-20 16:38 UTC (permalink / raw)
  To: Kees Cook, James Bottomley
  Cc: linux-kernel, Andrew Morton, linux-mm, Mike Rapoport

Hi There!

Because this is a global variable, it appears to be exploitable. Either we generate a sufficient number of processes to achieve this counter, or you increase the open file limit with ulimit or sysctl. Unless the kernel has a hard restriction on the number of potential file descriptors that I'm not aware of.

In any case, it's probably a good idea to patch this to make it explicitly secure. If you discover a hard-limit in the kernel for open file descriptors, please let me know. I'm genuinely ​interested :D!

Best Regards,

Jordy

> On 08/20/2021 12:05 PM Kees Cook <keescook@chromium.org> wrote:
> 
>  
> On Fri, Aug 20, 2021 at 07:57:25AM -0700, James Bottomley wrote:
> > On Fri, 2021-08-20 at 06:33 +0200, Jordy Zomer wrote:
> > > As you can see there's an `atomic_inc` for each `memfd` that is
> > > opened in the `memfd_secret` syscall. If a local attacker succeeds to
> > > open 2^32 memfd's, the counter will wrap around to 0. This implies
> > > that you may hibernate again, even though there are still regions of
> > > this secret memory, thereby bypassing the security check.
> > 
> > This isn't a possible attack, is it?  secret memory is per process and
> > each process usually has an open fd limit of 1024.  That's not to say
> > we shouldn't have overflow protection just in case, but I think today
> > we don't have a problem.
> 
> But it's a _global_ setting, so it's still possible, though likely
> impractical today. But refcount_t mitigates it and is a trivial change.
> :)
> 
> -- 
> Kees Cook

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] mm/secretmem: use refcount_t instead of atomic_t
  2021-08-20 16:38     ` Jordy Zomer
@ 2021-08-20 19:40       ` James Bottomley
  0 siblings, 0 replies; 8+ messages in thread
From: James Bottomley @ 2021-08-20 19:40 UTC (permalink / raw)
  To: Jordy Zomer, Kees Cook
  Cc: linux-kernel, Andrew Morton, linux-mm, Mike Rapoport

On Fri, 2021-08-20 at 12:38 -0400, Jordy Zomer wrote:
> Hi There!
> 
> Because this is a global variable, it appears to be exploitable.
> Either we generate a sufficient number of processes to achieve this
> counter, or you increase the open file limit with ulimit or sysctl.
> Unless the kernel has a hard restriction on the number of potential
> file descriptors that I'm not aware of.

There's no direct global counter for file descriptors, no; however,
there is an indirect limit: the number of processes per user which is
now defaulting to around 65535, so even a fork bomb opening the max
number of fds won't get you a wrap.

> In any case, it's probably a good idea to patch this to make it
> explicitly secure. If you discover a hard-limit in the kernel for
> open file descriptors, please let me know. I'm genuinely interested
> :D!

I didn't disagree it might be a useful think to update ... I just
didn't think it was currently exploitable.

James



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] mm/secretmem: use refcount_t instead of atomic_t
  2021-08-20  5:33 ` Kees Cook
@ 2021-08-24 14:05   ` Mike Rapoport
  2021-10-21  9:00     ` Dmitry Vyukov
  0 siblings, 1 reply; 8+ messages in thread
From: Mike Rapoport @ 2021-08-24 14:05 UTC (permalink / raw)
  To: Kees Cook
  Cc: Jordy Zomer, linux-kernel, Andrew Morton, linux-mm, James Bottomley

On Thu, Aug 19, 2021 at 10:33:49PM -0700, Kees Cook wrote:
> On Fri, Aug 20, 2021 at 06:33:38AM +0200, Jordy Zomer wrote:
> > When a secret memory region is active, memfd_secret disables
> > hibernation. One of the goals is to keep the secret data from being
> > written to persistent-storage.
> > 
> > It accomplishes this by maintaining a reference count to
> > `secretmem_users`. Once this reference is held your system can not be
> > hibernated due to the check in `hibernation_available()`. However,
> > because `secretmem_users` is of type `atomic_t`, reference counter
> > overflows are possible.
> 
> It's an unlikely condition to hit given max-open-fds, etc, but there's
> no reason to leave this weakness. Changing this to refcount_t is easy
> and better than using atomic_t.
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> > As you can see there's an `atomic_inc` for each `memfd` that is opened
> > in the `memfd_secret` syscall. If a local attacker succeeds to open 2^32
> > memfd's, the counter will wrap around to 0. This implies that you may
> > hibernate again, even though there are still regions of this secret
> > memory, thereby bypassing the security check.
> 
> IMO, this hibernation check is also buggy, since it looks to be
> vulnerable to ToCToU: processes aren't frozen when
> hibernation_available() checks secretmem_users(), so a process could add
> one and fill it before the process freezer stops it.
> 
> And of course, there's still the ptrace hole[1], which is think is quite
> serious as it renders the entire defense moot.

I thought about what can be done here and could not come up with anything
better that prevent PTRACE on a process with secretmem, but this seems to
me too much from usability vs security POV.

Protecting against root is always hard and secretmem anyway does not
provide 100% guarantee by itself but rather makes an accidental data leak
or non-target attack much harder.

To be effective it also presumes that other hardening features are turned
on by the system administrator on production systems, so it's not
unrealistic to rely on ptrace being disabled.

-- 
Sincerely yours,
Mike.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] mm/secretmem: use refcount_t instead of atomic_t
  2021-08-24 14:05   ` Mike Rapoport
@ 2021-10-21  9:00     ` Dmitry Vyukov
  0 siblings, 0 replies; 8+ messages in thread
From: Dmitry Vyukov @ 2021-10-21  9:00 UTC (permalink / raw)
  To: Mike Rapoport
  Cc: Kees Cook, Jordy Zomer, linux-kernel, Andrew Morton, linux-mm,
	James Bottomley

On Tue, 24 Aug 2021 at 16:06, Mike Rapoport <rppt@kernel.org> wrote:
>
> On Thu, Aug 19, 2021 at 10:33:49PM -0700, Kees Cook wrote:
> > On Fri, Aug 20, 2021 at 06:33:38AM +0200, Jordy Zomer wrote:
> > > When a secret memory region is active, memfd_secret disables
> > > hibernation. One of the goals is to keep the secret data from being
> > > written to persistent-storage.
> > >
> > > It accomplishes this by maintaining a reference count to
> > > `secretmem_users`. Once this reference is held your system can not be
> > > hibernated due to the check in `hibernation_available()`. However,
> > > because `secretmem_users` is of type `atomic_t`, reference counter
> > > overflows are possible.
> >
> > It's an unlikely condition to hit given max-open-fds, etc, but there's
> > no reason to leave this weakness. Changing this to refcount_t is easy
> > and better than using atomic_t.
> >
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> >
> > > As you can see there's an `atomic_inc` for each `memfd` that is opened
> > > in the `memfd_secret` syscall. If a local attacker succeeds to open 2^32
> > > memfd's, the counter will wrap around to 0. This implies that you may
> > > hibernate again, even though there are still regions of this secret
> > > memory, thereby bypassing the security check.
> >
> > IMO, this hibernation check is also buggy, since it looks to be
> > vulnerable to ToCToU: processes aren't frozen when
> > hibernation_available() checks secretmem_users(), so a process could add
> > one and fill it before the process freezer stops it.
> >
> > And of course, there's still the ptrace hole[1], which is think is quite
> > serious as it renders the entire defense moot.
>
> I thought about what can be done here and could not come up with anything
> better that prevent PTRACE on a process with secretmem, but this seems to
> me too much from usability vs security POV.
>
> Protecting against root is always hard and secretmem anyway does not
> provide 100% guarantee by itself but rather makes an accidental data leak
> or non-target attack much harder.
>
> To be effective it also presumes that other hardening features are turned
> on by the system administrator on production systems, so it's not
> unrealistic to rely on ptrace being disabled.

Hi,

The issue existed before this change, but I think refcount_inc needs
to be done before fd_install. After fd_install finishes, the fd can be
used by userspace and we can have secret data in memory before the
refcount_inc.

A straightforward mis-use where a user will predict the returned fd in
another thread before the syscall returns and will use it to store
secret data is somewhat dubious because such a user just shoots
themself in the foot.

But a more interesting mis-used would be to close the predicted fd and
decrement the refcount before the corresponding refcount_inc, this way
one can briefly drop the refcount to zero while there are other users
of secretmem.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-10-21  9:00 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-20  4:33 [PATCH] mm/secretmem: use refcount_t instead of atomic_t Jordy Zomer
2021-08-20  5:33 ` Kees Cook
2021-08-24 14:05   ` Mike Rapoport
2021-10-21  9:00     ` Dmitry Vyukov
2021-08-20 14:57 ` James Bottomley
2021-08-20 16:05   ` Kees Cook
2021-08-20 16:38     ` Jordy Zomer
2021-08-20 19:40       ` James Bottomley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).