From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752530AbdEETfq (ORCPT ); Fri, 5 May 2017 15:35:46 -0400 Received: from smtp.nsa.gov ([8.44.101.8]:23266 "EHLO emsm-gh1-uea10.nsa.gov" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751304AbdEETfl (ORCPT ); Fri, 5 May 2017 15:35:41 -0400 X-IronPort-AV: E=Sophos;i="5.38,294,1491264000"; d="scan'208";a="6714428" IronPort-PHdr: =?us-ascii?q?9a23=3AWq/CPhM6eFRLqxQqF6wl6mtUPXoX/o7sNwtQ0KIM?= =?us-ascii?q?zox0K/3zpMbcNUDSrc9gkEXOFd2CrakV1ayI7+u5AjJIyK3CmUhKSIZLWR4BhJ?= =?us-ascii?q?detC0bK+nBN3fGKuX3ZTcxBsVIWQwt1Xi6NU9IBJS2PAWK8TW94jEIBxrwKxd+?= =?us-ascii?q?KPjrFY7OlcS30P2594HObwlSijewZbJ/IA+3oAjeucUanIVvJ6cswRbVv3VEfP?= =?us-ascii?q?hby3l1LlyJhRb84cmw/J9n8ytOvv8q6tBNX6bncakmVLJUFDspPXw7683trhnD?= =?us-ascii?q?UBCA5mAAXWUMkxpHGBbK4RfnVZrsqCT6t+592C6HPc3qSL0/RDqv47t3RBLulS?= =?us-ascii?q?wKLCAy/n3JhcNsjaJbuBOhqAJ5w47Ie4GeKf5ycrrAcd8GWWZNW8BcWCJbAoO4?= =?us-ascii?q?coABEewPM+hFpIX5vlcCsweyCQyqCejyyDFHm2X20LU53eo8EQ/IwgIuENAAsH?= =?us-ascii?q?TTsNr7M6gdX++uwanUzjjOde9a1C3h5IXKdB0qvPGCXah3ccrU0UQhCh/FgUuI?= =?us-ascii?q?qYzlITyV0PkGvXWe7+V6UeKvj3AoqgFsqTWo3ccjl5LJi5kJylHE6Sp5wIE1Kc?= =?us-ascii?q?e+SE5ge9GoCpRQtyaEN4ZvRM4pXmJmuD4ix7EbtpO2czIGxZQ6yxLFdfCKfJaE?= =?us-ascii?q?7gj+WOuXPDx2nmhqeKiliBa36UWgz+r8WdSq31tStSpFl8XMtmgK1xzO9siLUv?= =?us-ascii?q?t98Vml2TaIzw3T9vtLIVo1larGMJ4t2KIwl5oPvkTDGS/6gkP2g7ONdko44OSo?= =?us-ascii?q?7uXnYrH+qp+dMY97lB3+P7wzlsGwDuk0KAgDU3WB9eii27Dv41f1TKhSgv0ziK?= =?us-ascii?q?bZsZTaJcoBpq6+Bg9Yyp0j5AukDzq9zNQZnWUILFJCeB6diYjpIEvBLOr3Dfe4?= =?us-ascii?q?nVSgiC1ryOzePr39HpXNKWDOkLX7crZ49UFc0xA/zdRF65JRDbEBOujzWk7qtN?= =?us-ascii?q?HDCR85KRa7zPrjCNpjzI8RRWWPAqqBOqPIrVCI/v4vI/WLZIINozbyMOUl5//1?= =?us-ascii?q?jX8inl8de7Kk0oYKZ3C+BPhpP0KZYX/0iNcbDWgKphY+TPDtiFCaVT5TZnCyX7?= =?us-ascii?q?8z5z0iC4KpE53DRoazj7yFxyq7AJJWaX5cClyWEnfnaZ+EW/ESZyKWOMNhlSYE?= =?us-ascii?q?Vbe5QY87yR6urBP6y6ZgLufM/i0YtJTj1MV65uHKjhE96yB7D9mH02GMU2F1nn?= =?us-ascii?q?oERzox3K9ivEx9zkmM3LNmg/xXC9NT/elFUgQkOp7Gyex1FdTyVhjGfteTR1as?= =?us-ascii?q?Ws+mDi0pTtIt398OZF5wG865gRDe3iqqGKQVl7uNBJAu763Twn7xKt1yy3bDzq?= =?us-ascii?q?Yhjl0mTtFTOm2hg64svzTUUrXEmUyCi6enc+w2wS/W72CFhT6VtlxZSxV3V+PJ?= =?us-ascii?q?UXY3aU7frNC/7UTHGfvmDbUhLxsE0sOJN7FLdsysiFJKWfPuEMrRbnj3mGqqAx?= =?us-ascii?q?uMgLSWY86if2QbwTWYC0UelQ0X1WiJOBJ4BSq7pW/aSjt0GhaneE7o8O9juFun?= =?us-ascii?q?X0Q0yEeMdEQn2L2rqTAPgvnJcO8exrIJvm8arjxwGFusl4bNB8GouxtqfKIaZ8?= =?us-ascii?q?g0plhAyzSK5ERGIpW8IvU61RYleANtsharjk8vBw=3D=3D?= X-IPAS-Result: =?us-ascii?q?A2FQAwDd0gxZ/wHyM5BdGgEBAQECAQEBAQgBAQEBFQEBAQE?= =?us-ascii?q?CAQEBAQgBAQEBgwEpgW6DaJpCBoEml3+GJAKESVcBAQEBAQEBAQIBAmgogjMiA?= =?us-ascii?q?YI/AQEBAQIBIwQLAUYFCwkCDQEKAgImAgJXBgESiAiCCwUIk0KdYYFsOiYCikA?= =?us-ascii?q?BAQEBAQEEAQEBAQEBIoELhQ6CJIMbh2mCXwEEiTuIN4t9kxeCBIkTDIZFiHqLP?= =?us-ascii?q?VhZMSUJAh4IHw+FOAEcgX8kNoh1AQEB?= Message-ID: <1494013182.5745.19.camel@tycho.nsa.gov> Subject: Re: [PATCH v2 1/2] selinux: add brief info to policydb From: Stephen Smalley To: Sebastien Buisson , linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, selinux@tycho.nsa.gov Cc: Sebastien Buisson , james.l.morris@oracle.com Date: Fri, 05 May 2017 15:39:42 -0400 In-Reply-To: <1493979053-16691-1-git-send-email-sbuisson@ddn.com> References: <1493979053-16691-1-git-send-email-sbuisson@ddn.com> Organization: National Security Agency Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.6 (3.22.6-2.fc25) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 2017-05-05 at 19:10 +0900, Sebastien Buisson wrote: > Add policybrief field to struct policydb. It holds a brief info > of the policydb, in the following form: > <0 or 1 for enforce>:<0 or 1 for checkreqprot>:= > Policy brief is computed every time the policy is loaded, and when > enforce or checkreqprot are changed. > > Add security_policy_brief hook to give access to policy brief to > the rest of the kernel. Lustre client makes use of this information. > > Signed-off-by: Sebastien Buisson > --- >  include/linux/lsm_hooks.h           |  2 + >  include/linux/security.h            |  7 ++++ >  security/security.c                 |  6 +++ >  security/selinux/hooks.c            | 11 +++++- >  security/selinux/include/security.h |  2 + >  security/selinux/selinuxfs.c        |  6 ++- >  security/selinux/ss/policydb.c      | 70 > +++++++++++++++++++++++++++++++++++ >  security/selinux/ss/policydb.h      |  3 ++ >  security/selinux/ss/services.c      | 73 > +++++++++++++++++++++++++++++++++++++ >  9 files changed, 178 insertions(+), 2 deletions(-) > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index e67a526..b4dd605 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -104,8 +104,10 @@ >  static int __init enforcing_setup(char *str) >  { >   unsigned long enforcing; > - if (!kstrtoul(str, 0, &enforcing)) > + if (!kstrtoul(str, 0, &enforcing)) { >   selinux_enforcing = enforcing ? 1 : 0; > + security_policydb_update_info(NULL); I don't think you need this. You are unlikely to request the policy brief until after policy has been loaded (and if you do, you'll only get a partial result), so you can just defer setting up the enforcing field until the initial policy load, and then update it on subsequent writes to /sys/fs/selinux/enforce. > + } >   return 1; >  } >  __setup("enforcing=", enforcing_setup); > diff --git a/security/selinux/selinuxfs.c > b/security/selinux/selinuxfs.c > index ce71718..b959ee7 100644 > --- a/security/selinux/selinuxfs.c > +++ b/security/selinux/selinuxfs.c > @@ -55,8 +55,10 @@ >  static int __init checkreqprot_setup(char *str) >  { >   unsigned long checkreqprot; > - if (!kstrtoul(str, 0, &checkreqprot)) > + if (!kstrtoul(str, 0, &checkreqprot)) { >   selinux_checkreqprot = checkreqprot ? 1 : 0; > + security_policydb_update_info(NULL); > + } Ditto. Just initialize it with the rest of the info on initial policy load, and update it on writes to checkreqprot. >   return 1; >  } >  __setup("checkreqprot=", checkreqprot_setup); > diff --git a/security/selinux/ss/policydb.c > b/security/selinux/ss/policydb.c > index 0080122..9eb2f82 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -32,6 +32,8 @@ >  #include >  #include >  #include > +#include > +#include >  #include "security.h" >   >  #include "policydb.h" > @@ -879,6 +881,8 @@ void policydb_destroy(struct policydb *p) >   ebitmap_destroy(&p->filename_trans_ttypes); >   ebitmap_destroy(&p->policycaps); >   ebitmap_destroy(&p->permissive_map); > + > + kfree(p->policybrief); >  } >   >  /* > @@ -2220,6 +2224,67 @@ static int ocontext_read(struct policydb *p, > struct policydb_compat_info *info, >  } >   >  /* > + * Compute summary of a policy database binary representation file, > + * and store it into a policy database structure. > + */ > +static int policydb_brief(struct policydb *policydb, void *ptr) > +{ > + struct policy_file *fp = ptr; > + struct crypto_shash *tfm; > + char hashalg[] = "sha256"; > + int hashsize = SHA256_DIGEST_SIZE; size_t > + char hashval[hashsize]; unsigned char or u8 > + int idx; > + unsigned char *p; > + > + if (policydb->policybrief) > + return -EINVAL; > + > + tfm = crypto_alloc_shash(hashalg, 0, 0); > + if (IS_ERR(tfm)) { > + printk(KERN_ERR "Failed to alloc crypto hash %s\n", > hashalg); > + return PTR_ERR(tfm); > + } Should you be checking crypto_shash_digestsize(tfm) against sizeof hashval, or allocating hashval based on it instead? Not a problem now, but if you ever change the algorithm and forget to update the hashsize... > + > + { > + int rc; > + > + SHASH_DESC_ON_STACK(desc, tfm); > + desc->tfm = tfm; > + desc->flags = 0; > + rc = crypto_shash_init(desc); > + if (rc) { > + printk(KERN_ERR "Failed to init shash\n"); > + crypto_free_shash(tfm); > + return rc; > + } > + > + crypto_shash_update(desc, fp->data, fp->len); > + crypto_shash_final(desc, hashval); Just use crypto_shash_digest(); handles _init, _update, and _final for you in one call. > + crypto_free_shash(tfm); > + } > + > + /* policy brief is in the form: > +  * <0 or 1 for enforce>:<0 or 1 for > checkreqprot>:= > +  */ > + policydb->policybrief = kzalloc(5 + strlen(hashalg) + > 2*hashsize + 1, > + GFP_KERNEL); > + if (policydb->policybrief == NULL) > + return -ENOMEM; > + > + sprintf(policydb->policybrief, "x:x:%s=", hashalg); Couldn't you just directly set the enforcing and checkreqprot fields above? No need to call another function. > + security_policydb_update_info(policydb); > + p = policydb->policybrief + strlen(policydb->policybrief); > + for (idx = 0; idx < hashsize; idx++) { > + snprintf(p, 3, "%02x", (unsigned > char)(hashval[idx])); No need to cast if you fix the type above. > + p += 2; > + } > + policydb->policybrief_len = (size_t)(p - policydb- > >policybrief); This length is actually computable at build time, right? It isn't variant based on policy, just on hash algorithm. No need to store it in the policydb. > + > + return 0; > +} > + > +/* >   * Read the configuration data from a policy database binary >   * representation file into a policy database structure. >   */ > @@ -2238,6 +2303,11 @@ int policydb_read(struct policydb *p, void > *fp) >   if (rc) >   return rc; >   > + /* Compute sumarry of policy, and store it in policydb */ summary > + rc = policydb_brief(p, fp); > + if (rc) > + goto bad; > + >   /* Read the magic number and string length. */ >   rc = next_entry(buf, fp, sizeof(u32) * 2); >   if (rc) > diff --git a/security/selinux/ss/services.c > b/security/selinux/ss/services.c > index 60d9b02..9a94f8e 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -2170,6 +2170,79 @@ size_t security_policydb_len(void) >  } >   >  /** > + * security_policydb_brief - Get policydb brief > + * @brief: pointer to buffer holding brief > + * @len: in: brief buffer length if no alloc, out: brief string len > + * @alloc: whether to allocate buffer for brief or not > + * > + * On success 0 is returned , or negative value on error. > + **/ > +int security_policydb_brief(char **brief, size_t *len, bool alloc) > +{ > + int rc = 0; > + size_t policybrief_len; > + > + if (brief == NULL) > + return -EINVAL; You can return immediately if !ss_initialized. > + > + read_lock(&policy_rwlock); > + policybrief_len = policydb.policybrief_len; The length is fixed by the hash algorithm. No need to fetch it. > + if (policydb.policybrief == NULL) > + rc = -EAGAIN; This shouldn't ever be possible if ss_initialized, right? > + read_unlock(&policy_rwlock); > + > + if (rc) > + return rc; > + > + if (alloc) > + /* *brief must be kfreed by caller in this case */ > + *brief = kzalloc(policybrief_len + 1, GFP_KERNEL); > + else > + /* > +  * if !alloc, caller must pass a buffer that > +  * can hold policybrief_len+1 chars > +  */ > + if (*len < policybrief_len + 1) { > + /* put in *len the string size we need to > write */ > + *len = policybrief_len; > + return -ENAMETOOLONG; > + } > + > + if (*brief == NULL) > + return -ENOMEM; > + > + read_lock(&policy_rwlock); > + strncpy(*brief, policydb.policybrief, > policydb.policybrief_len); > + *len = policydb.policybrief_len; > + read_unlock(&policy_rwlock); > + > + return rc; > +} > + > +void security_policydb_update_info(void *p) > +{ > + /* policy brief is in the form: > +  * <0 or 1 for enforce>:<0 or 1 for > checkreqprot>:= > +  */ if (!ss_initialized) return; > + if (p) { > + struct policydb *poldb = p; > + /* update policydb given as parameter if possible */ > + if (poldb->policybrief) { > + poldb->policybrief[0] = '0' + > selinux_enforcing; > + poldb->policybrief[2] = '0' + > selinux_checkreqprot; This case could be handled directly in the caller. > + } > + } else { > + /* update global policydb, needs write lock */ > + write_lock_irq(&policy_rwlock); > + if (policydb.policybrief) { Don't need this once ss_initialized is set. > + policydb.policybrief[0] = '0' + > selinux_enforcing; > + policydb.policybrief[2] = '0' + > selinux_checkreqprot; > + } Technically only need to update one of the two fields at any given time, and the caller can specify which one. But maybe it isn't worth it. > + write_unlock_irq(&policy_rwlock); > + } > +} > + > +/** >   * security_port_sid - Obtain the SID for a port. >   * @protocol: protocol number >   * @port: port number