* Re: [RFC PATCH 1/2] ima: Implement support for uncompressed module appended signatures
From: Lakshmi Ramasubramanian @ 2020-02-06 17:07 UTC (permalink / raw)
To: Eric Snowberg, zohar, dmitry.kasatkin, jmorris, serge
Cc: dhowells, geert, gregkh, nayna, tglx, bauerman, mpe,
linux-integrity, linux-security-module, linux-kernel
In-Reply-To: <20200206164226.24875-2-eric.snowberg@oracle.com>
On 2/6/2020 8:42 AM, Eric Snowberg wrote:
>
> @@ -31,6 +32,7 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
> ".ima",
> #endif
> ".platform",
> + ".builtin_trusted_keys",
> };
>
> #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> @@ -45,8 +47,11 @@ static struct key *integrity_keyring_from_id(const unsigned int id)
> return ERR_PTR(-EINVAL);
>
> if (!keyring[id]) {
> - keyring[id] =
> - request_key(&key_type_keyring, keyring_name[id], NULL);
> + if (id == INTEGRITY_KEYRING_KERNEL)
> + keyring[id] = VERIFY_USE_SECONDARY_KEYRING;
Since "Built-In Trusted Keyring" or "Secondary Trusted Keyring" is used,
would it be more appropriate to name this identifier
INTEGRITY_KEYRING_BUILTIN_OR_SECONDARY?
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 73fc286834d7..63f0e6bff0e0 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -145,7 +145,8 @@ int integrity_kernel_read(struct file *file, loff_t offset,
> #define INTEGRITY_KEYRING_EVM 0
> #define INTEGRITY_KEYRING_IMA 1
> #define INTEGRITY_KEYRING_PLATFORM 2
> -#define INTEGRITY_KEYRING_MAX 3
> +#define INTEGRITY_KEYRING_KERNEL 3
> +#define INTEGRITY_KEYRING_MAX 4
-lakshmi
^ permalink raw reply
* [RFC PATCH 1/2] ima: Implement support for uncompressed module appended signatures
From: Eric Snowberg @ 2020-02-06 16:42 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, jmorris, serge
Cc: dhowells, geert, gregkh, nayna, eric.snowberg, tglx, bauerman,
mpe, linux-integrity, linux-security-module, linux-kernel
In-Reply-To: <20200206164226.24875-1-eric.snowberg@oracle.com>
Currently IMA can validate compressed modules containing appended
signatures. This adds the ability to also validate uncompressed
modules when appraise_type=imasig|modsig.
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
security/integrity/digsig.c | 9 +++++++--
security/integrity/ima/ima_appraise.c | 3 +++
security/integrity/integrity.h | 3 ++-
3 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index ea1aae3d07b3..5e0c4d04ab9d 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -15,6 +15,7 @@
#include <linux/key-type.h>
#include <linux/digsig.h>
#include <linux/vmalloc.h>
+#include <linux/verification.h>
#include <crypto/public_key.h>
#include <keys/system_keyring.h>
@@ -31,6 +32,7 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
".ima",
#endif
".platform",
+ ".builtin_trusted_keys",
};
#ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
@@ -45,8 +47,11 @@ static struct key *integrity_keyring_from_id(const unsigned int id)
return ERR_PTR(-EINVAL);
if (!keyring[id]) {
- keyring[id] =
- request_key(&key_type_keyring, keyring_name[id], NULL);
+ if (id == INTEGRITY_KEYRING_KERNEL)
+ keyring[id] = VERIFY_USE_SECONDARY_KEYRING;
+ else
+ keyring[id] = request_key(&key_type_keyring,
+ keyring_name[id], NULL);
if (IS_ERR(keyring[id])) {
int err = PTR_ERR(keyring[id]);
pr_err("no %s keyring: %d\n", keyring_name[id], err);
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 300c8d2943c5..4c009c55d620 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -294,6 +294,9 @@ static int modsig_verify(enum ima_hooks func, const struct modsig *modsig,
func == KEXEC_KERNEL_CHECK)
rc = integrity_modsig_verify(INTEGRITY_KEYRING_PLATFORM,
modsig);
+ if (rc && func == MODULE_CHECK)
+ rc = integrity_modsig_verify(INTEGRITY_KEYRING_KERNEL, modsig);
+
if (rc) {
*cause = "invalid-signature";
*status = INTEGRITY_FAIL;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 73fc286834d7..63f0e6bff0e0 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -145,7 +145,8 @@ int integrity_kernel_read(struct file *file, loff_t offset,
#define INTEGRITY_KEYRING_EVM 0
#define INTEGRITY_KEYRING_IMA 1
#define INTEGRITY_KEYRING_PLATFORM 2
-#define INTEGRITY_KEYRING_MAX 3
+#define INTEGRITY_KEYRING_KERNEL 3
+#define INTEGRITY_KEYRING_MAX 4
extern struct dentry *integrity_dir;
--
2.18.1
^ permalink raw reply related
* [RFC PATCH 2/2] ima: Change default secure_boot policy to include appended signatures
From: Eric Snowberg @ 2020-02-06 16:42 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, jmorris, serge
Cc: dhowells, geert, gregkh, nayna, eric.snowberg, tglx, bauerman,
mpe, linux-integrity, linux-security-module, linux-kernel
In-Reply-To: <20200206164226.24875-1-eric.snowberg@oracle.com>
Change the default secure_boot policy from:
appraise func=MODULE_CHECK appraise_type=imasig
appraise func=FIRMWARE_CHECK appraise_type=imasig
appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig
appraise func=POLICY_CHECK appraise_type=imasig
to
appraise func=MODULE_CHECK appraise_type=imasig|modsig
appraise func=FIRMWARE_CHECK appraise_type=imasig
appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig
appraise func=POLICY_CHECK appraise_type=imasig
This will allow appended signatures to work with the default
secure_boot policy.
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
security/integrity/ima/ima_policy.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index ef8dfd47c7e3..5d835715b472 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -189,11 +189,11 @@ static struct ima_rule_entry build_appraise_rules[] __ro_after_init = {
static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {
{.action = APPRAISE, .func = MODULE_CHECK,
- .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
+ .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED | IMA_MODSIG_ALLOWED},
{.action = APPRAISE, .func = FIRMWARE_CHECK,
.flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
{.action = APPRAISE, .func = KEXEC_KERNEL_CHECK,
- .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
+ .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED | IMA_MODSIG_ALLOWED},
{.action = APPRAISE, .func = POLICY_CHECK,
.flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
};
--
2.18.1
^ permalink raw reply related
* [RFC PATCH 0/2] ima: uncompressed module appraisal support
From: Eric Snowberg @ 2020-02-06 16:42 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, jmorris, serge
Cc: dhowells, geert, gregkh, nayna, eric.snowberg, tglx, bauerman,
mpe, linux-integrity, linux-security-module, linux-kernel
When booting with either "ima_policy=secure_boot module.sig_enforce=1"
or building a kernel with CONFIG_IMA_ARCH_POLICY and booting with
"ima_policy=secure_boot", module loading behaves differently based on if
the module is compressed or not. Originally when appraising a module
with ima it had to be uncompressed and ima signed. Recent changes in 5.4
have allowed internally signed modules to load [1]. But this only works
if the internally signed module is compressed. The uncompressed module
that is internally signed must still be ima signed. This patch series
tries to bring the two in line.
I'm sending this as an RFC in case this was done intentionally. Or
maybe there is another way around this problem? I also realize the
uncompressed module will be verified again with module_sig_check. I'm
open to suggestions on improvement if this is seen as a problem.
[1] https://patchwork.kernel.org/cover/10986023
Eric Snowberg (2):
ima: Implement support for uncompressed module appended signatures
ima: Change default secure_boot policy to include appended signatures
security/integrity/digsig.c | 9 +++++++--
security/integrity/ima/ima_appraise.c | 3 +++
security/integrity/ima/ima_policy.c | 4 ++--
security/integrity/integrity.h | 3 ++-
4 files changed, 14 insertions(+), 5 deletions(-)
--
2.18.1
^ permalink raw reply
* RE: [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for template digests
From: Roberto Sassu @ 2020-02-06 16:36 UTC (permalink / raw)
To: Mimi Zohar, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <1581006834.5585.430.camel@linux.ibm.com>
> -----Original Message-----
> From: owner-linux-security-module@vger.kernel.org [mailto:owner-linux-
> security-module@vger.kernel.org] On Behalf Of Mimi Zohar
> Sent: Thursday, February 6, 2020 5:34 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>
> Subject: Re: [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for
> template digests
>
> On Thu, 2020-02-06 at 16:27 +0000, Roberto Sassu wrote:
> > > -----Original Message-----
> > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > Sent: Thursday, February 6, 2020 5:08 PM
> > > To: Roberto Sassu <roberto.sassu@huawei.com>;
> > > James.Bottomley@HansenPartnership.com;
> > > jarkko.sakkinen@linux.intel.com
> > > Cc: linux-integrity@vger.kernel.org; linux-security-
> module@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; Silviu Vlasceanu
> > > <Silviu.Vlasceanu@huawei.com>
> > > Subject: Re: [PATCH v2 5/8] ima: Switch to dynamically allocated buffer
> for
> > > template digests
> > >
> > > Hi Roberto,
> > >
> > > On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> > > > This patch dynamically allocates the array of tpm_digest structures in
> > > > ima_alloc_init_template() and ima_restore_template_data(). The size
> of
> > > the
> > > > array, stored in ima_num_template_digests, is initially equal to 1
> (SHA1)
> > > > and will be determined in the upcoming patches depending on the
> > > allocated
> > > > PCR banks and the chosen default IMA algorithm.
> > > >
> > > > Calculating the SHA1 digest is mandatory, as SHA1 still remains the
> default
> > > > hash algorithm for the measurement list. When IMA will support the
> > > Crypto
> > > > Agile format, remaining digests will be also provided.
> > > >
> > > > The position in the array of the SHA1 digest is stored in the
> ima_sha1_idx
> > > > global variable and it is determined at IMA initialization time.
> > > >
> > > > Changelog
> > > >
> > > > v1:
> > > > - move ima_sha1_idx to ima_crypto.c
> > > > - introduce ima_num_template_digests (suggested by Mimi)
> > >
> > > Instead of hardcoding "nr_allocated_banks + 1" or nr_allocated_banks +
> > > 2", I suggested defining "nr_allocated_banks + extra", where "extra"
> > > could be 0, 1, or 2.
> > >
> > > The rest of the code would remain exactly the same as you had.
> >
> > Ok. I did a small improvement. Since we determine the number of
> > required elements of ima_algo_array before kmalloc() I thought it
> > was ok to directly set that number of elements in a single variable.
> >
> > If you think that having two variables is better, I will change it.
>
> The connection to nr_allocated_banks is then not as visible. Using
> two variables is clearer.
Ok, no problem. I will change it in the next version.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
^ permalink raw reply
* Re: [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for template digests
From: Mimi Zohar @ 2020-02-06 16:33 UTC (permalink / raw)
To: Roberto Sassu, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <0b91e6977bac4cd5a638041adb3e76eb@huawei.com>
On Thu, 2020-02-06 at 16:27 +0000, Roberto Sassu wrote:
> > -----Original Message-----
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Thursday, February 6, 2020 5:08 PM
> > To: Roberto Sassu <roberto.sassu@huawei.com>;
> > James.Bottomley@HansenPartnership.com;
> > jarkko.sakkinen@linux.intel.com
> > Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> > linux-kernel@vger.kernel.org; Silviu Vlasceanu
> > <Silviu.Vlasceanu@huawei.com>
> > Subject: Re: [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for
> > template digests
> >
> > Hi Roberto,
> >
> > On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> > > This patch dynamically allocates the array of tpm_digest structures in
> > > ima_alloc_init_template() and ima_restore_template_data(). The size of
> > the
> > > array, stored in ima_num_template_digests, is initially equal to 1 (SHA1)
> > > and will be determined in the upcoming patches depending on the
> > allocated
> > > PCR banks and the chosen default IMA algorithm.
> > >
> > > Calculating the SHA1 digest is mandatory, as SHA1 still remains the default
> > > hash algorithm for the measurement list. When IMA will support the
> > Crypto
> > > Agile format, remaining digests will be also provided.
> > >
> > > The position in the array of the SHA1 digest is stored in the ima_sha1_idx
> > > global variable and it is determined at IMA initialization time.
> > >
> > > Changelog
> > >
> > > v1:
> > > - move ima_sha1_idx to ima_crypto.c
> > > - introduce ima_num_template_digests (suggested by Mimi)
> >
> > Instead of hardcoding "nr_allocated_banks + 1" or nr_allocated_banks +
> > 2", I suggested defining "nr_allocated_banks + extra", where "extra"
> > could be 0, 1, or 2.
> >
> > The rest of the code would remain exactly the same as you had.
>
> Ok. I did a small improvement. Since we determine the number of
> required elements of ima_algo_array before kmalloc() I thought it
> was ok to directly set that number of elements in a single variable.
>
> If you think that having two variables is better, I will change it.
The connection to nr_allocated_banks is then not as visible. Using
two variables is clearer.
Mimi
^ permalink raw reply
* RE: [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for template digests
From: Roberto Sassu @ 2020-02-06 16:27 UTC (permalink / raw)
To: Mimi Zohar, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <1581005284.5585.422.camel@linux.ibm.com>
> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Thursday, February 6, 2020 5:08 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>
> Subject: Re: [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for
> template digests
>
> Hi Roberto,
>
> On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> > This patch dynamically allocates the array of tpm_digest structures in
> > ima_alloc_init_template() and ima_restore_template_data(). The size of
> the
> > array, stored in ima_num_template_digests, is initially equal to 1 (SHA1)
> > and will be determined in the upcoming patches depending on the
> allocated
> > PCR banks and the chosen default IMA algorithm.
> >
> > Calculating the SHA1 digest is mandatory, as SHA1 still remains the default
> > hash algorithm for the measurement list. When IMA will support the
> Crypto
> > Agile format, remaining digests will be also provided.
> >
> > The position in the array of the SHA1 digest is stored in the ima_sha1_idx
> > global variable and it is determined at IMA initialization time.
> >
> > Changelog
> >
> > v1:
> > - move ima_sha1_idx to ima_crypto.c
> > - introduce ima_num_template_digests (suggested by Mimi)
>
> Instead of hardcoding "nr_allocated_banks + 1" or nr_allocated_banks +
> 2", I suggested defining "nr_allocated_banks + extra", where "extra"
> could be 0, 1, or 2.
>
> The rest of the code would remain exactly the same as you had.
Ok. I did a small improvement. Since we determine the number of
required elements of ima_algo_array before kmalloc() I thought it
was ok to directly set that number of elements in a single variable.
If you think that having two variables is better, I will change it.
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
^ permalink raw reply
* Re: [PATCH v2 5/8] ima: Switch to dynamically allocated buffer for template digests
From: Mimi Zohar @ 2020-02-06 16:08 UTC (permalink / raw)
To: Roberto Sassu, James.Bottomley, jarkko.sakkinen
Cc: linux-integrity, linux-security-module, linux-kernel,
silviu.vlasceanu
In-Reply-To: <20200205103317.29356-6-roberto.sassu@huawei.com>
Hi Roberto,
On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> This patch dynamically allocates the array of tpm_digest structures in
> ima_alloc_init_template() and ima_restore_template_data(). The size of the
> array, stored in ima_num_template_digests, is initially equal to 1 (SHA1)
> and will be determined in the upcoming patches depending on the allocated
> PCR banks and the chosen default IMA algorithm.
>
> Calculating the SHA1 digest is mandatory, as SHA1 still remains the default
> hash algorithm for the measurement list. When IMA will support the Crypto
> Agile format, remaining digests will be also provided.
>
> The position in the array of the SHA1 digest is stored in the ima_sha1_idx
> global variable and it is determined at IMA initialization time.
>
> Changelog
>
> v1:
> - move ima_sha1_idx to ima_crypto.c
> - introduce ima_num_template_digests (suggested by Mimi)
Instead of hardcoding "nr_allocated_banks + 1" or nr_allocated_banks +
2", I suggested defining "nr_allocated_banks + extra", where "extra"
could be 0, 1, or 2.
The rest of the code would remain exactly the same as you had.
Mimi
^ permalink raw reply
* Re: [PATCH v2] ima: export the measurement list when needed
From: Mimi Zohar @ 2020-02-06 14:13 UTC (permalink / raw)
To: Janne Karhunen, linux-integrity, linux-security-module
Cc: Ken Goldman, david.safford, monty.wiseman, Amir Goldstein,
linux-fsdevel
In-Reply-To: <CAE=NcrZrbRinOAbB+k1rjhcae3nqfJ8snC_EnY8njMDioM7=vg@mail.gmail.com>
Hi Janne,
On Fri, 2020-01-10 at 10:48 +0200, Janne Karhunen wrote:
> On Wed, Jan 8, 2020 at 1:18 PM Janne Karhunen <janne.karhunen@gmail.com> wrote:
> >
> > Some systems can end up carrying lots of entries in the ima
> > measurement list. Since every entry is using a bit of kernel
> > memory, allow the sysadmin to export the measurement list to
> > the filesystem to free up some memory.
>
> Hopefully this addressed comments from everyone. The flush event can
> now be triggered by the admin anytime and unique file names can be
> used for each flush (log.1, log.2, ...) etc, so getting to the correct
> item should be easy.
>
> While it can now be argued that since this is an admin-driven event,
> kernel does not need to write the file. However, the intention is to
> bring out a second patch a bit later that adds a variable to define
> the max number of entries to be kept in the kernel memory and
> workqueue based automatic flushing. In those cases the kernel has to
> be able to write the file without any help from the admin..
The implications of exporting and removing records from the IMA-
measurement list needs to be considered carefully. Verifying a TPM
quote will become dependent on knowing where the measurements are
stored. The existing measurement list is stored in kernel memory and,
barring a kernel memory attack, is protected from modification.
Before upstreaming this or a similar patch, there needs to be a
discussion as to how the measurement list will be protected once is it
exported to userspace.
This patch now attempts to address two very different scenarios. The
first scenario is where userspace is requesting exporting and removing
of the measurement list records. The other scenario is the kernel
exporting and removing of the measurement list records. Conflating
these two different use cases might not be the right solution, as we
originally thought.
The kernel already exports the IMA measurement list to userspace via a
securityfs file. From a userspace perspective, missing is the ability
of removing N number of records. In this scenario, userspace would be
responsible for safely storing the measurements (e.g. blockchain).
The kernel would only be responsible for limiting permission, perhaps
based on a capability, before removing records from the measurement
list.
In the kernel usecase, somehow the kernel would need to safely export
the measurement list, or some portion of the measurement list, to a
file and then delete that portion. What protects the exported records
stored in a file from modification?
Instead of exporting the measurement records, one option as suggested
by Amir Goldstein, would be to use a vfs_tmpfile() to get an anonymous
file for backing store. The existing securityfs measurement lists
would then read from this private copy of the anonymous file.
I've Cc'ed fsdevel for additional comments/suggestions.
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate
From: Mimi Zohar @ 2020-02-06 12:31 UTC (permalink / raw)
To: Roberto Sassu, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu,
stable@vger.kernel.org
In-Reply-To: <17bfd3e2b7fa4f31a46a6688e4a6e34f@huawei.com>
On Thu, 2020-02-06 at 12:28 +0000, Roberto Sassu wrote:
> > -----Original Message-----
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Thursday, February 6, 2020 1:17 PM
> > To: Roberto Sassu <roberto.sassu@huawei.com>;
> > James.Bottomley@HansenPartnership.com;
> > jarkko.sakkinen@linux.intel.com
> > Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> > linux-kernel@vger.kernel.org; Silviu Vlasceanu
> > <Silviu.Vlasceanu@huawei.com>; stable@vger.kernel.org
> > Subject: Re: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot
> > aggregate
> >
> > On Thu, 2020-02-06 at 09:36 +0000, Roberto Sassu wrote:
> > > > Hi Roberto,
> > > >
> > > > On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> > > >
> > > > <snip>
> > > >
> > > > > Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > > > Suggested-by: James Bottomley
> > > > <James.Bottomley@HansenPartnership.com>
> > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > Cc: stable@vger.kernel.org
> > > >
> > > > Cc'ing stable resulted in Sasha's automated message. If you're going
> > > > to Cc stable, then please include the stable kernel release (e.g. Cc:
> > > > stable@vger.kernel.org # v5.3). Also please include a "Fixes" tag.
> > > > Normally only bug fixes are backported.
> > >
> > > Ok, will add the kernel version. I also thought which commit I should
> > > mention in the Fixes tag. IMA always read the SHA1 bank from the
> > > beginning. I could mention the patch that introduces the new API
> > > to read other banks, but I'm not sure. What do you think?
> >
> > This patch is dependent on nr_allocated_banks. Please try applying
> > this patch to the earliest stable kernel with the commit that
> > introduces nr_allocated_banks and test to make sure it works properly.
>
> It also depends on 879b589210a9 ("tpm: retrieve digest size of unknown"
> algorithms with PCR read") which exported the mapping between TPM
> algorithm ID and crypto ID, and changed the definition of tpm_pcr_read()
> to read non-SHA1 PCR banks. It requires many patches, so backporting it
> is not a trivial task. I think the earliest kernel this patch can be backported to
> is 5.1.
Agreed. Thank you for checking.
Mimi
^ permalink raw reply
* RE: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate
From: Roberto Sassu @ 2020-02-06 12:28 UTC (permalink / raw)
To: Mimi Zohar, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu,
stable@vger.kernel.org
In-Reply-To: <1580991426.5585.334.camel@linux.ibm.com>
> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Thursday, February 6, 2020 1:17 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>; stable@vger.kernel.org
> Subject: Re: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot
> aggregate
>
> On Thu, 2020-02-06 at 09:36 +0000, Roberto Sassu wrote:
> > > Hi Roberto,
> > >
> > > On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> > >
> > > <snip>
> > >
> > > > Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > > Suggested-by: James Bottomley
> > > <James.Bottomley@HansenPartnership.com>
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > Cc: stable@vger.kernel.org
> > >
> > > Cc'ing stable resulted in Sasha's automated message. If you're going
> > > to Cc stable, then please include the stable kernel release (e.g. Cc:
> > > stable@vger.kernel.org # v5.3). Also please include a "Fixes" tag.
> > > Normally only bug fixes are backported.
> >
> > Ok, will add the kernel version. I also thought which commit I should
> > mention in the Fixes tag. IMA always read the SHA1 bank from the
> > beginning. I could mention the patch that introduces the new API
> > to read other banks, but I'm not sure. What do you think?
>
> This patch is dependent on nr_allocated_banks. Please try applying
> this patch to the earliest stable kernel with the commit that
> introduces nr_allocated_banks and test to make sure it works properly.
It also depends on 879b589210a9 ("tpm: retrieve digest size of unknown"
algorithms with PCR read") which exported the mapping between TPM
algorithm ID and crypto ID, and changed the definition of tpm_pcr_read()
to read non-SHA1 PCR banks. It requires many patches, so backporting it
is not a trivial task. I think the earliest kernel this patch can be backported to
is 5.1.
Roberto
^ permalink raw reply
* Re: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate
From: Mimi Zohar @ 2020-02-06 12:17 UTC (permalink / raw)
To: Roberto Sassu, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu,
stable@vger.kernel.org
In-Reply-To: <b1507c1121b64b3abc00e154fcfeef65@huawei.com>
On Thu, 2020-02-06 at 09:36 +0000, Roberto Sassu wrote:
> > Hi Roberto,
> >
> > On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> >
> > <snip>
> >
> > > Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > Suggested-by: James Bottomley
> > <James.Bottomley@HansenPartnership.com>
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Cc: stable@vger.kernel.org
> >
> > Cc'ing stable resulted in Sasha's automated message. If you're going
> > to Cc stable, then please include the stable kernel release (e.g. Cc:
> > stable@vger.kernel.org # v5.3). Also please include a "Fixes" tag.
> > Normally only bug fixes are backported.
>
> Ok, will add the kernel version. I also thought which commit I should
> mention in the Fixes tag. IMA always read the SHA1 bank from the
> beginning. I could mention the patch that introduces the new API
> to read other banks, but I'm not sure. What do you think?
This patch is dependent on nr_allocated_banks. Please try applying
this patch to the earliest stable kernel with the commit that
introduces nr_allocated_banks and test to make sure it works properly.
thanks,
Mimi
^ permalink raw reply
* RE: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate
From: Roberto Sassu @ 2020-02-06 9:36 UTC (permalink / raw)
To: Mimi Zohar, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu,
stable@vger.kernel.org
In-Reply-To: <1580936432.5585.309.camel@linux.ibm.com>
> -----Original Message-----
> From: owner-linux-security-module@vger.kernel.org [mailto:owner-linux-
> security-module@vger.kernel.org] On Behalf Of Mimi Zohar
> Sent: Wednesday, February 5, 2020 10:01 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>; stable@vger.kernel.org
> Subject: Re: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot
> aggregate
>
> Hi Roberto,
>
> On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
>
> <snip>
>
> > Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > Suggested-by: James Bottomley
> <James.Bottomley@HansenPartnership.com>
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Cc: stable@vger.kernel.org
>
> Cc'ing stable resulted in Sasha's automated message. If you're going
> to Cc stable, then please include the stable kernel release (e.g. Cc:
> stable@vger.kernel.org # v5.3). Also please include a "Fixes" tag.
> Normally only bug fixes are backported.
Ok, will add the kernel version. I also thought which commit I should
mention in the Fixes tag. IMA always read the SHA1 bank from the
beginning. I could mention the patch that introduces the new API
to read other banks, but I'm not sure. What do you think?
Thanks
Roberto
^ permalink raw reply
* RE: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate
From: Roberto Sassu @ 2020-02-06 9:33 UTC (permalink / raw)
To: Mimi Zohar, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu,
stable@vger.kernel.org
In-Reply-To: <1580935285.5585.297.camel@linux.ibm.com>
> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Wednesday, February 5, 2020 9:41 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>; stable@vger.kernel.org
> Subject: Re: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot
> aggregate
>
> On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> > boot_aggregate is the first entry of IMA measurement list. Its purpose is
> > to link pre-boot measurements to IMA measurements. As IMA was
> designed to
> > work with a TPM 1.2, the SHA1 PCR bank was always selected.
> >
> > Currently, even if a TPM 2.0 is used, the SHA1 PCR bank is selected.
> > However, the assumption that the SHA1 PCR bank is always available is not
> > correct, as PCR banks can be selected with the PCR_Allocate() TPM
> command.
> >
> > This patch tries to use ima_hash_algo as hash algorithm for
> boot_aggregate.
> > If no PCR bank uses that algorithm, the patch tries to find the SHA256 PCR
> > bank (which is mandatory in the TCG PC Client specification). If also this
> > bank is not found, the patch selects the first one. If the TPM algorithm
> > of that bank is not mapped to a crypto ID, boot_aggregate is set to zero.
> >
> > Changelog
> >
> > v1:
> > - add Mimi's comments
> > - if there is no PCR bank for the IMA default algorithm use SHA256 or the
> > first bank (suggested by James Bottomley)
>
> If the IMA default hash algorithm is not enabled, James' comment was
> to use SHA256 for TPM 2.0 and SHA1 for TPM 1.2. I don't remember him
> saying anything about using the first bank, that was in v1 of my
> patch. Please refer to v2 of my patch, based on James' comments.
Right. Will fix in the next version.
Thanks
Roberto
> Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > Suggested-by: James Bottomley
> <James.Bottomley@HansenPartnership.com>
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Cc: stable@vger.kernel.org
> > ---
> > security/integrity/ima/ima_crypto.c | 45
> +++++++++++++++++++++++++----
> > security/integrity/ima/ima_init.c | 22 ++++++++++----
> > 2 files changed, 56 insertions(+), 11 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_crypto.c
> b/security/integrity/ima/ima_crypto.c
> > index 73044fc6a952..f2f41a2bc3d4 100644
> > --- a/security/integrity/ima/ima_crypto.c
> > +++ b/security/integrity/ima/ima_crypto.c
> > @@ -655,18 +655,29 @@ static void __init ima_pcrread(u32 idx, struct
> tpm_digest *d)
> > }
> >
> > /*
> > - * Calculate the boot aggregate hash
> > + * The boot_aggregate is a cumulative hash over TPM registers 0 - 7.
> With
> > + * TPM 1.2 the boot_aggregate was based on reading the SHA1 PCRs, but
> with
> > + * TPM 2.0 hash agility, TPM chips could support multiple TPM PCR banks,
> > + * allowing firmware to configure and enable different banks.
> > + *
> > + * Knowing which TPM bank is read to calculate the boot_aggregate
> digest
> > + * needs to be conveyed to a verifier. For this reason, use the same
> > + * hash algorithm for reading the TPM PCRs as for calculating the boot
> > + * aggregate digest as stored in the measurement list.
> > */
> > -static int __init ima_calc_boot_aggregate_tfm(char *digest,
> > +static int __init ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
> > struct crypto_shash *tfm)
> > {
> > - struct tpm_digest d = { .alg_id = TPM_ALG_SHA1, .digest = {0} };
> > + struct tpm_digest d = { .alg_id = alg_id, .digest = {0} };
> > int rc;
> > u32 i;
> > SHASH_DESC_ON_STACK(shash, tfm);
> >
> > shash->tfm = tfm;
> >
> > + pr_devel("calculating the boot-aggregate based on TPM
> bank: %04x\n",
> > + d.alg_id);
> > +
>
> Good
>
> > rc = crypto_shash_init(shash);
> > if (rc != 0)
> > return rc;
> > @@ -675,7 +686,8 @@ static int __init ima_calc_boot_aggregate_tfm(char
> *digest,
> > for (i = TPM_PCR0; i < TPM_PCR8; i++) {
> > ima_pcrread(i, &d);
> > /* now accumulate with current aggregate */
> > - rc = crypto_shash_update(shash, d.digest,
> TPM_DIGEST_SIZE);
> > + rc = crypto_shash_update(shash, d.digest,
> > + crypto_shash_digestsize(tfm));
> > }
> > if (!rc)
> > crypto_shash_final(shash, digest);
> > @@ -685,14 +697,35 @@ static int __init
> ima_calc_boot_aggregate_tfm(char *digest,
> > int __init ima_calc_boot_aggregate(struct ima_digest_data *hash)
> > {
> >
> < snip >
> >
> > hash->length = crypto_shash_digestsize(tfm);
> > - rc = ima_calc_boot_aggregate_tfm(hash->digest, tfm);
> > + alg_id = ima_tpm_chip->allocated_banks[bank_idx].alg_id;
> > + rc = ima_calc_boot_aggregate_tfm(hash->digest, alg_id, tfm);
>
> Sure, backporting this change to ima_calc_boot_aggregate_tfm() should
> be fine.
>
> Mimi
>
> > ima_free_tfm(tfm);
> >
^ permalink raw reply
* Re: [GIT PULL] Smack patch for v5.6
From: pr-tracker-bot @ 2020-02-06 8:20 UTC (permalink / raw)
To: Casey Schaufler
Cc: Linux Security Module list, LKML, Linus Torvalds, Casey Schaufler
In-Reply-To: <7300e604-6fb5-16b1-4e1b-dfae7b494853@schaufler-ca.com>
The pull request you sent on Wed, 5 Feb 2020 18:07:09 -0800:
> https://github.com/cschaufler/smack-next tags/Smack-for-5.6
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/85e5529625f0e41beefe8939eb08baf33131f7d6
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* [GIT PULL] Smack patch for v5.6
From: Casey Schaufler @ 2020-02-06 2:07 UTC (permalink / raw)
To: Linux Security Module list, LKML, Linus Torvalds; +Cc: Casey Schaufler
In-Reply-To: <7300e604-6fb5-16b1-4e1b-dfae7b494853.ref@schaufler-ca.com>
Apologies for the late pull request. Testing on the originating
configuration was unfortunately delayed.
One fix for an obscure error found using an old version of
ping(1) that did not use IPv6 sockets in the documented way.
The following changes since commit d5226fa6dbae0569ee43ecfc08bdcd6770fc4755:
Linux 5.5 (2020-01-26 16:23:03 -0800)
are available in the Git repository at:
https://github.com/cschaufler/smack-next tags/Smack-for-5.6
for you to fetch changes up to 87fbfffcc89b92a4281b0aa53bd06af714087889:
broken ping to ipv6 linklocal addresses on debian buster (2020-02-05 14:16:27 -0800)
----------------------------------------------------------------
Smack change for Linux 5.6
----------------------------------------------------------------
Casey Schaufler (1):
broken ping to ipv6 linklocal addresses on debian buster
security/smack/smack_lsm.c | 41 +++++++++++++++++++----------------------
1 file changed, 19 insertions(+), 22 deletions(-)
^ permalink raw reply
* [RFC PATCH] ima: ima_init_ima_crypto() can be static
From: kbuild test robot @ 2020-02-05 23:41 UTC (permalink / raw)
To: Roberto Sassu
Cc: kbuild-all, zohar, James.Bottomley, jarkko.sakkinen,
linux-integrity, linux-security-module, linux-kernel,
silviu.vlasceanu, Roberto Sassu
In-Reply-To: <20200205103317.29356-7-roberto.sassu@huawei.com>
Fixes: d49f85a89d4e ("ima: Allocate and initialize tfm for each PCR bank")
Signed-off-by: kbuild test robot <lkp@intel.com>
---
ima_crypto.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 7b63a32eefea17..d865e26839ef6d 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -69,7 +69,7 @@ int ima_num_template_digests;
static struct ima_algo_desc *ima_algo_array;
-int __init ima_init_ima_crypto(void)
+static int __init ima_init_ima_crypto(void)
{
long rc;
^ permalink raw reply related
* Re: [PATCH v2 6/8] ima: Allocate and initialize tfm for each PCR bank
From: kbuild test robot @ 2020-02-05 23:41 UTC (permalink / raw)
To: Roberto Sassu
Cc: kbuild-all, zohar, James.Bottomley, jarkko.sakkinen,
linux-integrity, linux-security-module, linux-kernel,
silviu.vlasceanu, Roberto Sassu
In-Reply-To: <20200205103317.29356-7-roberto.sassu@huawei.com>
Hi Roberto,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on integrity/next-integrity]
[also build test WARNING on jss-tpmdd/next v5.5 next-20200205]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Roberto-Sassu/ima-support-stronger-algorithms-for-attestation/20200205-233901
base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-154-g1dc00f87-dirty
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> security/integrity/ima/ima_crypto.c:72:12: sparse: sparse: symbol 'ima_init_ima_crypto' was not declared. Should it be static?
security/integrity/ima/ima_crypto.c:545:36: sparse: sparse: invalid assignment: |=
security/integrity/ima/ima_crypto.c:545:36: sparse: left side has type unsigned int
security/integrity/ima/ima_crypto.c:545:36: sparse: right side has type restricted fmode_t
security/integrity/ima/ima_crypto.c:565:28: sparse: sparse: invalid assignment: &=
security/integrity/ima/ima_crypto.c:565:28: sparse: left side has type unsigned int
security/integrity/ima/ima_crypto.c:565:28: sparse: right side has type restricted fmode_t
security/integrity/ima/ima_crypto.c:592:52: sparse: sparse: restricted __le32 degrades to integer
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
^ permalink raw reply
* Re: [PATCH v2 1/8] tpm: Initialize crypto_id of allocated_banks to HASH_ALGO__LAST
From: Jarkko Sakkinen @ 2020-02-05 21:57 UTC (permalink / raw)
To: Roberto Sassu
Cc: zohar, James.Bottomley, linux-integrity, linux-security-module,
linux-kernel, silviu.vlasceanu, stable
In-Reply-To: <20200205103317.29356-2-roberto.sassu@huawei.com>
On Wed, Feb 05, 2020 at 11:33:10AM +0100, Roberto Sassu wrote:
> chip->allocated_banks, an array of tpm_bank_info structures, 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 of tpm_bank_info 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).
>
> Thus, initialize crypto_id to HASH_ALGO__LAST.
>
> Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> Cc: stable@vger.kernel.org
Cc should be first.
/Jarkko
^ permalink raw reply
* Re: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate
From: Mimi Zohar @ 2020-02-05 21:00 UTC (permalink / raw)
To: Roberto Sassu, James.Bottomley, jarkko.sakkinen
Cc: linux-integrity, linux-security-module, linux-kernel,
silviu.vlasceanu, stable
In-Reply-To: <20200205103317.29356-3-roberto.sassu@huawei.com>
Hi Roberto,
On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
<snip>
> Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Cc: stable@vger.kernel.org
Cc'ing stable resulted in Sasha's automated message. If you're going
to Cc stable, then please include the stable kernel release (e.g. Cc:
stable@vger.kernel.org # v5.3). Also please include a "Fixes" tag.
Normally only bug fixes are backported.
Mimi
^ permalink raw reply
* Re: [PATCH v2 2/8] ima: Switch to ima_hash_algo for boot aggregate
From: Mimi Zohar @ 2020-02-05 20:41 UTC (permalink / raw)
To: Roberto Sassu, James.Bottomley, jarkko.sakkinen
Cc: linux-integrity, linux-security-module, linux-kernel,
silviu.vlasceanu, stable
In-Reply-To: <20200205103317.29356-3-roberto.sassu@huawei.com>
On Wed, 2020-02-05 at 11:33 +0100, Roberto Sassu wrote:
> boot_aggregate is the first entry of IMA measurement list. Its purpose is
> to link pre-boot measurements to IMA measurements. As IMA was designed to
> work with a TPM 1.2, the SHA1 PCR bank was always selected.
>
> Currently, even if a TPM 2.0 is used, the SHA1 PCR bank is selected.
> However, the assumption that the SHA1 PCR bank is always available is not
> correct, as PCR banks can be selected with the PCR_Allocate() TPM command.
>
> This patch tries to use ima_hash_algo as hash algorithm for boot_aggregate.
> If no PCR bank uses that algorithm, the patch tries to find the SHA256 PCR
> bank (which is mandatory in the TCG PC Client specification). If also this
> bank is not found, the patch selects the first one. If the TPM algorithm
> of that bank is not mapped to a crypto ID, boot_aggregate is set to zero.
>
> Changelog
>
> v1:
> - add Mimi's comments
> - if there is no PCR bank for the IMA default algorithm use SHA256 or the
> first bank (suggested by James Bottomley)
If the IMA default hash algorithm is not enabled, James' comment was
to use SHA256 for TPM 2.0 and SHA1 for TPM 1.2. I don't remember him
saying anything about using the first bank, that was in v1 of my
patch. Please refer to v2 of my patch, based on James' comments.
>
> Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Cc: stable@vger.kernel.org
> ---
> security/integrity/ima/ima_crypto.c | 45 +++++++++++++++++++++++++----
> security/integrity/ima/ima_init.c | 22 ++++++++++----
> 2 files changed, 56 insertions(+), 11 deletions(-)
>
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 73044fc6a952..f2f41a2bc3d4 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -655,18 +655,29 @@ static void __init ima_pcrread(u32 idx, struct tpm_digest *d)
> }
>
> /*
> - * Calculate the boot aggregate hash
> + * The boot_aggregate is a cumulative hash over TPM registers 0 - 7. With
> + * TPM 1.2 the boot_aggregate was based on reading the SHA1 PCRs, but with
> + * TPM 2.0 hash agility, TPM chips could support multiple TPM PCR banks,
> + * allowing firmware to configure and enable different banks.
> + *
> + * Knowing which TPM bank is read to calculate the boot_aggregate digest
> + * needs to be conveyed to a verifier. For this reason, use the same
> + * hash algorithm for reading the TPM PCRs as for calculating the boot
> + * aggregate digest as stored in the measurement list.
> */
> -static int __init ima_calc_boot_aggregate_tfm(char *digest,
> +static int __init ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
> struct crypto_shash *tfm)
> {
> - struct tpm_digest d = { .alg_id = TPM_ALG_SHA1, .digest = {0} };
> + struct tpm_digest d = { .alg_id = alg_id, .digest = {0} };
> int rc;
> u32 i;
> SHASH_DESC_ON_STACK(shash, tfm);
>
> shash->tfm = tfm;
>
> + pr_devel("calculating the boot-aggregate based on TPM bank: %04x\n",
> + d.alg_id);
> +
Good
> rc = crypto_shash_init(shash);
> if (rc != 0)
> return rc;
> @@ -675,7 +686,8 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest,
> for (i = TPM_PCR0; i < TPM_PCR8; i++) {
> ima_pcrread(i, &d);
> /* now accumulate with current aggregate */
> - rc = crypto_shash_update(shash, d.digest, TPM_DIGEST_SIZE);
> + rc = crypto_shash_update(shash, d.digest,
> + crypto_shash_digestsize(tfm));
> }
> if (!rc)
> crypto_shash_final(shash, digest);
> @@ -685,14 +697,35 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest,
> int __init ima_calc_boot_aggregate(struct ima_digest_data *hash)
> {
>
< snip >
>
> hash->length = crypto_shash_digestsize(tfm);
> - rc = ima_calc_boot_aggregate_tfm(hash->digest, tfm);
> + alg_id = ima_tpm_chip->allocated_banks[bank_idx].alg_id;
> + rc = ima_calc_boot_aggregate_tfm(hash->digest, alg_id, tfm);
Sure, backporting this change to ima_calc_boot_aggregate_tfm() should
be fine.
Mimi
> ima_free_tfm(tfm);
>
^ permalink raw reply
* [PATCH v6 10/10] drivers/oprofile: open access for CAP_PERFMON privileged process
From: Alexey Budankov @ 2020-02-05 17:37 UTC (permalink / raw)
To: James Morris, Serge Hallyn, Stephen Smalley, Peter Zijlstra,
Arnaldo Carvalho de Melo, Ingo Molnar,
joonas.lahtinen@linux.intel.com, Alexei Starovoitov, Will Deacon,
Paul Mackerras, Michael Ellerman
Cc: Andi Kleen, Thomas Gleixner, Stephane Eranian, Igor Lubashev,
Jiri Olsa, linux-kernel, intel-gfx@lists.freedesktop.org,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
linux-arm-kernel, linuxppc-dev@lists.ozlabs.org,
linux-parisc@vger.kernel.org, oprofile-list
In-Reply-To: <576a6141-36d4-14c0-b395-8d195892b916@linux.intel.com>
Open access to monitoring for CAP_PERFMON privileged process.
Providing the access under CAP_PERFMON capability singly, without
the rest of CAP_SYS_ADMIN credentials, excludes chances to misuse
the credentials and makes operation more secure.
CAP_PERFMON implements the principal of least privilege for performance
monitoring and observability operations (POSIX IEEE 1003.1e 2.2.2.39
principle of least privilege: A security design principle that states
that a process or program be granted only those privileges (e.g.,
capabilities) necessary to accomplish its legitimate function, and only
for the time that such privileges are actually required)
For backward compatibility reasons access to the monitoring remains
open for CAP_SYS_ADMIN privileged processes but CAP_SYS_ADMIN usage
for secure monitoring is discouraged with respect to CAP_PERFMON
capability.
Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
drivers/oprofile/event_buffer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/oprofile/event_buffer.c b/drivers/oprofile/event_buffer.c
index 12ea4a4ad607..6c9edc8bbc95 100644
--- a/drivers/oprofile/event_buffer.c
+++ b/drivers/oprofile/event_buffer.c
@@ -113,7 +113,7 @@ static int event_buffer_open(struct inode *inode, struct file *file)
{
int err = -EPERM;
- if (!capable(CAP_SYS_ADMIN))
+ if (!perfmon_capable())
return -EPERM;
if (test_and_set_bit_lock(0, &buffer_opened))
--
2.20.1
^ permalink raw reply related
* [PATCH v6 09/10] drivers/perf: open access for CAP_PERFMON privileged process
From: Alexey Budankov @ 2020-02-05 17:36 UTC (permalink / raw)
To: James Morris, Serge Hallyn, Stephen Smalley, Peter Zijlstra,
Arnaldo Carvalho de Melo, Ingo Molnar,
joonas.lahtinen@linux.intel.com, Alexei Starovoitov, Will Deacon,
Paul Mackerras, Michael Ellerman
Cc: Andi Kleen, Thomas Gleixner, Stephane Eranian, Igor Lubashev,
Jiri Olsa, linux-kernel, intel-gfx@lists.freedesktop.org,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
linux-arm-kernel, linuxppc-dev@lists.ozlabs.org,
linux-parisc@vger.kernel.org, oprofile-list
In-Reply-To: <576a6141-36d4-14c0-b395-8d195892b916@linux.intel.com>
Open access to monitoring for CAP_PERFMON privileged process.
Providing the access under CAP_PERFMON capability singly, without
the rest of CAP_SYS_ADMIN credentials, excludes chances to misuse
the credentials and makes operation more secure.
CAP_PERFMON implements the principal of least privilege for performance
monitoring and observability operations (POSIX IEEE 1003.1e 2.2.2.39
principle of least privilege: A security design principle that states
that a process or program be granted only those privileges (e.g.,
capabilities) necessary to accomplish its legitimate function, and
only for the time that such privileges are actually required)
For backward compatibility reasons access to the monitoring remains
open for CAP_SYS_ADMIN privileged processes but CAP_SYS_ADMIN usage
for secure monitoring is discouraged with respect to CAP_PERFMON
capability.
Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
drivers/perf/arm_spe_pmu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index 4e4984a55cd1..5dff81bc3324 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -274,7 +274,7 @@ static u64 arm_spe_event_to_pmscr(struct perf_event *event)
if (!attr->exclude_kernel)
reg |= BIT(SYS_PMSCR_EL1_E1SPE_SHIFT);
- if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && capable(CAP_SYS_ADMIN))
+ if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable())
reg |= BIT(SYS_PMSCR_EL1_CX_SHIFT);
return reg;
@@ -700,7 +700,7 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
return -EOPNOTSUPP;
reg = arm_spe_event_to_pmscr(event);
- if (!capable(CAP_SYS_ADMIN) &&
+ if (!perfmon_capable() &&
(reg & (BIT(SYS_PMSCR_EL1_PA_SHIFT) |
BIT(SYS_PMSCR_EL1_CX_SHIFT) |
BIT(SYS_PMSCR_EL1_PCT_SHIFT))))
--
2.20.1
^ permalink raw reply related
* [PATCH v6 08/10] parisc/perf: open access for CAP_PERFMON privileged process
From: Alexey Budankov @ 2020-02-05 17:35 UTC (permalink / raw)
To: James Morris, Serge Hallyn, Stephen Smalley, Peter Zijlstra,
Arnaldo Carvalho de Melo, Ingo Molnar,
joonas.lahtinen@linux.intel.com, Alexei Starovoitov, Will Deacon,
Paul Mackerras, Michael Ellerman
Cc: Andi Kleen, Thomas Gleixner, Stephane Eranian, Igor Lubashev,
Jiri Olsa, linux-kernel, intel-gfx@lists.freedesktop.org,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
linux-arm-kernel, linuxppc-dev@lists.ozlabs.org,
linux-parisc@vger.kernel.org, oprofile-list
In-Reply-To: <576a6141-36d4-14c0-b395-8d195892b916@linux.intel.com>
Open access to monitoring for CAP_PERFMON privileged process.
Providing the access under CAP_PERFMON capability singly, without
the rest of CAP_SYS_ADMIN credentials, excludes chances to misuse
the credentials and makes operation more secure.
CAP_PERFMON implements the principal of least privilege for performance
monitoring and observability operations (POSIX IEEE 1003.1e 2.2.2.39
principle of least privilege: A security design principle that states
that a process or program be granted only those privileges (e.g.,
capabilities) necessary to accomplish its legitimate function, and
only for the time that such privileges are actually required)
For backward compatibility reasons access to the monitoring remains
open for CAP_SYS_ADMIN privileged processes but CAP_SYS_ADMIN usage
for secure monitoring is discouraged with respect to CAP_PERFMON
capability.
Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
arch/parisc/kernel/perf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/parisc/kernel/perf.c b/arch/parisc/kernel/perf.c
index 676683641d00..c4208d027794 100644
--- a/arch/parisc/kernel/perf.c
+++ b/arch/parisc/kernel/perf.c
@@ -300,7 +300,7 @@ static ssize_t perf_write(struct file *file, const char __user *buf,
else
return -EFAULT;
- if (!capable(CAP_SYS_ADMIN))
+ if (!perfmon_capable())
return -EACCES;
if (count != sizeof(uint32_t))
--
2.20.1
^ permalink raw reply related
* [PATCH v6 07/10] powerpc/perf: open access for CAP_PERFMON privileged process
From: Alexey Budankov @ 2020-02-05 17:35 UTC (permalink / raw)
To: James Morris, Serge Hallyn, Stephen Smalley, Peter Zijlstra,
Arnaldo Carvalho de Melo, Ingo Molnar,
joonas.lahtinen@linux.intel.com, Alexei Starovoitov, Will Deacon,
Paul Mackerras, Michael Ellerman
Cc: Andi Kleen, Thomas Gleixner, Stephane Eranian, Igor Lubashev,
Jiri Olsa, linux-kernel, intel-gfx@lists.freedesktop.org,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
linux-arm-kernel, linuxppc-dev@lists.ozlabs.org,
linux-parisc@vger.kernel.org, oprofile-list
In-Reply-To: <576a6141-36d4-14c0-b395-8d195892b916@linux.intel.com>
Open access to monitoring for CAP_PERFMON privileged process.
Providing the access under CAP_PERFMON capability singly, without
the rest of CAP_SYS_ADMIN credentials, excludes chances to misuse
the credentials and makes operation more secure.
CAP_PERFMON implements the principal of least privilege for performance
monitoring and observability operations (POSIX IEEE 1003.1e 2.2.2.39
principle of least privilege: A security design principle that states
that a process or program be granted only those privileges (e.g.,
capabilities) necessary to accomplish its legitimate function, and
only for the time that such privileges are actually required)
For backward compatibility reasons access to the monitoring remains
open for CAP_SYS_ADMIN privileged processes but CAP_SYS_ADMIN usage
for secure monitoring is discouraged with respect to CAP_PERFMON
capability.
Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
arch/powerpc/perf/imc-pmu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
index cb50a9e1fd2d..e837717492e4 100644
--- a/arch/powerpc/perf/imc-pmu.c
+++ b/arch/powerpc/perf/imc-pmu.c
@@ -898,7 +898,7 @@ static int thread_imc_event_init(struct perf_event *event)
if (event->attr.type != event->pmu->type)
return -ENOENT;
- if (!capable(CAP_SYS_ADMIN))
+ if (!perfmon_capable())
return -EACCES;
/* Sampling not supported */
@@ -1307,7 +1307,7 @@ static int trace_imc_event_init(struct perf_event *event)
if (event->attr.type != event->pmu->type)
return -ENOENT;
- if (!capable(CAP_SYS_ADMIN))
+ if (!perfmon_capable())
return -EACCES;
/* Return if this is a couting event */
--
2.20.1
^ 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