* Re: [PATCH] mm/secretmem: Avoid letting secretmem_users drop to zero
2021-10-21 15:40 [PATCH] mm/secretmem: Avoid letting secretmem_users drop to zero Kees Cook
@ 2021-10-21 15:48 ` Dmitry Vyukov
2021-10-21 15:50 ` David Hildenbrand
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Dmitry Vyukov @ 2021-10-21 15:48 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Mike Rapoport, Jordy Zomer, linux-mm,
James Bottomley, David Hildenbrand, linux-kernel,
linux-hardening
On Thu, 21 Oct 2021 at 17:40, Kees Cook <keescook@chromium.org> wrote:
>
> Quoting Dmitry: "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-use 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."
>
> Move fd_install() after refcount_inc().
>
> Cc: Mike Rapoport <rppt@kernel.org>
> Cc: Jordy Zomer <jordy@pwning.systems>
> Cc: linux-mm@kvack.org
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Link: https://lore.kernel.org/lkml/CACT4Y+b1sW6-Hkn8HQYw_SsT7X3tp-CJNh2ci0wG3ZnQz9jjig@mail.gmail.com
> Fixes: 9a436f8ff631 ("PM: hibernate: disable when there are active secretmem users")
> Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> ---
> mm/secretmem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/secretmem.c b/mm/secretmem.c
> index 1fea68b8d5a6..924d84ba481f 100644
> --- a/mm/secretmem.c
> +++ b/mm/secretmem.c
> @@ -217,8 +217,8 @@ SYSCALL_DEFINE1(memfd_secret, unsigned int, flags)
>
> file->f_flags |= O_LARGEFILE;
>
> - fd_install(fd, file);
> refcount_inc(&secretmem_users);
> + fd_install(fd, file);
> return fd;
>
> err_put_fd:
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] mm/secretmem: Avoid letting secretmem_users drop to zero
2021-10-21 15:40 [PATCH] mm/secretmem: Avoid letting secretmem_users drop to zero Kees Cook
2021-10-21 15:48 ` Dmitry Vyukov
@ 2021-10-21 15:50 ` David Hildenbrand
2021-10-21 16:47 ` Jordy Zomer
2021-10-22 2:53 ` Andrew Morton
3 siblings, 0 replies; 7+ messages in thread
From: David Hildenbrand @ 2021-10-21 15:50 UTC (permalink / raw)
To: Kees Cook, Andrew Morton
Cc: Mike Rapoport, Jordy Zomer, linux-mm, Dmitry Vyukov,
James Bottomley, linux-kernel, linux-hardening
On 21.10.21 17:40, Kees Cook wrote:
> Quoting Dmitry: "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-use 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."
>
> Move fd_install() after refcount_inc().
>
> Cc: Mike Rapoport <rppt@kernel.org>
> Cc: Jordy Zomer <jordy@pwning.systems>
> Cc: linux-mm@kvack.org
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Link: https://lore.kernel.org/lkml/CACT4Y+b1sW6-Hkn8HQYw_SsT7X3tp-CJNh2ci0wG3ZnQz9jjig@mail.gmail.com
> Fixes: 9a436f8ff631 ("PM: hibernate: disable when there are active secretmem users")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> mm/secretmem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/secretmem.c b/mm/secretmem.c
> index 1fea68b8d5a6..924d84ba481f 100644
> --- a/mm/secretmem.c
> +++ b/mm/secretmem.c
> @@ -217,8 +217,8 @@ SYSCALL_DEFINE1(memfd_secret, unsigned int, flags)
>
> file->f_flags |= O_LARGEFILE;
>
> - fd_install(fd, file);
> refcount_inc(&secretmem_users);
> + fd_install(fd, file);
> return fd;
>
> err_put_fd:
>
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] mm/secretmem: Avoid letting secretmem_users drop to zero
2021-10-21 15:40 [PATCH] mm/secretmem: Avoid letting secretmem_users drop to zero Kees Cook
2021-10-21 15:48 ` Dmitry Vyukov
2021-10-21 15:50 ` David Hildenbrand
@ 2021-10-21 16:47 ` Jordy Zomer
2021-10-22 2:53 ` Andrew Morton
3 siblings, 0 replies; 7+ messages in thread
From: Jordy Zomer @ 2021-10-21 16:47 UTC (permalink / raw)
To: Kees Cook, Andrew Morton
Cc: Mike Rapoport, linux-mm, Dmitry Vyukov, James Bottomley,
David Hildenbrand, linux-kernel, linux-hardening
Reviewed-by: Jordy Zomer <jordy@pwning.systems>
That's a nice find, Dmitry! Thanks. Also, thank you for writing a patch in such a short period of time, Kees:)
Looks good to me!
> On 10/21/2021 11:40 AM Kees Cook <keescook@chromium.org> wrote:
>
>
> Quoting Dmitry: "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-use 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."
>
> Move fd_install() after refcount_inc().
>
> Cc: Mike Rapoport <rppt@kernel.org>
> Cc: Jordy Zomer <jordy@pwning.systems>
> Cc: linux-mm@kvack.org
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Link: https://lore.kernel.org/lkml/CACT4Y+b1sW6-Hkn8HQYw_SsT7X3tp-CJNh2ci0wG3ZnQz9jjig@mail.gmail.com
> Fixes: 9a436f8ff631 ("PM: hibernate: disable when there are active secretmem users")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> mm/secretmem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/secretmem.c b/mm/secretmem.c
> index 1fea68b8d5a6..924d84ba481f 100644
> --- a/mm/secretmem.c
> +++ b/mm/secretmem.c
> @@ -217,8 +217,8 @@ SYSCALL_DEFINE1(memfd_secret, unsigned int, flags)
>
> file->f_flags |= O_LARGEFILE;
>
> - fd_install(fd, file);
> refcount_inc(&secretmem_users);
> + fd_install(fd, file);
> return fd;
>
> err_put_fd:
> --
> 2.30.2
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] mm/secretmem: Avoid letting secretmem_users drop to zero
2021-10-21 15:40 [PATCH] mm/secretmem: Avoid letting secretmem_users drop to zero Kees Cook
` (2 preceding siblings ...)
2021-10-21 16:47 ` Jordy Zomer
@ 2021-10-22 2:53 ` Andrew Morton
2021-10-22 3:39 ` Kees Cook
3 siblings, 1 reply; 7+ messages in thread
From: Andrew Morton @ 2021-10-22 2:53 UTC (permalink / raw)
To: Kees Cook
Cc: Mike Rapoport, Jordy Zomer, linux-mm, Dmitry Vyukov,
James Bottomley, David Hildenbrand, linux-kernel,
linux-hardening
On Thu, 21 Oct 2021 08:40:46 -0700 Kees Cook <keescook@chromium.org> wrote:
> Quoting Dmitry: "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-use 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."
>
> Move fd_install() after refcount_inc().
I added cc:stable. Or doesn't the benefit/risk ratio justify that?
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] mm/secretmem: Avoid letting secretmem_users drop to zero
2021-10-22 2:53 ` Andrew Morton
@ 2021-10-22 3:39 ` Kees Cook
2021-10-22 7:09 ` Mike Rapoport
0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2021-10-22 3:39 UTC (permalink / raw)
To: Andrew Morton
Cc: Mike Rapoport, Jordy Zomer, linux-mm, Dmitry Vyukov,
James Bottomley, David Hildenbrand, linux-kernel,
linux-hardening
On Thu, Oct 21, 2021 at 07:53:11PM -0700, Andrew Morton wrote:
> On Thu, 21 Oct 2021 08:40:46 -0700 Kees Cook <keescook@chromium.org> wrote:
>
> > Quoting Dmitry: "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-use 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."
> >
> > Move fd_install() after refcount_inc().
>
> I added cc:stable. Or doesn't the benefit/risk ratio justify that?
I hadn't because commit 110860541f44 ("mm/secretmem: use refcount_t
instead of atomic_t") wasn't, and this would build on top of it.
I think the exposure is very small in both places, so probably best to
avoid the churn, but I'm not _opposed_ to it.
--
Kees Cook
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] mm/secretmem: Avoid letting secretmem_users drop to zero
2021-10-22 3:39 ` Kees Cook
@ 2021-10-22 7:09 ` Mike Rapoport
0 siblings, 0 replies; 7+ messages in thread
From: Mike Rapoport @ 2021-10-22 7:09 UTC (permalink / raw)
To: Kees Cook, Jordy Zomer
Cc: Andrew Morton, linux-mm, Dmitry Vyukov, James Bottomley,
David Hildenbrand, linux-kernel, linux-hardening
On Thu, Oct 21, 2021 at 08:39:03PM -0700, Kees Cook wrote:
> On Thu, Oct 21, 2021 at 07:53:11PM -0700, Andrew Morton wrote:
> > On Thu, 21 Oct 2021 08:40:46 -0700 Kees Cook <keescook@chromium.org> wrote:
> >
> > > Quoting Dmitry: "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-use 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."
> > >
> > > Move fd_install() after refcount_inc().
> >
> > I added cc:stable. Or doesn't the benefit/risk ratio justify that?
>
> I hadn't because commit 110860541f44 ("mm/secretmem: use refcount_t
> instead of atomic_t") wasn't, and this would build on top of it.
Hmm, the commit 110860541f44 ("mm/secretmem: use refcount_t instead of
atomic_t") causes the splats below. I wonder if it was tested at all :(
[ 20.957833] ------------[ cut here ]------------
[ 20.957844] refcount_t: addition on 0; use-after-free.
[ 20.957897] WARNING: CPU: 3 PID: 598 at /home/rppt/git/linux/lib/refcount.c:25 refcount_warn_saturate+0xcf/0xf0
[ 20.957919] Modules linked in:
[ 20.957930] CPU: 3 PID: 598 Comm: secretmemfd Not tainted 5.15.0-rc6+ #432
[ 20.957944] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
[ 20.957948] RIP: 0010:refcount_warn_saturate+0xcf/0xf0
[ 20.957957] Code: 01 01 e8 d4 db c3 ff 0f 0b c3 80 3d 39 32 43 01 00 0f 85 6b ff ff ff 48 c7 c7 00 bc c5 af c6 05 25 32 43 01 01 e8 b1 db c3 ff <0f> 0b c3 48 c7 c7 b0 bb c5 af c6 05 10 32 43 01 01 e8 9b db c3 ff
[ 20.957962] RSP: 0018:ffffb188c0583f20 EFLAGS: 00010282
[ 20.957967] RAX: 0000000000000000 RBX: 0000000000000003 RCX: 0000000000000027
[ 20.957971] RDX: 0000000000000000 RSI: ffff8bfefbb975b0 RDI: ffff8bfefbb975b8
[ 20.957974] RBP: ffffb188c0583f48 R08: 0000000000000000 R09: 0000000000000001
[ 20.957977] R10: 0000000000000003 R11: ffffb188c0583d38 R12: 0000000000000000
[ 20.957980] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[ 20.957983] FS: 00007f9467b9c740(0000) GS:ffff8bfefbb80000(0000) knlGS:0000000000000000
[ 20.957993] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 20.957997] CR2: 00007ffe83be8084 CR3: 00000001100cc003 CR4: 0000000000060ee0
[ 20.958001] Call Trace:
[ 20.959285] __x64_sys_memfd_secret+0xa9/0xc0
[ 20.959308] do_syscall_64+0x3a/0x80
[ 20.959331] entry_SYSCALL_64_after_hwframe+0x44/0xae
[ 20.959352] RIP: 0033:0x7f9467cba89d
[ 20.959358] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c3 f5 0c 00 f7 d8 64 89 01 48
[ 20.959362] RSP: 002b:00007ffe83bb8148 EFLAGS: 00000206 ORIG_RAX: 00000000000001bf
[ 20.959368] RAX: ffffffffffffffda RBX: 0000561f62400d50 RCX: 00007f9467cba89d
[ 20.959372] RDX: 0000000000000e11 RSI: 0000000000008000 RDI: 0000000000000000
[ 20.959375] RBP: 00007ffe83bb8160 R08: 000000002c06910a R09: 0000000000000000
[ 20.959378] R10: 00007f9467d8a1c4 R11: 0000000000000206 R12: 0000561f624008d0
[ 20.959381] R13: 00007ffe83bb82b0 R14: 0000000000000000 R15: 0000000000000000
[ 20.959386] ---[ end trace 9368244c7159e4de ]---
[ 20.960666] ------------[ cut here ]------------
[ 20.960675] refcount_t: decrement hit 0; leaking memory.
[ 20.960717] WARNING: CPU: 1 PID: 598 at /home/rppt/git/linux/lib/refcount.c:31 refcount_warn_saturate+0x4f/0xf0
[ 20.960737] Modules linked in:
[ 20.960742] CPU: 1 PID: 598 Comm: secretmemfd Tainted: G W 5.15.0-rc6+ #432
[ 20.960748] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
[ 20.960751] RIP: 0010:refcount_warn_saturate+0x4f/0xf0
[ 20.960759] Code: 00 00 f3 c3 83 fe 03 74 43 83 fe 04 75 1f 80 3d b3 32 43 01 00 75 eb 48 c7 c7 58 bc c5 af c6 05 a3 32 43 01 01 e8 31 dc c3 ff <0f> 0b c3 80 3d 93 32 43 01 00 75 cc 48 c7 c7 88 bc c5 af c6 05 83
[ 20.960764] RSP: 0018:ffffb188c0583e40 EFLAGS: 00010286
[ 20.960769] RAX: 0000000000000000 RBX: ffff8bfec1f51900 RCX: 0000000000000027
[ 20.960772] RDX: 0000000000000000 RSI: ffff8bfefba975b0 RDI: ffff8bfefba975b8
[ 20.960775] RBP: 0000000000080003 R08: 0000000000000000 R09: 0000000000000001
[ 20.960778] R10: ffff8bfec439da80 R11: ffffb188c0583c58 R12: ffff8bfec4e576a0
[ 20.960781] R13: ffff8bfec01a8ca0 R14: ffff8bfecd314300 R15: 0000000000000000
[ 20.960784] FS: 0000000000000000(0000) GS:ffff8bfefba80000(0000) knlGS:0000000000000000
[ 20.960835] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 20.960840] CR2: 00007f9467c85290 CR3: 0000000080a0c004 CR4: 0000000000060ee0
[ 20.960843] Call Trace:
[ 20.960849] secretmem_release+0x26/0x30
[ 20.960862] __fput+0x85/0x240
[ 20.960868] task_work_run+0x67/0xa0
[ 20.960890] do_exit+0x363/0xbb0
[ 20.960902] do_group_exit+0x35/0x90
[ 20.960908] __x64_sys_exit_group+0xf/0x10
[ 20.960913] do_syscall_64+0x3a/0x80
[ 20.960922] entry_SYSCALL_64_after_hwframe+0x44/0xae
[ 20.960928] RIP: 0033:0x7f9467c852c6
[ 20.960933] Code: Unable to access opcode bytes at RIP 0x7f9467c8529c.
[ 20.960936] RSP: 002b:00007ffe83bb8168 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
[ 20.960941] RAX: ffffffffffffffda RBX: 00007f9467d8c610 RCX: 00007f9467c852c6
[ 20.960944] RDX: 0000000000000000 RSI: 000000000000003c RDI: 0000000000000000
[ 20.960947] RBP: 0000000000000000 R08: 00000000000000e7 R09: ffffffffffffff80
[ 20.960950] R10: 0000000000000003 R11: 0000000000000246 R12: 00007f9467d8c610
[ 20.960953] R13: 0000000000000001 R14: 00007f9467d8ffc8 R15: 0000000000000000
[ 20.960957] ---[ end trace 9368244c7159e4df ]---
> I think the exposure is very small in both places, so probably best to
> avoid the churn, but I'm not _opposed_ to it.
>
> --
> Kees Cook
--
Sincerely yours,
Mike.
^ permalink raw reply [flat|nested] 7+ messages in thread