* Re: [PATCH] security/integrity: Include __func__ in messages for easier debug
From: Joe Perches @ 2020-01-30 3:08 UTC (permalink / raw)
To: Shuah Khan, jmorris, serge, mpe, zohar, erichte, nayna,
yuehaibing
Cc: linux-security-module, linux-kernel
In-Reply-To: <20200130020129.15328-1-skhan@linuxfoundation.org>
On Wed, 2020-01-29 at 19:01 -0700, Shuah Khan wrote:
> Change messages to messages to make it easier to debug. The following
> error message isn't informative enough to figure out what failed.
> Change messages to include function information.
>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
> .../integrity/platform_certs/load_powerpc.c | 14 ++++++++------
> security/integrity/platform_certs/load_uefi.c | 17 ++++++++++-------
> 2 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/security/integrity/platform_certs/load_powerpc.c b/security/integrity/platform_certs/load_powerpc.c
perhaps instead add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
so all the pr_<level> logging is more specific.
This would prefix all pr_<level> output with "integrity: "
3integrity: Couldn't get size: 0x%lx
3integrity: Error reading db var: 0x%lx
3integrity: MODSIGN: Couldn't get UEFI db list
3integrity: Couldn't parse db signatures: %d
6integrity: Couldn't get UEFI MokListRT
3integrity: Couldn't parse MokListRT signatures: %d
6integrity: Couldn't get UEFI dbx list
3integrity: Couldn't parse dbx signatures: %d
5integrity: Platform Keyring initialized
6integrity: Error adding keys to platform keyring %s
---
security/integrity/platform_certs/load_powerpc.c | 3 +++
security/integrity/platform_certs/load_uefi.c | 2 ++
security/integrity/platform_certs/platform_keyring.c | 2 ++
3 files changed, 7 insertions(+)
diff --git a/security/integrity/platform_certs/load_powerpc.c b/security/integrity/platform_certs/load_powerpc.c
index a2900c..5cfbd0 100644
--- a/security/integrity/platform_certs/load_powerpc.c
+++ b/security/integrity/platform_certs/load_powerpc.c
@@ -5,6 +5,9 @@
*
* - loads keys and hashes stored and controlled by the firmware.
*/
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include <linux/kernel.h>
#include <linux/sched.h>
#include <linux/cred.h>
diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index 111898a..480450a 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include <linux/kernel.h>
#include <linux/sched.h>
#include <linux/cred.h>
diff --git a/security/integrity/platform_certs/platform_keyring.c b/security/integrity/platform_certs/platform_keyring.c
index 7646e35..9bd2846 100644
--- a/security/integrity/platform_certs/platform_keyring.c
+++ b/security/integrity/platform_certs/platform_keyring.c
@@ -6,6 +6,8 @@
* Author(s): Nayna Jain <nayna@linux.ibm.com>
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include <linux/export.h>
#include <linux/kernel.h>
#include <linux/sched.h>
^ permalink raw reply related
* Re: [PATCH 1/1] proc_keys_next should increase position index
From: Jarkko Sakkinen @ 2020-01-30 8:38 UTC (permalink / raw)
To: Vasily Averin, keyrings, linux-security-module
Cc: David Howells, James Morris, Serge E. Hallyn
In-Reply-To: <af9dcaa7-6e4f-281a-2bae-fb605cc55d2d@virtuozzo.com>
On Fri, 2020-01-24 at 09:25 +0300, Vasily Averin wrote:
> if seq_file .next fuction does not change position index,
> read after some lseek can generate unexpected output.
>
> https://bugzilla.kernel.org/show_bug.cgi?id=206283
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
Similar comments as I gave here:
https://patchwork.kernel.org/patch/11346943/
/Jarkko
^ permalink raw reply
* Re: [PATCH 1/1] proc_keys_next should increase position index
From: Jarkko Sakkinen @ 2020-01-30 8:42 UTC (permalink / raw)
To: David Howells, Vasily Averin
Cc: keyrings, linux-security-module, James Morris, Serge E. Hallyn
In-Reply-To: <1451508.1580125174@warthog.procyon.org.uk>
On Mon, 2020-01-27 at 11:39 +0000, David Howells wrote:
> I don't see the effect you're talking about with /proc/keys. I see the
> following:
>
> [root@andromeda ~]# dd if=/proc/keys bs=40 skip=1
> dd: /proc/keys: cannot skip to specified offset
>
> and then it follows up with the normal content with no obvious duplicates (the
> lines are numbered ascendingly in the first column).
>
> I think I may be being confused by what you mean by "the last line".
>
> David
The commit message is completely lacking cause and effect. For
similar TPM commit I gave the following remarks:
https://patchwork.kernel.org/patch/11346943/
/Jarkko
^ permalink raw reply
* Re: [PATCH 1/8] tpm: initialize crypto_id of allocated_banks to HASH_ALGO__LAST
From: Jarkko Sakkinen @ 2020-01-30 8:47 UTC (permalink / raw)
To: Roberto Sassu, zohar, james.bottomley, linux-integrity
Cc: linux-security-module, linux-kernel, silviu.vlasceanu
In-Reply-To: <20200127170443.21538-2-roberto.sassu@huawei.com>
On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> chip->allocated_banks contains the list of TPM algorithm IDs of allocated
> PCR banks. It also contains the corresponding ID of the crypto subsystem,
> so that users of the TPM driver can calculate a digest for a PCR extend
> operation.
>
> However, if there is no mapping between TPM algorithm ID and crypto ID, the
> crypto_id field in chip->allocated_banks remains set to zero (the array is
> allocated and initialized with kcalloc() in tpm2_get_pcr_allocation()).
> Zero should not be used as value for unknown mappings, as it is a valid
> crypto ID (HASH_ALGO_MD4).
>
> This patch initializes crypto_id to HASH_ALGO__LAST.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>---
Remarks:
* After the subsystem tag, short summary starts with a capital lettter.
* Missing fixes tag and cc tag to stable.
* A struct called allocated_banks does not exist.
* Please prefer using an imperative sentence when describing the action
to take e.g. "Thus, initialize crypto_id to HASH_ALGO__LAST".
/Jarkko
^ permalink raw reply
* [PATCH v2] keys: proc_keys_next should increase position index
From: Vasily Averin @ 2020-01-30 10:16 UTC (permalink / raw)
To: keyrings, linux-security-module
Cc: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn
In-Reply-To: <eaacb0b2-fd0d-480e-1868-0a1284c20185@virtuozzo.com>
If seq_file .next fuction does not change position index,
read after some lseek can generate unexpected output:
$ dd if=/proc/keys bs=1 # full usual output
0f6bfdf5 I--Q--- 2 perm 3f010000 1000 1000 user 4af2f79ab8848d0a: 740
1fb91b32 I--Q--- 3 perm 1f3f0000 1000 65534 keyring _uid.1000: 2
27589480 I--Q--- 1 perm 0b0b0000 0 0 user invocation_id: 16
2f33ab67 I--Q--- 152 perm 3f030000 0 0 keyring _ses: 2
33f1d8fa I--Q--- 4 perm 3f030000 1000 1000 keyring _ses: 1
3d427fda I--Q--- 2 perm 3f010000 1000 1000 user 69ec44aec7678e5a: 740
3ead4096 I--Q--- 1 perm 1f3f0000 1000 65534 keyring _uid_ses.1000: 1
521+0 records in
521+0 records out
521 bytes copied, 0,00123769 s, 421 kB/s
$ dd if=/proc/keys bs=500 skip=1 # read after lseek in middle of last line
dd: /proc/keys: cannot skip to specified offset
g _uid_ses.1000: 1 <<<< end of last line
3ead4096 I--Q--- 1 perm 1f3f0000 1000 65534 keyring _uid_ses.1000: 1
<<<< and whole last lien again
0+1 records in
0+1 records out
97 bytes copied, 0,000135035 s, 718 kB/s
$ dd if=/proc/keys bs=1000 skip=1 # read after lseek beyond end of file
dd: /proc/keys: cannot skip to specified offset
3ead4096 I--Q--- 1 perm 1f3f0000 1000 65534 keyring _uid_ses.1000: 1
<<<< generates last line
0+1 records in
0+1 records out
76 bytes copied, 0,000119981 s, 633 kB/s
Cc: stable@vger.kernel.org
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
https://bugzilla.kernel.org/show_bug.cgi?id=206283
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
security/keys/proc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/security/keys/proc.c b/security/keys/proc.c
index 415f3f1..d0cde66 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -139,6 +139,8 @@ static void *proc_keys_next(struct seq_file *p, void *v, loff_t *_pos)
n = key_serial_next(p, v);
if (n)
*_pos = key_node_serial(n);
+ else
+ (*_pos)++;
return n;
}
--
1.8.3.1
^ permalink raw reply related
* RE: [PATCH 1/8] tpm: initialize crypto_id of allocated_banks to HASH_ALGO__LAST
From: Roberto Sassu @ 2020-01-30 16:11 UTC (permalink / raw)
To: Jarkko Sakkinen, zohar@linux.ibm.com,
james.bottomley@hansenpartnership.com,
linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <50afe1f50297b02af52621b6738ffff0c24f1bdf.camel@linux.intel.com>
> -----Original Message-----
> From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> Sent: Thursday, January 30, 2020 9:48 AM
> To: Roberto Sassu <roberto.sassu@huawei.com>; zohar@linux.ibm.com;
> james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> Subject: Re: [PATCH 1/8] tpm: initialize crypto_id of allocated_banks to
> HASH_ALGO__LAST
>
> On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > chip->allocated_banks contains the list of TPM algorithm IDs of allocated
> > PCR banks. It also contains the corresponding ID of the crypto subsystem,
> > so that users of the TPM driver can calculate a digest for a PCR extend
> > operation.
> >
> > However, if there is no mapping between TPM algorithm ID and crypto ID,
> the
> > crypto_id field in chip->allocated_banks remains set to zero (the array is
> > allocated and initialized with kcalloc() in tpm2_get_pcr_allocation()).
> > Zero should not be used as value for unknown mappings, as it is a valid
> > crypto ID (HASH_ALGO_MD4).
> >
> > This patch initializes crypto_id to HASH_ALGO__LAST.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>---
>
> Remarks:
>
> * After the subsystem tag, short summary starts with a capital lettter.
> * Missing fixes tag and cc tag to stable.
> * A struct called allocated_banks does not exist.
> * Please prefer using an imperative sentence when describing the action
> to take e.g. "Thus, initialize crypto_id to HASH_ALGO__LAST".
Thanks. I will fix these issues in the next version of the patch set.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
^ permalink raw reply
* Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection in the measurement list
From: Mimi Zohar @ 2020-01-30 22:26 UTC (permalink / raw)
To: Roberto Sassu, jarkko.sakkinen, james.bottomley, linux-integrity
Cc: linux-security-module, linux-kernel, silviu.vlasceanu
In-Reply-To: <20200127170443.21538-8-roberto.sassu@huawei.com>
On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> Before calculating a digest for each PCR bank, collisions were detected
> with a SHA1 digest. This patch includes ima_hash_algo among the algorithms
> used to calculate the template digest and checks collisions on that digest.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Definitely needed to protect against a sha1 collision attack.
<snip>
>
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index ebaf0056735c..a9bb45de6db9 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -51,7 +51,7 @@ int ima_alloc_init_template(struct ima_event_data *event_data,
> if (!*entry)
> return -ENOMEM;
>
> - (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
> + (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
> sizeof(*(*entry)->digests), GFP_NOFS);
> if (!(*entry)->digests) {
> result = -ENOMEM;
I would prefer not having to allocate and use "nr_allocated_banks + 1"
everywhere, but I understand the need for it. I'm not sure this patch
warrants allocating +2. Perhaps, if a TPM bank doesn't exist for the
IMA default hash algorithm, use a different algorithm or, worst case,
continue using the ima_sha1_idx.
Mimi
^ permalink raw reply
* Re: [PATCH 0/8] ima: support stronger algorithms for attestation
From: Mimi Zohar @ 2020-01-30 22:26 UTC (permalink / raw)
To: Roberto Sassu, jarkko.sakkinen, james.bottomley, linux-integrity
Cc: linux-security-module, linux-kernel, silviu.vlasceanu
In-Reply-To: <20200127170443.21538-1-roberto.sassu@huawei.com>
Hi Roberto,
On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> IMA extends Platform Configuration Registers (PCRs) of the TPM to give a
> proof to a remote verifier that the measurement list contains all
> measurements done by the kernel and that the list was not maliciously
> modified by an attacker.
>
> IMA was originally designed to extend PCRs with a SHA1 digest, provided
> with the measurement list, and was subsequently updated to extend all PCR
> banks in case a TPM 2.0 is used. Non-SHA1 PCR banks are not supposed to be
> used for remote attestation, as they are extended with a SHA1 digest padded
> with zeros, which does not increase the strength.
>
> This patch set addresses this issue by extending PCRs with the digest of
> the measurement entry calculated with the crypto subsystem. The list of
> algorithms used to calculate the digest are taken from
> ima_tpm_chip->allocated_banks, returned by the TPM driver. The SHA1 digest
> is always calculated, as SHA1 still remains the default algorithm for the
> template digest in the measurement list.
>
> This patch set also makes two additional modifications related to the usage
> of hash algorithms. First, since now the template digest for the default
> IMA algorithm is always calculated, this is used for hash collision
> detection, to check if there are duplicate measurement entries.
>
> Second, it uses the default IMA hash algorithm to calculate the boot
> aggregate, assuming that the corresponding PCR bank is currently allocated.
> Otherwise, it finds the first PCR bank for which the crypto ID is known.
> IMA initialization fails only if no algorithm known to the crypto subsystem
> is found.
>
> This patch set does not yet modify the format of the measurement list to
> provide the digests passed to the TPM. However, reconstructing the value of
> the quoted PCR is still possible for the verifier by calculating the digest
> on measurement data found in binary_runtime_measurements.
Thank you! I'm still reviewing and testing the patches, but it is
really nicely written.
Mimi
^ permalink raw reply
* Re: [PATCH 5/8] ima: allocate and initialize tfm for each PCR bank
From: Mimi Zohar @ 2020-01-31 12:18 UTC (permalink / raw)
To: Roberto Sassu, jarkko.sakkinen, james.bottomley, linux-integrity
Cc: linux-security-module, linux-kernel, silviu.vlasceanu
In-Reply-To: <20200127170443.21538-6-roberto.sassu@huawei.com>
On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> This patch creates a crypto_shash structure for each allocated PCR bank and
> for SHA1 if a bank with that algorithm is not currently allocated.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
<snip>
> +int __init ima_init_crypto(void)
> +{
> + enum hash_algo algo;
> + long rc;
> + int i;
> +
> + rc = ima_init_ima_crypto();
> + if (rc)
> + return rc;
> +
> + ima_algo_array = kmalloc_array(ima_tpm_chip->nr_allocated_banks + 1,
> + sizeof(*ima_algo_array), GFP_KERNEL);
Perhaps instead of hard coding "nr_allocated_banks + 1", create a
variable for storing the number of "extra" banks. Walk the banks
setting ima_sha1_idx and, later, in a subsequent patch set
ima_hash_algo_idx.
> + if (!ima_algo_array) {
> + rc = -ENOMEM;
> + goto out;
> + }
> +
> + for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
> + ima_algo_array[i].tfm = NULL;
> + ima_algo_array[i].algo = HASH_ALGO__LAST;
> + }
ima_init_crypto() is executed once on initialization. I'm not sure if
it makes a difference, but if you're really concerned about an
additional loop, the initialization, here, could be limited to the
"extra" banks. The other banks could be initialized at the beginning
of the next loop.
thanks,
Mimi
> + ima_sha1_idx = -1;
> +
> + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
> + algo = ima_tpm_chip->allocated_banks[i].crypto_id;
> + ima_algo_array[i].algo = algo;
> +
> + /* unknown TPM algorithm */
> + if (algo == HASH_ALGO__LAST)
> + continue;
> +
> + if (algo == HASH_ALGO_SHA1)
> + ima_sha1_idx = i;
> +
> + if (algo == ima_hash_algo) {
> + ima_algo_array[i].tfm = ima_shash_tfm;
> + continue;
> + }
> +
> + ima_algo_array[i].tfm = ima_alloc_tfm(algo);
> + if (IS_ERR(ima_algo_array[i].tfm)) {
> + if (algo == HASH_ALGO_SHA1) {
> + rc = PTR_ERR(ima_algo_array[i].tfm);
> + ima_algo_array[i].tfm = NULL;
> + goto out_array;
> + }
> +
> + ima_algo_array[i].tfm = NULL;
> + }
> + }
> +
> + if (ima_sha1_idx < 0) {
> + ima_algo_array[i].tfm = ima_alloc_tfm(HASH_ALGO_SHA1);
> + if (IS_ERR(ima_algo_array[i].tfm)) {
> + rc = PTR_ERR(ima_algo_array[i].tfm);
> + goto out_array;
> + }
> +
> + ima_sha1_idx = i;
> + ima_algo_array[i].algo = HASH_ALGO_SHA1;
> + }
> +
> + return 0;
> +out_array:
> + for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
> + if (!ima_algo_array[i].tfm ||
> + ima_algo_array[i].tfm == ima_shash_tfm)
> + continue;
> +
> + crypto_free_shash(ima_algo_array[i].tfm);
> + }
> +out:
> + crypto_free_shash(ima_shash_tfm);
> + return rc;
> +}
^ permalink raw reply
* Re: [PATCH 1/8] tpm: initialize crypto_id of allocated_banks to HASH_ALGO__LAST
From: Mimi Zohar @ 2020-01-31 13:33 UTC (permalink / raw)
To: Roberto Sassu, Jarkko Sakkinen,
james.bottomley@hansenpartnership.com,
linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <8c15cf66708a4d38916b8ca39f26b5f6@huawei.com>
On Thu, 2020-01-30 at 16:11 +0000, Roberto Sassu wrote:
> > -----Original Message-----
> > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> > Sent: Thursday, January 30, 2020 9:48 AM
> > To: Roberto Sassu <roberto.sassu@huawei.com>; zohar@linux.ibm.com;
> > james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> > Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> > Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> > Subject: Re: [PATCH 1/8] tpm: initialize crypto_id of allocated_banks to
> > HASH_ALGO__LAST
> >
> > On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > > chip->allocated_banks contains the list of TPM algorithm IDs of allocated
> > > PCR banks. It also contains the corresponding ID of the crypto subsystem,
> > > so that users of the TPM driver can calculate a digest for a PCR extend
> > > operation.
> > >
> > > However, if there is no mapping between TPM algorithm ID and crypto ID,
> > the
> > > crypto_id field in chip->allocated_banks remains set to zero (the array is
> > > allocated and initialized with kcalloc() in tpm2_get_pcr_allocation()).
> > > Zero should not be used as value for unknown mappings, as it is a valid
> > > crypto ID (HASH_ALGO_MD4).
> > >
> > > This patch initializes crypto_id to HASH_ALGO__LAST.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>---
> >
> > Remarks:
> >
> > * After the subsystem tag, short summary starts with a capital lettter.
> > * Missing fixes tag and cc tag to stable.
> > * A struct called allocated_banks does not exist.
> > * Please prefer using an imperative sentence when describing the action
> > to take e.g. "Thus, initialize crypto_id to HASH_ALGO__LAST".
>
> Thanks. I will fix these issues in the next version of the patch set.
Jarkko, I realize this is a TPM patch, but this patch set is dependent
on it. When this patch is ready, could you create a topic branch,
which both of us could merge?
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH 2/8] ima: evaluate error in init_ima()
From: Mimi Zohar @ 2020-01-31 13:33 UTC (permalink / raw)
To: Roberto Sassu, jarkko.sakkinen, james.bottomley, linux-integrity
Cc: linux-security-module, linux-kernel, silviu.vlasceanu
In-Reply-To: <20200127170443.21538-3-roberto.sassu@huawei.com>
On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> Evaluate error in init_ima() before register_blocking_lsm_notifier() and
> return if not zero.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Thanks. Please include a "Fixes" tag.
Mimi
^ permalink raw reply
* RE: [PATCH 5/8] ima: allocate and initialize tfm for each PCR bank
From: Roberto Sassu @ 2020-01-31 13:42 UTC (permalink / raw)
To: Mimi Zohar, jarkko.sakkinen@linux.intel.com,
james.bottomley@hansenpartnership.com,
linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <1580473133.6104.48.camel@linux.ibm.com>
> -----Original Message-----
> From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> owner@vger.kernel.org] On Behalf Of Mimi Zohar
> Sent: Friday, January 31, 2020 1:19 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> jarkko.sakkinen@linux.intel.com;
> james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> Subject: Re: [PATCH 5/8] ima: allocate and initialize tfm for each PCR bank
>
> On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > This patch creates a crypto_shash structure for each allocated PCR bank
> and
> > for SHA1 if a bank with that algorithm is not currently allocated.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>
> <snip>
>
> > +int __init ima_init_crypto(void)
> > +{
> > + enum hash_algo algo;
> > + long rc;
> > + int i;
> > +
> > + rc = ima_init_ima_crypto();
> > + if (rc)
> > + return rc;
> > +
> > + ima_algo_array = kmalloc_array(ima_tpm_chip-
> >nr_allocated_banks + 1,
> > + sizeof(*ima_algo_array), GFP_KERNEL);
>
> Perhaps instead of hard coding "nr_allocated_banks + 1", create a
> variable for storing the number of "extra" banks. Walk the banks
> setting ima_sha1_idx and, later, in a subsequent patch set
> ima_hash_algo_idx.
I could store the indexes in an array and add ARRAY_SIZE of that array.
> > + if (!ima_algo_array) {
> > + rc = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
> > + ima_algo_array[i].tfm = NULL;
> > + ima_algo_array[i].algo = HASH_ALGO__LAST;
> > + }
>
> ima_init_crypto() is executed once on initialization. I'm not sure if
> it makes a difference, but if you're really concerned about an
> additional loop, the initialization, here, could be limited to the
> "extra" banks. The other banks could be initialized at the beginning
> of the next loop.
I set algo to HASH_ALGO__LAST to mark the ima_algo_array entries as
uninitialized. ima_alloc_tfm() uses ima_algo_array in the next loop.
Replacing kmalloc_array() with kcalloc() would cause algo to be set to
HASH_ALGO_MD4.
I could check tfm first, which ensures that also algo was initialized.
Roberto
> > + ima_sha1_idx = -1;
> > +
> > + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
> > + algo = ima_tpm_chip->allocated_banks[i].crypto_id;
> > + ima_algo_array[i].algo = algo;
> > +
> > + /* unknown TPM algorithm */
> > + if (algo == HASH_ALGO__LAST)
> > + continue;
> > +
> > + if (algo == HASH_ALGO_SHA1)
> > + ima_sha1_idx = i;
> > +
> > + if (algo == ima_hash_algo) {
> > + ima_algo_array[i].tfm = ima_shash_tfm;
> > + continue;
> > + }
> > +
> > + ima_algo_array[i].tfm = ima_alloc_tfm(algo);
> > + if (IS_ERR(ima_algo_array[i].tfm)) {
> > + if (algo == HASH_ALGO_SHA1) {
> > + rc = PTR_ERR(ima_algo_array[i].tfm);
> > + ima_algo_array[i].tfm = NULL;
> > + goto out_array;
> > + }
> > +
> > + ima_algo_array[i].tfm = NULL;
> > + }
> > + }
> > +
> > + if (ima_sha1_idx < 0) {
> > + ima_algo_array[i].tfm = ima_alloc_tfm(HASH_ALGO_SHA1);
> > + if (IS_ERR(ima_algo_array[i].tfm)) {
> > + rc = PTR_ERR(ima_algo_array[i].tfm);
> > + goto out_array;
> > + }
> > +
> > + ima_sha1_idx = i;
> > + ima_algo_array[i].algo = HASH_ALGO_SHA1;
> > + }
> > +
> > + return 0;
> > +out_array:
> > + for (i = 0; i < ima_tpm_chip->nr_allocated_banks + 1; i++) {
> > + if (!ima_algo_array[i].tfm ||
> > + ima_algo_array[i].tfm == ima_shash_tfm)
> > + continue;
> > +
> > + crypto_free_shash(ima_algo_array[i].tfm);
> > + }
> > +out:
> > + crypto_free_shash(ima_shash_tfm);
> > + return rc;
> > +}
^ permalink raw reply
* Re: [PATCH 5/8] ima: allocate and initialize tfm for each PCR bank
From: Mimi Zohar @ 2020-01-31 13:58 UTC (permalink / raw)
To: Roberto Sassu, jarkko.sakkinen@linux.intel.com,
james.bottomley@hansenpartnership.com,
linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <17b00b78aa2249f19ba376c7613cfb38@huawei.com>
On Fri, 2020-01-31 at 13:42 +0000, Roberto Sassu wrote:
> > -----Original Message-----
> > From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> > owner@vger.kernel.org] On Behalf Of Mimi Zohar
> > Sent: Friday, January 31, 2020 1:19 PM
> > To: Roberto Sassu <roberto.sassu@huawei.com>;
> > jarkko.sakkinen@linux.intel.com;
> > james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> > Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> > Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> > Subject: Re: [PATCH 5/8] ima: allocate and initialize tfm for each PCR bank
> >
> > On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > > This patch creates a crypto_shash structure for each allocated PCR bank
> > and
> > > for SHA1 if a bank with that algorithm is not currently allocated.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> >
> > <snip>
> >
> > > +int __init ima_init_crypto(void)
> > > +{
> > > + enum hash_algo algo;
> > > + long rc;
> > > + int i;
> > > +
> > > + rc = ima_init_ima_crypto();
> > > + if (rc)
> > > + return rc;
> > > +
> > > + ima_algo_array = kmalloc_array(ima_tpm_chip-
> > >nr_allocated_banks + 1,
> > > + sizeof(*ima_algo_array), GFP_KERNEL);
> >
> > Perhaps instead of hard coding "nr_allocated_banks + 1", create a
> > variable for storing the number of "extra" banks. Walk the banks
> > setting ima_sha1_idx and, later, in a subsequent patch set
> > ima_hash_algo_idx.
>
> I could store the indexes in an array and add ARRAY_SIZE of that array.
Your current patches are straight forward and easy to review. I
really like that. What I'm proposing is a minor change, at least I
think it is a minor change. I trust whatever change you decide to
make will be just as easy to review.
Before re-posting, please define ima_sha1_idx and ima_hash_algo_idx as
__ro_after_init.
thanks,
Mimi
^ permalink raw reply
* RE: [PATCH 7/8] ima: use ima_hash_algo for collision detection in the measurement list
From: Roberto Sassu @ 2020-01-31 14:02 UTC (permalink / raw)
To: Mimi Zohar, jarkko.sakkinen@linux.intel.com,
james.bottomley@hansenpartnership.com,
linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <1580423169.6104.18.camel@linux.ibm.com>
> -----Original Message-----
> From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> owner@vger.kernel.org] On Behalf Of Mimi Zohar
> Sent: Thursday, January 30, 2020 11:26 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> jarkko.sakkinen@linux.intel.com;
> james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> Subject: Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection in
> the measurement list
>
> On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > Before calculating a digest for each PCR bank, collisions were detected
> > with a SHA1 digest. This patch includes ima_hash_algo among the
> algorithms
> > used to calculate the template digest and checks collisions on that digest.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>
> Definitely needed to protect against a sha1 collision attack.
>
> <snip>
>
> >
> > diff --git a/security/integrity/ima/ima_api.c
> b/security/integrity/ima/ima_api.c
> > index ebaf0056735c..a9bb45de6db9 100644
> > --- a/security/integrity/ima/ima_api.c
> > +++ b/security/integrity/ima/ima_api.c
> > @@ -51,7 +51,7 @@ int ima_alloc_init_template(struct ima_event_data
> *event_data,
> > if (!*entry)
> > return -ENOMEM;
> >
> > - (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
> > + (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
> > sizeof(*(*entry)->digests), GFP_NOFS);
> > if (!(*entry)->digests) {
> > result = -ENOMEM;
>
> I would prefer not having to allocate and use "nr_allocated_banks + 1"
> everywhere, but I understand the need for it. I'm not sure this patch
> warrants allocating +2. Perhaps, if a TPM bank doesn't exist for the
> IMA default hash algorithm, use a different algorithm or, worst case,
> continue using the ima_sha1_idx.
We could introduce a new option called ima_hash_algo_tpm to specify
the algorithm of an allocated bank. We can use this for boot_aggregate
and hash collision detection.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
^ permalink raw reply
* Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection in the measurement list
From: Mimi Zohar @ 2020-01-31 14:22 UTC (permalink / raw)
To: Roberto Sassu, jarkko.sakkinen@linux.intel.com,
james.bottomley@hansenpartnership.com,
linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <44c1b3f6d3fe414e914317ef8e5c6f8f@huawei.com>
On Fri, 2020-01-31 at 14:02 +0000, Roberto Sassu wrote:
> > -----Original Message-----
> > From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> > owner@vger.kernel.org] On Behalf Of Mimi Zohar
> > Sent: Thursday, January 30, 2020 11:26 PM
> > To: Roberto Sassu <roberto.sassu@huawei.com>;
> > jarkko.sakkinen@linux.intel.com;
> > james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> > Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> > Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> > Subject: Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection in
> > the measurement list
> >
> > On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > > Before calculating a digest for each PCR bank, collisions were detected
> > > with a SHA1 digest. This patch includes ima_hash_algo among the
> > algorithms
> > > used to calculate the template digest and checks collisions on that digest.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> >
> > Definitely needed to protect against a sha1 collision attack.
> >
> > <snip>
> >
> > >
> > > diff --git a/security/integrity/ima/ima_api.c
> > b/security/integrity/ima/ima_api.c
> > > index ebaf0056735c..a9bb45de6db9 100644
> > > --- a/security/integrity/ima/ima_api.c
> > > +++ b/security/integrity/ima/ima_api.c
> > > @@ -51,7 +51,7 @@ int ima_alloc_init_template(struct ima_event_data
> > *event_data,
> > > if (!*entry)
> > > return -ENOMEM;
> > >
> > > - (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
> > > + (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
> > > sizeof(*(*entry)->digests), GFP_NOFS);
> > > if (!(*entry)->digests) {
> > > result = -ENOMEM;
> >
> > I would prefer not having to allocate and use "nr_allocated_banks + 1"
> > everywhere, but I understand the need for it. I'm not sure this patch
> > warrants allocating +2. Perhaps, if a TPM bank doesn't exist for the
> > IMA default hash algorithm, use a different algorithm or, worst case,
> > continue using the ima_sha1_idx.
>
> We could introduce a new option called ima_hash_algo_tpm to specify
> the algorithm of an allocated bank. We can use this for boot_aggregate
> and hash collision detection.
I don't think that would work in the case where the IMA default hash
is set to sha256, but the system has a TPM 1.2 chip. We would be left
using SHA1 for the file hash collision detection.
With my suggestion of defining an "extra" variable, I kind of back
tracked here. There are two problems that I'm trying to address -
hard coding the number of additional "banks" and unnecessarily
allocating more memory than necessary. By pre-walking the list,
calculating the "extra" banks, you'll resolve both issues.
Mimi
^ permalink raw reply
* RE: [PATCH 7/8] ima: use ima_hash_algo for collision detection in the measurement list
From: Roberto Sassu @ 2020-01-31 14:41 UTC (permalink / raw)
To: Mimi Zohar, jarkko.sakkinen@linux.intel.com,
james.bottomley@hansenpartnership.com,
linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <1580480525.6104.88.camel@linux.ibm.com>
> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Friday, January 31, 2020 3:22 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> jarkko.sakkinen@linux.intel.com;
> james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> Subject: Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection in
> the measurement list
>
> On Fri, 2020-01-31 at 14:02 +0000, Roberto Sassu wrote:
> > > -----Original Message-----
> > > From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> > > owner@vger.kernel.org] On Behalf Of Mimi Zohar
> > > Sent: Thursday, January 30, 2020 11:26 PM
> > > To: Roberto Sassu <roberto.sassu@huawei.com>;
> > > jarkko.sakkinen@linux.intel.com;
> > > james.bottomley@hansenpartnership.com; linux-
> integrity@vger.kernel.org
> > > Cc: linux-security-module@vger.kernel.org; linux-
> kernel@vger.kernel.org;
> > > Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> > > Subject: Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection
> in
> > > the measurement list
> > >
> > > On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > > > Before calculating a digest for each PCR bank, collisions were detected
> > > > with a SHA1 digest. This patch includes ima_hash_algo among the
> > > algorithms
> > > > used to calculate the template digest and checks collisions on that
> digest.
> > > >
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > >
> > > Definitely needed to protect against a sha1 collision attack.
> > >
> > > <snip>
> > >
> > > >
> > > > diff --git a/security/integrity/ima/ima_api.c
> > > b/security/integrity/ima/ima_api.c
> > > > index ebaf0056735c..a9bb45de6db9 100644
> > > > --- a/security/integrity/ima/ima_api.c
> > > > +++ b/security/integrity/ima/ima_api.c
> > > > @@ -51,7 +51,7 @@ int ima_alloc_init_template(struct
> ima_event_data
> > > *event_data,
> > > > if (!*entry)
> > > > return -ENOMEM;
> > > >
> > > > - (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1,
> > > > + (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2,
> > > > sizeof(*(*entry)->digests), GFP_NOFS);
> > > > if (!(*entry)->digests) {
> > > > result = -ENOMEM;
> > >
> > > I would prefer not having to allocate and use "nr_allocated_banks + 1"
> > > everywhere, but I understand the need for it. I'm not sure this patch
> > > warrants allocating +2. Perhaps, if a TPM bank doesn't exist for the
> > > IMA default hash algorithm, use a different algorithm or, worst case,
> > > continue using the ima_sha1_idx.
> >
> > We could introduce a new option called ima_hash_algo_tpm to specify
> > the algorithm of an allocated bank. We can use this for boot_aggregate
> > and hash collision detection.
>
> I don't think that would work in the case where the IMA default hash
> is set to sha256, but the system has a TPM 1.2 chip. We would be left
> using SHA1 for the file hash collision detection.
I thought that using a stronger algorithm for hash collision detection but
doing remote attestation with the weaker would not bring additional value.
If there is a hash collision on SHA1, an attacker can still replace the data of
one of the two entries in the measurement list with the data of the other
without being detected (without additional countermeasures).
If the verifier additionally checks for duplicate template digests, he could
detect the attack (IMA would not add a new measurement entry with the
same template digest of previous entries).
Ok, I will use ima_hash_algo for hash collision detection.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
^ permalink raw reply
* Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection in the measurement list
From: Mimi Zohar @ 2020-01-31 14:50 UTC (permalink / raw)
To: Roberto Sassu, jarkko.sakkinen@linux.intel.com,
james.bottomley@hansenpartnership.com,
linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <e299058034b94143af6fc1da4ae2c708@huawei.com>
On Fri, 2020-01-31 at 14:41 +0000, Roberto Sassu wrote:
> I thought that using a stronger algorithm for hash collision detection but
> doing remote attestation with the weaker would not bring additional value.
>
> If there is a hash collision on SHA1, an attacker can still replace the data of
> one of the two entries in the measurement list with the data of the other
> without being detected (without additional countermeasures).
>
> If the verifier additionally checks for duplicate template digests, he could
> detect the attack (IMA would not add a new measurement entry with the
> same template digest of previous entries).
>
> Ok, I will use ima_hash_algo for hash collision detection.
Thanks!
Mimi
^ permalink raw reply
* RE: [PATCH 8/8] ima: switch to ima_hash_algo for boot aggregate
From: Roberto Sassu @ 2020-01-31 15:21 UTC (permalink / raw)
To: zohar@linux.ibm.com, jarkko.sakkinen@linux.intel.com,
james.bottomley@hansenpartnership.com,
linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <20200127170443.21538-9-roberto.sassu@huawei.com>
> -----Original Message-----
> From: Roberto Sassu
> Sent: Monday, January 27, 2020 6:05 PM
> To: zohar@linux.ibm.com; jarkko.sakkinen@linux.intel.com;
> james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>; Roberto Sassu
> <roberto.sassu@huawei.com>
> Subject: [PATCH 8/8] ima: switch to ima_hash_algo for boot aggregate
I will remove this patch from the patch set.
[PATCH v3 1/2] ima: support calculating the boot aggregate based on non-SHA1 algorithms
[PATCH v3 2/2] ima: support calculating the boot_aggregate based on different TPM banks
by Mimi will provide similar functionality.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
^ permalink raw reply
* Re: [PATCH v2] keys: proc_keys_next should increase position index
From: David Howells @ 2020-01-31 17:19 UTC (permalink / raw)
To: Vasily Averin
Cc: dhowells, keyrings, linux-security-module, Jarkko Sakkinen,
James Morris, Serge E. Hallyn
In-Reply-To: <22307ecd-3254-6077-8bc7-02693338b586@virtuozzo.com>
Vasily Averin <vvs@virtuozzo.com> wrote:
> If seq_file .next fuction does not change position index,
"function"
> <<<< and whole last lien again
"line"
I can fix these up for you.
> https://bugzilla.kernel.org/show_bug.cgi?id=206283
I wonder if this should have a tag - it looks kind of out of place without
one, but I can't find one suggested.
David
^ permalink raw reply
* Re: linux-next: Tree for Jan 29 (security/smack/)
From: David Howells @ 2020-01-31 17:35 UTC (permalink / raw)
To: Casey Schaufler
Cc: dhowells, Randy Dunlap, Stephen Rothwell, Linux Next Mailing List,
Linux Kernel Mailing List, linux-security-module
In-Reply-To: <3ac1c817-b310-c8f8-6990-1602db540106@schaufler-ca.com>
Casey Schaufler <casey@schaufler-ca.com> wrote:
> This keeps coming up. It's in David Howells' watch queue changes.
> David, could you please fix this?
That should be fixed now in my keys-next branch.
David
^ permalink raw reply
* Re: Perf Data on LSM in v5.3
From: Casey Schaufler @ 2020-01-31 19:15 UTC (permalink / raw)
To: Wenhui Zhang, Stephen Smalley
Cc: John Johansen, Casey Schaufler, James Morris,
linux-security-module, SELinux, Kees Cook, penguin-kernel,
Paul Moore, Casey Schaufler
In-Reply-To: <CAOSEQ1rOQ50WjvvUSeVpf0RREenP_59u34yx1YQE1YdigzOXcg@mail.gmail.com>
On 1/31/2020 11:08 AM, Wenhui Zhang wrote:
> Hi, Smalley:
> DAC, MAC and SELinux's performance data is performed, and it seems like our conclusion is consistent with our previous evaluation.
> Please see here (configuration files are included as well):
> 5.3.0-results <https://drive.google.com/drive/folders/1NPkHYoffPnkvMlXIM5ytrqzBThLwXx86>
> I am trying to test other modules (SMACK, Apparmor, Integrity etc. )
>
> However what confused me a lot is the Kconfig file in ./linux/security/Kconfig:
> config LSM
> string "Ordered list of enabled LSMs"
> default "newmodule,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor" if DEFAULT_SECURITY_SMACK
> default "newmodule,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo" if DEFAULT_SECURITY_APPARMOR
> default "newmodule,yama,loadpin,safesetid,integrity,tomoyo" if DEFAULT_SECURITY_TOMOYO
> default "newmodule,yama,loadpin,safesetid,integrity" if DEFAULT_SECURITY_DAC
> default "newmodule,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor"
> IMHO, it seems like it is a little confusing, changing it to below maybe make it a lilttle more clear:
> config LSM
> string "Ordered list of enabled LSMs"
> default "newmodule,yama,loadpin,safesetid,integrity, tomoyo, apparmor, selinux, smack" if DEFAULT_SECURITY_SMACK
> default "newmodule,yama,loadpin,safesetid,integrity,tomoyo, smack, selinux, apparmor" if DEFAULT_SECURITY_APPARMOR
> default "newmodule,yama,loadpin,safesetid,integrity,tomoyo" if DEFAULT_SECURITY_TOMOYO
> default "" if DEFAULT_SECURITY_DAC *# could we leave this to empty string*
> default "newmodule,yama,loadpin,safesetid,integrity,*selinux,smack,tomoyo,apparmor" # on Ubuntu, apparmor be the the dedault, however on centos etc, maybe selinux be the default, on andriod smack as default *
> Any suggestions on clean up the code on this part please?
>
The ordering is historical. The "major" security modules are
listed in the order they where accepted into Linus' tree.
BTW, as much as I'd like it to be true, Android would not use
Smack as its default.
>
> On Fri, Jan 24, 2020 at 9:56 AM Stephen Smalley <sds@tycho.nsa.gov <mailto:sds@tycho.nsa.gov>> wrote:
>
> On 1/15/20 11:04 AM, Stephen Smalley wrote:
> > On 1/15/20 10:59 AM, Wenhui Zhang wrote:
> >>
> >>
> >> On Wed, Jan 15, 2020 at 10:41 AM Stephen Smalley <sds@tycho.nsa.gov <mailto:sds@tycho.nsa.gov>
> >> <mailto:sds@tycho.nsa.gov <mailto:sds@tycho.nsa.gov>>> wrote:
> >>
> >> On 1/15/20 10:34 AM, Stephen Smalley wrote:
> >> > On 1/15/20 10:21 AM, Wenhui Zhang wrote:
> >> >>
> >> >> On Wed, Jan 15, 2020 at 9:08 AM Stephen Smalley
> >> <sds@tycho.nsa.gov <mailto:sds@tycho.nsa.gov> <mailto:sds@tycho.nsa.gov <mailto:sds@tycho.nsa.gov>>
> >> >> <mailto:sds@tycho.nsa.gov <mailto:sds@tycho.nsa.gov> <mailto:sds@tycho.nsa.gov <mailto:sds@tycho.nsa.gov>>>> wrote:
> >> >>
> >> >> On 1/15/20 8:40 AM, Stephen Smalley wrote:
> >> >> > On 1/14/20 8:00 PM, Wenhui Zhang wrote:
> >> >> >> Hi, John:
> >> >> >>
> >> >> >> It seems like, the MAC hooks are default to*return 0 or
> >> empty
> >> >> void
> >> >> >> hooks* if CONFIG_SECURITY, CONFIG_SECURITY_NETWORK ,
> >> >> >> CONFIG_PAGE_TABLE_ISOLATION, CONFIG_SECURITY_INFINIBAND,
> >> >> >> CONFIG_SECURITY_PATH, CONFIG_INTEL_TXT,
> >> >> >> CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR,
> >> >> >>
> >> CONFIG_HARDENED_USERCOPY, CONFIG_HARDENED_USERCOPY_FALLBACK *are
> >> >> NOT
> >> >> >> set*.
> >> >> >>
> >> >> >> If HOOKs are "return 0 or empty void hooks", MAC is not
> >> enabled.
> >> >> >> In runtime of fs-benchmarks,
> >> if CONFIG_DEFAULT_SECURITY_DAC=y,
> >> >> then
> >> >> >> capability is enabled.
> >> >> >>
> >> >> >> Please correct me if I am wrong.
> >> >> >>
> >> >> >> For the first test, wo-sec is tested with:
> >> >> >> # CONFIG_SECURITY_DMESG_RESTRICT is not set
> >> >> >> # CONFIG_SECURITY is not set
> >> >> >> # CONFIG_SECURITYFS is not set
> >> >> >> # CONFIG_PAGE_TABLE_ISOLATION is not set
> >> >> >> # CONFIG_INTEL_TXT is not set
> >> >> >> CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR=y
> >> >> >> # CONFIG_HARDENED_USERCOPY is not set
> >> >> >> CONFIG_FORTIFY_SOURCE=y
> >> >> >> # CONFIG_STATIC_USERMODEHELPER is not set
> >> >> >> CONFIG_DEFAULT_SECURITY_DAC=y
> >> >> >>
> >> >> >>
> >> >> >> For the second test, w-sec is tested with:
> >> >> >> # CONFIG_SECURITY_DMESG_RESTRICT is not set
> >> >> >> CONFIG_SECURITY=y
> >> >> >> CONFIG_SECURITYFS=y
> >> >> >> # CONFIG_SECURITY_NETWORK is not set
> >> >> >> CONFIG_PAGE_TABLE_ISOLATION=y
> >> >> >> CONFIG_SECURITY_INFINIBAND=y
> >> >> >> CONFIG_SECURITY_PATH=y
> >> >> >> CONFIG_INTEL_TXT=y
> >> >> >> CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR=y
> >> >> >> CONFIG_HARDENED_USERCOPY=y
> >> >> >> CONFIG_HARDENED_USERCOPY_FALLBACK=y
> >> >> >> # CONFIG_HARDENED_USERCOPY_PAGESPAN is not set
> >> >> >> CONFIG_FORTIFY_SOURCE=y
> >> >> >> # CONFIG_STATIC_USERMODEHELPER is not set
> >> >> >> # CONFIG_SECURITY_SMACK is not set
> >> >> >> # CONFIG_SECURITY_TOMOYO is not set
> >> >> >> # CONFIG_SECURITY_APPARMOR is not set
> >> >> >> # CONFIG_SECURITY_LOADPIN is not set
> >> >> >> # CONFIG_SECURITY_YAMA is not set
> >> >> >> # CONFIG_SECURITY_SAFESETID is not set
> >> >> >> # CONFIG_INTEGRITY is not set
> >> >> >> CONFIG_DEFAULT_SECURITY_DAC=y
> >> >> >> #
> >> >> >>
> >> >>
> >> >>
> >>
> >> CONFIG_LSM="yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo"
> >>
> >>
> >> >>
> >> >>
> >> >> >>
> >> >> >
> >> >> > Your configs should only differ with respect to
> >> CONFIG_SECURITY*
> >> >> if you
> >> >> > want to evaluate LSM, SELinux, etc overheads.
> >> >> PAGE_TABLE_ISOLATION,
> >> >> > INTEL_TXT, and HARDENED_USERCOPY are not relevant to LSM
> >> itself.
> >> >> >
> >> >> > Also, what benchmarks are you using? Your own home-grown
> >> ones, a
> >> >> set of
> >> >> > open source standard benchmarks (if so, which ones?).
> >> You should
> >> >> > include both micro and macro benchmarks in your suite.
> >> >> >
> >> >> > How stable are your results? What kind of variance /
> >> standard
> >> >> deviation
> >> >> > are you seeing?
> >> >> >
> >> >> > It is hard to get meaningful, reliable performance
> >> measurements
> >> >> so going
> >> >> > down this road is not to be done lightly.
> >> >>
> >> >> Also, I note that you don't enable CONFIG_SECURITY_NETWORK
> >> above.
> >> >> That
> >> >> means you aren't including the base LSM overhead for the
> >> networking
> >> >> security hooks. So if you then compare that against SELinux
> >> (which
> >> >> requires CONFIG_SECURITY_NETWORK), you are going to end up
> >> >> attributing
> >> >> the cost of both the LSM overhead and SELinux overhead all to
> >> >> SELinux.
> >> >> If you truly want to isolate the base LSM overhead, you
> >> need to
> >> >> enable
> >> >> all the hooks.
> >> >>
> >> >> I will give it a try for enabling CONFIG_SECURITY_NETWORK later
> >> this
> >> >> week, however I wonder if this would affect the test results
> >> that much.
> >> >> I am testing with LMBench 2.5 , with focusing on filesystem unit
> >> >> tests, however not network stack at this time.
> >> >> My understanding of why this result is so different from previous
> >> >> paper 20 years ago, is that the Bottleneck changes.
> >> >> As Chris was testing with 4 cores , each 700MHz CPU, and 128MB
> >> memory,
> >> >> with HDD (latency is about 20,000,000 ns for sequential read).
> >> >> The Bottleneck of accessing files w/ MAC are mostly on I/O.
> >> >> However hardware setup is different now, we have much larger and
> >> >> faster memory (better prefetching as well), with SSD (latency is
> >> about
> >> >> 49,000 ns for sequential read). , while CPU speed is not
> >> increasing as
> >> >> much as that of I/O.
> >> >> The Bottleneck of accessing files w/ MAC are mostly on CPU now.
> >> >
> >> > Don't know if lmbench is still a good benchmark and I recall
> >> struggling
> >> > with it even back then to get stable results.
> >> >
> >> > Could be bottleneck changes, could be the fact that your kernel
> >> config
> >> > changes aren't limited to CONFIG_SECURITY* (e.g. PTI introduces
> >> > non-trivial overheads), could be changes to LSM since that time
> >> (e.g.
> >> > stacking support, moving security_ calls out-of-line, more hooks,
> >> ...),
> >> > could be that running SELinux w/o policy is flooding the system
> >> logs
> >> > with warnings or other messages since it wasn't really designed
> >> to be
> >> > used that way past initialization. Lots of options, can't tell
> >> without
> >> > more info on your details.
> >>
> >> I'd think that these days one would leverage perf and/or lkp for
> >> Linux
> >> kernel performance measurements, not lmbench.
> >>
> >>
> >> Thanks so much, I will give it a try for lkp and let you know how it
> >> goes.
> >> Maybe later next week or this weekend, we should have the results.
> >
> > Ok, please make sure your kernel configs are truly comparable (i.e. no
> > differences other than the right set of CONFIG_SECURITY* options), that
> > all of the same LSM hooks are enabled for comparing LSM-only versus
> > SELinux (i.e. CONFIG_SECURITY and CONFIG_SECURITY_NETWORK both enabled),
> > and consider using a distribution that actually supports SELinux out of
> > the box (e.g. Fedora) so that you can properly test SELinux with a
> > policy loaded in enforcing mode. Similarly if you want to do the same
> > for AppArmor, except for it you'll need to enable CONFIG_SECURITY_PATH
> > as well for the pathname-based hooks and you'll want to use Ubuntu or
> > latest Debian to get a working policy.
>
> One last point that I should have mentioned: you should likely run your
> benchmarks both under an unconfined profile/domain and under a confined
> profile/domain (the latter could be one that you define specifically for
> your benchmark that just allows it everything) at least for AppArmor.
> The reason is that AppArmor has an intrinsic concept of unconfined since
> it was designed for targeted enforcement and its hook functions directly
> test for the unconfined label early and skip permission checking in that
> case, so if you only collect data while running benchmarks unconfined,
> you won't see the real overheads imposed on a confined profile/domain.
> In contrast, SELinux has no intrinsic concept of unconfined since it was
> designed for full system confinement (as applied in "strict"
> configurations and in Android); if there is an unconfined domain, it is
> merely defined through policy (i.e. there must be explicit allow rules
> allowing it everything) and the hook functions invoke the same
> permission checking for both unconfined and confined domains alike.
>
>
>
> --
> V/R,
>
> Wenhui Zhang
>
> Email: wenhui@gwmail.gwu.edu <mailto:wenhui@gwmail.gwu.edu>
> Telephone: 1-(703) 424 3193
>
>
>
>
>
>
^ permalink raw reply
* Re: Perf Data on LSM in v5.3
From: Stephen Smalley @ 2020-01-31 19:50 UTC (permalink / raw)
To: Wenhui Zhang
Cc: John Johansen, Casey Schaufler, Casey Schaufler, James Morris,
linux-security-module, SELinux, Kees Cook, penguin-kernel,
Paul Moore
In-Reply-To: <CAOSEQ1rOQ50WjvvUSeVpf0RREenP_59u34yx1YQE1YdigzOXcg@mail.gmail.com>
On 1/31/20 2:08 PM, Wenhui Zhang wrote:
> Hi, Smalley:
> DAC, MAC and SELinux's performance data is performed, and it seems like
> our conclusion is consistent with our previous evaluation.
> Please see here (configuration files are included as well):
> 5.3.0-results
> <https://drive.google.com/drive/folders/1NPkHYoffPnkvMlXIM5ytrqzBThLwXx86>
> I am trying to test other modules (SMACK, Apparmor, Integrity etc. )
Still looks like there are some unrelated differences in kernel configs
among dac, mac, and selinux that aren't actually related to what you are
testing. Also, looks like there is a typo in your CONFIG_LSM for
selinux-config.txt, not sure what effect that has if any.
Looks like you are only running micro benchmarks?
What did you use as your base distribution? Fedora 31? Is SELinux
running enforcing with a policy loaded, and no denials during the
benchmark runs (i.e. no avc: denied messages in ausearch -m AVC -ts
boot or journalctl -b output)? Is the benchmark running in unconfined_t
or some other context?
> However what confused me a lot is the Kconfig file in
> ./linux/security/Kconfig:
> config LSM
> string "Ordered list of enabled LSMs"
> default
> "newmodule,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor"
> if DEFAULT_SECURITY_SMACK
> default
> "newmodule,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo"
> if DEFAULT_SECURITY_APPARMOR
> default "newmodule,yama,loadpin,safesetid,integrity,tomoyo" if
> DEFAULT_SECURITY_TOMOYO
> default "newmodule,yama,loadpin,safesetid,integrity" if
> DEFAULT_SECURITY_DAC
> default
> "newmodule,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor"
> IMHO, it seems like it is a little confusing, changing it to below maybe
> make it a lilttle more clear:
> config LSM
> string "Ordered list of enabled LSMs"
> default "newmodule,yama,loadpin,safesetid,integrity, tomoyo,
> apparmor, selinux, smack" if DEFAULT_SECURITY_SMACK
> default "newmodule,yama,loadpin,safesetid,integrity,tomoyo,
> smack, selinux, apparmor" if DEFAULT_SECURITY_APPARMOR
> default "newmodule,yama,loadpin,safesetid,integrity,tomoyo" if
> DEFAULT_SECURITY_TOMOYO
> default "" if DEFAULT_SECURITY_DAC *# could we leave this to
> empty string*
> default
> "newmodule,yama,loadpin,safesetid,integrity,*selinux,smack,tomoyo,apparmor"
> # on Ubuntu, apparmor be the the dedault, however on centos etc, maybe
> selinux be the default, on andriod smack as default *
> Any suggestions on clean up the code on this part please?
I could be wrong but I think the ordering is to preserve the old
behavior of DEFAULT_SECURITY_FOO while still allowing future stacking
if/when it is supported by the respective modules. So the default major
module has to precede any other major modules in the list in order to
win at registration time.
BTW Android uses SELinux [1], not Smack.
[1] http://selinuxproject.org/page/SEAndroid
^ permalink raw reply
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: Casey Schaufler @ 2020-01-31 22:10 UTC (permalink / raw)
To: Stephen Smalley, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul, Casey Schaufler
In-Reply-To: <6bd3e393-e1df-7117-d15a-81cb1946807b@tycho.nsa.gov>
On 1/24/2020 12:16 PM, Stephen Smalley wrote:
> On 1/24/20 2:28 PM, Casey Schaufler wrote:
>> On 1/24/2020 8:20 AM, Stephen Smalley wrote:
>>> On 1/24/20 9:42 AM, Stephen Smalley wrote:
>>>> On 1/23/20 7:23 PM, Casey Schaufler wrote:
>>>>> Add an entry /proc/.../attr/context which displays the full
>>>>> process security "context" in compound format:'
>>>>> lsm1\0value\0lsm2\0value\0...
>>>>> This entry is not writable.
>>>>>
>>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>>> Cc: linux-api@vger.kernel.org
>>>>
>>>> As previously discussed, there are issues with AppArmor's implementation of getprocattr() particularly around the trailing newline that dbus-daemon and perhaps others would like to see go away in any new interface. Hence, I don't think we should implement this new API using the existing getprocattr() hook lest it also be locked into the current behavior forever.
>>>
>>> Also, it would be good if whatever hook is introduced to support /proc/pid/attr/context could also be leveraged by the SO_PEERCONTEXT implementation in the future so that we are guaranteed a consistent result between the two interfaces, unlike the current situation for /proc/self/attr/current versus SO_PEERSEC.
>>
>> I don't believe that a new hook is necessary, and that introducing one
>> just to deal with a '\n' would be pedantic. We really have two rational
>> options. AppArmor could drop the '\n' from their "context". Or, we can
>> simply document that the /proc/pid/attr/context interface will trim any
>> trailing whitespace. I understand that this would be a break from the
>> notion that the LSM infrastructure does not constrain what a module uses
>> for its own data. On the other hand, we have been saying that "context"s
>> are strings, and ignoring trailing whitespace is usual behavior for
>> strings.
>
> Well, you can either introduce a new common underlying hook for use by /proc/pid/attr/context and SO_PEERCONTEXT to produce the string that is to be returned to userspace (in order to guarantee consistency in format and allowing them to be directly compared, which I think is what the dbus maintainers wanted), or you can modify every security module to provide that guarantee in its existing getprocattr and getpeersec* hook functions (SELinux already provides this guarantee; Smack and AppArmor produce slightly different results with respect to \0 and/or \n), or you can have the framework trim the values it gets from the security modules before composing them. But you need to do one of those things before this interface gets merged upstream.
>
> Aside from the trailing newline and \0 issues, AppArmor also has a whitespace-separated (mode) field that may or may not be present in the contexts it presently returns, ala "/usr/sbin/cupsd (enforce)". Not sure what they want for the new interfaces.
>
From c4085435215653b7c4d07a35a9df308120441d79 Mon Sep 17 00:00:00 2001
From: Casey Schaufler <casey@schaufler-ca.com>
Date: Fri, 31 Jan 2020 13:57:23 -0800
Subject: [PATCH v14] LSM: Move "context" format enforcement into security
modules
Document in lsm_hooks.h what is expected of a security module that
supplies the "context" attribute. Add handling of the "context"
attribute to SELinux, Smack and AppArmor security modules. The
AppArmor implementation provides a different string for "context"
than it does for other attributes to conform with the "context"
format.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/lsm_hooks.h | 6 ++++++
security/apparmor/include/procattr.h | 2 +-
security/apparmor/lsm.c | 8 ++++++--
security/apparmor/procattr.c | 11 +++++++----
security/security.c | 2 +-
security/selinux/hooks.c | 2 +-
security/smack/smack_lsm.c | 2 +-
7 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 2bf82e1cf347..61977a33f2c3 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1321,6 +1321,12 @@
* @pages contains the number of pages.
* Return 0 if permission is granted.
*
+ * @getprocattr:
+ * Provide the named process attribute for display in special files in
+ * the /proc/.../attr directory. Attribute naming and the data displayed
+ * is at the discretion of the security modules. The exception is the
+ * "context" attribute, which will contain the security context of the
+ * task as a nul terminated text string without trailing whitespace.
* @ismaclabel:
* Check if the extended attribute specified by @name
* represents a MAC label. Returns 1 if name is a MAC
diff --git a/security/apparmor/include/procattr.h b/security/apparmor/include/procattr.h
index 31689437e0e1..03dbfdb2f2c0 100644
--- a/security/apparmor/include/procattr.h
+++ b/security/apparmor/include/procattr.h
@@ -11,7 +11,7 @@
#ifndef __AA_PROCATTR_H
#define __AA_PROCATTR_H
-int aa_getprocattr(struct aa_label *label, char **string);
+int aa_getprocattr(struct aa_label *label, char **string, bool newline);
int aa_setprocattr_changehat(char *args, size_t size, int flags);
#endif /* __AA_PROCATTR_H */
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 37d003568e82..07729c28275e 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -593,6 +593,7 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
const struct cred *cred = get_task_cred(task);
struct aa_task_ctx *ctx = task_ctx(current);
struct aa_label *label = NULL;
+ bool newline = true;
if (strcmp(name, "current") == 0)
label = aa_get_newest_label(cred_label(cred));
@@ -600,11 +601,14 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
label = aa_get_newest_label(ctx->previous);
else if (strcmp(name, "exec") == 0 && ctx->onexec)
label = aa_get_newest_label(ctx->onexec);
- else
+ else if (strcmp(name, "context") == 0) {
+ label = aa_get_newest_label(cred_label(cred));
+ newline = false;
+ } else
error = -EINVAL;
if (label)
- error = aa_getprocattr(label, value);
+ error = aa_getprocattr(label, value, newline);
aa_put_label(label);
put_cred(cred);
diff --git a/security/apparmor/procattr.c b/security/apparmor/procattr.c
index c929bf4a3df1..1098bca3d0e4 100644
--- a/security/apparmor/procattr.c
+++ b/security/apparmor/procattr.c
@@ -20,6 +20,7 @@
* aa_getprocattr - Return the profile information for @profile
* @profile: the profile to print profile info about (NOT NULL)
* @string: Returns - string containing the profile info (NOT NULL)
+ * @newline: Should a newline be added to @string.
*
* Returns: length of @string on success else error on failure
*
@@ -30,7 +31,7 @@
*
* Returns: size of string placed in @string else error code on failure
*/
-int aa_getprocattr(struct aa_label *label, char **string)
+int aa_getprocattr(struct aa_label *label, char **string, bool newline)
{
struct aa_ns *ns = labels_ns(label);
struct aa_ns *current_ns = aa_get_current_ns();
@@ -60,11 +61,13 @@ int aa_getprocattr(struct aa_label *label, char **string)
return len;
}
- (*string)[len] = '\n';
- (*string)[len + 1] = 0;
+ if (newline) {
+ (*string)[len] = '\n';
+ (*string)[++len] = 0;
+ }
aa_put_ns(current_ns);
- return len + 1;
+ return len;
}
/**
diff --git a/security/security.c b/security/security.c
index fdd0c85df89e..5a4d90256fd7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2111,7 +2111,7 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
if (!strcmp(name, "context")) {
hlist_for_each_entry(hp, &security_hook_heads.getprocattr,
list) {
- rc = hp->hook.getprocattr(p, "current", &cp);
+ rc = hp->hook.getprocattr(p, "context", &cp);
if (rc == -EINVAL || rc == -ENOPROTOOPT)
continue;
if (rc < 0) {
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index cd4743331800..1f53a0c66a46 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6281,7 +6281,7 @@ static int selinux_getprocattr(struct task_struct *p,
goto bad;
}
- if (!strcmp(name, "current"))
+ if (!strcmp(name, "current") || !strcmp(name, "context"))
sid = __tsec->sid;
else if (!strcmp(name, "prev"))
sid = __tsec->osid;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 9ce67e03ac49..834b6e886e7b 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3487,7 +3487,7 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
char *cp;
int slen;
- if (strcmp(name, "current") != 0)
+ if (strcmp(name, "current") != 0 && strcmp(name, "context") != 0)
return -EINVAL;
cp = kstrdup(skp->smk_known, GFP_KERNEL);
--
2.24.1
^ permalink raw reply related
* Re: [PATCH v2] keys: proc_keys_next should increase position index
From: Vasily Averin @ 2020-02-01 7:54 UTC (permalink / raw)
To: David Howells
Cc: keyrings, linux-security-module, Jarkko Sakkinen, James Morris,
Serge E. Hallyn
In-Reply-To: <265151.1580491178@warthog.procyon.org.uk>
On 1/31/20 8:19 PM, David Howells wrote:
> Vasily Averin <vvs@virtuozzo.com> wrote:
>> If seq_file .next fuction does not change position index,
> "function"
>> <<<< and whole last lien again
> "line"
>
> I can fix these up for you.
thank you!
>> https://bugzilla.kernel.org/show_bug.cgi?id=206283
>
> I wonder if this should have a tag - it looks kind of out of place without
> one, but I can't find one suggested.
Some maintainers of another subsystems asked me to add
Cc: stable@vger.kernel.org
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
however frankly speaking I do not think it is required in this case.
Thank you,
Vasily Averin
^ permalink raw reply
* Re: [PATCH 1/8] tpm: initialize crypto_id of allocated_banks to HASH_ALGO__LAST
From: Jarkko Sakkinen @ 2020-02-01 17:10 UTC (permalink / raw)
To: Mimi Zohar
Cc: Roberto Sassu, james.bottomley@hansenpartnership.com,
linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <1580477590.6104.61.camel@linux.ibm.com>
On Fri, Jan 31, 2020 at 08:33:10AM -0500, Mimi Zohar wrote:
> On Thu, 2020-01-30 at 16:11 +0000, Roberto Sassu wrote:
> > > -----Original Message-----
> > > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> > > Sent: Thursday, January 30, 2020 9:48 AM
> > > To: Roberto Sassu <roberto.sassu@huawei.com>; zohar@linux.ibm.com;
> > > james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> > > Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> > > Subject: Re: [PATCH 1/8] tpm: initialize crypto_id of allocated_banks to
> > > HASH_ALGO__LAST
> > >
> > > On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > > > chip->allocated_banks contains the list of TPM algorithm IDs of allocated
> > > > PCR banks. It also contains the corresponding ID of the crypto subsystem,
> > > > so that users of the TPM driver can calculate a digest for a PCR extend
> > > > operation.
> > > >
> > > > However, if there is no mapping between TPM algorithm ID and crypto ID,
> > > the
> > > > crypto_id field in chip->allocated_banks remains set to zero (the array is
> > > > allocated and initialized with kcalloc() in tpm2_get_pcr_allocation()).
> > > > Zero should not be used as value for unknown mappings, as it is a valid
> > > > crypto ID (HASH_ALGO_MD4).
> > > >
> > > > This patch initializes crypto_id to HASH_ALGO__LAST.
> > > >
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>---
> > >
> > > Remarks:
> > >
> > > * After the subsystem tag, short summary starts with a capital lettter.
> > > * Missing fixes tag and cc tag to stable.
> > > * A struct called allocated_banks does not exist.
> > > * Please prefer using an imperative sentence when describing the action
> > > to take e.g. "Thus, initialize crypto_id to HASH_ALGO__LAST".
> >
> > Thanks. I will fix these issues in the next version of the patch set.
>
> Jarkko, I realize this is a TPM patch, but this patch set is dependent
> on it. When this patch is ready, could you create a topic branch,
> which both of us could merge?
WFM.
/Jarkko
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox