* Re: [PATCH v12 16/25] LSM: Use lsmcontext in security_dentry_init_security
From: Stephen Smalley @ 2019-12-18 16:16 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20191216223621.5127-17-casey@schaufler-ca.com>
On 12/16/19 5:36 PM, Casey Schaufler wrote:
> Change the security_dentry_init_security() interface to
> fill an lsmcontext structure instead of a void * data area
> and a length. The lone caller of this interface is NFS4,
> which may make copies of the data using its own mechanisms.
> A rework of the nfs4 code to use the lsmcontext properly
> is a significant project, so the coward's way out is taken,
> and the lsmcontext data from security_dentry_init_security()
> is copied, then released directly.
>
> This interface does not use the "display". There is currently
> not case where that is useful or reasonable.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> fs/nfs/nfs4proc.c | 26 ++++++++++++++++----------
> include/linux/security.h | 7 +++----
> security/security.c | 29 +++++++++++++++++++++++++----
> 3 files changed, 44 insertions(+), 18 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index a30e36654c57..78d63f7f0088 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -112,6 +112,7 @@ static inline struct nfs4_label *
> nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
> struct iattr *sattr, struct nfs4_label *label)
> {
> + struct lsmcontext context;
> int err;
>
> if (label == NULL)
> @@ -121,21 +122,26 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
> return NULL;
>
> err = security_dentry_init_security(dentry, sattr->ia_mode,
> - &dentry->d_name, (void **)&label->label, &label->len);
> - if (err == 0)
> - return label;
> + &dentry->d_name, &context);
> +
> + if (err)
> + return NULL;
> +
> + label->label = kmemdup(context.context, context.len, GFP_KERNEL);
This seems unfortunate; it introduces an extra allocation/copy of the
context. I'd prefer to avoid it. Also wondering if GFP_KERNEL is
always safe here.
> + if (label->label == NULL)
> + label = NULL;
> + else
> + label->len = context.len;
> +
> + security_release_secctx(&context);
> +
> + return label;
>
> - return NULL;
> }
> static inline void
> nfs4_label_release_security(struct nfs4_label *label)
> {
> - struct lsmcontext scaff; /* scaffolding */
> -
> - if (label) {
> - lsmcontext_init(&scaff, label->label, label->len, 0);
> - security_release_secctx(&scaff);
> - }
> + kfree(label->label);
> }
> static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label)
> {
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 00421941f683..a5eba06a9382 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -398,8 +398,8 @@ int security_add_mnt_opt(const char *option, const char *val,
> int len, void **mnt_opts);
> int security_move_mount(const struct path *from_path, const struct path *to_path);
> int security_dentry_init_security(struct dentry *dentry, int mode,
> - const struct qstr *name, void **ctx,
> - u32 *ctxlen);
> + const struct qstr *name,
> + struct lsmcontext *ctx);
> int security_dentry_create_files_as(struct dentry *dentry, int mode,
> struct qstr *name,
> const struct cred *old,
> @@ -790,8 +790,7 @@ static inline void security_inode_free(struct inode *inode)
> static inline int security_dentry_init_security(struct dentry *dentry,
> int mode,
> const struct qstr *name,
> - void **ctx,
> - u32 *ctxlen)
> + struct lsmcontext *ctx)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/security/security.c b/security/security.c
> index 4ba1a6ed36e0..8aa107b57af9 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1011,12 +1011,33 @@ void security_inode_free(struct inode *inode)
> inode_free_by_rcu);
> }
>
> +/*
> + * security_dentry_init_security - initial context for a dentry
> + * @dentry: directory entry
> + * @mode: access mode
> + * @name: path name
> + * @context: resulting security context
> + *
> + * Use at most one security module to get the initial
> + * security context. Do not use the "display".
> + *
> + * Returns -EOPNOTSUPP if not supplied by any module or the module result.
> + */
> int security_dentry_init_security(struct dentry *dentry, int mode,
> - const struct qstr *name, void **ctx,
> - u32 *ctxlen)
> + const struct qstr *name,
> + struct lsmcontext *cp)
> {
> - return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> - name, ctx, ctxlen);
> + struct security_hook_list *hp;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security,
> + list) {
> + cp->slot = hp->lsmid->slot;
> + return hp->hook.dentry_init_security(dentry, mode, name,
> + (void **)&cp->context,
> + &cp->len);
> + }
> +
> + return -EOPNOTSUPP;
> }
> EXPORT_SYMBOL(security_dentry_init_security);
>
>
^ permalink raw reply
* Re: [PATCH v12 13/25] LSM: Specify which LSM to display
From: Casey Schaufler @ 2019-12-18 16:32 UTC (permalink / raw)
To: Stephen Smalley, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul, Casey Schaufler
In-Reply-To: <cb93ecc4-97aa-770d-25c8-ec128f3bedc2@tycho.nsa.gov>
On 12/18/2019 7:17 AM, Stephen Smalley wrote:
> On 12/16/19 5:36 PM, Casey Schaufler wrote:
>> Create a new entry "display" in the procfs attr directory for
>> controlling which LSM security information is displayed for a
>> process. A process can only read or write its own display value.
>>
>> The name of an active LSM that supplies hooks for
>> human readable data may be written to "display" to set the
>> value. The name of the LSM currently in use can be read from
>> "display". At this point there can only be one LSM capable
>> of display active. A helper function lsm_task_display() is
>> provided to get the display slot for a task_struct.
>>
>> Setting the "display" requires that all security modules using
>> setprocattr hooks allow the action. Each security module is
>> responsible for defining its policy.
>>
>> AppArmor hook provided by John Johansen <john.johansen@canonical.com>
>> SELinux hook provided by Stephen Smalley <sds@tycho.nsa.gov>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>> fs/proc/base.c | 1 +
>> include/linux/lsm_hooks.h | 15 +++
>> security/apparmor/include/apparmor.h | 3 +-
>> security/apparmor/lsm.c | 32 +++++
>> security/security.c | 169 ++++++++++++++++++++++++---
>> security/selinux/hooks.c | 11 ++
>> security/selinux/include/classmap.h | 2 +-
>> security/smack/smack_lsm.c | 7 ++
>> 8 files changed, 221 insertions(+), 19 deletions(-)
>>
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index ebea9501afb8..950c200cb9ad 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -2652,6 +2652,7 @@ static const struct pid_entry attr_dir_stuff[] = {
>> ATTR(NULL, "fscreate", 0666),
>> ATTR(NULL, "keycreate", 0666),
>> ATTR(NULL, "sockcreate", 0666),
>> + ATTR(NULL, "display", 0666),
>> #ifdef CONFIG_SECURITY_SMACK
>> DIR("smack", 0555,
>> proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index 7eb808cde051..2bf82e1cf347 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -2186,4 +2186,19 @@ static inline void security_delete_hooks(struct security_hook_list *hooks,
>> extern int lsm_inode_alloc(struct inode *inode);
>> +/**
>> + * lsm_task_display - the "display" LSM for this task
>> + * @task: The task to report on
>> + *
>> + * Returns the task's display LSM slot.
>> + */
>> +static inline int lsm_task_display(struct task_struct *task)
>> +{
>> + int *display = task->security;
>> +
>> + if (display)
>> + return *display;
>> + return LSMBLOB_INVALID;
>> +}
>> +
>> #endif /* ! __LINUX_LSM_HOOKS_H */
>> diff --git a/security/apparmor/include/apparmor.h b/security/apparmor/include/apparmor.h
>> index 1fbabdb565a8..b1622fcb4394 100644
>> --- a/security/apparmor/include/apparmor.h
>> +++ b/security/apparmor/include/apparmor.h
>> @@ -28,8 +28,9 @@
>> #define AA_CLASS_SIGNAL 10
>> #define AA_CLASS_NET 14
>> #define AA_CLASS_LABEL 16
>> +#define AA_CLASS_DISPLAY_LSM 17
>> -#define AA_CLASS_LAST AA_CLASS_LABEL
>> +#define AA_CLASS_LAST AA_CLASS_DISPLAY_LSM
>> /* Control parameters settable through module/boot flags */
>> extern enum audit_mode aa_g_audit;
>> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
>> index 146d75e5e021..16b992235c11 100644
>> --- a/security/apparmor/lsm.c
>> +++ b/security/apparmor/lsm.c
>> @@ -612,6 +612,25 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
>> return error;
>> }
>> +
>> +static int profile_display_lsm(struct aa_profile *profile,
>> + struct common_audit_data *sa)
>> +{
>> + struct aa_perms perms = { };
>> + unsigned int state;
>> +
>> + state = PROFILE_MEDIATES(profile, AA_CLASS_DISPLAY_LSM);
>> + if (state) {
>> + aa_compute_perms(profile->policy.dfa, state, &perms);
>> + aa_apply_modes_to_perms(profile, &perms);
>> + aad(sa)->label = &profile->label;
>> +
>> + return aa_check_perms(profile, &perms, AA_MAY_WRITE, sa, NULL);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int apparmor_setprocattr(const char *name, void *value,
>> size_t size)
>> {
>> @@ -623,6 +642,19 @@ static int apparmor_setprocattr(const char *name, void *value,
>> if (size == 0)
>> return -EINVAL;
>> + /* LSM infrastructure does actual setting of display if allowed */
>> + if (!strcmp(name, "display")) {
>> + struct aa_profile *profile;
>> + struct aa_label *label;
>> +
>> + aad(&sa)->info = "set display lsm";
>> + label = begin_current_label_crit_section();
>> + error = fn_for_each_confined(label, profile,
>> + profile_display_lsm(profile, &sa));
>> + end_current_label_crit_section(label);
>> + return error;
>> + }
>> +
>> /* AppArmor requires that the buffer must be null terminated atm */
>> if (args[size - 1] != '\0') {
>> /* null terminate */
>> diff --git a/security/security.c b/security/security.c
>> index 32354942b7e8..aaac748e4d83 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -27,6 +27,7 @@
>> #include <linux/backing-dev.h>
>> #include <linux/string.h>
>> #include <linux/msg.h>
>> +#include <linux/binfmts.h>
>> #include <net/flow.h>
>> #include <net/sock.h>
>> @@ -43,7 +44,14 @@ static struct kmem_cache *lsm_file_cache;
>> static struct kmem_cache *lsm_inode_cache;
>> char *lsm_names;
>> -static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init;
>> +
>> +/*
>> + * The task blob includes the "display" slot used for
>> + * chosing which module presents contexts.
>> + */
>> +static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init = {
>> + .lbs_task = sizeof(int),
>> +};
>> /* Boot-time LSM user choice */
>> static __initdata const char *chosen_lsm_order;
>> @@ -438,8 +446,10 @@ static int lsm_append(const char *new, char **result)
>> /*
>> * Current index to use while initializing the lsmblob secid list.
>> + * Pointers to the LSM id structures for local use.
>> */
>> static int lsm_slot __lsm_ro_after_init;
>> +static struct lsm_id *lsm_slotlist[LSMBLOB_ENTRIES];
>> /**
>> * security_add_hooks - Add a modules hooks to the hook lists.
>> @@ -459,6 +469,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>> if (lsmid->slot == LSMBLOB_NEEDED) {
>> if (lsm_slot >= LSMBLOB_ENTRIES)
>> panic("%s Too many LSMs registered.\n", __func__);
>> + lsm_slotlist[lsm_slot] = lsmid;
>> lsmid->slot = lsm_slot++;
>> init_debug("%s assigned lsmblob slot %d\n", lsmid->lsm,
>> lsmid->slot);
>> @@ -588,6 +599,8 @@ int lsm_inode_alloc(struct inode *inode)
>> */
>> static int lsm_task_alloc(struct task_struct *task)
>> {
>> + int *display;
>> +
>> if (blob_sizes.lbs_task == 0) {
>> task->security = NULL;
>> return 0;
>> @@ -596,6 +609,15 @@ static int lsm_task_alloc(struct task_struct *task)
>> task->security = kzalloc(blob_sizes.lbs_task, GFP_KERNEL);
>> if (task->security == NULL)
>> return -ENOMEM;
>> +
>> + /*
>> + * The start of the task blob contains the "display" LSM slot number.
>> + * Start with it set to the invalid slot number, indicating that the
>> + * default first registered LSM be displayed.
>> + */
>> + display = task->security;
>> + *display = LSMBLOB_INVALID;
>> +
>> return 0;
>> }
>> @@ -1551,14 +1573,26 @@ int security_file_open(struct file *file)
>> int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
>> {
>> + int *odisplay = current->security;
>> + int *ndisplay;
>> int rc = lsm_task_alloc(task);
>> - if (rc)
>> + if (unlikely(rc))
>> return rc;
>> +
>> rc = call_int_hook(task_alloc, 0, task, clone_flags);
>> - if (unlikely(rc))
>> + if (unlikely(rc)) {
>> security_task_free(task);
>> - return rc;
>> + return rc;
>> + }
>> +
>> + if (odisplay) {
>> + ndisplay = task->security;
>> + if (ndisplay)
>> + *ndisplay = *odisplay;
>> + }
>> +
>> + return 0;
>> }
>> void security_task_free(struct task_struct *task)
>> @@ -1955,23 +1989,110 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>> char **value)
>> {
>> struct security_hook_list *hp;
>> + int display = lsm_task_display(current);
>> + int slot = 0;
>> +
>> + if (!strcmp(name, "display")) {
>> + /*
>> + * lsm_slot will be 0 if there are no displaying modules.
>> + */
>> + if (lsm_slot == 0)
>> + return -EINVAL;
>> +
>> + /*
>> + * Only allow getting the current process' display.
>> + * There are too few reasons to get another process'
>> + * display and too many LSM policy issues.
>> + */
>> + if (current != p)
>> + return -EINVAL;
>> +
>> + display = lsm_task_display(p);
>> + if (display != LSMBLOB_INVALID)
>> + slot = display;
>> + *value = kstrdup(lsm_slotlist[slot]->lsm, GFP_KERNEL);
>> + if (*value)
>> + return strlen(*value);
>> + return -ENOMEM;
>> + }
>> hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
>> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>> continue;
>> + if (lsm == NULL && display != LSMBLOB_INVALID &&
>> + display != hp->lsmid->slot)
>> + continue;
>> return hp->hook.getprocattr(p, name, value);
>> }
>> return -EINVAL;
>> }
>> +/**
>> + * security_setprocattr - Set process attributes via /proc
>> + * @lsm: name of module involved, or NULL
>> + * @name: name of the attribute
>> + * @value: value to set the attribute to
>> + * @size: size of the value
>> + *
>> + * Set the process attribute for the specified security module
>> + * to the specified value. Note that this can only be used to set
>> + * the process attributes for the current, or "self" process.
>> + * The /proc code has already done this check.
>> + *
>> + * Returns 0 on success, an appropriate code otherwise.
>> + */
>> int security_setprocattr(const char *lsm, const char *name, void *value,
>> size_t size)
>> {
>> struct security_hook_list *hp;
>> + char *term;
>> + char *cp;
>> + int *display = current->security;
>> + int rc = -EINVAL;
>> + int slot = 0;
>> +
>> + if (!strcmp(name, "display")) {
>> + /*
>> + * Change the "display" value only if all the security
>> + * modules that support setting a procattr allow it.
>> + * It is assumed that all such security modules will be
>> + * cooperative.
>> + */
>> + if (size == 0)
>> + return -EINVAL;
>> +
>> + hlist_for_each_entry(hp, &security_hook_heads.setprocattr,
>> + list) {
>> + rc = hp->hook.setprocattr(name, value, size);
>> + if (rc < 0)
>> + return rc;
>> + }
>> +
>> + rc = -EINVAL;
>> +
>> + term = kmemdup_nul(value, size, GFP_KERNEL);
>> + if (term == NULL)
>> + return -ENOMEM;
>> +
>> + cp = strsep(&term, " \n");
>> +
>> + for (slot = 0; slot < lsm_slot; slot++)
>> + if (!strcmp(cp, lsm_slotlist[slot]->lsm)) {
>> + *display = lsm_slotlist[slot]->slot;
>> + rc = size;
>> + break;
>> + }
>> +
>> + kfree(cp);
>
> This makes me slightly nervous; I see that it is correct currently but worry about cp being changed at some point to no longer refer to the start of the allocated buffer. I'd favor not mutating term (i.e. pass a temporary to strsep) and freeing it.
Sure. I'll make this more obviously safe.
>
>> + return rc;
>> + }
>> hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
>> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>> continue;
>> + if (lsm == NULL && *display != LSMBLOB_INVALID &&
>> + *display != hp->lsmid->slot)
>> + continue;
>> return hp->hook.setprocattr(name, value, size);
>> }
>> return -EINVAL;
>> @@ -1991,15 +2112,15 @@ EXPORT_SYMBOL(security_ismaclabel);
>> int security_secid_to_secctx(struct lsmblob *blob, char **secdata, u32 *seclen)
>> {
>> struct security_hook_list *hp;
>> - int rc;
>> + int display = lsm_task_display(current);
>> hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) {
>> if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>> continue;
>> - rc = hp->hook.secid_to_secctx(blob->secid[hp->lsmid->slot],
>> - secdata, seclen);
>> - if (rc != 0)
>> - return rc;
>> + if (display == LSMBLOB_INVALID || display == hp->lsmid->slot)
>> + return hp->hook.secid_to_secctx(
>> + blob->secid[hp->lsmid->slot],
>> + secdata, seclen);
>> }
>> return 0;
>> }
>> @@ -2009,16 +2130,15 @@ int security_secctx_to_secid(const char *secdata, u32 seclen,
>> struct lsmblob *blob)
>> {
>> struct security_hook_list *hp;
>> - int rc;
>> + int display = lsm_task_display(current);
>> lsmblob_init(blob, 0);
>> hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid, list) {
>> if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>> continue;
>> - rc = hp->hook.secctx_to_secid(secdata, seclen,
>> - &blob->secid[hp->lsmid->slot]);
>> - if (rc != 0)
>> - return rc;
>> + if (display == LSMBLOB_INVALID || display == hp->lsmid->slot)
>> + return hp->hook.secctx_to_secid(secdata, seclen,
>> + &blob->secid[hp->lsmid->slot]);
>> }
>> return 0;
>> }
>> @@ -2026,7 +2146,15 @@ EXPORT_SYMBOL(security_secctx_to_secid);
>> void security_release_secctx(char *secdata, u32 seclen)
>> {
>> - call_void_hook(release_secctx, secdata, seclen);
>> + struct security_hook_list *hp;
>> + int *display = current->security;
>> +
>> + hlist_for_each_entry(hp, &security_hook_heads.release_secctx, list)
>> + if (display == NULL || *display == LSMBLOB_INVALID ||
>> + *display == hp->lsmid->slot) {
>> + hp->hook.release_secctx(secdata, seclen);
>> + return;
>> + }
>> }
>
> I was wondering why you didn't use lsm_task_display() here and retain the same pattern as the other hooks?
Just missed it. Thanks for pointing it out. I'll fix it.
>
>> EXPORT_SYMBOL(security_release_secctx);
>> @@ -2151,8 +2279,15 @@ EXPORT_SYMBOL(security_sock_rcv_skb);
>> int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
>> int __user *optlen, unsigned len)
>> {
>> - return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock,
>> - optval, optlen, len);
>> + int display = lsm_task_display(current);
>> + struct security_hook_list *hp;
>> +
>> + hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream,
>> + list)
>> + if (display == LSMBLOB_INVALID || display == hp->lsmid->slot)
>> + return hp->hook.socket_getpeersec_stream(sock, optval,
>> + optlen, len);
>> + return -ENOPROTOOPT;
>> }
>> int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 97f2ee6e4080..b8501ca3c8f3 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -6323,6 +6323,17 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
>> /*
>> * Basic control over ability to set these attributes at all.
>> */
>> +
>> + /*
>> + * For setting display, we only perform a permission check;
>> + * the actual update to the display value is handled by the
>> + * LSM framework.
>> + */
>> + if (!strcmp(name, "display"))
>> + return avc_has_perm(&selinux_state,
>> + mysid, mysid, SECCLASS_PROCESS2,
>> + PROCESS2__SETDISPLAY, NULL);
>> +
>> if (!strcmp(name, "exec"))
>> error = avc_has_perm(&selinux_state,
>> mysid, mysid, SECCLASS_PROCESS,
>> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
>> index 7db24855e12d..323da8a38c43 100644
>> --- a/security/selinux/include/classmap.h
>> +++ b/security/selinux/include/classmap.h
>> @@ -52,7 +52,7 @@ struct security_class_mapping secclass_map[] = {
>> "execmem", "execstack", "execheap", "setkeycreate",
>> "setsockcreate", "getrlimit", NULL } },
>> { "process2",
>> - { "nnp_transition", "nosuid_transition", NULL } },
>> + { "nnp_transition", "nosuid_transition", "setdisplay", NULL } },
>> { "system",
>> { "ipc_info", "syslog_read", "syslog_mod",
>> "syslog_console", "module_request", "module_load", NULL } },
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index 82cbb3eeec76..9737ead06b39 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -3518,6 +3518,13 @@ static int smack_setprocattr(const char *name, void *value, size_t size)
>> struct smack_known_list_elem *sklep;
>> int rc;
>> + /*
>> + * Allow the /proc/.../attr/current and SO_PEERSEC "display"
>> + * to be reset at will.
>> + */
>> + if (strcmp(name, "display") == 0)
>> + return 0;
>> +
>> if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp->smk_relabel))
>> return -EPERM;
>>
>
^ permalink raw reply
* Re: [PATCH] integrity: Expose data structures required for include/linux/integrity.h
From: Florent Revest @ 2019-12-18 16:56 UTC (permalink / raw)
To: Mimi Zohar, Casey Schaufler, linux-integrity, Matthew Garrett
Cc: jmorris, serge, revest, allison, armijn, bauerman, linux-kernel,
linux-security-module, kpsingh
In-Reply-To: <1576679307.4579.401.camel@linux.ibm.com>
On Wed, 2019-12-18 at 09:28 -0500, Mimi Zohar wrote:
> [Cc'ing Matthew]
>
> On Wed, 2019-12-18 at 08:34 -0500, Mimi Zohar wrote:
> > On Wed, 2019-12-18 at 12:03 +0100, Florent Revest wrote:
> > > On Tue, 2019-12-17 at 18:08 -0500, Mimi Zohar wrote:
> > > > On Tue, 2019-12-17 at 08:25 -0800, Casey Schaufler wrote:
> > > > > On 12/17/2019 5:47 AM, Florent Revest wrote:
> > > > > > From: Florent Revest <revest@google.com>
> > > > > >
> > > > > > include/linux/integrity.h exposes the prototype of
> > > > > > integrity_inode_get().
> > > > > > However, it relies on struct integrity_iint_cache which is
> > > > > > currently
> > > > > > defined in an internal header,
> > > > > > security/integrity/integrity.h.
> > > > > >
> > > > > > To allow the rest of the kernel to use integrity_inode_get,
> > > > >
> > > > > Why do you want to do this?
> > > >
> > > > ditto
> > >
> > > My team works on KRSI (eBPF MAC policies presented at LSS by KP
> > > Singh).
> > > https://lkml.org/lkml/2019/9/10/393 We identified file hashes
> > > gathered
> > > from the integrity subsystem as an interesting field that we
> > > could
> > > potentially someday expose to eBPF programs through helpers.
> > >
> > > One of the reason behind writing KRSI is to replace a custom
> > > kernel
> > > auditing module that currently needs to redefine those structures
> > > to
> > > access them. I imagine other kernel modules could benefit from a
> > > file
> > > hash API too.
> > >
> > > This is the least intrusive patch I could come up with that
> > > allows us
> > > to lookup a hash from an inode. I was surprised to find that
> > > integrity_inode_get was exposed but not the structures it
> > > returns.
> > >
> > > If the community is interested in a different file hash API, I'd
> > > be
> > > happy to iterate on this patch based on your feedback.
> >
> > There's a major difference between returning just the file hash and
> > making the integrity_iint_cache structure public.
Certainly!
I am new to this subsystem so I just wanted to get the discussion
started. I am happy to make a more specific function.
> > Peter Moody's original code queried the cache[1]. Why do you need
> > access to the structure itself?
> > FYI, if/when we get to IMA namespacing, the cache structure will
> > change.
> >
> > [1] ima: add the ability to query ima for the hash of a given file.
>
> If you're using Peter's patch, or something similar, I'd appreciate
> your taking the time to upstream it.
Thank you for pointing me to Peter's patch! No one in my team was aware
of his work on this. Ugh!
It appears that Peter left the company while trying to upstream his
patch and the situation just got stuck there for 4+ years now.
If you are still positive about the idea of a ima_file_hash function, I
will take his v6 patch (this is the latest I could find on the
sourceforce archives of linux-ima-devel), rebase it, take your comments
into account and send a new version by the end of the week.
> Mimi
>
> > > > > > this patch
> > > > > > moves the definition of the necessary structures from a
> > > > > > private
> > > > > > header
> > > > > > to a global kernel header.
^ permalink raw reply
* Re: [PATCH v12 17/25] LSM: Use lsmcontext in security_inode_getsecctx
From: Stephen Smalley @ 2019-12-18 17:02 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20191216223621.5127-18-casey@schaufler-ca.com>
On 12/16/19 5:36 PM, Casey Schaufler wrote:
> Change the security_inode_getsecctx() interface to fill
> a lsmcontext structure instead of data and length pointers.
> This provides the information about which LSM created the
> context so that security_release_secctx() can use the
> correct hook.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> fs/nfsd/nfs4xdr.c | 23 +++++++++--------------
> include/linux/security.h | 5 +++--
> security/security.c | 13 +++++++++++--
> 3 files changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index e20011281915..98d20178e60f 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -2304,11 +2304,11 @@ nfsd4_encode_layout_types(struct xdr_stream *xdr, u32 layout_types)
> #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> static inline __be32
> nfsd4_encode_security_label(struct xdr_stream *xdr, struct svc_rqst *rqstp,
> - void *context, int len)
> + struct lsmcontext *context)
> {
> __be32 *p;
>
> - p = xdr_reserve_space(xdr, len + 4 + 4 + 4);
> + p = xdr_reserve_space(xdr, context->len + 4 + 4 + 4);
> if (!p)
> return nfserr_resource;
>
> @@ -2318,13 +2318,13 @@ nfsd4_encode_security_label(struct xdr_stream *xdr, struct svc_rqst *rqstp,
> */
> *p++ = cpu_to_be32(0); /* lfs */
> *p++ = cpu_to_be32(0); /* pi */
> - p = xdr_encode_opaque(p, context, len);
> + p = xdr_encode_opaque(p, context->context, context->len);
> return 0;
> }
> #else
> static inline __be32
> nfsd4_encode_security_label(struct xdr_stream *xdr, struct svc_rqst *rqstp,
> - void *context, int len)
> + struct lsmcontext *context)
> { return 0; }
> #endif
>
> @@ -2421,9 +2421,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
> int err;
> struct nfs4_acl *acl = NULL;
> #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> - struct lsmcontext scaff; /* scaffolding */
> - void *context = NULL;
> - int contextlen;
> + struct lsmcontext context = { };
> #endif
> bool contextsupport = false;
> struct nfsd4_compoundres *resp = rqstp->rq_resp;
> @@ -2481,7 +2479,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
> bmval0 & FATTR4_WORD0_SUPPORTED_ATTRS) {
> if (exp->ex_flags & NFSEXP_SECURITY_LABEL)
> err = security_inode_getsecctx(d_inode(dentry),
> - &context, &contextlen);
> + &context);
> else
> err = -EOPNOTSUPP;
> contextsupport = (err == 0);
> @@ -2911,8 +2909,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
>
> #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> if (bmval2 & FATTR4_WORD2_SECURITY_LABEL) {
> - status = nfsd4_encode_security_label(xdr, rqstp, context,
> - contextlen);
> + status = nfsd4_encode_security_label(xdr, rqstp, &context);
> if (status)
> goto out;
> }
> @@ -2924,10 +2921,8 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
>
> out:
> #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> - if (context) {
> - lsmcontext_init(&scaff, context, contextlen, 0); /*scaffolding*/
> - security_release_secctx(&scaff);
> - }
> + if (context.context)
> + security_release_secctx(&context);
> #endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
> kfree(acl);
> if (tempfh) {
> diff --git a/include/linux/security.h b/include/linux/security.h
> index a5eba06a9382..d0fab9f5dddf 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -537,7 +537,7 @@ void security_release_secctx(struct lsmcontext *cp);
> 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);
> -int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
> +int security_inode_getsecctx(struct inode *inode, struct lsmcontext *cp);
> int security_locked_down(enum lockdown_reason what);
> #else /* CONFIG_SECURITY */
>
> @@ -1362,7 +1362,8 @@ static inline int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32
> {
> return -EOPNOTSUPP;
> }
> -static inline int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
> +static inline int security_inode_getsecctx(struct inode *inode,
> + struct lsmcontext *cp)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/security/security.c b/security/security.c
> index 8aa107b57af9..963641acf9dc 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2207,9 +2207,18 @@ int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
> }
> EXPORT_SYMBOL(security_inode_setsecctx);
>
> -int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
> +int security_inode_getsecctx(struct inode *inode, struct lsmcontext *cp)
> {
> - return call_int_hook(inode_getsecctx, -EOPNOTSUPP, inode, ctx, ctxlen);
> + struct security_hook_list *hp;
> +
> + memset(cp, 0, sizeof(*cp));
> +
> + hlist_for_each_entry(hp, &security_hook_heads.inode_getsecctx, list) {
> + cp->slot = hp->lsmid->slot;
> + return hp->hook.inode_getsecctx(inode, (void **)&cp->context,
> + &cp->len);
> + }
> + return -EOPNOTSUPP;
> }
> EXPORT_SYMBOL(security_inode_getsecctx);
>
>
^ permalink raw reply
* Re: [PATCH v12 18/25] LSM: security_secid_to_secctx in netlink netfilter
From: Stephen Smalley @ 2019-12-18 17:10 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20191216223621.5127-19-casey@schaufler-ca.com>
On 12/16/19 5:36 PM, Casey Schaufler wrote:
> Change netlink netfilter interfaces to use lsmcontext
> pointers, and remove scaffolding.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: netdev@vger.kernel.org
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> net/netfilter/nfnetlink_queue.c | 32 +++++++++++++-------------------
> 1 file changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index 2d6668fd026c..a1296453d8f2 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -301,12 +301,10 @@ static int nfqnl_put_sk_uidgid(struct sk_buff *skb, struct sock *sk)
> return -1;
> }
>
> -static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
> +static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, struct lsmcontext *context)
> {
> - u32 seclen = 0;
> #if IS_ENABLED(CONFIG_NETWORK_SECMARK)
> struct lsmblob blob;
> - struct lsmcontext context = { };
>
> if (!skb || !sk_fullsock(skb->sk))
> return 0;
> @@ -314,15 +312,16 @@ static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
> read_lock_bh(&skb->sk->sk_callback_lock);
>
> if (skb->secmark) {
> + /* Any LSM might be looking for the secmark */
> lsmblob_init(&blob, skb->secmark);
> - security_secid_to_secctx(&blob, &context);
> - *secdata = context.context;
> + security_secid_to_secctx(&blob, context);
> }
>
> read_unlock_bh(&skb->sk->sk_callback_lock);
> - seclen = context.len;
> + return context->len;
> +#else
> + return 0;
> #endif
> - return seclen;
> }
>
> static u32 nfqnl_get_bridge_size(struct nf_queue_entry *entry)
> @@ -398,8 +397,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> enum ip_conntrack_info uninitialized_var(ctinfo);
> struct nfnl_ct_hook *nfnl_ct;
> bool csum_verify;
> - struct lsmcontext scaff; /* scaffolding */
> - char *secdata = NULL;
> + struct lsmcontext context = { };
> u32 seclen = 0;
>
> size = nlmsg_total_size(sizeof(struct nfgenmsg))
> @@ -466,7 +464,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> }
>
> if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
> - seclen = nfqnl_get_sk_secctx(entskb, &secdata);
> + seclen = nfqnl_get_sk_secctx(entskb, &context);
> if (seclen)
> size += nla_total_size(seclen);
> }
> @@ -601,7 +599,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> nfqnl_put_sk_uidgid(skb, entskb->sk) < 0)
> goto nla_put_failure;
>
> - if (seclen && nla_put(skb, NFQA_SECCTX, seclen, secdata))
> + if (seclen && nla_put(skb, NFQA_SECCTX, context.len, context.context))
> goto nla_put_failure;
>
> if (ct && nfnl_ct->build(skb, ct, ctinfo, NFQA_CT, NFQA_CT_INFO) < 0)
> @@ -629,10 +627,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> }
>
> nlh->nlmsg_len = skb->len;
> - if (seclen) {
> - lsmcontext_init(&scaff, secdata, seclen, 0);
> - security_release_secctx(&scaff);
> - }
> + if (seclen)
> + security_release_secctx(&context);
> return skb;
>
> nla_put_failure:
> @@ -640,10 +636,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> kfree_skb(skb);
> net_err_ratelimited("nf_queue: error creating packet message\n");
> nlmsg_failure:
> - if (seclen) {
> - lsmcontext_init(&scaff, secdata, seclen, 0);
> - security_release_secctx(&scaff);
> - }
> + if (seclen)
> + security_release_secctx(&context);
> return NULL;
> }
>
>
^ permalink raw reply
* Re: [PATCH v12 19/25] NET: Store LSM netlabel data in a lsmblob
From: Stephen Smalley @ 2019-12-18 17:41 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20191216223621.5127-20-casey@schaufler-ca.com>
On 12/16/19 5:36 PM, Casey Schaufler wrote:
> Netlabel uses LSM interfaces requiring an lsmblob and
> the internal storage is used to pass information between
> these interfaces, so change the internal data from a secid
> to a lsmblob. Update the netlabel interfaces and their
> callers to accommodate the change. This requires that the
> modules using netlabel use the lsm_id.slot to access the
> correct secid when using netlabel.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/net/netlabel.h | 8 ++--
> net/ipv4/cipso_ipv4.c | 6 ++-
> net/netlabel/netlabel_kapi.c | 6 +--
> net/netlabel/netlabel_unlabeled.c | 57 +++++++++++------------------
> net/netlabel/netlabel_unlabeled.h | 2 +-
> security/selinux/hooks.c | 2 +-
> security/selinux/include/security.h | 1 +
> security/selinux/netlabel.c | 2 +-
> security/selinux/ss/services.c | 4 +-
> security/smack/smack.h | 1 +
> security/smack/smack_lsm.c | 5 ++-
> security/smack/smackfs.c | 10 +++--
> 12 files changed, 50 insertions(+), 54 deletions(-)
>
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 376882215919..8ee7a804423e 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1467,7 +1467,8 @@ static int cipso_v4_gentag_loc(const struct cipso_v4_doi *doi_def,
>
> buffer[0] = CIPSO_V4_TAG_LOCAL;
> buffer[1] = CIPSO_V4_TAG_LOC_BLEN;
> - *(u32 *)&buffer[2] = secattr->attr.secid;
> + /* only one netlabel user - the first */
> + *(u32 *)&buffer[2] = secattr->attr.lsmblob.secid[0];
>
> return CIPSO_V4_TAG_LOC_BLEN;
> }
> @@ -1487,7 +1488,8 @@ static int cipso_v4_parsetag_loc(const struct cipso_v4_doi *doi_def,
> const unsigned char *tag,
> struct netlbl_lsm_secattr *secattr)
> {
> - secattr->attr.secid = *(u32 *)&tag[2];
> + /* only one netlabel user - the first */
> + secattr->attr.lsmblob.secid[0] = *(u32 *)&tag[2];
> secattr->flags |= NETLBL_SECATTR_SECID;
>
> return 0;
Here we always use secid[0].
> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> index 6a94b31b5472..d8d7603ab14e 100644
> --- a/security/selinux/netlabel.c
> +++ b/security/selinux/netlabel.c
> @@ -108,7 +108,7 @@ static struct netlbl_lsm_secattr *selinux_netlbl_sock_getattr(
> return NULL;
>
> if ((secattr->flags & NETLBL_SECATTR_SECID) &&
> - (secattr->attr.secid == sid))
> + (secattr->attr.lsmblob.secid[selinux_lsmid.slot] == sid))
> return secattr;
>
> return NULL;
And here we use secid[selinux_lsmid.slot]. So things will break in
interesting ways if selinux_lsmid.slot is anything other than zero?
What's the point of using this indirection in the security modules
until/unless NetLabel truly supports something other than slot 0?
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index a5813c7629c1..2b7680903b6b 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -3599,7 +3599,7 @@ int security_netlbl_secattr_to_sid(struct selinux_state *state,
> if (secattr->flags & NETLBL_SECATTR_CACHE)
> *sid = *(u32 *)secattr->cache->data;
> else if (secattr->flags & NETLBL_SECATTR_SECID)
> - *sid = secattr->attr.secid;
> + *sid = secattr->attr.lsmblob.secid[selinux_lsmid.slot];
> else if (secattr->flags & NETLBL_SECATTR_MLS_LVL) {
> rc = -EIDRM;
> ctx = sidtab_search(sidtab, SECINITSID_NETMSG);
> @@ -3672,7 +3672,7 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
> if (secattr->domain == NULL)
> goto out;
>
> - secattr->attr.secid = sid;
> + secattr->attr.lsmblob.secid[selinux_lsmid.slot] = sid;
> secattr->flags |= NETLBL_SECATTR_DOMAIN_CPY | NETLBL_SECATTR_SECID;
> mls_export_netlbl_lvl(policydb, ctx, secattr);
> rc = mls_export_netlbl_cat(policydb, ctx, secattr);
Ditto
^ permalink raw reply
* Re: [PATCH v12 20/25] LSM: Verify LSM display sanity in binder
From: Stephen Smalley @ 2019-12-18 17:43 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20191216223621.5127-21-casey@schaufler-ca.com>
On 12/16/19 5:36 PM, Casey Schaufler wrote:
> Verify that the tasks on the ends of a binder transaction
> use the same "display" security module. This prevents confusion
> of security "contexts".
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> security/security.c | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/security/security.c b/security/security.c
> index 963641acf9dc..bca092dd4f00 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -738,9 +738,38 @@ int security_binder_set_context_mgr(struct task_struct *mgr)
> return call_int_hook(binder_set_context_mgr, 0, mgr);
> }
>
> +/**
> + * security_binder_transaction - Binder driver transaction check
> + * @from: source of the transaction
> + * @to: destination of the transaction
> + *
> + * Verify that the tasks have the same LSM "display", then
> + * call the security module hooks.
> + *
> + * Returns -EINVAL if the displays don't match, or the
> + * result of the security module checks.
> + */
> int security_binder_transaction(struct task_struct *from,
> struct task_struct *to)
> {
> + int from_display = lsm_task_display(from);
> + int to_display = lsm_task_display(to);
> +
> + /*
> + * If the display is LSMBLOB_INVALID the first module that has
> + * an entry is used. This will be in the 0 slot.
> + *
> + * This is currently only required if the server has requested
> + * peer contexts, but it would be unwieldly to have too much of
> + * the binder driver detail here.
> + */
> + if (from_display == LSMBLOB_INVALID)
> + from_display = 0;
> + if (to_display == LSMBLOB_INVALID)
> + to_display = 0;
> + if (from_display != to_display)
> + return -EINVAL;
> +
> return call_int_hook(binder_transaction, 0, from, to);
> }
>
>
^ permalink raw reply
* Re: [PATCH v12 21/25] Audit: Add subj_LSM fields when necessary
From: Stephen Smalley @ 2019-12-18 17:55 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20191216223621.5127-22-casey@schaufler-ca.com>
On 12/16/19 5:36 PM, Casey Schaufler wrote:
> Add record entries to identify subject data for all of the
> security modules when there is more than one.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: netdev@vger.kernel.com
> cc: linux-audit@redhat.com
Disclaimer: should be reviewed by netdev & audit maintainers at least.
Also, I have not verified that the use of LSMBLOB_DISPLAY is always the
right thing to do for the netfilter cases or whether some ought to use
LSMBLOB_FIRST instead. Otherwise,
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> drivers/android/binder.c | 2 +-
> include/linux/audit.h | 1 +
> include/linux/security.h | 9 ++++-
> include/net/scm.h | 3 +-
> kernel/audit.c | 40 ++++++++++++++++++-
> kernel/audit_fsnotify.c | 1 +
> kernel/auditfilter.c | 1 +
> kernel/auditsc.c | 10 +++--
> net/ipv4/ip_sockglue.c | 2 +-
> net/netfilter/nf_conntrack_netlink.c | 4 +-
> net/netfilter/nf_conntrack_standalone.c | 2 +-
> net/netfilter/nfnetlink_queue.c | 2 +-
> net/netlabel/netlabel_unlabeled.c | 11 ++++--
> net/netlabel/netlabel_user.c | 2 +-
> net/xfrm/xfrm_policy.c | 2 +
> net/xfrm/xfrm_state.c | 2 +
> security/integrity/ima/ima_api.c | 1 +
> security/integrity/integrity_audit.c | 1 +
> security/security.c | 51 +++++++++++++++++++++++--
> 19 files changed, 124 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 3c5eee35aae6..c9324c094888 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3107,7 +3107,7 @@ static void binder_transaction(struct binder_proc *proc,
> size_t added_size;
>
> security_task_getsecid(proc->tsk, &blob);
> - ret = security_secid_to_secctx(&blob, &lsmctx);
> + ret = security_secid_to_secctx(&blob, &lsmctx, LSMBLOB_DISPLAY);
> if (ret) {
> return_error = BR_FAILED_REPLY;
> return_error_param = ret;
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index f9ceae57ca8d..2ce0e8da3922 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -161,6 +161,7 @@ extern void audit_log_path_denied(int type,
> extern void audit_log_lost(const char *message);
>
> extern int audit_log_task_context(struct audit_buffer *ab);
> +extern void audit_log_task_lsms(struct audit_buffer *ab);
> extern void audit_log_task_info(struct audit_buffer *ab);
>
> extern int audit_update_lsm_rules(void);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index d0fab9f5dddf..536db4dbfcbb 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -180,6 +180,8 @@ struct lsmblob {
> #define LSMBLOB_INVALID -1 /* Not a valid LSM slot number */
> #define LSMBLOB_NEEDED -2 /* Slot requested on initialization */
> #define LSMBLOB_NOT_NEEDED -3 /* Slot not requested */
> +#define LSMBLOB_DISPLAY -4 /* Use the "display" slot */
> +#define LSMBLOB_FIRST -5 /* Use the default "display" slot */
>
> /**
> * lsmblob_init - initialize an lsmblob structure.
> @@ -221,6 +223,8 @@ static inline bool lsmblob_equal(struct lsmblob *bloba, struct lsmblob *blobb)
> return !memcmp(bloba, blobb, sizeof(*bloba));
> }
>
> +const char *security_lsm_slot_name(int slot);
> +
> /* These functions are in security/commoncap.c */
> extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
> int cap, unsigned int opts);
> @@ -530,7 +534,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> size_t size);
> int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> int security_ismaclabel(const char *name);
> -int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp);
> +int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp,
> + int display);
> int security_secctx_to_secid(const char *secdata, u32 seclen,
> struct lsmblob *blob);
> void security_release_secctx(struct lsmcontext *cp);
> @@ -1334,7 +1339,7 @@ static inline int security_ismaclabel(const char *name)
> }
>
> static inline int security_secid_to_secctx(struct lsmblob *blob,
> - struct lsmcontext *cp)
> + struct lsmcontext *cp, int display)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/include/net/scm.h b/include/net/scm.h
> index 4a6ad8caf423..8b5a4737e1b8 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -96,7 +96,8 @@ static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
> int err;
>
> if (test_bit(SOCK_PASSSEC, &sock->flags)) {
> - err = security_secid_to_secctx(&scm->lsmblob, &context);
> + err = security_secid_to_secctx(&scm->lsmblob, &context,
> + LSMBLOB_DISPLAY);
>
> if (!err) {
> put_cmsg(msg, SOL_SOCKET, SCM_SECURITY,
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 224c7b4a1bc0..d40f64a47c4b 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -392,6 +392,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old,
> if (rc)
> allow_changes = 0; /* Something weird, deny request */
> audit_log_format(ab, " res=%d", allow_changes);
> + audit_log_task_lsms(ab);
> audit_log_end(ab);
> return rc;
> }
> @@ -1097,6 +1098,7 @@ static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature
> audit_log_format(ab, " feature=%s old=%u new=%u old_lock=%u new_lock=%u res=%d",
> audit_feature_names[which], !!old_feature, !!new_feature,
> !!old_lock, !!new_lock, res);
> + audit_log_task_lsms(ab);
> audit_log_end(ab);
> }
>
> @@ -1347,6 +1349,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> size--;
> audit_log_n_untrustedstring(ab, data, size);
> }
> + audit_log_task_lsms(ab);
> audit_log_end(ab);
> }
> break;
> @@ -1361,6 +1364,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> msg_type == AUDIT_ADD_RULE ?
> "add_rule" : "remove_rule",
> audit_enabled);
> + audit_log_task_lsms(ab);
> audit_log_end(ab);
> return -EPERM;
> }
> @@ -1374,6 +1378,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> audit_log_common_recv_msg(audit_context(), &ab,
> AUDIT_CONFIG_CHANGE);
> audit_log_format(ab, " op=trim res=1");
> + audit_log_task_lsms(ab);
> audit_log_end(ab);
> break;
> case AUDIT_MAKE_EQUIV: {
> @@ -1409,6 +1414,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> audit_log_format(ab, " new=");
> audit_log_untrustedstring(ab, new);
> audit_log_format(ab, " res=%d", !err);
> + audit_log_task_lsms(ab);
> audit_log_end(ab);
> kfree(old);
> kfree(new);
> @@ -1418,7 +1424,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> len = 0;
> if (lsmblob_is_set(&audit_sig_lsm)) {
> err = security_secid_to_secctx(&audit_sig_lsm,
> - &context);
> + &context, LSMBLOB_FIRST);
> if (err)
> return err;
> }
> @@ -1477,6 +1483,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> " old-log_passwd=%d new-log_passwd=%d res=%d",
> old.enabled, s.enabled, old.log_passwd,
> s.log_passwd, !err);
> + audit_log_task_lsms(ab);
> audit_log_end(ab);
> break;
> }
> @@ -2055,6 +2062,33 @@ void audit_log_key(struct audit_buffer *ab, char *key)
> audit_log_format(ab, "(null)");
> }
>
> +void audit_log_task_lsms(struct audit_buffer *ab)
> +{
> + int i;
> + const char *lsm;
> + struct lsmblob blob;
> + struct lsmcontext context;
> +
> + /*
> + * Don't do anything unless there is more than one LSM
> + * with a security context to report.
> + */
> + if (security_lsm_slot_name(1) == NULL)
> + return;
> +
> + security_task_getsecid(current, &blob);
> +
> + for (i = 0; i < LSMBLOB_ENTRIES; i++) {
> + lsm = security_lsm_slot_name(i);
> + if (lsm == NULL)
> + break;
> + if (security_secid_to_secctx(&blob, &context, i))
> + continue;
> + audit_log_format(ab, " subj_%s=%s", lsm, context.context);
> + security_release_secctx(&context);
> + }
> +}
> +
> int audit_log_task_context(struct audit_buffer *ab)
> {
> int error;
> @@ -2065,7 +2099,7 @@ int audit_log_task_context(struct audit_buffer *ab)
> if (!lsmblob_is_set(&blob))
> return 0;
>
> - error = security_secid_to_secctx(&blob, &context);
> + error = security_secid_to_secctx(&blob, &context, LSMBLOB_FIRST);
> if (error) {
> if (error != -EINVAL)
> goto error_path;
> @@ -2173,6 +2207,7 @@ void audit_log_path_denied(int type, const char *operation)
> audit_log_format(ab, "op=%s", operation);
> audit_log_task_info(ab);
> audit_log_format(ab, " res=0");
> + audit_log_task_lsms(ab);
> audit_log_end(ab);
> }
>
> @@ -2223,6 +2258,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid,
> oldloginuid, loginuid, tty ? tty_name(tty) : "(none)",
> oldsessionid, sessionid, !rc);
> audit_put_tty(tty);
> + audit_log_task_lsms(ab);
> audit_log_end(ab);
> }
>
> diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> index f0d243318452..7f8c4b1a2884 100644
> --- a/kernel/audit_fsnotify.c
> +++ b/kernel/audit_fsnotify.c
> @@ -126,6 +126,7 @@ static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, c
> audit_log_untrustedstring(ab, audit_mark->path);
> audit_log_key(ab, rule->filterkey);
> audit_log_format(ab, " list=%d res=1", rule->listnr);
> + audit_log_task_lsms(ab);
> audit_log_end(ab);
> }
>
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 19cfbe716f9d..bf28bb599b6d 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1103,6 +1103,7 @@ static void audit_log_rule_change(char *action, struct audit_krule *rule, int re
> audit_log_format(ab, " op=%s", action);
> audit_log_key(ab, rule->filterkey);
> audit_log_format(ab, " list=%d res=%d", rule->listnr, res);
> + audit_log_task_lsms(ab);
> audit_log_end(ab);
> }
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 6d273183dd87..e0dd643e9b13 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -973,7 +973,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> from_kuid(&init_user_ns, auid),
> from_kuid(&init_user_ns, uid), sessionid);
> if (lsmblob_is_set(blob)) {
> - if (security_secid_to_secctx(blob, &lsmctx)) {
> + if (security_secid_to_secctx(blob, &lsmctx, LSMBLOB_FIRST)) {
> audit_log_format(ab, " obj=(none)");
> rc = 1;
> } else {
> @@ -1218,7 +1218,8 @@ static void show_special(struct audit_context *context, int *call_panic)
> struct lsmblob blob;
>
> lsmblob_init(&blob, osid);
> - if (security_secid_to_secctx(&blob, &lsmcxt)) {
> + if (security_secid_to_secctx(&blob, &lsmcxt,
> + LSMBLOB_FIRST)) {
> audit_log_format(ab, " osid=%u", osid);
> *call_panic = 1;
> } else {
> @@ -1370,7 +1371,7 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
> struct lsmcontext lsmctx;
>
> lsmblob_init(&blob, n->osid);
> - if (security_secid_to_secctx(&blob, &lsmctx)) {
> + if (security_secid_to_secctx(&blob, &lsmctx, LSMBLOB_FIRST)) {
> audit_log_format(ab, " osid=%u", n->osid);
> if (call_panic)
> *call_panic = 2;
> @@ -1479,6 +1480,7 @@ static void audit_log_exit(void)
>
> audit_log_task_info(ab);
> audit_log_key(ab, context->filterkey);
> + audit_log_task_lsms(ab);
> audit_log_end(ab);
>
> for (aux = context->aux; aux; aux = aux->next) {
> @@ -2602,6 +2604,7 @@ void audit_core_dumps(long signr)
> return;
> audit_log_task(ab);
> audit_log_format(ab, " sig=%ld res=1", signr);
> + audit_log_task_lsms(ab);
> audit_log_end(ab);
> }
>
> @@ -2628,6 +2631,7 @@ void audit_seccomp(unsigned long syscall, long signr, int code)
> audit_log_format(ab, " sig=%ld arch=%x syscall=%ld compat=%d ip=0x%lx code=0x%x",
> signr, syscall_get_arch(current), syscall,
> in_compat_syscall(), KSTK_EIP(current), code);
> + audit_log_task_lsms(ab);
> audit_log_end(ab);
> }
>
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index 27af7a6b8780..10b418029cdd 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -138,7 +138,7 @@ static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
> if (err)
> return;
>
> - err = security_secid_to_secctx(&lb, &context);
> + err = security_secid_to_secctx(&lb, &context, LSMBLOB_DISPLAY);
> if (err)
> return;
>
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 255bcb886a2f..b2f522b39a1a 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -334,7 +334,7 @@ static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
> struct lsmcontext context;
>
> lsmblob_init(&blob, ct->secmark);
> - ret = security_secid_to_secctx(&blob, &context);
> + ret = security_secid_to_secctx(&blob, &context, LSMBLOB_DISPLAY);
> if (ret)
> return 0;
>
> @@ -649,7 +649,7 @@ static inline int ctnetlink_secctx_size(const struct nf_conn *ct)
> struct lsmcontext context;
>
> lsmblob_init(&blob, ct->secmark);
> - ret = security_secid_to_secctx(&blob, &context);
> + ret = security_secid_to_secctx(&blob, &context, LSMBLOB_DISPLAY);
> if (ret)
> return 0;
>
> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> index 8969754d7fe9..0ff2b8300c28 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -177,7 +177,7 @@ static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
> struct lsmcontext context;
>
> lsmblob_init(&blob, ct->secmark);
> - ret = security_secid_to_secctx(&blob, &context);
> + ret = security_secid_to_secctx(&blob, &context, LSMBLOB_DISPLAY);
> if (ret)
> return;
>
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index a1296453d8f2..b6f71be884e8 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -314,7 +314,7 @@ static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, struct lsmcontext *context)
> if (skb->secmark) {
> /* Any LSM might be looking for the secmark */
> lsmblob_init(&blob, skb->secmark);
> - security_secid_to_secctx(&blob, context);
> + security_secid_to_secctx(&blob, context, LSMBLOB_DISPLAY);
> }
>
> read_unlock_bh(&skb->sk->sk_callback_lock);
> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index 3b0f07b59436..60a7665de0e3 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -436,7 +436,8 @@ int netlbl_unlhsh_add(struct net *net,
> unlhsh_add_return:
> rcu_read_unlock();
> if (audit_buf != NULL) {
> - if (security_secid_to_secctx(lsmblob, &context) == 0) {
> + if (security_secid_to_secctx(lsmblob, &context,
> + LSMBLOB_FIRST) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s",
> context.context);
> security_release_secctx(&context);
> @@ -491,7 +492,8 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> if (dev != NULL)
> dev_put(dev);
> if (entry != NULL &&
> - security_secid_to_secctx(&entry->lsmblob, &context) == 0) {
> + security_secid_to_secctx(&entry->lsmblob, &context,
> + LSMBLOB_FIRST) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s",
> context.context);
> security_release_secctx(&context);
> @@ -551,7 +553,8 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> if (dev != NULL)
> dev_put(dev);
> if (entry != NULL &&
> - security_secid_to_secctx(&entry->lsmblob, &context) == 0) {
> + security_secid_to_secctx(&entry->lsmblob, &context,
> + LSMBLOB_FIRST) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s",
> context.context);
> security_release_secctx(&context);
> @@ -1122,7 +1125,7 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> lsmb = (struct lsmblob *)&addr6->lsmblob;
> }
>
> - ret_val = security_secid_to_secctx(lsmb, &context);
> + ret_val = security_secid_to_secctx(lsmb, &context, LSMBLOB_FIRST);
> if (ret_val != 0)
> goto list_cb_failure;
> ret_val = nla_put(cb_arg->skb,
> diff --git a/net/netlabel/netlabel_user.c b/net/netlabel/netlabel_user.c
> index 951ba0639d20..1941877fd16f 100644
> --- a/net/netlabel/netlabel_user.c
> +++ b/net/netlabel/netlabel_user.c
> @@ -100,7 +100,7 @@ struct audit_buffer *netlbl_audit_start_common(int type,
>
> lsmblob_init(&blob, audit_info->secid);
> if (audit_info->secid != 0 &&
> - security_secid_to_secctx(&blob, &context) == 0) {
> + security_secid_to_secctx(&blob, &context, LSMBLOB_FIRST) == 0) {
> audit_log_format(audit_buf, " subj=%s", context.context);
> security_release_secctx(&context);
> }
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index f2d1e573ea55..bd2b36a83e66 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -4206,6 +4206,7 @@ void xfrm_audit_policy_add(struct xfrm_policy *xp, int result, bool task_valid)
> xfrm_audit_helper_usrinfo(task_valid, audit_buf);
> audit_log_format(audit_buf, " res=%u", result);
> xfrm_audit_common_policyinfo(xp, audit_buf);
> + audit_log_task_lsms(audit_buf);
> audit_log_end(audit_buf);
> }
> EXPORT_SYMBOL_GPL(xfrm_audit_policy_add);
> @@ -4221,6 +4222,7 @@ void xfrm_audit_policy_delete(struct xfrm_policy *xp, int result,
> xfrm_audit_helper_usrinfo(task_valid, audit_buf);
> audit_log_format(audit_buf, " res=%u", result);
> xfrm_audit_common_policyinfo(xp, audit_buf);
> + audit_log_task_lsms(audit_buf);
> audit_log_end(audit_buf);
> }
> EXPORT_SYMBOL_GPL(xfrm_audit_policy_delete);
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index f3423562d933..bfb8402cb28d 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -2642,6 +2642,7 @@ void xfrm_audit_state_add(struct xfrm_state *x, int result, bool task_valid)
> xfrm_audit_helper_usrinfo(task_valid, audit_buf);
> xfrm_audit_helper_sainfo(x, audit_buf);
> audit_log_format(audit_buf, " res=%u", result);
> + audit_log_task_lsms(audit_buf);
> audit_log_end(audit_buf);
> }
> EXPORT_SYMBOL_GPL(xfrm_audit_state_add);
> @@ -2656,6 +2657,7 @@ void xfrm_audit_state_delete(struct xfrm_state *x, int result, bool task_valid)
> xfrm_audit_helper_usrinfo(task_valid, audit_buf);
> xfrm_audit_helper_sainfo(x, audit_buf);
> audit_log_format(audit_buf, " res=%u", result);
> + audit_log_task_lsms(audit_buf);
> audit_log_end(audit_buf);
> }
> EXPORT_SYMBOL_GPL(xfrm_audit_state_delete);
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index 1ab769fa7df6..252dc00700e8 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -363,6 +363,7 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
> audit_log_format(ab, " hash=\"%s:%s\"", algo_name, hash);
>
> audit_log_task_info(ab);
> + audit_log_task_lsms(ab);
> audit_log_end(ab);
>
> iint->flags |= IMA_AUDITED;
> diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
> index 5109173839cc..bca89ae72e3d 100644
> --- a/security/integrity/integrity_audit.c
> +++ b/security/integrity/integrity_audit.c
> @@ -54,5 +54,6 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
> audit_log_format(ab, " ino=%lu", inode->i_ino);
> }
> audit_log_format(ab, " res=%d", !result);
> + audit_log_task_lsms(ab);
> audit_log_end(ab);
> }
> diff --git a/security/security.c b/security/security.c
> index bca092dd4f00..d0b57a7c3b31 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -449,7 +449,31 @@ static int lsm_append(const char *new, char **result)
> * Pointers to the LSM id structures for local use.
> */
> static int lsm_slot __lsm_ro_after_init;
> -static struct lsm_id *lsm_slotlist[LSMBLOB_ENTRIES];
> +static struct lsm_id *lsm_slotlist[LSMBLOB_ENTRIES] __lsm_ro_after_init;
> +
> +/**
> + * security_lsm_slot_name - Get the name of the security module in a slot
> + * @slot: index into the "display" slot list.
> + *
> + * Provide the name of the security module associated with
> + * a display slot.
> + *
> + * If @slot is LSMBLOB_INVALID return the value
> + * for slot 0 if it has been set, otherwise NULL.
> + *
> + * Returns a pointer to the name string or NULL.
> + */
> +const char *security_lsm_slot_name(int slot)
> +{
> + if (slot == LSMBLOB_INVALID)
> + slot = 0;
> + else if (slot >= LSMBLOB_ENTRIES || slot < 0)
> + return NULL;
> +
> + if (lsm_slotlist[slot] == NULL)
> + return NULL;
> + return lsm_slotlist[slot]->lsm;
> +}
>
> /**
> * security_add_hooks - Add a modules hooks to the hook lists.
> @@ -2159,13 +2183,32 @@ int security_ismaclabel(const char *name)
> }
> EXPORT_SYMBOL(security_ismaclabel);
>
> -int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp)
> +int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp,
> + int display)
> {
> struct security_hook_list *hp;
> - int display = lsm_task_display(current);
>
> memset(cp, 0, sizeof(*cp));
>
> + /*
> + * display either is the slot number use for formatting
> + * or an instruction on which relative slot to use.
> + */
> + if (display == LSMBLOB_DISPLAY)
> + display = lsm_task_display(current);
> + else if (display == LSMBLOB_FIRST)
> + display = LSMBLOB_INVALID;
> + else if (display < 0) {
> + WARN_ONCE(true,
> + "LSM: security_secid_to_secctx unknown display\n");
> + display = LSMBLOB_INVALID;
> + } else if (display >= lsm_slot) {
> + WARN_ONCE(true,
> + "LSM: security_secid_to_secctx invalid display\n");
> + display = LSMBLOB_INVALID;
> + }
> +
> +
> hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) {
> if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> continue;
> @@ -2176,7 +2219,7 @@ int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp)
> &cp->context, &cp->len);
> }
> }
> - return 0;
> + return -EOPNOTSUPP;
> }
> EXPORT_SYMBOL(security_secid_to_secctx);
>
>
^ permalink raw reply
* Re: [PATCH v12 22/25] Audit: Include object data for all security modules
From: Stephen Smalley @ 2019-12-18 18:02 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20191216223621.5127-23-casey@schaufler-ca.com>
On 12/16/19 5:36 PM, Casey Schaufler wrote:
> When there is more than one context displaying security
> module extend what goes into the audit record by supplimenting
> the "obj=" with an "obj_<lsm>=" for each such security
> module.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Disclaimer: requires ack by audit maintainers. For me,
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> kernel/audit.h | 4 +-
> kernel/auditsc.c | 110 ++++++++++++++++++++++++-----------------------
> 2 files changed, 58 insertions(+), 56 deletions(-)
>
> diff --git a/kernel/audit.h b/kernel/audit.h
> index af9bc09e656c..c9f1e1641542 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -78,7 +78,7 @@ struct audit_names {
> kuid_t uid;
> kgid_t gid;
> dev_t rdev;
> - u32 osid;
> + struct lsmblob oblob;
> struct audit_cap_data fcap;
> unsigned int fcap_ver;
> unsigned char type; /* record type */
> @@ -152,7 +152,7 @@ struct audit_context {
> kuid_t uid;
> kgid_t gid;
> umode_t mode;
> - u32 osid;
> + struct lsmblob oblob;
> int has_perm;
> uid_t perm_uid;
> gid_t perm_gid;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index e0dd643e9b13..0c071947c2b3 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -659,17 +659,15 @@ static int audit_filter_rules(struct task_struct *tsk,
> if (f->lsm_rule) {
> /* Find files that match */
> if (name) {
> - lsmblob_init(&blob, name->osid);
> result = security_audit_rule_match(
> - &blob,
> + &name->oblob,
> f->type,
> f->op,
> f->lsm_rule);
> } else if (ctx) {
> list_for_each_entry(n, &ctx->names_list, list) {
> - lsmblob_init(&blob, n->osid);
> if (security_audit_rule_match(
> - &blob,
> + &n->oblob,
> f->type,
> f->op,
> f->lsm_rule)) {
> @@ -681,8 +679,7 @@ static int audit_filter_rules(struct task_struct *tsk,
> /* Find ipc objects that match */
> if (!ctx || ctx->type != AUDIT_IPC)
> break;
> - lsmblob_init(&blob, ctx->ipc.osid);
> - if (security_audit_rule_match(&blob,
> + if (security_audit_rule_match(&ctx->ipc.oblob,
> f->type, f->op,
> f->lsm_rule))
> ++result;
> @@ -956,13 +953,57 @@ static inline void audit_free_context(struct audit_context *context)
> kfree(context);
> }
>
> +static int audit_log_object_context(struct audit_buffer *ab,
> + struct lsmblob *blob)
> +{
> + struct lsmcontext context;
> + const char *lsm;
> + int i;
> +
> + /*
> + * None of the installed modules have object labels.
> + */
> + if (security_lsm_slot_name(0) == NULL)
> + return 0;
> +
> + if (blob->secid[0] != 0) {
> + if (security_secid_to_secctx(blob, &context, 0)) {
> + audit_log_format(ab, " obj=?");
> + return 1;
> + }
> + audit_log_format(ab, " obj=%s", context.context);
> + security_release_secctx(&context);
> + }
> +
> + /*
> + * Don't do anything more unless there is more than one LSM
> + * with a security context to report.
> + */
> + if (security_lsm_slot_name(1) == NULL)
> + return 0;
> +
> + for (i = 0; i < LSMBLOB_ENTRIES; i++) {
> + lsm = security_lsm_slot_name(i);
> + if (lsm == NULL)
> + break;
> + if (blob->secid[i] == 0)
> + continue;
> + if (security_secid_to_secctx(blob, &context, i)) {
> + audit_log_format(ab, " obj_%s=?", lsm);
> + continue;
> + }
> + audit_log_format(ab, " obj_%s=%s", lsm, context.context);
> + security_release_secctx(&context);
> + }
> + return 0;
> +}
> +
> static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> kuid_t auid, kuid_t uid,
> unsigned int sessionid,
> struct lsmblob *blob, char *comm)
> {
> struct audit_buffer *ab;
> - struct lsmcontext lsmctx;
> int rc = 0;
>
> ab = audit_log_start(context, GFP_KERNEL, AUDIT_OBJ_PID);
> @@ -972,15 +1013,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> audit_log_format(ab, "opid=%d oauid=%d ouid=%d oses=%d", pid,
> from_kuid(&init_user_ns, auid),
> from_kuid(&init_user_ns, uid), sessionid);
> - if (lsmblob_is_set(blob)) {
> - if (security_secid_to_secctx(blob, &lsmctx, LSMBLOB_FIRST)) {
> - audit_log_format(ab, " obj=(none)");
> - rc = 1;
> - } else {
> - audit_log_format(ab, " obj=%s", lsmctx.context);
> - security_release_secctx(&lsmctx);
> - }
> - }
> + rc = audit_log_object_context(ab, blob);
> audit_log_format(ab, " ocomm=");
> audit_log_untrustedstring(ab, comm);
> audit_log_end(ab);
> @@ -1207,26 +1240,14 @@ static void show_special(struct audit_context *context, int *call_panic)
> context->socketcall.args[i]);
> break; }
> case AUDIT_IPC: {
> - u32 osid = context->ipc.osid;
> + struct lsmblob *oblob = & context->ipc.oblob;
>
> audit_log_format(ab, "ouid=%u ogid=%u mode=%#ho",
> from_kuid(&init_user_ns, context->ipc.uid),
> from_kgid(&init_user_ns, context->ipc.gid),
> context->ipc.mode);
> - if (osid) {
> - struct lsmcontext lsmcxt;
> - struct lsmblob blob;
> -
> - lsmblob_init(&blob, osid);
> - if (security_secid_to_secctx(&blob, &lsmcxt,
> - LSMBLOB_FIRST)) {
> - audit_log_format(ab, " osid=%u", osid);
> - *call_panic = 1;
> - } else {
> - audit_log_format(ab, " obj=%s", lsmcxt.context);
> - security_release_secctx(&lsmcxt);
> - }
> - }
> + if (audit_log_object_context(ab, oblob))
> + *call_panic = 1;
> if (context->ipc.has_perm) {
> audit_log_end(ab);
> ab = audit_log_start(context, GFP_KERNEL,
> @@ -1366,20 +1387,8 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
> from_kgid(&init_user_ns, n->gid),
> MAJOR(n->rdev),
> MINOR(n->rdev));
> - if (n->osid != 0) {
> - struct lsmblob blob;
> - struct lsmcontext lsmctx;
> -
> - lsmblob_init(&blob, n->osid);
> - if (security_secid_to_secctx(&blob, &lsmctx, LSMBLOB_FIRST)) {
> - audit_log_format(ab, " osid=%u", n->osid);
> - if (call_panic)
> - *call_panic = 2;
> - } else {
> - audit_log_format(ab, " obj=%s", lsmctx.context);
> - security_release_secctx(&lsmctx);
> - }
> - }
> + if (audit_log_object_context(ab, &n->oblob) && call_panic)
> + *call_panic = 2;
>
> /* log the audit_names record type */
> switch (n->type) {
> @@ -1929,17 +1938,13 @@ static void audit_copy_inode(struct audit_names *name,
> const struct dentry *dentry,
> struct inode *inode, unsigned int flags)
> {
> - struct lsmblob blob;
> -
> name->ino = inode->i_ino;
> name->dev = inode->i_sb->s_dev;
> name->mode = inode->i_mode;
> name->uid = inode->i_uid;
> name->gid = inode->i_gid;
> name->rdev = inode->i_rdev;
> - security_inode_getsecid(inode, &blob);
> - /* scaffolding until osid is updated */
> - name->osid = blob.secid[0];
> + security_inode_getsecid(inode, &name->oblob);
> if (flags & AUDIT_INODE_NOEVAL) {
> name->fcap_ver = -1;
> return;
> @@ -2285,14 +2290,11 @@ void __audit_mq_getsetattr(mqd_t mqdes, struct mq_attr *mqstat)
> void __audit_ipc_obj(struct kern_ipc_perm *ipcp)
> {
> struct audit_context *context = audit_context();
> - struct lsmblob blob;
> context->ipc.uid = ipcp->uid;
> context->ipc.gid = ipcp->gid;
> context->ipc.mode = ipcp->mode;
> context->ipc.has_perm = 0;
> - security_ipc_getsecid(ipcp, &blob);
> - /* scaffolding on the [0] - change "osid" to a lsmblob */
> - context->ipc.osid = blob.secid[0];
> + security_ipc_getsecid(ipcp, &context->ipc.oblob);
> context->type = AUDIT_IPC;
> }
>
>
^ permalink raw reply
* Re: [PATCH v12 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs
From: Stephen Smalley @ 2019-12-18 18:28 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20191216223621.5127-24-casey@schaufler-ca.com>
On 12/16/19 5:36 PM, Casey Schaufler wrote:
> The getsockopt SO_PEERSEC provides the LSM based security
> information for a single module, but for reasons of backward
> compatibility cannot include the information for multiple
> modules. A new option SO_PEERCONTEXT is added to report the
> security "context" of multiple modules using a "compound" format
>
> lsm1\0value\0lsm2\0value\0
>
> This is expected to be used by system services, including dbus-daemon.
> The exact format of a compound context has been the subject of
> considerable debate. This format was suggested by Simon McVittie,
> a dbus maintainer with a significant stake in the format being
> usable.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: netdev@vger.kernel.org
Requires ack by netdev and linux-api. A couple of comments below.
> ---
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 2bf82e1cf347..2ae10e7f81a7 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -880,8 +880,8 @@
> * SO_GETPEERSEC. For tcp sockets this can be meaningful if the
> * socket is associated with an ipsec SA.
> * @sock is the local socket.
> - * @optval userspace memory where the security state is to be copied.
> - * @optlen userspace int where the module should copy the actual length
> + * @optval memory where the security state is to be copied.
This is misleading; it suggests that the caller is providing an
allocated buffer into which the security module copies its data. Instead
it is just a pointer to a pointer that is then set by the security
module to a buffer the module allocates.
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 536db4dbfcbb..b72bb90b1903 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -181,7 +181,7 @@ struct lsmblob {
> #define LSMBLOB_NEEDED -2 /* Slot requested on initialization */
> #define LSMBLOB_NOT_NEEDED -3 /* Slot not requested */
> #define LSMBLOB_DISPLAY -4 /* Use the "display" slot */
> -#define LSMBLOB_FIRST -5 /* Use the default "display" slot */
> +#define LSMBLOB_COMPOUND -5 /* A compound "display" */
I'm puzzled by the removal of LSMBLOB_FIRST by this patch; it suggests
it was never needed in the first place by the patch that introduced it.
But more below.
> diff --git a/security/security.c b/security/security.c
> index d0b57a7c3b31..1afe245f3246 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -723,6 +723,42 @@ static void __init lsm_early_task(struct task_struct *task)
> panic("%s: Early task alloc failed.\n", __func__);
> }
>
> +/**
> + * append_ctx - append a lsm/context pair to a compound context
> + * @ctx: the existing compound context
> + * @ctxlen: size of the old context, including terminating nul byte
> + * @lsm: new lsm name, nul terminated
> + * @new: new context, possibly nul terminated
> + * @newlen: maximum size of @new
> + *
> + * replace @ctx with a new compound context, appending @newlsm and @new
> + * to @ctx. On exit the new data replaces the old, which is freed.
> + * @ctxlen is set to the new size, which includes a trailing nul byte.
> + *
> + * Returns 0 on success, -ENOMEM if no memory is available.
> + */
> +static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char *new,
> + int newlen)
> +{
> + char *final;
> + int llen;
> +
> + llen = strlen(lsm) + 1;
> + newlen = strnlen(new, newlen) + 1;
> +
> + final = kzalloc(*ctxlen + llen + newlen, GFP_KERNEL);
> + if (final == NULL)
> + return -ENOMEM;
> + if (*ctxlen)
> + memcpy(final, *ctx, *ctxlen);
> + memcpy(final + *ctxlen, lsm, llen);
> + memcpy(final + *ctxlen + llen, new, newlen);
> + kfree(*ctx);
> + *ctx = final;
> + *ctxlen = *ctxlen + llen + newlen;
> + return 0;
> +}
You should likely take some precautions against integer overflows in the
above code?
> +
> /*
> * Hook list operation macros.
> *
> @@ -2164,8 +2200,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
> continue;
> - if (lsm == NULL && *display != LSMBLOB_INVALID &&
> - *display != hp->lsmid->slot)
> + if (lsm == NULL && display != NULL &&
> + *display != LSMBLOB_INVALID && *display != hp->lsmid->slot)
> continue;
> return hp->hook.setprocattr(name, value, size);
> }
Is this a bug fix that should be folded into the earlier patch that
introduced it?
> @@ -2196,7 +2232,7 @@ int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp,
> */
> if (display == LSMBLOB_DISPLAY)
> display = lsm_task_display(current);
> - else if (display == LSMBLOB_FIRST)
> + else if (display == 0)
> display = LSMBLOB_INVALID;
> else if (display < 0) {
> WARN_ONCE(true,
Why is it necessary to re-map display 0 in this manner? Previously if
display 0 was specified, it would require it to match the lsmid->slot
value. Won't it match anyway?
> @@ -2246,6 +2282,15 @@ void security_release_secctx(struct lsmcontext *cp)
> struct security_hook_list *hp;
> bool found = false;
>
> + if (cp->slot == LSMBLOB_INVALID)
> + return;
> +
> + if (cp->slot == LSMBLOB_COMPOUND) {
> + kfree(cp->context);
> + found = true;
> + goto clear_out;
> + }
> +
If you re-order your pr_warn() below with your memset() to address the
earlier comment, you'll end up trying to print the freed memory. Not a
problem if you just drop the pr_warn() altogether.
^ permalink raw reply
* Re: [PATCH] integrity: Expose data structures required for include/linux/integrity.h
From: Mimi Zohar @ 2019-12-18 18:43 UTC (permalink / raw)
To: Florent Revest, Casey Schaufler, linux-integrity, Matthew Garrett
Cc: jmorris, serge, revest, allison, armijn, bauerman, linux-kernel,
linux-security-module, kpsingh
In-Reply-To: <63f057fb98351324c8fc6210c42f3cbd76e85a68.camel@chromium.org>
On Wed, 2019-12-18 at 17:56 +0100, Florent Revest wrote:
> On Wed, 2019-12-18 at 09:28 -0500, Mimi Zohar wrote:
> > [Cc'ing Matthew]
> >
> > > There's a major difference between returning just the file hash and
> > > making the integrity_iint_cache structure public.
>
> Certainly!
> I am new to this subsystem so I just wanted to get the discussion
> started. I am happy to make a more specific function.
>
> > > Peter Moody's original code queried the cache[1]. Why do you need
> > > access to the structure itself?
> > > FYI, if/when we get to IMA namespacing, the cache structure will
> > > change.
> > >
> > > [1] ima: add the ability to query ima for the hash of a given file.
> >
> > If you're using Peter's patch, or something similar, I'd appreciate
> > your taking the time to upstream it.
>
> Thank you for pointing me to Peter's patch! No one in my team was aware
> of his work on this. Ugh!
> It appears that Peter left the company while trying to upstream his
> patch and the situation just got stuck there for 4+ years now.
>
> If you are still positive about the idea of a ima_file_hash function, I
> will take his v6 patch (this is the latest I could find on the
> sourceforce archives of linux-ima-devel), rebase it, take your comments
> into account and send a new version by the end of the week.
Matthew also wasn't aware of Peter's patch, until I sent it to him. I
assume they're using it or something similar. Please coordinate with
him, before refreshing and posting the patch.
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v12 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs
From: Stephen Smalley @ 2019-12-18 19:12 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <56b96440-a304-42b6-1515-1ad2659b2581@tycho.nsa.gov>
On 12/18/19 1:28 PM, Stephen Smalley wrote:
> On 12/16/19 5:36 PM, Casey Schaufler wrote:
>> The getsockopt SO_PEERSEC provides the LSM based security
>> information for a single module, but for reasons of backward
>> compatibility cannot include the information for multiple
>> modules. A new option SO_PEERCONTEXT is added to report the
>> security "context" of multiple modules using a "compound" format
>>
>> lsm1\0value\0lsm2\0value\0
>>
>> This is expected to be used by system services, including dbus-daemon.
>> The exact format of a compound context has been the subject of
>> considerable debate. This format was suggested by Simon McVittie,
>> a dbus maintainer with a significant stake in the format being
>> usable.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> cc: netdev@vger.kernel.org
>
> Requires ack by netdev and linux-api. A couple of comments below.
Also, have you tested this new interface? I may be doing something
wrong, but a trivial attempt to use SO_PEERCONTEXT with both SELinux and
AppArmor enabled only appeared to return the SELinux portion of the
label
(selinux\0unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023\0),
whereas /proc/self/attr/context returned a compound context (the same
but with apparmor\0unconfined\n\0 appended).
>
>> ---
>
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index 2bf82e1cf347..2ae10e7f81a7 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -880,8 +880,8 @@
>> * SO_GETPEERSEC. For tcp sockets this can be meaningful if the
>> * socket is associated with an ipsec SA.
>> * @sock is the local socket.
>> - * @optval userspace memory where the security state is to be copied.
>> - * @optlen userspace int where the module should copy the actual
>> length
>> + * @optval memory where the security state is to be copied.
>
> This is misleading; it suggests that the caller is providing an
> allocated buffer into which the security module copies its data. Instead
> it is just a pointer to a pointer that is then set by the security
> module to a buffer the module allocates.
>
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 536db4dbfcbb..b72bb90b1903 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -181,7 +181,7 @@ struct lsmblob {
>> #define LSMBLOB_NEEDED -2 /* Slot requested on
>> initialization */
>> #define LSMBLOB_NOT_NEEDED -3 /* Slot not requested */
>> #define LSMBLOB_DISPLAY -4 /* Use the "display" slot */
>> -#define LSMBLOB_FIRST -5 /* Use the default "display" slot */
>> +#define LSMBLOB_COMPOUND -5 /* A compound "display" */
>
> I'm puzzled by the removal of LSMBLOB_FIRST by this patch; it suggests
> it was never needed in the first place by the patch that introduced it.
> But more below.
>
>> diff --git a/security/security.c b/security/security.c
>> index d0b57a7c3b31..1afe245f3246 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -723,6 +723,42 @@ static void __init lsm_early_task(struct
>> task_struct *task)
>> panic("%s: Early task alloc failed.\n", __func__);
>> }
>> +/**
>> + * append_ctx - append a lsm/context pair to a compound context
>> + * @ctx: the existing compound context
>> + * @ctxlen: size of the old context, including terminating nul byte
>> + * @lsm: new lsm name, nul terminated
>> + * @new: new context, possibly nul terminated
>> + * @newlen: maximum size of @new
>> + *
>> + * replace @ctx with a new compound context, appending @newlsm and @new
>> + * to @ctx. On exit the new data replaces the old, which is freed.
>> + * @ctxlen is set to the new size, which includes a trailing nul byte.
>> + *
>> + * Returns 0 on success, -ENOMEM if no memory is available.
>> + */
>> +static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char
>> *new,
>> + int newlen)
>> +{
>> + char *final;
>> + int llen;
>> +
>> + llen = strlen(lsm) + 1;
>> + newlen = strnlen(new, newlen) + 1;
>> +
>> + final = kzalloc(*ctxlen + llen + newlen, GFP_KERNEL);
>> + if (final == NULL)
>> + return -ENOMEM;
>> + if (*ctxlen)
>> + memcpy(final, *ctx, *ctxlen);
>> + memcpy(final + *ctxlen, lsm, llen);
>> + memcpy(final + *ctxlen + llen, new, newlen);
>> + kfree(*ctx);
>> + *ctx = final;
>> + *ctxlen = *ctxlen + llen + newlen;
>> + return 0;
>> +}
>
> You should likely take some precautions against integer overflows in the
> above code?
>
>> +
>> /*
>> * Hook list operation macros.
>> *
>> @@ -2164,8 +2200,8 @@ int security_setprocattr(const char *lsm, const
>> char *name, void *value,
>> hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
>> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>> continue;
>> - if (lsm == NULL && *display != LSMBLOB_INVALID &&
>> - *display != hp->lsmid->slot)
>> + if (lsm == NULL && display != NULL &&
>> + *display != LSMBLOB_INVALID && *display != hp->lsmid->slot)
>> continue;
>> return hp->hook.setprocattr(name, value, size);
>> }
>
> Is this a bug fix that should be folded into the earlier patch that
> introduced it?
>
>> @@ -2196,7 +2232,7 @@ int security_secid_to_secctx(struct lsmblob
>> *blob, struct lsmcontext *cp,
>> */
>> if (display == LSMBLOB_DISPLAY)
>> display = lsm_task_display(current);
>> - else if (display == LSMBLOB_FIRST)
>> + else if (display == 0)
>> display = LSMBLOB_INVALID;
>> else if (display < 0) {
>> WARN_ONCE(true,
>
> Why is it necessary to re-map display 0 in this manner? Previously if
> display 0 was specified, it would require it to match the lsmid->slot
> value. Won't it match anyway?
>
>> @@ -2246,6 +2282,15 @@ void security_release_secctx(struct lsmcontext
>> *cp)
>> struct security_hook_list *hp;
>> bool found = false;
>> + if (cp->slot == LSMBLOB_INVALID)
>> + return;
>> +
>> + if (cp->slot == LSMBLOB_COMPOUND) {
>> + kfree(cp->context);
>> + found = true;
>> + goto clear_out;
>> + }
>> +
>
> If you re-order your pr_warn() below with your memset() to address the
> earlier comment, you'll end up trying to print the freed memory. Not a
> problem if you just drop the pr_warn() altogether.
^ permalink raw reply
* Re: [PATCH v12 15/25] LSM: Use lsmcontext in security_secid_to_secctx
From: Stephen Smalley @ 2019-12-18 19:33 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20191216223621.5127-16-casey@schaufler-ca.com>
On 12/16/19 5:36 PM, Casey Schaufler wrote:
> Replace the (secctx,seclen) pointer pair with a single
> lsmcontext pointer to allow return of the LSM identifier
> along with the context and context length. This allows
> security_release_secctx() to know how to release the
> context. Callers have been modified to use or save the
> returned data from the new structure.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: netdev@vger.kernel.org
Sorry, have to retract my ack. See below.
> ---
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 3305c4af43a8..224c7b4a1bc0 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1178,9 +1178,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> struct audit_buffer *ab;
> u16 msg_type = nlh->nlmsg_type;
> struct audit_sig_info *sig_data;
> - char *ctx = NULL;
> u32 len;
> - struct lsmcontext scaff; /* scaffolding */
> + struct lsmcontext context = { };
>
> err = audit_netlink_ok(skb, msg_type);
> if (err)
> @@ -1418,25 +1417,22 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> case AUDIT_SIGNAL_INFO:
> len = 0;
> if (lsmblob_is_set(&audit_sig_lsm)) {
> - err = security_secid_to_secctx(&audit_sig_lsm, &ctx,
> - &len);
> + err = security_secid_to_secctx(&audit_sig_lsm,
> + &context);
> if (err)
> return err;
> }
> sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL);
You forgot to update this kmalloc() to use context.len, so you'll end up
allocating too small a buffer and then writing out of bounds upon the
memcpy below.
> if (!sig_data) {
> - if (lsmblob_is_set(&audit_sig_lsm)) {
> - lsmcontext_init(&scaff, ctx, len, 0);
> - security_release_secctx(&scaff);
> - }
> + if (lsmblob_is_set(&audit_sig_lsm))
> + security_release_secctx(&context);
> return -ENOMEM;
> }
> sig_data->uid = from_kuid(&init_user_ns, audit_sig_uid);
> sig_data->pid = audit_sig_pid;
> if (lsmblob_is_set(&audit_sig_lsm)) {
> - memcpy(sig_data->ctx, ctx, len);
> - lsmcontext_init(&scaff, ctx, len, 0);
> - security_release_secctx(&scaff);
> + memcpy(sig_data->ctx, context.context, context.len);
> + security_release_secctx(&context);
> }
> audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
> sig_data, sizeof(*sig_data) + len);
^ permalink raw reply
* Re: [PATCH v4 1/9] capabilities: introduce CAP_SYS_PERFMON to kernel and user space
From: Stephen Smalley @ 2019-12-18 19:56 UTC (permalink / raw)
To: Alexey Budankov, Peter Zijlstra, Arnaldo Carvalho de Melo,
Ingo Molnar, jani.nikula@linux.intel.com,
joonas.lahtinen@linux.intel.com, rodrigo.vivi@intel.com,
Alexei Starovoitov, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, james.bottomley@hansenpartnership.com,
Serge Hallyn, James Morris, Will Deacon, Mark Rutland,
Casey Schaufler, Robert Richter
Cc: Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Kees Cook, Jann Horn,
Thomas Gleixner, Tvrtko Ursulin, Lionel Landwerlin, Song Liu,
linux-kernel, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, intel-gfx@lists.freedesktop.org,
bpf@vger.kernel.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-perf-users@vger.kernel.org,
linux-arm-kernel, oprofile-list
In-Reply-To: <e0cb2b8d-e964-bc23-bf80-58d7ac4ed6f1@linux.intel.com>
On 12/18/19 4:24 AM, Alexey Budankov wrote:
>
> Introduce CAP_SYS_PERFMON capability devoted to secure system performance
> monitoring and observability operations so that CAP_SYS_PERFMON would
> assist CAP_SYS_ADMIN capability in its governing role for perf_events,
> i915_perf and other subsystems of the kernel.
>
> CAP_SYS_PERFMON intends to harden system security and integrity during
> system performance monitoring and observability operations by decreasing
> attack surface that is available to CAP_SYS_ADMIN privileged processes.
>
> CAP_SYS_PERFMON intends to take over CAP_SYS_ADMIN credentials related
> to system performance monitoring and observability operations and balance
> amount of CAP_SYS_ADMIN credentials in accordance with the recommendations
> provided in the man page for CAP_SYS_ADMIN [1]: "Note: this capability
> is overloaded; see Notes to kernel developers, below."
>
> [1] http://man7.org/linux/man-pages/man7/capabilities.7.html
>
> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Note for selinux developers: we will need to update the
selinux-testsuite tests for perf_event when/if this change lands upstream.
> ---
> include/linux/capability.h | 4 ++++
> include/uapi/linux/capability.h | 8 +++++++-
> security/selinux/include/classmap.h | 4 ++--
> 3 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index ecce0f43c73a..883c879baa4b 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -251,6 +251,10 @@ extern bool privileged_wrt_inode_uidgid(struct user_namespace *ns, const struct
> extern bool capable_wrt_inode_uidgid(const struct inode *inode, int cap);
> extern bool file_ns_capable(const struct file *file, struct user_namespace *ns, int cap);
> extern bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns);
> +static inline bool perfmon_capable(void)
> +{
> + return capable(CAP_SYS_PERFMON) || capable(CAP_SYS_ADMIN);
> +}
>
> /* audit system wants to get cap info from files as well */
> extern int get_vfs_caps_from_disk(const struct dentry *dentry, struct cpu_vfs_cap_data *cpu_caps);
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index 240fdb9a60f6..98e03cc76c7c 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -366,8 +366,14 @@ struct vfs_ns_cap_data {
>
> #define CAP_AUDIT_READ 37
>
> +/*
> + * Allow system performance and observability privileged operations
> + * using perf_events, i915_perf and other kernel subsystems
> + */
> +
> +#define CAP_SYS_PERFMON 38
>
> -#define CAP_LAST_CAP CAP_AUDIT_READ
> +#define CAP_LAST_CAP CAP_SYS_PERFMON
>
> #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 7db24855e12d..bae602c623b0 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -27,9 +27,9 @@
> "audit_control", "setfcap"
>
> #define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \
> - "wake_alarm", "block_suspend", "audit_read"
> + "wake_alarm", "block_suspend", "audit_read", "sys_perfmon"
>
> -#if CAP_LAST_CAP > CAP_AUDIT_READ
> +#if CAP_LAST_CAP > CAP_SYS_PERFMON
> #error New capability defined, please update COMMON_CAP2_PERMS.
> #endif
>
>
^ permalink raw reply
* Re: [PATCH v12 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs
From: Stephen Smalley @ 2019-12-18 20:50 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <e7aa3b6f-cee1-6277-21dd-77a4db9bbc2b@tycho.nsa.gov>
On 12/18/19 2:12 PM, Stephen Smalley wrote:
> On 12/18/19 1:28 PM, Stephen Smalley wrote:
>> On 12/16/19 5:36 PM, Casey Schaufler wrote:
>>> The getsockopt SO_PEERSEC provides the LSM based security
>>> information for a single module, but for reasons of backward
>>> compatibility cannot include the information for multiple
>>> modules. A new option SO_PEERCONTEXT is added to report the
>>> security "context" of multiple modules using a "compound" format
>>>
>>> lsm1\0value\0lsm2\0value\0
>>>
>>> This is expected to be used by system services, including dbus-daemon.
>>> The exact format of a compound context has been the subject of
>>> considerable debate. This format was suggested by Simon McVittie,
>>> a dbus maintainer with a significant stake in the format being
>>> usable.
>>>
>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>> cc: netdev@vger.kernel.org
>>
>> Requires ack by netdev and linux-api. A couple of comments below.
>
> Also, have you tested this new interface? I may be doing something
> wrong, but a trivial attempt to use SO_PEERCONTEXT with both SELinux and
> AppArmor enabled only appeared to return the SELinux portion of the
> label
> (selinux\0unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023\0),
> whereas /proc/self/attr/context returned a compound context (the same
> but with apparmor\0unconfined\n\0 appended).
Ok, this seems to be a lack of support in AppArmor for saving the peer
info for unix/local domain sockets, so not your bug. Doesn't implement
the necessary hooks.
>
>>
>>> ---
>>
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index 2bf82e1cf347..2ae10e7f81a7 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -880,8 +880,8 @@
>>> * SO_GETPEERSEC. For tcp sockets this can be meaningful if the
>>> * socket is associated with an ipsec SA.
>>> * @sock is the local socket.
>>> - * @optval userspace memory where the security state is to be
>>> copied.
>>> - * @optlen userspace int where the module should copy the actual
>>> length
>>> + * @optval memory where the security state is to be copied.
>>
>> This is misleading; it suggests that the caller is providing an
>> allocated buffer into which the security module copies its data.
>> Instead it is just a pointer to a pointer that is then set by the
>> security module to a buffer the module allocates.
>>
>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>> index 536db4dbfcbb..b72bb90b1903 100644
>>> --- a/include/linux/security.h
>>> +++ b/include/linux/security.h
>>> @@ -181,7 +181,7 @@ struct lsmblob {
>>> #define LSMBLOB_NEEDED -2 /* Slot requested on
>>> initialization */
>>> #define LSMBLOB_NOT_NEEDED -3 /* Slot not requested */
>>> #define LSMBLOB_DISPLAY -4 /* Use the "display" slot */
>>> -#define LSMBLOB_FIRST -5 /* Use the default "display" slot */
>>> +#define LSMBLOB_COMPOUND -5 /* A compound "display" */
>>
>> I'm puzzled by the removal of LSMBLOB_FIRST by this patch; it suggests
>> it was never needed in the first place by the patch that introduced
>> it. But more below.
>>
>>> diff --git a/security/security.c b/security/security.c
>>> index d0b57a7c3b31..1afe245f3246 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -723,6 +723,42 @@ static void __init lsm_early_task(struct
>>> task_struct *task)
>>> panic("%s: Early task alloc failed.\n", __func__);
>>> }
>>> +/**
>>> + * append_ctx - append a lsm/context pair to a compound context
>>> + * @ctx: the existing compound context
>>> + * @ctxlen: size of the old context, including terminating nul byte
>>> + * @lsm: new lsm name, nul terminated
>>> + * @new: new context, possibly nul terminated
>>> + * @newlen: maximum size of @new
>>> + *
>>> + * replace @ctx with a new compound context, appending @newlsm and @new
>>> + * to @ctx. On exit the new data replaces the old, which is freed.
>>> + * @ctxlen is set to the new size, which includes a trailing nul byte.
>>> + *
>>> + * Returns 0 on success, -ENOMEM if no memory is available.
>>> + */
>>> +static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char
>>> *new,
>>> + int newlen)
>>> +{
>>> + char *final;
>>> + int llen;
>>> +
>>> + llen = strlen(lsm) + 1;
>>> + newlen = strnlen(new, newlen) + 1;
>>> +
>>> + final = kzalloc(*ctxlen + llen + newlen, GFP_KERNEL);
>>> + if (final == NULL)
>>> + return -ENOMEM;
>>> + if (*ctxlen)
>>> + memcpy(final, *ctx, *ctxlen);
>>> + memcpy(final + *ctxlen, lsm, llen);
>>> + memcpy(final + *ctxlen + llen, new, newlen);
>>> + kfree(*ctx);
>>> + *ctx = final;
>>> + *ctxlen = *ctxlen + llen + newlen;
>>> + return 0;
>>> +}
>>
>> You should likely take some precautions against integer overflows in
>> the above code?
>>
>>> +
>>> /*
>>> * Hook list operation macros.
>>> *
>>> @@ -2164,8 +2200,8 @@ int security_setprocattr(const char *lsm, const
>>> char *name, void *value,
>>> hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
>>> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>>> continue;
>>> - if (lsm == NULL && *display != LSMBLOB_INVALID &&
>>> - *display != hp->lsmid->slot)
>>> + if (lsm == NULL && display != NULL &&
>>> + *display != LSMBLOB_INVALID && *display != hp->lsmid->slot)
>>> continue;
>>> return hp->hook.setprocattr(name, value, size);
>>> }
>>
>> Is this a bug fix that should be folded into the earlier patch that
>> introduced it?
>>
>>> @@ -2196,7 +2232,7 @@ int security_secid_to_secctx(struct lsmblob
>>> *blob, struct lsmcontext *cp,
>>> */
>>> if (display == LSMBLOB_DISPLAY)
>>> display = lsm_task_display(current);
>>> - else if (display == LSMBLOB_FIRST)
>>> + else if (display == 0)
>>> display = LSMBLOB_INVALID;
>>> else if (display < 0) {
>>> WARN_ONCE(true,
>>
>> Why is it necessary to re-map display 0 in this manner? Previously if
>> display 0 was specified, it would require it to match the lsmid->slot
>> value. Won't it match anyway?
>>
>>> @@ -2246,6 +2282,15 @@ void security_release_secctx(struct lsmcontext
>>> *cp)
>>> struct security_hook_list *hp;
>>> bool found = false;
>>> + if (cp->slot == LSMBLOB_INVALID)
>>> + return;
>>> +
>>> + if (cp->slot == LSMBLOB_COMPOUND) {
>>> + kfree(cp->context);
>>> + found = true;
>>> + goto clear_out;
>>> + }
>>> +
>>
>> If you re-order your pr_warn() below with your memset() to address the
>> earlier comment, you'll end up trying to print the freed memory. Not
>> a problem if you just drop the pr_warn() altogether.
>
^ permalink raw reply
* Re: Looks like issue in handling active_nodes count in 4.19 kernel .
From: Paul Moore @ 2019-12-19 2:20 UTC (permalink / raw)
To: Stephen Smalley; +Cc: Ravi Kumar Siddojigari, selinux, linux-security-module
In-Reply-To: <21b5511a-fdba-3c2f-e9a6-efdc890b5881@tycho.nsa.gov>
On Tue, Dec 17, 2019 at 10:51 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> On 12/17/19 10:40 AM, Ravi Kumar Siddojigari wrote:
> > Yes indeed this is a stress test on ARM64 device with multicore where most of the cores /tasks are stuck in avc_reclaim_node .
> > We still see this issue even after picking the earlier patch " selinux: ensure we cleanup the internal AVC counters on error in avc_insert() commit: d8db60cb23e4"
> > Where selinux_state during issue was as below where all the slots are NULL and the count was more than threshold.
> > Which seem to be calling avc_reclaim_node always and as the all the slots are empty its going for full for- loop with locks and unlock and taking too long .
> > Not sure what could make the slots null , for sure its not due to flush() /Reset(). We think that still we need to call avc_kill_node in update_node function .
> > Adding the patch below can you please review or correct the following patch .
> >
> >
> > selinux_state = (
> > disabled = FALSE,
> > enforcing = TRUE,
> > checkreqprot = FALSE,
> > initialized = TRUE,
> > policycap = (TRUE, TRUE, TRUE, FALSE, FALSE, TRUE),
> > avc = 0xFFFFFF9BEFF1E890 -> (
> > avc_cache_threshold = 512, /* <<<<<not configured and its with default*/
> > avc_cache = (
> > slots = ((first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first /*<<<< all are NULL */
> > slots_lock = ((rlock = (raw_lock = (val = (counter = 0), locked = 0, pending = 0, locked_pending = 0, tail = 0), magic = 3735899821, owner_cpu = 4294967295, owner = 0xFFFFFFFFFFFFFFFF, dep_map = (key = 0xFFFFFF9BEFF298A8, cla
> > lru_hint = (counter = 616831529),
> > active_nodes = (counter = 547), /*<<<<< increased more than 512*/
> > latest_notif = 1)),
> > ss = 0xFFFFFF9BEFF2E578)
> >
> >
> > --
> > In AVC update we don't call avc_node_kill() when avc_xperms_populate()
> > fails, resulting in the avc->avc_cache.active_nodes counter having a
> > false value.In last patch this changes was missed , so correcting it.
> >
> > Change-Id: Ic0298162cc766c0f21be7ab232e259766654dad3
> > Signed-off-by: Jaihind Yadav<jaihindyadav@codeaurora.org>
> > ---
> > security/selinux/avc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> > index 91d24c2..3d1cff2 100644
> > --- a/security/selinux/avc.c
> > +++ b/security/selinux/avc.c
> > @@ -913,7 +913,7 @@ static int avc_update_node(struct selinux_avc *avc,
> > if (orig->ae.xp_node) {
> > rc = avc_xperms_populate(node, orig->ae.xp_node);
> > if (rc) {
> > - kmem_cache_free(avc_node_cachep, node);
> > + avc_node_kill(avc, node);
> > goto out_unlock;
> > }
> > }
> > --
>
> That looks correct to me; I guess that one got missed by the prior fix.
> Still not sure how your AVC got into that state though...
>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
This looks good to me too. Ravi, can you submit this as a proper
patch with From: set to Jaihing Yadav (assuming they are the author)
and your sign-off?
Thanks.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v4 4/9] drm/i915/perf: open access for CAP_SYS_PERFMON privileged process
From: Lionel Landwerlin @ 2019-12-19 9:10 UTC (permalink / raw)
To: Alexey Budankov, Peter Zijlstra, Arnaldo Carvalho de Melo,
Ingo Molnar, jani.nikula@linux.intel.com,
joonas.lahtinen@linux.intel.com, rodrigo.vivi@intel.com,
Alexei Starovoitov, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, james.bottomley@hansenpartnership.com,
Serge Hallyn, James Morris, Will Deacon, Mark Rutland,
Casey Schaufler, Robert Richter
Cc: Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Kees Cook, Jann Horn,
Thomas Gleixner, Tvrtko Ursulin, Song Liu, linux-kernel,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
intel-gfx@lists.freedesktop.org, bpf@vger.kernel.org,
linux-parisc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-perf-users@vger.kernel.org, linux-arm-kernel, oprofile-list
In-Reply-To: <ea050255-a125-8831-ce91-ee23bd6ad08b@linux.intel.com>
On 18/12/2019 11:27, Alexey Budankov wrote:
> Open access to i915_perf monitoring for CAP_SYS_PERFMON privileged
> processes. For backward compatibility reasons access to i915_perf
> subsystem remains open for CAP_SYS_ADMIN privileged processes but
> CAP_SYS_ADMIN usage for secure i915_perf monitoring is discouraged
> with respect to CAP_SYS_PERFMON capability.
>
> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
Acked-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
> drivers/gpu/drm/i915/i915_perf.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index e42b86827d6b..e2697f8d04de 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -2748,10 +2748,10 @@ i915_perf_open_ioctl_locked(struct drm_i915_private *dev_priv,
> /* Similar to perf's kernel.perf_paranoid_cpu sysctl option
> * we check a dev.i915.perf_stream_paranoid sysctl option
> * to determine if it's ok to access system wide OA counters
> - * without CAP_SYS_ADMIN privileges.
> + * without CAP_SYS_PERFMON or CAP_SYS_ADMIN privileges.
> */
> if (privileged_op &&
> - i915_perf_stream_paranoid && !capable(CAP_SYS_ADMIN)) {
> + i915_perf_stream_paranoid && !perfmon_capable()) {
> DRM_DEBUG("Insufficient privileges to open system-wide i915 perf stream\n");
> ret = -EACCES;
> goto err_ctx;
> @@ -2939,9 +2939,8 @@ static int read_properties_unlocked(struct drm_i915_private *dev_priv,
> } else
> oa_freq_hz = 0;
>
> - if (oa_freq_hz > i915_oa_max_sample_rate &&
> - !capable(CAP_SYS_ADMIN)) {
> - DRM_DEBUG("OA exponent would exceed the max sampling frequency (sysctl dev.i915.oa_max_sample_rate) %uHz without root privileges\n",
> + if (oa_freq_hz > i915_oa_max_sample_rate && !perfmon_capable()) {
> + DRM_DEBUG("OA exponent would exceed the max sampling frequency (sysctl dev.i915.oa_max_sample_rate) %uHz without CAP_SYS_PERFMON or CAP_SYS_ADMIN privileges\n",
> i915_oa_max_sample_rate);
> return -EACCES;
> }
> @@ -3328,7 +3327,7 @@ int i915_perf_add_config_ioctl(struct drm_device *dev, void *data,
> return -EINVAL;
> }
>
> - if (i915_perf_stream_paranoid && !capable(CAP_SYS_ADMIN)) {
> + if (i915_perf_stream_paranoid && !perfmon_capable()) {
> DRM_DEBUG("Insufficient privileges to add i915 OA config\n");
> return -EACCES;
> }
> @@ -3474,7 +3473,7 @@ int i915_perf_remove_config_ioctl(struct drm_device *dev, void *data,
> return -ENOTSUPP;
> }
>
> - if (i915_perf_stream_paranoid && !capable(CAP_SYS_ADMIN)) {
> + if (i915_perf_stream_paranoid && !perfmon_capable()) {
> DRM_DEBUG("Insufficient privileges to remove i915 OA config\n");
> return -EACCES;
> }
^ permalink raw reply
* RE: Looks like issue in handling active_nodes count in 4.19 kernel .
From: Ravi Kumar Siddojigari @ 2019-12-19 9:48 UTC (permalink / raw)
To: 'Paul Moore', 'Stephen Smalley'
Cc: selinux, linux-security-module
In-Reply-To: <CAHC9VhQYA8uTRQ0OajEmsTrDytNVx+BSiL5vEsGefKEhAw+gKA@mail.gmail.com>
Sorry , Re-adding the patch below as requested.
Stephen ,
Issue is fixed with this 2 changes , Issue as even reproduced on v4.14 and similar changes work there also .
--
From 77c618006397c7a65ead257f3cb4e4fe3da2d4b8 Mon Sep 17 00:00:00 2001
From: Jaihind Yadav <jaihindyadav@codeaurora.org>
Date: Tue, 17 Dec 2019 17:25:47 +0530
Subject: [PATCH] selinux: ensure we cleanup the internal AVC counters on error
in avc_update()
In AVC update we don't call avc_node_kill() when avc_xperms_populate()
fails, resulting in the avc->avc_cache.active_nodes counter having a
false value. In last patch this changes was missed , so correcting it.
Change-Id: Ic0298162cc766c0f21be7ab232e259766654dad3
Signed-off-by: Ravi Kumar Siddojigari <rsiddoji@codeaurora.org>
---
security/selinux/avc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 91d24c2..3d1cff2 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -913,7 +913,7 @@ static int avc_update_node(struct selinux_avc *avc,
if (orig->ae.xp_node) {
rc = avc_xperms_populate(node, orig->ae.xp_node);
if (rc) {
- kmem_cache_free(avc_node_cachep, node);
+ avc_node_kill(avc, node);
goto out_unlock;
}
}
--
1.9.1
Br,
-----Original Message-----
From: selinux-owner@vger.kernel.org <selinux-owner@vger.kernel.org> On Behalf Of Paul Moore
Sent: Thursday, December 19, 2019 7:50 AM
To: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Ravi Kumar Siddojigari <rsiddoji@codeaurora.org>; selinux@vger.kernel.org; linux-security-module@vger.kernel.org
Subject: Re: Looks like issue in handling active_nodes count in 4.19 kernel .
On Tue, Dec 17, 2019 at 10:51 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> On 12/17/19 10:40 AM, Ravi Kumar Siddojigari wrote:
> > Yes indeed this is a stress test on ARM64 device with multicore where most of the cores /tasks are stuck in avc_reclaim_node .
> > We still see this issue even after picking the earlier patch " selinux: ensure we cleanup the internal AVC counters on error in avc_insert() commit: d8db60cb23e4"
> > Where selinux_state during issue was as below where all the slots are NULL and the count was more than threshold.
> > Which seem to be calling avc_reclaim_node always and as the all the slots are empty its going for full for- loop with locks and unlock and taking too long .
> > Not sure what could make the slots null , for sure its not due to flush() /Reset(). We think that still we need to call avc_kill_node in update_node function .
> > Adding the patch below can you please review or correct the following patch .
> >
> >
> > selinux_state = (
> > disabled = FALSE,
> > enforcing = TRUE,
> > checkreqprot = FALSE,
> > initialized = TRUE,
> > policycap = (TRUE, TRUE, TRUE, FALSE, FALSE, TRUE),
> > avc = 0xFFFFFF9BEFF1E890 -> (
> > avc_cache_threshold = 512, /* <<<<<not configured and its with default*/
> > avc_cache = (
> > slots = ((first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first = 0x0), (first /*<<<< all are NULL */
> > slots_lock = ((rlock = (raw_lock = (val = (counter = 0), locked = 0, pending = 0, locked_pending = 0, tail = 0), magic = 3735899821, owner_cpu = 4294967295, owner = 0xFFFFFFFFFFFFFFFF, dep_map = (key = 0xFFFFFF9BEFF298A8, cla
> > lru_hint = (counter = 616831529),
> > active_nodes = (counter = 547), /*<<<<< increased more than 512*/
> > latest_notif = 1)),
> > ss = 0xFFFFFF9BEFF2E578)
> >
> >
> > --
> > In AVC update we don't call avc_node_kill() when
> > avc_xperms_populate() fails, resulting in the
> > avc->avc_cache.active_nodes counter having a false value.In last patch this changes was missed , so correcting it.
> >
> > Change-Id: Ic0298162cc766c0f21be7ab232e259766654dad3
> > Signed-off-by: Jaihind Yadav<jaihindyadav@codeaurora.org>
> > ---
> > security/selinux/avc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/security/selinux/avc.c b/security/selinux/avc.c index
> > 91d24c2..3d1cff2 100644
> > --- a/security/selinux/avc.c
> > +++ b/security/selinux/avc.c
> > @@ -913,7 +913,7 @@ static int avc_update_node(struct selinux_avc *avc,
> > if (orig->ae.xp_node) {
> > rc = avc_xperms_populate(node, orig->ae.xp_node);
> > if (rc) {
> > - kmem_cache_free(avc_node_cachep, node);
> > + avc_node_kill(avc, node);
> > goto out_unlock;
> > }
> > }
> > --
>
> That looks correct to me; I guess that one got missed by the prior fix.
> Still not sure how your AVC got into that state though...
>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
This looks good to me too. Ravi, can you submit this as a proper patch with From: set to Jaihing Yadav (assuming they are the author) and your sign-off?
Thanks.
--
paul moore
www.paul-moore.com
^ permalink raw reply related
* Re: [PATCH v12 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs
From: Simon McVittie @ 2019-12-19 12:19 UTC (permalink / raw)
To: Stephen Smalley
Cc: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux, keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <a522de22-ba62-a24d-24f7-b69418e7ec0b@tycho.nsa.gov>
On Wed, 18 Dec 2019 at 15:50:45 -0500, Stephen Smalley wrote:
> Ok, this seems to be a lack of support in AppArmor for saving the peer info
> for unix/local domain sockets, so not your bug. Doesn't implement the
> necessary hooks.
Ubuntu kernels have working getsockopt SO_PEERSEC for AppArmor (which
definitely works for D-Bus, or did when I tried it in the past), but it
seems to be a downstream patch.
Is there a combination of LSMs where this works correctly and shows
multiple LSMs' labels, either upstream or with downstream patches? D-Bus
ought to be an early consumer of this information, but that's going to
be difficult if there's nowhere I can test it. If there was a pair of
in-tree or out-of-tree test LSMs that automatically labelled process
12345 and all sockets that it created with something like (pseudocode)
{
"labeltest1": "labeltest1process12345",
"labeltest2": "labeltest2process12345"
}
then that would make testing straightforward.
From my user-space developer perspective, I'd really like the kernel
to have at least brief documentation of the assumptions that can be
made about the compound format, to avoid people like me having to
ask the linux-security-module mailing list and use the answers as our
documentation:
- Can I assume that the LSM names (conceptually, keys in the associative
array) are printable ASCII? If not, can I at least assume that they are
printable UTF-8, or that LSMs whose names are not printable UTF-8 are
too weird to be worth supporting in user-space APIs?
If this decision hasn't been made yet, I would like to request ASCII,
or UTF-8 if a wider character repertoire is desired - that would make
user-space APIs a lot easier to write.
- What can I assume about the format of the labels (values in the
associative array)?
From previous conversations on linux-security-module it seems the
answer is: they are intended to be printable strings; their encoding
is unspecified (could be ASCII or UTF-8, but equally could be Latin-1
or anything else); there are no reserved or disallowed characters
except '\0'; individual LSMs like SELinux and AppArmor might impose
tighter restrictions, but LSM-agnostic code can't assume those
restrictions are present.
Even if the format is conceptually an unspecified encoding with every
nonzero byte allowed, if LSM and security policy authors establish a
convention that the labels are ASCII or UTF-8 without control characters
such as newlines, that would make user-space a lot easier to deal with.
- Can I assume that the names and labels in /proc/$pid/attr/context
correspond exactly (same label <=> same bytes) to the ones in
SO_PEERCONTEXT?
In particular, a design issue in the previous interfaces with
/proc/$pid/attr/current and SO_PEERSEC was that AppArmor puts
"unconfined\n" in /proc/$pid/attr/current, but "unconfined\0" in
SO_PEERSEC, and implementations of protocols like D-Bus can't know
that these are meant to be the same without LSM-specific knowledge
(namely "AppArmor profile names can't contain newlines").
If this new API is an opportunity to declare that LSMs are expected
to put the same canonical form of a label in /proc/$pid/attr/context and
SO_PEERCONTEXT, possibly with a non-canonical version (adding '\n' or
'\0' or similar) exposed in the older /proc/$pid/attr/current and
SO_PEERSEC interfaces for backwards compatibility, then that would make
life a lot easier for user-space developers like me.
- Does the order of the names and values matter? Can I assume anything
about them?
It would make the D-Bus API more straightforward if I can assume that
the order doesn't matter, and represent it as an unordered map
(associative array), like a Perl hash or Python dict.
I'm asking all this from the D-Bus perspective, but anything that
wants to store or forward LSM contexts in an LSM-agnostic way will
have essentially the same requirements - I could imagine X11, Wayland,
varlink etc. hitting the same problems D-Bus did.
Thanks,
smcv
^ permalink raw reply
* Re: [PATCH v12 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs
From: Stephen Smalley @ 2019-12-19 13:47 UTC (permalink / raw)
To: Simon McVittie
Cc: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux, keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20191219121939.GA1291250@horizon>
On 12/19/19 7:19 AM, Simon McVittie wrote:
> On Wed, 18 Dec 2019 at 15:50:45 -0500, Stephen Smalley wrote:
>> Ok, this seems to be a lack of support in AppArmor for saving the peer info
>> for unix/local domain sockets, so not your bug. Doesn't implement the
>> necessary hooks.
>
> Ubuntu kernels have working getsockopt SO_PEERSEC for AppArmor (which
> definitely works for D-Bus, or did when I tried it in the past), but it
> seems to be a downstream patch.
>
> Is there a combination of LSMs where this works correctly and shows
> multiple LSMs' labels, either upstream or with downstream patches? D-Bus
> ought to be an early consumer of this information, but that's going to
> be difficult if there's nowhere I can test it. If there was a pair of
> in-tree or out-of-tree test LSMs that automatically labelled process
> 12345 and all sockets that it created with something like (pseudocode)
>
> {
> "labeltest1": "labeltest1process12345",
> "labeltest2": "labeltest2process12345"
> }
>
> then that would make testing straightforward.
AFAICT, you can't yet stack Smack+SELinux, and AppArmor requires
out-of-tree patches to support SO_PEERSEC so there is no in-tree
combination that will demonstrate the new SO_PEERCONTEXT for multiple lsms.
> From my user-space developer perspective, I'd really like the kernel
> to have at least brief documentation of the assumptions that can be
> made about the compound format, to avoid people like me having to
> ask the linux-security-module mailing list and use the answers as our
> documentation:
>
> - Can I assume that the LSM names (conceptually, keys in the associative
> array) are printable ASCII? If not, can I at least assume that they are
> printable UTF-8, or that LSMs whose names are not printable UTF-8 are
> too weird to be worth supporting in user-space APIs?
>
> If this decision hasn't been made yet, I would like to request ASCII,
> or UTF-8 if a wider character repertoire is desired - that would make
> user-space APIs a lot easier to write.
ASCII works for me.
>
> - What can I assume about the format of the labels (values in the
> associative array)?
>
> From previous conversations on linux-security-module it seems the
> answer is: they are intended to be printable strings; their encoding
> is unspecified (could be ASCII or UTF-8, but equally could be Latin-1
> or anything else); there are no reserved or disallowed characters
> except '\0'; individual LSMs like SELinux and AppArmor might impose
> tighter restrictions, but LSM-agnostic code can't assume those
> restrictions are present.
>
> Even if the format is conceptually an unspecified encoding with every
> nonzero byte allowed, if LSM and security policy authors establish a
> convention that the labels are ASCII or UTF-8 without control characters
> such as newlines, that would make user-space a lot easier to deal with.
I believe there is existing userspace code that seems to presume that
they are NUL-terminated strings printable using %s format specifiers to
printf-like functions in output and log messages _without_ any escaping.
Not just the LSM-specific libraries. systemd (for SO_PEERSEC),
procps-ng (for /proc/pid/attr/current), perhaps others. I think we can
rely on that remaining true since the world would break.
> - Can I assume that the names and labels in /proc/$pid/attr/context
> correspond exactly (same label <=> same bytes) to the ones in
> SO_PEERCONTEXT?
>
> In particular, a design issue in the previous interfaces with
> /proc/$pid/attr/current and SO_PEERSEC was that AppArmor puts
> "unconfined\n" in /proc/$pid/attr/current, but "unconfined\0" in
> SO_PEERSEC, and implementations of protocols like D-Bus can't know
> that these are meant to be the same without LSM-specific knowledge
> (namely "AppArmor profile names can't contain newlines").
>
> If this new API is an opportunity to declare that LSMs are expected
> to put the same canonical form of a label in /proc/$pid/attr/context and
> SO_PEERCONTEXT, possibly with a non-canonical version (adding '\n' or
> '\0' or similar) exposed in the older /proc/$pid/attr/current and
> SO_PEERSEC interfaces for backwards compatibility, then that would make
> life a lot easier for user-space developers like me.
I'm all for this but the current implementation reuses the same
underlying hooks as SO_PEERSEC, so it gets the same result for the
per-lsm values. We'd need a separate hook if we cannot alter the
current AppArmor SO_PEERSEC format. Technically AppArmor seemingly
doesn't truly provide SO_PEERSEC in mainline today even though it
implements the hook for returning the result because it never actually
sets the peer on Unix domain or INET domain connections AFAICT, so they
could change the format upstream without a compatibility break but it
may break Ubuntu. So it might require using a new hook for
SO_PEERCONTEXT. Which is fine with me.
> - Does the order of the names and values matter? Can I assume anything
> about them?
>
> It would make the D-Bus API more straightforward if I can assume that
> the order doesn't matter, and represent it as an unordered map
> (associative array), like a Perl hash or Python dict.
IMHO order shouldn't matter.
> I'm asking all this from the D-Bus perspective, but anything that
> wants to store or forward LSM contexts in an LSM-agnostic way will
> have essentially the same requirements - I could imagine X11, Wayland,
> varlink etc. hitting the same problems D-Bus did.
>
> Thanks,
> smcv
>
^ permalink raw reply
* Re: [PATCH v12 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs
From: Stephen Smalley @ 2019-12-19 15:00 UTC (permalink / raw)
To: Simon McVittie
Cc: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux, keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <55b5c889-ff38-38c4-578e-ec4211b837a4@tycho.nsa.gov>
On 12/19/19 8:47 AM, Stephen Smalley wrote:
> On 12/19/19 7:19 AM, Simon McVittie wrote:
>> On Wed, 18 Dec 2019 at 15:50:45 -0500, Stephen Smalley wrote:
>>> Ok, this seems to be a lack of support in AppArmor for saving the
>>> peer info
>>> for unix/local domain sockets, so not your bug. Doesn't implement the
>>> necessary hooks.
>>
>> Ubuntu kernels have working getsockopt SO_PEERSEC for AppArmor (which
>> definitely works for D-Bus, or did when I tried it in the past), but it
>> seems to be a downstream patch.
>>
>> Is there a combination of LSMs where this works correctly and shows
>> multiple LSMs' labels, either upstream or with downstream patches? D-Bus
>> ought to be an early consumer of this information, but that's going to
>> be difficult if there's nowhere I can test it. If there was a pair of
>> in-tree or out-of-tree test LSMs that automatically labelled process
>> 12345 and all sockets that it created with something like (pseudocode)
>>
>> {
>> "labeltest1": "labeltest1process12345",
>> "labeltest2": "labeltest2process12345"
>> }
>>
>> then that would make testing straightforward.
>
> AFAICT, you can't yet stack Smack+SELinux, and AppArmor requires
> out-of-tree patches to support SO_PEERSEC so there is no in-tree
> combination that will demonstrate the new SO_PEERCONTEXT for multiple lsms.
>
>> From my user-space developer perspective, I'd really like the kernel
>> to have at least brief documentation of the assumptions that can be
>> made about the compound format, to avoid people like me having to
>> ask the linux-security-module mailing list and use the answers as our
>> documentation:
>>
>> - Can I assume that the LSM names (conceptually, keys in the associative
>> array) are printable ASCII? If not, can I at least assume that they
>> are
>> printable UTF-8, or that LSMs whose names are not printable UTF-8 are
>> too weird to be worth supporting in user-space APIs?
>>
>> If this decision hasn't been made yet, I would like to request ASCII,
>> or UTF-8 if a wider character repertoire is desired - that would make
>> user-space APIs a lot easier to write.
>
> ASCII works for me.
>
>>
>> - What can I assume about the format of the labels (values in the
>> associative array)?
>>
>> From previous conversations on linux-security-module it seems the
>> answer is: they are intended to be printable strings; their encoding
>> is unspecified (could be ASCII or UTF-8, but equally could be Latin-1
>> or anything else); there are no reserved or disallowed characters
>> except '\0'; individual LSMs like SELinux and AppArmor might impose
>> tighter restrictions, but LSM-agnostic code can't assume those
>> restrictions are present.
>>
>> Even if the format is conceptually an unspecified encoding with every
>> nonzero byte allowed, if LSM and security policy authors establish a
>> convention that the labels are ASCII or UTF-8 without control
>> characters
>> such as newlines, that would make user-space a lot easier to deal
>> with.
>
> I believe there is existing userspace code that seems to presume that
> they are NUL-terminated strings printable using %s format specifiers to
> printf-like functions in output and log messages _without_ any escaping.
> Not just the LSM-specific libraries. systemd (for SO_PEERSEC),
> procps-ng (for /proc/pid/attr/current), perhaps others. I think we can
> rely on that remaining true since the world would break.
Looks like userspace is generally forgiving of whether the terminating
NUL byte is included or omitted by the kernel (with different behaviors
for SELinux - always included, Smack - omitted by /proc/pid/attr/current
but included in SO_PEERSEC, and AppArmor - omitted for
/proc/pid/attr/current but includes a terminating \n, omitted for
SO_PEERSEC but no terminating \n), and procps-ng explicitly tests for
printable characters (but truncates on the first unprintable character).
If we want consistency for /proc/pid/attr/context and SO_PEERCONTEXT,
we'd need different hooks for one or both of them; the current stacking
patches just internally use the existing hooks with their current
behaviors for current and SO_PEERSEC and then compose them.
>
>> - Can I assume that the names and labels in /proc/$pid/attr/context
>> correspond exactly (same label <=> same bytes) to the ones in
>> SO_PEERCONTEXT?
>>
>> In particular, a design issue in the previous interfaces with
>> /proc/$pid/attr/current and SO_PEERSEC was that AppArmor puts
>> "unconfined\n" in /proc/$pid/attr/current, but "unconfined\0" in
>> SO_PEERSEC, and implementations of protocols like D-Bus can't know
>> that these are meant to be the same without LSM-specific knowledge
>> (namely "AppArmor profile names can't contain newlines").
>>
>> If this new API is an opportunity to declare that LSMs are expected
>> to put the same canonical form of a label in
>> /proc/$pid/attr/context and
>> SO_PEERCONTEXT, possibly with a non-canonical version (adding '\n' or
>> '\0' or similar) exposed in the older /proc/$pid/attr/current and
>> SO_PEERSEC interfaces for backwards compatibility, then that would
>> make
>> life a lot easier for user-space developers like me.
>
> I'm all for this but the current implementation reuses the same
> underlying hooks as SO_PEERSEC, so it gets the same result for the
> per-lsm values. We'd need a separate hook if we cannot alter the
> current AppArmor SO_PEERSEC format. Technically AppArmor seemingly
> doesn't truly provide SO_PEERSEC in mainline today even though it
> implements the hook for returning the result because it never actually
> sets the peer on Unix domain or INET domain connections AFAICT, so they
> could change the format upstream without a compatibility break but it
> may break Ubuntu. So it might require using a new hook for
> SO_PEERCONTEXT. Which is fine with me.
>
>> - Does the order of the names and values matter? Can I assume anything
>> about them?
>>
>> It would make the D-Bus API more straightforward if I can assume that
>> the order doesn't matter, and represent it as an unordered map
>> (associative array), like a Perl hash or Python dict.
>
> IMHO order shouldn't matter.
>
>> I'm asking all this from the D-Bus perspective, but anything that
>> wants to store or forward LSM contexts in an LSM-agnostic way will
>> have essentially the same requirements - I could imagine X11, Wayland,
>> varlink etc. hitting the same problems D-Bus did.
>>
>> Thanks,
>> smcv
>>
>
^ permalink raw reply
* Re: Looks like issue in handling active_nodes count in 4.19 kernel .
From: Stephen Smalley @ 2019-12-19 16:00 UTC (permalink / raw)
To: Ravi Kumar Siddojigari, 'Paul Moore'
Cc: selinux, linux-security-module
In-Reply-To: <002701d5b651$8434b2b0$8c9e1810$@codeaurora.org>
On 12/19/19 4:48 AM, Ravi Kumar Siddojigari wrote:
> Sorry , Re-adding the patch below as requested.
>
> Stephen ,
> Issue is fixed with this 2 changes , Issue as even reproduced on v4.14 and similar changes work there also .
It would be preferable if you sent the patch directly via git send-email
or similar. In any event, for the final version, we should drop the
Change-Id because it is Android-specific and we should add a Fixes line
like so:
Fixes: fa1aa143ac4a ("selinux: extended permissions for ioctls")
Given the behavior you are describing and the fact that you could
reproduce it on v4.14 as well, I would recommend marking both it and
Paul's earlier patch for stable (Paul will do this if he agrees; no
action required by you).
>
> --
> From 77c618006397c7a65ead257f3cb4e4fe3da2d4b8 Mon Sep 17 00:00:00 2001
> From: Jaihind Yadav <jaihindyadav@codeaurora.org>
> Date: Tue, 17 Dec 2019 17:25:47 +0530
> Subject: [PATCH] selinux: ensure we cleanup the internal AVC counters on error
> in avc_update()
>
> In AVC update we don't call avc_node_kill() when avc_xperms_populate()
> fails, resulting in the avc->avc_cache.active_nodes counter having a
> false value. In last patch this changes was missed , so correcting it.
>
> Change-Id: Ic0298162cc766c0f21be7ab232e259766654dad3
> Signed-off-by: Ravi Kumar Siddojigari <rsiddoji@codeaurora.org>
> ---
> security/selinux/avc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 91d24c2..3d1cff2 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -913,7 +913,7 @@ static int avc_update_node(struct selinux_avc *avc,
> if (orig->ae.xp_node) {
> rc = avc_xperms_populate(node, orig->ae.xp_node);
> if (rc) {
> - kmem_cache_free(avc_node_cachep, node);
> + avc_node_kill(avc, node);
> goto out_unlock;
> }
> }
> --
> 1.9.1
>
> Br,
>
^ permalink raw reply
* Re: [PATCH v12 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs
From: Simon McVittie @ 2019-12-19 16:48 UTC (permalink / raw)
To: Stephen Smalley
Cc: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux, keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <93912039-e64e-cc56-20fc-095accf6c4dd@tycho.nsa.gov>
On Thu, 19 Dec 2019 at 10:00:31 -0500, Stephen Smalley wrote:
> Looks like userspace is generally forgiving of whether the terminating NUL
> byte is included or omitted by the kernel (with different behaviors for
> SELinux - always included, Smack - omitted by /proc/pid/attr/current but
> included in SO_PEERSEC, and AppArmor - omitted for /proc/pid/attr/current
> but includes a terminating \n, omitted for SO_PEERSEC but no terminating
> \n), and procps-ng explicitly tests for printable characters (but truncates
> on the first unprintable character).
Because LSM people have told me in the past that the '\0' is not
conceptually part of the label, the D-Bus specification and reference
implementation already assume that its presence or absence is irrelevant,
and normalize to a canonical form (which happens to be that it appends a
'\0' if missing, to be nice to C-like languages, but I could equally
have chosen to strip the '\0' and rely on an out-of-band length count).
By design, SO_PEERCONTEXT and /proc/pid/attr/context don't (can't!)
preserve whether the label originally ended with '\0' or not (because
they are designed to use '\0' as a terminator for each label), so these
new kernel interfaces are already a bit closer than the old kernel
interfaces to how D-Bus represents this information.
The problematic case is AppArmor's terminating '\n' on
/proc/pid/attr/current, because when I asked in the past, I was told
that it would be (unwise but) valid to have a LSM where "foo" and "foo\n"
are distinct labels.
If that hypothetical LSM would make procps-ng lose information (because
procps-ng truncates at the first unprintable character), does that change
the situation any? Would that make it acceptable for other LSM-agnostic
user-space components, like the reference implementation of D-Bus, to
assume that stripping a trailing newline from /proc/pid/attr/context
or from one of the component strings of /proc/pid/attr/current is a
non-lossy operation?
> > > If this new API is an opportunity to declare that LSMs are expected
> > > to put the same canonical form of a label in
> > > /proc/$pid/attr/context and
> > > SO_PEERCONTEXT, possibly with a non-canonical version (adding '\n' or
> > > '\0' or similar) exposed in the older /proc/$pid/attr/current and
> > > SO_PEERSEC interfaces for backwards compatibility, then that
> > > would make
> > > life a lot easier for user-space developers like me.
> >
> > I'm all for this but the current implementation reuses the same
> > underlying hooks as SO_PEERSEC, so it gets the same result for the
> > per-lsm values. We'd need a separate hook if we cannot alter the
> > current AppArmor SO_PEERSEC format.
If AppArmor was going to change the format of one of its interfaces
(or deviate from it when implementing new interfaces), I'd actually
prefer it to be /proc/pid/attr/current that changed or was superseded,
because /proc/pid/attr/current is the one that contains a newline that
consumers are meant to ignore.
For what it's worth, libapparmor explicitly removes the newline, so this
only matters to LSM-agnostic readers like D-Bus implementations, and to
lower-level AppArmor-aware readers that use the kernel interfaces directly
in preference to using libapparmor.
smcv
^ permalink raw reply
* Re: [PATCH v12 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs
From: Stephen Smalley @ 2019-12-19 17:02 UTC (permalink / raw)
To: Simon McVittie
Cc: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux, keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20191219164831.GA1308552@horizon>
On 12/19/19 11:48 AM, Simon McVittie wrote:
> On Thu, 19 Dec 2019 at 10:00:31 -0500, Stephen Smalley wrote:
>> Looks like userspace is generally forgiving of whether the terminating NUL
>> byte is included or omitted by the kernel (with different behaviors for
>> SELinux - always included, Smack - omitted by /proc/pid/attr/current but
>> included in SO_PEERSEC, and AppArmor - omitted for /proc/pid/attr/current
>> but includes a terminating \n, omitted for SO_PEERSEC but no terminating
>> \n), and procps-ng explicitly tests for printable characters (but truncates
>> on the first unprintable character).
>
> Because LSM people have told me in the past that the '\0' is not
> conceptually part of the label, the D-Bus specification and reference
> implementation already assume that its presence or absence is irrelevant,
> and normalize to a canonical form (which happens to be that it appends a
> '\0' if missing, to be nice to C-like languages, but I could equally
> have chosen to strip the '\0' and rely on an out-of-band length count).
>
> By design, SO_PEERCONTEXT and /proc/pid/attr/context don't (can't!)
> preserve whether the label originally ended with '\0' or not (because
> they are designed to use '\0' as a terminator for each label), so these
> new kernel interfaces are already a bit closer than the old kernel
> interfaces to how D-Bus represents this information.
>
> The problematic case is AppArmor's terminating '\n' on
> /proc/pid/attr/current, because when I asked in the past, I was told
> that it would be (unwise but) valid to have a LSM where "foo" and "foo\n"
> are distinct labels.
I don't agree with that stance, but obviously others may differ.
> If that hypothetical LSM would make procps-ng lose information (because
> procps-ng truncates at the first unprintable character), does that change
> the situation any? Would that make it acceptable for other LSM-agnostic
> user-space components, like the reference implementation of D-Bus, to
> assume that stripping a trailing newline from /proc/pid/attr/context
> or from one of the component strings of /proc/pid/attr/current is a
> non-lossy operation?
IMHO, yes. In fact, looking further, I see that systemd's
src/libsystemd/sd-bus/bus-creds.c:bus_creds_add_more() reads
/proc/pid/attr/current with its read_one_line_file() helper which
ultimately uses read_line_full() and treats EOF, \n, \r, or \0 as
terminators and truncates on first such occurrence.
>
>>>> If this new API is an opportunity to declare that LSMs are expected
>>>> to put the same canonical form of a label in
>>>> /proc/$pid/attr/context and
>>>> SO_PEERCONTEXT, possibly with a non-canonical version (adding '\n' or
>>>> '\0' or similar) exposed in the older /proc/$pid/attr/current and
>>>> SO_PEERSEC interfaces for backwards compatibility, then that
>>>> would make
>>>> life a lot easier for user-space developers like me.
>>>
>>> I'm all for this but the current implementation reuses the same
>>> underlying hooks as SO_PEERSEC, so it gets the same result for the
>>> per-lsm values. We'd need a separate hook if we cannot alter the
>>> current AppArmor SO_PEERSEC format.
>
> If AppArmor was going to change the format of one of its interfaces
> (or deviate from it when implementing new interfaces), I'd actually
> prefer it to be /proc/pid/attr/current that changed or was superseded,
> because /proc/pid/attr/current is the one that contains a newline that
> consumers are meant to ignore.
>
> For what it's worth, libapparmor explicitly removes the newline, so this
> only matters to LSM-agnostic readers like D-Bus implementations, and to
> lower-level AppArmor-aware readers that use the kernel interfaces directly
> in preference to using libapparmor.
Deferring to the AA maintainer(s) to speak to this.
^ permalink raw reply
* Re: Looks like issue in handling active_nodes count in 4.19 kernel .
From: Paul Moore @ 2019-12-19 18:11 UTC (permalink / raw)
To: Ravi Kumar Siddojigari; +Cc: Stephen Smalley, selinux, linux-security-module
In-Reply-To: <002701d5b651$8434b2b0$8c9e1810$@codeaurora.org>
On Thu, Dec 19, 2019 at 4:48 AM Ravi Kumar Siddojigari
<rsiddoji@codeaurora.org> wrote:
>
> Sorry , Re-adding the patch below as requested.
>
> Stephen ,
> Issue is fixed with this 2 changes , Issue as even reproduced on v4.14 and similar changes work there also .
>
> --
> From 77c618006397c7a65ead257f3cb4e4fe3da2d4b8 Mon Sep 17 00:00:00 2001
> From: Jaihind Yadav <jaihindyadav@codeaurora.org>
> Date: Tue, 17 Dec 2019 17:25:47 +0530
> Subject: [PATCH] selinux: ensure we cleanup the internal AVC counters on error
> in avc_update()
>
> In AVC update we don't call avc_node_kill() when avc_xperms_populate()
> fails, resulting in the avc->avc_cache.active_nodes counter having a
> false value. In last patch this changes was missed , so correcting it.
>
> Change-Id: Ic0298162cc766c0f21be7ab232e259766654dad3
> Signed-off-by: Ravi Kumar Siddojigari <rsiddoji@codeaurora.org>
> ---
> security/selinux/avc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Two things:
* As Stephen already pointed out, please don't include "Change-Id"
metadata in your commit, that means nothing to the upstream kernel.
* If the patch is really from Jaihind Yadav then they should include
their sign-off, and preferably you would include your sign-off as well
since you are the one posting the patch. Please look at the
"Developer's Certificate of Origin" section in
Documentation/process/submitting-patches.rst.
--
paul moore
www.paul-moore.com
^ 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