* Re: [PATCH v4 19/23] LSM: Use lsmcontext in security_inode_getsecctx
From: John Johansen @ 2019-06-27 21:36 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20190626192234.11725-20-casey@schaufler-ca.com>
On 6/26/19 12:22 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. A lsmcontext is used within kernfs to store
> the security information as well.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: John Johansen <john.johansen@canonical.com>
> ---
> fs/kernfs/dir.c | 8 ++------
> fs/kernfs/inode.c | 34 ++++++++++++----------------------
> fs/kernfs/kernfs-internal.h | 3 +--
> fs/nfsd/nfs4xdr.c | 23 +++++++++--------------
> include/linux/security.h | 5 +++--
> security/security.c | 14 ++++++++++++--
> 6 files changed, 39 insertions(+), 48 deletions(-)
>
> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> index 92afad387237..1d000289d8b7 100644
> --- a/fs/kernfs/dir.c
> +++ b/fs/kernfs/dir.c
> @@ -532,12 +532,8 @@ void kernfs_put(struct kernfs_node *kn)
> kfree_const(kn->name);
>
> if (kn->iattr) {
> - struct lsmcontext scaff; /* scaffolding */
> - if (kn->iattr->ia_secdata) {
> - lsmcontext_init(&scaff, kn->iattr->ia_secdata,
> - kn->iattr->ia_secdata_len, 0);
> - security_release_secctx(&scaff);
> - }
> + if (kn->iattr->ia_context.context)
> + security_release_secctx(&kn->iattr->ia_context);
> simple_xattrs_free(&kn->iattr->xattrs);
> kmem_cache_free(kernfs_iattrs_cache, kn->iattr);
> }
> diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> index 02cde9dac5ee..ffbf7863306d 100644
> --- a/fs/kernfs/inode.c
> +++ b/fs/kernfs/inode.c
> @@ -135,21 +135,14 @@ int kernfs_iop_setattr(struct dentry *dentry, struct iattr *iattr)
> return error;
> }
>
> -static int kernfs_node_setsecdata(struct kernfs_iattrs *attrs, void **secdata,
> - u32 *secdata_len)
> +static void kernfs_node_setsecdata(struct kernfs_iattrs *attrs,
> + struct lsmcontext *cp)
> {
> - void *old_secdata;
> - size_t old_secdata_len;
> + struct lsmcontext old_context;
>
> - old_secdata = attrs->ia_secdata;
> - old_secdata_len = attrs->ia_secdata_len;
> -
> - attrs->ia_secdata = *secdata;
> - attrs->ia_secdata_len = *secdata_len;
> -
> - *secdata = old_secdata;
> - *secdata_len = old_secdata_len;
> - return 0;
> + old_context = attrs->ia_context;
> + attrs->ia_context = *cp;
> + *cp = old_context;
> }
>
> ssize_t kernfs_iop_listxattr(struct dentry *dentry, char *buf, size_t size)
> @@ -192,8 +185,8 @@ static void kernfs_refresh_inode(struct kernfs_node *kn, struct inode *inode)
> * persistent copy in kernfs_node.
> */
> set_inode_attr(inode, &attrs->ia_iattr);
> - security_inode_notifysecctx(inode, attrs->ia_secdata,
> - attrs->ia_secdata_len);
> + security_inode_notifysecctx(inode, attrs->ia_context.context,
> + attrs->ia_context.len);
> }
>
> if (kernfs_type(kn) == KERNFS_DIR)
> @@ -350,8 +343,6 @@ static int kernfs_security_xattr_set(const struct xattr_handler *handler,
> struct kernfs_node *kn = inode->i_private;
> struct kernfs_iattrs *attrs;
> struct lsmcontext context;
> - void *secdata;
> - u32 secdata_len = 0;
> int error;
>
> attrs = kernfs_iattrs(kn);
> @@ -361,18 +352,17 @@ static int kernfs_security_xattr_set(const struct xattr_handler *handler,
> error = security_inode_setsecurity(inode, suffix, value, size, flags);
> if (error)
> return error;
> - error = security_inode_getsecctx(inode, &secdata, &secdata_len);
> + error = security_inode_getsecctx(inode, &context);
> if (error)
> return error;
>
> mutex_lock(&kernfs_mutex);
> - error = kernfs_node_setsecdata(attrs, &secdata, &secdata_len);
> + kernfs_node_setsecdata(attrs, &context);
> mutex_unlock(&kernfs_mutex);
>
> - if (secdata) {
> - lsmcontext_init(&context, secdata, secdata_len, 0);
> + if (context.context)
> security_release_secctx(&context);
> - }
> +
> return error;
> }
>
> diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
> index 0b7d197a904c..844a028d282f 100644
> --- a/fs/kernfs/kernfs-internal.h
> +++ b/fs/kernfs/kernfs-internal.h
> @@ -21,8 +21,7 @@
>
> struct kernfs_iattrs {
> struct iattr ia_iattr;
> - void *ia_secdata;
> - u32 ia_secdata_len;
> + struct lsmcontext ia_context;
>
> struct simple_xattrs xattrs;
> };
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index bb3db033e144..1209083565dd 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
>
> @@ -2420,9 +2420,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
> __be32 status;
> int err;
> struct nfs4_acl *acl = NULL;
> - struct lsmcontext scaff; /* scaffolding */
> - void *context = NULL;
> - int contextlen;
> + struct lsmcontext context;
> bool contextsupport = false;
> struct nfsd4_compoundres *resp = rqstp->rq_resp;
> u32 minorversion = resp->cstate.minorversion;
> @@ -2479,7 +2477,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);
> @@ -2908,8 +2906,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
> }
>
> 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;
> }
> @@ -2920,10 +2917,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 2674eb70c2d7..c16aea55be97 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -489,7 +489,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);
> #else /* CONFIG_SECURITY */
>
> static inline int call_lsm_notifier(enum lsm_event event, void *data)
> @@ -1290,7 +1290,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 61cdc6bcd32e..45b9f905f5c0 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2164,9 +2164,19 @@ 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);
> + int *display = current->security;
> + struct security_hook_list *hp;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.inode_getsecctx, list)
> + if (*display == LSMBLOB_INVALID ||
> + *display == hp->lsmid->slot) {
> + 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 v4 20/23] LSM: security_secid_to_secctx in netlink netfilter
From: John Johansen @ 2019-06-27 21:37 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20190626192234.11725-21-casey@schaufler-ca.com>
On 6/26/19 12:22 PM, Casey Schaufler wrote:
> Change netlink netfilter interfaces to use lsmcontext
> pointers, and remove scaffolding.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: John Johansen <john.johansen@canonical.com>
> ---
> 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 6da00c7add5b..69efb688383f 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -305,12 +305,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;
> @@ -318,15 +316,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)
> @@ -402,8 +401,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))
> @@ -470,7 +468,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);
> }
> @@ -605,7 +603,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)
> @@ -633,10 +631,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:
> @@ -644,10 +640,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 v4 21/23] Audit: Store LSM audit information in an lsmblob
From: John Johansen @ 2019-06-27 21:37 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20190626192234.11725-22-casey@schaufler-ca.com>
On 6/26/19 12:22 PM, Casey Schaufler wrote:
> Change the audit code to store full lsmblob data instead of
> a single u32 secid. This allows for multiple security modules
> to use the audit system at the same time. It also allows the
> removal of scaffolding code that was included during the
> revision of LSM interfaces.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: John Johansen <john.johansen@canonical.com>
> ---
> kernel/audit.h | 6 +++---
> kernel/auditsc.c | 40 +++++++++++++---------------------------
> 2 files changed, 16 insertions(+), 30 deletions(-)
>
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 29e29c6f4afb..a8dd479e9556 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -91,7 +91,7 @@ struct audit_names {
> kuid_t uid;
> kgid_t gid;
> dev_t rdev;
> - u32 osid;
> + struct lsmblob olsm;
> struct audit_cap_data fcap;
> unsigned int fcap_ver;
> unsigned char type; /* record type */
> @@ -148,7 +148,7 @@ struct audit_context {
> kuid_t target_auid;
> kuid_t target_uid;
> unsigned int target_sessionid;
> - struct lsmblob target_lsm;
> + struct lsmblob target_lsm;
> char target_comm[TASK_COMM_LEN];
>
> struct audit_tree_refs *trees, *first_trees;
> @@ -165,7 +165,7 @@ struct audit_context {
> kuid_t uid;
> kgid_t gid;
> umode_t mode;
> - u32 osid;
> + struct lsmblob olsm;
> int has_perm;
> uid_t perm_uid;
> gid_t perm_gid;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 0478680cd0a8..ec8872430fb6 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -646,17 +646,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->olsm,
> 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->olsm,
> f->type,
> f->op,
> f->lsm_rule)) {
> @@ -668,8 +666,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.olsm,
> f->type, f->op,
> f->lsm_rule))
> ++result;
> @@ -955,7 +952,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> from_kuid(&init_user_ns, uid), sessionid);
> if (lsmblob_is_set(blob)) {
> if (security_secid_to_secctx(blob, &lsmctx)) {
> - audit_log_format(ab, " obj=(none)");
> + audit_log_format(ab, " obj=?");
> rc = 1;
> } else {
> audit_log_format(ab, " obj=%s", lsmctx.context);
> @@ -1187,19 +1184,17 @@ 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 *olsm = &context->ipc.olsm;
>
> 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) {
> + if (lsmblob_is_set(olsm)) {
> struct lsmcontext lsmcxt;
> - struct lsmblob blob;
>
> - lsmblob_init(&blob, osid);
> - if (security_secid_to_secctx(&blob, &lsmcxt)) {
> - audit_log_format(ab, " osid=%u", osid);
> + if (security_secid_to_secctx(olsm, &lsmcxt)) {
> + audit_log_format(ab, " obj=?");
> *call_panic = 1;
> } else {
> audit_log_format(ab, " obj=%s", lsmcxt.context);
> @@ -1346,13 +1341,11 @@ 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;
> + if (lsmblob_is_set(&n->olsm)) {
> struct lsmcontext lsmctx;
>
> - lsmblob_init(&blob, n->osid);
> - if (security_secid_to_secctx(&blob, &lsmctx)) {
> - audit_log_format(ab, " osid=%u", n->osid);
> + if (security_secid_to_secctx(&n->olsm, &lsmctx)) {
> + audit_log_format(ab, " obj=?");
> if (call_panic)
> *call_panic = 2;
> } else {
> @@ -1906,17 +1899,13 @@ static inline int audit_copy_fcaps(struct audit_names *name,
> 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->olsm);
> if (flags & AUDIT_INODE_NOEVAL) {
> name->fcap_ver = -1;
> return;
> @@ -2266,14 +2255,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.olsm);
> context->type = AUDIT_IPC;
> }
>
>
^ permalink raw reply
* Re: [PATCH v4 22/23] NET: Store LSM netlabel data in a lsmblob
From: John Johansen @ 2019-06-27 21:38 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20190626192234.11725-23-casey@schaufler-ca.com>
On 6/26/19 12:22 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 accomodate the change. This requires that the
> modules using netlabel use the lsm_id.slot to access the
> correct secid when using netlabel.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: John Johansen <john.johansen@canonical.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/include/net/netlabel.h b/include/net/netlabel.h
> index 72d6435fc16c..6c550455e69f 100644
> --- a/include/net/netlabel.h
> +++ b/include/net/netlabel.h
> @@ -180,7 +180,7 @@ struct netlbl_lsm_catmap {
> * @attr.mls: MLS sensitivity label
> * @attr.mls.cat: MLS category bitmap
> * @attr.mls.lvl: MLS sensitivity level
> - * @attr.secid: LSM specific secid token
> + * @attr.lsmblob: LSM specific data
> *
> * Description:
> * This structure is used to pass security attributes between NetLabel and the
> @@ -215,7 +215,7 @@ struct netlbl_lsm_secattr {
> struct netlbl_lsm_catmap *cat;
> u32 lvl;
> } mls;
> - u32 secid;
> + struct lsmblob lsmblob;
> } attr;
> };
>
> @@ -429,7 +429,7 @@ int netlbl_cfg_unlbl_static_add(struct net *net,
> const void *addr,
> const void *mask,
> u16 family,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info);
> int netlbl_cfg_unlbl_static_del(struct net *net,
> const char *dev_name,
> @@ -537,7 +537,7 @@ static inline int netlbl_cfg_unlbl_static_add(struct net *net,
> const void *addr,
> const void *mask,
> u16 family,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info)
> {
> return -ENOSYS;
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index f0165c5f376b..eb4939f38a14 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1481,7 +1481,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;
> }
> @@ -1501,7 +1502,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;
> diff --git a/net/netlabel/netlabel_kapi.c b/net/netlabel/netlabel_kapi.c
> index ee3e5b6471a6..724d44943543 100644
> --- a/net/netlabel/netlabel_kapi.c
> +++ b/net/netlabel/netlabel_kapi.c
> @@ -210,7 +210,7 @@ int netlbl_cfg_unlbl_map_add(const char *domain,
> * @addr: IP address in network byte order (struct in[6]_addr)
> * @mask: address mask in network byte order (struct in[6]_addr)
> * @family: address family
> - * @secid: LSM secid value for the entry
> + * @lsmblob: LSM data value for the entry
> * @audit_info: NetLabel audit information
> *
> * Description:
> @@ -224,7 +224,7 @@ int netlbl_cfg_unlbl_static_add(struct net *net,
> const void *addr,
> const void *mask,
> u16 family,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info)
> {
> u32 addr_len;
> @@ -244,7 +244,7 @@ int netlbl_cfg_unlbl_static_add(struct net *net,
>
> return netlbl_unlhsh_add(net,
> dev_name, addr, mask, addr_len,
> - secid, audit_info);
> + lsmblob, audit_info);
> }
>
> /**
> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index 4716e0011ba5..57ede7781c8f 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -80,7 +80,7 @@ struct netlbl_unlhsh_tbl {
> #define netlbl_unlhsh_addr4_entry(iter) \
> container_of(iter, struct netlbl_unlhsh_addr4, list)
> struct netlbl_unlhsh_addr4 {
> - u32 secid;
> + struct lsmblob lsmblob;
>
> struct netlbl_af4list list;
> struct rcu_head rcu;
> @@ -88,7 +88,7 @@ struct netlbl_unlhsh_addr4 {
> #define netlbl_unlhsh_addr6_entry(iter) \
> container_of(iter, struct netlbl_unlhsh_addr6, list)
> struct netlbl_unlhsh_addr6 {
> - u32 secid;
> + struct lsmblob lsmblob;
>
> struct netlbl_af6list list;
> struct rcu_head rcu;
> @@ -233,7 +233,7 @@ static struct netlbl_unlhsh_iface *netlbl_unlhsh_search_iface(int ifindex)
> * @iface: the associated interface entry
> * @addr: IPv4 address in network byte order
> * @mask: IPv4 address mask in network byte order
> - * @secid: LSM secid value for entry
> + * @lsmblob: LSM data value for entry
> *
> * Description:
> * Add a new address entry into the unlabeled connection hash table using the
> @@ -244,7 +244,7 @@ static struct netlbl_unlhsh_iface *netlbl_unlhsh_search_iface(int ifindex)
> static int netlbl_unlhsh_add_addr4(struct netlbl_unlhsh_iface *iface,
> const struct in_addr *addr,
> const struct in_addr *mask,
> - u32 secid)
> + struct lsmblob *lsmblob)
> {
> int ret_val;
> struct netlbl_unlhsh_addr4 *entry;
> @@ -256,7 +256,7 @@ static int netlbl_unlhsh_add_addr4(struct netlbl_unlhsh_iface *iface,
> entry->list.addr = addr->s_addr & mask->s_addr;
> entry->list.mask = mask->s_addr;
> entry->list.valid = 1;
> - entry->secid = secid;
> + entry->lsmblob = *lsmblob;
>
> spin_lock(&netlbl_unlhsh_lock);
> ret_val = netlbl_af4list_add(&entry->list, &iface->addr4_list);
> @@ -273,7 +273,7 @@ static int netlbl_unlhsh_add_addr4(struct netlbl_unlhsh_iface *iface,
> * @iface: the associated interface entry
> * @addr: IPv6 address in network byte order
> * @mask: IPv6 address mask in network byte order
> - * @secid: LSM secid value for entry
> + * @lsmblob: LSM data value for entry
> *
> * Description:
> * Add a new address entry into the unlabeled connection hash table using the
> @@ -284,7 +284,7 @@ static int netlbl_unlhsh_add_addr4(struct netlbl_unlhsh_iface *iface,
> static int netlbl_unlhsh_add_addr6(struct netlbl_unlhsh_iface *iface,
> const struct in6_addr *addr,
> const struct in6_addr *mask,
> - u32 secid)
> + struct lsmblob *lsmblob)
> {
> int ret_val;
> struct netlbl_unlhsh_addr6 *entry;
> @@ -300,7 +300,7 @@ static int netlbl_unlhsh_add_addr6(struct netlbl_unlhsh_iface *iface,
> entry->list.addr.s6_addr32[3] &= mask->s6_addr32[3];
> entry->list.mask = *mask;
> entry->list.valid = 1;
> - entry->secid = secid;
> + entry->lsmblob = *lsmblob;
>
> spin_lock(&netlbl_unlhsh_lock);
> ret_val = netlbl_af6list_add(&entry->list, &iface->addr6_list);
> @@ -379,7 +379,7 @@ int netlbl_unlhsh_add(struct net *net,
> const void *addr,
> const void *mask,
> u32 addr_len,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info)
> {
> int ret_val;
> @@ -388,7 +388,6 @@ int netlbl_unlhsh_add(struct net *net,
> struct netlbl_unlhsh_iface *iface;
> struct audit_buffer *audit_buf = NULL;
> struct lsmcontext context;
> - struct lsmblob blob;
>
> if (addr_len != sizeof(struct in_addr) &&
> addr_len != sizeof(struct in6_addr))
> @@ -421,7 +420,7 @@ int netlbl_unlhsh_add(struct net *net,
> const struct in_addr *addr4 = addr;
> const struct in_addr *mask4 = mask;
>
> - ret_val = netlbl_unlhsh_add_addr4(iface, addr4, mask4, secid);
> + ret_val = netlbl_unlhsh_add_addr4(iface, addr4, mask4, lsmblob);
> if (audit_buf != NULL)
> netlbl_af4list_audit_addr(audit_buf, 1,
> dev_name,
> @@ -434,7 +433,7 @@ int netlbl_unlhsh_add(struct net *net,
> const struct in6_addr *addr6 = addr;
> const struct in6_addr *mask6 = mask;
>
> - ret_val = netlbl_unlhsh_add_addr6(iface, addr6, mask6, secid);
> + ret_val = netlbl_unlhsh_add_addr6(iface, addr6, mask6, lsmblob);
> if (audit_buf != NULL)
> netlbl_af6list_audit_addr(audit_buf, 1,
> dev_name,
> @@ -451,8 +450,7 @@ int netlbl_unlhsh_add(struct net *net,
> unlhsh_add_return:
> rcu_read_unlock();
> if (audit_buf != NULL) {
> - lsmblob_init(&blob, secid);
> - if (security_secid_to_secctx(&blob, &context) == 0) {
> + if (security_secid_to_secctx(lsmblob, &context) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s",
> context.context);
> security_release_secctx(&context);
> @@ -487,7 +485,6 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> struct lsmcontext context;
> - struct lsmblob blob;
>
> spin_lock(&netlbl_unlhsh_lock);
> list_entry = netlbl_af4list_remove(addr->s_addr, mask->s_addr,
> @@ -507,10 +504,8 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> addr->s_addr, mask->s_addr);
> if (dev != NULL)
> dev_put(dev);
> - if (entry != NULL)
> - lsmblob_init(&blob, entry->secid);
> if (entry != NULL &&
> - security_secid_to_secctx(&blob, &context) == 0) {
> + security_secid_to_secctx(&entry->lsmblob, &context) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s",
> context.context);
> security_release_secctx(&context);
> @@ -551,7 +546,6 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> struct lsmcontext context;
> - struct lsmblob blob;
>
> spin_lock(&netlbl_unlhsh_lock);
> list_entry = netlbl_af6list_remove(addr, mask, &iface->addr6_list);
> @@ -570,10 +564,8 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> addr, mask);
> if (dev != NULL)
> dev_put(dev);
> - if (entry != NULL)
> - lsmblob_init(&blob, entry->secid);
> if (entry != NULL &&
> - security_secid_to_secctx(&blob, &context) == 0) {
> + security_secid_to_secctx(&entry->lsmblob, &context) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s",
> context.context);
> security_release_secctx(&context);
> @@ -927,9 +919,8 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb,
> if (ret_val != 0)
> return ret_val;
>
> - /* scaffolding with the [0] */
> return netlbl_unlhsh_add(&init_net,
> - dev_name, addr, mask, addr_len, blob.secid[0],
> + dev_name, addr, mask, addr_len, &blob,
> &audit_info);
> }
>
> @@ -977,10 +968,8 @@ static int netlbl_unlabel_staticadddef(struct sk_buff *skb,
> if (ret_val != 0)
> return ret_val;
>
> - /* scaffolding with the [0] */
> return netlbl_unlhsh_add(&init_net,
> - NULL, addr, mask, addr_len, blob.secid[0],
> - &audit_info);
> + NULL, addr, mask, addr_len, &blob, &audit_info);
> }
>
> /**
> @@ -1092,8 +1081,7 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> struct net_device *dev;
> struct lsmcontext context;
> void *data;
> - u32 secid;
> - struct lsmblob blob;
> + struct lsmblob *lsmb;
>
> data = genlmsg_put(cb_arg->skb, NETLINK_CB(cb_arg->nl_cb->skb).portid,
> cb_arg->seq, &netlbl_unlabel_gnl_family,
> @@ -1131,7 +1119,7 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> if (ret_val != 0)
> goto list_cb_failure;
>
> - secid = addr4->secid;
> + lsmb = (struct lsmblob *)&addr4->lsmblob;
> } else {
> ret_val = nla_put_in6_addr(cb_arg->skb,
> NLBL_UNLABEL_A_IPV6ADDR,
> @@ -1145,11 +1133,10 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> if (ret_val != 0)
> goto list_cb_failure;
>
> - secid = addr6->secid;
> + lsmb = (struct lsmblob *)&addr6->lsmblob;
> }
>
> - lsmblob_init(&blob, secid);
> - ret_val = security_secid_to_secctx(&blob, &context);
> + ret_val = security_secid_to_secctx(lsmb, &context);
> if (ret_val != 0)
> goto list_cb_failure;
> ret_val = nla_put(cb_arg->skb,
> @@ -1500,7 +1487,7 @@ int netlbl_unlabel_getattr(const struct sk_buff *skb,
> &iface->addr4_list);
> if (addr4 == NULL)
> goto unlabel_getattr_nolabel;
> - secattr->attr.secid = netlbl_unlhsh_addr4_entry(addr4)->secid;
> + secattr->attr.lsmblob = netlbl_unlhsh_addr4_entry(addr4)->lsmblob;
> break;
> }
> #if IS_ENABLED(CONFIG_IPV6)
> @@ -1513,7 +1500,7 @@ int netlbl_unlabel_getattr(const struct sk_buff *skb,
> &iface->addr6_list);
> if (addr6 == NULL)
> goto unlabel_getattr_nolabel;
> - secattr->attr.secid = netlbl_unlhsh_addr6_entry(addr6)->secid;
> + secattr->attr.lsmblob = netlbl_unlhsh_addr6_entry(addr6)->lsmblob;
> break;
> }
> #endif /* IPv6 */
> diff --git a/net/netlabel/netlabel_unlabeled.h b/net/netlabel/netlabel_unlabeled.h
> index 3a9e5dc9511b..dcff99695c97 100644
> --- a/net/netlabel/netlabel_unlabeled.h
> +++ b/net/netlabel/netlabel_unlabeled.h
> @@ -225,7 +225,7 @@ int netlbl_unlhsh_add(struct net *net,
> const void *addr,
> const void *mask,
> u32 addr_len,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info);
> int netlbl_unlhsh_remove(struct net *net,
> const char *dev_name,
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 8c93b07bb353..d90dfce15d44 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6622,7 +6622,7 @@ struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
> .lbs_superblock = sizeof(struct superblock_security_struct),
> };
>
> -static struct lsm_id selinux_lsmid = { .lsm="selinux", .slot=LSMBLOB_NEEDED };
> +struct lsm_id selinux_lsmid = { .lsm="selinux", .slot=LSMBLOB_NEEDED };
>
> static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index b5b7c5aade8c..f0ca3879ba48 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -68,6 +68,7 @@
> struct netlbl_lsm_secattr;
>
> extern int selinux_enabled;
> +extern struct lsm_id selinux_lsmid;
>
> /* Policy capabilities */
> enum {
> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> index c40914a157b7..120d50c1bcac 100644
> --- a/security/selinux/netlabel.c
> +++ b/security/selinux/netlabel.c
> @@ -122,7 +122,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;
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index e3f5d6aece66..4ca0e006c3cc 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -3593,7 +3593,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);
> @@ -3666,7 +3666,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);
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index 7cc3a3382fee..039bf5de56b4 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -320,6 +320,7 @@ void smk_destroy_label_list(struct list_head *list);
> * Shared data.
> */
> extern int smack_enabled;
> +extern struct lsm_id smack_lsmid;
> extern int smack_cipso_direct;
> extern int smack_cipso_mapped;
> extern struct smack_known *smack_net_ambient;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 3d571c438dfa..a8d56ce00918 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3741,7 +3741,8 @@ static struct smack_known *smack_from_secattr(struct netlbl_lsm_secattr *sap,
> /*
> * Looks like a fallback, which gives us a secid.
> */
> - return smack_from_secid(sap->attr.secid);
> + return smack_from_secid(
> + sap->attr.lsmblob.secid[smack_lsmid.slot]);
> /*
> * Without guidance regarding the smack value
> * for the packet fall back on the network
> @@ -4558,7 +4559,7 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
> .lbs_superblock = sizeof(struct superblock_smack),
> };
>
> -static struct lsm_id smack_lsmid = { .lsm="smack", .slot=LSMBLOB_NEEDED };
> +struct lsm_id smack_lsmid = { .lsm="smack", .slot=LSMBLOB_NEEDED };
>
> static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(ptrace_access_check, smack_ptrace_access_check),
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index faf2ea3968b3..6a4c468c200c 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -1150,6 +1150,7 @@ static void smk_net4addr_insert(struct smk_net4addr *new)
> static ssize_t smk_write_net4addr(struct file *file, const char __user *buf,
> size_t count, loff_t *ppos)
> {
> + struct lsmblob lsmblob;
> struct smk_net4addr *snp;
> struct sockaddr_in newname;
> char *smack;
> @@ -1281,10 +1282,13 @@ static ssize_t smk_write_net4addr(struct file *file, const char __user *buf,
> * this host so that incoming packets get labeled.
> * but only if we didn't get the special CIPSO option
> */
> - if (rc == 0 && skp != NULL)
> + if (rc == 0 && skp != NULL) {
> + lsmblob_init(&lsmblob, 0);
> + lsmblob.secid[smack_lsmid.slot] = snp->smk_label->smk_secid;
> rc = netlbl_cfg_unlbl_static_add(&init_net, NULL,
> - &snp->smk_host, &snp->smk_mask, PF_INET,
> - snp->smk_label->smk_secid, &audit_info);
> + &snp->smk_host, &snp->smk_mask, PF_INET, &lsmblob,
> + &audit_info);
> + }
>
> if (rc == 0)
> rc = count;
>
^ permalink raw reply
* Re: [PATCH v4 23/23] AppArmor: Remove the exclusive flag
From: John Johansen @ 2019-06-27 21:38 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20190626192234.11725-24-casey@schaufler-ca.com>
On 6/26/19 12:22 PM, Casey Schaufler wrote:
> With the inclusion of the "display" process attribute
> mechanism AppArmor no longer needs to be treated as an
> "exclusive" security module. Remove the flag that indicates
> it is exclusive. Remove the stub getpeersec_dgram AppArmor
> hook as it has no effect in the single LSM case and
> interferes in the multiple LSM case.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: John Johansen <john.johansen@canonical.com>
> ---
> security/apparmor/lsm.c | 20 +-------------------
> 1 file changed, 1 insertion(+), 19 deletions(-)
>
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 6d2eefc9b7c1..fb5798683ae1 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1079,22 +1079,6 @@ static int apparmor_socket_getpeersec_stream(struct socket *sock,
> return error;
> }
>
> -/**
> - * apparmor_socket_getpeersec_dgram - get security label of packet
> - * @sock: the peer socket
> - * @skb: packet data
> - * @secid: pointer to where to put the secid of the packet
> - *
> - * Sets the netlabel socket state on sk from parent
> - */
> -static int apparmor_socket_getpeersec_dgram(struct socket *sock,
> - struct sk_buff *skb, u32 *secid)
> -
> -{
> - /* TODO: requires secid support */
> - return -ENOPROTOOPT;
> -}
> -
> /**
> * apparmor_sock_graft - Initialize newly created socket
> * @sk: child sock
> @@ -1195,8 +1179,6 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
> #endif
> LSM_HOOK_INIT(socket_getpeersec_stream,
> apparmor_socket_getpeersec_stream),
> - LSM_HOOK_INIT(socket_getpeersec_dgram,
> - apparmor_socket_getpeersec_dgram),
> LSM_HOOK_INIT(sock_graft, apparmor_sock_graft),
> #ifdef CONFIG_NETWORK_SECMARK
> LSM_HOOK_INIT(inet_conn_request, apparmor_inet_conn_request),
> @@ -1707,7 +1689,7 @@ static int __init apparmor_init(void)
>
> DEFINE_LSM(apparmor) = {
> .name = "apparmor",
> - .flags = LSM_FLAG_LEGACY_MAJOR | LSM_FLAG_EXCLUSIVE,
> + .flags = LSM_FLAG_LEGACY_MAJOR,
> .enabled = &apparmor_enabled,
> .blobs = &apparmor_blob_sizes,
> .init = apparmor_init,
>
^ permalink raw reply
* Re: [PATCH v4 00/23] LSM: Module stacking for AppArmor
From: John Johansen @ 2019-06-27 21:52 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20190626192234.11725-1-casey@schaufler-ca.com>
On 6/26/19 12:22 PM, Casey Schaufler wrote:
> This patchset provides the changes required for
> the AppArmor security module to stack safely with any other.
>
I have been doing some testing of this with Casey's suggested
fix of clearing the lsmcontext in security_secid_to_secctx().
So far things are looking good. I have done smoke testing
on booting with the following combinations under an ubuntu
image. For the combinations that have apparmor I ran the
apparmor regression tests, where noted the display LSM
was set for the apparmor regression tests because they
are currently only testing the shared interface.
capability
yama
capability,yama
capability,yama,apparmor
capability,yama,selinux (no selinux policy)
capability,yama,apparmor,selinux (no selinux policy)
capability,yama,selinux,apparmor (no selinux policy) (tests that use shared interfaces fail without display LSM set, pass with it set to apparmor)
capability,yama,smack (no smack policy)
capability,yama,apparmor,smack (no smack policy)
capability,yama,smack,apparmor (no smack policy) (tests that use shared interfaces fail without display LSM set, pass with it set to apparmor)
I have more test combinations churning but figure I could report what I have so far
^ permalink raw reply
* Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Casey Schaufler @ 2019-06-27 22:06 UTC (permalink / raw)
To: Cedric Xing, linux-sgx, linux-security-module, selinux
Cc: casey.schaufler, jmorris, luto, jethro, greg, sds,
jarkko.sakkinen, sean.j.christopherson, casey
In-Reply-To: <72420cff8fa944b64e57df8d25c63bd30f8aacfa.1561588012.git.cedric.xing@intel.com>
On 6/27/2019 11:56 AM, Cedric Xing wrote:
> SGX enclaves are loaded from pages in regular memory. Given the ability to
> create executable pages, the newly added SGX subsystem may present a backdoor
> for adversaries to circumvent LSM policies, such as creating an executable
> enclave page from a modified regular page that would otherwise not be made
> executable as prohibited by LSM. Therefore arises the primary question of
> whether an enclave page should be allowed to be created from a given source
> page in regular memory.
>
> A related question is whether to grant/deny a mprotect() request on a given
> enclave page/range. mprotect() is traditionally covered by
> security_file_mprotect() hook, however, enclave pages have a different lifespan
> than either MAP_PRIVATE or MAP_SHARED. Particularly, MAP_PRIVATE pages have the
> same lifespan as the VMA while MAP_SHARED pages have the same lifespan as the
> backing file (on disk), but enclave pages have the lifespan of the enclave’s
> file descriptor. For example, enclave pages could be munmap()’ed then mmap()’ed
> again without losing contents (like MAP_SHARED), but all enclave pages will be
> lost once its file descriptor has been closed (like MAP_PRIVATE). That said,
> LSM modules need some new data structure for tracking protections of enclave
> pages/ranges so that they can make proper decisions at mmap()/mprotect()
> syscalls.
>
> The last question, which is orthogonal to the 2 above, is whether or not to
> allow a given enclave to launch/run. Enclave pages are not visible to the rest
> of the system, so to some extent offer a better place for malicious software to
> hide. Thus, it is sometimes desirable to whitelist/blacklist enclaves by their
> measurements, signing public keys, or image files.
>
> To address the questions above, 2 new LSM hooks are added for enclaves.
> - security_enclave_load() – This hook allows LSM to decide whether or not to
> allow instantiation of a range of enclave pages using the specified VMA. It
> is invoked when a range of enclave pages is about to be loaded. It serves 3
> purposes: 1) indicate to LSM that the file struct in subject is an enclave;
> 2) allow LSM to decide whether or not to instantiate those pages and 3)
> allow LSM to initialize internal data structures for tracking
> origins/protections of those pages.
> - security_enclave_init() – This hook allows whitelisting/blacklisting or
> performing whatever checks deemed appropriate before an enclave is allowed
> to run. An LSM module may opt to use the file backing the SIGSTRUCT as a
> proxy to dictate allowed protections for anonymous pages.
>
> mprotect() of enclave pages continue to be governed by
> security_file_mprotect(), with the expectation that LSM is able to distinguish
> between regular and enclave pages inside the hook. For mmap(), the SGX
> subsystem is expected to invoke security_file_mprotect() explicitly to check
> protections against the requested protections for existing enclave pages. As
> stated earlier, enclave pages have different lifespan than the existing
> MAP_PRIVATE and MAP_SHARED pages, so would require a new data structure outside
> of VMA to track their protections and/or origins. Enclave Memory Area (or EMA
> for short) has been introduced to address the need. EMAs are maintained by the
> LSM framework for all LSM modules to share. EMAs will be instantiated for
> enclaves only so will not impose memory/performance overheads for regular
> applications/files. Please see include/linux/lsm_ema.h and security/lsm_ema.c
> for details.
>
> A new setup parameter – lsm.ema.cache_decisions has been introduced to offer
> the choice between memory consumption and accuracy of audit logs. Enabling
> lsm.ema.cache_decisions causes LSM framework NOT to keep backing files open for
> EMAs. While that saves memory, it requires LSM modules to make and cache
> decisions ahead of time, and makes it difficult for LSM modules to generate
> accurate audit logs. System administrators are expected to run LSM in
> permissive mode with lsm.ema.cache_decisions off to determine the minimal
> permissions needed, and then turn it back on in enforcing mode for optimal
> performance and memory usage. lsm.ema.cache_decisions is on by default and
> could be turned off by appending “lsm.ema.cache_decisions=0” or
> “lsm.ema.cache_decisions=off” to the kernel command line.
>
> Signed-off-by: Cedric Xing <cedric.xing@intel.com>
> ---
> include/linux/lsm_ema.h | 171 ++++++++++++++++++++++++++++++++++++++
> include/linux/lsm_hooks.h | 29 +++++++
> include/linux/security.h | 23 +++++
> security/Makefile | 1 +
> security/lsm_ema.c | 132 +++++++++++++++++++++++++++++
> security/security.c | 47 ++++++++++-
> 6 files changed, 402 insertions(+), 1 deletion(-)
> create mode 100644 include/linux/lsm_ema.h
> create mode 100644 security/lsm_ema.c
Don't use "lsm_ema". This isn't LSM infrastructure.
Three letter abbreviations are easy to type, but are
doomed to encounter conflicts and lead to confusion.
I suggest that you use "enclave", because it doesn't
start off conflicting with anything and is descriptive.
This code should not be mixed in with the LSM infrastructure.
It should all be contained in its own module, under
security/enclave.
> diff --git a/include/linux/lsm_ema.h b/include/linux/lsm_ema.h
> new file mode 100644
> index 000000000000..a09b8f96da05
> --- /dev/null
> +++ b/include/linux/lsm_ema.h
There's no need for this header to be used outside the enclave
LSM. It should be "security/enclave/enclave.h"
> @@ -0,0 +1,171 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
> +/**
> + * Enclave Memory Area interface for LSM modules
> + *
> + * Copyright(c) 2016-19 Intel Corporation.
> + */
> +
> +#ifndef _LSM_EMA_H_
> +#define _LSM_EMA_H_
> +
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/fs.h>
> +#include <linux/file.h>
> +
> +/**
> + * lsm_ema - LSM Enclave Memory Area structure
How about s/lsm_ema/enclave/ ?
> + *
> + * Data structure to track origins of enclave pages
> + *
> + * @link:
> + * Link to adjacent EMAs. EMAs are sorted by their addresses in ascending
> + * order
> + * @start:
> + * Starting address
> + * @end:
> + * Ending address
> + * @source:
> + * File from which this range was loaded from, or NULL if not loaded from
> + * any files
> + */
> +struct lsm_ema {
> + struct list_head link;
> + size_t start;
> + size_t end;
> + struct file *source;
> +};
> +
> +#define lsm_ema_data(ema, blob_sizes) \
> + ((char *)((struct lsm_ema *)(ema) + 1) + blob_sizes.lbs_ema_data)
Who uses this? The enclave LSM? Convention would have this
be selinux_enclave(ema) for the SELinux code. This is
inconsistent with the way other blobs are handled.
> +
> +/**
> + * lsm_ema_map - LSM Enclave Memory Map structure
enclave_map
> + *
> + * Container for EMAs of an enclave
> + *
> + * @list:
> + * Head of a list of sorted EMAs
> + * @lock:
> + * Acquire before querying/updateing the list EMAs
> + */
> +struct lsm_ema_map {
> + struct list_head list;
> + struct mutex lock;
> +};
> +
> +/**
> + * These are functions to be used by the LSM framework, and must be defined
> + * regardless CONFIG_INTEL_SGX is enabled or not.
Not acceptable for the LSM infrastructure. They
are inconsistent with the way data is used there.
> + */
> +
> +#ifdef CONFIG_INTEL_SGX
> +void lsm_ema_global_init(size_t);
> +void lsm_free_ema_map(atomic_long_t *);
> +#else
> +static inline void lsm_ema_global_init(size_t ema_data_size)
> +{
> +}
> +
> +static inline void lsm_free_ema_map(atomic_long_t *p)
> +{
> +}
> +#endif
> +
> +/**
> + * Below are APIs to be used by LSM modules
> + */
> +
> +struct lsm_ema_map *lsm_init_or_get_ema_map(atomic_long_t *);
> +struct lsm_ema *lsm_alloc_ema(void);
Do you mean security_alloc_enclave()?
That would go into security/security.h
> +void lsm_free_ema(struct lsm_ema *);
Do you mean security_free_enclave()?
That would go into security/security.h
> +void lsm_init_ema(struct lsm_ema *, size_t, size_t, struct file *);
This goes in the enclave LSM.
> +int lsm_merge_ema(struct lsm_ema *, struct lsm_ema_map *);
> +struct lsm_ema *lsm_split_ema(struct lsm_ema *, size_t, struct lsm_ema_map *);
> +
> +static inline struct lsm_ema_map *lsm_get_ema_map(struct file *f)
> +{
> + return (void *)atomic_long_read(f->f_security);
> +}
> +
> +static inline int __must_check lsm_lock_ema(struct lsm_ema_map *map)
> +{
> + return mutex_lock_interruptible(&map->lock);
> +}
> +
> +static inline void lsm_unlock_ema(struct lsm_ema_map *map)
> +{
> + mutex_unlock(&map->lock);
> +}
> +
> +static inline struct lsm_ema *lsm_prev_ema(struct lsm_ema *p,
> + struct lsm_ema_map *map)
> +{
> + p = list_prev_entry(p, link);
> + return &p->link == &map->list ? NULL : p;
> +}
> +
> +static inline struct lsm_ema *lsm_next_ema(struct lsm_ema *p,
> + struct lsm_ema_map *map)
> +{
> + p = list_next_entry(p, link);
> + return &p->link == &map->list ? NULL : p;
> +}
> +
> +static inline struct lsm_ema *lsm_find_ema(struct lsm_ema_map *map, size_t a)
> +{
> + struct lsm_ema *p;
> +
> + BUG_ON(!mutex_is_locked(&map->lock));
> +
> + list_for_each_entry(p, &map->list, link)
> + if (a < p->end)
> + break;
> + return &p->link == &map->list ? NULL : p;
> +}
> +
> +static inline int lsm_insert_ema(struct lsm_ema_map *map, struct lsm_ema *n)
> +{
> + struct lsm_ema *p = lsm_find_ema(map, n->start);
> +
> + if (!p)
> + list_add_tail(&n->link, &map->list);
> + else if (n->end <= p->start)
> + list_add_tail(&n->link, &p->link);
> + else
> + return -EEXIST;
> +
> + lsm_merge_ema(n, map);
> + if (p)
> + lsm_merge_ema(p, map);
> + return 0;
> +}
> +
> +static inline int lsm_for_each_ema(struct lsm_ema_map *map, size_t start,
> + size_t end, int (*cb)(struct lsm_ema *,
> + void *), void *arg)
> +{
> + struct lsm_ema *ema;
> + int rc;
> +
> + ema = lsm_find_ema(map, start);
> + while (ema && end > ema->start) {
> + if (start > ema->start)
> + lsm_split_ema(ema, start, map);
> + if (end < ema->end)
> + ema = lsm_split_ema(ema, end, map);
> +
> + rc = (*cb)(ema, arg);
> + lsm_merge_ema(ema, map);
> + if (rc)
> + return rc;
> +
> + ema = lsm_next_ema(ema, map);
> + }
> +
> + if (ema)
> + lsm_merge_ema(ema, map);
> + return 0;
> +}
There is no way that these belong as part of the LSM
infrastructure. If you need an enclave management API
you need to find some other place for it.
> +
> +#endif /* _LSM_EMA_H_ */
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 47f58cfb6a19..ade1f9f81e64 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -29,6 +29,8 @@
> #include <linux/init.h>
> #include <linux/rculist.h>
>
> +struct lsm_ema;
> +
> /**
> * union security_list_options - Linux Security Module hook function list
> *
> @@ -1446,6 +1448,21 @@
> * @bpf_prog_free_security:
> * Clean up the security information stored inside bpf prog.
> *
> + * @enclave_load:
> + * Decide if a range of pages shall be allowed to be loaded into an
> + * enclave
> + *
> + * @encl points to the file identifying the target enclave
> + * @ema specifies the target range to be loaded
> + * @flags contains protections being requested for the target range
> + * @source points to the VMA containing the source pages to be loaded
> + *
> + * @enclave_init:
> + * Decide if an enclave shall be allowed to launch
> + *
> + * @encl points to the file identifying the target enclave being launched
> + * @sigstruct contains a copy of the SIGSTRUCT in kernel memory
> + * @source points to the VMA backing SIGSTRUCT in user memory
> */
> union security_list_options {
> int (*binder_set_context_mgr)(struct task_struct *mgr);
> @@ -1807,6 +1824,13 @@ union security_list_options {
> int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
> void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
> #endif /* CONFIG_BPF_SYSCALL */
> +
> +#ifdef CONFIG_INTEL_SGX
> + int (*enclave_load)(struct file *encl, struct lsm_ema *ema,
> + size_t flags, struct vm_area_struct *source);
> + int (*enclave_init)(struct file *encl, struct sgx_sigstruct *sigstruct,
> + struct vm_area_struct *source);
> +#endif
> };
>
> struct security_hook_heads {
> @@ -2046,6 +2070,10 @@ struct security_hook_heads {
> struct hlist_head bpf_prog_alloc_security;
> struct hlist_head bpf_prog_free_security;
> #endif /* CONFIG_BPF_SYSCALL */
> +#ifdef CONFIG_INTEL_SGX
> + struct hlist_head enclave_load;
> + struct hlist_head enclave_init;
> +#endif
> } __randomize_layout;
>
> /*
> @@ -2069,6 +2097,7 @@ struct lsm_blob_sizes {
> int lbs_ipc;
> int lbs_msg_msg;
> int lbs_task;
> + int lbs_ema_data;
Is a module like SELinux expected to have its own
data for enclave? That's the only case where you would
have a enclave entry in the blob.
> };
>
> /*
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 659071c2e57c..52c200810004 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1829,5 +1829,28 @@ static inline void security_bpf_prog_free(struct bpf_prog_aux *aux)
> #endif /* CONFIG_SECURITY */
> #endif /* CONFIG_BPF_SYSCALL */
>
> +#ifdef CONFIG_INTEL_SGX
> +struct sgx_sigstruct;
> +#ifdef CONFIG_SECURITY
> +int security_enclave_load(struct file *encl, size_t start, size_t end,
> + size_t flags, struct vm_area_struct *source);
> +int security_enclave_init(struct file *encl, struct sgx_sigstruct *sigstruct,
> + struct vm_area_struct *source);
> +#else
> +static inline int security_enclave_load(struct file *encl, size_t start,
> + size_t end, struct vm_area_struct *src)
> +{
> + return 0;
> +}
> +
> +static inline int security_enclave_init(struct file *encl,
> + struct sgx_sigstruct *sigstruct,
> + struct vm_area_struct *src)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_SECURITY */
> +#endif /* CONFIG_INTEL_SGX */
> +
> #endif /* ! __LINUX_SECURITY_H */
>
> diff --git a/security/Makefile b/security/Makefile
> index c598b904938f..1bab8f1344b6 100644
> --- a/security/Makefile
> +++ b/security/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
> obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
> obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
> obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
> +obj-$(CONFIG_INTEL_SGX) += lsm_ema.o
This belongs in a subdirectory.
>
> # Object integrity file lists
> subdir-$(CONFIG_INTEGRITY) += integrity
> diff --git a/security/lsm_ema.c b/security/lsm_ema.c
> new file mode 100644
> index 000000000000..68fae0724d37
> --- /dev/null
> +++ b/security/lsm_ema.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +// Copyright(c) 2016-18 Intel Corporation.
> +
> +#include <linux/lsm_ema.h>
> +#include <linux/slab.h>
> +
> +static struct kmem_cache *lsm_ema_cache;
> +static size_t lsm_ema_data_size;
> +static int lsm_ema_cache_decisions = 1;
> +
> +void lsm_ema_global_init(size_t ema_size)
> +{
> + BUG_ON(lsm_ema_data_size > 0);
> +
> + lsm_ema_data_size = ema_size;
> +
> + ema_size += sizeof(struct lsm_ema);
> + ema_size = max(ema_size, sizeof(struct lsm_ema_map));
> + lsm_ema_cache = kmem_cache_create("lsm_ema_cache", ema_size,
> + __alignof__(struct lsm_ema),
> + SLAB_PANIC, NULL);
> +
> +}
> +
> +struct lsm_ema_map *lsm_init_or_get_ema_map(atomic_long_t *p)
> +{
> + struct lsm_ema_map *map;
> +
> + map = (typeof(map))atomic_long_read(p);
> + if (!map) {
> + long n;
> +
> + map = (typeof(map))lsm_alloc_ema();
> + if (!map)
> + return NULL;
> +
> + INIT_LIST_HEAD(&map->list);
> + mutex_init(&map->lock);
> +
> + n = atomic_long_cmpxchg(p, 0, (long)map);
> + if (n) {
> + atomic_long_t a;
> + atomic_long_set(&a, (long)map);
> + map = (typeof(map))n;
> + lsm_free_ema_map(&a);
> + }
> + }
> + return map;
> +}
> +
> +void lsm_free_ema_map(atomic_long_t *p)
> +{
> + struct lsm_ema_map *map;
> + struct lsm_ema *ema, *n;
> +
> + map = (typeof(map))atomic_long_read(p);
> + if (!map)
> + return;
> +
> + BUG_ON(mutex_is_locked(&map->lock));
> +
> + list_for_each_entry_safe(ema, n, &map->list, link)
> + lsm_free_ema(ema);
> + kmem_cache_free(lsm_ema_cache, map);
> +}
> +
> +struct lsm_ema *lsm_alloc_ema(void)
> +{
> + return kmem_cache_zalloc(lsm_ema_cache, GFP_KERNEL);
> +}
> +
> +void lsm_free_ema(struct lsm_ema *ema)
> +{
> + list_del(&ema->link);
> + if (ema->source)
> + fput(ema->source);
> + kmem_cache_free(lsm_ema_cache, ema);
> +}
> +
> +void lsm_init_ema(struct lsm_ema *ema, size_t start, size_t end,
> + struct file *source)
> +{
> + INIT_LIST_HEAD(&ema->link);
> + ema->start = start;
> + ema->end = end;
> + if (!lsm_ema_cache_decisions && source)
> + ema->source = get_file(source);
> +}
> +
> +int lsm_merge_ema(struct lsm_ema *p, struct lsm_ema_map *map)
> +{
> + struct lsm_ema *prev = list_prev_entry(p, link);
> +
> + BUG_ON(!mutex_is_locked(&map->lock));
> +
> + if (&prev->link == &map->list || prev->end != p->start ||
> + prev->source != p->source ||
> + memcmp(prev + 1, p + 1, lsm_ema_data_size))
> + return 0;
> +
> + p->start = prev->start;
> + fput(prev->source);
> + lsm_free_ema(prev);
> + return 1;
> +}
> +
> +struct lsm_ema *lsm_split_ema(struct lsm_ema *p, size_t at,
> + struct lsm_ema_map *map)
> +{
> + struct lsm_ema *n;
> +
> + BUG_ON(!mutex_is_locked(&map->lock));
> +
> + if (at <= p->start || at >= p->end)
> + return p;
> +
> + n = lsm_alloc_ema();
> + if (likely(n)) {
> + lsm_init_ema(n, p->start, at, p->source);
> + memcpy(n + 1, p + 1, lsm_ema_data_size);
> + p->start = at;
> + list_add_tail(&n->link, &p->link);
> + }
> + return n;
> +}
> +
> +static int __init set_ema_cache_decisions(char *str)
> +{
> + lsm_ema_cache_decisions = (strcmp(str, "0") && strcmp(str, "off"));
> + return 1;
> +}
> +__setup("lsm.ema.cache_decisions=", set_ema_cache_decisions);
> diff --git a/security/security.c b/security/security.c
> index f493db0bf62a..d50883f18be2 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -17,6 +17,7 @@
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/lsm_hooks.h>
> +#include <linux/lsm_ema.h>
> #include <linux/integrity.h>
> #include <linux/ima.h>
> #include <linux/evm.h>
> @@ -41,7 +42,9 @@ 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;
> +static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init = {
> + .lbs_file = sizeof(atomic_long_t) * IS_ENABLED(CONFIG_INTEL_SGX),
> +};
This belongs in the module specific code.
It does not belong here.
>
> /* Boot-time LSM user choice */
> static __initdata const char *chosen_lsm_order;
> @@ -169,6 +172,7 @@ static void __init lsm_set_blob_sizes(struct lsm_blob_sizes *needed)
> lsm_set_blob_size(&needed->lbs_ipc, &blob_sizes.lbs_ipc);
> lsm_set_blob_size(&needed->lbs_msg_msg, &blob_sizes.lbs_msg_msg);
> lsm_set_blob_size(&needed->lbs_task, &blob_sizes.lbs_task);
> + lsm_set_blob_size(&needed->lbs_ema_data, &blob_sizes.lbs_ema_data);
> }
>
> /* Prepare LSM for initialization. */
> @@ -314,6 +318,7 @@ static void __init ordered_lsm_init(void)
> lsm_inode_cache = kmem_cache_create("lsm_inode_cache",
> blob_sizes.lbs_inode, 0,
> SLAB_PANIC, NULL);
> + lsm_ema_global_init(blob_sizes.lbs_ema_data);
>
> lsm_early_cred((struct cred *) current->cred);
> lsm_early_task(current);
> @@ -1357,6 +1362,7 @@ void security_file_free(struct file *file)
> blob = file->f_security;
> if (blob) {
> file->f_security = NULL;
> + lsm_free_ema_map(blob);
> kmem_cache_free(lsm_file_cache, blob);
> }
> }
> @@ -1420,6 +1426,7 @@ int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
> {
> return call_int_hook(file_mprotect, 0, vma, reqprot, prot);
> }
> +EXPORT_SYMBOL(security_file_mprotect);
>
> int security_file_lock(struct file *file, unsigned int cmd)
> {
> @@ -2355,3 +2362,41 @@ void security_bpf_prog_free(struct bpf_prog_aux *aux)
> call_void_hook(bpf_prog_free_security, aux);
> }
> #endif /* CONFIG_BPF_SYSCALL */
> +
> +#ifdef CONFIG_INTEL_SGX
> +int security_enclave_load(struct file *encl, size_t start, size_t end,
> + size_t flags, struct vm_area_struct *src)
You are mixing module specific code into the infrastructure.
All of this should be in the enclave code. None of it should be here.
> +{
> + struct lsm_ema_map *map;
> + struct lsm_ema *ema;
> + int rc;
> +
> + map = lsm_init_or_get_ema_map(encl->f_security);
> + if (unlikely(!map))
> + return -ENOMEM;
> +
> + ema = lsm_alloc_ema();
> + if (unlikely(!ema))
> + return -ENOMEM;
> +
> + lsm_init_ema(ema, start, end, src->vm_file);
> + rc = call_int_hook(enclave_load, 0, encl, ema, flags, src);
> + if (!rc)
> + rc = lsm_lock_ema(map);
> + if (!rc) {
> + rc = lsm_insert_ema(map, ema);
> + lsm_unlock_ema(map);
> + }
> + if (rc)
> + lsm_free_ema(ema);
> + return rc;
> +}
> +EXPORT_SYMBOL(security_enclave_load);
> +
> +int security_enclave_init(struct file *encl, struct sgx_sigstruct *sigstruct,
> + struct vm_area_struct *src)
> +{
> + return call_int_hook(enclave_init, 0, encl, sigstruct, src);
> +}
> +EXPORT_SYMBOL(security_enclave_init);
> +#endif /* CONFIG_INTEL_SGX */
^ permalink raw reply
* Re: [PATCH 8/9] keys: Network namespace domain tag [ver #4]
From: David Howells @ 2019-06-27 22:07 UTC (permalink / raw)
To: Willem de Bruijn
Cc: dhowells, Eric W. Biederman, keyrings, linux-cifs, linux-nfs,
Network Development, linux-afs, dwalsh, vgoyal,
linux-security-module, linux-fsdevel, LKML
In-Reply-To: <CAF=yD-Kgdwt5=0iboxhvZz4zvNewSGow74U15mQQvO1u8VUGcw@mail.gmail.com>
Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> > +#ifdef CONFIG_KEYS
> > +out_free_2:
> > + kmem_cache_free(net_cachep, net);
>
> needs
> net = NULL;
>
> to signal failure
>
> > +#endif
I've folded that into the patch and retagged, remerged and repushed.
David
^ permalink raw reply
* Re: [PATCH v4 00/23] LSM: Module stacking for AppArmor
From: Casey Schaufler @ 2019-06-27 22:33 UTC (permalink / raw)
To: John Johansen, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <f5552cb8-1d6c-eb07-be4d-c85e0722c1fa@canonical.com>
On 6/27/2019 2:52 PM, John Johansen wrote:
> On 6/26/19 12:22 PM, Casey Schaufler wrote:
>> This patchset provides the changes required for
>> the AppArmor security module to stack safely with any other.
>>
> I have been doing some testing of this with Casey's suggested
> fix of clearing the lsmcontext in security_secid_to_secctx().
There are still cases where the lsmcontext needs local
initialization. If security_<fillscontext> isn't called,
and code later looks for context.context == NULL you can
get bitten. I am combing for those cases and will include
initializing them in v5.
>
> So far things are looking good. I have done smoke testing
> on booting with the following combinations under an ubuntu
> image. For the combinations that have apparmor I ran the
> apparmor regression tests, where noted the display LSM
> was set for the apparmor regression tests because they
> are currently only testing the shared interface.
>
> capability
> yama
> capability,yama
> capability,yama,apparmor
> capability,yama,selinux (no selinux policy)
> capability,yama,apparmor,selinux (no selinux policy)
> capability,yama,selinux,apparmor (no selinux policy) (tests that use shared interfaces fail without display LSM set, pass with it set to apparmor)
> capability,yama,smack (no smack policy)
> capability,yama,apparmor,smack (no smack policy)
> capability,yama,smack,apparmor (no smack policy) (tests that use shared interfaces fail without display LSM set, pass with it set to apparmor)
>
>
> I have more test combinations churning but figure I could report what I have so far
>
>
^ permalink raw reply
* RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Xing, Cedric @ 2019-06-27 22:52 UTC (permalink / raw)
To: Casey Schaufler, linux-sgx@vger.kernel.org,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org
Cc: Schaufler, Casey, jmorris@namei.org, luto@kernel.org,
jethro@fortanix.com, greg@enjellic.com, sds@tycho.nsa.gov,
jarkko.sakkinen@linux.intel.com, Christopherson, Sean J
In-Reply-To: <b08798a5-65f7-f96e-1c04-39dd0e60c114@schaufler-ca.com>
Hi Casey,
> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
> Sent: Thursday, June 27, 2019 3:07 PM
>
> Don't use "lsm_ema". This isn't LSM infrastructure.
> Three letter abbreviations are easy to type, but are doomed to encounter
> conflicts and lead to confusion.
> I suggest that you use "enclave", because it doesn't start off
> conflicting with anything and is descriptive.
>
> This code should not be mixed in with the LSM infrastructure.
> It should all be contained in its own module, under security/enclave.
lsm_ema is *intended* to be part of the LSM infrastructure. It is going to be shared among all LSMs that would like to track enclave pages and their origins. And they could be extended to store more information as deemed appropriate by the LSM module. The last patch of this series shows how to extend EMA inside SELinux.
>
> > diff --git a/include/linux/lsm_ema.h b/include/linux/lsm_ema.h new
> > file mode 100644 index 000000000000..a09b8f96da05
> > --- /dev/null
> > +++ b/include/linux/lsm_ema.h
>
> There's no need for this header to be used outside the enclave
> LSM. It should be "security/enclave/enclave.h"
This header file is supposed to be used by all LSM modules, similar to lsm_hooks.h. Hence it is placed in the same location.
>
>
> > @@ -0,0 +1,171 @@
> > +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
> > +/**
> > + * Enclave Memory Area interface for LSM modules
> > + *
> > + * Copyright(c) 2016-19 Intel Corporation.
> > + */
> > +
> > +#ifndef _LSM_EMA_H_
> > +#define _LSM_EMA_H_
> > +
> > +#include <linux/list.h>
> > +#include <linux/mutex.h>
> > +#include <linux/fs.h>
> > +#include <linux/file.h>
> > +
> > +/**
> > + * lsm_ema - LSM Enclave Memory Area structure
>
> How about s/lsm_ema/enclave/ ?
I understand your suggestion, but this structure is shared among all LSMs. And I think lsm_ema is pretty descriptive without being too verbose.
>
> > + *
> > + * Data structure to track origins of enclave pages
> > + *
> > + * @link:
> > + * Link to adjacent EMAs. EMAs are sorted by their addresses in
> ascending
> > + * order
> > + * @start:
> > + * Starting address
> > + * @end:
> > + * Ending address
> > + * @source:
> > + * File from which this range was loaded from, or NULL if not loaded
> from
> > + * any files
> > + */
> > +struct lsm_ema {
> > + struct list_head link;
> > + size_t start;
> > + size_t end;
> > + struct file *source;
> > +};
> > +
> > +#define lsm_ema_data(ema, blob_sizes) \
> > + ((char *)((struct lsm_ema *)(ema) + 1) + blob_sizes.lbs_ema_data)
>
> Who uses this? The enclave LSM? Convention would have this
> be selinux_enclave(ema) for the SELinux code. This is
> inconsistent with the way other blobs are handled.
This is to be used in various LSMs. As you can see in the last patch of this series, selinux_ema() is defined as a wrapper of this macro.
>
> > +
> > +/**
> > + * lsm_ema_map - LSM Enclave Memory Map structure
>
> enclave_map
>
> > + *
> > + * Container for EMAs of an enclave
> > + *
> > + * @list:
> > + * Head of a list of sorted EMAs
> > + * @lock:
> > + * Acquire before querying/updateing the list EMAs
> > + */
> > +struct lsm_ema_map {
> > + struct list_head list;
> > + struct mutex lock;
> > +};
> > +
> > +/**
> > + * These are functions to be used by the LSM framework, and must be
> defined
> > + * regardless CONFIG_INTEL_SGX is enabled or not.
>
> Not acceptable for the LSM infrastructure. They
> are inconsistent with the way data is used there.
I'm not sure I understand this comment.
>
> > + */
> > +
> > +#ifdef CONFIG_INTEL_SGX
> > +void lsm_ema_global_init(size_t);
> > +void lsm_free_ema_map(atomic_long_t *);
> > +#else
> > +static inline void lsm_ema_global_init(size_t ema_data_size)
> > +{
> > +}
> > +
> > +static inline void lsm_free_ema_map(atomic_long_t *p)
> > +{
> > +}
> > +#endif
> > +
> > +/**
> > + * Below are APIs to be used by LSM modules
> > + */
> > +
> > +struct lsm_ema_map *lsm_init_or_get_ema_map(atomic_long_t *);
> > +struct lsm_ema *lsm_alloc_ema(void);
>
> Do you mean security_alloc_enclave()?
> That would go into security/security.h
No. Neither lsm_alloc_ema() above, nor lsm_free_ema() below, is LSM hook. They are APIs to deal with EMAs.
>
> > +void lsm_free_ema(struct lsm_ema *);
>
> Do you mean security_free_enclave()?
> That would go into security/security.h
>
> > +void lsm_init_ema(struct lsm_ema *, size_t, size_t, struct file *);
>
> This goes in the enclave LSM.
There's NO enclave LSM. This patch is introducing new LSM hooks applicable to all LSM modules, but not introducing new LSM modules.
>
> > +int lsm_merge_ema(struct lsm_ema *, struct lsm_ema_map *);
> > +struct lsm_ema *lsm_split_ema(struct lsm_ema *, size_t, struct
> lsm_ema_map *);
> > +
> > +static inline struct lsm_ema_map *lsm_get_ema_map(struct file *f)
> > +{
> > + return (void *)atomic_long_read(f->f_security);
> > +}
> > +
> > +static inline int __must_check lsm_lock_ema(struct lsm_ema_map *map)
> > +{
> > + return mutex_lock_interruptible(&map->lock);
> > +}
> > +
> > +static inline void lsm_unlock_ema(struct lsm_ema_map *map)
> > +{
> > + mutex_unlock(&map->lock);
> > +}
> > +
> > +static inline struct lsm_ema *lsm_prev_ema(struct lsm_ema *p,
> > + struct lsm_ema_map *map)
> > +{
> > + p = list_prev_entry(p, link);
> > + return &p->link == &map->list ? NULL : p;
> > +}
> > +
> > +static inline struct lsm_ema *lsm_next_ema(struct lsm_ema *p,
> > + struct lsm_ema_map *map)
> > +{
> > + p = list_next_entry(p, link);
> > + return &p->link == &map->list ? NULL : p;
> > +}
> > +
> > +static inline struct lsm_ema *lsm_find_ema(struct lsm_ema_map *map,
> size_t a)
> > +{
> > + struct lsm_ema *p;
> > +
> > + BUG_ON(!mutex_is_locked(&map->lock));
> > +
> > + list_for_each_entry(p, &map->list, link)
> > + if (a < p->end)
> > + break;
> > + return &p->link == &map->list ? NULL : p;
> > +}
> > +
> > +static inline int lsm_insert_ema(struct lsm_ema_map *map, struct
> lsm_ema *n)
> > +{
> > + struct lsm_ema *p = lsm_find_ema(map, n->start);
> > +
> > + if (!p)
> > + list_add_tail(&n->link, &map->list);
> > + else if (n->end <= p->start)
> > + list_add_tail(&n->link, &p->link);
> > + else
> > + return -EEXIST;
> > +
> > + lsm_merge_ema(n, map);
> > + if (p)
> > + lsm_merge_ema(p, map);
> > + return 0;
> > +}
> > +
> > +static inline int lsm_for_each_ema(struct lsm_ema_map *map, size_t
> start,
> > + size_t end, int (*cb)(struct lsm_ema *,
> > + void *), void *arg)
> > +{
> > + struct lsm_ema *ema;
> > + int rc;
> > +
> > + ema = lsm_find_ema(map, start);
> > + while (ema && end > ema->start) {
> > + if (start > ema->start)
> > + lsm_split_ema(ema, start, map);
> > + if (end < ema->end)
> > + ema = lsm_split_ema(ema, end, map);
> > +
> > + rc = (*cb)(ema, arg);
> > + lsm_merge_ema(ema, map);
> > + if (rc)
> > + return rc;
> > +
> > + ema = lsm_next_ema(ema, map);
> > + }
> > +
> > + if (ema)
> > + lsm_merge_ema(ema, map);
> > + return 0;
> > +}
>
> There is no way that these belong as part of the LSM
> infrastructure. If you need an enclave management API
> you need to find some other place for it.
They are NO enclave management APIs. They don't manage enclaves. They track origins of enclave pages. They are needed by all LSMs.
As I stated in the cover letter, the primary question is how to prevent SGX from being abused as a backdoor to make executable pages that would otherwise not be executable without SGX. Any LSM module unaware of that would leave that "hole" open. So tracking enclave pages will become a common task for all LSMs that care page protections, and that's why I place it inside LSM infrastructure.
^ permalink raw reply
* Re: [PATCH V33 24/30] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: Matthew Garrett @ 2019-06-27 23:16 UTC (permalink / raw)
To: Stephen Smalley
Cc: James Morris, Andy Lutomirski, Andy Lutomirski, linux-security,
LKML, Linux API, David Howells, Alexei Starovoitov,
Network Development, Chun-Yi Lee, Daniel Borkmann, LSM List
In-Reply-To: <de8b15eb-ba6c-847a-7435-42742203d4a5@tycho.nsa.gov>
On Thu, Jun 27, 2019 at 1:16 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> That would only allow the LSM to further lock down the system above the
> lockdown level set at boot, not grant exemptions for specific
> functionality/interfaces required by the user or by a specific
> process/program. You'd have to boot with lockdown=none (or your
> lockdown=custom suggestion) in order for the LSM to allow anything
> covered by the integrity or confidentiality levels. And then the kernel
> would be unprotected prior to full initialization of the LSM, including
> policy load.
>
> It seems like one would want to be able to boot with lockdown=integrity
> to protect the kernel initially, then switch over to allowing the LSM to
> selectively override it.
One option would be to allow modules to be "unstacked" at runtime, but
there's still something of a problem here - how do you ensure that
your userland can be trusted to load a new policy before it does so?
If you're able to assert that your early userland is trustworthy
(perhaps because it's in an initramfs that's part of your signed boot
payload), there's maybe an argument that most of the lockdown
integrity guarantees are unnecessary before handoff - just using the
lockdown LSM to protect against attacks via kernel parameters would be
sufficient.
^ permalink raw reply
* Re: [PATCH v4 00/23] LSM: Module stacking for AppArmor
From: James Morris @ 2019-06-27 23:16 UTC (permalink / raw)
To: John Johansen
Cc: Casey Schaufler, casey.schaufler, linux-security-module, selinux,
keescook, penguin-kernel, paul, sds
In-Reply-To: <f5552cb8-1d6c-eb07-be4d-c85e0722c1fa@canonical.com>
On Thu, 27 Jun 2019, John Johansen wrote:
> I have more test combinations churning but figure I could report what I have so far
Do you have any way to test the nested scenario of say an AppArmor host
with SELinux running in containers?
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH V34 09/29] kexec_file: Restrict at runtime if the kernel is locked down
From: Matthew Garrett @ 2019-06-27 23:17 UTC (permalink / raw)
To: James Morris
Cc: LSM List, Linux Kernel Mailing List, Linux API, Jiri Bohac,
David Howells, kexec
In-Reply-To: <alpine.LRH.2.21.1906280411370.18880@namei.org>
On Thu, Jun 27, 2019 at 11:14 AM James Morris <jmorris@namei.org> wrote:
>
> On Thu, 27 Jun 2019, Matthew Garrett wrote:
>
> > By that metric, on a secure boot system how do we determine that code
> > running in the firmware environment wasn't compromised before it
> > launched the initial signed kernel?
>
> Remote attestation tied to a hardware root of trust, before allowing
> access to any further resources.
If you use IMA you can get the same guarantees over kexec.
^ permalink raw reply
* Re: [PATCH V33 24/30] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: Andy Lutomirski @ 2019-06-27 23:23 UTC (permalink / raw)
To: Matthew Garrett
Cc: Stephen Smalley, James Morris, Andy Lutomirski, linux-security,
LKML, Linux API, David Howells, Alexei Starovoitov,
Network Development, Chun-Yi Lee, Daniel Borkmann, LSM List
In-Reply-To: <CACdnJuuG8cR7h9v3pNcBKsxyckAzpKuBJs1GQxsz77jk5DRoQA@mail.gmail.com>
On Thu, Jun 27, 2019 at 4:16 PM Matthew Garrett <mjg59@google.com> wrote:
>
> On Thu, Jun 27, 2019 at 1:16 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > That would only allow the LSM to further lock down the system above the
> > lockdown level set at boot, not grant exemptions for specific
> > functionality/interfaces required by the user or by a specific
> > process/program. You'd have to boot with lockdown=none (or your
> > lockdown=custom suggestion) in order for the LSM to allow anything
> > covered by the integrity or confidentiality levels. And then the kernel
> > would be unprotected prior to full initialization of the LSM, including
> > policy load.
> >
> > It seems like one would want to be able to boot with lockdown=integrity
> > to protect the kernel initially, then switch over to allowing the LSM to
> > selectively override it.
>
> One option would be to allow modules to be "unstacked" at runtime, but
> there's still something of a problem here - how do you ensure that
> your userland can be trusted to load a new policy before it does so?
> If you're able to assert that your early userland is trustworthy
> (perhaps because it's in an initramfs that's part of your signed boot
> payload), there's maybe an argument that most of the lockdown
> integrity guarantees are unnecessary before handoff - just using the
> lockdown LSM to protect against attacks via kernel parameters would be
> sufficient.
I think that, if you don't trust your system enough to avoid
compromising itself before policy load, then your MAC policy is more
or less dead in the water. It seems to be that it ought to be good
enough to boot with lockdown=none and then have a real policy loaded
along with the rest of the MAC policy. Or, for applications that need
to be stricter, you accept that MAC policy can't override lockdown.
^ permalink raw reply
* [PATCH] ima: Update MAX_TEMPLATE_NAME_LEN to fit largest reasonable definition
From: Thiago Jung Bauermann @ 2019-06-27 23:25 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, linux-kernel, Mimi Zohar,
Thiago Jung Bauermann
MAX_TEMPLATE_NAME_LEN is used when restoring measurements carried over from
a kexec. It should be set to the length of a template containing all fields
except for 'd' and 'n', which don't need to be accounted for since they
shouldn't be defined in the same template description as 'd-ng' and 'n-ng'.
That length is greater than the current 15, so update using a sizeof() to
show where the number comes from and also can be visually shown to be
correct. The sizeof() is calculated at compile time.
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
security/integrity/ima/ima_template.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index a01a17e5c581..7343e8e0ae2f 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -47,7 +47,13 @@ static const struct ima_template_field supported_fields[] = {
{.field_id = "buf", .field_init = ima_eventbuf_init,
.field_show = ima_show_template_buf},
};
-#define MAX_TEMPLATE_NAME_LEN 15
+
+/*
+ * Used when restoring measurements carried over from a kexec. 'd' and 'n' don't
+ * need to be accounted for since they shouldn't be defined in the same template
+ * description as 'd-ng' and 'n-ng' respectively.
+ */
+#define MAX_TEMPLATE_NAME_LEN sizeof("d-ng|n-ng|sig|buf")
static struct ima_template_desc *ima_template;
^ permalink raw reply related
* Re: [PATCH V33 24/30] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: Andy Lutomirski @ 2019-06-27 23:27 UTC (permalink / raw)
To: Stephen Smalley
Cc: James Morris, Andy Lutomirski, Matthew Garrett, linux-security,
LKML, Linux API, David Howells, Alexei Starovoitov,
Matthew Garrett, Network Development, Chun-Yi Lee,
Daniel Borkmann, LSM List
In-Reply-To: <bce70c8b-9efd-6362-d536-cfbbcf70b0b7@tycho.nsa.gov>
On Thu, Jun 27, 2019 at 7:35 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> On 6/26/19 8:57 PM, Andy Lutomirski wrote:
> >
> >
> >> On Jun 26, 2019, at 1:22 PM, James Morris <jmorris@namei.org> wrote:
> >>
> >> [Adding the LSM mailing list: missed this patchset initially]
> >>
> >>> On Thu, 20 Jun 2019, Andy Lutomirski wrote:
> >>>
> >>> This patch exemplifies why I don't like this approach:
> >>>
> >>>> @@ -97,6 +97,7 @@ enum lockdown_reason {
> >>>> LOCKDOWN_INTEGRITY_MAX,
> >>>> LOCKDOWN_KCORE,
> >>>> LOCKDOWN_KPROBES,
> >>>> + LOCKDOWN_BPF,
> >>>> LOCKDOWN_CONFIDENTIALITY_MAX,
> >>>
> >>>> --- a/security/lockdown/lockdown.c
> >>>> +++ b/security/lockdown/lockdown.c
> >>>> @@ -33,6 +33,7 @@ static char *lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
> >>>> [LOCKDOWN_INTEGRITY_MAX] = "integrity",
> >>>> [LOCKDOWN_KCORE] = "/proc/kcore access",
> >>>> [LOCKDOWN_KPROBES] = "use of kprobes",
> >>>> + [LOCKDOWN_BPF] = "use of bpf",
> >>>> [LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality",
> >>>
> >>> The text here says "use of bpf", but what this patch is *really* doing
> >>> is locking down use of BPF to read kernel memory. If the details
> >>> change, then every LSM needs to get updated, and we risk breaking user
> >>> policies that are based on LSMs that offer excessively fine
> >>> granularity.
> >>
> >> Can you give an example of how the details might change?
> >>
> >>> I'd be more comfortable if the LSM only got to see "confidentiality"
> >>> or "integrity".
> >>
> >> These are not sufficient for creating a useful policy for the SELinux
> >> case.
> >>
> >>
> >
> > I may have misunderstood, but I thought that SELinux mainly needed to allow certain privileged programs to bypass the policy. Is there a real example of what SELinux wants to do that can’t be done in the simplified model?
> >
> > The think that specifically makes me uneasy about exposing all of these precise actions to LSMs is that they will get exposed to userspace in a way that forces us to treat them as stable ABIs.
>
> There are two scenarios where finer-grained distinctions make sense:
>
> - Users may need to enable specific functionality that falls under the
> umbrella of "confidentiality" or "integrity" lockdown. Finer-grained
> lockdown reasons free them from having to make an all-or-nothing choice
> between lost functionality or no lockdown at all. This can be supported
> directly by the lockdown module without any help from SELinux or other
> security modules; we just need the ability to specify these
> finer-grained lockdown levels via the boot parameters and securityfs nodes.
>
> - Different processes/programs may need to use different sets of
> functionality restricted via lockdown confidentiality or integrity
> categories. If we have to allow all-or-none for the set of
> interfaces/functionality covered by the generic confidentiality or
> integrity categories, then we'll end up having to choose between lost
> functionality or overprivileged processes, neither of which is optimal.
>
> Is it truly the case that everything under the "confidentiality"
> category poses the same level of risk to kernel confidentiality, and
> similarly for everything under the "integrity" category? If not, then
> being able to distinguish them definitely has benefit.
>
They're really quite similar in my mind. Certainly some things in the
"integrity" category give absolutely trivial control over the kernel
(e.g. modules) while others make it quite challenging (ioperm), but
the end result is very similar. And quite a few "confidentiality"
things genuinely do allow all kernel memory to be read.
I agree that finer-grained distinctions could be useful. My concern is
that it's a tradeoff, and the other end of the tradeoff is an ABI
stability issue. If someone decides down the road that some feature
that is currently "integrity" can be split into a narrow "integrity"
feature and a "confidentiality" feature then, if the user policy knows
about the individual features, there's a risk of breaking people's
systems. If we keep the fine-grained control, do we have a clear
compatibility story?
^ permalink raw reply
* Re: [PATCH V10 2/3] IMA: Define a new template field buf
From: Thiago Jung Bauermann @ 2019-06-27 23:27 UTC (permalink / raw)
To: Mimi Zohar
Cc: Prakhar Srivastava, linux-integrity, linux-security-module,
linux-kernel, roberto.sassu, vgoyal
In-Reply-To: <1561648111.4101.135.camel@linux.ibm.com>
Mimi Zohar <zohar@linux.ibm.com> writes:
> On Mon, 2019-06-24 at 19:03 -0300, Thiago Jung Bauermann wrote:
>> Hello Prakhar,
>>
>> Prakhar Srivastava <prsriva02@gmail.com> writes:
>>
>> > diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
>> > index 00dd5a434689..a01a17e5c581 100644
>> > --- a/security/integrity/ima/ima_template.c
>> > +++ b/security/integrity/ima/ima_template.c
>> > @@ -26,6 +26,7 @@ static struct ima_template_desc builtin_templates[] = {
>> > {.name = IMA_TEMPLATE_IMA_NAME, .fmt = IMA_TEMPLATE_IMA_FMT},
>> > {.name = "ima-ng", .fmt = "d-ng|n-ng"},
>> > {.name = "ima-sig", .fmt = "d-ng|n-ng|sig"},
>> > + {.name = "ima-buf", .fmt = "d-ng|n-ng|buf"},
>> > {.name = "", .fmt = ""}, /* placeholder for a custom format */
>> > };
>> >
>> > @@ -43,6 +44,8 @@ static const struct ima_template_field supported_fields[] = {
>> > .field_show = ima_show_template_string},
>> > {.field_id = "sig", .field_init = ima_eventsig_init,
>> > .field_show = ima_show_template_sig},
>> > + {.field_id = "buf", .field_init = ima_eventbuf_init,
>> > + .field_show = ima_show_template_buf},
>> > };
>> > #define MAX_TEMPLATE_NAME_LEN 15
>>
>> Currently, MAX_TEMPLATE_NAME_LEN is the length of a template that
>> contains all valid fields. It may make sense to increase it since
>> there's a new field being added.
>>
>> I suggest using a sizeof() to show where the number comes from (and
>> which can be visually shown to be correct):
>>
>> #define MAX_TEMPLATE_NAME_LEN sizeof("d|n|d-ng|n-ng|sig|buf")
>>
>> The sizeof() is calculated at compile time.
>
> MAX_TEMPLATE_NAME_LEN is used when restoring measurements carried over
> from a kexec. 'd' and 'd-ng' should not both be defined in the
> template description, nor should 'n' and 'n-ng'.
Ah, makes sense. Thanks for that information.
> Even without the
> duplication, the MAX_TEPLATE_NAME_LEN is greater than the current 15.
>
> Thiago, could you address this as a separate patch?
Yes, I just sent a patch.
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Casey Schaufler @ 2019-06-27 23:37 UTC (permalink / raw)
To: Xing, Cedric, linux-sgx@vger.kernel.org,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
casey
Cc: Schaufler, Casey, jmorris@namei.org, luto@kernel.org,
jethro@fortanix.com, greg@enjellic.com, sds@tycho.nsa.gov,
jarkko.sakkinen@linux.intel.com, Christopherson, Sean J
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F6551B8D7@ORSMSX116.amr.corp.intel.com>
On 6/27/2019 3:52 PM, Xing, Cedric wrote:
> Hi Casey,
>
>> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
>> Sent: Thursday, June 27, 2019 3:07 PM
>>
>> Don't use "lsm_ema". This isn't LSM infrastructure.
>> Three letter abbreviations are easy to type, but are doomed to encounter
>> conflicts and lead to confusion.
>> I suggest that you use "enclave", because it doesn't start off
>> conflicting with anything and is descriptive.
>>
>> This code should not be mixed in with the LSM infrastructure.
>> It should all be contained in its own module, under security/enclave.
> lsm_ema is *intended* to be part of the LSM infrastructure.
That's not going to fly, not for a minute.
> It is going to be shared among all LSMs that would like to track enclave pages and their origins.
That's true for InfiniBand, tun and sctp as well. Look at their implementations.
> And they could be extended to store more information as deemed appropriate by the LSM module.
Which is what blobs are for, but that does not appear to be how
you're using either the file blob or your new ema blob.
> The last patch of this series shows how to extend EMA inside SELinux.
I don't see (but I admit the code doesn't make a lot of sense to me)
anything you couldn't do in the SELinux code by adding data to the
file blob. The data you're adding to the LSM infrastructure doesn't
belong there, and it doesn't need to be there.
>
>>> diff --git a/include/linux/lsm_ema.h b/include/linux/lsm_ema.h new
>>> file mode 100644 index 000000000000..a09b8f96da05
>>> --- /dev/null
>>> +++ b/include/linux/lsm_ema.h
>> There's no need for this header to be used outside the enclave
>> LSM. It should be "security/enclave/enclave.h"
> This header file is supposed to be used by all LSM modules, similar to lsm_hooks.h. Hence it is placed in the same location.
>
>>
>>> @@ -0,0 +1,171 @@
>>> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
>>> +/**
>>> + * Enclave Memory Area interface for LSM modules
>>> + *
>>> + * Copyright(c) 2016-19 Intel Corporation.
>>> + */
>>> +
>>> +#ifndef _LSM_EMA_H_
>>> +#define _LSM_EMA_H_
>>> +
>>> +#include <linux/list.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/fs.h>
>>> +#include <linux/file.h>
>>> +
>>> +/**
>>> + * lsm_ema - LSM Enclave Memory Area structure
>> How about s/lsm_ema/enclave/ ?
> I understand your suggestion, but this structure is shared among all LSMs. And I think lsm_ema is pretty descriptive without being too verbose.
>
>>> + *
>>> + * Data structure to track origins of enclave pages
>>> + *
>>> + * @link:
>>> + * Link to adjacent EMAs. EMAs are sorted by their addresses in
>> ascending
>>> + * order
>>> + * @start:
>>> + * Starting address
>>> + * @end:
>>> + * Ending address
>>> + * @source:
>>> + * File from which this range was loaded from, or NULL if not loaded
>> from
>>> + * any files
>>> + */
>>> +struct lsm_ema {
>>> + struct list_head link;
>>> + size_t start;
>>> + size_t end;
>>> + struct file *source;
>>> +};
>>> +
>>> +#define lsm_ema_data(ema, blob_sizes) \
>>> + ((char *)((struct lsm_ema *)(ema) + 1) + blob_sizes.lbs_ema_data)
>> Who uses this? The enclave LSM? Convention would have this
>> be selinux_enclave(ema) for the SELinux code. This is
>> inconsistent with the way other blobs are handled.
> This is to be used in various LSMs. As you can see in the last patch of this series, selinux_ema() is defined as a wrapper of this macro.
>
>>> +
>>> +/**
>>> + * lsm_ema_map - LSM Enclave Memory Map structure
>> enclave_map
>>
>>> + *
>>> + * Container for EMAs of an enclave
>>> + *
>>> + * @list:
>>> + * Head of a list of sorted EMAs
>>> + * @lock:
>>> + * Acquire before querying/updateing the list EMAs
>>> + */
>>> +struct lsm_ema_map {
>>> + struct list_head list;
>>> + struct mutex lock;
>>> +};
>>> +
>>> +/**
>>> + * These are functions to be used by the LSM framework, and must be
>> defined
>>> + * regardless CONFIG_INTEL_SGX is enabled or not.
>> Not acceptable for the LSM infrastructure. They
>> are inconsistent with the way data is used there.
> I'm not sure I understand this comment.
It means that your definition and use of the lsm_ema_blob
does not match the way other blobs are managed and used.
The LSM infrastructure uses these entries in a very particular
way, and you're trying to use it differently. Your might be
able to change the rest of the enclave system to use it
correctly, or you might be able to find a different place
for it.
>>> + */
>>> +
>>> +#ifdef CONFIG_INTEL_SGX
>>> +void lsm_ema_global_init(size_t);
>>> +void lsm_free_ema_map(atomic_long_t *);
>>> +#else
>>> +static inline void lsm_ema_global_init(size_t ema_data_size)
>>> +{
>>> +}
>>> +
>>> +static inline void lsm_free_ema_map(atomic_long_t *p)
>>> +{
>>> +}
>>> +#endif
>>> +
>>> +/**
>>> + * Below are APIs to be used by LSM modules
>>> + */
>>> +
>>> +struct lsm_ema_map *lsm_init_or_get_ema_map(atomic_long_t *);
>>> +struct lsm_ema *lsm_alloc_ema(void);
>> Do you mean security_alloc_enclave()?
>> That would go into security/security.h
> No. Neither lsm_alloc_ema() above, nor lsm_free_ema() below, is LSM hook. They are APIs to deal with EMAs.
>
>>> +void lsm_free_ema(struct lsm_ema *);
>> Do you mean security_free_enclave()?
>> That would go into security/security.h
>>
>>> +void lsm_init_ema(struct lsm_ema *, size_t, size_t, struct file *);
>> This goes in the enclave LSM.
> There's NO enclave LSM. This patch is introducing new LSM hooks applicable to all LSM modules, but not introducing new LSM modules.
>
>>> +int lsm_merge_ema(struct lsm_ema *, struct lsm_ema_map *);
>>> +struct lsm_ema *lsm_split_ema(struct lsm_ema *, size_t, struct
>> lsm_ema_map *);
>>> +
>>> +static inline struct lsm_ema_map *lsm_get_ema_map(struct file *f)
>>> +{
>>> + return (void *)atomic_long_read(f->f_security);
>>> +}
>>> +
>>> +static inline int __must_check lsm_lock_ema(struct lsm_ema_map *map)
>>> +{
>>> + return mutex_lock_interruptible(&map->lock);
>>> +}
>>> +
>>> +static inline void lsm_unlock_ema(struct lsm_ema_map *map)
>>> +{
>>> + mutex_unlock(&map->lock);
>>> +}
>>> +
>>> +static inline struct lsm_ema *lsm_prev_ema(struct lsm_ema *p,
>>> + struct lsm_ema_map *map)
>>> +{
>>> + p = list_prev_entry(p, link);
>>> + return &p->link == &map->list ? NULL : p;
>>> +}
>>> +
>>> +static inline struct lsm_ema *lsm_next_ema(struct lsm_ema *p,
>>> + struct lsm_ema_map *map)
>>> +{
>>> + p = list_next_entry(p, link);
>>> + return &p->link == &map->list ? NULL : p;
>>> +}
>>> +
>>> +static inline struct lsm_ema *lsm_find_ema(struct lsm_ema_map *map,
>> size_t a)
>>> +{
>>> + struct lsm_ema *p;
>>> +
>>> + BUG_ON(!mutex_is_locked(&map->lock));
>>> +
>>> + list_for_each_entry(p, &map->list, link)
>>> + if (a < p->end)
>>> + break;
>>> + return &p->link == &map->list ? NULL : p;
>>> +}
>>> +
>>> +static inline int lsm_insert_ema(struct lsm_ema_map *map, struct
>> lsm_ema *n)
>>> +{
>>> + struct lsm_ema *p = lsm_find_ema(map, n->start);
>>> +
>>> + if (!p)
>>> + list_add_tail(&n->link, &map->list);
>>> + else if (n->end <= p->start)
>>> + list_add_tail(&n->link, &p->link);
>>> + else
>>> + return -EEXIST;
>>> +
>>> + lsm_merge_ema(n, map);
>>> + if (p)
>>> + lsm_merge_ema(p, map);
>>> + return 0;
>>> +}
>>> +
>>> +static inline int lsm_for_each_ema(struct lsm_ema_map *map, size_t
>> start,
>>> + size_t end, int (*cb)(struct lsm_ema *,
>>> + void *), void *arg)
>>> +{
>>> + struct lsm_ema *ema;
>>> + int rc;
>>> +
>>> + ema = lsm_find_ema(map, start);
>>> + while (ema && end > ema->start) {
>>> + if (start > ema->start)
>>> + lsm_split_ema(ema, start, map);
>>> + if (end < ema->end)
>>> + ema = lsm_split_ema(ema, end, map);
>>> +
>>> + rc = (*cb)(ema, arg);
>>> + lsm_merge_ema(ema, map);
>>> + if (rc)
>>> + return rc;
>>> +
>>> + ema = lsm_next_ema(ema, map);
>>> + }
>>> +
>>> + if (ema)
>>> + lsm_merge_ema(ema, map);
>>> + return 0;
>>> +}
>> There is no way that these belong as part of the LSM
>> infrastructure. If you need an enclave management API
>> you need to find some other place for it.
> They are NO enclave management APIs. They don't manage enclaves. They track origins of enclave pages. They are needed by all LSMs.
>
> As I stated in the cover letter, the primary question is how to prevent SGX from being abused as a backdoor to make executable pages that would otherwise not be executable without SGX. Any LSM module unaware of that would leave that "hole" open. So tracking enclave pages will become a common task for all LSMs that care page protections, and that's why I place it inside LSM infrastructure.
Page protections are an important part of many security features,
but that's beside the point. The LSM system provides mechanism for
providing additional restrictions to existing security mechanisms.
First, you create the security mechanism (e.g. enclaves) then you
add LSM hooks so that security modules (e.g. SELinux) can apply
their own policies in addition. In support of this, the LSM blob
mechanism allows security modules to maintain their own information
about the system components (e.g. file, inode, cred, task) they
care about. The LSM infrastructure does not itself provide or
support security data or policy. That's strictly for the modules
to do.
^ permalink raw reply
* Re: [RFC PATCH v5 1/1] Add dm verity root hash pkcs7 sig validation.
From: Eric Biggers @ 2019-06-27 23:41 UTC (permalink / raw)
To: Jaskaran Khurana
Cc: linux-security-module, linux-kernel, linux-integrity,
linux-fsdevel, agk, snitzer, dm-devel, jmorris, scottsh, mpatocka,
gmazyland
In-Reply-To: <20190619191048.20365-2-jaskarankhurana@linux.microsoft.com>
Hi Jaskaran, one comment (I haven't reviewed this in detail):
On Wed, Jun 19, 2019 at 12:10:48PM -0700, Jaskaran Khurana wrote:
> diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
> index db269a348b20..2d658a3512cb 100644
> --- a/drivers/md/Kconfig
> +++ b/drivers/md/Kconfig
> @@ -475,6 +475,7 @@ config DM_VERITY
> select CRYPTO
> select CRYPTO_HASH
> select DM_BUFIO
> + select SYSTEM_DATA_VERIFICATION
> ---help---
> This device-mapper target creates a read-only device that
> transparently validates the data on one underlying device against
> diff --git a/drivers/md/Makefile b/drivers/md/Makefile
> index be7a6eb92abc..3b47b256b15e 100644
> --- a/drivers/md/Makefile
> +++ b/drivers/md/Makefile
> @@ -18,7 +18,7 @@ dm-cache-y += dm-cache-target.o dm-cache-metadata.o dm-cache-policy.o \
> dm-cache-background-tracker.o
> dm-cache-smq-y += dm-cache-policy-smq.o
> dm-era-y += dm-era-target.o
> -dm-verity-y += dm-verity-target.o
> +dm-verity-y += dm-verity-target.o dm-verity-verify-sig.o
> md-mod-y += md.o md-bitmap.o
> raid456-y += raid5.o raid5-cache.o raid5-ppl.o
> dm-zoned-y += dm-zoned-target.o dm-zoned-metadata.o dm-zoned-reclaim.o
Perhaps this should be made optional and controlled by a kconfig option
CONFIG_DM_VERITY_SIGNATURE_VERIFICATION, similar to CONFIG_DM_VERITY_FEC?
CONFIG_SYSTEM_DATA_VERIFICATION brings in a lot of stuff, which might be
unnecessary for some dm-verity users. Also, you've already separated most of
the code out into a separate .c file anyway.
- Eric
^ permalink raw reply
* Re: [PATCH v4 00/23] LSM: Module stacking for AppArmor
From: John Johansen @ 2019-06-27 23:44 UTC (permalink / raw)
To: James Morris
Cc: Casey Schaufler, casey.schaufler, linux-security-module, selinux,
keescook, penguin-kernel, paul, sds
In-Reply-To: <alpine.LRH.2.21.1906271614490.1670@namei.org>
On 6/27/19 4:16 PM, James Morris wrote:
> On Thu, 27 Jun 2019, John Johansen wrote:
>
>> I have more test combinations churning but figure I could report what I have so far
>
> Do you have any way to test the nested scenario of say an AppArmor host
> with SELinux running in containers?
>
No, an selinux container doesn't really work atm. The issue is to do with
namespacing. I can boot an AppArmor host with selinux enabled, but the
container loading selinux policy gets interesting, and without namespacing
the container policy affects the host.
It is of course possible to label the system such that you can sort of
make it work, but it isn't really practical.
I have played with the selinuxns branch trying to get this to work, but
I ran into some issues I couldn't resolve. However it has been five
months since I tried that so I can look at it again.
The AppArmor container on an selinux host case is easier partly because of
how policy is applied, partly because namespacing of its policy is already
supported upstream, and partly because I just know it better.
I do have plans to test the apparmor container on selinux but I haven't
gotten that far and am planning on waiting for this one until Casey kicks
out v5.
^ permalink raw reply
* RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Xing, Cedric @ 2019-06-28 0:47 UTC (permalink / raw)
To: Casey Schaufler, linux-sgx@vger.kernel.org,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org
Cc: Schaufler, Casey, jmorris@namei.org, luto@kernel.org,
jethro@fortanix.com, greg@enjellic.com, sds@tycho.nsa.gov,
jarkko.sakkinen@linux.intel.com, Christopherson, Sean J
In-Reply-To: <9f525db2-f46b-b4cb-c4e9-b9dbd18ed4d2@schaufler-ca.com>
> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
> Sent: Thursday, June 27, 2019 4:37 PM
> >>
> >> This code should not be mixed in with the LSM infrastructure.
> >> It should all be contained in its own module, under security/enclave.
> > lsm_ema is *intended* to be part of the LSM infrastructure.
>
> That's not going to fly, not for a minute.
Why not, if there's a need for it?
And what's the concern here if it becomes part of the LSM infrastructure.
>
> > It is going to be shared among all LSMs that would like to track
> enclave pages and their origins.
>
> That's true for InfiniBand, tun and sctp as well. Look at their
> implementations.
As far as I can tell, InfiniBand, tun and sctp, all of them seemed used inside SELinux only.
If you had a chance to look at v1 of my series, I started by burying everything inside SELinux too. But Stephen pointed out such tracking would be needed by all LSMs so code duplication might be a concern. Thus I responded by moving it into LSM infrastructure.
>
> > And they could be extended to store more information as deemed
> appropriate by the LSM module.
>
> Which is what blobs are for, but that does not appear to be how
> you're using either the file blob or your new ema blob.
A lsm_ema_map pointer is stored in file->f_security. Each lsm_ema_map contains a list of lsm_ema structures. In my last patch, SELinux stores a ema_security_struct with every ema, by setting selinux_blob_sizes.lbs_ema_data to sizeof(ema_security_struct).
ema_security_struct is initialized in selinux_enclave_load(), and checked in enclave_mprotect(), which is a subroutine of selinux_file_mprotect(). BTW, it is alloced/freed automatically by LSM infrastructure in security_enclave_load()/security_file_free().
>
> > The last patch of this series shows how to extend EMA inside SELinux.
>
> I don't see (but I admit the code doesn't make a lot of sense to me)
> anything you couldn't do in the SELinux code by adding data to the
> file blob. The data you're adding to the LSM infrastructure doesn't
> belong there, and it doesn't need to be there.
You are correct. My v1 did it inside SELinux.
The key question I think is whether only SELinux needs it, or all LSMs need it. Stephen thought it was the latter (and I agree with him) so I moved it into the LSM infrastructure to be shared, just like the auditing code.
> >> Not acceptable for the LSM infrastructure. They
> >> are inconsistent with the way data is used there.
> > I'm not sure I understand this comment.
>
> It means that your definition and use of the lsm_ema_blob
> does not match the way other blobs are managed and used.
> The LSM infrastructure uses these entries in a very particular
> way, and you're trying to use it differently. Your might be
> able to change the rest of the enclave system to use it
> correctly, or you might be able to find a different place
> for it.
I'm still not sure why you think this (lbs_ema_data) is inconsistent with other blobs.
Same as all other blobs, an LSM requests it by storing the needed size in it, and is assigned an offset, and the buffer is allocated/freed by the infrastructure. Am I missing anything?
> >
> > As I stated in the cover letter, the primary question is how to
> prevent SGX from being abused as a backdoor to make executable pages
> that would otherwise not be executable without SGX. Any LSM module
> unaware of that would leave that "hole" open. So tracking enclave pages
> will become a common task for all LSMs that care page protections, and
> that's why I place it inside LSM infrastructure.
>
> Page protections are an important part of many security features,
> but that's beside the point. The LSM system provides mechanism for
> providing additional restrictions to existing security mechanisms.
> First, you create the security mechanism (e.g. enclaves) then you
> add LSM hooks so that security modules (e.g. SELinux) can apply
> their own policies in addition. In support of this, the LSM blob
> mechanism allows security modules to maintain their own information
> about the system components (e.g. file, inode, cred, task) they
> care about. The LSM infrastructure does not itself provide or
> support security data or policy. That's strictly for the modules
> to do.
Agreed!
EMA doesn't dictate policies for sure. Is it considered "security data"? I'm not sure the definition of "security data" here. It does store some "data", something that multiple LSM modules would need to duplicate if not pulled into a common place. It is meant to be a "helper" data structure, just like the auditing code.
^ permalink raw reply
* Re: [RFC PATCH v5 1/1] Add dm verity root hash pkcs7 sig validation.
From: Jaskaran Singh Khurana @ 2019-06-28 1:49 UTC (permalink / raw)
To: Eric Biggers
Cc: linux-security-module, linux-kernel, linux-integrity,
linux-fsdevel, agk, snitzer, dm-devel, jmorris, scottsh, mpatocka,
gmazyland
In-Reply-To: <20190627234149.GA212823@gmail.com>
On Thu, 27 Jun 2019, Eric Biggers wrote:
> Hi Jaskaran, one comment (I haven't reviewed this in detail):
>
> On Wed, Jun 19, 2019 at 12:10:48PM -0700, Jaskaran Khurana wrote:
>> diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
>> index db269a348b20..2d658a3512cb 100644
>> --- a/drivers/md/Kconfig
>> +++ b/drivers/md/Kconfig
>> @@ -475,6 +475,7 @@ config DM_VERITY
>> select CRYPTO
>> select CRYPTO_HASH
>> select DM_BUFIO
>> + select SYSTEM_DATA_VERIFICATION
>> ---help---
>> This device-mapper target creates a read-only device that
>> transparently validates the data on one underlying device against
>> diff --git a/drivers/md/Makefile b/drivers/md/Makefile
>> index be7a6eb92abc..3b47b256b15e 100644
>> --- a/drivers/md/Makefile
>> +++ b/drivers/md/Makefile
>> @@ -18,7 +18,7 @@ dm-cache-y += dm-cache-target.o dm-cache-metadata.o dm-cache-policy.o \
>> dm-cache-background-tracker.o
>> dm-cache-smq-y += dm-cache-policy-smq.o
>> dm-era-y += dm-era-target.o
>> -dm-verity-y += dm-verity-target.o
>> +dm-verity-y += dm-verity-target.o dm-verity-verify-sig.o
>> md-mod-y += md.o md-bitmap.o
>> raid456-y += raid5.o raid5-cache.o raid5-ppl.o
>> dm-zoned-y += dm-zoned-target.o dm-zoned-metadata.o dm-zoned-reclaim.o
>
> Perhaps this should be made optional and controlled by a kconfig option
> CONFIG_DM_VERITY_SIGNATURE_VERIFICATION, similar to CONFIG_DM_VERITY_FEC?
>
> CONFIG_SYSTEM_DATA_VERIFICATION brings in a lot of stuff, which might be
> unnecessary for some dm-verity users. Also, you've already separated most of
> the code out into a separate .c file anyway.
>
> - Eric
>
Hello Eric,
This started with a config (see V4). We didnot want scripts that pass this
parameter to suddenly stop working if for some reason the
verification is turned off so the optional parameter was just
parsed and no validation happened if the CONFIG was turned off. This was
changed to a commandline parameter after feedback from the community, so I would prefer
to keep it *now* as commandline parameter. Let me know if you are OK with
this.
Regards,
JK
^ permalink raw reply
* Re: [RFC PATCH v5 1/1] Add dm verity root hash pkcs7 sig validation.
From: Jaskaran Singh Khurana @ 2019-06-28 1:52 UTC (permalink / raw)
To: Milan Broz
Cc: linux-security-module, linux-kernel, linux-integrity,
linux-fsdevel, agk, snitzer, dm-devel, jmorris, scottsh, ebiggers,
mpatocka
In-Reply-To: <568f2532-e46b-5ac7-4fc5-c96983702f2d@gmail.com>
On Thu, 27 Jun 2019, Milan Broz wrote:
> Hi,
>
> I tried to test test the patch, two comments below.
>
> On 19/06/2019 21:10, Jaskaran Khurana wrote:
>> The verification is to support cases where the roothash is not secured by
>> Trusted Boot, UEFI Secureboot or similar technologies.
>> One of the use cases for this is for dm-verity volumes mounted after boot,
>> the root hash provided during the creation of the dm-verity volume has to
>> be secure and thus in-kernel validation implemented here will be used
>> before we trust the root hash and allow the block device to be created.
>>
>> The signature being provided for verification must verify the root hash and
>> must be trusted by the builtin keyring for verification to succeed.
>>
>> The hash is added as a key of type "user" and the description is passed to
>> the kernel so it can look it up and use it for verification.
>>
>> Kernel commandline parameter will indicate whether to check (only if
>> specified) or force (for all dm verity volumes) roothash signature
>> verification.
>>
>> Kernel commandline: dm_verity.verify_sig=1 or 2 for check/force root hash
>> signature validation respectively.
>
> 1) I think the switch should be just boolean - enforce signatures for all dm-verity targets
> (with default to false/off).
>
> The rest should be handled by simple logic - if the root_hash_sig_key_desc option
> is specified, the signature MUST be validated in the constructor, all errors should cause
> failure (bad reference in keyring, bad signature, etc).
>
> (Now it ignores for example bad reference to the keyring, this is quite misleading.)
>
> If a user wants to activate a dm-verity device without a signature, just remove
> optional argument referencing the signature.
> (This is not possible with dm_verity.verify_sig set to true/on.)
>
>
> 2) All DM targets must provide the same mapping table status ("dmsetup table"
> command) as initially configured.
> The output of the command should be directly usable as mapping table constructor.
>
> Your patch is missing that part, I tried to fix it, add-on patch is here
> https://git.kernel.org/pub/scm/linux/kernel/git/mbroz/linux.git/commit/?h=dm-cryptsetup&id=a26c10806f5257e255b6a436713127e762935ad3
> (feel free to fold it in your patch)
>
>
> Thanks,
> Milan
>
Hello Milan,
Thanks for testing and reviewing this. I will take care of the above
comments change it to a boolean and for the second point merge the add-on
patch that you shared.
Regards,
Jaskaran
^ permalink raw reply
* [PATCH v12 00/11] Appended signatures support for IMA appraisal
From: Thiago Jung Bauermann @ 2019-06-28 2:19 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, David Howells, David Woodhouse,
Jessica Yu, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Thiago Jung Bauermann
Hello,
This version is essentially identical to the last one.
It is only a rebase on top of today's linux-integrity/next-queued-testing,
prompted by conflicts with Prakhar Srivastava's patches to measure the
kernel command line. It also drops two patches that are already present in
that branch.
As I mentioned in an earlier email, I believe Mimi is happy with this
version but before she can accept it I still need acks from maintainers of
the module and asymmetric keys subsystems for the first three patches.
Many thanks to Mimi Zohar for her help with the development of this patch
series.
This patch which I sent earlier today needs to be applied first:
ima: Update MAX_TEMPLATE_NAME_LEN to fit largest reasonable definition
Original cover letter:
On the OpenPOWER platform, secure boot and trusted boot are being
implemented using IMA for taking measurements and verifying signatures.
Since the kernel image on Power servers is an ELF binary, kernels are
signed using the scripts/sign-file tool and thus use the same signature
format as signed kernel modules.
This patch series adds support in IMA for verifying those signatures.
It adds flexibility to OpenPOWER secure boot, because it allows it to boot
kernels with the signature appended to them as well as kernels where the
signature is stored in the IMA extended attribute.
Changes since v11:
- Patch "integrity: Introduce struct evm_xattr"
- Dropped since it's already in linux-integrity/next-queued-testing.
- Patch "ima: Use designated initializers for struct ima_event_data"
- Dropped since it's already in linux-integrity/next-queued-testing.
Changes since v10:
- Patch "MODSIGN: Export module signature definitions"
- Moved config MODULE_SIG_FORMAT definition before its use. Suggested by
Mimi Zohar.
- Added missing kerneldoc for @name parameter. Suggested by Mimi Zohar.
- Patch "ima: Implement support for module-style appended signatures"
- Bugfix: don't check status variable when deciding whether to verify
modsig in ima_appraise_measurement(). Suggested by Mimi Zohar.
- Bugfix: verify the modsig in ima_appraise_measurement() if the xattr
contains a digest. Suggested by Mimi Zohar.
- Patch "ima: Define ima-modsig template"
- Renamed ima_modsig_serialize() to ima_get_raw_modsig().
- Renamed check_current_template_modsig() to check_template_modsig().
- Fixed outdated comment in ima_eventmodsig_init(). Suggested by Mimi
Zohar.
- Check either the global or the per-rule template when an appraisal rule
allows modsig. Suggested by Mimi Zohar.
- Patch "ima: Store the measurement again when appraising a modsig"
- Bugfix: Only re-measure file containing modsig if it was measured
before.
- Check for modsig-related fields in the template_desc obtained in
process_measurement() which can be a per-rule template. Suggested by Mimi
Zohar.
- Patch "ima: Allow template= option for appraise rules as well"
- New patch. Suggested by Mimi Zohar.
Changes since v9:
- Patch "MODSIGN: Export module signature definitions"
- Moved mod_check_sig() to a new file so that CONFIG_IMA_APPRAISE_MODSIG
doesn't have to depend on CONFIG_MODULES.
- Changed scripts/Makefile to build sign-file if CONFIG_MODULE_SIG_FORMAT
is set.
- Removed Mimi's Reviewed-by because of the changes in this version.
- Patch "PKCS#7: Refactor verify_pkcs7_signature()"
- Don't add function pkcs7_get_message_sig() anymore, since it's not
needed in the current version.
- Patch "PKCS#7: Introduce pkcs7_get_digest()"
- Changed 'len' argument from 'u8 *' to 'u32 *'.
- Added 'hash_algo' argument to obtain the algo used for the digest.
- Don't check whether 'buf', 'len' and 'hash_algo' output arguments are NULL,
since the function's only caller always sets them.
- Removed Mimi's Reviewed-by because of the changes in this version.
- Patch "integrity: Introduce asymmetric_sig_has_known_key()"
- Dropped.
- Patch "integrity: Introduce integrity_keyring_from_id"
- Squashed into "ima: Implement support for module-style appended signatures"
- Changed integrity_keyring_from_id() to a static function (suggested by Mimi
Zohar).
- Patch "ima: Introduce is_signed()"
- Dropped.
- Patch "ima: Export func_tokens"
- Squashed into "ima: Implement support for module-style appended signatures"
- Patch "ima: Use designated initializers for struct ima_event_data"
- New patch.
- Patch "ima: Factor xattr_verify() out of ima_appraise_measurement()"
- New patch.
- Patch "ima: Implement support for module-style appended signatures"
- Renamed 'struct modsig_hdr' to 'struct modsig'.
- Added integrity_modsig_verify() to integrity/digsig.c so that it's not
necessary to export integrity_keyring_from_id() (Suggested by Mimi Zohar).
- Don't add functions ima_xattr_sig_known_key() and
modsig_has_known_key() since they're not necessary anymore.
- Added modsig argument to ima_appraise_measurement().
- Verify modsig in a separate function called by ima_appraise_measurement().
- Renamed ima_read_collect_modsig() to ima_read_modsig(), with a separate
collect function added in patch "ima: Collect modsig" (suggested by Mimi
Zohar).
- In ima_read_modsig(), moved code saving of raw PKCS7 data to 'struct
modsig' to patch "ima: Collect modsig".
- In ima_read_modsig(), moved all parts related to the modsig hash to
patch "ima: Collect modsig".
- In ima_read_modsig(), don't check if the buf pointer is NULL since it's
never supposed to happen.
- Renamed ima_free_xattr_data() to ima_free_modsig().
- No need to check for modsig in ima_read_xattr() and
ima_inode_set_xattr() anymore.
- In ima_modsig_verify(), don't check if the modsig pointer is NULL since
it's not supposed to happen.
- Don't define IMA_MODSIG element in enum evm_ima_xattr_type.
- Patch "ima: Collect modsig"
- New patch.
- Patch "ima: Define ima-modsig template"
- Patch renamed from "ima: Add new "d-sig" template field"
- Renamed 'd-sig' template field to 'd-modsig'.
- Added 'modsig' template field.
- Added 'ima-modsig' defined template descriptor.
- Renamed ima_modsig_serialize_data() to ima_modsig_serialize().
- Renamed ima_get_modsig_hash() to ima_get_modsig_digest(). Also the
function is a lot simpler now since what it used to do is now done in
ima_collect_modsig() and pkcs7_get_digest().
- Added check for failed modsig collection in ima_eventdigest_modsig_init().
- Added modsig argument to ima_store_measurement().
- Added 'modsig' field to struct ima_event_data.
- Removed check for modsig == NULL in ima_get_modsig_digest() and in
ima_modsig_serialize_data() since their callers already performs that
check.
- Moved check_current_template_modsig() to this patch, previously was in
"ima: Store the measurement again when appraising a modsig".
- Patch "ima: Store the measurement again when appraising a modsig"
- Renamed ima_template_has_sig() to ima_template_has_modsig().
- Added a change to ima_collect_measurement(), making it to call
ima_collect_modsig() even if IMA_COLLECT is set in iint->flags.
- Removed IMA_READ_MEASURE flag.
- Renamed template_has_sig global variable to template_has_modsig.
- Renamed find_sig_in_template() to find_modsig_in_template().
Thiago Jung Bauermann (11):
MODSIGN: Export module signature definitions
PKCS#7: Refactor verify_pkcs7_signature()
PKCS#7: Introduce pkcs7_get_digest()
integrity: Select CONFIG_KEYS instead of depending on it
ima: Add modsig appraise_type option for module-style appended
signatures
ima: Factor xattr_verify() out of ima_appraise_measurement()
ima: Implement support for module-style appended signatures
ima: Collect modsig
ima: Define ima-modsig template
ima: Store the measurement again when appraising a modsig
ima: Allow template= option for appraise rules as well
Documentation/ABI/testing/ima_policy | 6 +-
Documentation/security/IMA-templates.rst | 3 +
certs/system_keyring.c | 61 +++++--
crypto/asymmetric_keys/pkcs7_verify.c | 33 ++++
include/crypto/pkcs7.h | 4 +
include/linux/module.h | 3 -
include/linux/module_signature.h | 44 +++++
include/linux/verification.h | 10 ++
init/Kconfig | 6 +-
kernel/Makefile | 1 +
kernel/module.c | 1 +
kernel/module_signature.c | 46 +++++
kernel/module_signing.c | 56 +------
scripts/Makefile | 2 +-
security/integrity/Kconfig | 2 +-
security/integrity/digsig.c | 43 ++++-
security/integrity/ima/Kconfig | 13 ++
security/integrity/ima/Makefile | 1 +
security/integrity/ima/ima.h | 60 ++++++-
security/integrity/ima/ima_api.c | 23 ++-
security/integrity/ima/ima_appraise.c | 194 ++++++++++++++--------
security/integrity/ima/ima_main.c | 24 ++-
security/integrity/ima/ima_modsig.c | 169 +++++++++++++++++++
security/integrity/ima/ima_policy.c | 68 +++++++-
security/integrity/ima/ima_template.c | 26 ++-
security/integrity/ima/ima_template_lib.c | 64 ++++++-
security/integrity/ima/ima_template_lib.h | 4 +
security/integrity/integrity.h | 20 +++
28 files changed, 819 insertions(+), 168 deletions(-)
create mode 100644 include/linux/module_signature.h
create mode 100644 kernel/module_signature.c
create mode 100644 security/integrity/ima/ima_modsig.c
^ permalink raw reply
* [PATCH v12 02/11] PKCS#7: Refactor verify_pkcs7_signature()
From: Thiago Jung Bauermann @ 2019-06-28 2:19 UTC (permalink / raw)
To: linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, David Howells, David Woodhouse,
Jessica Yu, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Thiago Jung Bauermann
In-Reply-To: <20190628021934.4260-1-bauerman@linux.ibm.com>
IMA will need to verify a PKCS#7 signature which has already been parsed.
For this reason, factor out the code which does that from
verify_pkcs7_signature() into a new function which takes a struct
pkcs7_message instead of a data buffer.
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
---
certs/system_keyring.c | 61 ++++++++++++++++++++++++++----------
include/linux/verification.h | 10 ++++++
2 files changed, 55 insertions(+), 16 deletions(-)
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index c05c29ae4d5d..4ba82e52e4b4 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -194,33 +194,27 @@ late_initcall(load_system_certificate_list);
#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
/**
- * verify_pkcs7_signature - Verify a PKCS#7-based signature on system data.
+ * verify_pkcs7_message_sig - Verify a PKCS#7-based signature on system data.
* @data: The data to be verified (NULL if expecting internal data).
* @len: Size of @data.
- * @raw_pkcs7: The PKCS#7 message that is the signature.
- * @pkcs7_len: The size of @raw_pkcs7.
+ * @pkcs7: The PKCS#7 message that is the signature.
* @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
* (void *)1UL for all trusted keys).
* @usage: The use to which the key is being put.
* @view_content: Callback to gain access to content.
* @ctx: Context for callback.
*/
-int verify_pkcs7_signature(const void *data, size_t len,
- const void *raw_pkcs7, size_t pkcs7_len,
- struct key *trusted_keys,
- enum key_being_used_for usage,
- int (*view_content)(void *ctx,
- const void *data, size_t len,
- size_t asn1hdrlen),
- void *ctx)
+int verify_pkcs7_message_sig(const void *data, size_t len,
+ struct pkcs7_message *pkcs7,
+ struct key *trusted_keys,
+ enum key_being_used_for usage,
+ int (*view_content)(void *ctx,
+ const void *data, size_t len,
+ size_t asn1hdrlen),
+ void *ctx)
{
- struct pkcs7_message *pkcs7;
int ret;
- pkcs7 = pkcs7_parse_message(raw_pkcs7, pkcs7_len);
- if (IS_ERR(pkcs7))
- return PTR_ERR(pkcs7);
-
/* The data should be detached - so we need to supply it. */
if (data && pkcs7_supply_detached_data(pkcs7, data, len) < 0) {
pr_err("PKCS#7 signature with non-detached data\n");
@@ -273,6 +267,41 @@ int verify_pkcs7_signature(const void *data, size_t len,
}
error:
+ pr_devel("<==%s() = %d\n", __func__, ret);
+ return ret;
+}
+
+/**
+ * verify_pkcs7_signature - Verify a PKCS#7-based signature on system data.
+ * @data: The data to be verified (NULL if expecting internal data).
+ * @len: Size of @data.
+ * @raw_pkcs7: The PKCS#7 message that is the signature.
+ * @pkcs7_len: The size of @raw_pkcs7.
+ * @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
+ * (void *)1UL for all trusted keys).
+ * @usage: The use to which the key is being put.
+ * @view_content: Callback to gain access to content.
+ * @ctx: Context for callback.
+ */
+int verify_pkcs7_signature(const void *data, size_t len,
+ const void *raw_pkcs7, size_t pkcs7_len,
+ struct key *trusted_keys,
+ enum key_being_used_for usage,
+ int (*view_content)(void *ctx,
+ const void *data, size_t len,
+ size_t asn1hdrlen),
+ void *ctx)
+{
+ struct pkcs7_message *pkcs7;
+ int ret;
+
+ pkcs7 = pkcs7_parse_message(raw_pkcs7, pkcs7_len);
+ if (IS_ERR(pkcs7))
+ return PTR_ERR(pkcs7);
+
+ ret = verify_pkcs7_message_sig(data, len, pkcs7, trusted_keys, usage,
+ view_content, ctx);
+
pkcs7_free_message(pkcs7);
pr_devel("<==%s() = %d\n", __func__, ret);
return ret;
diff --git a/include/linux/verification.h b/include/linux/verification.h
index 018fb5f13d44..5e1d41f2b336 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -36,6 +36,7 @@ extern const char *const key_being_used_for[NR__KEY_BEING_USED_FOR];
#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
struct key;
+struct pkcs7_message;
extern int verify_pkcs7_signature(const void *data, size_t len,
const void *raw_pkcs7, size_t pkcs7_len,
@@ -45,6 +46,15 @@ extern int verify_pkcs7_signature(const void *data, size_t len,
const void *data, size_t len,
size_t asn1hdrlen),
void *ctx);
+extern int verify_pkcs7_message_sig(const void *data, size_t len,
+ struct pkcs7_message *pkcs7,
+ struct key *trusted_keys,
+ enum key_being_used_for usage,
+ int (*view_content)(void *ctx,
+ const void *data,
+ size_t len,
+ size_t asn1hdrlen),
+ void *ctx);
#ifdef CONFIG_SIGNED_PE_FILE_VERIFICATION
extern int verify_pefile_signature(const void *pebuf, unsigned pelen,
^ permalink raw reply related
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