* Re: [PATCH bpf-next v10 06/10] bpf,landlock: Add a new map type: inode
From: Mickaël Salaün @ 2019-07-31 18:46 UTC (permalink / raw)
To: Alexei Starovoitov, Mickaël Salaün
Cc: linux-kernel, Alexander Viro, Alexei Starovoitov, Andrew Morton,
Andy Lutomirski, Arnaldo Carvalho de Melo, Casey Schaufler,
Daniel Borkmann, David Drysdale, David S . Miller,
Eric W . Biederman, James Morris, Jann Horn, John Johansen,
Jonathan Corbet, Kees Cook, Michael Kerrisk, Paul Moore,
Sargun Dhillon, Serge E . Hallyn, Shuah Khan, Stephen Smalley,
Tejun Heo, Tetsuo Handa, Thomas Graf, Tycho Andersen, Will Drewry,
kernel-hardening, linux-api, linux-fsdevel, linux-security-module,
netdev
In-Reply-To: <20190727014048.3czy3n2hi6hfdy3m@ast-mbp.dhcp.thefacebook.com>
On 27/07/2019 03:40, Alexei Starovoitov wrote:
> On Sun, Jul 21, 2019 at 11:31:12PM +0200, Mickaël Salaün wrote:
>> FIXME: 64-bits in the doc
FYI, this FIXME was fixed, just not removed from this message. :)
>>
>> This new map store arbitrary values referenced by inode keys. The map
>> can be updated from user space with file descriptor pointing to inodes
>> tied to a file system. From an eBPF (Landlock) program point of view,
>> such a map is read-only and can only be used to retrieved a value tied
>> to a given inode. This is useful to recognize an inode tagged by user
>> space, without access right to this inode (i.e. no need to have a write
>> access to this inode).
>>
>> Add dedicated BPF functions to handle this type of map:
>> * bpf_inode_htab_map_update_elem()
>> * bpf_inode_htab_map_lookup_elem()
>> * bpf_inode_htab_map_delete_elem()
>>
>> This new map require a dedicated helper inode_map_lookup_elem() because
>> of the key which is a pointer to an opaque data (only provided by the
>> kernel). This act like a (physical or cryptographic) key, which is why
>> it is also not allowed to get the next key.
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>
> there are too many things to comment on.
> Let's do this patch.
>
> imo inode_map concept is interesting, but see below...
>
>> +
>> + /*
>> + * Limit number of entries in an inode map to the maximum number of
>> + * open files for the current process. The maximum number of file
>> + * references (including all inode maps) for a process is then
>> + * (RLIMIT_NOFILE - 1) * RLIMIT_NOFILE. If the process' RLIMIT_NOFILE
>> + * is 0, then any entry update is forbidden.
>> + *
>> + * An eBPF program can inherit all the inode map FD. The worse case is
>> + * to fill a bunch of arraymaps, create an eBPF program, close the
>> + * inode map FDs, and start again. The maximum number of inode map
>> + * entries can then be close to RLIMIT_NOFILE^3.
>> + */
>> + if (attr->max_entries > rlimit(RLIMIT_NOFILE))
>> + return -EMFILE;
>
> rlimit is checked, but no fd are consumed.
> Once created such inode map_fd can be passed to a different process.
> map_fd can be pinned into bpffs.
> etc.
> what the value of the check?
I was looking for the most meaningful limit for a process, and rlimit is
the best I found. As the limit of open FD per processes, rlimit is not
perfect, but I think the semantic is close here (e.g. a process can also
pass FD through unix socket).
>
>> +
>> + /* decorelate UAPI from kernel API */
>> + attr->key_size = sizeof(struct inode *);
>> +
>> + return htab_map_alloc_check(attr);
>> +}
>> +
>> +static void inode_htab_put_key(void *key)
>> +{
>> + struct inode **inode = key;
>> +
>> + if ((*inode)->i_state & I_FREEING)
>> + return;
>
> checking the state without take a lock? isn't it racy?
This should only trigger when called from security_inode_free(). I'll
add a comment.
>
>> + iput(*inode);
>> +}
>> +
>> +/* called from syscall or (never) from eBPF program */
>> +static int map_get_next_no_key(struct bpf_map *map, void *key, void *next_key)
>> +{
>> + /* do not leak a file descriptor */
>
> what this comment suppose to mean?
Because a key is a reference to an inode, a possible return value for
this function could be a file descriptor pointing to this inode (the
same way a file descriptor is use to add an element). For now, I don't
want to implement a way for a process with such a map to extract such
inode, which I compare to a possible leak (of information, not kernel
memory nor object). This could be implemented in the future if there is
value in it (and probably some additional safeguards), though.
>
>> + return -ENOTSUPP;
>> +}
>> +
>> +/* must call iput(inode) after this call */
>> +static struct inode *inode_from_fd(int ufd, bool check_access)
>> +{
>> + struct inode *ret;
>> + struct fd f;
>> + int deny;
>> +
>> + f = fdget(ufd);
>> + if (unlikely(!f.file))
>> + return ERR_PTR(-EBADF);
>> + /* TODO?: add this check when called from an eBPF program too (already
>> + * checked by the LSM parent hooks anyway) */
>> + if (unlikely(IS_PRIVATE(file_inode(f.file)))) {
>> + ret = ERR_PTR(-EINVAL);
>> + goto put_fd;
>> + }
>> + /* check if the FD is tied to a mount point */
>> + /* TODO?: add this check when called from an eBPF program too */
>> + if (unlikely(f.file->f_path.mnt->mnt_flags & MNT_INTERNAL)) {
>> + ret = ERR_PTR(-EINVAL);
>> + goto put_fd;
>> + }
>
> a bunch of TODOs do not inspire confidence.
I think the current implement is good, but these TODOs are here to draw
attention on particular points for which I would like external review
and opinion (hence the "?").
>
>> + if (check_access) {
>> + /*
>> + * must be allowed to access attributes from this file to then
>> + * be able to compare an inode to its map entry
>> + */
>> + deny = security_inode_getattr(&f.file->f_path);
>> + if (deny) {
>> + ret = ERR_PTR(deny);
>> + goto put_fd;
>> + }
>> + }
>> + ret = file_inode(f.file);
>> + ihold(ret);
>> +
>> +put_fd:
>> + fdput(f);
>> + return ret;
>> +}
>> +
>> +/*
>> + * The key is a FD when called from a syscall, but an inode address when called
>> + * from an eBPF program.
>> + */
>> +
>> +/* called from syscall */
>> +int bpf_inode_fd_htab_map_lookup_elem(struct bpf_map *map, int *key, void *value)
>> +{
>> + void *ptr;
>> + struct inode *inode;
>> + int ret;
>> +
>> + /* check inode access */
>> + inode = inode_from_fd(*key, true);
>> + if (IS_ERR(inode))
>> + return PTR_ERR(inode);
>> +
>> + rcu_read_lock();
>> + ptr = htab_map_lookup_elem(map, &inode);
>> + iput(inode);
>> + if (IS_ERR(ptr)) {
>> + ret = PTR_ERR(ptr);
>> + } else if (!ptr) {
>> + ret = -ENOENT;
>> + } else {
>> + ret = 0;
>> + copy_map_value(map, value, ptr);
>> + }
>> + rcu_read_unlock();
>> + return ret;
>> +}
>> +
>> +/* called from kernel */
>
> wrong comment?
> kernel side cannot call it, right?
This is called from bpf_inode_fd_htab_map_delete_elem() (code just
beneath), and from
kernel/bpf/syscall.c:bpf_inode_ptr_unlocked_htab_map_delet_elem() which
can be called by security_inode_free() (hook_inode_free_security).
>
>> +int bpf_inode_ptr_locked_htab_map_delete_elem(struct bpf_map *map,
>> + struct inode **key, bool remove_in_inode)
>> +{
>> + if (remove_in_inode)
>> + landlock_inode_remove_map(*key, map);
>> + return htab_map_delete_elem(map, key);
>> +}
>> +
>> +/* called from syscall */
>> +int bpf_inode_fd_htab_map_delete_elem(struct bpf_map *map, int *key)
>> +{
>> + struct inode *inode;
>> + int ret;
>> +
>> + /* do not check inode access (similar to directory check) */
>> + inode = inode_from_fd(*key, false);
>> + if (IS_ERR(inode))
>> + return PTR_ERR(inode);
>> + ret = bpf_inode_ptr_locked_htab_map_delete_elem(map, &inode, true);
>> + iput(inode);
>> + return ret;
>> +}
>> +
>> +/* called from syscall */
>> +int bpf_inode_fd_htab_map_update_elem(struct bpf_map *map, int *key, void *value,
>> + u64 map_flags)
>> +{
>> + struct inode *inode;
>> + int ret;
>> +
>> + WARN_ON_ONCE(!rcu_read_lock_held());
>> +
>> + /* check inode access */
>> + inode = inode_from_fd(*key, true);
>> + if (IS_ERR(inode))
>> + return PTR_ERR(inode);
>> + ret = htab_map_update_elem(map, &inode, value, map_flags);
>> + if (!ret)
>> + ret = landlock_inode_add_map(inode, map);
>> + iput(inode);
>> + return ret;
>> +}
>> +
>> +static void inode_htab_map_free(struct bpf_map *map)
>> +{
>> + struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
>> + struct hlist_nulls_node *n;
>> + struct hlist_nulls_head *head;
>> + struct htab_elem *l;
>> + int i;
>> +
>> + for (i = 0; i < htab->n_buckets; i++) {
>> + head = select_bucket(htab, i);
>> + hlist_nulls_for_each_entry_safe(l, n, head, hash_node) {
>> + landlock_inode_remove_map(*((struct inode **)l->key), map);
>> + }
>> + }
>> + htab_map_free(map);
>> +}
>
> user space can delete the map.
> that will trigger inode_htab_map_free() which will call
> landlock_inode_remove_map().
> which will simply itereate the list and delete from the list.
landlock_inode_remove_map() removes the reference to the map (being
freed) from the inode (with an RCU lock).
>
> While in parallel inode can be destoyed and hook_inode_free_security()
> will be called.
> I think nothing that protects from this race.
According to security_inode_free(), the inode is effectively freed after
the RCU grace period. However, I forgot to call bpf_map_inc() in
landlock_inode_add_map(), which would prevent the map to be freed
outside of the security_inode_free(). I'll fix that.
>
>> +
>> +/*
>> + * We need a dedicated helper to deal with inode maps because the key is a
>> + * pointer to an opaque data, only provided by the kernel. This really act
>> + * like a (physical or cryptographic) key, which is why it is also not allowed
>> + * to get the next key with map_get_next_key().
>
> inode pointer is like cryptographic key? :)
I wanted to highlight the fact that, contrary to other map key types,
the value of this one should not be readable, only usable. A "secret
value" is more appropriate but still confusing. I'll rephrase that.
>
>> + */
>> +BPF_CALL_2(bpf_inode_map_lookup_elem, struct bpf_map *, map, void *, key)
>> +{
>> + WARN_ON_ONCE(!rcu_read_lock_held());
>> + return (unsigned long)htab_map_lookup_elem(map, &key);
>> +}
>> +
>> +const struct bpf_func_proto bpf_inode_map_lookup_elem_proto = {
>> + .func = bpf_inode_map_lookup_elem,
>> + .gpl_only = false,
>> + .pkt_access = true,
>
> pkt_access ? :)
This slipped in with this rebase, I'll remove it. :)
>
>> + .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
>> + .arg1_type = ARG_CONST_MAP_PTR,
>> + .arg2_type = ARG_PTR_TO_INODE,
>> +};
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index b2a8cb14f28e..e46441c42b68 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -801,6 +801,8 @@ static int map_lookup_elem(union bpf_attr *attr)
>> } else if (map->map_type == BPF_MAP_TYPE_QUEUE ||
>> map->map_type == BPF_MAP_TYPE_STACK) {
>> err = map->ops->map_peek_elem(map, value);
>> + } else if (map->map_type == BPF_MAP_TYPE_INODE) {
>> + err = bpf_inode_fd_htab_map_lookup_elem(map, key, value);
>> } else {
>> rcu_read_lock();
>> if (map->ops->map_lookup_elem_sys_only)
>> @@ -951,6 +953,10 @@ static int map_update_elem(union bpf_attr *attr)
>> } else if (map->map_type == BPF_MAP_TYPE_QUEUE ||
>> map->map_type == BPF_MAP_TYPE_STACK) {
>> err = map->ops->map_push_elem(map, value, attr->flags);
>> + } else if (map->map_type == BPF_MAP_TYPE_INODE) {
>> + rcu_read_lock();
>> + err = bpf_inode_fd_htab_map_update_elem(map, key, value, attr->flags);
>> + rcu_read_unlock();
>> } else {
>> rcu_read_lock();
>> err = map->ops->map_update_elem(map, key, value, attr->flags);
>> @@ -1006,7 +1012,10 @@ static int map_delete_elem(union bpf_attr *attr)
>> preempt_disable();
>> __this_cpu_inc(bpf_prog_active);
>> rcu_read_lock();
>> - err = map->ops->map_delete_elem(map, key);
>> + if (map->map_type == BPF_MAP_TYPE_INODE)
>> + err = bpf_inode_fd_htab_map_delete_elem(map, key);
>> + else
>> + err = map->ops->map_delete_elem(map, key);
>> rcu_read_unlock();
>> __this_cpu_dec(bpf_prog_active);
>> preempt_enable();
>> @@ -1018,6 +1027,22 @@ static int map_delete_elem(union bpf_attr *attr)
>> return err;
>> }
>>
>> +int bpf_inode_ptr_unlocked_htab_map_delete_elem(struct bpf_map *map,
>> + struct inode **key, bool remove_in_inode)
>> +{
>> + int err;
>> +
>> + preempt_disable();
>> + __this_cpu_inc(bpf_prog_active);
>> + rcu_read_lock();
>> + err = bpf_inode_ptr_locked_htab_map_delete_elem(map, key, remove_in_inode);
>> + rcu_read_unlock();
>> + __this_cpu_dec(bpf_prog_active);
>> + preempt_enable();
>> + maybe_wait_bpf_programs(map);
>
> if that function was actually doing synchronize_rcu() the consequences
> would have been unpleasant. Fortunately it's a nop in this case.
> Please read the code carefully before copy-paste.
> Also what do you think the reason of bpf_prog_active above?
> What is the reason of rcu_read_lock above?
The RCU is used as for every map modifications (usually from userspace).
I wasn't sure about the other protections so I kept the same (generic)
checks as in map_delete_elem() (just above) because this function follow
the same semantic. What can I safely remove?
>
> I think the patch set needs to shrink at least in half to be reviewable.
> The way you tie seccomp and lsm is probably the biggest obstacle
> than any of the bugs above.
> Can you drop seccomp ? and do it as normal lsm ?
The seccomp/enforcement part is needed to have a minimum viable product,
i.e. a process able to sandbox itself. Are you suggesting to first merge
a version when it is only possible to create inode maps but not use them
in an useful way (i.e. for sandboxing)? I can do it if it's OK with you,
and I hope it will not be a problem for the security folks if it can
help to move forward.
--
Mickaël Salaün
ANSSI/SDE/ST/LAM
Les données à caractère personnel recueillies et traitées dans le cadre de cet échange, le sont à seule fin d’exécution d’une relation professionnelle et s’opèrent dans cette seule finalité et pour la durée nécessaire à cette relation. Si vous souhaitez faire usage de vos droits de consultation, de rectification et de suppression de vos données, veuillez contacter contact.rgpd@sgdsn.gouv.fr. Si vous avez reçu ce message par erreur, nous vous remercions d’en informer l’expéditeur et de détruire le message. The personal data collected and processed during this exchange aims solely at completing a business relationship and is limited to the necessary duration of that relationship. If you wish to use your rights of consultation, rectification and deletion of your data, please contact: contact.rgpd@sgdsn.gouv.fr. If you have received this message in error, we thank you for informing the sender and destroying the message.
^ permalink raw reply
* Re: [PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Casey Schaufler @ 2019-07-31 17:26 UTC (permalink / raw)
To: Aaron Goidel, paul
Cc: selinux, linux-security-module, linux-fsdevel, dhowells, jack,
amir73il, jmorris, sds, linux-kernel
In-Reply-To: <20190731153443.4984-1-acgoide@tycho.nsa.gov>
On 7/31/2019 8:34 AM, Aaron Goidel wrote:
> As of now, setting watches on filesystem objects has, at most, applied a
> check for read access to the inode, and in the case of fanotify, requires
> CAP_SYS_ADMIN. No specific security hook or permission check has been
> provided to control the setting of watches. Using any of inotify, dnotify,
> or fanotify, it is possible to observe, not only write-like operations, but
> even read access to a file. Modeling the watch as being merely a read from
> the file is insufficient for the needs of SELinux. This is due to the fact
> that read access should not necessarily imply access to information about
> when another process reads from a file. Furthermore, fanotify watches grant
> more power to an application in the form of permission events. While
> notification events are solely, unidirectional (i.e. they only pass
> information to the receiving application), permission events are blocking.
> Permission events make a request to the receiving application which will
> then reply with a decision as to whether or not that action may be
> completed. This causes the issue of the watching application having the
> ability to exercise control over the triggering process. Without drawing a
> distinction within the permission check, the ability to read would imply
> the greater ability to control an application. Additionally, mount and
> superblock watches apply to all files within the same mount or superblock.
> Read access to one file should not necessarily imply the ability to watch
> all files accessed within a given mount or superblock.
>
> In order to solve these issues, a new LSM hook is implemented and has been
> placed within the system calls for marking filesystem objects with inotify,
> fanotify, and dnotify watches. These calls to the hook are placed at the
> point at which the target path has been resolved and are provided with the
> path struct, the mask of requested notification events, and the type of
> object on which the mark is being set (inode, superblock, or mount). The
> mask and obj_type have already been translated into common FS_* values
> shared by the entirety of the fs notification infrastructure. The path
> struct is passed rather than just the inode so that the mount is available,
> particularly for mount watches. This also allows for use of the hook by
> pathname-based security modules. However, since the hook is intended for
> use even by inode based security modules, it is not placed under the
> CONFIG_SECURITY_PATH conditional. Otherwise, the inode-based security
> modules would need to enable all of the path hooks, even though they do not
> use any of them.
>
> This only provides a hook at the point of setting a watch, and presumes
> that permission to set a particular watch implies the ability to receive
> all notification about that object which match the mask. This is all that
> is required for SELinux. If other security modules require additional hooks
> or infrastructure to control delivery of notification, these can be added
> by them. It does not make sense for us to propose hooks for which we have
> no implementation. The understanding that all notifications received by the
> requesting application are all strictly of a type for which the application
> has been granted permission shows that this implementation is sufficient in
> its coverage.
>
> Security modules wishing to provide complete control over fanotify must
> also implement a security_file_open hook that validates that the access
> requested by the watching application is authorized. Fanotify has the issue
> that it returns a file descriptor with the file mode specified during
> fanotify_init() to the watching process on event. This is already covered
> by the LSM security_file_open hook if the security module implements
> checking of the requested file mode there. Otherwise, a watching process
> can obtain escalated access to a file for which it has not been authorized.
>
> The selinux_path_notify hook implementation works by adding five new file
> permissions: watch, watch_mount, watch_sb, watch_reads, and watch_with_perm
> (descriptions about which will follow), and one new filesystem permission:
> watch (which is applied to superblock checks). The hook then decides which
> subset of these permissions must be held by the requesting application
> based on the contents of the provided mask and the obj_type. The
> selinux_file_open hook already checks the requested file mode and therefore
> ensures that a watching process cannot escalate its access through
> fanotify.
>
> The watch, watch_mount, and watch_sb permissions are the baseline
> permissions for setting a watch on an object and each are a requirement for
> any watch to be set on a file, mount, or superblock respectively. It should
> be noted that having either of the other two permissions (watch_reads and
> watch_with_perm) does not imply the watch, watch_mount, or watch_sb
> permission. Superblock watches further require the filesystem watch
> permission to the superblock. As there is no labeled object in view for
> mounts, there is no specific check for mount watches beyond watch_mount to
> the inode. Such a check could be added in the future, if a suitable labeled
> object existed representing the mount.
>
> The watch_reads permission is required to receive notifications from
> read-exclusive events on filesystem objects. These events include accessing
> a file for the purpose of reading and closing a file which has been opened
> read-only. This distinction has been drawn in order to provide a direct
> indication in the policy for this otherwise not obvious capability. Read
> access to a file should not necessarily imply the ability to observe read
> events on a file.
>
> Finally, watch_with_perm only applies to fanotify masks since it is the
> only way to set a mask which allows for the blocking, permission event.
> This permission is needed for any watch which is of this type. Though
> fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit
> trust to root, which we do not do, and does not support least privilege.
>
> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
I can't say that I accept your arguments that this is sufficient,
but as you point out, the SELinux team does, and if I want more
for Smack that's my fish to fry.
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> fs/notify/dnotify/dnotify.c | 15 +++++++--
> fs/notify/fanotify/fanotify_user.c | 27 +++++++++++++++--
> fs/notify/inotify/inotify_user.c | 13 ++++++--
> include/linux/lsm_hooks.h | 9 +++++-
> include/linux/security.h | 10 ++++--
> security/security.c | 6 ++++
> security/selinux/hooks.c | 47 +++++++++++++++++++++++++++++
> security/selinux/include/classmap.h | 5 +--
> 8 files changed, 120 insertions(+), 12 deletions(-)
>
> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
> index 250369d6901d..87a7f61bc91c 100644
> --- a/fs/notify/dnotify/dnotify.c
> +++ b/fs/notify/dnotify/dnotify.c
> @@ -22,6 +22,7 @@
> #include <linux/sched/signal.h>
> #include <linux/dnotify.h>
> #include <linux/init.h>
> +#include <linux/security.h>
> #include <linux/spinlock.h>
> #include <linux/slab.h>
> #include <linux/fdtable.h>
> @@ -288,6 +289,17 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
> goto out_err;
> }
>
> + /*
> + * convert the userspace DN_* "arg" to the internal FS_*
> + * defined in fsnotify
> + */
> + mask = convert_arg(arg);
> +
> + error = security_path_notify(&filp->f_path, mask,
> + FSNOTIFY_OBJ_TYPE_INODE);
> + if (error)
> + goto out_err;
> +
> /* expect most fcntl to add new rather than augment old */
> dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL);
> if (!dn) {
> @@ -302,9 +314,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
> goto out_err;
> }
>
> - /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */
> - mask = convert_arg(arg);
> -
> /* set up the new_fsn_mark and new_dn_mark */
> new_fsn_mark = &new_dn_mark->fsn_mark;
> fsnotify_init_mark(new_fsn_mark, dnotify_group);
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index a90bb19dcfa2..b83f27021f54 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -528,9 +528,10 @@ static const struct file_operations fanotify_fops = {
> };
>
> static int fanotify_find_path(int dfd, const char __user *filename,
> - struct path *path, unsigned int flags)
> + struct path *path, unsigned int flags, __u64 mask)
> {
> int ret;
> + unsigned int obj_type;
>
> pr_debug("%s: dfd=%d filename=%p flags=%x\n", __func__,
> dfd, filename, flags);
> @@ -567,8 +568,30 @@ static int fanotify_find_path(int dfd, const char __user *filename,
>
> /* you can only watch an inode if you have read permissions on it */
> ret = inode_permission(path->dentry->d_inode, MAY_READ);
> + if (ret) {
> + path_put(path);
> + goto out;
> + }
> +
> + switch (flags & FANOTIFY_MARK_TYPE_BITS) {
> + case FAN_MARK_MOUNT:
> + obj_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
> + break;
> + case FAN_MARK_FILESYSTEM:
> + obj_type = FSNOTIFY_OBJ_TYPE_SB;
> + break;
> + case FAN_MARK_INODE:
> + obj_type = FSNOTIFY_OBJ_TYPE_INODE;
> + break;
> + default:
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = security_path_notify(path, mask, obj_type);
> if (ret)
> path_put(path);
> +
> out:
> return ret;
> }
> @@ -1014,7 +1037,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
> goto fput_and_out;
> }
>
> - ret = fanotify_find_path(dfd, pathname, &path, flags);
> + ret = fanotify_find_path(dfd, pathname, &path, flags, mask);
> if (ret)
> goto fput_and_out;
>
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index 7b53598c8804..e0d593c2d437 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -39,6 +39,7 @@
> #include <linux/poll.h>
> #include <linux/wait.h>
> #include <linux/memcontrol.h>
> +#include <linux/security.h>
>
> #include "inotify.h"
> #include "../fdinfo.h"
> @@ -342,7 +343,8 @@ static const struct file_operations inotify_fops = {
> /*
> * find_inode - resolve a user-given path to a specific inode
> */
> -static int inotify_find_inode(const char __user *dirname, struct path *path, unsigned flags)
> +static int inotify_find_inode(const char __user *dirname, struct path *path,
> + unsigned int flags, __u64 mask)
> {
> int error;
>
> @@ -351,8 +353,15 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns
> return error;
> /* you can only watch an inode if you have read permissions on it */
> error = inode_permission(path->dentry->d_inode, MAY_READ);
> + if (error) {
> + path_put(path);
> + return error;
> + }
> + error = security_path_notify(path, mask,
> + FSNOTIFY_OBJ_TYPE_INODE);
> if (error)
> path_put(path);
> +
> return error;
> }
>
> @@ -744,7 +753,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
> if (mask & IN_ONLYDIR)
> flags |= LOOKUP_DIRECTORY;
>
> - ret = inotify_find_inode(pathname, &path, flags);
> + ret = inotify_find_inode(pathname, &path, flags, mask);
> if (ret)
> goto fput_and_out;
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 47f58cfb6a19..ead98af9c602 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -339,6 +339,9 @@
> * Check for permission to change root directory.
> * @path contains the path structure.
> * Return 0 if permission is granted.
> + * @path_notify:
> + * Check permissions before setting a watch on events as defined by @mask,
> + * on an object at @path, whose type is defined by @obj_type.
> * @inode_readlink:
> * Check the permission to read the symbolic link.
> * @dentry contains the dentry structure for the file link.
> @@ -1535,7 +1538,9 @@ union security_list_options {
> int (*path_chown)(const struct path *path, kuid_t uid, kgid_t gid);
> int (*path_chroot)(const struct path *path);
> #endif
> -
> + /* Needed for inode based security check */
> + int (*path_notify)(const struct path *path, u64 mask,
> + unsigned int obj_type);
> int (*inode_alloc_security)(struct inode *inode);
> void (*inode_free_security)(struct inode *inode);
> int (*inode_init_security)(struct inode *inode, struct inode *dir,
> @@ -1860,6 +1865,8 @@ struct security_hook_heads {
> struct hlist_head path_chown;
> struct hlist_head path_chroot;
> #endif
> + /* Needed for inode based modules as well */
> + struct hlist_head path_notify;
> struct hlist_head inode_alloc_security;
> struct hlist_head inode_free_security;
> struct hlist_head inode_init_security;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 659071c2e57c..7d9c1da1f659 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -259,7 +259,8 @@ int security_dentry_create_files_as(struct dentry *dentry, int mode,
> struct qstr *name,
> const struct cred *old,
> struct cred *new);
> -
> +int security_path_notify(const struct path *path, u64 mask,
> + unsigned int obj_type);
> int security_inode_alloc(struct inode *inode);
> void security_inode_free(struct inode *inode);
> int security_inode_init_security(struct inode *inode, struct inode *dir,
> @@ -387,7 +388,6 @@ int security_ismaclabel(const char *name);
> int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
> void security_release_secctx(char *secdata, u32 seclen);
> -
> void security_inode_invalidate_secctx(struct inode *inode);
> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
> @@ -621,6 +621,12 @@ static inline int security_move_mount(const struct path *from_path,
> return 0;
> }
>
> +static inline int security_path_notify(const struct path *path, u64 mask,
> + unsigned int obj_type)
> +{
> + return 0;
> +}
> +
> static inline int security_inode_alloc(struct inode *inode)
> {
> return 0;
> diff --git a/security/security.c b/security/security.c
> index 613a5c00e602..30687e1366b7 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -871,6 +871,12 @@ int security_move_mount(const struct path *from_path, const struct path *to_path
> return call_int_hook(move_mount, 0, from_path, to_path);
> }
>
> +int security_path_notify(const struct path *path, u64 mask,
> + unsigned int obj_type)
> +{
> + return call_int_hook(path_notify, 0, path, mask, obj_type);
> +}
> +
> int security_inode_alloc(struct inode *inode)
> {
> int rc = lsm_inode_alloc(inode);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c61787b15f27..a1aaf79421df 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -92,6 +92,8 @@
> #include <linux/kernfs.h>
> #include <linux/stringhash.h> /* for hashlen_string() */
> #include <uapi/linux/mount.h>
> +#include <linux/fsnotify.h>
> +#include <linux/fanotify.h>
>
> #include "avc.h"
> #include "objsec.h"
> @@ -3261,6 +3263,50 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
> return -EACCES;
> }
>
> +static int selinux_path_notify(const struct path *path, u64 mask,
> + unsigned int obj_type)
> +{
> + int ret;
> + u32 perm;
> +
> + struct common_audit_data ad;
> +
> + ad.type = LSM_AUDIT_DATA_PATH;
> + ad.u.path = *path;
> +
> + /*
> + * Set permission needed based on the type of mark being set.
> + * Performs an additional check for sb watches.
> + */
> + switch (obj_type) {
> + case FSNOTIFY_OBJ_TYPE_VFSMOUNT:
> + perm = FILE__WATCH_MOUNT;
> + break;
> + case FSNOTIFY_OBJ_TYPE_SB:
> + perm = FILE__WATCH_SB;
> + ret = superblock_has_perm(current_cred(), path->dentry->d_sb,
> + FILESYSTEM__WATCH, &ad);
> + if (ret)
> + return ret;
> + break;
> + case FSNOTIFY_OBJ_TYPE_INODE:
> + perm = FILE__WATCH;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + // check if the mask is requesting ability to set a blocking watch
> + if (mask & (ALL_FSNOTIFY_PERM_EVENTS))
> + perm |= FILE__WATCH_WITH_PERM; // if so, check that permission
> +
> + // is the mask asking to watch file reads?
> + if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_CLOSE_NOWRITE))
> + perm |= FILE__WATCH_READS; // check that permission as well
> +
> + return path_has_perm(current_cred(), path, perm);
> +}
> +
> /*
> * Copy the inode security context value to the user.
> *
> @@ -6797,6 +6843,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
> LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
> LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr),
> + LSM_HOOK_INIT(path_notify, selinux_path_notify),
>
> LSM_HOOK_INIT(kernfs_init_security, selinux_kernfs_init_security),
>
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 201f7e588a29..32e9b03be3dd 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -7,7 +7,8 @@
>
> #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
> "rename", "execute", "quotaon", "mounton", "audit_access", \
> - "open", "execmod"
> + "open", "execmod", "watch", "watch_mount", "watch_sb", \
> + "watch_with_perm", "watch_reads"
>
> #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
> "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \
> @@ -60,7 +61,7 @@ struct security_class_mapping secclass_map[] = {
> { "filesystem",
> { "mount", "remount", "unmount", "getattr",
> "relabelfrom", "relabelto", "associate", "quotamod",
> - "quotaget", NULL } },
> + "quotaget", "watch", NULL } },
> { "file",
> { COMMON_FILE_PERMS,
> "execute_no_trans", "entrypoint", NULL } },
^ permalink raw reply
* [PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Aaron Goidel @ 2019-07-31 15:34 UTC (permalink / raw)
To: paul
Cc: selinux, linux-security-module, linux-fsdevel, dhowells, jack,
amir73il, jmorris, sds, linux-kernel, Aaron Goidel
As of now, setting watches on filesystem objects has, at most, applied a
check for read access to the inode, and in the case of fanotify, requires
CAP_SYS_ADMIN. No specific security hook or permission check has been
provided to control the setting of watches. Using any of inotify, dnotify,
or fanotify, it is possible to observe, not only write-like operations, but
even read access to a file. Modeling the watch as being merely a read from
the file is insufficient for the needs of SELinux. This is due to the fact
that read access should not necessarily imply access to information about
when another process reads from a file. Furthermore, fanotify watches grant
more power to an application in the form of permission events. While
notification events are solely, unidirectional (i.e. they only pass
information to the receiving application), permission events are blocking.
Permission events make a request to the receiving application which will
then reply with a decision as to whether or not that action may be
completed. This causes the issue of the watching application having the
ability to exercise control over the triggering process. Without drawing a
distinction within the permission check, the ability to read would imply
the greater ability to control an application. Additionally, mount and
superblock watches apply to all files within the same mount or superblock.
Read access to one file should not necessarily imply the ability to watch
all files accessed within a given mount or superblock.
In order to solve these issues, a new LSM hook is implemented and has been
placed within the system calls for marking filesystem objects with inotify,
fanotify, and dnotify watches. These calls to the hook are placed at the
point at which the target path has been resolved and are provided with the
path struct, the mask of requested notification events, and the type of
object on which the mark is being set (inode, superblock, or mount). The
mask and obj_type have already been translated into common FS_* values
shared by the entirety of the fs notification infrastructure. The path
struct is passed rather than just the inode so that the mount is available,
particularly for mount watches. This also allows for use of the hook by
pathname-based security modules. However, since the hook is intended for
use even by inode based security modules, it is not placed under the
CONFIG_SECURITY_PATH conditional. Otherwise, the inode-based security
modules would need to enable all of the path hooks, even though they do not
use any of them.
This only provides a hook at the point of setting a watch, and presumes
that permission to set a particular watch implies the ability to receive
all notification about that object which match the mask. This is all that
is required for SELinux. If other security modules require additional hooks
or infrastructure to control delivery of notification, these can be added
by them. It does not make sense for us to propose hooks for which we have
no implementation. The understanding that all notifications received by the
requesting application are all strictly of a type for which the application
has been granted permission shows that this implementation is sufficient in
its coverage.
Security modules wishing to provide complete control over fanotify must
also implement a security_file_open hook that validates that the access
requested by the watching application is authorized. Fanotify has the issue
that it returns a file descriptor with the file mode specified during
fanotify_init() to the watching process on event. This is already covered
by the LSM security_file_open hook if the security module implements
checking of the requested file mode there. Otherwise, a watching process
can obtain escalated access to a file for which it has not been authorized.
The selinux_path_notify hook implementation works by adding five new file
permissions: watch, watch_mount, watch_sb, watch_reads, and watch_with_perm
(descriptions about which will follow), and one new filesystem permission:
watch (which is applied to superblock checks). The hook then decides which
subset of these permissions must be held by the requesting application
based on the contents of the provided mask and the obj_type. The
selinux_file_open hook already checks the requested file mode and therefore
ensures that a watching process cannot escalate its access through
fanotify.
The watch, watch_mount, and watch_sb permissions are the baseline
permissions for setting a watch on an object and each are a requirement for
any watch to be set on a file, mount, or superblock respectively. It should
be noted that having either of the other two permissions (watch_reads and
watch_with_perm) does not imply the watch, watch_mount, or watch_sb
permission. Superblock watches further require the filesystem watch
permission to the superblock. As there is no labeled object in view for
mounts, there is no specific check for mount watches beyond watch_mount to
the inode. Such a check could be added in the future, if a suitable labeled
object existed representing the mount.
The watch_reads permission is required to receive notifications from
read-exclusive events on filesystem objects. These events include accessing
a file for the purpose of reading and closing a file which has been opened
read-only. This distinction has been drawn in order to provide a direct
indication in the policy for this otherwise not obvious capability. Read
access to a file should not necessarily imply the ability to observe read
events on a file.
Finally, watch_with_perm only applies to fanotify masks since it is the
only way to set a mask which allows for the blocking, permission event.
This permission is needed for any watch which is of this type. Though
fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit
trust to root, which we do not do, and does not support least privilege.
Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
---
fs/notify/dnotify/dnotify.c | 15 +++++++--
fs/notify/fanotify/fanotify_user.c | 27 +++++++++++++++--
fs/notify/inotify/inotify_user.c | 13 ++++++--
include/linux/lsm_hooks.h | 9 +++++-
include/linux/security.h | 10 ++++--
security/security.c | 6 ++++
security/selinux/hooks.c | 47 +++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 5 +--
8 files changed, 120 insertions(+), 12 deletions(-)
diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index 250369d6901d..87a7f61bc91c 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -22,6 +22,7 @@
#include <linux/sched/signal.h>
#include <linux/dnotify.h>
#include <linux/init.h>
+#include <linux/security.h>
#include <linux/spinlock.h>
#include <linux/slab.h>
#include <linux/fdtable.h>
@@ -288,6 +289,17 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
goto out_err;
}
+ /*
+ * convert the userspace DN_* "arg" to the internal FS_*
+ * defined in fsnotify
+ */
+ mask = convert_arg(arg);
+
+ error = security_path_notify(&filp->f_path, mask,
+ FSNOTIFY_OBJ_TYPE_INODE);
+ if (error)
+ goto out_err;
+
/* expect most fcntl to add new rather than augment old */
dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL);
if (!dn) {
@@ -302,9 +314,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
goto out_err;
}
- /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */
- mask = convert_arg(arg);
-
/* set up the new_fsn_mark and new_dn_mark */
new_fsn_mark = &new_dn_mark->fsn_mark;
fsnotify_init_mark(new_fsn_mark, dnotify_group);
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index a90bb19dcfa2..b83f27021f54 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -528,9 +528,10 @@ static const struct file_operations fanotify_fops = {
};
static int fanotify_find_path(int dfd, const char __user *filename,
- struct path *path, unsigned int flags)
+ struct path *path, unsigned int flags, __u64 mask)
{
int ret;
+ unsigned int obj_type;
pr_debug("%s: dfd=%d filename=%p flags=%x\n", __func__,
dfd, filename, flags);
@@ -567,8 +568,30 @@ static int fanotify_find_path(int dfd, const char __user *filename,
/* you can only watch an inode if you have read permissions on it */
ret = inode_permission(path->dentry->d_inode, MAY_READ);
+ if (ret) {
+ path_put(path);
+ goto out;
+ }
+
+ switch (flags & FANOTIFY_MARK_TYPE_BITS) {
+ case FAN_MARK_MOUNT:
+ obj_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
+ break;
+ case FAN_MARK_FILESYSTEM:
+ obj_type = FSNOTIFY_OBJ_TYPE_SB;
+ break;
+ case FAN_MARK_INODE:
+ obj_type = FSNOTIFY_OBJ_TYPE_INODE;
+ break;
+ default:
+ ret = -EINVAL;
+ goto out;
+ }
+
+ ret = security_path_notify(path, mask, obj_type);
if (ret)
path_put(path);
+
out:
return ret;
}
@@ -1014,7 +1037,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
goto fput_and_out;
}
- ret = fanotify_find_path(dfd, pathname, &path, flags);
+ ret = fanotify_find_path(dfd, pathname, &path, flags, mask);
if (ret)
goto fput_and_out;
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 7b53598c8804..e0d593c2d437 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -39,6 +39,7 @@
#include <linux/poll.h>
#include <linux/wait.h>
#include <linux/memcontrol.h>
+#include <linux/security.h>
#include "inotify.h"
#include "../fdinfo.h"
@@ -342,7 +343,8 @@ static const struct file_operations inotify_fops = {
/*
* find_inode - resolve a user-given path to a specific inode
*/
-static int inotify_find_inode(const char __user *dirname, struct path *path, unsigned flags)
+static int inotify_find_inode(const char __user *dirname, struct path *path,
+ unsigned int flags, __u64 mask)
{
int error;
@@ -351,8 +353,15 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns
return error;
/* you can only watch an inode if you have read permissions on it */
error = inode_permission(path->dentry->d_inode, MAY_READ);
+ if (error) {
+ path_put(path);
+ return error;
+ }
+ error = security_path_notify(path, mask,
+ FSNOTIFY_OBJ_TYPE_INODE);
if (error)
path_put(path);
+
return error;
}
@@ -744,7 +753,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
if (mask & IN_ONLYDIR)
flags |= LOOKUP_DIRECTORY;
- ret = inotify_find_inode(pathname, &path, flags);
+ ret = inotify_find_inode(pathname, &path, flags, mask);
if (ret)
goto fput_and_out;
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 47f58cfb6a19..ead98af9c602 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -339,6 +339,9 @@
* Check for permission to change root directory.
* @path contains the path structure.
* Return 0 if permission is granted.
+ * @path_notify:
+ * Check permissions before setting a watch on events as defined by @mask,
+ * on an object at @path, whose type is defined by @obj_type.
* @inode_readlink:
* Check the permission to read the symbolic link.
* @dentry contains the dentry structure for the file link.
@@ -1535,7 +1538,9 @@ union security_list_options {
int (*path_chown)(const struct path *path, kuid_t uid, kgid_t gid);
int (*path_chroot)(const struct path *path);
#endif
-
+ /* Needed for inode based security check */
+ int (*path_notify)(const struct path *path, u64 mask,
+ unsigned int obj_type);
int (*inode_alloc_security)(struct inode *inode);
void (*inode_free_security)(struct inode *inode);
int (*inode_init_security)(struct inode *inode, struct inode *dir,
@@ -1860,6 +1865,8 @@ struct security_hook_heads {
struct hlist_head path_chown;
struct hlist_head path_chroot;
#endif
+ /* Needed for inode based modules as well */
+ struct hlist_head path_notify;
struct hlist_head inode_alloc_security;
struct hlist_head inode_free_security;
struct hlist_head inode_init_security;
diff --git a/include/linux/security.h b/include/linux/security.h
index 659071c2e57c..7d9c1da1f659 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -259,7 +259,8 @@ int security_dentry_create_files_as(struct dentry *dentry, int mode,
struct qstr *name,
const struct cred *old,
struct cred *new);
-
+int security_path_notify(const struct path *path, u64 mask,
+ unsigned int obj_type);
int security_inode_alloc(struct inode *inode);
void security_inode_free(struct inode *inode);
int security_inode_init_security(struct inode *inode, struct inode *dir,
@@ -387,7 +388,6 @@ int security_ismaclabel(const char *name);
int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
void security_release_secctx(char *secdata, u32 seclen);
-
void security_inode_invalidate_secctx(struct inode *inode);
int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
@@ -621,6 +621,12 @@ static inline int security_move_mount(const struct path *from_path,
return 0;
}
+static inline int security_path_notify(const struct path *path, u64 mask,
+ unsigned int obj_type)
+{
+ return 0;
+}
+
static inline int security_inode_alloc(struct inode *inode)
{
return 0;
diff --git a/security/security.c b/security/security.c
index 613a5c00e602..30687e1366b7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -871,6 +871,12 @@ int security_move_mount(const struct path *from_path, const struct path *to_path
return call_int_hook(move_mount, 0, from_path, to_path);
}
+int security_path_notify(const struct path *path, u64 mask,
+ unsigned int obj_type)
+{
+ return call_int_hook(path_notify, 0, path, mask, obj_type);
+}
+
int security_inode_alloc(struct inode *inode)
{
int rc = lsm_inode_alloc(inode);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c61787b15f27..a1aaf79421df 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -92,6 +92,8 @@
#include <linux/kernfs.h>
#include <linux/stringhash.h> /* for hashlen_string() */
#include <uapi/linux/mount.h>
+#include <linux/fsnotify.h>
+#include <linux/fanotify.h>
#include "avc.h"
#include "objsec.h"
@@ -3261,6 +3263,50 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
return -EACCES;
}
+static int selinux_path_notify(const struct path *path, u64 mask,
+ unsigned int obj_type)
+{
+ int ret;
+ u32 perm;
+
+ struct common_audit_data ad;
+
+ ad.type = LSM_AUDIT_DATA_PATH;
+ ad.u.path = *path;
+
+ /*
+ * Set permission needed based on the type of mark being set.
+ * Performs an additional check for sb watches.
+ */
+ switch (obj_type) {
+ case FSNOTIFY_OBJ_TYPE_VFSMOUNT:
+ perm = FILE__WATCH_MOUNT;
+ break;
+ case FSNOTIFY_OBJ_TYPE_SB:
+ perm = FILE__WATCH_SB;
+ ret = superblock_has_perm(current_cred(), path->dentry->d_sb,
+ FILESYSTEM__WATCH, &ad);
+ if (ret)
+ return ret;
+ break;
+ case FSNOTIFY_OBJ_TYPE_INODE:
+ perm = FILE__WATCH;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ // check if the mask is requesting ability to set a blocking watch
+ if (mask & (ALL_FSNOTIFY_PERM_EVENTS))
+ perm |= FILE__WATCH_WITH_PERM; // if so, check that permission
+
+ // is the mask asking to watch file reads?
+ if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_CLOSE_NOWRITE))
+ perm |= FILE__WATCH_READS; // check that permission as well
+
+ return path_has_perm(current_cred(), path, perm);
+}
+
/*
* Copy the inode security context value to the user.
*
@@ -6797,6 +6843,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr),
+ LSM_HOOK_INIT(path_notify, selinux_path_notify),
LSM_HOOK_INIT(kernfs_init_security, selinux_kernfs_init_security),
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 201f7e588a29..32e9b03be3dd 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -7,7 +7,8 @@
#define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
"rename", "execute", "quotaon", "mounton", "audit_access", \
- "open", "execmod"
+ "open", "execmod", "watch", "watch_mount", "watch_sb", \
+ "watch_with_perm", "watch_reads"
#define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
"listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \
@@ -60,7 +61,7 @@ struct security_class_mapping secclass_map[] = {
{ "filesystem",
{ "mount", "remount", "unmount", "getattr",
"relabelfrom", "relabelto", "associate", "quotamod",
- "quotaget", NULL } },
+ "quotaget", "watch", NULL } },
{ "file",
{ COMMON_FILE_PERMS,
"execute_no_trans", "entrypoint", NULL } },
--
2.21.0
^ permalink raw reply related
* Re: [RFC v2 0/6] Introduce TEE based Trusted Keys support
From: Sumit Garg @ 2019-07-31 14:23 UTC (permalink / raw)
To: Janne Karhunen
Cc: keyrings, linux-integrity, linux-security-module, Jens Wiklander,
Jonathan Corbet, dhowells, jejb, Jarkko Sakkinen, Mimi Zohar,
James Morris, Serge E. Hallyn, Casey Schaufler, Ard Biesheuvel,
Daniel Thompson, Linux Doc Mailing List,
Linux Kernel Mailing List, linux-arm-kernel,
tee-dev @ lists . linaro . org
In-Reply-To: <CAE=Ncrb23q++z8R8UMbjDE2epEq4YVcNGzrRD31eH3JAooYejg@mail.gmail.com>
On Wed, 31 Jul 2019 at 16:33, Janne Karhunen <janne.karhunen@gmail.com> wrote:
>
> On Wed, Jul 31, 2019 at 1:26 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> > > Interesting, I wrote something similar and posted it to the lists a while back:
> > > https://github.com/jkrh/linux/commit/d77ea03afedcb5fd42234cd834da8f8a0809f6a6
> > >
> > > Since there are no generic 'TEEs' available,
> >
> > There is already a generic TEE interface driver available in kernel.
> > Have a look here: "Documentation/tee.txt".
>
> I guess my wording was wrong, tried to say that physical TEEs in the
> wild vary massively hardware wise. Generalizing these things is rough.
>
There are already well defined GlobalPlatform Standards to generalize
the TEE interface. One of them is GlobalPlatform TEE Client API [1]
which provides the basis for this TEE interface.
>
> > > I implemented the same
> > > thing as a generic protocol translator. The shared memory binding for
> > > instance already assumes fair amount about the TEE and how that is
> > > physically present in the system. Besides, the help from usage of shm
> > > is pretty limited due to the size of the keydata.
> > >
> >
> > If you look at patch #1 and #2, they add support to register kernel
> > memory buffer (keydata buffer in this case) with TEE to operate on. So
> > there isn't any limitation due to the size of the keydata.
>
> Ah, didn't mean that. Meant that the keydata is typically pretty small
> in size, so there is limited benefit from passing that in via shm if
> that complicates anything.
>
Ah, ok. Do you think of any better approach rather than to use SHM?
[1] https://globalplatform.org/specs-library/tee-client-api-specification/
-Sumit
>
> --
> Janne
^ permalink raw reply
* Re: [RFC v2 0/6] Introduce TEE based Trusted Keys support
From: Sumit Garg @ 2019-07-31 13:58 UTC (permalink / raw)
To: Janne Karhunen
Cc: keyrings, linux-integrity, linux-security-module, Jens Wiklander,
Jonathan Corbet, dhowells, jejb, Jarkko Sakkinen, Mimi Zohar,
James Morris, Serge E. Hallyn, Casey Schaufler, Ard Biesheuvel,
Daniel Thompson, Linux Doc Mailing List,
Linux Kernel Mailing List, linux-arm-kernel,
tee-dev @ lists . linaro . org
In-Reply-To: <CAE=NcrY7b8eTTovOszBhGhVbjfJAXoAYehiUJyPENGfwWoVcPw@mail.gmail.com>
On Wed, 31 Jul 2019 at 15:51, Janne Karhunen <janne.karhunen@gmail.com> wrote:
>
> Hi,
>
> To clarify a bit further - my thought was to support any type of trust
> source.
That could be very well accomplished via Trusted Keys abstraction
framework [1]. A trust source just need to implement following APIs:
struct trusted_key_ops ts_trusted_key_ops = {
.migratable = 0, /* non-migratable */
.init = init_ts_trusted,
.seal = ts_key_seal,
.unseal = ts_key_unseal,
.get_random = ts_get_random,
.cleanup = cleanup_ts_trusted,
};
> Remote, local or both. Just having one particular type of
> locally bound 'TEE' sounded very limited,
TEE is just one of trust source like TPM, we can have other trust
source as mentioned above.
> especially when nothing from
> the TEE execution side is really needed for supporting the kernel
> crypto. What you really need is the seal/unseal transaction going
> somewhere and where that somewhere is does not matter much.
Its only the seal/unseal operations that are provided by TEE driver
that hooks up under trusted keys abstraction layer.
> With the
> user mode helper in between anyone can easily add their own thing in
> there.
>
Isn't actual purpose to have trusted keys is to protect user-space
from access to kernel keys in plain format? Doesn't user mode helper
defeat that purpose in one way or another?
>
[1] https://lkml.org/lkml/2019/7/18/284
-Sumit
> --
> Janne
>
> On Wed, Jul 31, 2019 at 10:11 AM Janne Karhunen
> <janne.karhunen@gmail.com> wrote:
> >
> > Hi,
> >
> > Interesting, I wrote something similar and posted it to the lists a while back:
> > https://github.com/jkrh/linux/commit/d77ea03afedcb5fd42234cd834da8f8a0809f6a6
> >
> > Since there are no generic 'TEEs' available, I implemented the same
> > thing as a generic protocol translator. The shared memory binding for
> > instance already assumes fair amount about the TEE and how that is
> > physically present in the system. Besides, the help from usage of shm
> > is pretty limited due to the size of the keydata.
> >
> >
> > --
> > Janne
> >
> >
> >
> >
> > On Tue, Jul 30, 2019 at 3:26 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > Add support for TEE based trusted keys where TEE provides the functionality
> > > to seal and unseal trusted keys using hardware unique key. Also, this is
> > > an alternative in case platform doesn't possess a TPM device.
> > >
> > > This series also adds some TEE features like:
> > >
> > > Patch #1, #2 enables support for registered kernel shared memory with TEE.
> > >
> > > Patch #3 enables support for private kernel login method required for
> > > cases like trusted keys where we don't wan't user-space to directly access
> > > TEE service to retrieve trusted key contents.
> > >
> > > Rest of the patches from #4 to #6 adds support for TEE based trusted keys.
> > >
> > > This patch-set has been tested with OP-TEE based pseudo TA which can be
> > > found here [1].
> > >
> > > Also, this patch-set is dependent on generic Trusted Keys framework
> > > patch-set [2].
> > >
> > > [1] https://github.com/OP-TEE/optee_os/pull/3082
> > > [2] https://lkml.org/lkml/2019/7/18/284
> > >
> > > Changes in v2:
> > > 1. Add reviewed-by tags for patch #1 and #2.
> > > 2. Incorporate comments from Jens for patch #3.
> > > 3. Switch to use generic trusted keys framework.
> > >
> > > Sumit Garg (6):
> > > tee: optee: allow kernel pages to register as shm
> > > tee: enable support to register kernel memory
> > > tee: add private login method for kernel clients
> > > KEYS: trusted: Introduce TEE based Trusted Keys
> > > doc: keys: Document usage of TEE based Trusted Keys
> > > MAINTAINERS: Add entry for TEE based Trusted Keys
> > >
> > > Documentation/security/keys/index.rst | 1 +
> > > Documentation/security/keys/tee-trusted.rst | 93 +++++++++
> > > MAINTAINERS | 9 +
> > > drivers/tee/optee/call.c | 7 +
> > > drivers/tee/tee_core.c | 6 +
> > > drivers/tee/tee_shm.c | 16 +-
> > > include/keys/trusted-type.h | 3 +
> > > include/keys/trusted_tee.h | 66 +++++++
> > > include/linux/tee_drv.h | 1 +
> > > include/uapi/linux/tee.h | 8 +
> > > security/keys/Kconfig | 3 +
> > > security/keys/trusted-keys/Makefile | 3 +-
> > > security/keys/trusted-keys/trusted-tee.c | 282 ++++++++++++++++++++++++++++
> > > security/keys/trusted-keys/trusted.c | 3 +
> > > 14 files changed, 498 insertions(+), 3 deletions(-)
> > > create mode 100644 Documentation/security/keys/tee-trusted.rst
> > > create mode 100644 include/keys/trusted_tee.h
> > > create mode 100644 security/keys/trusted-keys/trusted-tee.c
> > >
> > > --
> > > 2.7.4
> > >
^ permalink raw reply
* Re: [RFC v2 0/6] Introduce TEE based Trusted Keys support
From: Janne Karhunen @ 2019-07-31 11:02 UTC (permalink / raw)
To: Sumit Garg
Cc: keyrings, linux-integrity, linux-security-module, Jens Wiklander,
Jonathan Corbet, dhowells, jejb, Jarkko Sakkinen, Mimi Zohar,
James Morris, Serge E. Hallyn, Casey Schaufler, Ard Biesheuvel,
Daniel Thompson, Linux Doc Mailing List,
Linux Kernel Mailing List, linux-arm-kernel,
tee-dev @ lists . linaro . org
In-Reply-To: <CAFA6WYPJAzbPdcpBqioxjY=T8RLw-73B_hpzX4cGnwVvm5zpJw@mail.gmail.com>
On Wed, Jul 31, 2019 at 1:26 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> > Interesting, I wrote something similar and posted it to the lists a while back:
> > https://github.com/jkrh/linux/commit/d77ea03afedcb5fd42234cd834da8f8a0809f6a6
> >
> > Since there are no generic 'TEEs' available,
>
> There is already a generic TEE interface driver available in kernel.
> Have a look here: "Documentation/tee.txt".
I guess my wording was wrong, tried to say that physical TEEs in the
wild vary massively hardware wise. Generalizing these things is rough.
> > I implemented the same
> > thing as a generic protocol translator. The shared memory binding for
> > instance already assumes fair amount about the TEE and how that is
> > physically present in the system. Besides, the help from usage of shm
> > is pretty limited due to the size of the keydata.
> >
>
> If you look at patch #1 and #2, they add support to register kernel
> memory buffer (keydata buffer in this case) with TEE to operate on. So
> there isn't any limitation due to the size of the keydata.
Ah, didn't mean that. Meant that the keydata is typically pretty small
in size, so there is limited benefit from passing that in via shm if
that complicates anything.
--
Janne
^ permalink raw reply
* Re: [RFC v2 0/6] Introduce TEE based Trusted Keys support
From: Sumit Garg @ 2019-07-31 10:26 UTC (permalink / raw)
To: Janne Karhunen
Cc: keyrings, linux-integrity, linux-security-module, Jens Wiklander,
Jonathan Corbet, dhowells, jejb, Jarkko Sakkinen, Mimi Zohar,
James Morris, Serge E. Hallyn, Casey Schaufler, Ard Biesheuvel,
Daniel Thompson, Linux Doc Mailing List,
Linux Kernel Mailing List, linux-arm-kernel,
tee-dev @ lists . linaro . org
In-Reply-To: <CAE=Ncrb63dQLe-nDQyO9OPv7XjwM_9mzL9SrcLiUi2Dr10cD4A@mail.gmail.com>
Hi Janne,
On Wed, 31 Jul 2019 at 12:41, Janne Karhunen <janne.karhunen@gmail.com> wrote:
>
> Hi,
>
> Interesting, I wrote something similar and posted it to the lists a while back:
> https://github.com/jkrh/linux/commit/d77ea03afedcb5fd42234cd834da8f8a0809f6a6
>
> Since there are no generic 'TEEs' available,
There is already a generic TEE interface driver available in kernel.
Have a look here: "Documentation/tee.txt".
> I implemented the same
> thing as a generic protocol translator. The shared memory binding for
> instance already assumes fair amount about the TEE and how that is
> physically present in the system. Besides, the help from usage of shm
> is pretty limited due to the size of the keydata.
>
If you look at patch #1 and #2, they add support to register kernel
memory buffer (keydata buffer in this case) with TEE to operate on. So
there isn't any limitation due to the size of the keydata.
-Sumit
>
> --
> Janne
>
>
>
>
> On Tue, Jul 30, 2019 at 3:26 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Add support for TEE based trusted keys where TEE provides the functionality
> > to seal and unseal trusted keys using hardware unique key. Also, this is
> > an alternative in case platform doesn't possess a TPM device.
> >
> > This series also adds some TEE features like:
> >
> > Patch #1, #2 enables support for registered kernel shared memory with TEE.
> >
> > Patch #3 enables support for private kernel login method required for
> > cases like trusted keys where we don't wan't user-space to directly access
> > TEE service to retrieve trusted key contents.
> >
> > Rest of the patches from #4 to #6 adds support for TEE based trusted keys.
> >
> > This patch-set has been tested with OP-TEE based pseudo TA which can be
> > found here [1].
> >
> > Also, this patch-set is dependent on generic Trusted Keys framework
> > patch-set [2].
> >
> > [1] https://github.com/OP-TEE/optee_os/pull/3082
> > [2] https://lkml.org/lkml/2019/7/18/284
> >
> > Changes in v2:
> > 1. Add reviewed-by tags for patch #1 and #2.
> > 2. Incorporate comments from Jens for patch #3.
> > 3. Switch to use generic trusted keys framework.
> >
> > Sumit Garg (6):
> > tee: optee: allow kernel pages to register as shm
> > tee: enable support to register kernel memory
> > tee: add private login method for kernel clients
> > KEYS: trusted: Introduce TEE based Trusted Keys
> > doc: keys: Document usage of TEE based Trusted Keys
> > MAINTAINERS: Add entry for TEE based Trusted Keys
> >
> > Documentation/security/keys/index.rst | 1 +
> > Documentation/security/keys/tee-trusted.rst | 93 +++++++++
> > MAINTAINERS | 9 +
> > drivers/tee/optee/call.c | 7 +
> > drivers/tee/tee_core.c | 6 +
> > drivers/tee/tee_shm.c | 16 +-
> > include/keys/trusted-type.h | 3 +
> > include/keys/trusted_tee.h | 66 +++++++
> > include/linux/tee_drv.h | 1 +
> > include/uapi/linux/tee.h | 8 +
> > security/keys/Kconfig | 3 +
> > security/keys/trusted-keys/Makefile | 3 +-
> > security/keys/trusted-keys/trusted-tee.c | 282 ++++++++++++++++++++++++++++
> > security/keys/trusted-keys/trusted.c | 3 +
> > 14 files changed, 498 insertions(+), 3 deletions(-)
> > create mode 100644 Documentation/security/keys/tee-trusted.rst
> > create mode 100644 include/keys/trusted_tee.h
> > create mode 100644 security/keys/trusted-keys/trusted-tee.c
> >
> > --
> > 2.7.4
> >
^ permalink raw reply
* Re: [RFC v2 0/6] Introduce TEE based Trusted Keys support
From: Janne Karhunen @ 2019-07-31 10:21 UTC (permalink / raw)
To: Sumit Garg
Cc: keyrings, linux-integrity, linux-security-module, jens.wiklander,
corbet, dhowells, jejb, jarkko.sakkinen, Mimi Zohar, James Morris,
Serge E. Hallyn, Casey Schaufler, ard.biesheuvel, daniel.thompson,
linux-doc, Linux Kernel Mailing List, linux-arm-kernel, tee-dev
In-Reply-To: <CAE=Ncrb63dQLe-nDQyO9OPv7XjwM_9mzL9SrcLiUi2Dr10cD4A@mail.gmail.com>
Hi,
To clarify a bit further - my thought was to support any type of trust
source. Remote, local or both. Just having one particular type of
locally bound 'TEE' sounded very limited, especially when nothing from
the TEE execution side is really needed for supporting the kernel
crypto. What you really need is the seal/unseal transaction going
somewhere and where that somewhere is does not matter much. With the
user mode helper in between anyone can easily add their own thing in
there.
--
Janne
On Wed, Jul 31, 2019 at 10:11 AM Janne Karhunen
<janne.karhunen@gmail.com> wrote:
>
> Hi,
>
> Interesting, I wrote something similar and posted it to the lists a while back:
> https://github.com/jkrh/linux/commit/d77ea03afedcb5fd42234cd834da8f8a0809f6a6
>
> Since there are no generic 'TEEs' available, I implemented the same
> thing as a generic protocol translator. The shared memory binding for
> instance already assumes fair amount about the TEE and how that is
> physically present in the system. Besides, the help from usage of shm
> is pretty limited due to the size of the keydata.
>
>
> --
> Janne
>
>
>
>
> On Tue, Jul 30, 2019 at 3:26 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Add support for TEE based trusted keys where TEE provides the functionality
> > to seal and unseal trusted keys using hardware unique key. Also, this is
> > an alternative in case platform doesn't possess a TPM device.
> >
> > This series also adds some TEE features like:
> >
> > Patch #1, #2 enables support for registered kernel shared memory with TEE.
> >
> > Patch #3 enables support for private kernel login method required for
> > cases like trusted keys where we don't wan't user-space to directly access
> > TEE service to retrieve trusted key contents.
> >
> > Rest of the patches from #4 to #6 adds support for TEE based trusted keys.
> >
> > This patch-set has been tested with OP-TEE based pseudo TA which can be
> > found here [1].
> >
> > Also, this patch-set is dependent on generic Trusted Keys framework
> > patch-set [2].
> >
> > [1] https://github.com/OP-TEE/optee_os/pull/3082
> > [2] https://lkml.org/lkml/2019/7/18/284
> >
> > Changes in v2:
> > 1. Add reviewed-by tags for patch #1 and #2.
> > 2. Incorporate comments from Jens for patch #3.
> > 3. Switch to use generic trusted keys framework.
> >
> > Sumit Garg (6):
> > tee: optee: allow kernel pages to register as shm
> > tee: enable support to register kernel memory
> > tee: add private login method for kernel clients
> > KEYS: trusted: Introduce TEE based Trusted Keys
> > doc: keys: Document usage of TEE based Trusted Keys
> > MAINTAINERS: Add entry for TEE based Trusted Keys
> >
> > Documentation/security/keys/index.rst | 1 +
> > Documentation/security/keys/tee-trusted.rst | 93 +++++++++
> > MAINTAINERS | 9 +
> > drivers/tee/optee/call.c | 7 +
> > drivers/tee/tee_core.c | 6 +
> > drivers/tee/tee_shm.c | 16 +-
> > include/keys/trusted-type.h | 3 +
> > include/keys/trusted_tee.h | 66 +++++++
> > include/linux/tee_drv.h | 1 +
> > include/uapi/linux/tee.h | 8 +
> > security/keys/Kconfig | 3 +
> > security/keys/trusted-keys/Makefile | 3 +-
> > security/keys/trusted-keys/trusted-tee.c | 282 ++++++++++++++++++++++++++++
> > security/keys/trusted-keys/trusted.c | 3 +
> > 14 files changed, 498 insertions(+), 3 deletions(-)
> > create mode 100644 Documentation/security/keys/tee-trusted.rst
> > create mode 100644 include/keys/trusted_tee.h
> > create mode 100644 security/keys/trusted-keys/trusted-tee.c
> >
> > --
> > 2.7.4
> >
^ permalink raw reply
* Re: [PATCH v2 bpf-next 1/4] bpf: unprivileged BPF access via /dev/bpf
From: Song Liu @ 2019-07-31 8:10 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Kees Cook, Networking, bpf, Alexei Starovoitov, Daniel Borkmann,
Kernel Team, Lorenz Bauer, Jann Horn, Greg KH, Linux API,
LSM List
In-Reply-To: <CALCETrUpVMrk7aaf0trfg9AfZ4fy279uJgZH7V+gZzjFw=hUxA@mail.gmail.com>
> On Jul 30, 2019, at 1:24 PM, Andy Lutomirski <luto@kernel.org> wrote:
>
> On Mon, Jul 29, 2019 at 10:07 PM Song Liu <songliubraving@fb.com> wrote:
>>
>> Hi Andy,
>>
>>> On Jul 27, 2019, at 11:20 AM, Song Liu <songliubraving@fb.com> wrote:
>>>
>>> Hi Andy,
>>>
>>>
[...]
>>>
>>
>> I would like more comments on this.
>>
>> Currently, bpf permission is more or less "root or nothing", which we
>> would like to change.
>>
>> The short term goal is to separate bpf from root, in other words, it is
>> "all or nothing". Special user space utilities, such as systemd, would
>> benefit from this. Once this is implemented, systemd can call sys_bpf()
>> when it is not running as root.
>
> As generally nasty as Linux capabilities are, this sounds like a good
> use for CAP_BPF_ADMIN.
I actually agree CAP_BPF_ADMIN makes sense. The hard part is to make
existing tools (setcap, getcap, etc.) and libraries aware of the new CAP.
>
> But what do you have in mind? Isn't non-root systemd mostly just the
> user systemd session? That should *not* have bpf() privileges until
> bpf() is improved such that you can't use it to compromise the system.
cgroup bpf is the major use case here. A less important use case is to
run bpf selftests without being root.
>
>>
>> In longer term, it may be useful to provide finer grain permission of
>> sys_bpf(). For example, sys_bpf() should be aware of containers; and
>> user may only have access to certain bpf maps. Let's call this
>> "fine grain" capability.
>>
>>
>> Since we are seeing new use cases every year, we will need many
>> iterations to implement the fine grain permission. I think we need an
>> API that is flexible enough to cover different types of permission
>> control.
>>
>> For example, bpf_with_cap() can be flexible:
>>
>> bpf_with_cap(cmd, attr, size, perm_fd);
>>
>> We can get different types of permission via different combinations of
>> arguments:
>>
>> A perm_fd to /dev/bpf gives access to all sys_bpf() commands, so
>> this is "all or nothing" permission.
>>
>> A perm_fd to /sys/fs/cgroup/.../bpf.xxx would only allow some
>> commands to this specific cgroup.
>>
>
> I don't see why you need to invent a whole new mechanism for this.
> The entire cgroup ecosystem outside bpf() does just fine using the
> write permission on files in cgroupfs to control access. Why can't
> bpf() do the same thing?
It is easier to use write permission for BPF_PROG_ATTACH. But it is
not easy to do the same for other bpf commands: BPF_PROG_LOAD and
BPF_MAP_*. A lot of these commands don't have target concept. Maybe
we should have target concept for all these commands. But that is a
much bigger project. OTOH, "all or nothing" model allows all these
commands at once.
Well, that being said, I will look more into using write permission
in cgroupfs.
Thanks again for all these comments and suggestions. Please let us
know your future thoughts and insights.
Song
^ permalink raw reply
* Re: [RFC v2 0/6] Introduce TEE based Trusted Keys support
From: Janne Karhunen @ 2019-07-31 7:11 UTC (permalink / raw)
To: Sumit Garg
Cc: keyrings, linux-integrity, linux-security-module, jens.wiklander,
corbet, dhowells, jejb, jarkko.sakkinen, Mimi Zohar, James Morris,
Serge E. Hallyn, Casey Schaufler, ard.biesheuvel, daniel.thompson,
linux-doc, Linux Kernel Mailing List, linux-arm-kernel, tee-dev
In-Reply-To: <1564489420-677-1-git-send-email-sumit.garg@linaro.org>
Hi,
Interesting, I wrote something similar and posted it to the lists a while back:
https://github.com/jkrh/linux/commit/d77ea03afedcb5fd42234cd834da8f8a0809f6a6
Since there are no generic 'TEEs' available, I implemented the same
thing as a generic protocol translator. The shared memory binding for
instance already assumes fair amount about the TEE and how that is
physically present in the system. Besides, the help from usage of shm
is pretty limited due to the size of the keydata.
--
Janne
On Tue, Jul 30, 2019 at 3:26 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Add support for TEE based trusted keys where TEE provides the functionality
> to seal and unseal trusted keys using hardware unique key. Also, this is
> an alternative in case platform doesn't possess a TPM device.
>
> This series also adds some TEE features like:
>
> Patch #1, #2 enables support for registered kernel shared memory with TEE.
>
> Patch #3 enables support for private kernel login method required for
> cases like trusted keys where we don't wan't user-space to directly access
> TEE service to retrieve trusted key contents.
>
> Rest of the patches from #4 to #6 adds support for TEE based trusted keys.
>
> This patch-set has been tested with OP-TEE based pseudo TA which can be
> found here [1].
>
> Also, this patch-set is dependent on generic Trusted Keys framework
> patch-set [2].
>
> [1] https://github.com/OP-TEE/optee_os/pull/3082
> [2] https://lkml.org/lkml/2019/7/18/284
>
> Changes in v2:
> 1. Add reviewed-by tags for patch #1 and #2.
> 2. Incorporate comments from Jens for patch #3.
> 3. Switch to use generic trusted keys framework.
>
> Sumit Garg (6):
> tee: optee: allow kernel pages to register as shm
> tee: enable support to register kernel memory
> tee: add private login method for kernel clients
> KEYS: trusted: Introduce TEE based Trusted Keys
> doc: keys: Document usage of TEE based Trusted Keys
> MAINTAINERS: Add entry for TEE based Trusted Keys
>
> Documentation/security/keys/index.rst | 1 +
> Documentation/security/keys/tee-trusted.rst | 93 +++++++++
> MAINTAINERS | 9 +
> drivers/tee/optee/call.c | 7 +
> drivers/tee/tee_core.c | 6 +
> drivers/tee/tee_shm.c | 16 +-
> include/keys/trusted-type.h | 3 +
> include/keys/trusted_tee.h | 66 +++++++
> include/linux/tee_drv.h | 1 +
> include/uapi/linux/tee.h | 8 +
> security/keys/Kconfig | 3 +
> security/keys/trusted-keys/Makefile | 3 +-
> security/keys/trusted-keys/trusted-tee.c | 282 ++++++++++++++++++++++++++++
> security/keys/trusted-keys/trusted.c | 3 +
> 14 files changed, 498 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/security/keys/tee-trusted.rst
> create mode 100644 include/keys/trusted_tee.h
> create mode 100644 security/keys/trusted-keys/trusted-tee.c
>
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH bpf-next v10 10/10] landlock: Add user and kernel documentation for Landlock
From: Randy Dunlap @ 2019-07-31 1:53 UTC (permalink / raw)
To: Mickaël Salaün, linux-kernel
Cc: Alexander Viro, Alexei Starovoitov, Andrew Morton,
Andy Lutomirski, Arnaldo Carvalho de Melo, Casey Schaufler,
Daniel Borkmann, David Drysdale, David S . Miller,
Eric W . Biederman, James Morris, Jann Horn, John Johansen,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Paul Moore, Sargun Dhillon,
Serge E . Hallyn, Shuah Khan, Stephen Smalley, Tejun Heo,
Tetsuo Handa, Thomas Graf, Tycho Andersen, Will Drewry,
kernel-hardening, linux-api, linux-fsdevel, linux-security-module,
netdev
In-Reply-To: <20190721213116.23476-11-mic@digikod.net>
On 7/21/19 2:31 PM, Mickaël Salaün wrote:
> This documentation can be built with the Sphinx framework.
>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: James Morris <jmorris@namei.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Serge E. Hallyn <serge@hallyn.com>
> ---
>
> Changes since v9:
> * update with expected attach type and expected attach triggers
>
> Changes since v8:
> * remove documentation related to chaining and tagging according to this
> patch series
>
> Changes since v7:
> * update documentation according to the Landlock revamp
>
> Changes since v6:
> * add a check for ctx->event
> * rename BPF_PROG_TYPE_LANDLOCK to BPF_PROG_TYPE_LANDLOCK_RULE
> * rename Landlock version to ABI to better reflect its purpose and add a
> dedicated changelog section
> * update tables
> * relax no_new_privs recommendations
> * remove ABILITY_WRITE related functions
> * reword rule "appending" to "prepending" and explain it
> * cosmetic fixes
>
> Changes since v5:
> * update the rule hierarchy inheritance explanation
> * briefly explain ctx->arg2
> * add ptrace restrictions
> * explain EPERM
> * update example (subtype)
> * use ":manpage:"
> ---
> Documentation/security/index.rst | 1 +
> Documentation/security/landlock/index.rst | 20 +++
> Documentation/security/landlock/kernel.rst | 99 ++++++++++++++
> Documentation/security/landlock/user.rst | 147 +++++++++++++++++++++
> 4 files changed, 267 insertions(+)
> create mode 100644 Documentation/security/landlock/index.rst
> create mode 100644 Documentation/security/landlock/kernel.rst
> create mode 100644 Documentation/security/landlock/user.rst
> diff --git a/Documentation/security/landlock/kernel.rst b/Documentation/security/landlock/kernel.rst
> new file mode 100644
> index 000000000000..7d1e06d544bf
> --- /dev/null
> +++ b/Documentation/security/landlock/kernel.rst
> @@ -0,0 +1,99 @@
> +==============================
> +Landlock: kernel documentation
> +==============================
> +
> +eBPF properties
> +===============
> +
> +To get an expressive language while still being safe and small, Landlock is
> +based on eBPF. Landlock should be usable by untrusted processes and must
> +therefore expose a minimal attack surface. The eBPF bytecode is minimal,
> +powerful, widely used and designed to be used by untrusted applications. Thus,
> +reusing the eBPF support in the kernel enables a generic approach while
> +minimizing new code.
> +
> +An eBPF program has access to an eBPF context containing some fields used to
> +inspect the current object. These arguments can be used directly (e.g. cookie)
> +or passed to helper functions according to their types (e.g. inode pointer). It
> +is then possible to do complex access checks without race conditions or
> +inconsistent evaluation (i.e. `incorrect mirroring of the OS code and state
> +<https://www.ndss-symposium.org/ndss2003/traps-and-pitfalls-practical-problems-system-call-interposition-based-security-tools/>`_).
> +
> +A Landlock hook describes a particular access type. For now, there is two
there are two
> +hooks dedicated to filesystem related operations: LANDLOCK_HOOK_FS_PICK and
> +LANDLOCK_HOOK_FS_WALK. A Landlock program is tied to one hook. This makes it
> +possible to statically check context accesses, potentially performed by such
> +program, and hence prevents kernel address leaks and ensure the right use of
ensures
> +hook arguments with eBPF functions. Any user can add multiple Landlock
> +programs per Landlock hook. They are stacked and evaluated one after the
> +other, starting from the most recent program, as seccomp-bpf does with its
> +filters. Underneath, a hook is an abstraction over a set of LSM hooks.
> +
> +
> +Guiding principles
> +==================
> +
> +Unprivileged use
> +----------------
> +
> +* Landlock helpers and context should be usable by any unprivileged and
> + untrusted program while following the system security policy enforced by
> + other access control mechanisms (e.g. DAC, LSM).
> +
> +
> +Landlock hook and context
> +-------------------------
> +
> +* A Landlock hook shall be focused on access control on kernel objects instead
> + of syscall filtering (i.e. syscall arguments), which is the purpose of
> + seccomp-bpf.
> +* A Landlock context provided by a hook shall express the minimal and more
> + generic interface to control an access for a kernel object.
> +* A hook shall guaranty that all the BPF function calls from a program are> + safe. Thus, the related Landlock context arguments shall always be of the
> + same type for a particular hook. For example, a network hook could share
> + helpers with a file hook because of UNIX socket. However, the same helpers
> + may not be compatible for a file system handle and a net handle.
> +* Multiple hooks may use the same context interface.
> +
> +
> +Landlock helpers
> +----------------
> +
> +* Landlock helpers shall be as generic as possible while at the same time being
> + as simple as possible and following the syscall creation principles (cf.
> + *Documentation/adding-syscalls.txt*).
> +* The only behavior change allowed on a helper is to fix a (logical) bug to
> + match the initial semantic.
> +* Helpers shall be reentrant, i.e. only take inputs from arguments (e.g. from
> + the BPF context), to enable a hook to use a cache. Future program options
> + might change this cache behavior.
> +* It is quite easy to add new helpers to extend Landlock. The main concern
> + should be about the possibility to leak information from the kernel that may
> + not be accessible otherwise (i.e. side-channel attack).
> +
> +
> +Questions and answers
> +=====================
> +
> +Why not create a custom hook for each kind of action?
> +-----------------------------------------------------
> +
> +Landlock programs can handle these checks. Adding more exceptions to the
> +kernel code would lead to more code complexity. A decision to ignore a kind of
> +action can and should be done at the beginning of a Landlock program.
> +
> +
> +Why a program does not return an errno or a kill code?
> +------------------------------------------------------
> +
> +seccomp filters can return multiple kind of code, including an errno value or a
kinds
> +kill signal, which may be convenient for access control. Those return codes
> +are hardwired in the userland ABI. Instead, Landlock's approach is to return a
> +boolean to allow or deny an action, which is much simpler and more generic.
> +Moreover, we do not really have a choice because, unlike to seccomp, Landlock
> +programs are not enforced at the syscall entry point but may be executed at any
> +point in the kernel (through LSM hooks) where an errno return code may not make
> +sense. However, with this simple ABI and with the ability to call helpers,
> +Landlock may gain features similar to seccomp-bpf in the future while being
> +compatible with previous programs.
> diff --git a/Documentation/security/landlock/user.rst b/Documentation/security/landlock/user.rst
> new file mode 100644
> index 000000000000..14c4f3b377bd
> --- /dev/null
> +++ b/Documentation/security/landlock/user.rst
> @@ -0,0 +1,147 @@
> +================================
> +Landlock: userland documentation
> +================================
> +
> +Landlock programs
> +=================
> +
> +eBPF programs are used to create security programs. They are contained and can
> +call only a whitelist of dedicated functions. Moreover, they can only loop
> +under strict conditions, which protects from denial of service. More
> +information on BPF can be found in *Documentation/networking/filter.txt*.
> +
> +
> +Writing a program
> +-----------------
> +
> +To enforce a security policy, a thread first needs to create a Landlock program.
> +The easiest way to write an eBPF program depicting a security program is to write
> +it in the C language. As described in *samples/bpf/README.rst*, LLVM can
> +compile such programs. Files *samples/bpf/landlock1_kern.c* and those in
> +*tools/testing/selftests/landlock/* can be used as examples.
> +
> +Once the eBPF program is created, the next step is to create the metadata
> +describing the Landlock program. This metadata includes an expected attach type which
> +contains the hook type to which the program is tied, and expected attach
> +triggers which identify the actions for which the program should be run.
> +
> +A hook is a policy decision point which exposes the same context type for
> +each program evaluation.
> +
> +A Landlock hook describes the kind of kernel object for which a program will be
> +triggered to allow or deny an action. For example, the hook
> +BPF_LANDLOCK_FS_PICK can be triggered every time a landlocked thread performs a
> +set of action related to the filesystem (e.g. open, read, write, mount...).
actions
> +This actions are identified by the `triggers` bitfield.
> +
> +The next step is to fill a :c:type:`struct bpf_load_program_attr
> +<bpf_load_program_attr>` with BPF_PROG_TYPE_LANDLOCK_HOOK, the expected attach
> +type and other BPF program metadata. This bpf_attr must then be passed to the
> +:manpage:`bpf(2)` syscall alongside the BPF_PROG_LOAD command. If everything
> +is deemed correct by the kernel, the thread gets a file descriptor referring to
> +this program.
> +
> +In the following code, the *insn* variable is an array of BPF instructions
> +which can be extracted from an ELF file as is done in bpf_load_file() from
> +*samples/bpf/bpf_load.c*.
A little confusing. Is there a mixup of <insn> and <insns>?
> +
> +.. code-block:: c
> +
> + int prog_fd;
> + struct bpf_load_program_attr load_attr;
> +
> + memset(&load_attr, 0, sizeof(struct bpf_load_program_attr));
> + load_attr.prog_type = BPF_PROG_TYPE_LANDLOCK_HOOK;
> + load_attr.expected_attach_type = BPF_LANDLOCK_FS_PICK;
> + load_attr.expected_attach_triggers = LANDLOCK_TRIGGER_FS_PICK_OPEN;
> + load_attr.insns = insns;
> + load_attr.insns_cnt = sizeof(insn) / sizeof(struct bpf_insn);
> + load_attr.license = "GPL";
> +
> + prog_fd = bpf_load_program_xattr(&load_attr, log_buf, log_buf_sz);
> + if (prog_fd == -1)
> + exit(1);
> +
> +
> +Enforcing a program
> +-------------------
> +
> +Once the Landlock program has been created or received (e.g. through a UNIX
> +socket), the thread willing to sandbox itself (and its future children) should
> +perform the following two steps.
> +
> +The thread should first request to never be allowed to get new privileges with a
> +call to :manpage:`prctl(2)` and the PR_SET_NO_NEW_PRIVS option. More
> +information can be found in *Documentation/prctl/no_new_privs.txt*.
> +
> +.. code-block:: c
> +
> + if (prctl(PR_SET_NO_NEW_PRIVS, 1, NULL, 0, 0))
> + exit(1);
> +
> +A thread can apply a program to itself by using the :manpage:`seccomp(2)` syscall.
> +The operation is SECCOMP_PREPEND_LANDLOCK_PROG, the flags must be empty and the
> +*args* argument must point to a valid Landlock program file descriptor.
> +
> +.. code-block:: c
> +
> + if (seccomp(SECCOMP_PREPEND_LANDLOCK_PROG, 0, &fd))
> + exit(1);
> +
> +If the syscall succeeds, the program is now enforced on the calling thread and
> +will be enforced on all its subsequently created children of the thread as
> +well. Once a thread is landlocked, there is no way to remove this security
> +policy, only stacking more restrictions is allowed. The program evaluation is
> +performed from the newest to the oldest.
> +
> +When a syscall ask for an action on a kernel object, if this action is denied,
asks
> +then an EACCES errno code is returned through the syscall.
> +
> +
> +.. _inherited_programs:
> +
> +Inherited programs
> +------------------
> +
> +Every new thread resulting from a :manpage:`clone(2)` inherits Landlock program
> +restrictions from its parent. This is similar to the seccomp inheritance as
> +described in *Documentation/prctl/seccomp_filter.txt*.
> +
> +
> +Ptrace restrictions
> +-------------------
> +
> +A landlocked process has less privileges than a non-landlocked process and must
> +then be subject to additional restrictions when manipulating another process.
> +To be allowed to use :manpage:`ptrace(2)` and related syscalls on a target
> +process, a landlocked process must have a subset of the target process programs.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Maybe that last statement is correct, but it seems to me that it is missing something.
> +
> +
> +Landlock structures and constants
> +=================================
> +
> +Hook types
> +----------
> +
> +.. kernel-doc:: include/uapi/linux/landlock.h
> + :functions: landlock_hook_type
> +
> +
> +Contexts
> +--------
> +
> +.. kernel-doc:: include/uapi/linux/landlock.h
> + :functions: landlock_ctx_fs_pick landlock_ctx_fs_walk landlock_ctx_fs_get
> +
> +
> +Triggers for fs_pick
> +--------------------
> +
> +.. kernel-doc:: include/uapi/linux/landlock.h
> + :functions: landlock_triggers
> +
> +
> +Additional documentation
> +========================
> +
> +See https://landlock.io
>
--
~Randy
^ permalink raw reply
* Re: [PATCH] tracefs: Restrict tracefs when the kernel is locked down
From: Steven Rostedt @ 2019-07-31 1:48 UTC (permalink / raw)
To: Matthew Garrett
Cc: jmorris, linux-security-module, linux-kernel, linux-api,
Matthew Garrett
In-Reply-To: <20190730184734.202386-1-matthewgarrett@google.com>
On Tue, 30 Jul 2019 11:47:34 -0700
Matthew Garrett <matthewgarrett@google.com> wrote:
> Tracefs may release more information about the kernel than desirable, so
> restrict it when the kernel is locked down in confidentiality mode by
> preventing open().
>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
-- Steve
^ permalink raw reply
* Re: [PATCH 1/2] KEYS: Replace uid/gid/perm permissions checking with an ACL
From: Eric Biggers @ 2019-07-31 1:16 UTC (permalink / raw)
To: David Howells, keyrings, linux-security-module, linux-fsdevel,
linux-kernel
In-Reply-To: <20190730034956.GB1966@sol.localdomain>
On Mon, Jul 29, 2019 at 08:49:56PM -0700, Eric Biggers wrote:
> Hi David,
>
> On Tue, Jul 09, 2019 at 06:16:01PM -0700, Eric Biggers wrote:
> > On Thu, May 23, 2019 at 04:58:27PM +0100, David Howells wrote:
> > > Replace the uid/gid/perm permissions checking on a key with an ACL to allow
> > > the SETATTR and SEARCH permissions to be split. This will also allow a
> > > greater range of subjects to represented.
> > >
> >
> > This patch broke 'keyctl new_session', and hence broke all the fscrypt tests:
> >
> > $ keyctl new_session
> > keyctl_session_to_parent: Permission denied
> >
> > Output of 'keyctl show' is
> >
> > $ keyctl show
> > Session Keyring
> > 605894913 --alswrv 0 0 keyring: _ses
> > 189223103 ----s-rv 0 0 \_ user: invocation_id
> >
> > - Eric
>
> This bug is still present in next-20190729.
>
> - Eric
This fixes it:
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index aa3bfcadbc660..519c94f1cc3c2 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -58,7 +58,7 @@ static struct key_acl session_keyring_acl = {
.possessor_viewable = true,
.nr_ace = 2,
.aces = {
- KEY_POSSESSOR_ACE(KEY_ACE__PERMS & ~KEY_ACE_JOIN),
+ KEY_POSSESSOR_ACE(KEY_ACE__PERMS),
KEY_OWNER_ACE(KEY_ACE_VIEW | KEY_ACE_READ),
}
};
The old permissions were KEY_POS_ALL | KEY_USR_VIEW | KEY_USR_READ, so
I'm not sure why JOIN permission was removed?
- Eric
^ permalink raw reply related
* Re: [PATCH v12 0/5] overlayfs override_creds=off
From: Casey Schaufler @ 2019-07-30 21:37 UTC (permalink / raw)
To: Mark Salyzyn, linux-kernel
Cc: kernel-team, Miklos Szeredi, Jonathan Corbet, Vivek Goyal,
Eric W . Biederman, Amir Goldstein, Randy Dunlap, Stephen Smalley,
linux-unionfs, linux-doc, Linux Security Module list
In-Reply-To: <20190730172904.79146-1-salyzyn@android.com>
On 7/30/2019 10:28 AM, Mark Salyzyn wrote:
> Patch series:
Please add linux-security-module@vger.kernel.org to the CC
for all changes affecting handling of security xattrs.
>
> overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh
> Add flags option to get xattr method paired to __vfs_getxattr
> overlayfs: handle XATTR_NOSECURITY flag for get xattr method
> overlayfs: internal getxattr operations without sepolicy checking
> overlayfs: override_creds=off option bypass creator_cred
>
> The first four patches address fundamental security issues that should
> be solved regardless of the override_creds=off feature.
> on them).
>
> The fifth adds the feature depends on these other fixes.
>
> By default, all access to the upper, lower and work directories is the
> recorded mounter's MAC and DAC credentials. The incoming accesses are
> checked against the caller's credentials.
>
> If the principles of least privilege are applied for sepolicy, the
> mounter's credentials might not overlap the credentials of the caller's
> when accessing the overlayfs filesystem. For example, a file that a
> lower DAC privileged caller can execute, is MAC denied to the
> generally higher DAC privileged mounter, to prevent an attack vector.
>
> We add the option to turn off override_creds in the mount options; all
> subsequent operations after mount on the filesystem will be only the
> caller's credentials. The module boolean parameter and mount option
> override_creds is also added as a presence check for this "feature",
> existence of /sys/module/overlay/parameters/overlay_creds
>
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: linux-unionfs@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
>
> ---
> v12:
> - Restore squished out patch 2 and 3 in the series,
> then change algorithm to add flags argument.
> Per-thread flag is a large security surface.
>
> v11:
> - Squish out v10 introduced patch 2 and 3 in the series,
> then and use per-thread flag instead for nesting.
> - Switch name to ovl_do_vds_getxattr for __vds_getxattr wrapper.
> - Add sb argument to ovl_revert_creds to match future work.
>
> v10:
> - Return NULL on CAP_DAC_READ_SEARCH
> - Add __get xattr method to solve sepolicy logging issue
> - Drop unnecessary sys_admin sepolicy checking for administrative
> driver internal xattr functions.
>
> v6:
> - Drop CONFIG_OVERLAY_FS_OVERRIDE_CREDS.
> - Do better with the documentation, drop rationalizations.
> - pr_warn message adjusted to report consequences.
>
> v5:
> - beefed up the caveats in the Documentation
> - Is dependent on
> "overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh"
> "overlayfs: check CAP_MKNOD before issuing vfs_whiteout"
> - Added prwarn when override_creds=off
>
> v4:
> - spelling and grammar errors in text
>
> v3:
> - Change name from caller_credentials / creator_credentials to the
> boolean override_creds.
> - Changed from creator to mounter credentials.
> - Updated and fortified the documentation.
> - Added CONFIG_OVERLAY_FS_OVERRIDE_CREDS
>
> v2:
> - Forward port changed attr to stat, resulting in a build error.
> - altered commit message.
>
^ permalink raw reply
* Re: [PATCH v2 bpf-next 1/4] bpf: unprivileged BPF access via /dev/bpf
From: Andy Lutomirski @ 2019-07-30 20:24 UTC (permalink / raw)
To: Song Liu
Cc: Andy Lutomirski, Kees Cook, Networking, bpf, Alexei Starovoitov,
Daniel Borkmann, Kernel Team, Lorenz Bauer, Jann Horn, Greg KH,
Linux API, LSM List
In-Reply-To: <369476A8-4CE1-43DA-9239-06437C0384C7@fb.com>
On Mon, Jul 29, 2019 at 10:07 PM Song Liu <songliubraving@fb.com> wrote:
>
> Hi Andy,
>
> > On Jul 27, 2019, at 11:20 AM, Song Liu <songliubraving@fb.com> wrote:
> >
> > Hi Andy,
> >
> >>>>>
> >>>>
> >>>> Well, yes. sys_bpf() is pretty powerful.
> >>>>
> >>>> The goal of /dev/bpf is to enable special users to call sys_bpf(). In
> >>>> the meanwhile, such users should not take down the whole system easily
> >>>> by accident, e.g., with rm -rf /.
> >>>
> >>> That’s easy, though — bpftool could learn to read /etc/bpfusers before allowing ruid != 0.
> >>
> >> This is a great idea! fscaps + /etc/bpfusers should do the trick.
> >
> > After some discussions and more thinking on this, I have some concerns
> > with the user space only approach.
> >
> > IIUC, your proposal for user space only approach is like:
> >
> > 1. bpftool (and other tools) check /etc/bpfusers and only do
> > setuid for allowed users:
> >
> > int main()
> > {
> > if (/* uid in /etc/bpfusers */)
> > setuid(0);
> > sys_bpf(...);
> > }
> >
> > 2. bpftool (and other tools) is installed with CAP_SETUID:
> >
> > setcap cap_setuid=e+p /bin/bpftool
> >
> > 3. sys admin maintains proper /etc/bpfusers.
> >
> > This approach is not ideal, because we need to trust the tool to give
> > it CAP_SETUID. A hacked tool could easily bypass /etc/bpfusers check
> > or use other root only sys calls after setuid(0).
> >
>
> I would like more comments on this.
>
> Currently, bpf permission is more or less "root or nothing", which we
> would like to change.
>
> The short term goal is to separate bpf from root, in other words, it is
> "all or nothing". Special user space utilities, such as systemd, would
> benefit from this. Once this is implemented, systemd can call sys_bpf()
> when it is not running as root.
As generally nasty as Linux capabilities are, this sounds like a good
use for CAP_BPF_ADMIN.
But what do you have in mind? Isn't non-root systemd mostly just the
user systemd session? That should *not* have bpf() privileges until
bpf() is improved such that you can't use it to compromise the system.
>
> In longer term, it may be useful to provide finer grain permission of
> sys_bpf(). For example, sys_bpf() should be aware of containers; and
> user may only have access to certain bpf maps. Let's call this
> "fine grain" capability.
>
>
> Since we are seeing new use cases every year, we will need many
> iterations to implement the fine grain permission. I think we need an
> API that is flexible enough to cover different types of permission
> control.
>
> For example, bpf_with_cap() can be flexible:
>
> bpf_with_cap(cmd, attr, size, perm_fd);
>
> We can get different types of permission via different combinations of
> arguments:
>
> A perm_fd to /dev/bpf gives access to all sys_bpf() commands, so
> this is "all or nothing" permission.
>
> A perm_fd to /sys/fs/cgroup/.../bpf.xxx would only allow some
> commands to this specific cgroup.
>
I don't see why you need to invent a whole new mechanism for this.
The entire cgroup ecosystem outside bpf() does just fine using the
write permission on files in cgroupfs to control access. Why can't
bpf() do the same thing?
>
> Alexei raised another idea in offline discussions: instead of adding
> bpf_with_cap(), we add a command LOAD_PERM_FD, which enables special
> permission for the _next_ sys_bpf() from current task:
>
> bpf(LOAD_PERM_FD, perm_fd);
> /* the next sys_bpf() uses permission from perm_fd */
> bpf(cmd, attr, size);
>
> This is equivalent to bpf_with_cap(cmd, attr, size, perm_fd), but
> doesn't require the new sys call.
That sounds almost every bit as problematic as the approach where you
ask for permission once and it sticks.
>
> 1. User space only approach doesn't work, even for "all or nothing"
> permission control. I expanded the discussion in the previous
> email. Please let me know if I missed anything there.
As in my previous email, I disagree.
^ permalink raw reply
* [PATCH] tracefs: Restrict tracefs when the kernel is locked down
From: Matthew Garrett @ 2019-07-30 18:47 UTC (permalink / raw)
To: Steven Rostedt
Cc: jmorris, linux-security-module, linux-kernel, linux-api,
Matthew Garrett, Matthew Garrett
In-Reply-To: <20190724222354.7cbd6c6e@oasis.local.home>
Tracefs may release more information about the kernel than desirable, so
restrict it when the kernel is locked down in confidentiality mode by
preventing open().
Signed-off-by: Matthew Garrett <mjg59@google.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
Added the iput()
fs/tracefs/inode.c | 40 +++++++++++++++++++++++++++++++++++-
include/linux/security.h | 1 +
security/lockdown/lockdown.c | 1 +
3 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index eeeae0475da9..fb4d1d89ce53 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -20,6 +20,7 @@
#include <linux/parser.h>
#include <linux/magic.h>
#include <linux/slab.h>
+#include <linux/security.h>
#define TRACEFS_DEFAULT_MODE 0700
@@ -27,6 +28,23 @@ static struct vfsmount *tracefs_mount;
static int tracefs_mount_count;
static bool tracefs_registered;
+static int default_open_file(struct inode *inode, struct file *filp)
+{
+ struct dentry *dentry = filp->f_path.dentry;
+ struct file_operations *real_fops;
+ int ret;
+
+ if (!dentry)
+ return -EINVAL;
+
+ ret = security_locked_down(LOCKDOWN_TRACEFS);
+ if (ret)
+ return ret;
+
+ real_fops = dentry->d_fsdata;
+ return real_fops->open(inode, filp);
+}
+
static ssize_t default_read_file(struct file *file, char __user *buf,
size_t count, loff_t *ppos)
{
@@ -221,6 +239,12 @@ static int tracefs_apply_options(struct super_block *sb)
return 0;
}
+static void tracefs_destroy_inode(struct inode *inode)
+{
+ if (S_ISREG(inode->i_mode))
+ kfree(inode->i_fop);
+}
+
static int tracefs_remount(struct super_block *sb, int *flags, char *data)
{
int err;
@@ -256,6 +280,7 @@ static int tracefs_show_options(struct seq_file *m, struct dentry *root)
static const struct super_operations tracefs_super_operations = {
.statfs = simple_statfs,
+ .destroy_inode = tracefs_destroy_inode,
.remount_fs = tracefs_remount,
.show_options = tracefs_show_options,
};
@@ -387,6 +412,7 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
struct dentry *parent, void *data,
const struct file_operations *fops)
{
+ struct file_operations *proxy_fops;
struct dentry *dentry;
struct inode *inode;
@@ -402,8 +428,20 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
if (unlikely(!inode))
return failed_creating(dentry);
+ proxy_fops = kzalloc(sizeof(struct file_operations), GFP_KERNEL);
+ if (unlikely(!proxy_fops)) {
+ iput(inode);
+ return failed_creating(dentry);
+ }
+
+ if (!fops)
+ fops = &tracefs_file_operations;
+
+ dentry->d_fsdata = (void *)fops;
+ memcpy(proxy_fops, fops, sizeof(*proxy_fops));
+ proxy_fops->open = default_open_file;
inode->i_mode = mode;
- inode->i_fop = fops ? fops : &tracefs_file_operations;
+ inode->i_fop = proxy_fops;
inode->i_private = data;
d_instantiate(dentry, inode);
fsnotify_create(dentry->d_parent->d_inode, dentry);
diff --git a/include/linux/security.h b/include/linux/security.h
index d92323b44a3f..807dc0d24982 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -121,6 +121,7 @@ enum lockdown_reason {
LOCKDOWN_KPROBES,
LOCKDOWN_BPF_READ,
LOCKDOWN_PERF,
+ LOCKDOWN_TRACEFS,
LOCKDOWN_CONFIDENTIALITY_MAX,
};
diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
index 88064ce1c844..173191562047 100644
--- a/security/lockdown/lockdown.c
+++ b/security/lockdown/lockdown.c
@@ -36,6 +36,7 @@ static char *lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
[LOCKDOWN_KPROBES] = "use of kprobes",
[LOCKDOWN_BPF_READ] = "use of bpf to read kernel RAM",
[LOCKDOWN_PERF] = "unsafe use of perf",
+ [LOCKDOWN_TRACEFS] = "use of tracefs",
[LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality",
};
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related
* [RFC v2 6/6] MAINTAINERS: Add entry for TEE based Trusted Keys
From: Sumit Garg @ 2019-07-30 12:23 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-security-module
Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
jmorris, serge, casey, ard.biesheuvel, daniel.thompson, linux-doc,
linux-kernel, linux-arm-kernel, tee-dev, Sumit Garg
In-Reply-To: <1564489420-677-1-git-send-email-sumit.garg@linaro.org>
Add MAINTAINERS entry for TEE based Trusted Keys framework.
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
MAINTAINERS | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index ce06877..0b61ecf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8930,6 +8930,15 @@ F: include/keys/trusted-type.h
F: security/keys/trusted.c
F: security/keys/trusted.h
+KEYS-TEE-TRUSTED
+M: Sumit Garg <sumit.garg@linaro.org>
+L: linux-integrity@vger.kernel.org
+L: keyrings@vger.kernel.org
+S: Supported
+F: Documentation/security/keys/tee-trusted.rst
+F: include/keys/trusted_tee.h
+F: security/keys/trusted-keys/trusted-tee.c
+
KEYS/KEYRINGS:
M: David Howells <dhowells@redhat.com>
L: keyrings@vger.kernel.org
--
2.7.4
^ permalink raw reply related
* [RFC v2 5/6] doc: keys: Document usage of TEE based Trusted Keys
From: Sumit Garg @ 2019-07-30 12:23 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-security-module
Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
jmorris, serge, casey, ard.biesheuvel, daniel.thompson, linux-doc,
linux-kernel, linux-arm-kernel, tee-dev, Sumit Garg
In-Reply-To: <1564489420-677-1-git-send-email-sumit.garg@linaro.org>
Provide documentation for usage of TEE based Trusted Keys via existing
user-space "keyctl" utility. Also, document various use-cases.
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
Documentation/security/keys/index.rst | 1 +
Documentation/security/keys/tee-trusted.rst | 93 +++++++++++++++++++++++++++++
2 files changed, 94 insertions(+)
create mode 100644 Documentation/security/keys/tee-trusted.rst
diff --git a/Documentation/security/keys/index.rst b/Documentation/security/keys/index.rst
index 647d58f..f9ef557 100644
--- a/Documentation/security/keys/index.rst
+++ b/Documentation/security/keys/index.rst
@@ -9,3 +9,4 @@ Kernel Keys
ecryptfs
request-key
trusted-encrypted
+ tee-trusted
diff --git a/Documentation/security/keys/tee-trusted.rst b/Documentation/security/keys/tee-trusted.rst
new file mode 100644
index 0000000..ef03745
--- /dev/null
+++ b/Documentation/security/keys/tee-trusted.rst
@@ -0,0 +1,93 @@
+======================
+TEE based Trusted Keys
+======================
+
+TEE based Trusted Keys provides an alternative approach for providing Trusted
+Keys in case TPM chip isn't present.
+
+Trusted Keys use a TEE service/device both to generate and to seal the keys.
+Keys are sealed under a hardware unique key in the TEE, and only unsealed by
+the TEE.
+
+For more information about TEE, refer to ``Documentation/tee.txt``.
+
+Usage::
+
+ keyctl add trusted name "new keylen" ring
+ keyctl add trusted name "load hex_blob" ring
+ keyctl print keyid
+
+"keyctl print" returns an ascii hex copy of the sealed key, which is in format
+specific to TEE device implementation. The key length for new keys are always
+in bytes. Trusted Keys can be 32 - 128 bytes (256 - 1024 bits).
+
+Examples of trusted key and its usage as 'master' key for encrypted key usage:
+
+More details about encrypted keys can be found here:
+``Documentation/security/keys/trusted-encrypted.rst``
+
+Create and save a trusted key named "kmk" of length 32 bytes::
+
+ $ keyctl add trusted kmk "new 32" @u
+ 754414669
+
+ $ keyctl show
+ Session Keyring
+ 827385718 --alswrv 0 65534 keyring: _uid_ses.0
+ 274124851 --alswrv 0 65534 \_ keyring: _uid.0
+ 754414669 --als-rv 0 0 \_ trusted: kmk
+
+ $ keyctl print 754414669
+ 15676790697861b422175596ae001c2f505cea2c6f3ebbc5fb08eeb1f343a07e
+
+ $ keyctl pipe 754414669 > kmk.blob
+
+Load a trusted key from the saved blob::
+
+ $ keyctl add trusted kmk "load `cat kmk.blob`" @u
+ 491638700
+
+ $ keyctl print 491638700
+ 15676790697861b422175596ae001c2f505cea2c6f3ebbc5fb08eeb1f343a07e
+
+The initial consumer of trusted keys is EVM, which at boot time needs a high
+quality symmetric key for HMAC protection of file metadata. The use of a
+TEE based trusted key provides security that the EVM key has not been
+compromised by a user level problem and tied to particular hardware.
+
+Create and save an encrypted key "evm" using the above trusted key "kmk":
+
+option 1: omitting 'format'::
+
+ $ keyctl add encrypted evm "new trusted:kmk 32" @u
+ 608915065
+
+option 2: explicitly defining 'format' as 'default'::
+
+ $ keyctl add encrypted evm "new default trusted:kmk 32" @u
+ 608915065
+
+ $ keyctl print 608915065
+ default trusted:kmk 32 f380ac588a925f488d5be007cf23e4c900b8b652ab62241c8
+ ed54906189b6659d139d619d4b51752a2645537b11fd44673f13154a65b3f595d5fb2131
+ 2fe45529ea0407c644ea4026f2a1a75661f2c9b66
+
+ $ keyctl pipe 608915065 > evm.blob
+
+Load an encrypted key "evm" from saved blob::
+
+ $ keyctl add encrypted evm "load `cat evm.blob`" @u
+ 831684262
+
+ $ keyctl print 831684262
+ default trusted:kmk 32 f380ac588a925f488d5be007cf23e4c900b8b652ab62241c8
+ ed54906189b6659d139d619d4b51752a2645537b11fd44673f13154a65b3f595d5fb2131
+ 2fe45529ea0407c644ea4026f2a1a75661f2c9b66
+
+Other uses for trusted and encrypted keys, such as for disk and file encryption
+are anticipated. In particular the 'ecryptfs' encrypted keys format can be used
+to mount an eCryptfs filesystem. More details about the usage can be found in
+the file ``Documentation/security/keys/ecryptfs.rst``.
+
+Another format 'enc32' can be used to support encrypted keys with payload size
+of 32 bytes.
--
2.7.4
^ permalink raw reply related
* [RFC v2 4/6] KEYS: trusted: Introduce TEE based Trusted Keys
From: Sumit Garg @ 2019-07-30 12:23 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-security-module
Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
jmorris, serge, casey, ard.biesheuvel, daniel.thompson, linux-doc,
linux-kernel, linux-arm-kernel, tee-dev, Sumit Garg
In-Reply-To: <1564489420-677-1-git-send-email-sumit.garg@linaro.org>
Add support for TEE based trusted keys where TEE provides the functionality
to seal and unseal trusted keys using hardware unique key.
Refer to Documentation/tee.txt for detailed information about TEE.
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
include/keys/trusted-type.h | 3 +
include/keys/trusted_tee.h | 66 ++++++++
security/keys/Kconfig | 3 +
security/keys/trusted-keys/Makefile | 3 +-
security/keys/trusted-keys/trusted-tee.c | 282 +++++++++++++++++++++++++++++++
security/keys/trusted-keys/trusted.c | 3 +
6 files changed, 359 insertions(+), 1 deletion(-)
create mode 100644 include/keys/trusted_tee.h
create mode 100644 security/keys/trusted-keys/trusted-tee.c
diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h
index 5559010..e0df5df 100644
--- a/include/keys/trusted-type.h
+++ b/include/keys/trusted-type.h
@@ -67,6 +67,9 @@ extern struct key_type key_type_trusted;
#if defined(CONFIG_TCG_TPM)
extern struct trusted_key_ops tpm_trusted_key_ops;
#endif
+#if defined(CONFIG_TEE)
+extern struct trusted_key_ops tee_trusted_key_ops;
+#endif
#define TRUSTED_DEBUG 0
diff --git a/include/keys/trusted_tee.h b/include/keys/trusted_tee.h
new file mode 100644
index 0000000..ab58ffd
--- /dev/null
+++ b/include/keys/trusted_tee.h
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2019 Linaro Ltd.
+ *
+ * Author:
+ * Sumit Garg <sumit.garg@linaro.org>
+ */
+
+#ifndef __TEE_TRUSTED_KEY_H
+#define __TEE_TRUSTED_KEY_H
+
+#include <linux/tee_drv.h>
+
+#define DRIVER_NAME "tee-trusted-key"
+
+/*
+ * Get random data for symmetric key
+ *
+ * [out] memref[0] Random data
+ *
+ * Result:
+ * TEE_SUCCESS - Invoke command success
+ * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
+ */
+#define TA_CMD_GET_RANDOM 0x0
+
+/*
+ * Seal trusted key using hardware unique key
+ *
+ * [in] memref[0] Plain key
+ * [out] memref[1] Sealed key datablob
+ *
+ * Result:
+ * TEE_SUCCESS - Invoke command success
+ * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
+ */
+#define TA_CMD_SEAL 0x1
+
+/*
+ * Unseal trusted key using hardware unique key
+ *
+ * [in] memref[0] Sealed key datablob
+ * [out] memref[1] Plain key
+ *
+ * Result:
+ * TEE_SUCCESS - Invoke command success
+ * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
+ */
+#define TA_CMD_UNSEAL 0x2
+
+/**
+ * struct trusted_key_private - TEE Trusted key private data
+ * @dev: TEE based Trusted key device.
+ * @ctx: TEE context handler.
+ * @session_id: Trusted key TA session identifier.
+ * @shm_pool: Memory pool shared with TEE device.
+ */
+struct trusted_key_private {
+ struct device *dev;
+ struct tee_context *ctx;
+ u32 session_id;
+ u32 data_rate;
+ struct tee_shm *shm_pool;
+};
+
+#endif
diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index dd31343..0d5e37c 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -88,6 +88,9 @@ config TRUSTED_KEYS
if the boot PCRs and other criteria match. Userspace will only ever
see encrypted blobs.
+ It also provides support for alternative TEE based Trusted keys
+ generation and sealing in case TPM isn't present.
+
If you are unsure as to whether this is required, answer N.
config ENCRYPTED_KEYS
diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
index 6ecadfb..5fcf2ae 100644
--- a/security/keys/trusted-keys/Makefile
+++ b/security/keys/trusted-keys/Makefile
@@ -4,4 +4,5 @@
#
obj-$(CONFIG_TRUSTED_KEYS) += trusted.o \
- trusted-tpm.o
+ trusted-tpm.o \
+ trusted-tee.o
diff --git a/security/keys/trusted-keys/trusted-tee.c b/security/keys/trusted-keys/trusted-tee.c
new file mode 100644
index 0000000..724a73c
--- /dev/null
+++ b/security/keys/trusted-keys/trusted-tee.c
@@ -0,0 +1,282 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Linaro Ltd.
+ *
+ * Author:
+ * Sumit Garg <sumit.garg@linaro.org>
+ */
+
+#include <linux/err.h>
+#include <linux/key-type.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/uuid.h>
+
+#include <keys/trusted-type.h>
+#include <keys/trusted_tee.h>
+
+static struct trusted_key_private pvt_data;
+
+/*
+ * Have the TEE seal(encrypt) the symmetric key
+ */
+static int tee_key_seal(struct trusted_key_payload *p, char *datablob)
+{
+ int ret = 0;
+ struct tee_ioctl_invoke_arg inv_arg;
+ struct tee_param param[4];
+ struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
+
+ memset(&inv_arg, 0, sizeof(inv_arg));
+ memset(¶m, 0, sizeof(param));
+
+ reg_shm_in = tee_shm_register(pvt_data.ctx, (unsigned long)p->key,
+ p->key_len, TEE_SHM_DMA_BUF |
+ TEE_SHM_KERNEL_MAPPED);
+ if (IS_ERR(reg_shm_in)) {
+ dev_err(pvt_data.dev, "key shm register failed\n");
+ return PTR_ERR(reg_shm_in);
+ }
+
+ reg_shm_out = tee_shm_register(pvt_data.ctx, (unsigned long)p->blob,
+ sizeof(p->blob), TEE_SHM_DMA_BUF |
+ TEE_SHM_KERNEL_MAPPED);
+ if (IS_ERR(reg_shm_out)) {
+ dev_err(pvt_data.dev, "blob shm register failed\n");
+ ret = PTR_ERR(reg_shm_out);
+ goto out;
+ }
+
+ inv_arg.func = TA_CMD_SEAL;
+ inv_arg.session = pvt_data.session_id;
+ inv_arg.num_params = 4;
+
+ param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
+ param[0].u.memref.shm = reg_shm_in;
+ param[0].u.memref.size = p->key_len;
+ param[0].u.memref.shm_offs = 0;
+ param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
+ param[1].u.memref.shm = reg_shm_out;
+ param[1].u.memref.size = sizeof(p->blob);
+ param[1].u.memref.shm_offs = 0;
+
+ ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
+ if ((ret < 0) || (inv_arg.ret != 0)) {
+ dev_err(pvt_data.dev, "TA_CMD_SEAL invoke err: %x\n",
+ inv_arg.ret);
+ ret = -EFAULT;
+ } else {
+ p->blob_len = param[1].u.memref.size;
+ }
+
+out:
+ if (reg_shm_out)
+ tee_shm_free(reg_shm_out);
+ if (reg_shm_in)
+ tee_shm_free(reg_shm_in);
+
+ return ret;
+}
+
+/*
+ * Have the TEE unseal(decrypt) the symmetric key
+ */
+static int tee_key_unseal(struct trusted_key_payload *p, char *datablob)
+{
+ int ret = 0;
+ struct tee_ioctl_invoke_arg inv_arg;
+ struct tee_param param[4];
+ struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
+
+ memset(&inv_arg, 0, sizeof(inv_arg));
+ memset(¶m, 0, sizeof(param));
+
+ reg_shm_in = tee_shm_register(pvt_data.ctx, (unsigned long)p->blob,
+ p->blob_len, TEE_SHM_DMA_BUF |
+ TEE_SHM_KERNEL_MAPPED);
+ if (IS_ERR(reg_shm_in)) {
+ dev_err(pvt_data.dev, "blob shm register failed\n");
+ return PTR_ERR(reg_shm_in);
+ }
+
+ reg_shm_out = tee_shm_register(pvt_data.ctx, (unsigned long)p->key,
+ sizeof(p->key), TEE_SHM_DMA_BUF |
+ TEE_SHM_KERNEL_MAPPED);
+ if (IS_ERR(reg_shm_out)) {
+ dev_err(pvt_data.dev, "key shm register failed\n");
+ ret = PTR_ERR(reg_shm_out);
+ goto out;
+ }
+
+ inv_arg.func = TA_CMD_UNSEAL;
+ inv_arg.session = pvt_data.session_id;
+ inv_arg.num_params = 4;
+
+ param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
+ param[0].u.memref.shm = reg_shm_in;
+ param[0].u.memref.size = p->blob_len;
+ param[0].u.memref.shm_offs = 0;
+ param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
+ param[1].u.memref.shm = reg_shm_out;
+ param[1].u.memref.size = sizeof(p->key);
+ param[1].u.memref.shm_offs = 0;
+
+ ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
+ if ((ret < 0) || (inv_arg.ret != 0)) {
+ dev_err(pvt_data.dev, "TA_CMD_UNSEAL invoke err: %x\n",
+ inv_arg.ret);
+ ret = -EFAULT;
+ } else {
+ p->key_len = param[1].u.memref.size;
+ }
+
+out:
+ if (reg_shm_out)
+ tee_shm_free(reg_shm_out);
+ if (reg_shm_in)
+ tee_shm_free(reg_shm_in);
+
+ return ret;
+}
+
+/*
+ * Have the TEE generate random symmetric key
+ */
+static int tee_get_random(unsigned char *key, size_t key_len)
+{
+ int ret = 0;
+ struct tee_ioctl_invoke_arg inv_arg;
+ struct tee_param param[4];
+ struct tee_shm *reg_shm = NULL;
+
+ memset(&inv_arg, 0, sizeof(inv_arg));
+ memset(¶m, 0, sizeof(param));
+
+ reg_shm = tee_shm_register(pvt_data.ctx, (unsigned long)key, key_len,
+ TEE_SHM_DMA_BUF | TEE_SHM_KERNEL_MAPPED);
+ if (IS_ERR(reg_shm)) {
+ dev_err(pvt_data.dev, "random key shm register failed\n");
+ return PTR_ERR(reg_shm);
+ }
+
+ inv_arg.func = TA_CMD_GET_RANDOM;
+ inv_arg.session = pvt_data.session_id;
+ inv_arg.num_params = 4;
+
+ param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
+ param[0].u.memref.shm = reg_shm;
+ param[0].u.memref.size = key_len;
+ param[0].u.memref.shm_offs = 0;
+
+ ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
+ if ((ret < 0) || (inv_arg.ret != 0)) {
+ dev_err(pvt_data.dev, "TA_CMD_GET_RANDOM invoke err: %x\n",
+ inv_arg.ret);
+ ret = -EFAULT;
+ } else {
+ ret = param[0].u.memref.size;
+ }
+
+ tee_shm_free(reg_shm);
+
+ return ret;
+}
+
+static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
+{
+ if (ver->impl_id == TEE_IMPL_ID_OPTEE)
+ return 1;
+ else
+ return 0;
+}
+
+static int trusted_key_probe(struct device *dev)
+{
+ struct tee_client_device *rng_device = to_tee_client_device(dev);
+ int ret = 0, err = -ENODEV;
+ struct tee_ioctl_open_session_arg sess_arg;
+
+ memset(&sess_arg, 0, sizeof(sess_arg));
+
+ /* Open context with TEE driver */
+ pvt_data.ctx = tee_client_open_context(NULL, optee_ctx_match, NULL,
+ NULL);
+ if (IS_ERR(pvt_data.ctx))
+ return -ENODEV;
+
+ /* Open session with hwrng Trusted App */
+ memcpy(sess_arg.uuid, rng_device->id.uuid.b, TEE_IOCTL_UUID_LEN);
+ sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
+ sess_arg.num_params = 0;
+
+ ret = tee_client_open_session(pvt_data.ctx, &sess_arg, NULL);
+ if ((ret < 0) || (sess_arg.ret != 0)) {
+ dev_err(dev, "tee_client_open_session failed, err: %x\n",
+ sess_arg.ret);
+ err = -EINVAL;
+ goto out_ctx;
+ }
+ pvt_data.session_id = sess_arg.session;
+
+ ret = register_key_type(&key_type_trusted);
+ if (ret < 0)
+ goto out_sess;
+
+ pvt_data.dev = dev;
+
+ return 0;
+
+out_sess:
+ tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
+out_ctx:
+ tee_client_close_context(pvt_data.ctx);
+
+ return err;
+}
+
+static int trusted_key_remove(struct device *dev)
+{
+ unregister_key_type(&key_type_trusted);
+ tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
+ tee_client_close_context(pvt_data.ctx);
+
+ return 0;
+}
+
+static const struct tee_client_device_id trusted_key_id_table[] = {
+ {UUID_INIT(0xf04a0fe7, 0x1f5d, 0x4b9b,
+ 0xab, 0xf7, 0x61, 0x9b, 0x85, 0xb4, 0xce, 0x8c)},
+ {}
+};
+
+MODULE_DEVICE_TABLE(tee, trusted_key_id_table);
+
+static struct tee_client_driver trusted_key_driver = {
+ .id_table = trusted_key_id_table,
+ .driver = {
+ .name = DRIVER_NAME,
+ .bus = &tee_bus_type,
+ .probe = trusted_key_probe,
+ .remove = trusted_key_remove,
+ },
+};
+
+static int __init init_tee_trusted(void)
+{
+ return driver_register(&trusted_key_driver.driver);
+}
+
+static void __exit cleanup_tee_trusted(void)
+{
+ driver_unregister(&trusted_key_driver.driver);
+}
+
+struct trusted_key_ops tee_trusted_key_ops = {
+ .migratable = 0, /* non-migratable */
+ .init = init_tee_trusted,
+ .seal = tee_key_seal,
+ .unseal = tee_key_unseal,
+ .get_random = tee_get_random,
+ .cleanup = cleanup_tee_trusted,
+};
+EXPORT_SYMBOL_GPL(tee_trusted_key_ops);
diff --git a/security/keys/trusted-keys/trusted.c b/security/keys/trusted-keys/trusted.c
index 8f00fde..a0a171f 100644
--- a/security/keys/trusted-keys/trusted.c
+++ b/security/keys/trusted-keys/trusted.c
@@ -27,6 +27,9 @@ static struct trusted_key_ops *available_tk_ops[] = {
#if defined(CONFIG_TCG_TPM)
&tpm_trusted_key_ops,
#endif
+#if defined(CONFIG_TEE)
+ &tee_trusted_key_ops,
+#endif
};
static struct trusted_key_ops *tk_ops;
--
2.7.4
^ permalink raw reply related
* [RFC v2 3/6] tee: add private login method for kernel clients
From: Sumit Garg @ 2019-07-30 12:23 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-security-module
Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
jmorris, serge, casey, ard.biesheuvel, daniel.thompson, linux-doc,
linux-kernel, linux-arm-kernel, tee-dev, Sumit Garg
In-Reply-To: <1564489420-677-1-git-send-email-sumit.garg@linaro.org>
There are use-cases where user-space shouldn't be allowed to communicate
directly with a TEE device which is dedicated to provide a specific
service for a kernel client. So add a private login method for kernel
clients and disallow user-space to open-session using GP implementation
defined login method range: (0x80000000 - 0xFFFFFFFF).
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
drivers/tee/tee_core.c | 6 ++++++
include/uapi/linux/tee.h | 8 ++++++++
2 files changed, 14 insertions(+)
diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 0f16d9f..2c2f646 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -334,6 +334,12 @@ static int tee_ioctl_open_session(struct tee_context *ctx,
goto out;
}
+ if (arg.clnt_login & TEE_IOCTL_LOGIN_MASK) {
+ pr_debug("login method not allowed for user-space client\n");
+ rc = -EPERM;
+ goto out;
+ }
+
rc = ctx->teedev->desc->ops->open_session(ctx, &arg, params);
if (rc)
goto out;
diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
index 4b9eb06..a0a3d52 100644
--- a/include/uapi/linux/tee.h
+++ b/include/uapi/linux/tee.h
@@ -172,6 +172,14 @@ struct tee_ioctl_buf_data {
#define TEE_IOCTL_LOGIN_APPLICATION 4
#define TEE_IOCTL_LOGIN_USER_APPLICATION 5
#define TEE_IOCTL_LOGIN_GROUP_APPLICATION 6
+/*
+ * Disallow user-space to use GP implementation specific login
+ * method range (0x80000000 - 0xFFFFFFFF). This range is rather
+ * being reserved for REE kernel clients or TEE implementation.
+ */
+#define TEE_IOCTL_LOGIN_MASK 0x80000000
+/* Private login method for REE kernel clients */
+#define TEE_IOCTL_LOGIN_REE_KERNEL 0x80000000
/**
* struct tee_ioctl_param - parameter
--
2.7.4
^ permalink raw reply related
* [RFC v2 2/6] tee: enable support to register kernel memory
From: Sumit Garg @ 2019-07-30 12:23 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-security-module
Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
jmorris, serge, casey, ard.biesheuvel, daniel.thompson, linux-doc,
linux-kernel, linux-arm-kernel, tee-dev, Sumit Garg
In-Reply-To: <1564489420-677-1-git-send-email-sumit.garg@linaro.org>
Enable support to register kernel memory reference with TEE. This change
will allow TEE bus drivers to register memory references.
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
---
drivers/tee/tee_shm.c | 16 ++++++++++++++--
include/linux/tee_drv.h | 1 +
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 2da026f..5c69b89 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -9,6 +9,7 @@
#include <linux/sched.h>
#include <linux/slab.h>
#include <linux/tee_drv.h>
+#include <linux/uio.h>
#include "tee_private.h"
static void tee_shm_release(struct tee_shm *shm)
@@ -224,13 +225,14 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
{
struct tee_device *teedev = ctx->teedev;
const u32 req_flags = TEE_SHM_DMA_BUF | TEE_SHM_USER_MAPPED;
+ const u32 req_ker_flags = TEE_SHM_DMA_BUF | TEE_SHM_KERNEL_MAPPED;
struct tee_shm *shm;
void *ret;
int rc;
int num_pages;
unsigned long start;
- if (flags != req_flags)
+ if (flags != req_flags && flags != req_ker_flags)
return ERR_PTR(-ENOTSUPP);
if (!tee_device_get(teedev))
@@ -264,7 +266,17 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
goto err;
}
- rc = get_user_pages_fast(start, num_pages, FOLL_WRITE, shm->pages);
+ if (flags & TEE_SHM_USER_MAPPED) {
+ rc = get_user_pages_fast(start, num_pages, FOLL_WRITE,
+ shm->pages);
+ } else {
+ const struct kvec kiov = {
+ .iov_base = (void *)start,
+ .iov_len = PAGE_SIZE
+ };
+
+ rc = get_kernel_pages(&kiov, num_pages, 0, shm->pages);
+ }
if (rc > 0)
shm->num_pages = rc;
if (rc != num_pages) {
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 7a03f68..dedf8fa 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -26,6 +26,7 @@
#define TEE_SHM_REGISTER BIT(3) /* Memory registered in secure world */
#define TEE_SHM_USER_MAPPED BIT(4) /* Memory mapped in user space */
#define TEE_SHM_POOL BIT(5) /* Memory allocated from pool */
+#define TEE_SHM_KERNEL_MAPPED BIT(6) /* Memory mapped in kernel space */
struct device;
struct tee_device;
--
2.7.4
^ permalink raw reply related
* [RFC v2 1/6] tee: optee: allow kernel pages to register as shm
From: Sumit Garg @ 2019-07-30 12:23 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-security-module
Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
jmorris, serge, casey, ard.biesheuvel, daniel.thompson, linux-doc,
linux-kernel, linux-arm-kernel, tee-dev, Sumit Garg
In-Reply-To: <1564489420-677-1-git-send-email-sumit.garg@linaro.org>
Kernel pages are marked as normal type memory only so allow kernel pages
to be registered as shared memory with OP-TEE.
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
---
drivers/tee/optee/call.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index aa94270..bce45b1 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -553,6 +553,13 @@ static int check_mem_type(unsigned long start, size_t num_pages)
struct mm_struct *mm = current->mm;
int rc;
+ /*
+ * Allow kernel address to register with OP-TEE as kernel
+ * pages are configured as normal memory only.
+ */
+ if (virt_addr_valid(start))
+ return 0;
+
down_read(&mm->mmap_sem);
rc = __check_mem_type(find_vma(mm, start),
start + num_pages * PAGE_SIZE);
--
2.7.4
^ permalink raw reply related
* [RFC v2 0/6] Introduce TEE based Trusted Keys support
From: Sumit Garg @ 2019-07-30 12:23 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-security-module
Cc: jens.wiklander, corbet, dhowells, jejb, jarkko.sakkinen, zohar,
jmorris, serge, casey, ard.biesheuvel, daniel.thompson, linux-doc,
linux-kernel, linux-arm-kernel, tee-dev, Sumit Garg
Add support for TEE based trusted keys where TEE provides the functionality
to seal and unseal trusted keys using hardware unique key. Also, this is
an alternative in case platform doesn't possess a TPM device.
This series also adds some TEE features like:
Patch #1, #2 enables support for registered kernel shared memory with TEE.
Patch #3 enables support for private kernel login method required for
cases like trusted keys where we don't wan't user-space to directly access
TEE service to retrieve trusted key contents.
Rest of the patches from #4 to #6 adds support for TEE based trusted keys.
This patch-set has been tested with OP-TEE based pseudo TA which can be
found here [1].
Also, this patch-set is dependent on generic Trusted Keys framework
patch-set [2].
[1] https://github.com/OP-TEE/optee_os/pull/3082
[2] https://lkml.org/lkml/2019/7/18/284
Changes in v2:
1. Add reviewed-by tags for patch #1 and #2.
2. Incorporate comments from Jens for patch #3.
3. Switch to use generic trusted keys framework.
Sumit Garg (6):
tee: optee: allow kernel pages to register as shm
tee: enable support to register kernel memory
tee: add private login method for kernel clients
KEYS: trusted: Introduce TEE based Trusted Keys
doc: keys: Document usage of TEE based Trusted Keys
MAINTAINERS: Add entry for TEE based Trusted Keys
Documentation/security/keys/index.rst | 1 +
Documentation/security/keys/tee-trusted.rst | 93 +++++++++
MAINTAINERS | 9 +
drivers/tee/optee/call.c | 7 +
drivers/tee/tee_core.c | 6 +
drivers/tee/tee_shm.c | 16 +-
include/keys/trusted-type.h | 3 +
include/keys/trusted_tee.h | 66 +++++++
include/linux/tee_drv.h | 1 +
include/uapi/linux/tee.h | 8 +
security/keys/Kconfig | 3 +
security/keys/trusted-keys/Makefile | 3 +-
security/keys/trusted-keys/trusted-tee.c | 282 ++++++++++++++++++++++++++++
security/keys/trusted-keys/trusted.c | 3 +
14 files changed, 498 insertions(+), 3 deletions(-)
create mode 100644 Documentation/security/keys/tee-trusted.rst
create mode 100644 include/keys/trusted_tee.h
create mode 100644 security/keys/trusted-keys/trusted-tee.c
--
2.7.4
^ permalink raw reply
* Re: [PATCH 1/2] KEYS: Replace uid/gid/perm permissions checking with an ACL
From: Eric Biggers @ 2019-07-30 3:49 UTC (permalink / raw)
To: David Howells, keyrings, linux-security-module, linux-fsdevel,
linux-kernel
In-Reply-To: <20190710011559.GA7973@sol.localdomain>
Hi David,
On Tue, Jul 09, 2019 at 06:16:01PM -0700, Eric Biggers wrote:
> On Thu, May 23, 2019 at 04:58:27PM +0100, David Howells wrote:
> > Replace the uid/gid/perm permissions checking on a key with an ACL to allow
> > the SETATTR and SEARCH permissions to be split. This will also allow a
> > greater range of subjects to represented.
> >
>
> This patch broke 'keyctl new_session', and hence broke all the fscrypt tests:
>
> $ keyctl new_session
> keyctl_session_to_parent: Permission denied
>
> Output of 'keyctl show' is
>
> $ keyctl show
> Session Keyring
> 605894913 --alswrv 0 0 keyring: _ses
> 189223103 ----s-rv 0 0 \_ user: invocation_id
>
> - Eric
This bug is still present in next-20190729.
- Eric
^ permalink raw reply
* Re: [PATCH V36 19/29] Lock down module params that specify hardware parameters (eg. ioport)
From: Matthew Garrett @ 2019-07-29 21:47 UTC (permalink / raw)
To: James Morris
Cc: LSM List, Linux Kernel Mailing List, Linux API, David Howells,
Alan Cox, Kees Cook, Jessica Yu
In-Reply-To: <20190718194415.108476-20-matthewgarrett@google.com>
On Thu, Jul 18, 2019 at 12:45 PM Matthew Garrett
<matthewgarrett@google.com> wrote:
>
> From: David Howells <dhowells@redhat.com>
>
> Provided an annotation for module parameters that specify hardware
> parameters (such as io ports, iomem addresses, irqs, dma channels, fixed
> dma buffers and other types).
>
> Suggested-by: Alan Cox <gnomes@lxorguk.ukuu.org.uk>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Cc: Jessica Yu <jeyu@kernel.org>
Jessica, any feedback on this?
^ 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