* Re: [RFC PATCH v2 4/5] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
From: Jarkko Sakkinen @ 2019-06-10 16:05 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <20190606021145.12604-5-sean.j.christopherson@intel.com>
On Wed, Jun 05, 2019 at 07:11:44PM -0700, Sean Christopherson wrote:
> enclave_load() is roughly analogous to the existing file_mprotect().
>
> Due to the nature of SGX and its Enclave Page Cache (EPC), all enclave
> VMAs are backed by a single file, i.e. /dev/sgx/enclave, that must be
> MAP_SHARED. Furthermore, all enclaves need read, write and execute
> VMAs. As a result, the existing/standard call to file_mprotect() does
> not provide any meaningful security for enclaves since an LSM can only
> deny/grant access to the EPC as a whole.
>
> security_enclave_load() is called when SGX is first loading an enclave
> page, i.e. copying a page from normal memory into the EPC. Although
> the prototype for enclave_load() is similar to file_mprotect(), e.g.
> SGX could theoretically use file_mprotect() and set reqprot=prot, a
> separate hook is desirable as the semantics of an enclave's protection
> bits are different than those of vmas, e.g. an enclave page tracks the
> maximal set of protections, whereas file_mprotect() operates on the
> actual protections being provided. In other words, LSMs will likely
> want to implement different policies for enclave page protections.
>
> Note, extensive discussion yielded no sane alternative to some form of
> SGX specific LSM hook[1].
>
> [1] https://lkml.kernel.org/r/CALCETrXf8mSK45h7sTK5Wf+pXLVn=Bjsc_RLpgO-h-qdzBRo5Q@mail.gmail.com
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
4/5 and 5/5 should only be added after upstreaming SGX.
/Jarkko
^ permalink raw reply
* Re: [RFC PATCH v2 3/5] x86/sgx: Enforce noexec filesystem restriction for enclaves
From: Jarkko Sakkinen @ 2019-06-10 16:00 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <20190606021145.12604-4-sean.j.christopherson@intel.com>
On Wed, Jun 05, 2019 at 07:11:43PM -0700, Sean Christopherson wrote:
> + goto out;
> + }
> +
> + /*
> + * Query VM_MAYEXEC as an indirect path_noexec() check (see do_mmap()),
> + * but with some future proofing against other cases that may deny
> + * execute permissions.
> + */
> + if (!(vma->vm_flags & VM_MAYEXEC)) {
> + ret = -EACCES;
> + goto out;
> + }
> +
> + if (copy_from_user(dst, (void __user *)src, PAGE_SIZE))
> + ret = -EFAULT;
> + else
> + ret = 0;
> +
> +out:
> + up_read(¤t->mm->mmap_sem);
> +
> + return ret;
> +}
I would suggest to express the above instead like this for clarity
and consistency:
goto err_map_sem;
}
/* Query VM_MAYEXEC as an indirect path_noexec() check
* (see do_mmap()).
*/
if (!(vma->vm_flags & VM_MAYEXEC)) {
ret = -EACCES;
goto err_mmap_sem;
}
if (copy_from_user(dst, (void __user *)src, PAGE_SIZE)) {
ret = -EFAULT;
goto err_mmap_sem;
}
return 0;
err_mmap_sem:
up_read(¤t->mm->mmap_sem);
return ret;
}
The comment about future proofing is unnecessary.
/Jarkk
^ permalink raw reply
* Re: [RFC PATCH v2 1/5] mm: Introduce vm_ops->may_mprotect()
From: Sean Christopherson @ 2019-06-10 15:55 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Andy Lutomirski, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <20190610150600.GA3752@linux.intel.com>
On Mon, Jun 10, 2019 at 06:06:00PM +0300, Jarkko Sakkinen wrote:
> On Wed, Jun 05, 2019 at 07:11:41PM -0700, Sean Christopherson wrote:
> > SGX will use the may_mprotect() hook to prevent userspace from
> > circumventing various security checks, e.g. Linux Security Modules.
> > Naming it may_mprotect() instead of simply mprotect() is intended to
> > reflect the hook's purpose as a way to gate mprotect() as opposed to
> > a wholesale replacement.
>
> "This commit adds may_mprotect() to struct vm_operations_struct, which
> can be used to ask from the owner of a VMA if mprotect() is allowed."
>
> This would be more appropriate statement because that is what the code
> change aims for precisely. I did not even understand what you meant by
> gating in this context. I would leave SGX and LSM's (and especially
> "various security checks", which means abssolutely nothing) out of the
> first paragraph completely.
>
> > Enclaves are built by copying data from normal memory into the Enclave
> > Page Cache (EPC). Due to the nature of SGX, the EPC is represented by a
> > single file that must be MAP_SHARED, i.e. mprotect() only ever sees a
> > MAP_SHARED vm_file that references single file path. Furthermore, all
> > enclaves will need read, write and execute pages in the EPC.
>
> I would just say that "Due to the fact that EPC is delivered as IO
> memory from the preboot firmware, it can be only mapped as MAP_SHARED".
> It is what it is.
I was trying to convey that the nature of SGX itself requires that an
enclave's pages are shared between process. E.g. {MAP,VM}_SHARED would be
required even if we modified the mmu to handle EPC memory in such a way
that it didn't have to be tagged with VM_PFNMAP.
> > As a result, LSM policies cannot be meaningfully applied, e.g. an LSM
> > can deny access to the EPC as a whole, but can't deny PROT_EXEC on page
> > that originated in a non-EXECUTE file (which is long gone by the time
> > mprotect() is called).
>
> I have hard time following what is paragraph is trying to say.
>
> > By hooking mprotect(), SGX can make explicit LSM upcalls while an
> > enclave is being built, i.e. when the kernel has a handle to origin of
> > each enclave page, and enforce the result of the LSM policy whenever
> > userspace maps the enclave page in the future.
>
> "LSM policy whenever calls mprotect()"? I'm no sure why you mean by
> mapping here and if there is any need to talk about future. Isn't this
> needed now?
Future is referring to the timeline of a running kernel, not the future
of the kernel code.
Rather than trying to explain all of the above with words, I'll provide
code examples to show how ->may_protect() will be used by SGX and why it
is the preferred solution.
> > Alternatively, SGX could play games with MAY_{READ,WRITE,EXEC}, but
> > that approach is quite ugly, e.g. would require userspace to call an
> > SGX ioctl() prior to using mprotect() to extend a page's protections.
>
> Instead of talking "playing games" I would state what could be done with
> VM_MAY{READ,WRITE,EXEC} and why it is bad. Leaves questions otherwise.
>
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> > include/linux/mm.h | 2 ++
> > mm/mprotect.c | 15 +++++++++++----
> > 2 files changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 0e8834ac32b7..a697996040ac 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -458,6 +458,8 @@ struct vm_operations_struct {
> > void (*close)(struct vm_area_struct * area);
> > int (*split)(struct vm_area_struct * area, unsigned long addr);
> > int (*mremap)(struct vm_area_struct * area);
> > + int (*may_mprotect)(struct vm_area_struct * area, unsigned long start,
> > + unsigned long end, unsigned long prot);
>
> Could be just boolean.
>
> /Jarkko
^ permalink raw reply
* Re: [RFC PATCH v2 2/5] x86/sgx: Require userspace to define enclave pages' protection bits
From: Jarkko Sakkinen @ 2019-06-10 15:27 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <20190606021145.12604-3-sean.j.christopherson@intel.com>
On Wed, Jun 05, 2019 at 07:11:42PM -0700, Sean Christopherson wrote:
> [SNAP]
Same general criticism as for the previous patch: try to say things as
they are without anything extra.
> A third alternative would be to pull the protection bits from the page's
> SECINFO, i.e. make decisions based on the protections enforced by
> hardware. However, with SGX2, userspace can extend the hardware-
> enforced protections via ENCLU[EMODPE], e.g. can add a page as RW and
> later convert it to RX. With SGX2, making a decision based on the
> initial protections would either create a security hole or force SGX to
> dynamically track "dirty" pages (see first alternative above).
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
'flags' should would renamed as 'secinfo_flags_mask' even if the name is
longish. It would use the same values as the SECINFO flags. The field in
struct sgx_encl_page should have the same name. That would express
exactly relation between SECINFO and the new field. I would have never
asked on last iteration why SECINFO is not enough with a better naming.
The same field can be also used to cage page type to a subset of values.
/Jarkko
^ permalink raw reply
* Re: [RFC][PATCH 00/13] Mount, FS, Block and Keyrings notifications [ver #4]
From: Stephen Smalley @ 2019-06-10 15:21 UTC (permalink / raw)
To: David Howells, viro
Cc: linux-usb, linux-security-module, Casey Schaufler,
Greg Kroah-Hartman, raven, linux-fsdevel, linux-api, linux-block,
keyrings, linux-kernel, Paul Moore
In-Reply-To: <155991702981.15579.6007568669839441045.stgit@warthog.procyon.org.uk>
On 6/7/19 10:17 AM, David Howells wrote:
>
> Hi Al,
>
> Here's a set of patches to add a general variable-length notification queue
> concept and to add sources of events for:
>
> (1) Mount topology events, such as mounting, unmounting, mount expiry,
> mount reconfiguration.
>
> (2) Superblock events, such as R/W<->R/O changes, quota overrun and I/O
> errors (not complete yet).
>
> (3) Key/keyring events, such as creating, linking and removal of keys.
>
> (4) General device events (single common queue) including:
>
> - Block layer events, such as device errors
>
> - USB subsystem events, such as device/bus attach/remove, device
> reset, device errors.
>
> One of the reasons for this is so that we can remove the issue of processes
> having to repeatedly and regularly scan /proc/mounts, which has proven to
> be a system performance problem. To further aid this, the fsinfo() syscall
> on which this patch series depends, provides a way to access superblock and
> mount information in binary form without the need to parse /proc/mounts.
>
>
> LSM support is included, but controversial:
>
> (1) The creds of the process that did the fput() that reduced the refcount
> to zero are cached in the file struct.
>
> (2) __fput() overrides the current creds with the creds from (1) whilst
> doing the cleanup, thereby making sure that the creds seen by the
> destruction notification generated by mntput() appears to come from
> the last fputter.
>
> (3) security_post_notification() is called for each queue that we might
> want to post a notification into, thereby allowing the LSM to prevent
> covert communications.
>
> (?) Do I need to add security_set_watch(), say, to rule on whether a watch
> may be set in the first place? I might need to add a variant per
> watch-type.
>
> (?) Do I really need to keep track of the process creds in which an
> implicit object destruction happened? For example, imagine you create
> an fd with fsopen()/fsmount(). It is marked to dissolve the mount it
> refers to on close unless move_mount() clears that flag. Now, imagine
> someone looking at that fd through procfs at the same time as you exit
> due to an error. The LSM sees the destruction notification come from
> the looker if they happen to do their fput() after yours.
I remain unconvinced that (1), (2), (3), and the final (?) above are a
good idea.
For SELinux, I would expect that one would implement a collection of per
watch-type WATCH permission checks on the target object (or to some
well-defined object label like the kernel SID if there is no object)
that allow receipt of all notifications of that watch-type for objects
related to the target object, where "related to" is defined per watch-type.
I wouldn't expect SELinux to implement security_post_notification() at
all. I can't see how one can construct a meaningful, stable policy for
it. I'd argue that the triggering process is not posting the
notification; the kernel is posting the notification and the watcher has
been authorized to receive it.
>
>
> Design decisions:
>
> (1) A misc chardev is used to create and open a ring buffer:
>
> fd = open("/dev/watch_queue", O_RDWR);
>
> which is then configured and mmap'd into userspace:
>
> ioctl(fd, IOC_WATCH_QUEUE_SET_SIZE, BUF_SIZE);
> ioctl(fd, IOC_WATCH_QUEUE_SET_FILTER, &filter);
> buf = mmap(NULL, BUF_SIZE * page_size, PROT_READ | PROT_WRITE,
> MAP_SHARED, fd, 0);
>
> The fd cannot be read or written (though there is a facility to use
> write to inject records for debugging) and userspace just pulls data
> directly out of the buffer.
>
> (2) The ring index pointers are stored inside the ring and are thus
> accessible to userspace. Userspace should only update the tail
> pointer and never the head pointer or risk breaking the buffer. The
> kernel checks that the pointers appear valid before trying to use
> them. A 'skip' record is maintained around the pointers.
>
> (3) poll() can be used to wait for data to appear in the buffer.
>
> (4) Records in the buffer are binary, typed and have a length so that they
> can be of varying size.
>
> This means that multiple heterogeneous sources can share a common
> buffer. Tags may be specified when a watchpoint is created to help
> distinguish the sources.
>
> (5) The queue is reusable as there are 16 million types available, of
> which I've used 4, so there is scope for others to be used.
>
> (6) Records are filterable as types have up to 256 subtypes that can be
> individually filtered. Other filtration is also available.
>
> (7) Each time the buffer is opened, a new buffer is created - this means
> that there's no interference between watchers.
>
> (8) When recording a notification, the kernel will not sleep, but will
> rather mark a queue as overrun if there's insufficient space, thereby
> avoiding userspace causing the kernel to hang.
>
> (9) The 'watchpoint' should be specific where possible, meaning that you
> specify the object that you want to watch.
>
> (10) The buffer is created and then watchpoints are attached to it, using
> one of:
>
> keyctl_watch_key(KEY_SPEC_SESSION_KEYRING, fd, 0x01);
> mount_notify(AT_FDCWD, "/", 0, fd, 0x02);
> sb_notify(AT_FDCWD, "/mnt", 0, fd, 0x03);
>
> where in all three cases, fd indicates the queue and the number after
> is a tag between 0 and 255.
>
> (11) The watch must be removed if either the watch buffer is destroyed or
> the watched object is destroyed.
>
>
> Things I want to avoid:
>
> (1) Introducing features that make the core VFS dependent on the network
> stack or networking namespaces (ie. usage of netlink).
>
> (2) Dumping all this stuff into dmesg and having a daemon that sits there
> parsing the output and distributing it as this then puts the
> responsibility for security into userspace and makes handling
> namespaces tricky. Further, dmesg might not exist or might be
> inaccessible inside a container.
>
> (3) Letting users see events they shouldn't be able to see.
>
>
> Further things that could be considered:
>
> (1) Adding a keyctl call to allow a watch on a keyring to be extended to
> "children" of that keyring, such that the watch is removed from the
> child if it is unlinked from the keyring.
>
> (2) Adding global superblock event queue.
>
> (3) Propagating watches to child superblock over automounts.
>
>
> The patches can be found here also:
>
> http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=notifications
>
> Changes:
>
> v4: Split the basic UAPI bits out into their own patch and then split the
> LSM hooks out into an intermediate patch. Add LSM hooks for setting
> watches.
>
> Rename the *_notify() system calls to watch_*() for consistency.
>
> v3: I've added a USB notification source and reformulated the block
> notification source so that there's now a common watch list, for which
> the system call is now device_notify().
>
> I've assigned a pair of unused ioctl numbers in the 'W' series to the
> ioctls added by this series.
>
> I've also added a description of the kernel API to the documentation.
>
> v2: I've fixed various issues raised by Jann Horn and GregKH and moved to
> krefs for refcounting. I've added some security features to try and
> give Casey Schaufler the LSM control he wants.
>
> David
> ---
> David Howells (13):
> security: Override creds in __fput() with last fputter's creds
> uapi: General notification ring definitions
> security: Add hooks to rule on setting a watch
> security: Add a hook for the point of notification insertion
> General notification queue with user mmap()'able ring buffer
> keys: Add a notification facility
> vfs: Add a mount-notification facility
> vfs: Add superblock notifications
> fsinfo: Export superblock notification counter
> Add a general, global device notification watch list
> block: Add block layer notifications
> usb: Add USB subsystem notifications
> Add sample notification program
>
>
> Documentation/ioctl/ioctl-number.txt | 1
> Documentation/security/keys/core.rst | 58 ++
> Documentation/watch_queue.rst | 492 ++++++++++++++++++
> arch/x86/entry/syscalls/syscall_32.tbl | 3
> arch/x86/entry/syscalls/syscall_64.tbl | 3
> block/Kconfig | 9
> block/blk-core.c | 29 +
> drivers/base/Kconfig | 9
> drivers/base/Makefile | 1
> drivers/base/watch.c | 89 +++
> drivers/misc/Kconfig | 13
> drivers/misc/Makefile | 1
> drivers/misc/watch_queue.c | 889 ++++++++++++++++++++++++++++++++
> drivers/usb/core/Kconfig | 10
> drivers/usb/core/devio.c | 55 ++
> drivers/usb/core/hub.c | 3
> fs/Kconfig | 21 +
> fs/Makefile | 1
> fs/file_table.c | 12
> fs/fsinfo.c | 12
> fs/mount.h | 33 +
> fs/mount_notify.c | 187 +++++++
> fs/namespace.c | 9
> fs/super.c | 122 ++++
> include/linux/blkdev.h | 15 +
> include/linux/dcache.h | 1
> include/linux/device.h | 7
> include/linux/fs.h | 79 +++
> include/linux/key.h | 4
> include/linux/lsm_hooks.h | 48 ++
> include/linux/security.h | 35 +
> include/linux/syscalls.h | 5
> include/linux/usb.h | 19 +
> include/linux/watch_queue.h | 87 +++
> include/uapi/linux/fsinfo.h | 10
> include/uapi/linux/keyctl.h | 1
> include/uapi/linux/watch_queue.h | 213 ++++++++
> kernel/sys_ni.c | 7
> samples/Kconfig | 6
> samples/Makefile | 1
> samples/vfs/test-fsinfo.c | 13
> samples/watch_queue/Makefile | 9
> samples/watch_queue/watch_test.c | 308 +++++++++++
> security/keys/Kconfig | 10
> security/keys/compat.c | 2
> security/keys/gc.c | 5
> security/keys/internal.h | 30 +
> security/keys/key.c | 37 +
> security/keys/keyctl.c | 95 +++
> security/keys/keyring.c | 17 -
> security/keys/request_key.c | 4
> security/security.c | 29 +
> 52 files changed, 3121 insertions(+), 38 deletions(-)
> create mode 100644 Documentation/watch_queue.rst
> create mode 100644 drivers/base/watch.c
> create mode 100644 drivers/misc/watch_queue.c
> create mode 100644 fs/mount_notify.c
> create mode 100644 include/linux/watch_queue.h
> create mode 100644 include/uapi/linux/watch_queue.h
> create mode 100644 samples/watch_queue/Makefile
> create mode 100644 samples/watch_queue/watch_test.c
>
^ permalink raw reply
* Re: [RFC PATCH v2 1/5] mm: Introduce vm_ops->may_mprotect()
From: Jarkko Sakkinen @ 2019-06-10 15:06 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <20190606021145.12604-2-sean.j.christopherson@intel.com>
On Wed, Jun 05, 2019 at 07:11:41PM -0700, Sean Christopherson wrote:
> SGX will use the may_mprotect() hook to prevent userspace from
> circumventing various security checks, e.g. Linux Security Modules.
> Naming it may_mprotect() instead of simply mprotect() is intended to
> reflect the hook's purpose as a way to gate mprotect() as opposed to
> a wholesale replacement.
"This commit adds may_mprotect() to struct vm_operations_struct, which
can be used to ask from the owner of a VMA if mprotect() is allowed."
This would be more appropriate statement because that is what the code
change aims for precisely. I did not even understand what you meant by
gating in this context. I would leave SGX and LSM's (and especially
"various security checks", which means abssolutely nothing) out of the
first paragraph completely.
> Enclaves are built by copying data from normal memory into the Enclave
> Page Cache (EPC). Due to the nature of SGX, the EPC is represented by a
> single file that must be MAP_SHARED, i.e. mprotect() only ever sees a
> MAP_SHARED vm_file that references single file path. Furthermore, all
> enclaves will need read, write and execute pages in the EPC.
I would just say that "Due to the fact that EPC is delivered as IO
memory from the preboot firmware, it can be only mapped as MAP_SHARED".
It is what it is.
> As a result, LSM policies cannot be meaningfully applied, e.g. an LSM
> can deny access to the EPC as a whole, but can't deny PROT_EXEC on page
> that originated in a non-EXECUTE file (which is long gone by the time
> mprotect() is called).
I have hard time following what is paragraph is trying to say.
> By hooking mprotect(), SGX can make explicit LSM upcalls while an
> enclave is being built, i.e. when the kernel has a handle to origin of
> each enclave page, and enforce the result of the LSM policy whenever
> userspace maps the enclave page in the future.
"LSM policy whenever calls mprotect()"? I'm no sure why you mean by
mapping here and if there is any need to talk about future. Isn't this
needed now?
> Alternatively, SGX could play games with MAY_{READ,WRITE,EXEC}, but
> that approach is quite ugly, e.g. would require userspace to call an
> SGX ioctl() prior to using mprotect() to extend a page's protections.
Instead of talking "playing games" I would state what could be done with
VM_MAY{READ,WRITE,EXEC} and why it is bad. Leaves questions otherwise.
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> include/linux/mm.h | 2 ++
> mm/mprotect.c | 15 +++++++++++----
> 2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 0e8834ac32b7..a697996040ac 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -458,6 +458,8 @@ struct vm_operations_struct {
> void (*close)(struct vm_area_struct * area);
> int (*split)(struct vm_area_struct * area, unsigned long addr);
> int (*mremap)(struct vm_area_struct * area);
> + int (*may_mprotect)(struct vm_area_struct * area, unsigned long start,
> + unsigned long end, unsigned long prot);
Could be just boolean.
/Jarkko
^ permalink raw reply
* Re: [PATCH 35/58] LSM: Limit calls to certain module hooks
From: Ondrej Mosnacek @ 2019-06-10 10:20 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, Linux Security Module list,
selinux, Kees Cook, John Johansen, penguin-kernel, Paul Moore,
Stephen Smalley
In-Reply-To: <20190602165101.25079-36-casey@schaufler-ca.com>
Hi Casey,
On Sun, Jun 2, 2019 at 6:53 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> LSM hooks dealing with security context strings should
> only be called for one security module. Add call macros
> that invoke a single module hook and us in for those cases.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> security/security.c | 32 ++++++++++++++++++++++++++++----
> 1 file changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index 69983ad68233..365970f2501d 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -698,6 +698,16 @@ int lsm_superblock_alloc(struct super_block *sb)
> P->hook.FUNC(__VA_ARGS__); \
> } while (0)
>
> +#define call_one_void_hook(FUNC, ...) \
> + do { \
> + struct security_hook_list *P; \
> + \
> + hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> + P->hook.FUNC(__VA_ARGS__); \
> + break; \
> + } \
> + } while (0)
> +
> #define call_int_hook(FUNC, IRC, ...) ({ \
> int RC = IRC; \
> do { \
> @@ -712,6 +722,19 @@ int lsm_superblock_alloc(struct super_block *sb)
> RC; \
> })
>
> +#define call_one_int_hook(FUNC, IRC, ...) ({ \
> + int RC = IRC; \
> + do { \
> + struct security_hook_list *P; \
> + \
> + hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> + RC = P->hook.FUNC(__VA_ARGS__); \
> + break; \
> + } \
> + } while (0); \
> + RC; \
> +})
> +
> /* Security operations */
>
> int security_binder_set_context_mgr(struct task_struct *mgr)
> @@ -1951,7 +1974,8 @@ EXPORT_SYMBOL(security_ismaclabel);
>
> int security_secid_to_secctx(struct lsm_export *l, char **secdata, u32 *seclen)
> {
> - return call_int_hook(secid_to_secctx, -EOPNOTSUPP, l, secdata, seclen);
> + return call_one_int_hook(secid_to_secctx, -EOPNOTSUPP, l, secdata,
> + seclen);
> }
> EXPORT_SYMBOL(security_secid_to_secctx);
>
> @@ -1959,13 +1983,13 @@ int security_secctx_to_secid(const char *secdata, u32 seclen,
> struct lsm_export *l)
> {
> lsm_export_init(l);
> - return call_int_hook(secctx_to_secid, 0, secdata, seclen, l);
> + return call_one_int_hook(secctx_to_secid, 0, secdata, seclen, l);
> }
> EXPORT_SYMBOL(security_secctx_to_secid);
>
> void security_release_secctx(char *secdata, u32 seclen)
> {
> - call_void_hook(release_secctx, secdata, seclen);
> + call_one_void_hook(release_secctx, secdata, seclen);
> }
> EXPORT_SYMBOL(security_release_secctx);
>
> @@ -2090,7 +2114,7 @@ EXPORT_SYMBOL(security_sock_rcv_skb);
> int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> int __user *optlen, unsigned len)
> {
> - return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock,
> + return call_one_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock,
> optval, optlen, len);
> }
>
> --
> 2.19.1
>
Shouldn't dentry_init_security() use call_one_int_hook() as well? It
also returns a context string.
Thanks,
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply
* [RFC PATCH v1 1/3] LSM/x86/sgx: Add SGX specific LSM hooks
From: Cedric Xing @ 2019-06-10 7:03 UTC (permalink / raw)
To: linux-security-module, selinux, linux-kernel, linux-sgx
Cc: Cedric Xing, jarkko.sakkinen, luto, sds, jmorris, serge, paul,
eparis, jethro, dave.hansen, tglx, torvalds, akpm, nhorman,
pmccallum, serge.ayoun, shay.katz-zamir, haitao.huang,
andriy.shevchenko, kai.svahn, bp, josh, kai.huang, rientjes,
william.c.roberts, philip.b.tricca
In-Reply-To: <cover.1560131039.git.cedric.xing@intel.com>
This patch has made two changes to LSM hooks.
The first change is the addition of two new SGX specific LSM hooks.
security_enclave_load() - is called whenever new EPC pages are added to an
enclave, so that an LSM module could initialize internal states for those
pages. An LSM module may track protections ever granted to enclave pages in
order to come to reasonable decisions in security_file_mprotect() hook in
future.
security_enclave_init() - is called when an enclave is about to be intialized
(by EINIT). An LSM module may approve/decline the request by looking into the
SIGSTRUCT, or the file from which the SIGSTRUCT was loaded from.
The second change is to export symbol security_file_mprotect() to make it
available to kernel modules. The SGX module will invoke
security_file_mprotect() to validate protection for the virtual memory range
being mmap()'ed.
Please see include/linux/lsm_hooks.h for more information.
Signed-off-by: Cedric Xing <cedric.xing@intel.com>
---
include/linux/lsm_hooks.h | 33 +++++++++++++++++++++++++++++++++
include/linux/security.h | 26 ++++++++++++++++++++++++++
security/security.c | 21 +++++++++++++++++++++
3 files changed, 80 insertions(+)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 47f58cfb6a19..ceb18c5c25f3 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1446,6 +1446,27 @@
* @bpf_prog_free_security:
* Clean up the security information stored inside bpf prog.
*
+ * Security hooks for SGX enclaves
+ *
+ * @enclave_load:
+ * Check permissions before loading pages into enclaves. Must be called
+ * with current->mm->mmap_sem locked.
+ * @encl: file pointer identifying the enclave
+ * @addr: linear address to which new pages are being added. Must be page
+ * aligned
+ * @size: total size of pages being added. Must be integral multiple of
+ * page size
+ * @prot: requested protection. Shall be the same protection as the VMA
+ * covering the target linear range, or 0 if target range not mapped
+ * @source: the VMA containing the source pages. Shall be NULL if there's
+ * no source pages (e.g. EAUG)
+ *
+ * @enclave_init:
+ * Check SIGSTRUCT before initializing (EINIT) enclaves. Must be called
+ * with current->mm->mmap_sem locked.
+ * @encl: file pointer identifying the enclave being initialized
+ * @sigstruct: pointer to sigstruct in kernel memory
+ * @sigstruct_vma: vma containing the original sigstruct in user space
*/
union security_list_options {
int (*binder_set_context_mgr)(struct task_struct *mgr);
@@ -1807,6 +1828,14 @@ union security_list_options {
int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
#endif /* CONFIG_BPF_SYSCALL */
+#ifdef CONFIG_INTEL_SGX
+ int (*enclave_load)(struct file *encl, unsigned long addr,
+ unsigned long size, unsigned long prot,
+ struct vm_area_struct *source);
+ int (*enclave_init)(struct file *encl,
+ const struct sgx_sigstruct *sigstruct,
+ struct vm_area_struct *sigstruct_vma);
+#endif /* CONFIG_INTEL_SGX */
};
struct security_hook_heads {
@@ -2046,6 +2075,10 @@ struct security_hook_heads {
struct hlist_head bpf_prog_alloc_security;
struct hlist_head bpf_prog_free_security;
#endif /* CONFIG_BPF_SYSCALL */
+#ifdef CONFIG_INTEL_SGX
+ struct hlist_head enclave_load;
+ struct hlist_head enclave_init;
+#endif /* CONFIG_INTEL_SGX */
} __randomize_layout;
/*
diff --git a/include/linux/security.h b/include/linux/security.h
index 659071c2e57c..d44655dd06dd 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1829,5 +1829,31 @@ static inline void security_bpf_prog_free(struct bpf_prog_aux *aux)
#endif /* CONFIG_SECURITY */
#endif /* CONFIG_BPF_SYSCALL */
+#ifdef CONFIG_INTEL_SGX
+struct sgx_sigstruct;
+#ifdef CONFIG_SECURITY
+extern int security_enclave_load(struct file *encl, unsigned long addr,
+ unsigned long size, unsigned long prot,
+ struct vm_area_struct *source);
+extern int security_enclave_init(struct file *encl,
+ const struct sgx_sigstruct *sigstruct,
+ struct vm_area_struct *sigstruct_vma);
+#else
+static inline int security_enclave_load(struct file *encl, unsigned long addr,
+ unsigned long size, unsigned long prot,
+ struct vm_area_struct *source)
+{
+ return 0;
+}
+
+static inline int security_enclave_init(struct file *encl,
+ const struct sigstruct *sigstruct,
+ struct vm_area_struct *sigstruct_vma)
+{
+ return 0;
+}
+#endif /* CONFIG_SECURITY */
+#endif /* CONFIG_INTEL_SGX */
+
#endif /* ! __LINUX_SECURITY_H */
diff --git a/security/security.c b/security/security.c
index f493db0bf62a..3a5c9847f2c8 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1420,6 +1420,7 @@ int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
{
return call_int_hook(file_mprotect, 0, vma, reqprot, prot);
}
+EXPORT_SYMBOL(security_file_mprotect);
int security_file_lock(struct file *file, unsigned int cmd)
{
@@ -2355,3 +2356,23 @@ void security_bpf_prog_free(struct bpf_prog_aux *aux)
call_void_hook(bpf_prog_free_security, aux);
}
#endif /* CONFIG_BPF_SYSCALL */
+
+#ifdef CONFIG_INTEL_SGX
+
+int security_enclave_load(struct file *encl, unsigned long addr,
+ unsigned long size, unsigned long prot,
+ struct vm_area_struct *source)
+{
+ return call_int_hook(enclave_load, 0, encl, addr, size, prot, source);
+}
+EXPORT_SYMBOL(security_enclave_load);
+
+int security_enclave_init(struct file *encl,
+ const struct sgx_sigstruct *sigstruct,
+ struct vm_area_struct *sigstruct_vma)
+{
+ return call_int_hook(enclave_init, 0, encl, sigstruct, sigstruct_vma);
+}
+EXPORT_SYMBOL(security_enclave_init);
+
+#endif /* CONFIG_INTEL_SGX */
--
2.17.1
^ permalink raw reply related
* [RFC PATCH v1 3/3] LSM/x86/sgx: Call new LSM hooks from SGX subsystem
From: Cedric Xing @ 2019-06-10 7:03 UTC (permalink / raw)
To: linux-security-module, selinux, linux-kernel, linux-sgx
Cc: Cedric Xing, jarkko.sakkinen, luto, sds, jmorris, serge, paul,
eparis, jethro, dave.hansen, tglx, torvalds, akpm, nhorman,
pmccallum, serge.ayoun, shay.katz-zamir, haitao.huang,
andriy.shevchenko, kai.svahn, bp, josh, kai.huang, rientjes,
william.c.roberts, philip.b.tricca
In-Reply-To: <cover.1560131039.git.cedric.xing@intel.com>
There are three places LSM hooks are called from within the SGX subsystem.
The first place is to invoke security_file_mprotect() in sgx_mmap() to validate
requested protection. Given the architecture of SGX subsystem, all enclaves
look like file mappings of /dev/sgx/enclave device file, meaning the existing
security_mmap_file() invoked inside vm_mmap_pgoff() cannot provide any
meaningful information to LSM. Based on the idea that mmap(prot) is equivalent
to mmap(PROT_NONE) followed by mprotect(prot), security_file_mprotect() shall
be queried with more specific enclave/page information.
Secondly, security_enclave_load() is invoked upon loading of every enclave
page.
Lastly, security_enclave_init() is invoked before initializing (EINIT) every
enclave.
Signed-off-by: Cedric Xing <cedric.xing@intel.com>
---
arch/x86/kernel/cpu/sgx/driver/ioctl.c | 72 +++++++++++++++++++++++---
arch/x86/kernel/cpu/sgx/driver/main.c | 12 ++++-
2 files changed, 74 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index b186fb7b48d5..a3f22a6f6d2b 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -11,6 +11,7 @@
#include <linux/shmem_fs.h>
#include <linux/slab.h>
#include <linux/suspend.h>
+#include <linux/security.h>
#include "driver.h"
struct sgx_add_page_req {
@@ -575,6 +576,42 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
return ret;
}
+static int sgx_encl_prepare_page(struct file *filp, unsigned long dst,
+ unsigned long src, void *buf)
+{
+ struct vm_area_struct *vma;
+ unsigned long prot;
+ int rc = 0;
+
+ if (dst & ~PAGE_SIZE)
+ return -EINVAL;
+
+ down_read(¤t->mm->mmap_sem);
+
+ vma = find_vma(current->mm, dst);
+ if (vma && dst >= vma->vm_start)
+ prot = vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC);
+ else
+ prot = 0;
+
+ vma = find_vma(current->mm, src);
+ if (!vma || src < vma->vm_start || src + PAGE_SIZE > vma->vm_end)
+ rc = -EFAULT;
+
+ if (!rc && !(vma->vm_flags & VM_MAYEXEC))
+ rc = -EACCES;
+
+ if (!rc)
+ rc = security_enclave_load(filp, dst, PAGE_SIZE, prot, vma);
+
+ if (!rc && copy_from_user(buf, (void __user *)src, PAGE_SIZE))
+ rc = -EFAULT;
+
+ up_read(¤t->mm->mmap_sem);
+
+ return rc;
+}
+
/**
* sgx_ioc_enclave_add_page - handler for %SGX_IOC_ENCLAVE_ADD_PAGE
*
@@ -613,10 +650,9 @@ static long sgx_ioc_enclave_add_page(struct file *filep, unsigned int cmd,
data = kmap(data_page);
- if (copy_from_user((void *)data, (void __user *)addp->src, PAGE_SIZE)) {
- ret = -EFAULT;
+ ret = sgx_encl_prepare_page(filep, addp->addr, addp->src, data);
+ if (ret)
goto out;
- }
ret = sgx_encl_add_page(encl, addp->addr, data, &secinfo, addp->mrmask);
if (ret)
@@ -718,6 +754,29 @@ static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct,
return ret;
}
+static int sgx_encl_prepare_sigstruct(struct file *filp, unsigned long src,
+ struct sgx_sigstruct *ss)
+{
+ struct vm_area_struct *vma;
+ int rc = 0;
+
+ down_read(¤t->mm->mmap_sem);
+
+ vma = find_vma(current->mm, src);
+ if (!vma || src < vma->vm_start || src + sizeof(*ss) > vma->vm_end)
+ rc = -EFAULT;
+
+ if (!rc && copy_from_user(ss, (void __user *)src, sizeof(*ss)))
+ rc = -EFAULT;
+
+ if (!rc)
+ rc = security_enclave_init(filp, ss, vma);
+
+ up_read(¤t->mm->mmap_sem);
+
+ return rc;
+}
+
/**
* sgx_ioc_enclave_init - handler for %SGX_IOC_ENCLAVE_INIT
*
@@ -753,12 +812,9 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned int cmd,
((unsigned long)sigstruct + PAGE_SIZE / 2);
memset(einittoken, 0, sizeof(*einittoken));
- if (copy_from_user(sigstruct, (void __user *)initp->sigstruct,
- sizeof(*sigstruct))) {
- ret = -EFAULT;
+ ret = sgx_encl_prepare_sigstruct(filep, initp->sigstruct, sigstruct);
+ if (ret)
goto out;
- }
-
ret = sgx_encl_init(encl, sigstruct, einittoken);
diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
index 58ba6153070b..c634df440c16 100644
--- a/arch/x86/kernel/cpu/sgx/driver/main.c
+++ b/arch/x86/kernel/cpu/sgx/driver/main.c
@@ -63,14 +63,22 @@ static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
{
struct sgx_encl *encl = file->private_data;
+ unsigned long prot;
+ int rc;
vma->vm_ops = &sgx_vm_ops;
vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;
vma->vm_private_data = encl;
- kref_get(&encl->refcount);
+ prot = vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC);
+ vma->vm_flags &= ~prot;
+ rc = security_file_mprotect(vma, prot, prot);
+ if (!rc) {
+ vma->vm_flags |= prot;
+ kref_get(&encl->refcount);
+ }
- return 0;
+ return rc;
}
static unsigned long sgx_get_unmapped_area(struct file *file,
--
2.17.1
^ permalink raw reply related
* [RFC PATCH v1 2/3] LSM/x86/sgx: Implement SGX specific hooks in SELinux
From: Cedric Xing @ 2019-06-10 7:03 UTC (permalink / raw)
To: linux-security-module, selinux, linux-kernel, linux-sgx
Cc: Cedric Xing, jarkko.sakkinen, luto, sds, jmorris, serge, paul,
eparis, jethro, dave.hansen, tglx, torvalds, akpm, nhorman,
pmccallum, serge.ayoun, shay.katz-zamir, haitao.huang,
andriy.shevchenko, kai.svahn, bp, josh, kai.huang, rientjes,
william.c.roberts, philip.b.tricca
In-Reply-To: <cover.1560131039.git.cedric.xing@intel.com>
In this patch, SELinux maintains two bits per enclave page, namely SGX__EXECUTE
and SGX__EXECMOD.
SGX__EXECUTE is set initially (by selinux_enclave_load) for every enclave page
that was loaded from a potentially executable source page. SGX__EXECMOD is set
for every page that was loaded from a file that has FILE__EXECMOD.
At runtime, on every protection change (resulted in a call to
selinux_file_mprotect), SGX__EXECUTE is cleared for a page if VM_WRITE is
requested, unless SGX__EXECMOD is set.
To track enclave page protection changes, SELinux has been changed in four
different places.
Firstly, storage is required for storing per page SGX__EXECUTE and SGX__EXECMOD
bits. Given every enclave instance is uniquely tied to an open file (i.e.
struct file), the storage is allocated by extending `file_security_struct`.
More precisely, a new field `esec` has been added, initially zero, to point to
the data structure for tracking per page protection. `esec` will be
allocated/initialized at the first invocation of selinux_enclave_load().
Then, selinux_enclave_load() initializes those 2 bits for every new enclave as
described above. One more detail worth noting, is that selinux_enclave_load()
sets SGX__EXECUTE/SGX__EXECMOD for EAUG'ed pages (for upcoming SGX2) only if
the calling process has FILE__EXECMOD on the sigstruct file.
Afterwards, every change on protection will go through selinux_file_mprotect()
so will be noted. Please note that user space could munmap() then mmap() to
work around mprotect(), but that "leak" could be "plugged" by SGX subsystem
calling security_file_mprotect() explicitly whenever new mappings are created.
Finally, the storage for page protection tracking must be freed when the
associated file is closed. Hence a new selinux_file_free_security() has been
added.
Signed-off-by: Cedric Xing <cedric.xing@intel.com>
---
security/selinux/Makefile | 2 +
security/selinux/hooks.c | 77 ++++++-
security/selinux/include/intel_sgx.h | 18 ++
security/selinux/include/objsec.h | 3 +
security/selinux/intel_sgx.c | 292 +++++++++++++++++++++++++++
5 files changed, 391 insertions(+), 1 deletion(-)
create mode 100644 security/selinux/include/intel_sgx.h
create mode 100644 security/selinux/intel_sgx.c
diff --git a/security/selinux/Makefile b/security/selinux/Makefile
index ccf950409384..58a05a9639e0 100644
--- a/security/selinux/Makefile
+++ b/security/selinux/Makefile
@@ -14,6 +14,8 @@ selinux-$(CONFIG_SECURITY_NETWORK_XFRM) += xfrm.o
selinux-$(CONFIG_NETLABEL) += netlabel.o
+selinux-$(CONFIG_INTEL_SGX) += intel_sgx.o
+
ccflags-y := -I$(srctree)/security/selinux -I$(srctree)/security/selinux/include
$(addprefix $(obj)/,$(selinux-y)): $(obj)/flask.h
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3ec702cf46ca..17f855871a41 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -103,6 +103,7 @@
#include "netlabel.h"
#include "audit.h"
#include "avc_ss.h"
+#include "intel_sgx.h"
struct selinux_state selinux_state;
@@ -3485,6 +3486,11 @@ static int selinux_file_alloc_security(struct file *file)
return file_alloc_security(file);
}
+static void selinux_file_free_security(struct file *file)
+{
+ sgxsec_enclave_free(file);
+}
+
/*
* Check whether a task has the ioctl permission and cmd
* operation to an inode.
@@ -3656,6 +3662,7 @@ static int selinux_file_mprotect(struct vm_area_struct *vma,
unsigned long reqprot,
unsigned long prot)
{
+ int rc;
const struct cred *cred = current_cred();
u32 sid = cred_sid(cred);
@@ -3664,7 +3671,7 @@ static int selinux_file_mprotect(struct vm_area_struct *vma,
if (default_noexec &&
(prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) {
- int rc = 0;
+ rc = 0;
if (vma->vm_start >= vma->vm_mm->start_brk &&
vma->vm_end <= vma->vm_mm->brk) {
rc = avc_has_perm(&selinux_state,
@@ -3691,6 +3698,12 @@ static int selinux_file_mprotect(struct vm_area_struct *vma,
return rc;
}
+#ifdef CONFIG_INTEL_SGX
+ rc = sgxsec_mprotect(vma, prot);
+ if (rc <= 0)
+ return rc;
+#endif
+
return file_map_prot_check(vma->vm_file, prot, vma->vm_flags&VM_SHARED);
}
@@ -6726,6 +6739,62 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
}
#endif
+#ifdef CONFIG_INTEL_SGX
+
+static int selinux_enclave_load(struct file *encl, unsigned long addr,
+ unsigned long size, unsigned long prot,
+ struct vm_area_struct *source)
+{
+ if (source) {
+ /**
+ * Adding page from source => EADD request
+ */
+ int rc = selinux_file_mprotect(source, prot, prot);
+ if (rc)
+ return rc;
+
+ if (!(prot & VM_EXEC) &&
+ selinux_file_mprotect(source, VM_EXEC, VM_EXEC))
+ prot = 0;
+ else {
+ prot = SGX__EXECUTE;
+ if (source->vm_file &&
+ !file_has_perm(current_cred(), source->vm_file,
+ FILE__EXECMOD))
+ prot |= SGX__EXECMOD;
+ }
+ return sgxsec_eadd(encl, addr, size, prot);
+ } else {
+ /**
+ * Adding page from NULL => EAUG request
+ */
+ return sgxsec_eaug(encl, addr, size, prot);
+ }
+}
+
+static int selinux_enclave_init(struct file *encl,
+ const struct sgx_sigstruct *sigstruct,
+ struct vm_area_struct *vma)
+{
+ int rc = 0;
+
+ if (!vma)
+ rc = -EINVAL;
+
+ if (!rc && !(vma->vm_flags & VM_EXEC))
+ rc = selinux_file_mprotect(vma, VM_EXEC, VM_EXEC);
+
+ if (!rc) {
+ if (vma->vm_file)
+ rc = file_has_perm(current_cred(), vma->vm_file,
+ FILE__EXECMOD);
+ rc = sgxsec_einit(encl, sigstruct, !rc);
+ }
+ return rc;
+}
+
+#endif
+
struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
.lbs_cred = sizeof(struct task_security_struct),
.lbs_file = sizeof(struct file_security_struct),
@@ -6808,6 +6877,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(file_permission, selinux_file_permission),
LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),
+ LSM_HOOK_INIT(file_free_security, selinux_file_free_security),
LSM_HOOK_INIT(file_ioctl, selinux_file_ioctl),
LSM_HOOK_INIT(mmap_file, selinux_mmap_file),
LSM_HOOK_INIT(mmap_addr, selinux_mmap_addr),
@@ -6968,6 +7038,11 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
LSM_HOOK_INIT(bpf_prog_free_security, selinux_bpf_prog_free),
#endif
+
+#ifdef CONFIG_INTEL_SGX
+ LSM_HOOK_INIT(enclave_load, selinux_enclave_load),
+ LSM_HOOK_INIT(enclave_init, selinux_enclave_init),
+#endif
};
static __init int selinux_init(void)
diff --git a/security/selinux/include/intel_sgx.h b/security/selinux/include/intel_sgx.h
new file mode 100644
index 000000000000..8f9c6c734921
--- /dev/null
+++ b/security/selinux/include/intel_sgx.h
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2016-18 Intel Corporation.
+
+#ifndef _SELINUX_SGXSEC_H_
+#define _SELINUX_SGXSEC_H_
+
+#include <linux/lsm_hooks.h>
+
+#define SGX__EXECUTE 1
+#define SGX__EXECMOD 2
+
+void sgxsec_enclave_free(struct file *);
+int sgxsec_mprotect(struct vm_area_struct *, size_t);
+int sgxsec_eadd(struct file *, size_t, size_t, size_t);
+int sgxsec_eaug(struct file *, size_t, size_t, size_t);
+int sgxsec_einit(struct file *, const struct sgx_sigstruct *, int);
+
+#endif
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index 231262d8eac9..0fb4da7e3a8a 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -71,6 +71,9 @@ struct file_security_struct {
u32 fown_sid; /* SID of file owner (for SIGIO) */
u32 isid; /* SID of inode at the time of file open */
u32 pseqno; /* Policy seqno at the time of file open */
+#ifdef CONFIG_INTEL_SGX
+ atomic_long_t esec;
+#endif
};
struct superblock_security_struct {
diff --git a/security/selinux/intel_sgx.c b/security/selinux/intel_sgx.c
new file mode 100644
index 000000000000..37dacf5c295f
--- /dev/null
+++ b/security/selinux/intel_sgx.c
@@ -0,0 +1,292 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2016-18 Intel Corporation.
+
+#include "objsec.h"
+#include "intel_sgx.h"
+
+struct region {
+ struct list_head link;
+ size_t start;
+ size_t end;
+ size_t data;
+};
+
+static inline struct region *region_new(void)
+{
+ struct region *n = kzalloc(sizeof(struct region), GFP_KERNEL);
+ if (n)
+ INIT_LIST_HEAD(&n->link);
+ return n;
+}
+
+static inline void region_free(struct region *r)
+{
+ list_del(&r->link);
+ kfree(r);
+}
+
+static struct list_head *
+region_apply_to_range(struct list_head *rgs,
+ size_t start, size_t end,
+ struct list_head *(*cb)(struct region *,
+ size_t, size_t, size_t),
+ size_t arg)
+{
+ struct region *r, *n;
+
+ list_for_each_entry(r, rgs, link)
+ if (start < r-> end)
+ break;
+
+ if (&r->link == rgs || end <= r->start)
+ return rgs;
+
+ do {
+ struct list_head *ret;
+ n = list_next_entry(r, link);
+ ret = (*cb)(r, start, end, arg);
+ if (ret)
+ return ret;
+ r = n;
+ } while (&r->link != rgs && r->start < end);
+ return &r->link;
+}
+
+static struct list_head *
+region_clear_cb(struct region *r, size_t start, size_t end, size_t arg)
+{
+ if (end < r->end) {
+ if (start > r->start) {
+ struct region *n = region_new();
+ if (unlikely(!n))
+ return ERR_PTR(-ENOMEM);
+
+ n->start = r->start;
+ n->end = start;
+ n->data = r->data;
+ list_add_tail(&n->link, &r->link);
+ }
+ r->start = end;
+ return &r->link;
+ }
+
+ if (start > r->start)
+ r->end = start;
+ else
+ region_free(r);
+ return NULL;
+}
+
+static inline struct list_head *
+region_clear_range(struct list_head *rgs, size_t start, size_t end)
+{
+ return region_apply_to_range(rgs, start, end, region_clear_cb, 0);
+}
+
+static struct list_head *
+region_add_range(struct list_head *rgs, size_t start, size_t end, size_t data)
+{
+ struct region *r, *n;
+
+ n = list_entry(region_clear_range(rgs, start, end), typeof(*n), link);
+ if (unlikely(IS_ERR_VALUE(&n->link)))
+ return &n->link;
+
+ if (&n->link != rgs && end == n->start && data == n->data) {
+ n->start = start;
+ r = n;
+ } else {
+ r = region_new();
+ if (unlikely(!r))
+ return ERR_PTR(-ENOMEM);
+
+ r->start = start;
+ r->end = end;
+ r->data = data;
+ list_add_tail(&r->link, &n->link);
+ }
+
+ n = list_prev_entry(r, link);
+ if (&n->link != rgs && start == n->end && data == n->data) {
+ r->start = n->start;
+ region_free(n);
+ }
+
+ return &r->link;
+}
+
+static inline int
+enclave_add_pages(struct list_head *rgs, size_t start, size_t end, size_t flags)
+{
+ void *p = region_add_range(rgs, start, end, flags);
+ return PTR_ERR_OR_ZERO(p);
+}
+
+static inline int enclave_prot_allowed(size_t prot, size_t flags)
+{
+ return !(prot & VM_EXEC) || (flags & SGX__EXECUTE);
+}
+
+static struct list_head *
+enclave_prot_check_cb(struct region *r, size_t start, size_t end, size_t prot)
+{
+ if (!enclave_prot_allowed(prot, r->data))
+ return ERR_PTR(-EACCES);
+ return NULL;
+}
+
+static struct list_head *
+enclave_prot_set_cb(struct region *r, size_t start, size_t end, size_t prot)
+{
+ BUG_ON(!enclave_prot_allowed(prot, r->data));
+
+ if (!(prot & VM_WRITE) ||
+ (r->data & SGX__EXECMOD) ||
+ !(r->data & SGX__EXECUTE))
+ return NULL;
+
+ if (end < r->end) {
+ struct region *n = region_new();
+ if (unlikely(!n))
+ return ERR_PTR(-ENOMEM);
+
+ n->start = end;
+ n->end = r->end;
+ n->data = r->data;
+ r->end = end;
+ list_add(&n->link, &r->link);
+ }
+
+ if (start > r->start) {
+ struct region *n = region_new();
+ if (unlikely(!n))
+ return ERR_PTR(-ENOMEM);
+
+ n->start = r->start;
+ n->end = start;
+ n->data = r->data;
+ r->start = start;
+ list_add_tail(&n->link, &r->link);
+ }
+
+ r->data &= ~SGX__EXECUTE;
+ return NULL;
+}
+
+static inline int
+enclave_mprotect(struct list_head *rgs, size_t start, size_t end, size_t prot)
+{
+ void *ret;
+
+ ret = region_apply_to_range(rgs, start, end,
+ enclave_prot_check_cb, prot);
+ if (!IS_ERR_VALUE(ret) && (prot & VM_WRITE))
+ ret = region_apply_to_range(rgs, start, end,
+ enclave_prot_set_cb, prot);
+ return PTR_ERR_OR_ZERO(ret);
+}
+
+struct enclave_sec {
+ struct rw_semaphore sem;
+ struct list_head regions;
+ size_t eaug_perm;
+};
+
+static inline struct enclave_sec *__esec(struct file_security_struct *fsec)
+{
+ return (struct enclave_sec *)atomic_long_read(&fsec->esec);
+}
+
+static struct enclave_sec *encl_esec(struct file *encl)
+{
+ struct file_security_struct *fsec = selinux_file(encl);
+ struct enclave_sec *esec = __esec(fsec);
+
+ if (unlikely(!esec)) {
+ long n;
+
+ esec = kzalloc(sizeof(*esec), GFP_KERNEL);
+ if (!esec)
+ return NULL;
+
+ init_rwsem(&esec->sem);
+ INIT_LIST_HEAD(&esec->regions);
+
+ n = atomic_long_cmpxchg(&fsec->esec, 0, (long)esec);
+ if (n) {
+ kfree(esec);
+ esec = (typeof(esec))n;
+ }
+ }
+
+ return esec;
+}
+
+void sgxsec_enclave_free(struct file *encl)
+{
+ struct enclave_sec *esec = __esec(selinux_file(encl));
+
+ if (esec) {
+ struct region *r, *n;
+
+ BUG_ON(rwsem_is_locked(&esec->sem));
+
+ list_for_each_entry_safe(r, n, &esec->regions, link)
+ region_free(r);
+
+ kfree(esec);
+ }
+}
+
+int sgxsec_mprotect(struct vm_area_struct *vma, size_t prot)
+{
+ struct enclave_sec *esec;
+ int rc;
+
+ if (!vma->vm_file || !(esec = __esec(selinux_file(vma->vm_file)))) {
+ /* Positive return value indicates non-enclave VMA */
+ return 1;
+ }
+
+ down_read(&esec->sem);
+ rc = enclave_mprotect(&esec->regions, vma->vm_start, vma->vm_end, prot);
+ up_read(&esec->sem);
+ return rc;
+}
+
+int sgxsec_eadd(struct file *encl, size_t start, size_t size, size_t perm)
+{
+ struct enclave_sec *esec = encl_esec(encl);
+ int rc;
+
+ if (down_write_killable(&esec->sem))
+ return -EINTR;
+ rc = enclave_add_pages(&esec->regions, start, start + size, perm);
+ up_write(&esec->sem);
+ return rc;
+}
+
+int sgxsec_eaug(struct file *encl, size_t start, size_t size, size_t prot)
+{
+ struct enclave_sec *esec = encl_esec(encl);
+ int rc = -EPERM;
+
+ if (down_write_killable(&esec->sem))
+ return -EINTR;
+ if (enclave_prot_allowed(prot, esec->eaug_perm))
+ rc = enclave_add_pages(&esec->regions, start, start + size,
+ esec->eaug_perm);
+ up_write(&esec->sem);
+ return rc;
+}
+
+int sgxsec_einit(struct file *encl, const struct sgx_sigstruct *sigstruct, int execmod)
+{
+ struct enclave_sec *esec = encl_esec(encl);
+
+ if (down_write_killable(&esec->sem))
+ return -EINTR;
+ esec->eaug_perm = execmod ? SGX__EXECUTE | SGX__EXECMOD : 0;
+ up_write(&esec->sem);
+ return 0;
+}
--
2.17.1
^ permalink raw reply related
* [RFC PATCH v1 0/3] security/x86/sgx: SGX specific LSM hooks
From: Cedric Xing @ 2019-06-10 7:03 UTC (permalink / raw)
To: linux-security-module, selinux, linux-kernel, linux-sgx
Cc: Cedric Xing, jarkko.sakkinen, luto, sds, jmorris, serge, paul,
eparis, jethro, dave.hansen, tglx, torvalds, akpm, nhorman,
pmccallum, serge.ayoun, shay.katz-zamir, haitao.huang,
andriy.shevchenko, kai.svahn, bp, josh, kai.huang, rientjes,
william.c.roberts, philip.b.tricca
In-Reply-To: <20190606021145.12604-1-sean.j.christopherson@intel.com>
This series intends to make the new SGX subsystem and the existing LSM
architecture work together smoothly so that, say, SGX cannot be abused to work
around restrictions set forth by LSM. This series applies on top of Jarkko
Sakkinen's SGX series v20 (https://lkml.org/lkml/2019/4/17/344), where abundant
details of this SGX/LSM problem could be found.
This series is an alternative to Sean Christopherson's recent RFC series
(https://lkml.org/lkml/2019/6/5/1070) that was trying to solve the same
problem. The key problem is for LSM to determine the "maximal (most permissive)
protection" allowed for individual enclave pages. Sean's approach is to take
that from user mode code as a parameter of the EADD ioctl, validate it with LSM
ahead of time, and then enforce it inside the SGX subsystem. The major
disadvantage IMHO is that a priori knowledge of "maximal protection" is needed,
but it isn't always available in certain use cases. In fact, it is an unusual
approach to take "maximal protection" from user code, as what SELinux is doing
today is to determine "maximal protection" of a vma using attributes associated
with vma->vm_file instead. When it comes to enclaves, vma->vm_file always
points /dev/sgx/enclave, so what's missing is a new way for LSM modules to
remember origins of enclave pages so that they don't solely depend on
vma->vm_file to determine "maximal protection".
This series takes advantage of the fact that enclave pages cannot be remapped
(to different linear address), therefore the pair of { vma->vm_file,
linear_address } can be used to uniquely identify an enclave page. Then by
notifying LSM on creation of every enclave page (via a new LSM hook -
security_enclave_load), LSM modules would be able to track origin and
protection changes of every page, hence be able to judge correctly upon
mmap/mprotect requests.
Cedric Xing (3):
LSM/x86/sgx: Add SGX specific LSM hooks
LSM/x86/sgx: Implement SGX specific hooks in SELinux
LSM/x86/sgx: Call new LSM hooks from SGX subsystem
arch/x86/kernel/cpu/sgx/driver/ioctl.c | 72 +++++-
arch/x86/kernel/cpu/sgx/driver/main.c | 12 +-
include/linux/lsm_hooks.h | 33 +++
include/linux/security.h | 26 +++
security/security.c | 21 ++
security/selinux/Makefile | 2 +
security/selinux/hooks.c | 77 ++++++-
security/selinux/include/intel_sgx.h | 18 ++
security/selinux/include/objsec.h | 3 +
security/selinux/intel_sgx.c | 292 +++++++++++++++++++++++++
10 files changed, 545 insertions(+), 11 deletions(-)
create mode 100644 security/selinux/include/intel_sgx.h
create mode 100644 security/selinux/intel_sgx.c
--
2.17.1
^ permalink raw reply
* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
From: Janne Karhunen @ 2019-06-09 17:06 UTC (permalink / raw)
To: James Morris
Cc: Paul Moore, Casey Schaufler, Stephen Smalley, Mimi Zohar,
linux-integrity, linux-security-module
In-Reply-To: <alpine.LRH.2.21.1906080748010.24873@namei.org>
On Sat, Jun 8, 2019 at 12:48 AM James Morris <jmorris@namei.org> wrote:
> > simple fact that we started with one type of notifier for the LSM, and
> > we are now switching to the other (and getting lucky that it is safe
> > to do so for the existing callers) seems to lend some weight to the
> > argument we may need both and adding "block"/"blocking"/etc. to the
> > name has value.
>
> Fair enough.
Ok, I take this to mean we have an agreement to go with this variant.
I will post the fixes to the Mimi's findings on top of this one
tomorrow.
--
Janne
^ permalink raw reply
* [PATCH] tomoyo: Don't check open/getattr permission on sockets.
From: Tetsuo Handa @ 2019-06-09 6:41 UTC (permalink / raw)
To: Al Viro, linux-fsdevel
Cc: syzbot, jmorris, linux-kernel, linux-security-module, serge,
syzkaller-bugs, takedakn
In-Reply-To: <201906060520.x565Kd8j017983@www262.sakura.ne.jp>
syzbot is reporting that use of SOCKET_I()->sk from open() can result in
use after free problem [1], for socket's inode is still reachable via
/proc/pid/fd/n despite destruction of SOCKET_I()->sk already completed.
But there is no point with calling security_file_open() on sockets
because open("/proc/pid/fd/n", !O_PATH) on sockets fails with -ENXIO.
There is some point with calling security_inode_getattr() on sockets
because stat("/proc/pid/fd/n") and fstat(open("/proc/pid/fd/n", O_PATH))
are valid. If we want to access "struct sock"->sk_{family,type,protocol}
fields, we will need to use security_socket_post_create() hook and
security_inode_free() hook in order to remember these fields because
security_sk_free() hook is called before the inode is destructed. But
since information which can be protected by checking
security_inode_getattr() on sockets is trivial, let's not be bothered by
"struct inode"->i_security management.
There is point with calling security_file_ioctl() on sockets. Since
ioctl(open("/proc/pid/fd/n", O_PATH)) is invalid, security_file_ioctl()
on sockets should remain safe.
[1] https://syzkaller.appspot.com/bug?id=73d590010454403d55164cca23bd0565b1eb3b74
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: syzbot <syzbot+0341f6a4d729d4e0acf1@syzkaller.appspotmail.com>
---
security/tomoyo/tomoyo.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 716c92e..9661b86 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -126,6 +126,9 @@ static int tomoyo_bprm_check_security(struct linux_binprm *bprm)
*/
static int tomoyo_inode_getattr(const struct path *path)
{
+ /* It is not safe to call tomoyo_get_socket_name(). */
+ if (path->dentry->d_inode && S_ISSOCK(path->dentry->d_inode->i_mode))
+ return 0;
return tomoyo_path_perm(TOMOYO_TYPE_GETATTR, path, NULL);
}
@@ -316,6 +319,10 @@ static int tomoyo_file_open(struct file *f)
/* Don't check read permission here if called from do_execve(). */
if (current->in_execve)
return 0;
+ /* Sockets can't be opened by open(). */
+ if (f->f_path.dentry->d_inode &&
+ S_ISSOCK(f->f_path.dentry->d_inode->i_mode))
+ return 0;
return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path,
f->f_flags);
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH 02/13] uapi: General notification ring definitions [ver #4]
From: Randy Dunlap @ 2019-06-09 4:35 UTC (permalink / raw)
To: David Howells, Darrick J. Wong
Cc: viro, raven, linux-fsdevel, linux-api, linux-block, keyrings,
linux-security-module, linux-kernel
In-Reply-To: <29222.1559922719@warthog.procyon.org.uk>
On 6/7/19 8:51 AM, David Howells wrote:
> Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
>>> + __u32 info;
>>> +#define WATCH_INFO_OVERRUN 0x00000001 /* Event(s) lost due to overrun */
>>> +#define WATCH_INFO_ENOMEM 0x00000002 /* Event(s) lost due to ENOMEM */
>>> +#define WATCH_INFO_RECURSIVE 0x00000004 /* Change was recursive */
>>> +#define WATCH_INFO_LENGTH 0x000001f8 /* Length of record / sizeof(watch_notification) */
>>
>> This is a mask, isn't it? Could we perhaps have some helpers here?
>> Something along the lines of...
>>
>> #define WATCH_INFO_LENGTH_MASK 0x000001f8
>> #define WATCH_INFO_LENGTH_SHIFT 3
>>
>> static inline size_t watch_notification_length(struct watch_notification *wn)
>> {
>> return (wn->info & WATCH_INFO_LENGTH_MASK) >> WATCH_INFO_LENGTH_SHIFT *
>> sizeof(struct watch_notification);
>> }
>>
>> static inline struct watch_notification *watch_notification_next(
>> struct watch_notification *wn)
>> {
>> return wn + ((wn->info & WATCH_INFO_LENGTH_MASK) >>
>> WATCH_INFO_LENGTH_SHIFT);
>> }
>
> No inline functions in UAPI headers, please. I'd love to kill off the ones
> that we have, but that would break things.
Hi David,
What is the problem with inline functions in UAPI headers?
>> ...so that we don't have to opencode all of the ring buffer walking
>> magic and stuff?
>
> There'll end up being a small userspace library, I think.
>>> +};
>>> +
>>> +#define WATCH_LENGTH_SHIFT 3
>>> +
>>> +struct watch_queue_buffer {
>>> + union {
>>> + /* The first few entries are special, containing the
>>> + * ring management variables.
>>
>> The first /two/ entries, correct?
>
> Currently two.
>
>> Also, weird multiline comment style.
>
> Not really.
Yes really.
>>> + */
It does not match the preferred coding style for multi-line comments
according to coding-style.rst.
thanks.
--
~Randy
^ permalink raw reply
* [PATCH v3 18/33] docs: netlabel: convert docs to ReST and rename to *.rst
From: Mauro Carvalho Chehab @ 2019-06-09 2:27 UTC (permalink / raw)
To: Linux Doc Mailing List
Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet, Paul Moore, netdev, linux-security-module
In-Reply-To: <cover.1560045490.git.mchehab+samsung@kernel.org>
Convert netlabel documentation to ReST.
This was trivial: just add proper title markups.
At its new index.rst, let's add a :orphan: while this is not linked to
the main index.rst file, in order to avoid build warnings.
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
.../{cipso_ipv4.txt => cipso_ipv4.rst} | 19 +++++++++++------
Documentation/netlabel/draft_ietf.rst | 5 +++++
Documentation/netlabel/index.rst | 21 +++++++++++++++++++
.../{introduction.txt => introduction.rst} | 16 +++++++++-----
.../{lsm_interface.txt => lsm_interface.rst} | 16 +++++++++-----
5 files changed, 61 insertions(+), 16 deletions(-)
rename Documentation/netlabel/{cipso_ipv4.txt => cipso_ipv4.rst} (87%)
create mode 100644 Documentation/netlabel/draft_ietf.rst
create mode 100644 Documentation/netlabel/index.rst
rename Documentation/netlabel/{introduction.txt => introduction.rst} (91%)
rename Documentation/netlabel/{lsm_interface.txt => lsm_interface.rst} (88%)
diff --git a/Documentation/netlabel/cipso_ipv4.txt b/Documentation/netlabel/cipso_ipv4.rst
similarity index 87%
rename from Documentation/netlabel/cipso_ipv4.txt
rename to Documentation/netlabel/cipso_ipv4.rst
index a6075481fd60..cbd3f3231221 100644
--- a/Documentation/netlabel/cipso_ipv4.txt
+++ b/Documentation/netlabel/cipso_ipv4.rst
@@ -1,10 +1,13 @@
+===================================
NetLabel CIPSO/IPv4 Protocol Engine
-==============================================================================
+===================================
+
Paul Moore, paul.moore@hp.com
May 17, 2006
- * Overview
+Overview
+========
The NetLabel CIPSO/IPv4 protocol engine is based on the IETF Commercial
IP Security Option (CIPSO) draft from July 16, 1992. A copy of this
@@ -13,7 +16,8 @@ draft can be found in this directory
it to an RFC standard it has become a de-facto standard for labeled
networking and is used in many trusted operating systems.
- * Outbound Packet Processing
+Outbound Packet Processing
+==========================
The CIPSO/IPv4 protocol engine applies the CIPSO IP option to packets by
adding the CIPSO label to the socket. This causes all packets leaving the
@@ -24,7 +28,8 @@ label by using the NetLabel security module API; if the NetLabel "domain" is
configured to use CIPSO for packet labeling then a CIPSO IP option will be
generated and attached to the socket.
- * Inbound Packet Processing
+Inbound Packet Processing
+=========================
The CIPSO/IPv4 protocol engine validates every CIPSO IP option it finds at the
IP layer without any special handling required by the LSM. However, in order
@@ -33,7 +38,8 @@ NetLabel security module API to extract the security attributes of the packet.
This is typically done at the socket layer using the 'socket_sock_rcv_skb()'
LSM hook.
- * Label Translation
+Label Translation
+=================
The CIPSO/IPv4 protocol engine contains a mechanism to translate CIPSO security
attributes such as sensitivity level and category to values which are
@@ -42,7 +48,8 @@ Domain Of Interpretation (DOI) definition and are configured through the
NetLabel user space communication layer. Each DOI definition can have a
different security attribute mapping table.
- * Label Translation Cache
+Label Translation Cache
+=======================
The NetLabel system provides a framework for caching security attribute
mappings from the network labels to the corresponding LSM identifiers. The
diff --git a/Documentation/netlabel/draft_ietf.rst b/Documentation/netlabel/draft_ietf.rst
new file mode 100644
index 000000000000..5ed39ab8234b
--- /dev/null
+++ b/Documentation/netlabel/draft_ietf.rst
@@ -0,0 +1,5 @@
+Draft IETF CIPSO IP Security
+----------------------------
+
+ .. include:: draft-ietf-cipso-ipsecurity-01.txt
+ :literal:
diff --git a/Documentation/netlabel/index.rst b/Documentation/netlabel/index.rst
new file mode 100644
index 000000000000..47f1e0e5acd1
--- /dev/null
+++ b/Documentation/netlabel/index.rst
@@ -0,0 +1,21 @@
+:orphan:
+
+========
+NetLabel
+========
+
+.. toctree::
+ :maxdepth: 1
+
+ introduction
+ cipso_ipv4
+ lsm_interface
+
+ draft_ietf
+
+.. only:: subproject and html
+
+ Indices
+ =======
+
+ * :ref:`genindex`
diff --git a/Documentation/netlabel/introduction.txt b/Documentation/netlabel/introduction.rst
similarity index 91%
rename from Documentation/netlabel/introduction.txt
rename to Documentation/netlabel/introduction.rst
index 3caf77bcff0f..9333bbb0adc1 100644
--- a/Documentation/netlabel/introduction.txt
+++ b/Documentation/netlabel/introduction.rst
@@ -1,10 +1,13 @@
+=====================
NetLabel Introduction
-==============================================================================
+=====================
+
Paul Moore, paul.moore@hp.com
August 2, 2006
- * Overview
+Overview
+========
NetLabel is a mechanism which can be used by kernel security modules to attach
security attributes to outgoing network packets generated from user space
@@ -12,7 +15,8 @@ applications and read security attributes from incoming network packets. It
is composed of three main components, the protocol engines, the communication
layer, and the kernel security module API.
- * Protocol Engines
+Protocol Engines
+================
The protocol engines are responsible for both applying and retrieving the
network packet's security attributes. If any translation between the network
@@ -24,7 +28,8 @@ the NetLabel kernel security module API described below.
Detailed information about each NetLabel protocol engine can be found in this
directory.
- * Communication Layer
+Communication Layer
+===================
The communication layer exists to allow NetLabel configuration and monitoring
from user space. The NetLabel communication layer uses a message based
@@ -33,7 +38,8 @@ formatting of these NetLabel messages as well as the Generic NETLINK family
names can be found in the 'net/netlabel/' directory as comments in the
header files as well as in 'include/net/netlabel.h'.
- * Security Module API
+Security Module API
+===================
The purpose of the NetLabel security module API is to provide a protocol
independent interface to the underlying NetLabel protocol engines. In addition
diff --git a/Documentation/netlabel/lsm_interface.txt b/Documentation/netlabel/lsm_interface.rst
similarity index 88%
rename from Documentation/netlabel/lsm_interface.txt
rename to Documentation/netlabel/lsm_interface.rst
index 638c74f7de7f..026fc267f798 100644
--- a/Documentation/netlabel/lsm_interface.txt
+++ b/Documentation/netlabel/lsm_interface.rst
@@ -1,10 +1,13 @@
+========================================
NetLabel Linux Security Module Interface
-==============================================================================
+========================================
+
Paul Moore, paul.moore@hp.com
May 17, 2006
- * Overview
+Overview
+========
NetLabel is a mechanism which can set and retrieve security attributes from
network packets. It is intended to be used by LSM developers who want to make
@@ -12,7 +15,8 @@ use of a common code base for several different packet labeling protocols.
The NetLabel security module API is defined in 'include/net/netlabel.h' but a
brief overview is given below.
- * NetLabel Security Attributes
+NetLabel Security Attributes
+============================
Since NetLabel supports multiple different packet labeling protocols and LSMs
it uses the concept of security attributes to refer to the packet's security
@@ -24,7 +28,8 @@ configuration. It is up to the LSM developer to translate the NetLabel
security attributes into whatever security identifiers are in use for their
particular LSM.
- * NetLabel LSM Protocol Operations
+NetLabel LSM Protocol Operations
+================================
These are the functions which allow the LSM developer to manipulate the labels
on outgoing packets as well as read the labels on incoming packets. Functions
@@ -32,7 +37,8 @@ exist to operate both on sockets as well as the sk_buffs directly. These high
level functions are translated into low level protocol operations based on how
the administrator has configured the NetLabel subsystem.
- * NetLabel Label Mapping Cache Operations
+NetLabel Label Mapping Cache Operations
+=======================================
Depending on the exact configuration, translation between the network packet
label and the internal LSM security identifier can be time consuming. The
--
2.21.0
^ permalink raw reply related
* Re: [RFC PATCH v3 1/1] Add dm verity root hash pkcs7 sig validation
From: Milan Broz @ 2019-06-08 9:11 UTC (permalink / raw)
To: Jaskaran Khurana, linux-security-module, linux-kernel,
linux-integrity, linux-fsdevel
Cc: agk, snitzer, dm-devel, jmorris, scottsh, ebiggers,
Mikulas Patocka
In-Reply-To: <20190607223140.16979-2-jaskarankhurana@linux.microsoft.com>
On 08/06/2019 00:31, Jaskaran Khurana wrote:
> The verification is to support cases where the roothash is not secured by
> Trusted Boot, UEFI Secureboot or similar technologies.
> One of the use cases for this is for dm-verity volumes mounted after boot,
> the root hash provided during the creation of the dm-verity volume has to
> be secure and thus in-kernel validation implemented here will be used
> before we trust the root hash and allow the block device to be created.
>
> The signature being provided for verification must verify the root hash and
> must be trusted by the builtin keyring for verification to succeed.
>
> The hash is added as a key of type "user" and the description is passed to
> the kernel so it can look it up and use it for verification.
>
> Adds DM_VERITY_VERIFY_ROOTHASH_SIG: roothash verification
> against the roothash signature file *if* specified, if signature file is
> specified verification must succeed prior to creation of device mapper
> block device.
>
> Adds DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE: roothash signature *must* be
> specified for all dm verity volumes and verification must succeed prior
> to creation of device mapper block device.
AFAIK there are tools that use dm-verity internally (some container
functions in systemd can recognize and check dm-verity partitions) and with
this option we will just kill possibility to use it without signature.
Anyway, this is up to Mike and Mikulas, I guess generic distros will not
set this option.
Some minor details below:
> diff --git a/drivers/md/Makefile b/drivers/md/Makefile
> index be7a6eb92abc..8a8c142bcfe1 100644
> --- a/drivers/md/Makefile
> +++ b/drivers/md/Makefile
> @@ -61,7 +61,7 @@ obj-$(CONFIG_DM_LOG_USERSPACE) += dm-log-userspace.o
> obj-$(CONFIG_DM_ZERO) += dm-zero.o
> obj-$(CONFIG_DM_RAID) += dm-raid.o
> obj-$(CONFIG_DM_THIN_PROVISIONING) += dm-thin-pool.o
> -obj-$(CONFIG_DM_VERITY) += dm-verity.o
> +obj-$(CONFIG_DM_VERITY) += dm-verity.o dm-verity-verify-sig.o
Why is this different from existing FEC extension?
FEC uses ifdefs in header to blind functions if config is not set.
ifeq ($(CONFIG_DM_VERITY_FEC),y)
dm-verity-objs += dm-verity-fec.o
endif
...
> diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
> new file mode 100644
> index 000000000000..1a889be76ede
> --- /dev/null
> +++ b/drivers/md/dm-verity-verify-sig.c
...
> + key = request_key(&key_type_user,
> + key_desc, NULL);
> + if (IS_ERR(key))
> + return PTR_ERR(key);
You will need dependence on keyring here (kernel can be configured without it),
try to compile it without CONFIG_KEYS selected.
I think it is ok that DM_VERITY_VERIFY_ROOTHASH_SIG can directly require CONFIG_KEYS.
(Add depends on CONFIG_KEYS in KConfig)
Also please increase minor version of dm-verity target when adding functions, something like
@@ -1175,7 +1175,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
static struct target_type verity_target = {
.name = "verity",
- .version = {1, 4, 0},
+ .version = {1, 5, 0},
Thanks,
Milan
^ permalink raw reply
* Re: [RFC PATCH v3 0/1] Add dm verity root hash pkcs7 sig validation.
From: Milan Broz @ 2019-06-08 8:46 UTC (permalink / raw)
To: Jaskaran Khurana, linux-security-module, linux-kernel,
linux-integrity, linux-fsdevel
Cc: agk, snitzer, dm-devel, jmorris, scottsh, ebiggers, mpatocka
In-Reply-To: <20190607223140.16979-1-jaskarankhurana@linux.microsoft.com>
On 08/06/2019 00:31, Jaskaran Khurana wrote:
> This patch set adds in-kernel pkcs7 signature checking for the roothash of
> the dm-verity hash tree.
> The verification is to support cases where the roothash is not secured by
> Trusted Boot, UEFI Secureboot or similar technologies.
...
> drivers/md/Kconfig | 23 ++++++
> drivers/md/Makefile | 2 +-
> drivers/md/dm-verity-target.c | 34 +++++++-
> drivers/md/dm-verity-verify-sig.c | 132 ++++++++++++++++++++++++++++++
> drivers/md/dm-verity-verify-sig.h | 30 +++++++
Please could you also modify Documentation/device-mapper/verity.txt and
describe the new table parameter?
It would be also nice to have a reference example how to configure it,
including how to create the signature file.
Milan
^ permalink raw reply
* [RFC PATCH v3 1/1] Add dm verity root hash pkcs7 sig validation
From: Jaskaran Khurana @ 2019-06-07 22:31 UTC (permalink / raw)
To: linux-security-module, linux-kernel, linux-integrity,
linux-fsdevel
Cc: agk, snitzer, dm-devel, jmorris, scottsh, ebiggers, mpatocka
In-Reply-To: <20190607223140.16979-1-jaskarankhurana@linux.microsoft.com>
The verification is to support cases where the roothash is not secured by
Trusted Boot, UEFI Secureboot or similar technologies.
One of the use cases for this is for dm-verity volumes mounted after boot,
the root hash provided during the creation of the dm-verity volume has to
be secure and thus in-kernel validation implemented here will be used
before we trust the root hash and allow the block device to be created.
The signature being provided for verification must verify the root hash and
must be trusted by the builtin keyring for verification to succeed.
The hash is added as a key of type "user" and the description is passed to
the kernel so it can look it up and use it for verification.
Adds DM_VERITY_VERIFY_ROOTHASH_SIG: roothash verification
against the roothash signature file *if* specified, if signature file is
specified verification must succeed prior to creation of device mapper
block device.
Adds DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE: roothash signature *must* be
specified for all dm verity volumes and verification must succeed prior
to creation of device mapper block device.
Signed-off-by: Jaskaran Khurana <jaskarankhurana@linux.microsoft.com>
---
drivers/md/Kconfig | 23 ++++++
drivers/md/Makefile | 2 +-
drivers/md/dm-verity-target.c | 34 +++++++-
drivers/md/dm-verity-verify-sig.c | 132 ++++++++++++++++++++++++++++++
drivers/md/dm-verity-verify-sig.h | 30 +++++++
5 files changed, 216 insertions(+), 5 deletions(-)
create mode 100644 drivers/md/dm-verity-verify-sig.c
create mode 100644 drivers/md/dm-verity-verify-sig.h
diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index db269a348b20..da4115753f25 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -489,6 +489,29 @@ config DM_VERITY
If unsure, say N.
+config DM_VERITY_VERIFY_ROOTHASH_SIG
+ def_bool n
+ bool "Verity data device root hash signature verification support"
+ depends on DM_VERITY
+ select SYSTEM_DATA_VERIFICATION
+ help
+ The device mapper target created by DM-VERITY can be validated if the
+ pre-generated tree of cryptographic checksums passed has a pkcs#7
+ signature file that can validate the roothash of the tree.
+
+ If unsure, say N.
+
+config DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE
+ def_bool n
+ bool "Forces all dm verity data device root hash should be signed"
+ depends on DM_VERITY_VERIFY_ROOTHASH_SIG
+ help
+ The device mapper target created by DM-VERITY will succeed only if the
+ pre-generated tree of cryptographic checksums passed also has a pkcs#7
+ signature file that can validate the roothash of the tree.
+
+ If unsure, say N.
+
config DM_VERITY_FEC
bool "Verity forward error correction support"
depends on DM_VERITY
diff --git a/drivers/md/Makefile b/drivers/md/Makefile
index be7a6eb92abc..8a8c142bcfe1 100644
--- a/drivers/md/Makefile
+++ b/drivers/md/Makefile
@@ -61,7 +61,7 @@ obj-$(CONFIG_DM_LOG_USERSPACE) += dm-log-userspace.o
obj-$(CONFIG_DM_ZERO) += dm-zero.o
obj-$(CONFIG_DM_RAID) += dm-raid.o
obj-$(CONFIG_DM_THIN_PROVISIONING) += dm-thin-pool.o
-obj-$(CONFIG_DM_VERITY) += dm-verity.o
+obj-$(CONFIG_DM_VERITY) += dm-verity.o dm-verity-verify-sig.o
obj-$(CONFIG_DM_CACHE) += dm-cache.o
obj-$(CONFIG_DM_CACHE_SMQ) += dm-cache-smq.o
obj-$(CONFIG_DM_ERA) += dm-era.o
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index f4c31ffaa88e..5a2f84d5bd8a 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -16,7 +16,7 @@
#include "dm-verity.h"
#include "dm-verity-fec.h"
-
+#include "dm-verity-verify-sig.h"
#include <linux/module.h>
#include <linux/reboot.h>
@@ -34,7 +34,8 @@
#define DM_VERITY_OPT_IGN_ZEROES "ignore_zero_blocks"
#define DM_VERITY_OPT_AT_MOST_ONCE "check_at_most_once"
-#define DM_VERITY_OPTS_MAX (2 + DM_VERITY_OPTS_FEC)
+#define DM_VERITY_OPTS_MAX (2 + DM_VERITY_OPTS_FEC + \
+ DM_VERITY_ROOT_HASH_VERIFICATION_OPTS)
static unsigned dm_verity_prefetch_cluster = DM_VERITY_DEFAULT_PREFETCH_SIZE;
@@ -855,7 +856,8 @@ static int verity_alloc_zero_digest(struct dm_verity *v)
return r;
}
-static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
+static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
+ struct dm_verity_sig_opts *verify_args)
{
int r;
unsigned argc;
@@ -904,6 +906,14 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
if (r)
return r;
continue;
+ } else if (verity_verify_is_sig_opt_arg(arg_name)) {
+ r = verity_verify_sig_parse_opt_args(as, v,
+ verify_args,
+ &argc, arg_name);
+ if (r)
+ return r;
+ continue;
+
}
ti->error = "Unrecognized verity feature request";
@@ -930,6 +940,7 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
{
struct dm_verity *v;
+ struct dm_verity_sig_opts verify_args = {0};
struct dm_arg_set as;
unsigned int num;
unsigned long long num_ll;
@@ -937,6 +948,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
int i;
sector_t hash_position;
char dummy;
+ char *root_hash_digest_to_validate;
v = kzalloc(sizeof(struct dm_verity), GFP_KERNEL);
if (!v) {
@@ -1070,6 +1082,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
r = -EINVAL;
goto bad;
}
+ root_hash_digest_to_validate = argv[8];
if (strcmp(argv[9], "-")) {
v->salt_size = strlen(argv[9]) / 2;
@@ -1095,11 +1108,20 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
as.argc = argc;
as.argv = argv;
- r = verity_parse_opt_args(&as, v);
+ r = verity_parse_opt_args(&as, v, &verify_args);
if (r < 0)
goto bad;
}
+ /* Root hash signature is a optional parameter*/
+ r = verity_verify_root_hash(root_hash_digest_to_validate,
+ strlen(root_hash_digest_to_validate),
+ verify_args.sig,
+ verify_args.sig_size);
+ if (r < 0) {
+ ti->error = "Root hash verification failed";
+ goto bad;
+ }
v->hash_per_block_bits =
__fls((1 << v->hash_dev_block_bits) / v->digest_size);
@@ -1165,9 +1187,13 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
ti->per_io_data_size = roundup(ti->per_io_data_size,
__alignof__(struct dm_verity_io));
+ verity_verify_sig_opts_cleanup(&verify_args);
+
return 0;
bad:
+
+ verity_verify_sig_opts_cleanup(&verify_args);
verity_dtr(ti);
return r;
diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
new file mode 100644
index 000000000000..1a889be76ede
--- /dev/null
+++ b/drivers/md/dm-verity-verify-sig.c
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Microsoft Corporation.
+ *
+ * Author: Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
+ *
+ */
+#include <linux/device-mapper.h>
+#include <linux/verification.h>
+#include <keys/user-type.h>
+#include "dm-verity.h"
+#include "dm-verity-verify-sig.h"
+
+#define DM_VERITY_VERIFY_ERR(s) DM_VERITY_ROOT_HASH_VERIFICATION " " s
+
+
+bool verity_verify_is_sig_opt_arg(const char *arg_name)
+{
+ return (!strcasecmp(arg_name,
+ DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY));
+}
+
+static int verity_verify_get_sig_from_key(const char *key_desc,
+ struct dm_verity_sig_opts *sig_opts)
+{
+ struct key *key;
+ const struct user_key_payload *ukp;
+ int ret = 0;
+
+ if (!IS_ENABLED(CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG))
+ return 0;
+
+ key = request_key(&key_type_user,
+ key_desc, NULL);
+ if (IS_ERR(key))
+ return PTR_ERR(key);
+
+ down_read(&key->sem);
+
+ ukp = user_key_payload_locked(key);
+ if (!ukp) {
+ ret = -EKEYREVOKED;
+ goto end;
+ }
+
+ sig_opts->sig = kmalloc(ukp->datalen, GFP_KERNEL);
+ if (!sig_opts->sig) {
+ ret = -ENOMEM;
+ goto end;
+ }
+ sig_opts->sig_size = ukp->datalen;
+
+ memcpy(sig_opts->sig, ukp->data, sig_opts->sig_size);
+
+end:
+ up_read(&key->sem);
+ key_put(key);
+
+ return ret;
+}
+
+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as,
+ struct dm_verity *v,
+ struct dm_verity_sig_opts *sig_opts,
+ unsigned int *argc,
+ const char *arg_name)
+{
+ struct dm_target *ti = v->ti;
+ int ret = 0;
+ const char *sig_key = NULL;
+
+ if (!*argc) {
+ ti->error = DM_VERITY_VERIFY_ERR("Signature key not specified");
+ return -EINVAL;
+ }
+
+ sig_key = dm_shift_arg(as);
+ (*argc)--;
+
+ ret = verity_verify_get_sig_from_key(sig_key, sig_opts);
+ if (ret < 0)
+ ti->error = DM_VERITY_VERIFY_ERR("Invalid key specified");
+
+ return ret;
+}
+
+#ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG
+/*
+ * verify_verify_roothash - Verify the root hash of the verity hash device
+ * using builtin trusted keys.
+ *
+ * @root_hash: For verity, the roothash/data to be verified.
+ * @root_hash_len: Size of the roothash/data to be verified.
+ * @sig_data: The trusted signature that verifies the roothash/data.
+ * @sig_len: Size of the signature.
+ *
+ */
+int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
+ const void *sig_data, size_t sig_len)
+{
+ int ret;
+
+ if (!root_hash || root_hash_len == 0)
+ return -EINVAL;
+
+ if (!sig_data || sig_len == 0) {
+ if (IS_ENABLED(CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE))
+ return -ENOKEY;
+ else
+ return 0;
+ }
+
+ ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
+ sig_len, NULL, VERIFYING_UNSPECIFIED_SIGNATURE,
+ NULL, NULL);
+
+ return ret;
+}
+#else
+int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
+ const void *sig_data, size_t sig_len)
+{
+ return 0;
+}
+#endif
+
+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts)
+{
+ kfree(sig_opts->sig);
+ sig_opts->sig = NULL;
+ sig_opts->sig_size = 0;
+}
diff --git a/drivers/md/dm-verity-verify-sig.h b/drivers/md/dm-verity-verify-sig.h
new file mode 100644
index 000000000000..339818e6b527
--- /dev/null
+++ b/drivers/md/dm-verity-verify-sig.h
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Microsoft Corporation.
+ *
+ * Author: Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
+ *
+ */
+#ifndef DM_VERITY_SIG_VERIFICATION_H
+#define DM_VERITY_SIG_VERIFICATION_H
+
+#define DM_VERITY_ROOT_HASH_VERIFICATION "DM Verity Sig Verification"
+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY "root_hash_sig_key_desc"
+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPTS 2
+
+struct dm_verity_sig_opts {
+ unsigned int sig_size;
+ u8 *sig;
+};
+int verity_verify_root_hash(const void *data, size_t data_len,
+ const void *sig_data, size_t sig_len);
+
+bool verity_verify_is_sig_opt_arg(const char *arg_name);
+
+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
+ struct dm_verity_sig_opts *sig_opts,
+ unsigned int *argc, const char *arg_name);
+
+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts);
+
+#endif /* DM_VERITY_SIG_VERIFICATION_H */
--
2.17.1
^ permalink raw reply related
* [RFC PATCH v3 0/1] Add dm verity root hash pkcs7 sig validation.
From: Jaskaran Khurana @ 2019-06-07 22:31 UTC (permalink / raw)
To: linux-security-module, linux-kernel, linux-integrity,
linux-fsdevel
Cc: agk, snitzer, dm-devel, jmorris, scottsh, ebiggers, mpatocka
This patch set adds in-kernel pkcs7 signature checking for the roothash of
the dm-verity hash tree.
The verification is to support cases where the roothash is not secured by
Trusted Boot, UEFI Secureboot or similar technologies.
One of the use cases for this is for dm-verity volumes mounted after boot,
the root hash provided during the creation of the dm-verity volume has to
be secure and thus in-kernel validation implemented here will be used
before we trust the root hash and allow the block device to be created.
Why we are doing validation in the Kernel?
The reason is to still be secure in cases where the attacker is able to
compromise the user mode application in which case the user mode validation
could not have been trusted.
The root hash signature validation in the kernel along with existing
dm-verity implementation gives a higher level of confidence in the
executable code or the protected data. Before allowing the creation of
the device mapper block device the kernel code will check that the detached
pkcs7 signature passed to it validates the roothash and the signature is
trusted by builtin keys set at kernel creation. The kernel should be
secured using Verified boot, UEFI Secure Boot or similar technologies so we
can trust it.
What about attacker mounting non dm-verity volumes to run executable
code?
This verification can be used to have a security architecture where a LSM
can enforce this verification for all the volumes and by doing this it can
ensure that all executable code runs from signed and trusted dm-verity
volumes.
Further patches will be posted that build on this and enforce this
verification based on policy for all the volumes on the system.
How are these changes tested?
veritysetup part of cryptsetup library was modified to take a optional
root-hash-sig parameter.
Commandline used to test the changes:
veritysetup open <data_device> <name> <hash_device> <root_hash>
--root-hash-sig=<root_hash_pkcs7_detached_sig>
The changes for veritysetup are in a topic branch for now at:
https://github.com/jaskarankhurana/veritysetup/tree/veritysetup_add_sig
Changelog:
v3:
- Code review feedback given by Sasha Levin.
- Removed EXPORT_SYMBOL_GPL since this was not required.
- Removed "This file is released under the GPLv2" since we have SPDX
identifier.
- Inside verity_verify_root_hash changed EINVAL to ENOKEY when the key
descriptor is not specified but due to force option being set it is
expected.
- Moved CONFIG check to inside verity_verify_get_sig_from_key.
(Did not move the sig_opts_cleanup to inside verity_dtr as the
sig_opts do not need to be allocated for the entire duration the block
device is active unlike the verity structure, note verity_dtr is called
only if verity_ctr fails or after the lifetime of the block device.)
v2:
- Code review feedback to pass the signature binary blob as a key that can be
Jaskaran Khurana (1):
Adds in-kernel pkcs7 sig checking the roothash of the dm-verity hash
tree
drivers/md/Kconfig | 23 ++++++
drivers/md/Makefile | 2 +-
drivers/md/dm-verity-target.c | 34 +++++++-
drivers/md/dm-verity-verify-sig.c | 132 ++++++++++++++++++++++++++++++
drivers/md/dm-verity-verify-sig.h | 30 +++++++
5 files changed, 216 insertions(+), 5 deletions(-)
create mode 100644 drivers/md/dm-verity-verify-sig.c
create mode 100644 drivers/md/dm-verity-verify-sig.h
--
2.17.1
^ permalink raw reply
* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
From: James Morris @ 2019-06-07 21:48 UTC (permalink / raw)
To: Paul Moore
Cc: Casey Schaufler, Janne Karhunen, Stephen Smalley, Mimi Zohar,
linux-integrity, linux-security-module
In-Reply-To: <CAHC9VhSrjVmnsAuXDmHVmsyDaEF10nsvdxq7VsfCsh=NfaOMQg@mail.gmail.com>
On Fri, 7 Jun 2019, Paul Moore wrote:
> On Thu, Jun 6, 2019 at 8:45 PM James Morris <jmorris@namei.org> wrote:
> > On Wed, 5 Jun 2019, Paul Moore wrote:
> > > On Wed, Jun 5, 2019 at 1:05 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > On 6/5/2019 9:51 AM, Janne Karhunen wrote:
> > > >
> > > > One hook with an added "bool blocking" argument, if
> > > > that's the only difference?
> > >
> > > I think there is value in keeping a similar convention to the notifier
> > > code on which this is based, see include/linux/notifier.h.
> >
> > Although this doesn't seem to be what other users in the kernel are doing.
>
> How many of them potentially have the need for both blocking and
> non-blocking notifiers? I didn't go through the entire list of
> callers, but it seems all that I looked at used only one type. The
> simple fact that we started with one type of notifier for the LSM, and
> we are now switching to the other (and getting lucky that it is safe
> to do so for the existing callers) seems to lend some weight to the
> argument we may need both and adding "block"/"blocking"/etc. to the
> name has value.
Fair enough.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [RFC PATCH v2 5/5] security/selinux: Add enclave_load() implementation
From: Stephen Smalley @ 2019-06-07 21:16 UTC (permalink / raw)
To: Sean Christopherson, Jarkko Sakkinen
Cc: Andy Lutomirski, Cedric Xing, James Morris, Serge E . Hallyn,
LSM List, Paul Moore, Eric Paris, selinux, Jethro Beekman,
Dave Hansen, Thomas Gleixner, Linus Torvalds, LKML, X86 ML,
linux-sgx, Andrew Morton, nhorman, npmccallum, Serge Ayoun,
Shay Katz-zamir, Haitao Huang, Andy Shevchenko, Kai Svahn,
Borislav Petkov, Josh Triplett, Kai Huang, David Rientjes,
William Roberts, Philip Tricca
In-Reply-To: <20190606021145.12604-6-sean.j.christopherson@intel.com>
On 6/5/19 10:11 PM, Sean Christopherson wrote:
> The goal of selinux_enclave_load() is to provide a facsimile of the
> existing selinux_file_mprotect() and file_map_prot_check() policies,
> but tailored to the unique properties of SGX.
>
> For example, an enclave page is technically backed by a MAP_SHARED file,
> but the "file" is essentially shared memory that is never persisted
> anywhere and also requires execute permissions (for some pages).
>
> The basic concept is to require appropriate execute permissions on the
> source of the enclave for pages that are requesting PROT_EXEC, e.g. if
> an enclave page is being loaded from a regular file, require
> FILE__EXECUTE and/or FILE__EXECMOND, and if it's coming from an
> anonymous/private mapping, require PROCESS__EXECMEM since the process
> is essentially executing from the mapping, albeit in a roundabout way.
>
> Note, FILE__READ and FILE__WRITE are intentionally not required even if
> the source page is backed by a regular file. Writes to the enclave page
> are contained to the EPC, i.e. never hit the original file, and read
> permissions have already been vetted (or the VMA doesn't have PROT_READ,
> in which case loading the page into the enclave will fail).
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> security/selinux/hooks.c | 69 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 69 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3ec702cf46ca..3c5418edf51c 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6726,6 +6726,71 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
> }
> #endif
>
> +#ifdef CONFIG_INTEL_SGX
> +int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot)
> +{
> + const struct cred *cred = current_cred();
> + u32 sid = cred_sid(cred);
> + int ret;
> +
> + /* SGX is supported only in 64-bit kernels. */
> + WARN_ON_ONCE(!default_noexec);
> +
> + /* Only executable enclave pages are restricted in any way. */
> + if (!(prot & PROT_EXEC))
> + return 0;
prot/PROT_EXEC or vmflags/VM_EXEC
> +
> + /*
> + * The source page is exectuable, i.e. has already passed SELinux's
executable
> + * checks, and userspace is not requesting RW->RX capabilities.
Is it requesting W->X or WX?
> + */
> + if ((vma->vm_flags & VM_EXEC) && !(prot & PROT_WRITE))
> + return 0;
> +
> + /*
> + * The source page is not executable, or userspace is requesting the
> + * ability to do a RW->RX conversion. Permissions are required as
> + * follows, in order of increasing privelege:
> + *
> + * EXECUTE - Load an executable enclave page without RW->RX intent from
> + * a non-executable vma that is backed by a shared mapping to
> + * a regular file that has not undergone COW.
Shared mapping or unmodified private file mapping
> + *
> + * EXECMOD - Load an executable enclave page without RW->RX intent from
> + * a non-executable vma that is backed by a shared mapping to
> + * a regular file that *has* undergone COW.
modified private file mapping (write to shared mapping won't trigger
COW; it would have been checked by FILE__WRITE earlier)
> + *
> + * - Load an enclave page *with* RW->RX intent from a shared
> + * mapping to a regular file.
> + *
> + * EXECMEM - Load an exectuable enclave page from an anonymous mapping.
executable
> + *
> + * - Load an exectuable enclave page from a private file, e.g.
executable
> + * from a shared mapping to a hugetlbfs file.
> + *
> + * - Load an enclave page *with* RW->RX intent from a private
W->X or WX?
> + * mapping to a regular file.
> + *
> + * Note, this hybrid EXECMOD and EXECMEM behavior is intentional and
> + * reflects the nature of enclaves and the EPC, e.g. EPC is effectively
> + * a non-persistent shared file, but each enclave is a private domain
> + * within that shared file, so delegate to the source of the enclave.
> + */
> + if (vma->vm_file && !IS_PRIVATE(file_inode(vma->vm_file) &&
> + ((vma->vm_flags & VM_SHARED) || !(prot & PROT_WRITE)))) {
> + if (!vma->anon_vma && !(prot & PROT_WRITE))
> + ret = file_has_perm(cred, vma->vm_file, FILE__EXECUTE);
> + else
> + ret = file_has_perm(cred, vma->vm_file, FILE__EXECMOD);
> + } else {
> + ret = avc_has_perm(&selinux_state,
> + sid, sid, SECCLASS_PROCESS,
> + PROCESS__EXECMEM, NULL);
> + }
> + return ret;
> +}
> +#endif
> +
> struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
> .lbs_cred = sizeof(struct task_security_struct),
> .lbs_file = sizeof(struct file_security_struct),
> @@ -6968,6 +7033,10 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
> LSM_HOOK_INIT(bpf_prog_free_security, selinux_bpf_prog_free),
> #endif
> +
> +#ifdef CONFIG_INTEL_SGX
> + LSM_HOOK_INIT(enclave_load, selinux_enclave_load),
> +#endif
> };
>
> static __init int selinux_init(void)
>
^ permalink raw reply
* Re: [RFC PATCH v2 4/5] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
From: Stephen Smalley @ 2019-06-07 19:58 UTC (permalink / raw)
To: Sean Christopherson, Jarkko Sakkinen
Cc: Andy Lutomirski, Cedric Xing, James Morris, Serge E . Hallyn,
LSM List, Paul Moore, Eric Paris, selinux, Jethro Beekman,
Dave Hansen, Thomas Gleixner, Linus Torvalds, LKML, X86 ML,
linux-sgx, Andrew Morton, nhorman, npmccallum, Serge Ayoun,
Shay Katz-zamir, Haitao Huang, Andy Shevchenko, Kai Svahn,
Borislav Petkov, Josh Triplett, Kai Huang, David Rientjes,
William Roberts, Philip Tricca
In-Reply-To: <20190606021145.12604-5-sean.j.christopherson@intel.com>
On 6/5/19 10:11 PM, Sean Christopherson wrote:
> enclave_load() is roughly analogous to the existing file_mprotect().
>
> Due to the nature of SGX and its Enclave Page Cache (EPC), all enclave
> VMAs are backed by a single file, i.e. /dev/sgx/enclave, that must be
> MAP_SHARED. Furthermore, all enclaves need read, write and execute
> VMAs. As a result, the existing/standard call to file_mprotect() does
> not provide any meaningful security for enclaves since an LSM can only
> deny/grant access to the EPC as a whole.
>
> security_enclave_load() is called when SGX is first loading an enclave
> page, i.e. copying a page from normal memory into the EPC. Although
> the prototype for enclave_load() is similar to file_mprotect(), e.g.
> SGX could theoretically use file_mprotect() and set reqprot=prot, a
> separate hook is desirable as the semantics of an enclave's protection
> bits are different than those of vmas, e.g. an enclave page tracks the
> maximal set of protections, whereas file_mprotect() operates on the
> actual protections being provided. In other words, LSMs will likely
> want to implement different policies for enclave page protections.
>
> Note, extensive discussion yielded no sane alternative to some form of
> SGX specific LSM hook[1].
>
> [1] https://lkml.kernel.org/r/CALCETrXf8mSK45h7sTK5Wf+pXLVn=Bjsc_RLpgO-h-qdzBRo5Q@mail.gmail.com
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> arch/x86/kernel/cpu/sgx/driver/ioctl.c | 12 ++++++------
> include/linux/lsm_hooks.h | 13 +++++++++++++
> include/linux/security.h | 12 ++++++++++++
> security/security.c | 7 +++++++
> 4 files changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index 44b2d73de7c3..29c0df672250 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -8,6 +8,7 @@
> #include <linux/highmem.h>
> #include <linux/ratelimit.h>
> #include <linux/sched/signal.h>
> +#include <linux/security.h>
> #include <linux/shmem_fs.h>
> #include <linux/slab.h>
> #include <linux/suspend.h>
> @@ -582,9 +583,6 @@ static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot)
> struct vm_area_struct *vma;
> int ret;
>
> - if (!(prot & VM_EXEC))
> - return 0;
> -
Is there a real use case where LSM will want to be called if !(prot &
VM_EXEC)? Also, you seem to be mixing prot and PROT_EXEC with vm_flags
and VM_EXEC; other code does not appear to assume they are identical and
explicitly converts, e.g. calc_vm_prot_bits().
> /* Hold mmap_sem across copy_from_user() to avoid a TOCTOU race. */
> down_read(¤t->mm->mmap_sem);
>
> @@ -599,15 +597,17 @@ static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot)
> * but with some future proofing against other cases that may deny
> * execute permissions.
> */
> - if (!(vma->vm_flags & VM_MAYEXEC)) {
> + if ((prot & VM_EXEC) && !(vma->vm_flags & VM_MAYEXEC)) {
> ret = -EACCES;
> goto out;
> }
>
> + ret = security_enclave_load(vma, prot);
> + if (ret)
> + goto out;
> +
> if (copy_from_user(dst, (void __user *)src, PAGE_SIZE))
> ret = -EFAULT;
> - else
> - ret = 0;
>
> out:
> up_read(¤t->mm->mmap_sem);
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 47f58cfb6a19..c6f47a7eef70 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1446,6 +1446,12 @@
> * @bpf_prog_free_security:
> * Clean up the security information stored inside bpf prog.
> *
> + * Security hooks for Intel SGX enclaves.
> + *
> + * @enclave_load:
> + * @vma: the source memory region of the enclave page being loaded.
> + * @prot: the (maximal) protections of the enclave page.
> + * Return 0 if permission is granted.
> */
> union security_list_options {
> int (*binder_set_context_mgr)(struct task_struct *mgr);
> @@ -1807,6 +1813,10 @@ union security_list_options {
> int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
> void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
> #endif /* CONFIG_BPF_SYSCALL */
> +
> +#ifdef CONFIG_INTEL_SGX
> + int (*enclave_load)(struct vm_area_struct *vma, unsigned long prot);
> +#endif /* CONFIG_INTEL_SGX */
> };
>
> struct security_hook_heads {
> @@ -2046,6 +2056,9 @@ struct security_hook_heads {
> struct hlist_head bpf_prog_alloc_security;
> struct hlist_head bpf_prog_free_security;
> #endif /* CONFIG_BPF_SYSCALL */
> +#ifdef CONFIG_INTEL_SGX
> + struct hlist_head enclave_load;
> +#endif /* CONFIG_INTEL_SGX */
> } __randomize_layout;
>
> /*
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 659071c2e57c..0b6d1eb7368b 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1829,5 +1829,17 @@ static inline void security_bpf_prog_free(struct bpf_prog_aux *aux)
> #endif /* CONFIG_SECURITY */
> #endif /* CONFIG_BPF_SYSCALL */
>
> +#ifdef CONFIG_INTEL_SGX
> +#ifdef CONFIG_SECURITY
> +int security_enclave_load(struct vm_area_struct *vma, unsigned long prot);
> +#else
> +static inline int security_enclave_load(struct vm_area_struct *vma,
> + unsigned long prot)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_SECURITY */
> +#endif /* CONFIG_INTEL_SGX */
> +
> #endif /* ! __LINUX_SECURITY_H */
>
> diff --git a/security/security.c b/security/security.c
> index 613a5c00e602..c6f7f26969b2 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2359,3 +2359,10 @@ void security_bpf_prog_free(struct bpf_prog_aux *aux)
> call_void_hook(bpf_prog_free_security, aux);
> }
> #endif /* CONFIG_BPF_SYSCALL */
> +
> +#ifdef CONFIG_INTEL_SGX
> +int security_enclave_load(struct vm_area_struct *vma, unsigned long prot)
> +{
> + return call_int_hook(enclave_load, 0, vma, prot);
> +}
> +#endif /* CONFIG_INTEL_SGX */
>
^ permalink raw reply
* Re: [PATCH 1/1 v2] Add dm verity root hash pkcs7 sig validation.
From: Sasha Levin @ 2019-06-07 19:36 UTC (permalink / raw)
To: Jaskaran Khurana
Cc: linux-security-module, linux-kernel, agk, snitzer, dm-devel,
jmorris, scottsh
In-Reply-To: <20190524230411.9238-2-jaskarankhurana@linux.microsoft.com>
On Fri, May 24, 2019 at 04:04:11PM -0700, Jaskaran Khurana wrote:
>The verification is to support cases where the roothash is not secured by
>Trusted Boot, UEFI Secureboot or similar technologies.
>One of the use cases for this is for dm-verity volumes mounted after boot,
>the root hash provided during the creation of the dm-verity volume has to
>be secure and thus in-kernel validation implemented here will be used
>before we trust the root hash and allow the block device to be created.
>
>The signature being provided for verification must verify the root hash and
>must be trusted by the builtin keyring for verification to succeed.
>
>The hash is added as a key of type "user" and the description is passed to
>the kernel so it can look it up and use it for verification.
>
>Adds DM_VERITY_VERIFY_ROOTHASH_SIG: roothash verification
>against the roothash signature file *if* specified, if signature file is
>specified verification must succeed prior to creation of device mapper
>block device.
>
>Adds DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE: roothash signature *must* be
>specified for all dm verity volumes and verification must succeed prior
>to creation of device mapper block device.
>
>Signed-off-by: Jaskaran Khurana <jaskarankhurana@linux.microsoft.com>
>---
> drivers/md/Kconfig | 23 +++++
> drivers/md/Makefile | 2 +-
> drivers/md/dm-verity-target.c | 34 +++++++-
> drivers/md/dm-verity-verify-sig.c | 137 ++++++++++++++++++++++++++++++
> drivers/md/dm-verity-verify-sig.h | 31 +++++++
> 5 files changed, 222 insertions(+), 5 deletions(-)
> create mode 100644 drivers/md/dm-verity-verify-sig.c
> create mode 100644 drivers/md/dm-verity-verify-sig.h
>
>diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
>index db269a348b20..da4115753f25 100644
>--- a/drivers/md/Kconfig
>+++ b/drivers/md/Kconfig
>@@ -489,6 +489,29 @@ config DM_VERITY
>
> If unsure, say N.
>
>+config DM_VERITY_VERIFY_ROOTHASH_SIG
>+ def_bool n
>+ bool "Verity data device root hash signature verification support"
>+ depends on DM_VERITY
>+ select SYSTEM_DATA_VERIFICATION
>+ help
>+ The device mapper target created by DM-VERITY can be validated if the
>+ pre-generated tree of cryptographic checksums passed has a pkcs#7
>+ signature file that can validate the roothash of the tree.
>+
>+ If unsure, say N.
>+
>+config DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE
>+ def_bool n
>+ bool "Forces all dm verity data device root hash should be signed"
>+ depends on DM_VERITY_VERIFY_ROOTHASH_SIG
>+ help
>+ The device mapper target created by DM-VERITY will succeed only if the
>+ pre-generated tree of cryptographic checksums passed also has a pkcs#7
>+ signature file that can validate the roothash of the tree.
>+
>+ If unsure, say N.
>+
> config DM_VERITY_FEC
> bool "Verity forward error correction support"
> depends on DM_VERITY
>diff --git a/drivers/md/Makefile b/drivers/md/Makefile
>index be7a6eb92abc..8a8c142bcfe1 100644
>--- a/drivers/md/Makefile
>+++ b/drivers/md/Makefile
>@@ -61,7 +61,7 @@ obj-$(CONFIG_DM_LOG_USERSPACE) += dm-log-userspace.o
> obj-$(CONFIG_DM_ZERO) += dm-zero.o
> obj-$(CONFIG_DM_RAID) += dm-raid.o
> obj-$(CONFIG_DM_THIN_PROVISIONING) += dm-thin-pool.o
>-obj-$(CONFIG_DM_VERITY) += dm-verity.o
>+obj-$(CONFIG_DM_VERITY) += dm-verity.o dm-verity-verify-sig.o
> obj-$(CONFIG_DM_CACHE) += dm-cache.o
> obj-$(CONFIG_DM_CACHE_SMQ) += dm-cache-smq.o
> obj-$(CONFIG_DM_ERA) += dm-era.o
>diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
>index f4c31ffaa88e..56276669ac20 100644
>--- a/drivers/md/dm-verity-target.c
>+++ b/drivers/md/dm-verity-target.c
>@@ -16,7 +16,7 @@
>
> #include "dm-verity.h"
> #include "dm-verity-fec.h"
>-
>+#include "dm-verity-verify-sig.h"
> #include <linux/module.h>
> #include <linux/reboot.h>
>
>@@ -34,7 +34,8 @@
> #define DM_VERITY_OPT_IGN_ZEROES "ignore_zero_blocks"
> #define DM_VERITY_OPT_AT_MOST_ONCE "check_at_most_once"
>
>-#define DM_VERITY_OPTS_MAX (2 + DM_VERITY_OPTS_FEC)
>+#define DM_VERITY_OPTS_MAX (2 + DM_VERITY_OPTS_FEC + \
>+ DM_VERITY_ROOT_HASH_VERIFICATION_OPTS)
>
> static unsigned dm_verity_prefetch_cluster = DM_VERITY_DEFAULT_PREFETCH_SIZE;
>
>@@ -855,7 +856,8 @@ static int verity_alloc_zero_digest(struct dm_verity *v)
> return r;
> }
>
>-static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
>+static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
>+ struct dm_verity_sig_opts *verify_args)
> {
> int r;
> unsigned argc;
>@@ -904,6 +906,14 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
> if (r)
> return r;
> continue;
>+ } else if (verity_verify_is_sig_opt_arg(arg_name)) {
>+ r = verity_verify_sig_parse_opt_args(as, v,
>+ verify_args,
>+ &argc, arg_name);
>+ if (r)
>+ return r;
>+ continue;
>+
> }
>
> ti->error = "Unrecognized verity feature request";
>@@ -930,6 +940,7 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
> static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
> {
> struct dm_verity *v;
>+ struct dm_verity_sig_opts verify_args = {0};
> struct dm_arg_set as;
> unsigned int num;
> unsigned long long num_ll;
>@@ -937,6 +948,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
> int i;
> sector_t hash_position;
> char dummy;
>+ char *root_hash_digest_to_validate = NULL;
Does it need to be initialized here? There's nothing that relies on this
logic later.
> v = kzalloc(sizeof(struct dm_verity), GFP_KERNEL);
> if (!v) {
>@@ -1070,6 +1082,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
> r = -EINVAL;
> goto bad;
> }
>+ root_hash_digest_to_validate = argv[8];
>
> if (strcmp(argv[9], "-")) {
> v->salt_size = strlen(argv[9]) / 2;
>@@ -1095,11 +1108,20 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
> as.argc = argc;
> as.argv = argv;
>
>- r = verity_parse_opt_args(&as, v);
>+ r = verity_parse_opt_args(&as, v, &verify_args);
> if (r < 0)
> goto bad;
> }
>
>+ /* Root hash signature is a optional parameter*/
an
>+ r = verity_verify_root_hash(root_hash_digest_to_validate,
>+ strlen(root_hash_digest_to_validate),
>+ verify_args.sig,
>+ verify_args.sig_size);
>+ if (r < 0) {
>+ ti->error = "Root hash verification failed";
>+ goto bad;
>+ }
> v->hash_per_block_bits =
> __fls((1 << v->hash_dev_block_bits) / v->digest_size);
>
>@@ -1165,9 +1187,13 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
> ti->per_io_data_size = roundup(ti->per_io_data_size,
> __alignof__(struct dm_verity_io));
>
>+ verity_verify_sig_opts_cleanup(&verify_args);
>+
> return 0;
>
> bad:
>+
>+ verity_verify_sig_opts_cleanup(&verify_args);
> verity_dtr(ti);
>
> return r;
>diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
>new file mode 100644
>index 000000000000..ba87c9342d55
>--- /dev/null
>+++ b/drivers/md/dm-verity-verify-sig.c
>@@ -0,0 +1,137 @@
>+// SPDX-License-Identifier: GPL-2.0
>+/*
>+ * Copyright (C) 2019 Microsoft Corporation.
>+ *
>+ * Author: Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
>+ *
>+ * This file is released under the GPLv2.
There's no need to explicitly state licensing here, we have the SPDX
line at the beginning for that.
>+ */
>+#include <linux/device-mapper.h>
>+#include <linux/verification.h>
>+#include <keys/user-type.h>
>+#include "dm-verity.h"
>+#include "dm-verity-verify-sig.h"
>+
>+#define DM_VERITY_VERIFY_ERR(s) DM_VERITY_ROOT_HASH_VERIFICATION " " s
>+
>+
>+bool verity_verify_is_sig_opt_arg(const char *arg_name)
>+{
>+ return (!strcasecmp(arg_name,
>+ DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY));
>+}
>+EXPORT_SYMBOL_GPL(verity_verify_is_sig_opt_arg);
Why are you exporting all these symbols?
>+static int verity_verify_get_sig_from_key(const char *key_desc,
>+ struct dm_verity_sig_opts *sig_opts)
>+{
>+ struct key *key;
>+ const struct user_key_payload *ukp;
>+ int ret = 0;
>+
>+ key = request_key(&key_type_user,
>+ key_desc, NULL);
>+ if (IS_ERR(key))
>+ return PTR_ERR(key);
>+
>+ down_read(&key->sem);
>+
>+ ukp = user_key_payload_locked(key);
>+ if (!ukp) {
>+ ret = -EKEYREVOKED;
>+ goto end;
>+ }
>+
>+ sig_opts->sig = kmalloc(ukp->datalen, GFP_KERNEL);
>+ if (!sig_opts->sig) {
>+ ret = -ENOMEM;
>+ goto end;
>+ }
>+ sig_opts->sig_size = ukp->datalen;
>+
>+ memcpy(sig_opts->sig, ukp->data, sig_opts->sig_size);
>+
>+end:
>+ up_read(&key->sem);
>+ key_put(key);
>+
>+ return ret;
>+}
>+
>+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as,
>+ struct dm_verity *v,
>+ struct dm_verity_sig_opts *sig_opts,
>+ unsigned int *argc,
>+ const char *arg_name)
>+{
>+ struct dm_target *ti = v->ti;
>+ int ret = 0;
>+ const char *sig_key = NULL;
>+
>+ if (!*argc) {
>+ ti->error = DM_VERITY_VERIFY_ERR("Signature key not specified");
>+ return -EINVAL;
>+ }
>+
>+ sig_key = dm_shift_arg(as);
>+ (*argc)--;
>+
>+ if (!IS_ENABLED(CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG))
>+ return 0;
Do we need to explicitly check it here? It would be nicer if we just
rely on verity_verify_get_sig_from_key() to "succeed" if the config
option isn't set.
>+
>+ ret = verity_verify_get_sig_from_key(sig_key, sig_opts);
>+ if (ret < 0)
>+ ti->error = DM_VERITY_VERIFY_ERR("Invalid key specified");
>+
>+ return ret;
>+}
>+EXPORT_SYMBOL_GPL(verity_verify_sig_parse_opt_args);
>+
>+#ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG
>+/*
>+ * verify_verify_roothash - Verify the root hash of the verity hash device
>+ * using builtin trusted keys.
>+ *
>+ * @root_hash: For verity, the roothash/data to be verified.
>+ * @root_hash_len: Size of the roothash/data to be verified.
>+ * @sig_data: The trusted signature that verifies the roothash/data.
>+ * @sig_len: Size of the signature.
>+ *
>+ */
>+int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
>+ const void *sig_data, size_t sig_len)
>+{
>+ int ret;
>+
>+ if (!root_hash || root_hash_len == 0)
>+ return -EINVAL;
>+
>+ if (!sig_data || sig_len == 0) {
>+ if (IS_ENABLED(CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE))
>+ return -EINVAL;
Is -EINVAL the right error here?
>+ else
>+ return 0;
>+ }
>+
>+ ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
>+ sig_len, NULL, VERIFYING_UNSPECIFIED_SIGNATURE,
>+ NULL, NULL);
>+
>+ return ret;
>+}
>+#else
>+int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
>+ const void *sig_data, size_t sig_len)
>+{
>+ return 0;
>+}
>+#endif
>+EXPORT_SYMBOL_GPL(verity_verify_root_hash);
>+
>+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts)
Why doesn't all of this cleanup code live in verity_dtr()?
--
Thanks,
Sasha
>+{
>+ kfree(sig_opts->sig);
>+ sig_opts->sig = NULL;
>+ sig_opts->sig_size = 0;
>+}
>+EXPORT_SYMBOL_GPL(verity_verify_sig_opts_cleanup);
>diff --git a/drivers/md/dm-verity-verify-sig.h b/drivers/md/dm-verity-verify-sig.h
>new file mode 100644
>index 000000000000..4cdb5eeb90d4
>--- /dev/null
>+++ b/drivers/md/dm-verity-verify-sig.h
>@@ -0,0 +1,31 @@
>+// SPDX-License-Identifier: GPL-2.0
>+/*
>+ * Copyright (C) 2019 Microsoft Corporation.
>+ *
>+ * Author: Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
>+ *
>+ * This file is released under the GPLv2.
>+ */
>+#ifndef DM_VERITY_SIG_VERIFICATION_H
>+#define DM_VERITY_SIG_VERIFICATION_H
>+
>+#define DM_VERITY_ROOT_HASH_VERIFICATION "DM Verity Sig Verification"
>+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY "root_hash_sig_key_desc"
>+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPTS 2
>+
>+struct dm_verity_sig_opts {
>+ unsigned int sig_size;
>+ u8 *sig;
>+};
>+int verity_verify_root_hash(const void *data, size_t data_len,
>+ const void *sig_data, size_t sig_len);
>+
>+bool verity_verify_is_sig_opt_arg(const char *arg_name);
>+
>+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
>+ struct dm_verity_sig_opts *sig_opts,
>+ unsigned int *argc, const char *arg_name);
>+
>+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts);
>+
>+#endif /* DM_VERITY_SIG_VERIFICATION_H */
>--
>2.17.1
>
^ permalink raw reply
* [PATCH] x86/ima: fix the Kconfig dependency for IMA_ARCH_POLICY
From: Nayna Jain @ 2019-06-07 19:12 UTC (permalink / raw)
To: linux-integrity
Cc: zohar, linux-security-module, linux-kernel, Nayna Jain,
Eric Biederman, Dave Young
If enabled, ima arch specific policies always adds the measurements rules,
this makes it dependent on CONFIG_IMA. CONFIG_IMA_APPRAISE implicitly takes
care of this, however it is needed explicitly for CONFIG_KEXEC_VERIFY_SIG.
This patch adds the CONFIG_IMA dependency in combination with
CONFIG_KEXEC_VERIFY_SIG for CONFIG_IMA_ARCH_POLICY
Fixes: d958083a8f640 (x86/ima: define arch_get_ima_policy() for x86)
Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Dave Young <dyoung@redhat.com>
---
security/integrity/ima/Kconfig | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index a18f8c6d13b5..df65d2d41905 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -159,7 +159,8 @@ config IMA_APPRAISE
config IMA_ARCH_POLICY
bool "Enable loading an IMA architecture specific policy"
- depends on KEXEC_VERIFY_SIG || IMA_APPRAISE && INTEGRITY_ASYMMETRIC_KEYS
+ depends on (KEXEC_VERIFY_SIG && IMA) || IMA_APPRAISE \
+ && INTEGRITY_ASYMMETRIC_KEYS
default n
help
This option enables loading an IMA architecture specific policy
--
2.17.1
^ permalink raw reply related
* Re: [GIT PULL] apparmor bug fixes for v5.3-rc4
From: Linus Torvalds @ 2019-06-07 16:40 UTC (permalink / raw)
To: John Johansen; +Cc: LKLM, open list:SECURITY SUBSYSTEM
In-Reply-To: <de770db0-5451-c57f-df52-75114ad290e9@canonical.com>
On Thu, Jun 6, 2019 at 5:39 PM John Johansen
<john.johansen@canonical.com> wrote:
>
> Can you please pull the following bug fixes for apparmor
No I can not.
You have for some completely unrecognizable reason rebased OTHER PEOPLES WORK.
There are two new apparmor commits in there, but there are also 89
random rebased patches from the networking tree.
What is going with all the security subsystem issues? Why does this
garbage keep on happening, and why are the security trees doing this
when everybody else has learnt.
DO NOT REBASE. NOT EVER. YOU CLEARLY DO NOT KNOW WHAT YOU ARE DOING.
"git rebase" and "git merge" are not commands you should ever EVER
EVER use, because you don't seem to understand what they do.
I'm seriously fed up with all the security subsystem by now. This is
not ok. It's not even one person, it seems to be a common issue where
people do completely odd things that make zero sense.
Linus
^ 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