* Re: mount.nfs: Protocol error after upgrade to linux/master
From: Tetsuo Handa @ 2019-03-21 21:10 UTC (permalink / raw)
To: Kees Cook
Cc: Casey Schaufler, Jakub Kicinski, linux-security-module,
Trond Myklebust, open list:NFS, SUNRPC, AND..., Anna Schumaker,
LKML
In-Reply-To: <CAGXu5jKj-Pq4krHmKd__4MoXBe1FK399cDZnSxdPigZo78tPfA@mail.gmail.com>
On 2019/03/22 1:38, Kees Cook wrote:
> This is mostly good. I'd like to keep the other LSMs listed though
> (similar to what I had originally) so that if a legacy-major doesn't
> initialize, later ones will be. I want to remove the concept of
> "major" LSMs. The only thing that should matter is init order...
Excuse me? Are you saying that
if a legacy-major (which is defined as the "Default security module")
doesn't initialize, later ones (any of selinux,smack,tomoyo,apparmor
except the one which is defined as "Default security module") will be
initialized
? That sounds strange to me. Any of selinux,smack,tomoyo,apparmor can be
initialized when specified by lsm= kernel command line option (or security=
kernel command line option if lsm= kernel command line option is not
specified), won't it?
^ permalink raw reply
* Re: [GIT PULL] SELinux fixes for v5.1 (#2)
From: pr-tracker-bot @ 2019-03-21 19:05 UTC (permalink / raw)
To: Paul Moore; +Cc: Linus Torvalds, selinux, linux-security-module, linux-kernel
In-Reply-To: <CAHC9VhRoo4_A0fg+Gp-Qsn_XVkTOQhe1pUShGig-k3ELKeW7XQ@mail.gmail.com>
The pull request you sent on Thu, 21 Mar 2019 10:09:24 -0400:
> git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20190321
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/fb549c55475bbd6e34325005681d7801d9d6f6bd
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages
From: Eric Biggers @ 2019-03-21 17:51 UTC (permalink / raw)
To: Kees Cook
Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM,
Linux Crypto Mailing List, Linux Kernel Mailing List
In-Reply-To: <CAGXu5jL-5C1ZGZ1JgaNFqK-z1Pp1N1X8gD3_ysKTCa=omD+_VQ@mail.gmail.com>
On Thu, Mar 21, 2019 at 10:45:31AM -0700, Kees Cook wrote:
> On Wed, Mar 20, 2019 at 11:57 AM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Tue, Mar 19, 2019 at 10:09:13AM -0700, Eric Biggers wrote:
> > > On Tue, Mar 19, 2019 at 12:54:23PM +0100, Geert Uytterhoeven wrote:
> > > > When running the sha1-asm crypto selftest on arm with
> > > > CONFIG_HARDENED_USERCOPY_PAGESPAN=y:
> > > >
> > > > usercopy: Kernel memory overwrite attempt detected to spans
> > > > multiple pages (offset 0, size 42)!
> > > > ------------[ cut here ]------------
> > > > kernel BUG at mm/usercopy.c:102!
> > > > Internal error: Oops - BUG: 0 [#1] SMP ARM
> > > > Modules linked in:
> > > > CPU: 0 PID: 35 Comm: cryptomgr_test Not tainted
> > > > 5.1.0-rc1-koelsch-01109-gbeb7d6376ecfbf07-dirty #397
> > > > Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> > > > PC is at usercopy_abort+0x68/0x90
> > > > LR is at usercopy_abort+0x68/0x90
> > > > pc : [<c030fd60>] lr : [<c030fd60>] psr: 60000013
> > > > sp : ea54bc60 ip : 00000010 fp : cccccccd
> > > > r10: 00000000 r9 : c0e0ce04 r8 : ea54d009
> > > > r7 : ea54d00a r6 : 00000000 r5 : 0000002a r4 : c09d1120
> > > > r3 : dd6cd422 r2 : dd6cd422 r1 : 2abb4000 r0 : 0000005f
> > > > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
> > > > Control: 30c5387d Table: 40003000 DAC: fffffffd
> > > > Process cryptomgr_test (pid: 35, stack limit = 0x(ptrval))
> > > > Stack: (0xea54bc60 to 0xea54c000)
> > > > bc60: c09d1120 c09d1120 c09d1120 00000000 0000002a 0000002a
> > > > 00000000 c0310060
> > > > bc80: 0000002a 00000000 000001c0 00000000 00000000 c0eb11e8
> > > > ea54cfe0 ea538c00
> > > > bca0: 00000000 ea54cfe0 ebef73e0 0000002a ea538c20 ea54bd84
> > > > 0000003a c0427a30
> > > > bcc0: ea54bdbc 00000000 00000000 c081cf70 eb074280 c081cf70
> > > > 0000002a c081cf80
> > > > bce0: 0000000e c07da138 ea54bd0c 00000000 c084061c c04248e8
> > > > c0e0a408 eb074240
> > > > bd00: eb074200 c04253c8 eb074280 ea550000 00000012 dd6cd422
> > > > ebef7480 eb074200
> > > > bd20: ea54bd84 c081cf64 ea537200 00000002 00000000 00000014
> > > > c084061c c0428c38
> > > > bd40: ea54bd84 ea54bdbc c081cd34 00000000 c0e4e4b4 ea538c40
> > > > 00000002 eabe4e80
> > > > bd60: ea538c00 00000400 ea4f7a00 ea4f7a60 eb074240 00000060
> > > > 00000006 c09d544c
> > > > bd80: 00000038 00000003 00000000 00000038 ea54bd7c 00000001
> > > > eb074200 00000000
> > > > bda0: 00000000 dead4ead ffffffff ffffffff ea54bdb0 ea54bdb0
> > > > 00000000 c081cf70
> > > > bdc0: c081ce68 c081ce78 ea4f7480 eb000780 00000dc0 eb000780
> > > > c0e4ee80 443e9884
> > > > bde0: 6ed23b1c a14aaeba e52951f9 f17046e5 fefefefe fefefefe
> > > > fefefefe fefefefe
> > > > be00: eb000780 c04292c4 c0e0a638 60000013 60000013 c0305298
> > > > ea4f7a00 c03062bc
> > > > be20: eb000780 00000cc0 ea4f7a00 dd6cd422 00000cc0 ea538c00
> > > > 00000002 eabe4e40
> > > > be40: ea537200 00000007 00000000 ea4f7a00 eb074200 c0429314
> > > > eb074200 ea538c00
> > > > be60: ea4f7a00 0000000a eabe4e80 c084061c c08405fc 00000006
> > > > c04dace8 00000006
> > > > be80: 00000000 c084065c ea537200 0000000e 00000400 eb04de08
> > > > ea4f71a8 c0429420
> > > > bea0: 00000400 ea537200 0000000e ea537200 0000000e c0429374
> > > > 00000400 ffffffff
> > > > bec0: 000000a2 c042a414 00000103 c0e0a408 00000000 c0e0a438
> > > > c0e5a2a0 c0e5a2a0
> > > > bee0: 00000001 00000001 00000017 ffffe000 00000000 60000013
> > > > c0e5a2a0 c0269470
> > > > bf00: c09c9ed0 ea54bf5c 00000103 00000000 00000000 c0e0a408
> > > > ea537280 0000000e
> > > > bf20: 00000400 c0426500 00000000 eb04de08 ea4f71a8 c02694f4
> > > > c09c9ed0 ea54bf5c
> > > > bf40: ea54bf28 c02699d0 ea54bf5c dd6cd422 ea537200 dd6cd422
> > > > c09c9ed0 ea537200
> > > > bf60: ea4af1c0 ea54a000 ea537200 c0426500 00000000 eb04de08
> > > > ea4f71a8 c0426524
> > > > bf80: ea4f7180 c023dcec ea54a000 ea4af1c0 c023dbb4 00000000
> > > > 00000000 00000000
> > > > bfa0: 00000000 00000000 00000000 c02010d8 00000000 00000000
> > > > 00000000 00000000
> > > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000
> > > > 00000000 00000000
> > > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> > > > 00000000 00000000
> > > > [<c030fd60>] (usercopy_abort) from [<c0310060>]
> > > > (__check_object_size+0x2d8/0x448)
> > > > [<c0310060>] (__check_object_size) from [<c0427a30>]
> > > > (build_test_sglist+0x268/0x2d8)
> > > > [<c0427a30>] (build_test_sglist) from [<c0428c38>]
> > > > (test_hash_vec_cfg+0x110/0x694)
> > > > [<c0428c38>] (test_hash_vec_cfg) from [<c0429314>]
> > > > (__alg_test_hash+0x158/0x1b8)
> > > > [<c0429314>] (__alg_test_hash) from [<c0429420>] (alg_test_hash+0xac/0xf4)
> > > > [<c0429420>] (alg_test_hash) from [<c042a414>] (alg_test.part.4+0x264/0x2f8)
> > > > [<c042a414>] (alg_test.part.4) from [<c0426524>] (cryptomgr_test+0x24/0x44)
> > > > [<c0426524>] (cryptomgr_test) from [<c023dcec>] (kthread+0x138/0x150)
> > > > [<c023dcec>] (kthread) from [<c02010d8>] (ret_from_fork+0x14/0x3c)
> > > > Exception stack(0xea54bfb0 to 0xea54bff8)
> > > > bfa0: 00000000 00000000
> > > > 00000000 00000000
> > > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000
> > > > 00000000 00000000
> > > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> > > > Code: e58de000 e98d0012 e1a0100c ebfd6712 (e7f001f2)
> > > > ---[ end trace 190b3cf48e720f78 ]---
> > > > BUG: sleeping function called from invalid context at
> > > > include/linux/percpu-rwsem.h:34
> > > > in_atomic(): 0, irqs_disabled(): 128, pid: 35, name: cryptomgr_test
> > > > CPU: 0 PID: 35 Comm: cryptomgr_test Tainted: G D
> > > > 5.1.0-rc1-koelsch-01109-gbeb7d6376ecfbf07-dirty #397
> > > > Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> > > > [<c020ec74>] (unwind_backtrace) from [<c020ae58>] (show_stack+0x10/0x14)
> > > > [<c020ae58>] (show_stack) from [<c07c3624>] (dump_stack+0x7c/0x9c)
> > > > [<c07c3624>] (dump_stack) from [<c0242e14>] (___might_sleep+0xf4/0x158)
> > > > [<c0242e14>] (___might_sleep) from [<c0230210>] (exit_signals+0x2c/0x258)
> > > > [<c0230210>] (exit_signals) from [<c0223d6c>] (do_exit+0x114/0xa20)
> > > > [<c0223d6c>] (do_exit) from [<c020b160>] (die+0x304/0x344)
> > > > [<c020b160>] (die) from [<c020b388>] (do_undefinstr+0x80/0x190)
> > > > [<c020b388>] (do_undefinstr) from [<c0201b24>] (__und_svc_finish+0x0/0x3c)
> > > > Exception stack(0xea54bc10 to 0xea54bc58)
> > > > bc00: 0000005f 2abb4000
> > > > dd6cd422 dd6cd422
> > > > bc20: c09d1120 0000002a 00000000 ea54d00a ea54d009 c0e0ce04
> > > > 00000000 cccccccd
> > > > bc40: 00000010 ea54bc60 c030fd60 c030fd60 60000013 ffffffff
> > > > [<c0201b24>] (__und_svc_finish) from [<c030fd60>] (usercopy_abort+0x68/0x90)
> > > > [<c030fd60>] (usercopy_abort) from [<c0310060>]
> > > > (__check_object_size+0x2d8/0x448)
> > > > [<c0310060>] (__check_object_size) from [<c0427a30>]
> > > > (build_test_sglist+0x268/0x2d8)
> > > > [<c0427a30>] (build_test_sglist) from [<c0428c38>]
> > > > (test_hash_vec_cfg+0x110/0x694)
> > > > [<c0428c38>] (test_hash_vec_cfg) from [<c0429314>]
> > > > (__alg_test_hash+0x158/0x1b8)
> > > > [<c0429314>] (__alg_test_hash) from [<c0429420>] (alg_test_hash+0xac/0xf4)
> > > > [<c0429420>] (alg_test_hash) from [<c042a414>] (alg_test.part.4+0x264/0x2f8)
> > > > [<c042a414>] (alg_test.part.4) from [<c0426524>] (cryptomgr_test+0x24/0x44)
> > > > [<c0426524>] (cryptomgr_test) from [<c023dcec>] (kthread+0x138/0x150)
> > > > [<c023dcec>] (kthread) from [<c02010d8>] (ret_from_fork+0x14/0x3c)
> > > > Exception stack(0xea54bfb0 to 0xea54bff8)
> > > > bfa0: 00000000 00000000
> > > > 00000000 00000000
> > > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000
> > > > 00000000 00000000
> > > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> > > >
> > >
> > > Well, this must happen with the new (in 5.1) crypto self-tests implementation
> > > for any crypto algorithm when CONFIG_HARDENED_USERCOPY_PAGESPAN=y. I don't
> > > understand why hardened usercopy considers it a bug though, as there's no buffer
> > > overflow. The crypto tests use copy_from_iter() to copy data into a 2-page
> > > buffer that was allocated with __get_free_pages():
> > >
> > > __get_free_pages(GFP_KERNEL, 1)
> > >
> > > ... where 1 means an order-1 allocation.
> > >
> > > If it copies to offset=4064 len=42, for example, then hardened usercopy
> > > considers it a bug even though the buffer is 8192 bytes long. Why?
> > >
> > > It isn't actually copying anything to/from userspace, BTW; it's using iov_iter
> > > with ITER_KVEC.
> > >
> > > - Eric
> >
> > Kees, any thoughts on why hardened usercopy rejects copies spanning a page
> > boundary when they seem to be fine?
>
> This is due to missing the compound page marking, if I remember
> correctly. However, I tend to leave the pagespan test disabled: it
> really isn't ready for production use -- there are a lot of missing
> annotations still.
>
So do I need to add __GFP_COMP? Is there any actual reason to do so?
Why does hardened usercopy check for it?
- Eric
^ permalink raw reply
* Re: [PATCH v2 1/2] initmem: introduce CONFIG_INIT_ALL_MEMORY and CONFIG_INIT_ALL_STACK
From: Kees Cook @ 2019-03-21 17:47 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Masahiro Yamada, James Morris, Serge E. Hallyn,
linux-security-module, Linux Kbuild mailing list,
Nick Desaulniers, Kostya Serebryany, Dmitriy Vyukov,
Sandeep Patil, Kernel Hardening
In-Reply-To: <CAG_fn=U6Rvg9S0AxLun0oHVx9i=JFc0Lzb+rvkiQn1sTHd-caA@mail.gmail.com>
On Wed, Mar 20, 2019 at 7:44 AM Alexander Potapenko <glider@google.com> wrote:
> A friendly ping.
> Please let me know if I should wait for any upcoming changes to
> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL.
Hi! Sorry for the delay -- between the merge window and travel, I'm
still a couple weeks behind. I should be able to look more closely at
this late next week.
--
Kees Cook
^ permalink raw reply
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages
From: Kees Cook @ 2019-03-21 17:45 UTC (permalink / raw)
To: Eric Biggers
Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM,
Linux Crypto Mailing List, Linux Kernel Mailing List
In-Reply-To: <20190320185719.GB180195@gmail.com>
On Wed, Mar 20, 2019 at 11:57 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Mar 19, 2019 at 10:09:13AM -0700, Eric Biggers wrote:
> > On Tue, Mar 19, 2019 at 12:54:23PM +0100, Geert Uytterhoeven wrote:
> > > When running the sha1-asm crypto selftest on arm with
> > > CONFIG_HARDENED_USERCOPY_PAGESPAN=y:
> > >
> > > usercopy: Kernel memory overwrite attempt detected to spans
> > > multiple pages (offset 0, size 42)!
> > > ------------[ cut here ]------------
> > > kernel BUG at mm/usercopy.c:102!
> > > Internal error: Oops - BUG: 0 [#1] SMP ARM
> > > Modules linked in:
> > > CPU: 0 PID: 35 Comm: cryptomgr_test Not tainted
> > > 5.1.0-rc1-koelsch-01109-gbeb7d6376ecfbf07-dirty #397
> > > Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> > > PC is at usercopy_abort+0x68/0x90
> > > LR is at usercopy_abort+0x68/0x90
> > > pc : [<c030fd60>] lr : [<c030fd60>] psr: 60000013
> > > sp : ea54bc60 ip : 00000010 fp : cccccccd
> > > r10: 00000000 r9 : c0e0ce04 r8 : ea54d009
> > > r7 : ea54d00a r6 : 00000000 r5 : 0000002a r4 : c09d1120
> > > r3 : dd6cd422 r2 : dd6cd422 r1 : 2abb4000 r0 : 0000005f
> > > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
> > > Control: 30c5387d Table: 40003000 DAC: fffffffd
> > > Process cryptomgr_test (pid: 35, stack limit = 0x(ptrval))
> > > Stack: (0xea54bc60 to 0xea54c000)
> > > bc60: c09d1120 c09d1120 c09d1120 00000000 0000002a 0000002a
> > > 00000000 c0310060
> > > bc80: 0000002a 00000000 000001c0 00000000 00000000 c0eb11e8
> > > ea54cfe0 ea538c00
> > > bca0: 00000000 ea54cfe0 ebef73e0 0000002a ea538c20 ea54bd84
> > > 0000003a c0427a30
> > > bcc0: ea54bdbc 00000000 00000000 c081cf70 eb074280 c081cf70
> > > 0000002a c081cf80
> > > bce0: 0000000e c07da138 ea54bd0c 00000000 c084061c c04248e8
> > > c0e0a408 eb074240
> > > bd00: eb074200 c04253c8 eb074280 ea550000 00000012 dd6cd422
> > > ebef7480 eb074200
> > > bd20: ea54bd84 c081cf64 ea537200 00000002 00000000 00000014
> > > c084061c c0428c38
> > > bd40: ea54bd84 ea54bdbc c081cd34 00000000 c0e4e4b4 ea538c40
> > > 00000002 eabe4e80
> > > bd60: ea538c00 00000400 ea4f7a00 ea4f7a60 eb074240 00000060
> > > 00000006 c09d544c
> > > bd80: 00000038 00000003 00000000 00000038 ea54bd7c 00000001
> > > eb074200 00000000
> > > bda0: 00000000 dead4ead ffffffff ffffffff ea54bdb0 ea54bdb0
> > > 00000000 c081cf70
> > > bdc0: c081ce68 c081ce78 ea4f7480 eb000780 00000dc0 eb000780
> > > c0e4ee80 443e9884
> > > bde0: 6ed23b1c a14aaeba e52951f9 f17046e5 fefefefe fefefefe
> > > fefefefe fefefefe
> > > be00: eb000780 c04292c4 c0e0a638 60000013 60000013 c0305298
> > > ea4f7a00 c03062bc
> > > be20: eb000780 00000cc0 ea4f7a00 dd6cd422 00000cc0 ea538c00
> > > 00000002 eabe4e40
> > > be40: ea537200 00000007 00000000 ea4f7a00 eb074200 c0429314
> > > eb074200 ea538c00
> > > be60: ea4f7a00 0000000a eabe4e80 c084061c c08405fc 00000006
> > > c04dace8 00000006
> > > be80: 00000000 c084065c ea537200 0000000e 00000400 eb04de08
> > > ea4f71a8 c0429420
> > > bea0: 00000400 ea537200 0000000e ea537200 0000000e c0429374
> > > 00000400 ffffffff
> > > bec0: 000000a2 c042a414 00000103 c0e0a408 00000000 c0e0a438
> > > c0e5a2a0 c0e5a2a0
> > > bee0: 00000001 00000001 00000017 ffffe000 00000000 60000013
> > > c0e5a2a0 c0269470
> > > bf00: c09c9ed0 ea54bf5c 00000103 00000000 00000000 c0e0a408
> > > ea537280 0000000e
> > > bf20: 00000400 c0426500 00000000 eb04de08 ea4f71a8 c02694f4
> > > c09c9ed0 ea54bf5c
> > > bf40: ea54bf28 c02699d0 ea54bf5c dd6cd422 ea537200 dd6cd422
> > > c09c9ed0 ea537200
> > > bf60: ea4af1c0 ea54a000 ea537200 c0426500 00000000 eb04de08
> > > ea4f71a8 c0426524
> > > bf80: ea4f7180 c023dcec ea54a000 ea4af1c0 c023dbb4 00000000
> > > 00000000 00000000
> > > bfa0: 00000000 00000000 00000000 c02010d8 00000000 00000000
> > > 00000000 00000000
> > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000
> > > 00000000 00000000
> > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> > > 00000000 00000000
> > > [<c030fd60>] (usercopy_abort) from [<c0310060>]
> > > (__check_object_size+0x2d8/0x448)
> > > [<c0310060>] (__check_object_size) from [<c0427a30>]
> > > (build_test_sglist+0x268/0x2d8)
> > > [<c0427a30>] (build_test_sglist) from [<c0428c38>]
> > > (test_hash_vec_cfg+0x110/0x694)
> > > [<c0428c38>] (test_hash_vec_cfg) from [<c0429314>]
> > > (__alg_test_hash+0x158/0x1b8)
> > > [<c0429314>] (__alg_test_hash) from [<c0429420>] (alg_test_hash+0xac/0xf4)
> > > [<c0429420>] (alg_test_hash) from [<c042a414>] (alg_test.part.4+0x264/0x2f8)
> > > [<c042a414>] (alg_test.part.4) from [<c0426524>] (cryptomgr_test+0x24/0x44)
> > > [<c0426524>] (cryptomgr_test) from [<c023dcec>] (kthread+0x138/0x150)
> > > [<c023dcec>] (kthread) from [<c02010d8>] (ret_from_fork+0x14/0x3c)
> > > Exception stack(0xea54bfb0 to 0xea54bff8)
> > > bfa0: 00000000 00000000
> > > 00000000 00000000
> > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000
> > > 00000000 00000000
> > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> > > Code: e58de000 e98d0012 e1a0100c ebfd6712 (e7f001f2)
> > > ---[ end trace 190b3cf48e720f78 ]---
> > > BUG: sleeping function called from invalid context at
> > > include/linux/percpu-rwsem.h:34
> > > in_atomic(): 0, irqs_disabled(): 128, pid: 35, name: cryptomgr_test
> > > CPU: 0 PID: 35 Comm: cryptomgr_test Tainted: G D
> > > 5.1.0-rc1-koelsch-01109-gbeb7d6376ecfbf07-dirty #397
> > > Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> > > [<c020ec74>] (unwind_backtrace) from [<c020ae58>] (show_stack+0x10/0x14)
> > > [<c020ae58>] (show_stack) from [<c07c3624>] (dump_stack+0x7c/0x9c)
> > > [<c07c3624>] (dump_stack) from [<c0242e14>] (___might_sleep+0xf4/0x158)
> > > [<c0242e14>] (___might_sleep) from [<c0230210>] (exit_signals+0x2c/0x258)
> > > [<c0230210>] (exit_signals) from [<c0223d6c>] (do_exit+0x114/0xa20)
> > > [<c0223d6c>] (do_exit) from [<c020b160>] (die+0x304/0x344)
> > > [<c020b160>] (die) from [<c020b388>] (do_undefinstr+0x80/0x190)
> > > [<c020b388>] (do_undefinstr) from [<c0201b24>] (__und_svc_finish+0x0/0x3c)
> > > Exception stack(0xea54bc10 to 0xea54bc58)
> > > bc00: 0000005f 2abb4000
> > > dd6cd422 dd6cd422
> > > bc20: c09d1120 0000002a 00000000 ea54d00a ea54d009 c0e0ce04
> > > 00000000 cccccccd
> > > bc40: 00000010 ea54bc60 c030fd60 c030fd60 60000013 ffffffff
> > > [<c0201b24>] (__und_svc_finish) from [<c030fd60>] (usercopy_abort+0x68/0x90)
> > > [<c030fd60>] (usercopy_abort) from [<c0310060>]
> > > (__check_object_size+0x2d8/0x448)
> > > [<c0310060>] (__check_object_size) from [<c0427a30>]
> > > (build_test_sglist+0x268/0x2d8)
> > > [<c0427a30>] (build_test_sglist) from [<c0428c38>]
> > > (test_hash_vec_cfg+0x110/0x694)
> > > [<c0428c38>] (test_hash_vec_cfg) from [<c0429314>]
> > > (__alg_test_hash+0x158/0x1b8)
> > > [<c0429314>] (__alg_test_hash) from [<c0429420>] (alg_test_hash+0xac/0xf4)
> > > [<c0429420>] (alg_test_hash) from [<c042a414>] (alg_test.part.4+0x264/0x2f8)
> > > [<c042a414>] (alg_test.part.4) from [<c0426524>] (cryptomgr_test+0x24/0x44)
> > > [<c0426524>] (cryptomgr_test) from [<c023dcec>] (kthread+0x138/0x150)
> > > [<c023dcec>] (kthread) from [<c02010d8>] (ret_from_fork+0x14/0x3c)
> > > Exception stack(0xea54bfb0 to 0xea54bff8)
> > > bfa0: 00000000 00000000
> > > 00000000 00000000
> > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000
> > > 00000000 00000000
> > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> > >
> >
> > Well, this must happen with the new (in 5.1) crypto self-tests implementation
> > for any crypto algorithm when CONFIG_HARDENED_USERCOPY_PAGESPAN=y. I don't
> > understand why hardened usercopy considers it a bug though, as there's no buffer
> > overflow. The crypto tests use copy_from_iter() to copy data into a 2-page
> > buffer that was allocated with __get_free_pages():
> >
> > __get_free_pages(GFP_KERNEL, 1)
> >
> > ... where 1 means an order-1 allocation.
> >
> > If it copies to offset=4064 len=42, for example, then hardened usercopy
> > considers it a bug even though the buffer is 8192 bytes long. Why?
> >
> > It isn't actually copying anything to/from userspace, BTW; it's using iov_iter
> > with ITER_KVEC.
> >
> > - Eric
>
> Kees, any thoughts on why hardened usercopy rejects copies spanning a page
> boundary when they seem to be fine?
This is due to missing the compound page marking, if I remember
correctly. However, I tend to leave the pagespan test disabled: it
really isn't ready for production use -- there are a lot of missing
annotations still.
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v19 17/27] x86/sgx: Add provisioning
From: Andy Lutomirski @ 2019-03-21 16:50 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: X86 ML, linux-sgx, Andrew Morton, Dave Hansen,
Christopherson, Sean J, nhorman, npmccallum, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Thomas Gleixner,
Svahn, Kai, Borislav Petkov, Josh Triplett, Andrew Lutomirski,
Huang, Kai, David Rientjes, James Morris, Serge E . Hallyn,
LSM List
In-Reply-To: <20190317211456.13927-18-jarkko.sakkinen@linux.intel.com>
On Sun, Mar 17, 2019 at 2:18 PM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> In order to provide a mechanism for devilering provisoning rights:
>
> 1. Add a new file to the securityfs file called sgx/provision that works
> as a token for allowing an enclave to have the provisioning privileges.
> 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
> following data structure:
>
> struct sgx_enclave_set_attribute {
> __u64 addr;
> __u64 token_fd;
> };
Here's a potential issue:
For container use, is it reasonable for a container manager to
bind-mount a file into securityfs? Or would something in /dev make
this easier?
^ permalink raw reply
* Re: mount.nfs: Protocol error after upgrade to linux/master
From: Kees Cook @ 2019-03-21 16:38 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Casey Schaufler, Jakub Kicinski, linux-security-module,
Trond Myklebust, open list:NFS, SUNRPC, AND..., Anna Schumaker,
LKML
In-Reply-To: <2bf23acd-22c4-a260-7648-845887a409d5@i-love.sakura.ne.jp>
On Tue, Mar 19, 2019 at 3:56 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> Since Kees Cook seems to be busy now, here is my version...
>
> From 885553e4793d9af2d4e9e99c7d137b0ec7b5f8ad Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Tue, 19 Mar 2019 19:52:31 +0900
> Subject: [PATCH] LSM: Revive CONFIG_DEFAULT_SECURITY_* for "make oldconfig"
>
> Commit 70b62c25665f636c ("LoadPin: Initialize as ordered LSM") removed
> CONFIG_DEFAULT_SECURITY_{SELINUX,SMACK,TOMOYO,APPARMOR,DAC} from
> security/Kconfig and changed CONFIG_LSM to provide a fixed ordering as a
> default value. That commit expected that existing users (upgrading from
> Linux 5.0 and earlier) will edit CONFIG_LSM value in accordance with
> their CONFIG_DEFAULT_SECURITY_* choice in their old kernel configs. But
> since users might forget to edit CONFIG_LSM value, this patch revives
> the choice (only for providing the default value for CONFIG_LSM) in order
> to make sure that CONFIG_LSM reflects CONFIG_DEFAULT_SECURITY_* from their
> old kernel configs.
>
> Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> security/Kconfig | 36 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/security/Kconfig b/security/Kconfig
> index 1d6463f..743e594 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -239,9 +239,43 @@ source "security/safesetid/Kconfig"
>
> source "security/integrity/Kconfig"
>
> +choice
> + prompt "Default security module [superseded by 'Ordered list of enabled LSMs' below]"
> + default DEFAULT_SECURITY_SELINUX if SECURITY_SELINUX
> + default DEFAULT_SECURITY_SMACK if SECURITY_SMACK
> + default DEFAULT_SECURITY_TOMOYO if SECURITY_TOMOYO
> + default DEFAULT_SECURITY_APPARMOR if SECURITY_APPARMOR
> + default DEFAULT_SECURITY_DAC
> +
> + help
> + This choice is there only for converting CONFIG_DEFAULT_SECURITY in old
> + kernel config to CONFIG_LSM in new kernel config. Don't change this choice
> + unless you are creating a fresh kernel config, for this choice will be
> + ignored after CONFIG_LSM is once defined.
> +
> + config DEFAULT_SECURITY_SELINUX
> + bool "SELinux" if SECURITY_SELINUX=y
> +
> + config DEFAULT_SECURITY_SMACK
> + bool "Simplified Mandatory Access Control" if SECURITY_SMACK=y
> +
> + config DEFAULT_SECURITY_TOMOYO
> + bool "TOMOYO" if SECURITY_TOMOYO=y
> +
> + config DEFAULT_SECURITY_APPARMOR
> + bool "AppArmor" if SECURITY_APPARMOR=y
> + config DEFAULT_SECURITY_DAC
> + bool "Unix Discretionary Access Controls"
> +
> +endchoice
> +
> config LSM
> string "Ordered list of enabled LSMs"
> - default "yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor"
> + default "yama,loadpin,safesetid,integrity,selinux" if DEFAULT_SECURITY_SELINUX
> + default "yama,loadpin,safesetid,integrity,smack" if DEFAULT_SECURITY_SMACK
> + default "yama,loadpin,safesetid,integrity,tomoyo" if DEFAULT_SECURITY_TOMOYO
> + default "yama,loadpin,safesetid,integrity,apparmor" if DEFAULT_SECURITY_APPARMOR
> + default "yama,loadpin,safesetid,integrity"
> help
> A comma-separated list of LSMs, in initialization order.
> Any LSMs left off this list will be ignored. This can be
This is mostly good. I'd like to keep the other LSMs listed though
(similar to what I had originally) so that if a legacy-major doesn't
initialize, later ones will be. I want to remove the concept of
"major" LSMs. The only thing that should matter is init order...
-Kees
> --
> 1.8.3.1
>
>
--
Kees Cook
^ permalink raw reply
* Re: what happened to SECURITY_DAC?
From: Kees Cook @ 2019-03-21 16:36 UTC (permalink / raw)
To: Randy Dunlap; +Cc: LKML, linux-security-module
In-Reply-To: <ba89a02a-5550-689f-6609-1e8c025f5b91@infradead.org>
On Wed, Mar 20, 2019 at 5:39 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> wow. Commit 70b62c25665f636c9f6c700b26af7df296b0887e
> from last Sept. 14, 2018, total commit description says:
>
> LoadPin: Initialize as ordered LSM
>
> This converts LoadPin from being a direct "minor" LSM into an ordered LSM.
>
> Nowhere does it say anything like "this also deletes any notions of
> DEFAULT_SECURITY and DAC."
This was a mistaken merge of chunks from a larger series that Casey
and I were working on. It should have been split out.
> Was this deletion a (sekrit) security issue that was not being highlighted on purpose?
No, just an error while refactoring to in creating CONFIG_LSM.
> and what do you recommend for simple DAC-like security?
See CONFIG_LSM. There are some related issues getting discussed here, too:
https://lore.kernel.org/linux-security-module/f23d0fad-dc72-0e53-cac6-31abfd12a050@I-love.SAKURA.ne.jp/
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v19 17/27] x86/sgx: Add provisioning
From: Nathaniel McCallum @ 2019-03-21 14:38 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: x86, linux-sgx, akpm, dave.hansen, sean.j.christopherson,
Neil Horman, Ayoun, Serge, shay.katz-zamir, Huang, Haitao,
andriy.shevchenko, tglx, Svahn, Kai, bp, josh, luto, kai.huang,
rientjes, James Morris, Serge E . Hallyn, linux-security-module
In-Reply-To: <20190317211456.13927-18-jarkko.sakkinen@linux.intel.com>
On Sun, Mar 17, 2019 at 5:18 PM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> In order to provide a mechanism for devilering provisoning rights:
I'm not sure what nefarious spirits have to do with this patch.
Perhaps you meant "delivering provisioning"? ;)
> 1. Add a new file to the securityfs file called sgx/provision that works
> as a token for allowing an enclave to have the provisioning privileges.
> 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
> following data structure:
>
> struct sgx_enclave_set_attribute {
> __u64 addr;
> __u64 token_fd;
> };
>
> A daemon could sit on top of sgx/provision and send a file descriptor of
> this file to a process that needs to be able to provision enclaves.
>
> The way this API is used is more or less straight-forward. Lets assume that
> dev_fd is a handle to /dev/sgx and prov_fd is a handle to sgx/provision.
> You would allow SGX_IOC_ENCLAVE_CREATE to initialize an enclave with the
> PROVISIONKEY attribute by
>
> params.addr = <enclave address>;
> params.token_fd = prov_fd;
>
> ioctl(dev_fd, SGX_IOC_ENCLAVE_SET_ATTRIBUTE, ¶ms);
>
> Cc: James Morris <jmorris@namei.org>
> Cc: Serge E. Hallyn <serge@hallyn.com>
> Cc: linux-security-module@vger.kernel.org
> Suggested-by: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
> arch/x86/include/uapi/asm/sgx.h | 13 +++++++
> arch/x86/kernel/cpu/sgx/driver/ioctl.c | 43 +++++++++++++++++++++++
> arch/x86/kernel/cpu/sgx/driver/main.c | 47 ++++++++++++++++++++++++++
> 3 files changed, 103 insertions(+)
>
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index aadf9c76e360..150a784db395 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -16,6 +16,8 @@
> _IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page)
> #define SGX_IOC_ENCLAVE_INIT \
> _IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
> +#define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
> + _IOW(SGX_MAGIC, 0x03, struct sgx_enclave_set_attribute)
>
> /* IOCTL return values */
> #define SGX_POWER_LOST_ENCLAVE 0x40000000
> @@ -56,4 +58,15 @@ struct sgx_enclave_init {
> __u64 sigstruct;
> };
>
> +/**
> + * struct sgx_enclave_set_attribute - parameter structure for the
> + * %SGX_IOC_ENCLAVE_INIT ioctl
> + * @addr: address within the ELRANGE
> + * @attribute_fd: file handle of the attribute file in the securityfs
> + */
> +struct sgx_enclave_set_attribute {
> + __u64 addr;
> + __u64 attribute_fd;
> +};
> +
> #endif /* _UAPI_ASM_X86_SGX_H */
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index 4b9a91b53b50..5d85bd3f7876 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -759,6 +759,46 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned int cmd,
> return ret;
> }
>
> +/**
> + * sgx_ioc_enclave_set_attribute - handler for %SGX_IOC_ENCLAVE_SET_ATTRIBUTE
> + * @filep: open file to /dev/sgx
> + * @cmd: the command value
> + * @arg: pointer to a struct sgx_enclave_set_attribute instance
> + *
> + * Sets an attribute matching the attribute file that is pointed by the
> + * parameter structure field attribute_fd.
> + *
> + * Return: 0 on success, -errno otherwise
> + */
> +static long sgx_ioc_enclave_set_attribute(struct file *filep, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct sgx_enclave_set_attribute *params = (void *)arg;
> + struct file *attribute_file;
> + struct sgx_encl *encl;
> + int ret;
> +
> + attribute_file = fget(params->attribute_fd);
> + if (!attribute_file->f_op)
> + return -EINVAL;
> +
> + if (attribute_file->f_op != &sgx_fs_provision_fops) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = sgx_encl_get(params->addr, &encl);
> + if (ret)
> + goto out;
> +
> + encl->allowed_attributes |= SGX_ATTR_PROVISIONKEY;
> + kref_put(&encl->refcount, sgx_encl_release);
> +
> +out:
> + fput(attribute_file);
> + return ret;
> +}
> +
> typedef long (*sgx_ioc_t)(struct file *filep, unsigned int cmd,
> unsigned long arg);
>
> @@ -778,6 +818,9 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> case SGX_IOC_ENCLAVE_INIT:
> handler = sgx_ioc_enclave_init;
> break;
> + case SGX_IOC_ENCLAVE_SET_ATTRIBUTE:
> + handler = sgx_ioc_enclave_set_attribute;
> + break;
> default:
> return -ENOIOCTLCMD;
> }
> diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
> index 16f36cd0af04..9a5360dcad98 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/main.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/main.c
> @@ -22,6 +22,11 @@ u64 sgx_attributes_reserved_mask;
> u64 sgx_xfrm_reserved_mask = ~0x3;
> u32 sgx_xsave_size_tbl[64];
>
> +const struct file_operations sgx_fs_provision_fops;
> +
> +static struct dentry *sgx_fs;
> +static struct dentry *sgx_fs_provision;
> +
> #ifdef CONFIG_COMPAT
> static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
> unsigned long arg)
> @@ -147,6 +152,40 @@ static struct sgx_dev_ctx *sgxm_dev_ctx_alloc(struct device *parent)
> return ctx;
> }
>
> +static int sgx_fs_init(struct device *dev)
> +{
> + int ret;
> +
> + sgx_fs = securityfs_create_dir(dev_name(dev), NULL);
> + if (IS_ERR(sgx_fs)) {
> + ret = PTR_ERR(sgx_fs);
> + goto err_sgx_fs;
> + }
> +
> + sgx_fs_provision = securityfs_create_file("provision", 0600, sgx_fs,
> + NULL, &sgx_fs_provision_fops);
> + if (IS_ERR(sgx_fs)) {
> + ret = PTR_ERR(sgx_fs_provision);
> + goto err_sgx_fs_provision;
> + }
> +
> + return 0;
> +
> +err_sgx_fs_provision:
> + securityfs_remove(sgx_fs);
> + sgx_fs_provision = NULL;
> +
> +err_sgx_fs:
> + sgx_fs = NULL;
> + return ret;
> +}
> +
> +static void sgx_fs_remove(void)
> +{
> + securityfs_remove(sgx_fs_provision);
> + securityfs_remove(sgx_fs);
> +}
> +
> static int sgx_dev_init(struct device *parent)
> {
> struct sgx_dev_ctx *sgx_dev;
> @@ -190,6 +229,10 @@ static int sgx_dev_init(struct device *parent)
> if (!sgx_encl_wq)
> return -ENOMEM;
>
> + ret = sgx_fs_init(&sgx_dev->ctrl_dev);
> + if (ret)
> + goto err_fs_init;
> +
> ret = cdev_device_add(&sgx_dev->ctrl_cdev, &sgx_dev->ctrl_dev);
> if (ret)
> goto err_device_add;
> @@ -197,6 +240,9 @@ static int sgx_dev_init(struct device *parent)
> return 0;
>
> err_device_add:
> + sgx_fs_remove();
> +
> +err_fs_init:
> destroy_workqueue(sgx_encl_wq);
> return ret;
> }
> @@ -220,6 +266,7 @@ static int sgx_drv_remove(struct platform_device *pdev)
> {
> struct sgx_dev_ctx *ctx = dev_get_drvdata(&pdev->dev);
>
> + sgx_fs_remove();
> cdev_device_del(&ctx->ctrl_cdev, &ctx->ctrl_dev);
> destroy_workqueue(sgx_encl_wq);
>
> --
> 2.19.1
>
^ permalink raw reply
* Re: [PATCH v19 17/27] x86/sgx: Add provisioning
From: Jarkko Sakkinen @ 2019-03-21 14:32 UTC (permalink / raw)
To: Huang, Kai
Cc: Christopherson, Sean J, Svahn, Kai, nhorman@redhat.com,
jmorris@namei.org, rientjes@google.com, josh@joshtriplett.org,
tglx@linutronix.de, Ayoun, Serge, Huang, Haitao,
linux-security-module@vger.kernel.org, x86@kernel.org,
akpm@linux-foundation.org, npmccallum@redhat.com,
linux-sgx@vger.kernel.org, luto@kernel.org, Katz-zamir, Shay,
Hansen, Dave, bp@alien8.de, serge@hallyn.com,
andriy.shevchenko@linux.intel.com
In-Reply-To: <1553134108.1952.4.camel@intel.com>
On Thu, Mar 21, 2019 at 02:08:36AM +0000, Huang, Kai wrote:
> On Tue, 2019-03-19 at 13:09 -0700, Sean Christopherson wrote:
> > On Sun, Mar 17, 2019 at 11:14:46PM +0200, Jarkko Sakkinen wrote:
> > > In order to provide a mechanism for devilering provisoning rights:
> > >
> > > 1. Add a new file to the securityfs file called sgx/provision that works
> > > as a token for allowing an enclave to have the provisioning privileges.
> > > 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
> > > following data structure:
> > >
> > > struct sgx_enclave_set_attribute {
> > > __u64 addr;
> > > __u64 token_fd;
> > > };
>
> Would you elaborate why the name is "token_fd"? I think *token* in SGX
> has more specific meaning?
I'm open for other names.
/Jarkko
^ permalink raw reply
* Re: [PATCH v19 17/27] x86/sgx: Add provisioning
From: Jarkko Sakkinen @ 2019-03-21 14:30 UTC (permalink / raw)
To: Sean Christopherson
Cc: x86, linux-sgx, akpm, dave.hansen, nhorman, npmccallum,
serge.ayoun, shay.katz-zamir, haitao.huang, andriy.shevchenko,
tglx, kai.svahn, bp, josh, luto, kai.huang, rientjes,
James Morris, Serge E . Hallyn, linux-security-module
In-Reply-To: <20190319200912.GH25575@linux.intel.com>
On Tue, Mar 19, 2019 at 01:09:12PM -0700, Sean Christopherson wrote:
> On Sun, Mar 17, 2019 at 11:14:46PM +0200, Jarkko Sakkinen wrote:
> > In order to provide a mechanism for devilering provisoning rights:
> >
> > 1. Add a new file to the securityfs file called sgx/provision that works
> > as a token for allowing an enclave to have the provisioning privileges.
> > 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
> > following data structure:
> >
> > struct sgx_enclave_set_attribute {
> > __u64 addr;
> > __u64 token_fd;
> > };
> >
> > A daemon could sit on top of sgx/provision and send a file descriptor of
> > this file to a process that needs to be able to provision enclaves.
> >
> > The way this API is used is more or less straight-forward. Lets assume that
> > dev_fd is a handle to /dev/sgx and prov_fd is a handle to sgx/provision.
> > You would allow SGX_IOC_ENCLAVE_CREATE to initialize an enclave with the
> > PROVISIONKEY attribute by
> >
> > params.addr = <enclave address>;
> > params.token_fd = prov_fd;
> >
> > ioctl(dev_fd, SGX_IOC_ENCLAVE_SET_ATTRIBUTE, ¶ms);
> >
> > Cc: James Morris <jmorris@namei.org>
> > Cc: Serge E. Hallyn <serge@hallyn.com>
> > Cc: linux-security-module@vger.kernel.org
> > Suggested-by: Andy Lutomirski <luto@kernel.org>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> > arch/x86/include/uapi/asm/sgx.h | 13 +++++++
> > arch/x86/kernel/cpu/sgx/driver/ioctl.c | 43 +++++++++++++++++++++++
> > arch/x86/kernel/cpu/sgx/driver/main.c | 47 ++++++++++++++++++++++++++
> > 3 files changed, 103 insertions(+)
> >
> > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> > index aadf9c76e360..150a784db395 100644
> > --- a/arch/x86/include/uapi/asm/sgx.h
> > +++ b/arch/x86/include/uapi/asm/sgx.h
> > @@ -16,6 +16,8 @@
> > _IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page)
> > #define SGX_IOC_ENCLAVE_INIT \
> > _IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
> > +#define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
> > + _IOW(SGX_MAGIC, 0x03, struct sgx_enclave_set_attribute)
> >
> > /* IOCTL return values */
> > #define SGX_POWER_LOST_ENCLAVE 0x40000000
> > @@ -56,4 +58,15 @@ struct sgx_enclave_init {
> > __u64 sigstruct;
> > };
> >
> > +/**
> > + * struct sgx_enclave_set_attribute - parameter structure for the
> > + * %SGX_IOC_ENCLAVE_INIT ioctl
> > + * @addr: address within the ELRANGE
> > + * @attribute_fd: file handle of the attribute file in the securityfs
> > + */
> > +struct sgx_enclave_set_attribute {
> > + __u64 addr;
> > + __u64 attribute_fd;
> > +};
> > +
> > #endif /* _UAPI_ASM_X86_SGX_H */
> > diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > index 4b9a91b53b50..5d85bd3f7876 100644
> > --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > @@ -759,6 +759,46 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned int cmd,
> > return ret;
> > }
> >
> > +/**
> > + * sgx_ioc_enclave_set_attribute - handler for %SGX_IOC_ENCLAVE_SET_ATTRIBUTE
> > + * @filep: open file to /dev/sgx
> > + * @cmd: the command value
> > + * @arg: pointer to a struct sgx_enclave_set_attribute instance
> > + *
> > + * Sets an attribute matching the attribute file that is pointed by the
> > + * parameter structure field attribute_fd.
>
> With the @data change (see below), this becomes something like:
>
> * Allow the enclave to request the attribute managed by the SGX security file
> * pointed at by the parameter structure field attribute_fd.
I see your point but the current implementation is just a tiny bit
simpler and right now we have one single file.
But I would do to fix this patch right now is to rename
sgx_fs_provision_ops as sgx_fs_ops to point out that only single fops
are required.
The current implementation is "good enough" for handling the
provisioning key.
/Jarkko
^ permalink raw reply
* [GIT PULL] SELinux fixes for v5.1 (#2)
From: Paul Moore @ 2019-03-21 14:09 UTC (permalink / raw)
To: Linus Torvalds; +Cc: selinux, linux-security-module, linux-kernel
Hi Linus,
Another small SELinux fix for v5.1, please merge.
Thanks,
-Paul
--
The following changes since commit 9e98c678c2d6ae3a17cb2de55d17f69dddaa231b:
Linux 5.1-rc1 (2019-03-17 14:22:26 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
tags/selinux-pr-20190321
for you to fetch changes up to 6a1afffb08ce5f9fb9ccc20f7ab24846c0142984:
selinux: fix NULL dereference in policydb_destroy()
(2019-03-18 12:19:48 -0400)
----------------------------------------------------------------
selinux/stable-5.1 PR 20190321
----------------------------------------------------------------
Ondrej Mosnacek (1):
selinux: fix NULL dereference in policydb_destroy()
security/selinux/ss/policydb.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v10, RESEND 5/6] KEYS: trusted: explicitly use tpm_chip structure from tpm_default_chip()
From: Roberto Sassu @ 2019-03-21 13:25 UTC (permalink / raw)
To: Jarkko Sakkinen, Dan Williams
Cc: zohar, david.safford, monty.wiseman, matthewgarrett,
linux-integrity, linux-security-module, keyrings,
Linux Kernel Mailing List, silviu.vlasceanu, linux-nvdimm
In-Reply-To: <20190321131554.GB2267@linux.intel.com>
On 3/21/2019 2:15 PM, Jarkko Sakkinen wrote:
> On Mon, Mar 18, 2019 at 03:35:08PM -0700, Dan Williams wrote:
>> On Wed, Feb 6, 2019 at 10:30 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>>>
>>> When crypto agility support will be added to the TPM driver, users of the
>>> driver have to retrieve the allocated banks from chip->allocated_banks and
>>> use this information to prepare the array of tpm_digest structures to be
>>> passed to tpm_pcr_extend().
>>>
>>> This patch retrieves a tpm_chip pointer from tpm_default_chip() so that the
>>> pointer can be used to prepare the array of tpm_digest structures.
>>>
>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>> ---
>>> security/keys/trusted.c | 38 ++++++++++++++++++++++++--------------
>>> 1 file changed, 24 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
>>> index 4d98f4f87236..5b852263eae1 100644
>>> --- a/security/keys/trusted.c
>>> +++ b/security/keys/trusted.c
>>> @@ -34,6 +34,7 @@
>>>
>>> static const char hmac_alg[] = "hmac(sha1)";
>>> static const char hash_alg[] = "sha1";
>>> +static struct tpm_chip *chip;
>>>
>>> struct sdesc {
>>> struct shash_desc shash;
>>> @@ -362,7 +363,7 @@ int trusted_tpm_send(unsigned char *cmd, size_t buflen)
>>> int rc;
>>>
>>> dump_tpm_buf(cmd);
>>> - rc = tpm_send(NULL, cmd, buflen);
>>> + rc = tpm_send(chip, cmd, buflen);
>>> dump_tpm_buf(cmd);
>>> if (rc > 0)
>>> /* Can't return positive return codes values to keyctl */
>>> @@ -384,10 +385,10 @@ static int pcrlock(const int pcrnum)
>>>
>>> if (!capable(CAP_SYS_ADMIN))
>>> return -EPERM;
>>> - ret = tpm_get_random(NULL, hash, SHA1_DIGEST_SIZE);
>>> + ret = tpm_get_random(chip, hash, SHA1_DIGEST_SIZE);
>>> if (ret != SHA1_DIGEST_SIZE)
>>> return ret;
>>> - return tpm_pcr_extend(NULL, pcrnum, hash) ? -EINVAL : 0;
>>> + return tpm_pcr_extend(chip, pcrnum, hash) ? -EINVAL : 0;
>>> }
>>>
>>> /*
>>> @@ -400,7 +401,7 @@ static int osap(struct tpm_buf *tb, struct osapsess *s,
>>> unsigned char ononce[TPM_NONCE_SIZE];
>>> int ret;
>>>
>>> - ret = tpm_get_random(NULL, ononce, TPM_NONCE_SIZE);
>>> + ret = tpm_get_random(chip, ononce, TPM_NONCE_SIZE);
>>> if (ret != TPM_NONCE_SIZE)
>>> return ret;
>>>
>>> @@ -496,7 +497,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
>>> if (ret < 0)
>>> goto out;
>>>
>>> - ret = tpm_get_random(NULL, td->nonceodd, TPM_NONCE_SIZE);
>>> + ret = tpm_get_random(chip, td->nonceodd, TPM_NONCE_SIZE);
>>> if (ret != TPM_NONCE_SIZE)
>>> goto out;
>>> ordinal = htonl(TPM_ORD_SEAL);
>>> @@ -606,7 +607,7 @@ static int tpm_unseal(struct tpm_buf *tb,
>>>
>>> ordinal = htonl(TPM_ORD_UNSEAL);
>>> keyhndl = htonl(SRKHANDLE);
>>> - ret = tpm_get_random(NULL, nonceodd, TPM_NONCE_SIZE);
>>> + ret = tpm_get_random(chip, nonceodd, TPM_NONCE_SIZE);
>>> if (ret != TPM_NONCE_SIZE) {
>>> pr_info("trusted_key: tpm_get_random failed (%d)\n", ret);
>>> return ret;
>>> @@ -751,7 +752,7 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
>>> int i;
>>> int tpm2;
>>>
>>> - tpm2 = tpm_is_tpm2(NULL);
>>> + tpm2 = tpm_is_tpm2(chip);
>>> if (tpm2 < 0)
>>> return tpm2;
>>>
>>> @@ -920,7 +921,7 @@ static struct trusted_key_options *trusted_options_alloc(void)
>>> struct trusted_key_options *options;
>>> int tpm2;
>>>
>>> - tpm2 = tpm_is_tpm2(NULL);
>>> + tpm2 = tpm_is_tpm2(chip);
>>> if (tpm2 < 0)
>>> return NULL;
>>>
>>> @@ -970,7 +971,7 @@ static int trusted_instantiate(struct key *key,
>>> size_t key_len;
>>> int tpm2;
>>>
>>> - tpm2 = tpm_is_tpm2(NULL);
>>> + tpm2 = tpm_is_tpm2(chip);
>>> if (tpm2 < 0)
>>> return tpm2;
>>>
>>> @@ -1011,7 +1012,7 @@ static int trusted_instantiate(struct key *key,
>>> switch (key_cmd) {
>>> case Opt_load:
>>> if (tpm2)
>>> - ret = tpm_unseal_trusted(NULL, payload, options);
>>> + ret = tpm_unseal_trusted(chip, payload, options);
>>> else
>>> ret = key_unseal(payload, options);
>>> dump_payload(payload);
>>> @@ -1021,13 +1022,13 @@ static int trusted_instantiate(struct key *key,
>>> break;
>>> case Opt_new:
>>> key_len = payload->key_len;
>>> - ret = tpm_get_random(NULL, payload->key, key_len);
>>> + ret = tpm_get_random(chip, payload->key, key_len);
>>> if (ret != key_len) {
>>> pr_info("trusted_key: key_create failed (%d)\n", ret);
>>> goto out;
>>> }
>>> if (tpm2)
>>> - ret = tpm_seal_trusted(NULL, payload, options);
>>> + ret = tpm_seal_trusted(chip, payload, options);
>>> else
>>> ret = key_seal(payload, options);
>>> if (ret < 0)
>>> @@ -1225,17 +1226,26 @@ static int __init init_trusted(void)
>>> {
>>> int ret;
>>>
>>> + chip = tpm_default_chip();
>>> + if (!chip)
>>> + return -ENOENT;
>>
>> This change causes a regression loading the encrypted_keys module on
>> systems that don't have a tpm.
>>
>> Module init functions should not have hardware dependencies.
>>
>> The effect is that the libnvdimm module, which is an encrypted_keys
>> user, fails to load, but up until this change encrypted_keys did not
>> have a hard dependency on TPM presence.
>
> Sorry for the latency. I was in flu for couple of days.
>
> I missed that addition in the review process albeit this patch set
> went numerous rounds. Apologies about ths. Also the return value is
> wrong. Should be -ENODEV but it doesn't matter because this needs to
> be removed anyway.
>
> Roberto, can you submit a fix ASAP that:
Ok, I will do it now.
Roberto
> 1. Allows the module to initialize even if the chip is not found.
> 2. In the beginning of each function (before tpm_is_tpm2()) you
> should check if chip is NULL and return -ENODEV if it is.
>
> Add also these tags before your signed-off-by:
>
> Cc: stable@vger.kernel.org
> Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure from tpm_default_chip()")
> Reported-by: Dan Williams <dan.j.williams@gmail.com>
> Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>
> /Jarkko
>
--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI
^ permalink raw reply
* Re: [PATCH v10, RESEND 5/6] KEYS: trusted: explicitly use tpm_chip structure from tpm_default_chip()
From: Jarkko Sakkinen @ 2019-03-21 13:15 UTC (permalink / raw)
To: Dan Williams, roberto.sassu
Cc: Roberto Sassu, zohar, david.safford, monty.wiseman,
matthewgarrett, linux-integrity, linux-security-module, keyrings,
Linux Kernel Mailing List, silviu.vlasceanu, linux-nvdimm
In-Reply-To: <CAA9_cmf0j1EoyrGmbfPWCWPafgGfKWR6cyPpN8YEFZdemeg1kA@mail.gmail.com>
On Mon, Mar 18, 2019 at 03:35:08PM -0700, Dan Williams wrote:
> On Wed, Feb 6, 2019 at 10:30 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
> >
> > When crypto agility support will be added to the TPM driver, users of the
> > driver have to retrieve the allocated banks from chip->allocated_banks and
> > use this information to prepare the array of tpm_digest structures to be
> > passed to tpm_pcr_extend().
> >
> > This patch retrieves a tpm_chip pointer from tpm_default_chip() so that the
> > pointer can be used to prepare the array of tpm_digest structures.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> > security/keys/trusted.c | 38 ++++++++++++++++++++++++--------------
> > 1 file changed, 24 insertions(+), 14 deletions(-)
> >
> > diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> > index 4d98f4f87236..5b852263eae1 100644
> > --- a/security/keys/trusted.c
> > +++ b/security/keys/trusted.c
> > @@ -34,6 +34,7 @@
> >
> > static const char hmac_alg[] = "hmac(sha1)";
> > static const char hash_alg[] = "sha1";
> > +static struct tpm_chip *chip;
> >
> > struct sdesc {
> > struct shash_desc shash;
> > @@ -362,7 +363,7 @@ int trusted_tpm_send(unsigned char *cmd, size_t buflen)
> > int rc;
> >
> > dump_tpm_buf(cmd);
> > - rc = tpm_send(NULL, cmd, buflen);
> > + rc = tpm_send(chip, cmd, buflen);
> > dump_tpm_buf(cmd);
> > if (rc > 0)
> > /* Can't return positive return codes values to keyctl */
> > @@ -384,10 +385,10 @@ static int pcrlock(const int pcrnum)
> >
> > if (!capable(CAP_SYS_ADMIN))
> > return -EPERM;
> > - ret = tpm_get_random(NULL, hash, SHA1_DIGEST_SIZE);
> > + ret = tpm_get_random(chip, hash, SHA1_DIGEST_SIZE);
> > if (ret != SHA1_DIGEST_SIZE)
> > return ret;
> > - return tpm_pcr_extend(NULL, pcrnum, hash) ? -EINVAL : 0;
> > + return tpm_pcr_extend(chip, pcrnum, hash) ? -EINVAL : 0;
> > }
> >
> > /*
> > @@ -400,7 +401,7 @@ static int osap(struct tpm_buf *tb, struct osapsess *s,
> > unsigned char ononce[TPM_NONCE_SIZE];
> > int ret;
> >
> > - ret = tpm_get_random(NULL, ononce, TPM_NONCE_SIZE);
> > + ret = tpm_get_random(chip, ononce, TPM_NONCE_SIZE);
> > if (ret != TPM_NONCE_SIZE)
> > return ret;
> >
> > @@ -496,7 +497,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> > if (ret < 0)
> > goto out;
> >
> > - ret = tpm_get_random(NULL, td->nonceodd, TPM_NONCE_SIZE);
> > + ret = tpm_get_random(chip, td->nonceodd, TPM_NONCE_SIZE);
> > if (ret != TPM_NONCE_SIZE)
> > goto out;
> > ordinal = htonl(TPM_ORD_SEAL);
> > @@ -606,7 +607,7 @@ static int tpm_unseal(struct tpm_buf *tb,
> >
> > ordinal = htonl(TPM_ORD_UNSEAL);
> > keyhndl = htonl(SRKHANDLE);
> > - ret = tpm_get_random(NULL, nonceodd, TPM_NONCE_SIZE);
> > + ret = tpm_get_random(chip, nonceodd, TPM_NONCE_SIZE);
> > if (ret != TPM_NONCE_SIZE) {
> > pr_info("trusted_key: tpm_get_random failed (%d)\n", ret);
> > return ret;
> > @@ -751,7 +752,7 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
> > int i;
> > int tpm2;
> >
> > - tpm2 = tpm_is_tpm2(NULL);
> > + tpm2 = tpm_is_tpm2(chip);
> > if (tpm2 < 0)
> > return tpm2;
> >
> > @@ -920,7 +921,7 @@ static struct trusted_key_options *trusted_options_alloc(void)
> > struct trusted_key_options *options;
> > int tpm2;
> >
> > - tpm2 = tpm_is_tpm2(NULL);
> > + tpm2 = tpm_is_tpm2(chip);
> > if (tpm2 < 0)
> > return NULL;
> >
> > @@ -970,7 +971,7 @@ static int trusted_instantiate(struct key *key,
> > size_t key_len;
> > int tpm2;
> >
> > - tpm2 = tpm_is_tpm2(NULL);
> > + tpm2 = tpm_is_tpm2(chip);
> > if (tpm2 < 0)
> > return tpm2;
> >
> > @@ -1011,7 +1012,7 @@ static int trusted_instantiate(struct key *key,
> > switch (key_cmd) {
> > case Opt_load:
> > if (tpm2)
> > - ret = tpm_unseal_trusted(NULL, payload, options);
> > + ret = tpm_unseal_trusted(chip, payload, options);
> > else
> > ret = key_unseal(payload, options);
> > dump_payload(payload);
> > @@ -1021,13 +1022,13 @@ static int trusted_instantiate(struct key *key,
> > break;
> > case Opt_new:
> > key_len = payload->key_len;
> > - ret = tpm_get_random(NULL, payload->key, key_len);
> > + ret = tpm_get_random(chip, payload->key, key_len);
> > if (ret != key_len) {
> > pr_info("trusted_key: key_create failed (%d)\n", ret);
> > goto out;
> > }
> > if (tpm2)
> > - ret = tpm_seal_trusted(NULL, payload, options);
> > + ret = tpm_seal_trusted(chip, payload, options);
> > else
> > ret = key_seal(payload, options);
> > if (ret < 0)
> > @@ -1225,17 +1226,26 @@ static int __init init_trusted(void)
> > {
> > int ret;
> >
> > + chip = tpm_default_chip();
> > + if (!chip)
> > + return -ENOENT;
>
> This change causes a regression loading the encrypted_keys module on
> systems that don't have a tpm.
>
> Module init functions should not have hardware dependencies.
>
> The effect is that the libnvdimm module, which is an encrypted_keys
> user, fails to load, but up until this change encrypted_keys did not
> have a hard dependency on TPM presence.
Sorry for the latency. I was in flu for couple of days.
I missed that addition in the review process albeit this patch set
went numerous rounds. Apologies about ths. Also the return value is
wrong. Should be -ENODEV but it doesn't matter because this needs to
be removed anyway.
Roberto, can you submit a fix ASAP that:
1. Allows the module to initialize even if the chip is not found.
2. In the beginning of each function (before tpm_is_tpm2()) you
should check if chip is NULL and return -ENODEV if it is.
Add also these tags before your signed-off-by:
Cc: stable@vger.kernel.org
Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure from tpm_default_chip()")
Reported-by: Dan Williams <dan.j.williams@gmail.com>
Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
^ permalink raw reply
* Re: [PATCH v7 0/7] Allow initializing the kernfs node's secctx based on its parent
From: Ondrej Mosnacek @ 2019-03-21 8:56 UTC (permalink / raw)
To: Paul Moore
Cc: selinux, Stephen Smalley, Linux Security Module list, Tejun Heo,
Casey Schaufler, Serge E . Hallyn, Greg Kroah-Hartman,
James Morris, linux-fsdevel, cgroups
In-Reply-To: <CAHC9VhT3pEAwYyHcv7KY4xAHJQKu4PN-NPxAkpcnuGCSwVHLtQ@mail.gmail.com>
On Thu, Mar 21, 2019 at 3:14 AM Paul Moore <paul@paul-moore.com> wrote:
> On Fri, Feb 22, 2019 at 9:57 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > TL;DR:
> > This series adds a new security hook that allows to initialize the security
> > context of kernfs properly, taking into account the parent context (and
> > possibly other attributes). Kernfs nodes require special handling here, since
> > they are not bound to specific inodes/superblocks, but instead represent the
> > backing tree structure that is used to build the VFS tree when the kernfs
> > tree is mounted.
> >
> > Changes in v7:
> > - simplify the new security hook's interface
> > - rather than trying to extract kernfs data into common structures, just
> > pass the kernfs nodes themselves and add helper functions to
> > <linux/kernfs.h> for accessing their security xattrs
> > - in case other LSMs need more kernfs node attributes than the file mode
> > (uid/gid/...), they can simply add new helpers to <linux/kernfs.h> as
> > needed
> > - refactor "kernfs: use simple_xattrs for security attributes" to keep using
> > a single common simple_xattrs structure
> > - turns out having two separate simple_xattrs wouldn't work right (see
> > the definition of simple_xattr_list() in fs/xattr.c)
> > - drop unnecessary initializations from inode_doinit_use_xattr()
> > - move the IOP_XATTR check out of inode_doinit_use_xattr()
> > - add two kernfs cleanup patches
> > - these could be applied independently, but the rest of the patches depend on
> > them, so I'd rather they stay bundled with the rest to avoid cross-tree
> > conflicts
> >
> > v6: https://lore.kernel.org/selinux/20190214095015.16032-1-omosnace@redhat.com/T/
> > Changes in v6:
> > - remove copy-pasted duplicate macro definition
> >
> > v5: https://lore.kernel.org/selinux/20190205110638.30782-1-omosnace@redhat.com/T/
> > Changes in v5:
> > - fix misplaced semicolon detected by 0day robot
> >
> > v4: https://lore.kernel.org/selinux/20190205085915.5183-1-omosnace@redhat.com/T/
> > Changes in v4:
> > - reorder and rename hook arguments
> > - avoid allocating kernfs_iattrs unless needed
> >
> > v3: https://lore.kernel.org/selinux/20190130114150.27807-1-omosnace@redhat.com/T/
> > Changes in v3:
> > - rename the hook to "kernfs_init_security"
> > - change the hook interface to simply pass pointers to struct iattr and
> > struct simple_xattrs of both the new node and its parent
> > - add full security xattr support to kernfs (and fixup SELinux behavior
> > to handle it properly)
> >
> > v2: https://lore.kernel.org/selinux/20190109162830.8309-1-omosnace@redhat.com/T/
> > Changes in v2:
> > - add docstring for the new hook in union security_list_options
> > - initialize *ctx to NULL and *ctxlen to 0 in case the hook is not
> > implemented
> >
> > v1: https://lore.kernel.org/selinux/20190109091028.24485-1-omosnace@redhat.com/T/
> >
> > The kernfs nodes initially do not store any security context and rely on
> > the LSM to assign some default context to inodes created over them. Kernfs
> > inodes, however, allow setting an explicit context via the *setxattr(2)
> > syscalls, in which case the context is stored inside the kernfs node's
> > internal structure.
> >
> > SELinux (and possibly other LSMs) initialize the context of newly created
> > FS objects based on the parent object's context (usually the child inherits
> > the parent's context, unless the policy dictates otherwise). This is done
> > by hooking the creation of the new inode corresponding to the newly created
> > file/directory via security_inode_init_security() (most filesystems always
> > create a fresh inode when a new FS object is created). However, kernfs nodes
> > can be created "behind the scenes" while the filesystem is not mounted
> > anywhere and thus no inodes can exist for them yet.
> >
> > Therefore, to allow maintaining similar behavior for kernfs nodes, a new
> > LSM hook is needed, which will allow initializing the kernfs node's
> > security context based on its own attributes and those of the parent's
> > node.
> >
> > The main motivation for this change is that the userspace users of cgroupfs
> > (which is built on kernfs) expect the usual security context inheritance
> > to work under SELinux (see [1] and [2]). This functionality is required for
> > better confinement of containers under SELinux.
> >
> > Patch 1/7 simplifies the kernfs_iattrs structure and patch 2/7 optimizes
> > kernfs to not allocate kernfs_iattrs when getting the value of an xattr.
> >
> > Patch 3/7 changes SELinux to fetch security context from extended
> > attributes on kernfs filesystems, falling back to genfs-defined context
> > if that fails. Without this patch the 4/7 would be a regression for
> > SELinux (due to the removal of ...notifysecctx() call.
> >
> > Patch 4/7 implements full security xattr support in kernfs using
> > simple_xattrs; patch 5/7 adds the new LSM hook; patch 6/7 implements the
> > new hook in SELinux; and patch 7/7 modifies kernfs to call the new hook
> > on new node creation.
> >
> > Testing:
> > - passed the reproducer from the commit message of the last patch
> > - passed SELinux testsuite on Fedora Rawhide (x86_64) when applied on top
> > of current Rawhide kernel (5.0.0-0.rc7.git2.1) [3]
> > - including the new proposed selinux-testsuite subtest [4] (adapted
> > from the reproducer)
> >
> > [1] https://github.com/SELinuxProject/selinux-kernel/issues/39
> > [2] https://bugzilla.redhat.com/show_bug.cgi?id=1553803
> > [3] https://koji.fedoraproject.org/koji/taskinfo?taskID=32963825
> > [4] https://github.com/SELinuxProject/selinux-testsuite/pull/48
> >
> > Ondrej Mosnacek (7):
> > kernfs: clean up struct kernfs_iattrs
> > kernfs: do not alloc iattrs in kernfs_xattr_get
> > selinux: try security xattr after genfs for kernfs filesystems
> > kernfs: use simple_xattrs for security attributes
> > LSM: add new hook for kernfs node initialization
> > selinux: implement the kernfs_init_security hook
> > kernfs: initialize security of newly created nodes
> >
> > fs/kernfs/dir.c | 28 ++--
> > fs/kernfs/inode.c | 166 +++++++++------------
> > fs/kernfs/kernfs-internal.h | 8 +-
> > fs/kernfs/symlink.c | 4 +-
> > include/linux/kernfs.h | 15 ++
> > include/linux/lsm_hooks.h | 13 ++
> > include/linux/security.h | 9 ++
> > security/security.c | 6 +
> > security/selinux/hooks.c | 223 +++++++++++++++++++---------
> > security/selinux/include/security.h | 1 +
> > 10 files changed, 290 insertions(+), 183 deletions(-)
>
> I just merged this patchset into selinux/next, thanks everyone.
>
> Ondrej, every patch in the patchset except for one required some
> amount of merge fixups, please take a look and make sure everything in
> the selinux/next branch still looks right to you.
Looks good to me, thanks! I checked by rebasing the original branch
on v5.1-rc1 myself and then comparing my result with selinux/next.
The only difference was one extra empty line on my side, but that
doesn't bother me :)
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH v7 0/7] Allow initializing the kernfs node's secctx based on its parent
From: Paul Moore @ 2019-03-21 2:14 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: selinux, Stephen Smalley, linux-security-module, Tejun Heo,
Casey Schaufler, Serge E . Hallyn, Greg Kroah-Hartman,
James Morris, linux-fsdevel, cgroups
In-Reply-To: <20190222145718.5740-1-omosnace@redhat.com>
On Fri, Feb 22, 2019 at 9:57 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> TL;DR:
> This series adds a new security hook that allows to initialize the security
> context of kernfs properly, taking into account the parent context (and
> possibly other attributes). Kernfs nodes require special handling here, since
> they are not bound to specific inodes/superblocks, but instead represent the
> backing tree structure that is used to build the VFS tree when the kernfs
> tree is mounted.
>
> Changes in v7:
> - simplify the new security hook's interface
> - rather than trying to extract kernfs data into common structures, just
> pass the kernfs nodes themselves and add helper functions to
> <linux/kernfs.h> for accessing their security xattrs
> - in case other LSMs need more kernfs node attributes than the file mode
> (uid/gid/...), they can simply add new helpers to <linux/kernfs.h> as
> needed
> - refactor "kernfs: use simple_xattrs for security attributes" to keep using
> a single common simple_xattrs structure
> - turns out having two separate simple_xattrs wouldn't work right (see
> the definition of simple_xattr_list() in fs/xattr.c)
> - drop unnecessary initializations from inode_doinit_use_xattr()
> - move the IOP_XATTR check out of inode_doinit_use_xattr()
> - add two kernfs cleanup patches
> - these could be applied independently, but the rest of the patches depend on
> them, so I'd rather they stay bundled with the rest to avoid cross-tree
> conflicts
>
> v6: https://lore.kernel.org/selinux/20190214095015.16032-1-omosnace@redhat.com/T/
> Changes in v6:
> - remove copy-pasted duplicate macro definition
>
> v5: https://lore.kernel.org/selinux/20190205110638.30782-1-omosnace@redhat.com/T/
> Changes in v5:
> - fix misplaced semicolon detected by 0day robot
>
> v4: https://lore.kernel.org/selinux/20190205085915.5183-1-omosnace@redhat.com/T/
> Changes in v4:
> - reorder and rename hook arguments
> - avoid allocating kernfs_iattrs unless needed
>
> v3: https://lore.kernel.org/selinux/20190130114150.27807-1-omosnace@redhat.com/T/
> Changes in v3:
> - rename the hook to "kernfs_init_security"
> - change the hook interface to simply pass pointers to struct iattr and
> struct simple_xattrs of both the new node and its parent
> - add full security xattr support to kernfs (and fixup SELinux behavior
> to handle it properly)
>
> v2: https://lore.kernel.org/selinux/20190109162830.8309-1-omosnace@redhat.com/T/
> Changes in v2:
> - add docstring for the new hook in union security_list_options
> - initialize *ctx to NULL and *ctxlen to 0 in case the hook is not
> implemented
>
> v1: https://lore.kernel.org/selinux/20190109091028.24485-1-omosnace@redhat.com/T/
>
> The kernfs nodes initially do not store any security context and rely on
> the LSM to assign some default context to inodes created over them. Kernfs
> inodes, however, allow setting an explicit context via the *setxattr(2)
> syscalls, in which case the context is stored inside the kernfs node's
> internal structure.
>
> SELinux (and possibly other LSMs) initialize the context of newly created
> FS objects based on the parent object's context (usually the child inherits
> the parent's context, unless the policy dictates otherwise). This is done
> by hooking the creation of the new inode corresponding to the newly created
> file/directory via security_inode_init_security() (most filesystems always
> create a fresh inode when a new FS object is created). However, kernfs nodes
> can be created "behind the scenes" while the filesystem is not mounted
> anywhere and thus no inodes can exist for them yet.
>
> Therefore, to allow maintaining similar behavior for kernfs nodes, a new
> LSM hook is needed, which will allow initializing the kernfs node's
> security context based on its own attributes and those of the parent's
> node.
>
> The main motivation for this change is that the userspace users of cgroupfs
> (which is built on kernfs) expect the usual security context inheritance
> to work under SELinux (see [1] and [2]). This functionality is required for
> better confinement of containers under SELinux.
>
> Patch 1/7 simplifies the kernfs_iattrs structure and patch 2/7 optimizes
> kernfs to not allocate kernfs_iattrs when getting the value of an xattr.
>
> Patch 3/7 changes SELinux to fetch security context from extended
> attributes on kernfs filesystems, falling back to genfs-defined context
> if that fails. Without this patch the 4/7 would be a regression for
> SELinux (due to the removal of ...notifysecctx() call.
>
> Patch 4/7 implements full security xattr support in kernfs using
> simple_xattrs; patch 5/7 adds the new LSM hook; patch 6/7 implements the
> new hook in SELinux; and patch 7/7 modifies kernfs to call the new hook
> on new node creation.
>
> Testing:
> - passed the reproducer from the commit message of the last patch
> - passed SELinux testsuite on Fedora Rawhide (x86_64) when applied on top
> of current Rawhide kernel (5.0.0-0.rc7.git2.1) [3]
> - including the new proposed selinux-testsuite subtest [4] (adapted
> from the reproducer)
>
> [1] https://github.com/SELinuxProject/selinux-kernel/issues/39
> [2] https://bugzilla.redhat.com/show_bug.cgi?id=1553803
> [3] https://koji.fedoraproject.org/koji/taskinfo?taskID=32963825
> [4] https://github.com/SELinuxProject/selinux-testsuite/pull/48
>
> Ondrej Mosnacek (7):
> kernfs: clean up struct kernfs_iattrs
> kernfs: do not alloc iattrs in kernfs_xattr_get
> selinux: try security xattr after genfs for kernfs filesystems
> kernfs: use simple_xattrs for security attributes
> LSM: add new hook for kernfs node initialization
> selinux: implement the kernfs_init_security hook
> kernfs: initialize security of newly created nodes
>
> fs/kernfs/dir.c | 28 ++--
> fs/kernfs/inode.c | 166 +++++++++------------
> fs/kernfs/kernfs-internal.h | 8 +-
> fs/kernfs/symlink.c | 4 +-
> include/linux/kernfs.h | 15 ++
> include/linux/lsm_hooks.h | 13 ++
> include/linux/security.h | 9 ++
> security/security.c | 6 +
> security/selinux/hooks.c | 223 +++++++++++++++++++---------
> security/selinux/include/security.h | 1 +
> 10 files changed, 290 insertions(+), 183 deletions(-)
I just merged this patchset into selinux/next, thanks everyone.
Ondrej, every patch in the patchset except for one required some
amount of merge fixups, please take a look and make sure everything in
the selinux/next branch still looks right to you.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v19 17/27] x86/sgx: Add provisioning
From: Huang, Kai @ 2019-03-21 2:08 UTC (permalink / raw)
To: jarkko.sakkinen@linux.intel.com, Christopherson, Sean J
Cc: Svahn, Kai, nhorman@redhat.com, jmorris@namei.org,
rientjes@google.com, josh@joshtriplett.org, tglx@linutronix.de,
Ayoun, Serge, Huang, Haitao,
linux-security-module@vger.kernel.org, x86@kernel.org,
akpm@linux-foundation.org, npmccallum@redhat.com,
linux-sgx@vger.kernel.org, luto@kernel.org, Katz-zamir, Shay,
Hansen, Dave, bp@alien8.de, serge@hallyn.com,
andriy.shevchenko@linux.intel.com
In-Reply-To: <20190319200912.GH25575@linux.intel.com>
On Tue, 2019-03-19 at 13:09 -0700, Sean Christopherson wrote:
> On Sun, Mar 17, 2019 at 11:14:46PM +0200, Jarkko Sakkinen wrote:
> > In order to provide a mechanism for devilering provisoning rights:
> >
> > 1. Add a new file to the securityfs file called sgx/provision that works
> > as a token for allowing an enclave to have the provisioning privileges.
> > 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
> > following data structure:
> >
> > struct sgx_enclave_set_attribute {
> > __u64 addr;
> > __u64 token_fd;
> > };
Would you elaborate why the name is "token_fd"? I think *token* in SGX has more specific meaning?
> >
> > A daemon could sit on top of sgx/provision and send a file descriptor of
> > this file to a process that needs to be able to provision enclaves.
> >
> > The way this API is used is more or less straight-forward. Lets assume that
> > dev_fd is a handle to /dev/sgx and prov_fd is a handle to sgx/provision.
> > You would allow SGX_IOC_ENCLAVE_CREATE to initialize an enclave with the
> > PROVISIONKEY attribute by
> >
> > params.addr = <enclave address>;
> > params.token_fd = prov_fd;
> >
> > ioctl(dev_fd, SGX_IOC_ENCLAVE_SET_ATTRIBUTE, ¶ms);
> >
> > Cc: James Morris <jmorris@namei.org>
> > Cc: Serge E. Hallyn <serge@hallyn.com>
> > Cc: linux-security-module@vger.kernel.org
> > Suggested-by: Andy Lutomirski <luto@kernel.org>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> > arch/x86/include/uapi/asm/sgx.h | 13 +++++++
> > arch/x86/kernel/cpu/sgx/driver/ioctl.c | 43 +++++++++++++++++++++++
> > arch/x86/kernel/cpu/sgx/driver/main.c | 47 ++++++++++++++++++++++++++
> > 3 files changed, 103 insertions(+)
> >
> > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> > index aadf9c76e360..150a784db395 100644
> > --- a/arch/x86/include/uapi/asm/sgx.h
> > +++ b/arch/x86/include/uapi/asm/sgx.h
> > @@ -16,6 +16,8 @@
> > _IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page)
> > #define SGX_IOC_ENCLAVE_INIT \
> > _IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
> > +#define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
> > + _IOW(SGX_MAGIC, 0x03, struct sgx_enclave_set_attribute)
> >
> > /* IOCTL return values */
> > #define SGX_POWER_LOST_ENCLAVE 0x40000000
> > @@ -56,4 +58,15 @@ struct sgx_enclave_init {
> > __u64 sigstruct;
> > };
> >
> > +/**
> > + * struct sgx_enclave_set_attribute - parameter structure for the
> > + * %SGX_IOC_ENCLAVE_INIT ioctl
> > + * @addr: address within the ELRANGE
> > + * @attribute_fd: file handle of the attribute file in the securityfs
> > + */
> > +struct sgx_enclave_set_attribute {
> > + __u64 addr;
> > + __u64 attribute_fd;
> > +};
> > +
> > #endif /* _UAPI_ASM_X86_SGX_H */
> > diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > index 4b9a91b53b50..5d85bd3f7876 100644
> > --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > @@ -759,6 +759,46 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned int cmd,
> > return ret;
> > }
> >
> > +/**
> > + * sgx_ioc_enclave_set_attribute - handler for %SGX_IOC_ENCLAVE_SET_ATTRIBUTE
> > + * @filep: open file to /dev/sgx
> > + * @cmd: the command value
> > + * @arg: pointer to a struct sgx_enclave_set_attribute instance
> > + *
> > + * Sets an attribute matching the attribute file that is pointed by the
> > + * parameter structure field attribute_fd.
>
> With the @data change (see below), this becomes something like:
>
> * Allow the enclave to request the attribute managed by the SGX security file
> * pointed at by the parameter structure field attribute_fd.
>
> > + *
> > + * Return: 0 on success, -errno otherwise
> > + */
> > +static long sgx_ioc_enclave_set_attribute(struct file *filep, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + struct sgx_enclave_set_attribute *params = (void *)arg;
> > + struct file *attribute_file;
> > + struct sgx_encl *encl;
> > + int ret;
> > +
> > + attribute_file = fget(params->attribute_fd);
> > + if (!attribute_file->f_op)
>
> This should be:
>
> if (!attribute_file)
> return -EINVAL;
>
> > + return -EINVAL;
> > +
> > + if (attribute_file->f_op != &sgx_fs_provision_fops) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + ret = sgx_encl_get(params->addr, &encl);
> > + if (ret)
> > + goto out;
> > +
> > + encl->allowed_attributes |= SGX_ATTR_PROVISIONKEY;
>
> A cleanr approach would be to pass SGX_ATTR_PROVISIONKEY via @data to
> securityfs_create_file(). Then you don't need to define dummy file_ops
> for each file, i.e. a generic sgx_sec_fs_ops would suffice for the above
> check. And you don't have this weird hardcoding of the provision bit.
>
> E.g.:
>
> if (attribute_file->f_op != &sgx_sec_fs_fops) {
> ret = -EINVAL;
> goto out;
> }
>
> ret = sgx_encl_get(params->addr, &encl);
> if (ret)
> goto out;
>
> encl->allowed_attributes |= (u64)attribute_file->private_data;
>
> Since SGX doesn't support 32-bit builds we don't even need to worry about
> the (very distant) future where SGX defines bits in the 63:32 range.
>
Agree with Sean that passing SGX_ATTR_PROVISIONKEY via @data is more cleaner.
Thanks,
-Kai
^ permalink raw reply
* Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
From: Paul Moore @ 2019-03-21 1:03 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: linux-integrity, linux-security-module, Linux-Audit Mailing List,
LKML, sgrubb, omosnace, Eric Paris, Serge Hallyn, zohar, mjg59
In-Reply-To: <20190321005008.wfz3bk7q262km5fz@madcap2.tricolour.ca>
On Wed, Mar 20, 2019 at 8:50 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-03-20 19:48, Paul Moore wrote:
> > On Sat, Mar 16, 2019 at 8:10 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > In commit fa516b66a1bf ("EVM: Allow runtime modification of the set of
> > > verified xattrs"), the call to audit_log_start() is missing a context to
> > > link it to an audit event. Since this event is in user context, add
> > > the process' syscall context to the record.
> > >
> > > In addition, the orphaned keyword "locked" appears in the record.
> > > Normalize this by changing it to "xattr=(locked)".
> > >
> > > Please see the github issue
> > > https://github.com/linux-audit/audit-kernel/issues/109
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > > security/integrity/evm/evm_secfs.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
...
> > > @@ -222,7 +223,7 @@ static ssize_t evm_write_xattrs(struct file *file, const char __user *buf,
> > > inode_lock(inode);
> > > err = simple_setattr(evm_xattrs, &newattrs);
> > > inode_unlock(inode);
> > > - audit_log_format(ab, "locked");
> > > + audit_log_format(ab, "xattr=(locked)");
> >
> > Two things come to mind:
> >
> > * While we can clearly trust the string above, should we be logging
> > the xattr field value as an untrusted string so it is consistent with
> > how we record other xattr names?
>
> That would be a question for Steve.
Yep, that's who I wanted to hear from, it's not really something I
expected you to answer Richard. I vaguely remember something about
Steve's audit libs being able to handle both trusted and untrusted
value strings for a given field, but I could have confused "able to
handle" with "don't care".
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
From: Richard Guy Briggs @ 2019-03-21 0:50 UTC (permalink / raw)
To: Paul Moore
Cc: linux-integrity, linux-security-module, Linux-Audit Mailing List,
LKML, sgrubb, omosnace, Eric Paris, Serge Hallyn, zohar, mjg59
In-Reply-To: <CAHC9VhS61LTQMXiALRre+5_+wvwe6Wa9RN-WYNTjvz5LVsA5rw@mail.gmail.com>
On 2019-03-20 19:48, Paul Moore wrote:
> On Sat, Mar 16, 2019 at 8:10 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > In commit fa516b66a1bf ("EVM: Allow runtime modification of the set of
> > verified xattrs"), the call to audit_log_start() is missing a context to
> > link it to an audit event. Since this event is in user context, add
> > the process' syscall context to the record.
> >
> > In addition, the orphaned keyword "locked" appears in the record.
> > Normalize this by changing it to "xattr=(locked)".
> >
> > Please see the github issue
> > https://github.com/linux-audit/audit-kernel/issues/109
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > security/integrity/evm/evm_secfs.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
> > index 015aea8fdf1e..4171d174e9da 100644
> > --- a/security/integrity/evm/evm_secfs.c
> > +++ b/security/integrity/evm/evm_secfs.c
> > @@ -192,7 +192,8 @@ static ssize_t evm_write_xattrs(struct file *file, const char __user *buf,
> > if (count > XATTR_NAME_MAX)
> > return -E2BIG;
> >
> > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_EVM_XATTR);
> > + ab = audit_log_start(audit_context(), GFP_KERNEL,
> > + AUDIT_INTEGRITY_EVM_XATTR);
>
> This part is fine.
>
> > if (!ab)
> > return -ENOMEM;
> >
> > @@ -222,7 +223,7 @@ static ssize_t evm_write_xattrs(struct file *file, const char __user *buf,
> > inode_lock(inode);
> > err = simple_setattr(evm_xattrs, &newattrs);
> > inode_unlock(inode);
> > - audit_log_format(ab, "locked");
> > + audit_log_format(ab, "xattr=(locked)");
>
> Two things come to mind:
>
> * While we can clearly trust the string above, should we be logging
> the xattr field value as an untrusted string so it is consistent with
> how we record other xattr names?
That would be a question for Steve.
> * I'm not sure you can ever have parens in a xattr (I would hope not),
> but if we are going to use the xattr field, perhaps we should simply
> stick with the name as provided (".") so we don't ever run afoul of
> xattr names? I'm curious to hear what the IMA/EVM folks think of
> this.
The legal xaddr names start with XATTR_SECURITY_PREFIX which is
"security." so there is no danger of collision with legal names, but I
suppose someone could try to use "(locked)" as a name which would look
identical but fail with a different res= number. I think I prefer your
idea of printing the given value verbatim.
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
From: Paul Moore @ 2019-03-20 23:48 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: linux-integrity, linux-security-module, Linux-Audit Mailing List,
LKML, sgrubb, omosnace, Eric Paris, Serge Hallyn, zohar, mjg59
In-Reply-To: <81d0122d14c4fbb3a2ad33d25fdf2dd001c7dcc7.1552737854.git.rgb@redhat.com>
On Sat, Mar 16, 2019 at 8:10 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> In commit fa516b66a1bf ("EVM: Allow runtime modification of the set of
> verified xattrs"), the call to audit_log_start() is missing a context to
> link it to an audit event. Since this event is in user context, add
> the process' syscall context to the record.
>
> In addition, the orphaned keyword "locked" appears in the record.
> Normalize this by changing it to "xattr=(locked)".
>
> Please see the github issue
> https://github.com/linux-audit/audit-kernel/issues/109
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> security/integrity/evm/evm_secfs.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
> index 015aea8fdf1e..4171d174e9da 100644
> --- a/security/integrity/evm/evm_secfs.c
> +++ b/security/integrity/evm/evm_secfs.c
> @@ -192,7 +192,8 @@ static ssize_t evm_write_xattrs(struct file *file, const char __user *buf,
> if (count > XATTR_NAME_MAX)
> return -E2BIG;
>
> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_EVM_XATTR);
> + ab = audit_log_start(audit_context(), GFP_KERNEL,
> + AUDIT_INTEGRITY_EVM_XATTR);
This part is fine.
> if (!ab)
> return -ENOMEM;
>
> @@ -222,7 +223,7 @@ static ssize_t evm_write_xattrs(struct file *file, const char __user *buf,
> inode_lock(inode);
> err = simple_setattr(evm_xattrs, &newattrs);
> inode_unlock(inode);
> - audit_log_format(ab, "locked");
> + audit_log_format(ab, "xattr=(locked)");
Two things come to mind:
* While we can clearly trust the string above, should we be logging
the xattr field value as an untrusted string so it is consistent with
how we record other xattr names?
* I'm not sure you can ever have parens in a xattr (I would hope not),
but if we are going to use the xattr field, perhaps we should simply
stick with the name as provided (".") so we don't ever run afoul of
xattr names? I'm curious to hear what the IMA/EVM folks think of
this.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages
From: Eric Biggers @ 2019-03-20 18:57 UTC (permalink / raw)
To: Kees Cook
Cc: Geert Uytterhoeven, Herbert Xu, linux-security-module, Linux ARM,
Linux Crypto Mailing List, Linux Kernel Mailing List
In-Reply-To: <20190319170911.GB202956@gmail.com>
On Tue, Mar 19, 2019 at 10:09:13AM -0700, Eric Biggers wrote:
> On Tue, Mar 19, 2019 at 12:54:23PM +0100, Geert Uytterhoeven wrote:
> > When running the sha1-asm crypto selftest on arm with
> > CONFIG_HARDENED_USERCOPY_PAGESPAN=y:
> >
> > usercopy: Kernel memory overwrite attempt detected to spans
> > multiple pages (offset 0, size 42)!
> > ------------[ cut here ]------------
> > kernel BUG at mm/usercopy.c:102!
> > Internal error: Oops - BUG: 0 [#1] SMP ARM
> > Modules linked in:
> > CPU: 0 PID: 35 Comm: cryptomgr_test Not tainted
> > 5.1.0-rc1-koelsch-01109-gbeb7d6376ecfbf07-dirty #397
> > Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> > PC is at usercopy_abort+0x68/0x90
> > LR is at usercopy_abort+0x68/0x90
> > pc : [<c030fd60>] lr : [<c030fd60>] psr: 60000013
> > sp : ea54bc60 ip : 00000010 fp : cccccccd
> > r10: 00000000 r9 : c0e0ce04 r8 : ea54d009
> > r7 : ea54d00a r6 : 00000000 r5 : 0000002a r4 : c09d1120
> > r3 : dd6cd422 r2 : dd6cd422 r1 : 2abb4000 r0 : 0000005f
> > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
> > Control: 30c5387d Table: 40003000 DAC: fffffffd
> > Process cryptomgr_test (pid: 35, stack limit = 0x(ptrval))
> > Stack: (0xea54bc60 to 0xea54c000)
> > bc60: c09d1120 c09d1120 c09d1120 00000000 0000002a 0000002a
> > 00000000 c0310060
> > bc80: 0000002a 00000000 000001c0 00000000 00000000 c0eb11e8
> > ea54cfe0 ea538c00
> > bca0: 00000000 ea54cfe0 ebef73e0 0000002a ea538c20 ea54bd84
> > 0000003a c0427a30
> > bcc0: ea54bdbc 00000000 00000000 c081cf70 eb074280 c081cf70
> > 0000002a c081cf80
> > bce0: 0000000e c07da138 ea54bd0c 00000000 c084061c c04248e8
> > c0e0a408 eb074240
> > bd00: eb074200 c04253c8 eb074280 ea550000 00000012 dd6cd422
> > ebef7480 eb074200
> > bd20: ea54bd84 c081cf64 ea537200 00000002 00000000 00000014
> > c084061c c0428c38
> > bd40: ea54bd84 ea54bdbc c081cd34 00000000 c0e4e4b4 ea538c40
> > 00000002 eabe4e80
> > bd60: ea538c00 00000400 ea4f7a00 ea4f7a60 eb074240 00000060
> > 00000006 c09d544c
> > bd80: 00000038 00000003 00000000 00000038 ea54bd7c 00000001
> > eb074200 00000000
> > bda0: 00000000 dead4ead ffffffff ffffffff ea54bdb0 ea54bdb0
> > 00000000 c081cf70
> > bdc0: c081ce68 c081ce78 ea4f7480 eb000780 00000dc0 eb000780
> > c0e4ee80 443e9884
> > bde0: 6ed23b1c a14aaeba e52951f9 f17046e5 fefefefe fefefefe
> > fefefefe fefefefe
> > be00: eb000780 c04292c4 c0e0a638 60000013 60000013 c0305298
> > ea4f7a00 c03062bc
> > be20: eb000780 00000cc0 ea4f7a00 dd6cd422 00000cc0 ea538c00
> > 00000002 eabe4e40
> > be40: ea537200 00000007 00000000 ea4f7a00 eb074200 c0429314
> > eb074200 ea538c00
> > be60: ea4f7a00 0000000a eabe4e80 c084061c c08405fc 00000006
> > c04dace8 00000006
> > be80: 00000000 c084065c ea537200 0000000e 00000400 eb04de08
> > ea4f71a8 c0429420
> > bea0: 00000400 ea537200 0000000e ea537200 0000000e c0429374
> > 00000400 ffffffff
> > bec0: 000000a2 c042a414 00000103 c0e0a408 00000000 c0e0a438
> > c0e5a2a0 c0e5a2a0
> > bee0: 00000001 00000001 00000017 ffffe000 00000000 60000013
> > c0e5a2a0 c0269470
> > bf00: c09c9ed0 ea54bf5c 00000103 00000000 00000000 c0e0a408
> > ea537280 0000000e
> > bf20: 00000400 c0426500 00000000 eb04de08 ea4f71a8 c02694f4
> > c09c9ed0 ea54bf5c
> > bf40: ea54bf28 c02699d0 ea54bf5c dd6cd422 ea537200 dd6cd422
> > c09c9ed0 ea537200
> > bf60: ea4af1c0 ea54a000 ea537200 c0426500 00000000 eb04de08
> > ea4f71a8 c0426524
> > bf80: ea4f7180 c023dcec ea54a000 ea4af1c0 c023dbb4 00000000
> > 00000000 00000000
> > bfa0: 00000000 00000000 00000000 c02010d8 00000000 00000000
> > 00000000 00000000
> > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000
> > 00000000 00000000
> > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> > 00000000 00000000
> > [<c030fd60>] (usercopy_abort) from [<c0310060>]
> > (__check_object_size+0x2d8/0x448)
> > [<c0310060>] (__check_object_size) from [<c0427a30>]
> > (build_test_sglist+0x268/0x2d8)
> > [<c0427a30>] (build_test_sglist) from [<c0428c38>]
> > (test_hash_vec_cfg+0x110/0x694)
> > [<c0428c38>] (test_hash_vec_cfg) from [<c0429314>]
> > (__alg_test_hash+0x158/0x1b8)
> > [<c0429314>] (__alg_test_hash) from [<c0429420>] (alg_test_hash+0xac/0xf4)
> > [<c0429420>] (alg_test_hash) from [<c042a414>] (alg_test.part.4+0x264/0x2f8)
> > [<c042a414>] (alg_test.part.4) from [<c0426524>] (cryptomgr_test+0x24/0x44)
> > [<c0426524>] (cryptomgr_test) from [<c023dcec>] (kthread+0x138/0x150)
> > [<c023dcec>] (kthread) from [<c02010d8>] (ret_from_fork+0x14/0x3c)
> > Exception stack(0xea54bfb0 to 0xea54bff8)
> > bfa0: 00000000 00000000
> > 00000000 00000000
> > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000
> > 00000000 00000000
> > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> > Code: e58de000 e98d0012 e1a0100c ebfd6712 (e7f001f2)
> > ---[ end trace 190b3cf48e720f78 ]---
> > BUG: sleeping function called from invalid context at
> > include/linux/percpu-rwsem.h:34
> > in_atomic(): 0, irqs_disabled(): 128, pid: 35, name: cryptomgr_test
> > CPU: 0 PID: 35 Comm: cryptomgr_test Tainted: G D
> > 5.1.0-rc1-koelsch-01109-gbeb7d6376ecfbf07-dirty #397
> > Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> > [<c020ec74>] (unwind_backtrace) from [<c020ae58>] (show_stack+0x10/0x14)
> > [<c020ae58>] (show_stack) from [<c07c3624>] (dump_stack+0x7c/0x9c)
> > [<c07c3624>] (dump_stack) from [<c0242e14>] (___might_sleep+0xf4/0x158)
> > [<c0242e14>] (___might_sleep) from [<c0230210>] (exit_signals+0x2c/0x258)
> > [<c0230210>] (exit_signals) from [<c0223d6c>] (do_exit+0x114/0xa20)
> > [<c0223d6c>] (do_exit) from [<c020b160>] (die+0x304/0x344)
> > [<c020b160>] (die) from [<c020b388>] (do_undefinstr+0x80/0x190)
> > [<c020b388>] (do_undefinstr) from [<c0201b24>] (__und_svc_finish+0x0/0x3c)
> > Exception stack(0xea54bc10 to 0xea54bc58)
> > bc00: 0000005f 2abb4000
> > dd6cd422 dd6cd422
> > bc20: c09d1120 0000002a 00000000 ea54d00a ea54d009 c0e0ce04
> > 00000000 cccccccd
> > bc40: 00000010 ea54bc60 c030fd60 c030fd60 60000013 ffffffff
> > [<c0201b24>] (__und_svc_finish) from [<c030fd60>] (usercopy_abort+0x68/0x90)
> > [<c030fd60>] (usercopy_abort) from [<c0310060>]
> > (__check_object_size+0x2d8/0x448)
> > [<c0310060>] (__check_object_size) from [<c0427a30>]
> > (build_test_sglist+0x268/0x2d8)
> > [<c0427a30>] (build_test_sglist) from [<c0428c38>]
> > (test_hash_vec_cfg+0x110/0x694)
> > [<c0428c38>] (test_hash_vec_cfg) from [<c0429314>]
> > (__alg_test_hash+0x158/0x1b8)
> > [<c0429314>] (__alg_test_hash) from [<c0429420>] (alg_test_hash+0xac/0xf4)
> > [<c0429420>] (alg_test_hash) from [<c042a414>] (alg_test.part.4+0x264/0x2f8)
> > [<c042a414>] (alg_test.part.4) from [<c0426524>] (cryptomgr_test+0x24/0x44)
> > [<c0426524>] (cryptomgr_test) from [<c023dcec>] (kthread+0x138/0x150)
> > [<c023dcec>] (kthread) from [<c02010d8>] (ret_from_fork+0x14/0x3c)
> > Exception stack(0xea54bfb0 to 0xea54bff8)
> > bfa0: 00000000 00000000
> > 00000000 00000000
> > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000
> > 00000000 00000000
> > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> >
>
> Well, this must happen with the new (in 5.1) crypto self-tests implementation
> for any crypto algorithm when CONFIG_HARDENED_USERCOPY_PAGESPAN=y. I don't
> understand why hardened usercopy considers it a bug though, as there's no buffer
> overflow. The crypto tests use copy_from_iter() to copy data into a 2-page
> buffer that was allocated with __get_free_pages():
>
> __get_free_pages(GFP_KERNEL, 1)
>
> ... where 1 means an order-1 allocation.
>
> If it copies to offset=4064 len=42, for example, then hardened usercopy
> considers it a bug even though the buffer is 8192 bytes long. Why?
>
> It isn't actually copying anything to/from userspace, BTW; it's using iov_iter
> with ITER_KVEC.
>
> - Eric
Kees, any thoughts on why hardened usercopy rejects copies spanning a page
boundary when they seem to be fine?
- Eric
^ permalink raw reply
* [PATCH v19,RESEND 17/27] x86/sgx: Add provisioning
From: Jarkko Sakkinen @ 2019-03-20 16:21 UTC (permalink / raw)
To: linux-kernel, x86, linux-sgx
Cc: akpm, dave.hansen, sean.j.christopherson, nhorman, npmccallum,
serge.ayoun, shay.katz-zamir, haitao.huang, andriy.shevchenko,
tglx, kai.svahn, bp, josh, luto, kai.huang, rientjes,
Jarkko Sakkinen, James Morris, Serge E . Hallyn,
linux-security-module
In-Reply-To: <20190320162119.4469-1-jarkko.sakkinen@linux.intel.com>
In order to provide a mechanism for devilering provisoning rights:
1. Add a new file to the securityfs file called sgx/provision that works
as a token for allowing an enclave to have the provisioning privileges.
2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
following data structure:
struct sgx_enclave_set_attribute {
__u64 addr;
__u64 token_fd;
};
A daemon could sit on top of sgx/provision and send a file descriptor of
this file to a process that needs to be able to provision enclaves.
The way this API is used is more or less straight-forward. Lets assume that
dev_fd is a handle to /dev/sgx and prov_fd is a handle to sgx/provision.
You would allow SGX_IOC_ENCLAVE_CREATE to initialize an enclave with the
PROVISIONKEY attribute by
params.addr = <enclave address>;
params.token_fd = prov_fd;
ioctl(dev_fd, SGX_IOC_ENCLAVE_SET_ATTRIBUTE, ¶ms);
Cc: James Morris <jmorris@namei.org>
Cc: Serge E. Hallyn <serge@hallyn.com>
Cc: linux-security-module@vger.kernel.org
Suggested-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
arch/x86/include/uapi/asm/sgx.h | 13 +++++++
arch/x86/kernel/cpu/sgx/driver/ioctl.c | 43 +++++++++++++++++++++++
arch/x86/kernel/cpu/sgx/driver/main.c | 47 ++++++++++++++++++++++++++
3 files changed, 103 insertions(+)
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index aadf9c76e360..150a784db395 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -16,6 +16,8 @@
_IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page)
#define SGX_IOC_ENCLAVE_INIT \
_IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
+#define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
+ _IOW(SGX_MAGIC, 0x03, struct sgx_enclave_set_attribute)
/* IOCTL return values */
#define SGX_POWER_LOST_ENCLAVE 0x40000000
@@ -56,4 +58,15 @@ struct sgx_enclave_init {
__u64 sigstruct;
};
+/**
+ * struct sgx_enclave_set_attribute - parameter structure for the
+ * %SGX_IOC_ENCLAVE_INIT ioctl
+ * @addr: address within the ELRANGE
+ * @attribute_fd: file handle of the attribute file in the securityfs
+ */
+struct sgx_enclave_set_attribute {
+ __u64 addr;
+ __u64 attribute_fd;
+};
+
#endif /* _UAPI_ASM_X86_SGX_H */
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 4b9a91b53b50..5d85bd3f7876 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -759,6 +759,46 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned int cmd,
return ret;
}
+/**
+ * sgx_ioc_enclave_set_attribute - handler for %SGX_IOC_ENCLAVE_SET_ATTRIBUTE
+ * @filep: open file to /dev/sgx
+ * @cmd: the command value
+ * @arg: pointer to a struct sgx_enclave_set_attribute instance
+ *
+ * Sets an attribute matching the attribute file that is pointed by the
+ * parameter structure field attribute_fd.
+ *
+ * Return: 0 on success, -errno otherwise
+ */
+static long sgx_ioc_enclave_set_attribute(struct file *filep, unsigned int cmd,
+ unsigned long arg)
+{
+ struct sgx_enclave_set_attribute *params = (void *)arg;
+ struct file *attribute_file;
+ struct sgx_encl *encl;
+ int ret;
+
+ attribute_file = fget(params->attribute_fd);
+ if (!attribute_file->f_op)
+ return -EINVAL;
+
+ if (attribute_file->f_op != &sgx_fs_provision_fops) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ ret = sgx_encl_get(params->addr, &encl);
+ if (ret)
+ goto out;
+
+ encl->allowed_attributes |= SGX_ATTR_PROVISIONKEY;
+ kref_put(&encl->refcount, sgx_encl_release);
+
+out:
+ fput(attribute_file);
+ return ret;
+}
+
typedef long (*sgx_ioc_t)(struct file *filep, unsigned int cmd,
unsigned long arg);
@@ -778,6 +818,9 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
case SGX_IOC_ENCLAVE_INIT:
handler = sgx_ioc_enclave_init;
break;
+ case SGX_IOC_ENCLAVE_SET_ATTRIBUTE:
+ handler = sgx_ioc_enclave_set_attribute;
+ break;
default:
return -ENOIOCTLCMD;
}
diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
index 16f36cd0af04..9a5360dcad98 100644
--- a/arch/x86/kernel/cpu/sgx/driver/main.c
+++ b/arch/x86/kernel/cpu/sgx/driver/main.c
@@ -22,6 +22,11 @@ u64 sgx_attributes_reserved_mask;
u64 sgx_xfrm_reserved_mask = ~0x3;
u32 sgx_xsave_size_tbl[64];
+const struct file_operations sgx_fs_provision_fops;
+
+static struct dentry *sgx_fs;
+static struct dentry *sgx_fs_provision;
+
#ifdef CONFIG_COMPAT
static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
unsigned long arg)
@@ -147,6 +152,40 @@ static struct sgx_dev_ctx *sgxm_dev_ctx_alloc(struct device *parent)
return ctx;
}
+static int sgx_fs_init(struct device *dev)
+{
+ int ret;
+
+ sgx_fs = securityfs_create_dir(dev_name(dev), NULL);
+ if (IS_ERR(sgx_fs)) {
+ ret = PTR_ERR(sgx_fs);
+ goto err_sgx_fs;
+ }
+
+ sgx_fs_provision = securityfs_create_file("provision", 0600, sgx_fs,
+ NULL, &sgx_fs_provision_fops);
+ if (IS_ERR(sgx_fs)) {
+ ret = PTR_ERR(sgx_fs_provision);
+ goto err_sgx_fs_provision;
+ }
+
+ return 0;
+
+err_sgx_fs_provision:
+ securityfs_remove(sgx_fs);
+ sgx_fs_provision = NULL;
+
+err_sgx_fs:
+ sgx_fs = NULL;
+ return ret;
+}
+
+static void sgx_fs_remove(void)
+{
+ securityfs_remove(sgx_fs_provision);
+ securityfs_remove(sgx_fs);
+}
+
static int sgx_dev_init(struct device *parent)
{
struct sgx_dev_ctx *sgx_dev;
@@ -190,6 +229,10 @@ static int sgx_dev_init(struct device *parent)
if (!sgx_encl_wq)
return -ENOMEM;
+ ret = sgx_fs_init(&sgx_dev->ctrl_dev);
+ if (ret)
+ goto err_fs_init;
+
ret = cdev_device_add(&sgx_dev->ctrl_cdev, &sgx_dev->ctrl_dev);
if (ret)
goto err_device_add;
@@ -197,6 +240,9 @@ static int sgx_dev_init(struct device *parent)
return 0;
err_device_add:
+ sgx_fs_remove();
+
+err_fs_init:
destroy_workqueue(sgx_encl_wq);
return ret;
}
@@ -220,6 +266,7 @@ static int sgx_drv_remove(struct platform_device *pdev)
{
struct sgx_dev_ctx *ctx = dev_get_drvdata(&pdev->dev);
+ sgx_fs_remove();
cdev_device_del(&ctx->ctrl_cdev, &ctx->ctrl_dev);
destroy_workqueue(sgx_encl_wq);
--
2.19.1
^ permalink raw reply related
* Re: [PATCH v2 1/2] initmem: introduce CONFIG_INIT_ALL_MEMORY and CONFIG_INIT_ALL_STACK
From: Alexander Potapenko @ 2019-03-20 14:44 UTC (permalink / raw)
To: Masahiro Yamada, James Morris, Serge E. Hallyn
Cc: linux-security-module, Linux Kbuild mailing list,
Nick Desaulniers, Kostya Serebryany, Dmitriy Vyukov, Kees Cook,
Sandeep Patil, Kernel Hardening
In-Reply-To: <20190308132701.133598-2-glider@google.com>
On Fri, Mar 8, 2019 at 2:27 PM Alexander Potapenko <glider@google.com> wrote:
>
> CONFIG_INIT_ALL_MEMORY is going to be an umbrella config for options
> that force heap and stack initialization.
> The rationale behind doing so is to reduce the severity of bugs caused
> by using uninitialized memory.
>
> CONFIG_INIT_ALL_STACK turns on stack initialization based on
> -ftrivial-auto-var-init in Clang builds and on
> -fplugin-arg-structleak_plugin-byref-all in GCC builds.
>
> -ftrivial-auto-var-init is a Clang flag that 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) 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.
>
> The proposed config builds the kernel with
> -ftrivial-auto-var-init=pattern.
>
> Developers have the possibility to opt-out of this feature on a
> per-file (by using the INIT_ALL_MEMORY_ Makefile prefix) or per-variable
> (by using __attribute__((uninitialized))) basis.
>
> For GCC builds, CONFIG_INIT_ALL_STACK is simply wired up to
> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL. No opt-out is possible at the
> moment.
>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Kostya Serebryany <kcc@google.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Sandeep Patil <sspatil@android.com>
> Cc: linux-security-module@vger.kernel.org
> Cc: linux-kbuild@vger.kernel.org
> Cc: kernel-hardening@lists.openwall.com
> ---
> v2:
> - addressed Kees Cook's comments: added GCC support
A friendly ping.
Please let me know if I should wait for any upcoming changes to
CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL.
> ---
> Makefile | 3 ++-
> scripts/Makefile.initmem | 10 ++++++++++
> scripts/Makefile.lib | 6 ++++++
> security/Kconfig | 1 +
> security/Kconfig.initmem | 29 +++++++++++++++++++++++++++++
> 5 files changed, 48 insertions(+), 1 deletion(-)
> create mode 100644 scripts/Makefile.initmem
> create mode 100644 security/Kconfig.initmem
>
> diff --git a/Makefile b/Makefile
> index f070e0d65186..028ca37878fd 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -448,7 +448,7 @@ export HOSTCXX KBUILD_HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
>
> export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS KBUILD_LDFLAGS
> export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE
> -export CFLAGS_KASAN CFLAGS_KASAN_NOSANITIZE CFLAGS_UBSAN
> +export CFLAGS_KASAN CFLAGS_KASAN_NOSANITIZE CFLAGS_UBSAN CFLAGS_INITMEM
> export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE
> export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_LDFLAGS_MODULE
> export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL
> @@ -840,6 +840,7 @@ KBUILD_ARFLAGS := $(call ar-option,D)
> include scripts/Makefile.kasan
> include scripts/Makefile.extrawarn
> include scripts/Makefile.ubsan
> +include scripts/Makefile.initmem
>
> # Add any arch overrides and user supplied CPPFLAGS, AFLAGS and CFLAGS as the
> # last assignments
> diff --git a/scripts/Makefile.initmem b/scripts/Makefile.initmem
> new file mode 100644
> index 000000000000..a6253d78fe35
> --- /dev/null
> +++ b/scripts/Makefile.initmem
> @@ -0,0 +1,10 @@
> +ifdef CONFIG_INIT_ALL_STACK
> +
> +# Clang's -ftrivial-auto-var-init=pattern flag initializes the
> +# uninitialized parts of local variables (including fields and padding)
> +# with a fixed pattern (0xAA in most cases).
> +ifdef CONFIG_CC_HAS_AUTO_VAR_INIT
> + CFLAGS_INITMEM := -ftrivial-auto-var-init=pattern
> +endif
> +
> +endif
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 12b88d09c3a4..53d18fd15c79 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -131,6 +131,12 @@ _c_flags += $(if $(patsubst n%,, \
> $(CFLAGS_UBSAN))
> endif
>
> +ifeq ($(CONFIG_INIT_ALL_MEMORY),y)
> +_c_flags += $(if $(patsubst n%,, \
> + $(INIT_ALL_MEMORY_$(basetarget).o)$(INIT_ALL_MEMORY)y), \
> + $(CFLAGS_INITMEM))
> +endif
> +
> ifeq ($(CONFIG_KCOV),y)
> _c_flags += $(if $(patsubst n%,, \
> $(KCOV_INSTRUMENT_$(basetarget).o)$(KCOV_INSTRUMENT)$(CONFIG_KCOV_INSTRUMENT_ALL)), \
> diff --git a/security/Kconfig b/security/Kconfig
> index e4fe2f3c2c65..cc12a39424dd 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -230,6 +230,7 @@ config STATIC_USERMODEHELPER_PATH
> If you wish for all usermode helper programs to be disabled,
> specify an empty string here (i.e. "").
>
> +source "security/Kconfig.initmem"
> source "security/selinux/Kconfig"
> source "security/smack/Kconfig"
> source "security/tomoyo/Kconfig"
> diff --git a/security/Kconfig.initmem b/security/Kconfig.initmem
> new file mode 100644
> index 000000000000..27aec394365e
> --- /dev/null
> +++ b/security/Kconfig.initmem
> @@ -0,0 +1,29 @@
> +menu "Initialize all memory"
> +
> +config CC_HAS_AUTO_VAR_INIT
> + def_bool $(cc-option,-ftrivial-auto-var-init=pattern)
> +
> +config INIT_ALL_MEMORY
> + bool "Initialize all memory"
> + default n
> + help
> + Enforce memory initialization to mitigate infoleaks and make
> + the control-flow bugs depending on uninitialized values more
> + deterministic.
> +
> +if INIT_ALL_MEMORY
> +
> +config INIT_ALL_STACK
> + bool "Initialize all stack"
> + depends on INIT_ALL_MEMORY
> + depends on CC_HAS_AUTO_VAR_INIT || HAVE_GCC_PLUGINS
> + select GCC_PLUGINS if !CC_HAS_AUTO_VAR_INIT
> + select GCC_PLUGIN_STRUCTLEAK if !CC_HAS_AUTO_VAR_INIT
> + select GCC_PLUGIN_STRUCTLEAK_BYREF_ALL if !CC_HAS_AUTO_VAR_INIT
> + default y
> + help
> + Initialize uninitialized stack data with a fixed pattern
> + (0x00 in GCC, 0xAA in Clang).
> +
> +endif # INIT_ALL_MEMORY
> +endmenu
> --
> 2.21.0.360.g471c308f928-goog
>
--
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 09/27] hibernate: Disable when the kernel is locked down
From: Pavel Machek @ 2019-03-19 22:15 UTC (permalink / raw)
To: Matthew Garrett; +Cc: jmorris, linux-security-module, linux-kernel, dhowells
In-Reply-To: <20190228231203.212359-9-matthewgarrett@google.com>
[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]
On Thu 2019-02-28 15:11:45, Matthew Garrett wrote:
> From: Josh Boyer <jwboyer@fedoraproject.org>
>
> There is currently no way to verify the resume image when returning
> from hibernate. This might compromise the signed modules trust model,
> so until we can work with signed hibernate images we disable it when the
> kernel is locked down.
>
> Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Reviewed-by: "Lee, Chun-Yi" <jlee@suse.com>
> cc: linux-pm@vger.kernel.org
It would be good to cc hibernation maintainers here.
> ---
> kernel/power/hibernate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index abef759de7c8..802795becb88 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -70,7 +70,7 @@ static const struct platform_hibernation_ops *hibernation_ops;
>
> bool hibernation_available(void)
> {
> - return (nohibernate == 0);
> + return nohibernate == 0 && !kernel_is_locked_down("Hibernation");
> }
>
> /**
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v19 17/27] x86/sgx: Add provisioning
From: Sean Christopherson @ 2019-03-19 20:09 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: x86, linux-sgx, akpm, dave.hansen, nhorman, npmccallum,
serge.ayoun, shay.katz-zamir, haitao.huang, andriy.shevchenko,
tglx, kai.svahn, bp, josh, luto, kai.huang, rientjes,
James Morris, Serge E . Hallyn, linux-security-module
In-Reply-To: <20190317211456.13927-18-jarkko.sakkinen@linux.intel.com>
On Sun, Mar 17, 2019 at 11:14:46PM +0200, Jarkko Sakkinen wrote:
> In order to provide a mechanism for devilering provisoning rights:
>
> 1. Add a new file to the securityfs file called sgx/provision that works
> as a token for allowing an enclave to have the provisioning privileges.
> 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
> following data structure:
>
> struct sgx_enclave_set_attribute {
> __u64 addr;
> __u64 token_fd;
> };
>
> A daemon could sit on top of sgx/provision and send a file descriptor of
> this file to a process that needs to be able to provision enclaves.
>
> The way this API is used is more or less straight-forward. Lets assume that
> dev_fd is a handle to /dev/sgx and prov_fd is a handle to sgx/provision.
> You would allow SGX_IOC_ENCLAVE_CREATE to initialize an enclave with the
> PROVISIONKEY attribute by
>
> params.addr = <enclave address>;
> params.token_fd = prov_fd;
>
> ioctl(dev_fd, SGX_IOC_ENCLAVE_SET_ATTRIBUTE, ¶ms);
>
> Cc: James Morris <jmorris@namei.org>
> Cc: Serge E. Hallyn <serge@hallyn.com>
> Cc: linux-security-module@vger.kernel.org
> Suggested-by: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
> arch/x86/include/uapi/asm/sgx.h | 13 +++++++
> arch/x86/kernel/cpu/sgx/driver/ioctl.c | 43 +++++++++++++++++++++++
> arch/x86/kernel/cpu/sgx/driver/main.c | 47 ++++++++++++++++++++++++++
> 3 files changed, 103 insertions(+)
>
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index aadf9c76e360..150a784db395 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -16,6 +16,8 @@
> _IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page)
> #define SGX_IOC_ENCLAVE_INIT \
> _IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
> +#define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
> + _IOW(SGX_MAGIC, 0x03, struct sgx_enclave_set_attribute)
>
> /* IOCTL return values */
> #define SGX_POWER_LOST_ENCLAVE 0x40000000
> @@ -56,4 +58,15 @@ struct sgx_enclave_init {
> __u64 sigstruct;
> };
>
> +/**
> + * struct sgx_enclave_set_attribute - parameter structure for the
> + * %SGX_IOC_ENCLAVE_INIT ioctl
> + * @addr: address within the ELRANGE
> + * @attribute_fd: file handle of the attribute file in the securityfs
> + */
> +struct sgx_enclave_set_attribute {
> + __u64 addr;
> + __u64 attribute_fd;
> +};
> +
> #endif /* _UAPI_ASM_X86_SGX_H */
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index 4b9a91b53b50..5d85bd3f7876 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -759,6 +759,46 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned int cmd,
> return ret;
> }
>
> +/**
> + * sgx_ioc_enclave_set_attribute - handler for %SGX_IOC_ENCLAVE_SET_ATTRIBUTE
> + * @filep: open file to /dev/sgx
> + * @cmd: the command value
> + * @arg: pointer to a struct sgx_enclave_set_attribute instance
> + *
> + * Sets an attribute matching the attribute file that is pointed by the
> + * parameter structure field attribute_fd.
With the @data change (see below), this becomes something like:
* Allow the enclave to request the attribute managed by the SGX security file
* pointed at by the parameter structure field attribute_fd.
> + *
> + * Return: 0 on success, -errno otherwise
> + */
> +static long sgx_ioc_enclave_set_attribute(struct file *filep, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct sgx_enclave_set_attribute *params = (void *)arg;
> + struct file *attribute_file;
> + struct sgx_encl *encl;
> + int ret;
> +
> + attribute_file = fget(params->attribute_fd);
> + if (!attribute_file->f_op)
This should be:
if (!attribute_file)
return -EINVAL;
> + return -EINVAL;
> +
> + if (attribute_file->f_op != &sgx_fs_provision_fops) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = sgx_encl_get(params->addr, &encl);
> + if (ret)
> + goto out;
> +
> + encl->allowed_attributes |= SGX_ATTR_PROVISIONKEY;
A cleanr approach would be to pass SGX_ATTR_PROVISIONKEY via @data to
securityfs_create_file(). Then you don't need to define dummy file_ops
for each file, i.e. a generic sgx_sec_fs_ops would suffice for the above
check. And you don't have this weird hardcoding of the provision bit.
E.g.:
if (attribute_file->f_op != &sgx_sec_fs_fops) {
ret = -EINVAL;
goto out;
}
ret = sgx_encl_get(params->addr, &encl);
if (ret)
goto out;
encl->allowed_attributes |= (u64)attribute_file->private_data;
Since SGX doesn't support 32-bit builds we don't even need to worry about
the (very distant) future where SGX defines bits in the 63:32 range.
> + kref_put(&encl->refcount, sgx_encl_release);
> +
> +out:
> + fput(attribute_file);
> + return ret;
> +}
> +
> typedef long (*sgx_ioc_t)(struct file *filep, unsigned int cmd,
> unsigned long arg);
>
> @@ -778,6 +818,9 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> case SGX_IOC_ENCLAVE_INIT:
> handler = sgx_ioc_enclave_init;
> break;
> + case SGX_IOC_ENCLAVE_SET_ATTRIBUTE:
> + handler = sgx_ioc_enclave_set_attribute;
> + break;
> default:
> return -ENOIOCTLCMD;
> }
> diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
> index 16f36cd0af04..9a5360dcad98 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/main.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/main.c
> @@ -22,6 +22,11 @@ u64 sgx_attributes_reserved_mask;
> u64 sgx_xfrm_reserved_mask = ~0x3;
> u32 sgx_xsave_size_tbl[64];
>
> +const struct file_operations sgx_fs_provision_fops;
> +
> +static struct dentry *sgx_fs;
> +static struct dentry *sgx_fs_provision;
> +
> #ifdef CONFIG_COMPAT
> static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
> unsigned long arg)
> @@ -147,6 +152,40 @@ static struct sgx_dev_ctx *sgxm_dev_ctx_alloc(struct device *parent)
> return ctx;
> }
>
> +static int sgx_fs_init(struct device *dev)
> +{
> + int ret;
> +
> + sgx_fs = securityfs_create_dir(dev_name(dev), NULL);
> + if (IS_ERR(sgx_fs)) {
> + ret = PTR_ERR(sgx_fs);
> + goto err_sgx_fs;
> + }
> +
> + sgx_fs_provision = securityfs_create_file("provision", 0600, sgx_fs,
> + NULL, &sgx_fs_provision_fops);
Per above, pass SGX_ATTR_PROVISIONKEY instead of NULL.
> + if (IS_ERR(sgx_fs)) {
> + ret = PTR_ERR(sgx_fs_provision);
> + goto err_sgx_fs_provision;
> + }
> +
> + return 0;
> +
> +err_sgx_fs_provision:
> + securityfs_remove(sgx_fs);
> + sgx_fs_provision = NULL;
> +
> +err_sgx_fs:
> + sgx_fs = NULL;
> + return ret;
> +}
> +
> +static void sgx_fs_remove(void)
> +{
> + securityfs_remove(sgx_fs_provision);
> + securityfs_remove(sgx_fs);
> +}
> +
> static int sgx_dev_init(struct device *parent)
> {
> struct sgx_dev_ctx *sgx_dev;
> @@ -190,6 +229,10 @@ static int sgx_dev_init(struct device *parent)
> if (!sgx_encl_wq)
> return -ENOMEM;
>
> + ret = sgx_fs_init(&sgx_dev->ctrl_dev);
> + if (ret)
> + goto err_fs_init;
> +
> ret = cdev_device_add(&sgx_dev->ctrl_cdev, &sgx_dev->ctrl_dev);
> if (ret)
> goto err_device_add;
> @@ -197,6 +240,9 @@ static int sgx_dev_init(struct device *parent)
> return 0;
>
> err_device_add:
> + sgx_fs_remove();
> +
> +err_fs_init:
> destroy_workqueue(sgx_encl_wq);
> return ret;
> }
> @@ -220,6 +266,7 @@ static int sgx_drv_remove(struct platform_device *pdev)
> {
> struct sgx_dev_ctx *ctx = dev_get_drvdata(&pdev->dev);
>
> + sgx_fs_remove();
> cdev_device_del(&ctx->ctrl_cdev, &ctx->ctrl_dev);
> destroy_workqueue(sgx_encl_wq);
>
> --
> 2.19.1
>
^ 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