* Re: [PATCH 2/6] Adjust watch_queue documentation to mention mount and superblock watches. [ver #5]
From: David Howells @ 2019-07-01 8:52 UTC (permalink / raw)
To: Randy Dunlap
Cc: dhowells, viro, Casey Schaufler, Stephen Smalley,
Greg Kroah-Hartman, nicolas.dichtel, raven, Christian Brauner,
keyrings, linux-usb, linux-security-module, linux-fsdevel,
linux-api, linux-block, linux-kernel
In-Reply-To: <7a288c2c-11a1-87df-9550-b247d6ce3010@infradead.org>
Randy Dunlap <rdunlap@infradead.org> wrote:
> I'm having a little trouble parsing that sentence.
> Could you clarify it or maybe rewrite/modify it?
> Thanks.
How about:
* ``info_filter`` and ``info_mask`` act as a filter on the info field of the
notification record. The notification is only written into the buffer if::
(watch.info & info_mask) == info_filter
This could be used, for example, to ignore events that are not exactly on
the watched point in a mount tree by specifying NOTIFY_MOUNT_IN_SUBTREE
must not be set, e.g.::
{
.type = WATCH_TYPE_MOUNT_NOTIFY,
.info_filter = 0,
.info_mask = NOTIFY_MOUNT_IN_SUBTREE,
.subtype_filter = ...,
}
as an event would be only permissible with this filter if::
(watch.info & NOTIFY_MOUNT_IN_SUBTREE) == 0
David
^ permalink raw reply
* Re: [RFC PATCH v5 0/1] Add dm verity root hash pkcs7 sig validation.
From: Milan Broz @ 2019-07-01 9:41 UTC (permalink / raw)
To: James Morris, Eric Biggers
Cc: Jaskaran Khurana, linux-security-module, linux-kernel,
linux-integrity, linux-fsdevel, agk, snitzer, dm-devel, scottsh,
mpatocka
In-Reply-To: <alpine.LRH.2.21.1906282040490.15624@namei.org>
On 29/06/2019 06:01, James Morris wrote:
> On Thu, 27 Jun 2019, Eric Biggers wrote:
>
>> I don't understand your justification for this feature.
>>
>> If userspace has already been pwned severely enough for the attacker to be
>> executing arbitrary code with CAP_SYS_ADMIN (which is what the device mapper
>> ioctls need), what good are restrictions on loading more binaries from disk?
>>
>> Please explain your security model.
>
> Let's say the system has a policy where all code must be signed with a
> valid key, and that one mechanism for enforcing this is via signed
> dm-verity volumes. Validating the signature within the kernel provides
> stronger assurance than userspace validation. The kernel validates and
> executes the code, using kernel-resident keys, and does not need to rely
> on validation which has occurred across a trust boundary.
Yes, but as it is implemented in this patch, a certificate is provided as
a binary blob by the (super)user that activates the dm-verity device.
Actually, I can put there anything that looks like a correct signature (self-signed
or so), and dm-verity code is happy because the root hash is now signed.
Maybe could this concept be extended to support in-kernel compiled certificates?
I like the idea of signed root hash, but the truth is that if you have access
to device activation, it brings nothing, you can just put any cert in the keyring
and use it.
Milan
>
> You don't need arbitrary CAP_SYS_ADMIN code execution, you just need a
> flaw in the app (or its dependent libraries, or configuration) which
> allows signature validation to be bypassed.
>
> The attacker now needs a kernel rather than a userspace vulnerability to
> bypass the signed code policy.
>
^ permalink raw reply
* Re: [PATCH v4 2/3] initramfs: read metadata from special file METADATA!!!
From: Mimi Zohar @ 2019-07-01 12:54 UTC (permalink / raw)
To: Roberto Sassu, viro
Cc: linux-security-module, linux-integrity, initramfs, linux-api,
linux-fsdevel, linux-kernel, bug-cpio, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, hpa, arnd, rob,
james.w.mcmechan, niveditas98
In-Reply-To: <20190523121803.21638-3-roberto.sassu@huawei.com>
Hi Roberto,
> diff --git a/init/initramfs.c b/init/initramfs.c
> index 5de396a6aac0..862c03123de8 100644
> --- a/init/initramfs.c
> +++ b/init/initramfs.c
> +static int __init do_process_metadata(char *buf, int len, bool last)
> +{
Part of the problem in upstreaming CPIO xattr support has been the
difficulty in reading and understanding the initramfs code due to a
lack of comments. At least for any new code, let's add some comments
to simplify the review. In this case, understanding "last", before
reading the code, would help.
Mimi
> + int ret = 0;
> +
> + if (!metadata_buf) {
> + metadata_buf_ptr = metadata_buf = kmalloc(body_len, GFP_KERNEL);
> + if (!metadata_buf_ptr) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + metadata_len = body_len;
> + }
> +
> + if (metadata_buf_ptr + len > metadata_buf + metadata_len) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + memcpy(metadata_buf_ptr, buf, len);
> + metadata_buf_ptr += len;
> +
> + if (last)
> + do_parse_metadata(previous_name_buf);
> +out:
> + if (ret < 0 || last) {
> + kfree(metadata_buf);
> + metadata_buf = NULL;
> + metadata = 0;
> + }
> +
> + return ret;
> +}
> +
> static int __init do_copy(void)
> {
> if (byte_count >= body_len) {
> if (xwrite(wfd, victim, body_len) != body_len)
> error("write error");
> + if (metadata)
> + do_process_metadata(victim, body_len, true);
> ksys_close(wfd);
> do_utime(vcollected, mtime);
> kfree(vcollected);
> @@ -458,6 +500,8 @@ static int __init do_copy(void)
> } else {
> if (xwrite(wfd, victim, byte_count) != byte_count)
> error("write error");
> + if (metadata)
> + do_process_metadata(victim, byte_count, false);
> body_len -= byte_count;
> eat(byte_count);
> return 1;
>
^ permalink raw reply
* [PATCH] Revert "tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()"
From: Michal Suchanek @ 2019-07-01 13:15 UTC (permalink / raw)
To: linux-integrity
Cc: Michal Suchanek, Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe,
Arnd Bergmann, Greg Kroah-Hartman, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, James Bottomley, David Howells,
Roberto Sassu, Tomas Winkler, Armijn Hemel, Stefan Berger,
Jerry Snitselaar, Thomas Gleixner, linux-kernel,
linux-security-module, keyrings
This reverts commit 0b6cf6b97b7ef1fa3c7fefab0cac897a1c4a3400 to avoid following crash:
BUG: Kernel NULL pointer dereference at 0x00000012
Faulting instruction address: 0xc000000000897908
Oops: Kernel access of bad area, sig: 11 [#1]
LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc6-2.gf99f70b-default #1 openSUSE Tumbleweed (unreleased)
NIP: c000000000897908 LR: c000000000897860 CTR: 0000000000000009
REGS: c000000004eb7550 TRAP: 0380 Not tainted (5.2.0-rc6-2.gf99f70b-default)
MSR: 800000010280b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE,TM[E]> CR: 88000224 XER: 20040001
CFAR: c000000000c4e964 IRQMASK: 0
GPR00: c000000000897860 c000000004eb77e0 c0000000015ced00 0000000000000000
GPR04: 0000000000000003 0000000000000001 0000000022000000 000000000000000e
GPR08: c0000005b46b0000 0000000000010000 0000000000000022 0000000010325476
GPR12: 0000000048000222 c0000000019a0000 c000000000010b70 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR24: c000000000eded00 c0000000014ecb60 c000000000ccedf8 c0000005b5c9d000
GPR28: 000000000000000a 0000000000000012 c000000000f01180 c00c0000016d1ac0
NIP [c000000000897908] tpm1_pcr_extend+0x118/0x1d0
LR [c000000000897860] tpm1_pcr_extend+0x70/0x1d0
Call Trace:
[c000000004eb77e0] [c000000000897860] tpm1_pcr_extend+0x70/0x1d0 (unreliable)
[c000000004eb7890] [c0000000008964e4] tpm_pcr_extend+0xe4/0x170
[c000000004eb78d0] [c000000000667bac] ima_add_template_entry+0x1ac/0x350
[c000000004eb79b0] [c00000000066ad04] ima_store_template+0xc4/0x150
[c000000004eb7a30] [c000000001021a4c] ima_add_boot_aggregate+0xec/0x17c
[c000000004eb7b30] [c000000001021b70] ima_init+0x94/0xbc
[c000000004eb7b90] [c000000001021ce0] init_ima+0x44/0xec
[c000000004eb7c10] [c000000000010694] do_one_initcall+0x64/0x2b0
[c000000004eb7ce0] [c000000000fd445c] kernel_init_freeable+0x2e4/0x3cc
[c000000004eb7db0] [c000000000010b94] kernel_init+0x2c/0x148
[c000000004eb7e20] [c00000000000bc54] ret_from_kernel_thread+0x5c/0x68
Instruction dump:
90c80002 81410070 714a0001 e9010078 39480002 7d40542c 79470020 4082003c
394a0014 7c0a4840 41810090 5546c03e <e87d0000> e89d0008 80bd0010 7d283a14
---[ end trace 786ebab24be797a3 ]---
---
drivers/char/tpm/tpm-interface.c | 30 +++++++++++++--------
drivers/char/tpm/tpm.h | 2 +-
drivers/char/tpm/tpm2-cmd.c | 10 ++++---
include/linux/tpm.h | 5 ++--
security/integrity/ima/ima.h | 1 -
security/integrity/ima/ima_init.c | 4 ---
security/integrity/ima/ima_queue.c | 27 +------------------
security/keys/trusted.c | 42 ++++++------------------------
8 files changed, 38 insertions(+), 83 deletions(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1b4f95c13e00..dda742184ce2 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -303,34 +303,42 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
* tpm_pcr_extend - extend a PCR value in SHA1 bank.
* @chip: a &struct tpm_chip instance, %NULL for the default chip
* @pcr_idx: the PCR to be retrieved
- * @digests: array of tpm_digest structures used to extend PCRs
+ * @hash: the hash value used to extend the PCR value
*
- * Note: callers must pass a digest for every allocated PCR bank, in the same
- * order of the banks in chip->allocated_banks.
+ * Note: with TPM 2.0 extends also those banks for which no digest was
+ * specified in order to prevent malicious use of those PCR banks.
*
* Return: same as with tpm_transmit_cmd()
*/
-int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
- struct tpm_digest *digests)
+int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
{
int rc;
+ struct tpm_digest *digest_list;
int i;
chip = tpm_find_get_ops(chip);
if (!chip)
return -ENODEV;
- for (i = 0; i < chip->nr_allocated_banks; i++)
- if (digests[i].alg_id != chip->allocated_banks[i].alg_id)
- return -EINVAL;
-
if (chip->flags & TPM_CHIP_FLAG_TPM2) {
- rc = tpm2_pcr_extend(chip, pcr_idx, digests);
+ digest_list = kcalloc(chip->nr_allocated_banks,
+ sizeof(*digest_list), GFP_KERNEL);
+ if (!digest_list)
+ return -ENOMEM;
+
+ for (i = 0; i < chip->nr_allocated_banks; i++) {
+ digest_list[i].alg_id = chip->allocated_banks[i].alg_id;
+ memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
+ }
+
+ rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks,
+ digest_list);
+ kfree(digest_list);
tpm_put_ops(chip);
return rc;
}
- rc = tpm1_pcr_extend(chip, pcr_idx, digests[0].digest,
+ rc = tpm1_pcr_extend(chip, pcr_idx, hash,
"attempting extend a PCR value");
tpm_put_ops(chip);
return rc;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index e503ffc3aa39..3cf8ed290305 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -436,7 +436,7 @@ static inline u32 tpm2_rc_value(u32 rc)
int tpm2_get_timeouts(struct tpm_chip *chip);
int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
struct tpm_digest *digest, u16 *digest_size_ptr);
-int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
+int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
struct tpm_digest *digests);
int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 4de49924cfc4..231937d5f58c 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -242,11 +242,12 @@ struct tpm2_null_auth_area {
*
* @chip: TPM chip to use.
* @pcr_idx: index of the PCR.
+ * @count: number of digests passed.
* @digests: list of pcr banks and corresponding digest values to extend.
*
* Return: Same as with tpm_transmit_cmd.
*/
-int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
+int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
struct tpm_digest *digests)
{
struct tpm_buf buf;
@@ -254,6 +255,9 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
int rc;
int i;
+ if (count > chip->nr_allocated_banks)
+ return -EINVAL;
+
rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
if (rc)
return rc;
@@ -268,9 +272,9 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area));
tpm_buf_append(&buf, (const unsigned char *)&auth_area,
sizeof(auth_area));
- tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
+ tpm_buf_append_u32(&buf, count);
- for (i = 0; i < chip->nr_allocated_banks; i++) {
+ for (i = 0; i < count; i++) {
tpm_buf_append_u16(&buf, digests[i].alg_id);
tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
chip->allocated_banks[i].digest_size);
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 53c0ea9ec9df..d4516164dc9f 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -166,8 +166,7 @@ struct tpm_chip {
extern int tpm_is_tpm2(struct tpm_chip *chip);
extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
struct tpm_digest *digest);
-extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
- struct tpm_digest *digests);
+extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash);
extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen);
extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max);
extern int tpm_seal_trusted(struct tpm_chip *chip,
@@ -190,7 +189,7 @@ static inline int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
}
static inline int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
- struct tpm_digest *digests)
+ const u8 *hash)
{
return -ENODEV;
}
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index ca10917b5f89..bf7178ef1619 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -149,7 +149,6 @@ int ima_measurements_show(struct seq_file *m, void *v);
unsigned long ima_get_binary_runtime_size(void);
int ima_init_template(void);
void ima_init_template_list(void);
-int __init ima_init_digests(void);
/*
* used to protect h_table and sha_table
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 1e47c1026471..cf7a2f58077e 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -119,12 +119,8 @@ int __init ima_init(void)
if (rc != 0)
return rc;
- /* It can be called before ima_init_digests(), it does not use TPM. */
ima_load_kexec_buffer();
- rc = ima_init_digests();
- if (rc != 0)
- return rc;
rc = ima_add_boot_aggregate(); /* boot aggregate must be first entry */
if (rc != 0)
return rc;
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 1ce8b1701566..03afa67c2e04 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -23,9 +23,6 @@
#define AUDIT_CAUSE_LEN_MAX 32
-/* pre-allocated array of tpm_digest structures to extend a PCR */
-static struct tpm_digest *digests;
-
LIST_HEAD(ima_measurements); /* list of all measurements */
#ifdef CONFIG_IMA_KEXEC
static unsigned long binary_runtime_size;
@@ -139,15 +136,11 @@ unsigned long ima_get_binary_runtime_size(void)
static int ima_pcr_extend(const u8 *hash, int pcr)
{
int result = 0;
- int i;
if (!ima_tpm_chip)
return result;
- for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++)
- memcpy(digests[i].digest, hash, TPM_DIGEST_SIZE);
-
- result = tpm_pcr_extend(ima_tpm_chip, pcr, digests);
+ result = tpm_pcr_extend(ima_tpm_chip, pcr, hash);
if (result != 0)
pr_err("Error Communicating to TPM chip, result: %d\n", result);
return result;
@@ -214,21 +207,3 @@ int ima_restore_measurement_entry(struct ima_template_entry *entry)
mutex_unlock(&ima_extend_list_mutex);
return result;
}
-
-int __init ima_init_digests(void)
-{
- int i;
-
- if (!ima_tpm_chip)
- return 0;
-
- digests = kcalloc(ima_tpm_chip->nr_allocated_banks, sizeof(*digests),
- GFP_NOFS);
- if (!digests)
- return -ENOMEM;
-
- for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++)
- digests[i].alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
-
- return 0;
-}
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index 9a94672e7adc..287553039f0f 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -32,7 +32,6 @@
static const char hmac_alg[] = "hmac(sha1)";
static const char hash_alg[] = "sha1";
static struct tpm_chip *chip;
-static struct tpm_digest *digests;
struct sdesc {
struct shash_desc shash;
@@ -386,10 +385,15 @@ EXPORT_SYMBOL_GPL(trusted_tpm_send);
*/
static int pcrlock(const int pcrnum)
{
+ unsigned char hash[SHA1_DIGEST_SIZE];
+ int ret;
+
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
-
- return tpm_pcr_extend(chip, pcrnum, digests) ? -EINVAL : 0;
+ ret = tpm_get_random(chip, hash, SHA1_DIGEST_SIZE);
+ if (ret != SHA1_DIGEST_SIZE)
+ return ret;
+ return tpm_pcr_extend(chip, pcrnum, hash) ? -EINVAL : 0;
}
/*
@@ -1226,29 +1230,6 @@ static int __init trusted_shash_alloc(void)
return ret;
}
-static int __init init_digests(void)
-{
- u8 digest[TPM_MAX_DIGEST_SIZE];
- int ret;
- int i;
-
- ret = tpm_get_random(chip, digest, TPM_MAX_DIGEST_SIZE);
- if (ret < 0)
- return ret;
- if (ret < TPM_MAX_DIGEST_SIZE)
- return -EFAULT;
-
- digests = kcalloc(chip->nr_allocated_banks, sizeof(*digests),
- GFP_KERNEL);
- if (!digests)
- return -ENOMEM;
-
- for (i = 0; i < chip->nr_allocated_banks; i++)
- memcpy(digests[i].digest, digest, TPM_MAX_DIGEST_SIZE);
-
- return 0;
-}
-
static int __init init_trusted(void)
{
int ret;
@@ -1259,21 +1240,15 @@ static int __init init_trusted(void)
chip = tpm_default_chip();
if (!chip)
return 0;
-
- ret = init_digests();
- if (ret < 0)
- goto err_put;
ret = trusted_shash_alloc();
if (ret < 0)
- goto err_free;
+ goto err_put;
ret = register_key_type(&key_type_trusted);
if (ret < 0)
goto err_release;
return 0;
err_release:
trusted_shash_release();
-err_free:
- kfree(digests);
err_put:
put_device(&chip->dev);
return ret;
@@ -1283,7 +1258,6 @@ static void __exit cleanup_trusted(void)
{
if (chip) {
put_device(&chip->dev);
- kfree(digests);
trusted_shash_release();
unregister_key_type(&key_type_trusted);
}
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v4 0/3] initramfs: add support for xattrs in the initial ram disk
From: Mimi Zohar @ 2019-07-01 13:22 UTC (permalink / raw)
To: Roberto Sassu, viro
Cc: linux-security-module, linux-integrity, initramfs, linux-api,
linux-fsdevel, linux-kernel, bug-cpio, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, hpa, arnd, rob,
james.w.mcmechan, niveditas98
In-Reply-To: <20190523121803.21638-1-roberto.sassu@huawei.com>
On Thu, 2019-05-23 at 14:18 +0200, Roberto Sassu wrote:
> This patch set aims at solving the following use case: appraise files from
> the initial ram disk. To do that, IMA checks the signature/hash from the
> security.ima xattr. Unfortunately, this use case cannot be implemented
> currently, as the CPIO format does not support xattrs.
>
> This proposal consists in including file metadata as additional files named
> METADATA!!!, for each file added to the ram disk. The CPIO parser in the
> kernel recognizes these special files from the file name, and calls the
> appropriate parser to add metadata to the previously extracted file. It has
> been proposed to use bit 17:16 of the file mode as a way to recognize files
> with metadata, but both the kernel and the cpio tool declare the file mode
> as unsigned short.
Thanks, Roberto!
Victor, Taras, Rob, Arvind, Peter, if you're good with this latest
design, could we get some Reviewed-by, Acked-by, or Tested-by?
thanks!
Mimi
^ permalink raw reply
* Re: [PATCH v4 0/3] initramfs: add support for xattrs in the initial ram disk
From: Roberto Sassu @ 2019-07-01 13:42 UTC (permalink / raw)
To: Mimi Zohar, Rob Landley, viro
Cc: linux-security-module, linux-integrity, initramfs, linux-api,
linux-fsdevel, linux-kernel, bug-cpio, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, hpa, arnd, james.w.mcmechan,
niveditas98
In-Reply-To: <1561909199.3985.33.camel@linux.ibm.com>
On 6/30/2019 6:39 PM, Mimi Zohar wrote:
> On Wed, 2019-06-26 at 10:15 +0200, Roberto Sassu wrote:
>> On 6/3/2019 8:32 PM, Rob Landley wrote:
>>> On 6/3/19 4:31 AM, Roberto Sassu wrote:
>>>>> This patch set aims at solving the following use case: appraise files from
>>>>> the initial ram disk. To do that, IMA checks the signature/hash from the
>>>>> security.ima xattr. Unfortunately, this use case cannot be implemented
>>>>> currently, as the CPIO format does not support xattrs.
>>>>>
>>>>> This proposal consists in including file metadata as additional files named
>>>>> METADATA!!!, for each file added to the ram disk. The CPIO parser in the
>>>>> kernel recognizes these special files from the file name, and calls the
>>>>> appropriate parser to add metadata to the previously extracted file. It has
>>>>> been proposed to use bit 17:16 of the file mode as a way to recognize files
>>>>> with metadata, but both the kernel and the cpio tool declare the file mode
>>>>> as unsigned short.
>>>>
>>>> Any opinion on this patch set?
>>>>
>>>> Thanks
>>>>
>>>> Roberto
>>>
>>> Sorry, I've had the window open since you posted it but haven't gotten around to
>>> it. I'll try to build it later today.
>>>
>>> It does look interesting, and I have no objections to the basic approach. I
>>> should be able to add support to toybox cpio over a weekend once I've got the
>>> kernel doing it to test against.
>>
>> Ok.
>>
>> Let me give some instructions so that people can test this patch set.
>>
>> To add xattrs to the ram disk embedded in the kernel it is sufficient
>> to set CONFIG_INITRAMFS_FILE_METADATA="xattr" and
>> CONFIG_INITRAMFS_SOURCE="<file with xattr>" in the kernel configuration.
>>
>> To add xattrs to the external ram disk, it is necessary to patch cpio:
>>
>> https://github.com/euleros/cpio/commit/531cabc88e9ecdc3231fad6e4856869baa9a91ef
>> (xattr-v1 branch)
>>
>> and dracut:
>>
>> https://github.com/euleros/dracut/commit/a2dee56ea80495c2c1871bc73186f7b00dc8bf3b
>> (digest-lists branch)
>>
>> The same modification can be done for mkinitramfs (add '-e xattr' to the
>> cpio command line).
>>
>> To simplify the test, it would be sufficient to replace only the cpio
>> binary and the dracut script with the modified versions. For dracut, the
>> patch should be applied to the local dracut (after it has been renamed
>> to dracut.sh).
>>
>> Then, run:
>>
>> dracut -e xattr -I <file with xattr> (add -f to overwrite the ram disk)
>>
>> Xattrs can be seen by stopping the boot process for example by adding
>> rd.break to the kernel command line.
>
> A simple way of testing, without needing any changes other than the
> kernel patches, is to save the dracut temporary directory by supplying
> "--keep" on the dracut command line, calling
> usr/gen_initramfs_list.sh, followed by usr/gen_init_cpio with the "-e
> xattr" option.
Alternatively, follow the instructions to create the embedded ram disk
with xattrs, and use the existing external ram disk created with dracut
to check if xattrs are created.
Roberto
--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI
^ permalink raw reply
* Re: [PATCH v4 0/3] initramfs: add support for xattrs in the initial ram disk
From: Mimi Zohar @ 2019-07-01 14:31 UTC (permalink / raw)
To: Roberto Sassu, Rob Landley, viro
Cc: linux-security-module, linux-integrity, initramfs, linux-api,
linux-fsdevel, linux-kernel, bug-cpio, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, hpa, arnd, james.w.mcmechan,
niveditas98
In-Reply-To: <45164486-782f-a442-e442-6f56f9299c66@huawei.com>
On Mon, 2019-07-01 at 16:42 +0300, Roberto Sassu wrote:
> On 6/30/2019 6:39 PM, Mimi Zohar wrote:
> > On Wed, 2019-06-26 at 10:15 +0200, Roberto Sassu wrote:
> >> On 6/3/2019 8:32 PM, Rob Landley wrote:
> >>> On 6/3/19 4:31 AM, Roberto Sassu wrote:
> >>>>> This patch set aims at solving the following use case: appraise files from
> >>>>> the initial ram disk. To do that, IMA checks the signature/hash from the
> >>>>> security.ima xattr. Unfortunately, this use case cannot be implemented
> >>>>> currently, as the CPIO format does not support xattrs.
> >>>>>
> >>>>> This proposal consists in including file metadata as additional files named
> >>>>> METADATA!!!, for each file added to the ram disk. The CPIO parser in the
> >>>>> kernel recognizes these special files from the file name, and calls the
> >>>>> appropriate parser to add metadata to the previously extracted file. It has
> >>>>> been proposed to use bit 17:16 of the file mode as a way to recognize files
> >>>>> with metadata, but both the kernel and the cpio tool declare the file mode
> >>>>> as unsigned short.
> >>>>
> >>>> Any opinion on this patch set?
> >>>>
> >>>> Thanks
> >>>>
> >>>> Roberto
> >>>
> >>> Sorry, I've had the window open since you posted it but haven't gotten around to
> >>> it. I'll try to build it later today.
> >>>
> >>> It does look interesting, and I have no objections to the basic approach. I
> >>> should be able to add support to toybox cpio over a weekend once I've got the
> >>> kernel doing it to test against.
> >>
> >> Ok.
> >>
> >> Let me give some instructions so that people can test this patch set.
> >>
> >> To add xattrs to the ram disk embedded in the kernel it is sufficient
> >> to set CONFIG_INITRAMFS_FILE_METADATA="xattr" and
> >> CONFIG_INITRAMFS_SOURCE="<file with xattr>" in the kernel configuration.
> >>
> >> To add xattrs to the external ram disk, it is necessary to patch cpio:
> >>
> >> https://github.com/euleros/cpio/commit/531cabc88e9ecdc3231fad6e4856869baa9a91ef
> >> (xattr-v1 branch)
> >>
> >> and dracut:
> >>
> >> https://github.com/euleros/dracut/commit/a2dee56ea80495c2c1871bc73186f7b00dc8bf3b
> >> (digest-lists branch)
> >>
> >> The same modification can be done for mkinitramfs (add '-e xattr' to the
> >> cpio command line).
> >>
> >> To simplify the test, it would be sufficient to replace only the cpio
> >> binary and the dracut script with the modified versions. For dracut, the
> >> patch should be applied to the local dracut (after it has been renamed
> >> to dracut.sh).
> >>
> >> Then, run:
> >>
> >> dracut -e xattr -I <file with xattr> (add -f to overwrite the ram disk)
> >>
> >> Xattrs can be seen by stopping the boot process for example by adding
> >> rd.break to the kernel command line.
> >
> > A simple way of testing, without needing any changes other than the
> > kernel patches, is to save the dracut temporary directory by supplying
> > "--keep" on the dracut command line, calling
> > usr/gen_initramfs_list.sh, followed by usr/gen_init_cpio with the "-e
> > xattr" option.
>
> Alternatively, follow the instructions to create the embedded ram disk
> with xattrs, and use the existing external ram disk created with dracut
> to check if xattrs are created.
True, but this alternative is for those who normally use dracut to
create an initramfs, but don't want to update cpio or dracut.
Mimi
^ permalink raw reply
* Re: [PATCH v12 00/11] Appended signatures support for IMA appraisal
From: Mimi Zohar @ 2019-07-01 14:38 UTC (permalink / raw)
To: Thiago Jung Bauermann, linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Dmitry Kasatkin, James Morris,
Serge E. Hallyn, David Howells, David Woodhouse, Jessica Yu,
Herbert Xu, David S. Miller, Jonathan Corbet, AKASHI, Takahiro
In-Reply-To: <20190628021934.4260-1-bauerman@linux.ibm.com>
On Thu, 2019-06-27 at 23:19 -0300, Thiago Jung Bauermann wrote:
> Hello,
>
> This version is essentially identical to the last one.
>
> It is only a rebase on top of today's linux-integrity/next-queued-testing,
> prompted by conflicts with Prakhar Srivastava's patches to measure the
> kernel command line. It also drops two patches that are already present in
> that branch.
Thanks, Thiago. These patches are now in next-queued-testing waiting
for some additional reviews/acks.
Mimi
^ permalink raw reply
* Re: [PATCH v12 01/11] MODSIGN: Export module signature definitions
From: Jessica Yu @ 2019-07-01 14:47 UTC (permalink / raw)
To: Thiago Jung Bauermann
Cc: linux-integrity, linux-security-module, keyrings, linux-crypto,
linuxppc-dev, linux-doc, linux-kernel, Mimi Zohar,
Dmitry Kasatkin, James Morris, Serge E. Hallyn, David Howells,
David Woodhouse, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro
In-Reply-To: <20190628021934.4260-2-bauerman@linux.ibm.com>
+++ Thiago Jung Bauermann [27/06/19 23:19 -0300]:
>IMA will use the module_signature format for append signatures, so export
>the relevant definitions and factor out the code which verifies that the
>appended signature trailer is valid.
>
>Also, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it
>and be able to use mod_check_sig() without having to depend on either
>CONFIG_MODULE_SIG or CONFIG_MODULES.
>
>Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>Cc: Jessica Yu <jeyu@kernel.org>
>---
> include/linux/module.h | 3 --
> include/linux/module_signature.h | 44 +++++++++++++++++++++++++
> init/Kconfig | 6 +++-
> kernel/Makefile | 1 +
> kernel/module.c | 1 +
> kernel/module_signature.c | 46 ++++++++++++++++++++++++++
> kernel/module_signing.c | 56 +++++---------------------------
> scripts/Makefile | 2 +-
> 8 files changed, 106 insertions(+), 53 deletions(-)
>
>diff --git a/include/linux/module.h b/include/linux/module.h
>index 188998d3dca9..aa56f531cf1e 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -25,9 +25,6 @@
> #include <linux/percpu.h>
> #include <asm/module.h>
>
>-/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
>-#define MODULE_SIG_STRING "~Module signature appended~\n"
>-
Hi Thiago, apologies for the delay.
It looks like arch/s390/kernel/machine_kexec_file.c also relies on
MODULE_SIG_STRING being defined, so module_signature.h will need to be
included there too, otherwise we'll run into a compilation error.
Other than that, the module-related changes look good to me:
Acked-by: Jessica Yu <jeyu@kernel.org>
Thanks!
Jessica
> /* Not Yet Implemented */
> #define MODULE_SUPPORTED_DEVICE(name)
>
>diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
>new file mode 100644
>index 000000000000..523617fc5b6a
>--- /dev/null
>+++ b/include/linux/module_signature.h
>@@ -0,0 +1,44 @@
>+/* SPDX-License-Identifier: GPL-2.0+ */
>+/*
>+ * Module signature handling.
>+ *
>+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
>+ * Written by David Howells (dhowells@redhat.com)
>+ */
>+
>+#ifndef _LINUX_MODULE_SIGNATURE_H
>+#define _LINUX_MODULE_SIGNATURE_H
>+
>+/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
>+#define MODULE_SIG_STRING "~Module signature appended~\n"
>+
>+enum pkey_id_type {
>+ PKEY_ID_PGP, /* OpenPGP generated key ID */
>+ PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */
>+ PKEY_ID_PKCS7, /* Signature in PKCS#7 message */
>+};
>+
>+/*
>+ * Module signature information block.
>+ *
>+ * The constituents of the signature section are, in order:
>+ *
>+ * - Signer's name
>+ * - Key identifier
>+ * - Signature data
>+ * - Information block
>+ */
>+struct module_signature {
>+ u8 algo; /* Public-key crypto algorithm [0] */
>+ u8 hash; /* Digest algorithm [0] */
>+ u8 id_type; /* Key identifier type [PKEY_ID_PKCS7] */
>+ u8 signer_len; /* Length of signer's name [0] */
>+ u8 key_id_len; /* Length of key identifier [0] */
>+ u8 __pad[3];
>+ __be32 sig_len; /* Length of signature data */
>+};
>+
>+int mod_check_sig(const struct module_signature *ms, size_t file_len,
>+ const char *name);
>+
>+#endif /* _LINUX_MODULE_SIGNATURE_H */
>diff --git a/init/Kconfig b/init/Kconfig
>index 8b9ffe236e4f..c2286a3c74c5 100644
>--- a/init/Kconfig
>+++ b/init/Kconfig
>@@ -1852,6 +1852,10 @@ config BASE_SMALL
> default 0 if BASE_FULL
> default 1 if !BASE_FULL
>
>+config MODULE_SIG_FORMAT
>+ def_bool n
>+ select SYSTEM_DATA_VERIFICATION
>+
> menuconfig MODULES
> bool "Enable loadable module support"
> option modules
>@@ -1929,7 +1933,7 @@ config MODULE_SRCVERSION_ALL
> config MODULE_SIG
> bool "Module signature verification"
> depends on MODULES
>- select SYSTEM_DATA_VERIFICATION
>+ select MODULE_SIG_FORMAT
> help
> Check modules for valid signatures upon load: the signature
> is simply appended to the module. For more information see
>diff --git a/kernel/Makefile b/kernel/Makefile
>index 33824f0385b3..f29ae2997a43 100644
>--- a/kernel/Makefile
>+++ b/kernel/Makefile
>@@ -58,6 +58,7 @@ endif
> obj-$(CONFIG_UID16) += uid16.o
> obj-$(CONFIG_MODULES) += module.o
> obj-$(CONFIG_MODULE_SIG) += module_signing.o
>+obj-$(CONFIG_MODULE_SIG_FORMAT) += module_signature.o
> obj-$(CONFIG_KALLSYMS) += kallsyms.o
> obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
> obj-$(CONFIG_CRASH_CORE) += crash_core.o
>diff --git a/kernel/module.c b/kernel/module.c
>index 6e6712b3aaf5..2712f4d217f5 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -19,6 +19,7 @@
> #include <linux/export.h>
> #include <linux/extable.h>
> #include <linux/moduleloader.h>
>+#include <linux/module_signature.h>
> #include <linux/trace_events.h>
> #include <linux/init.h>
> #include <linux/kallsyms.h>
>diff --git a/kernel/module_signature.c b/kernel/module_signature.c
>new file mode 100644
>index 000000000000..4224a1086b7d
>--- /dev/null
>+++ b/kernel/module_signature.c
>@@ -0,0 +1,46 @@
>+// SPDX-License-Identifier: GPL-2.0+
>+/*
>+ * Module signature checker
>+ *
>+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
>+ * Written by David Howells (dhowells@redhat.com)
>+ */
>+
>+#include <linux/errno.h>
>+#include <linux/printk.h>
>+#include <linux/module_signature.h>
>+#include <asm/byteorder.h>
>+
>+/**
>+ * mod_check_sig - check that the given signature is sane
>+ *
>+ * @ms: Signature to check.
>+ * @file_len: Size of the file to which @ms is appended.
>+ * @name: What is being checked. Used for error messages.
>+ */
>+int mod_check_sig(const struct module_signature *ms, size_t file_len,
>+ const char *name)
>+{
>+ if (be32_to_cpu(ms->sig_len) >= file_len - sizeof(*ms))
>+ return -EBADMSG;
>+
>+ if (ms->id_type != PKEY_ID_PKCS7) {
>+ pr_err("%s: Module is not signed with expected PKCS#7 message\n",
>+ name);
>+ return -ENOPKG;
>+ }
>+
>+ if (ms->algo != 0 ||
>+ ms->hash != 0 ||
>+ ms->signer_len != 0 ||
>+ ms->key_id_len != 0 ||
>+ ms->__pad[0] != 0 ||
>+ ms->__pad[1] != 0 ||
>+ ms->__pad[2] != 0) {
>+ pr_err("%s: PKCS#7 signature info has unexpected non-zero params\n",
>+ name);
>+ return -EBADMSG;
>+ }
>+
>+ return 0;
>+}
>diff --git a/kernel/module_signing.c b/kernel/module_signing.c
>index 6b9a926fd86b..cdd04a6b8074 100644
>--- a/kernel/module_signing.c
>+++ b/kernel/module_signing.c
>@@ -11,37 +11,13 @@
>
> #include <linux/kernel.h>
> #include <linux/errno.h>
>+#include <linux/module.h>
>+#include <linux/module_signature.h>
> #include <linux/string.h>
> #include <linux/verification.h>
> #include <crypto/public_key.h>
> #include "module-internal.h"
>
>-enum pkey_id_type {
>- PKEY_ID_PGP, /* OpenPGP generated key ID */
>- PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */
>- PKEY_ID_PKCS7, /* Signature in PKCS#7 message */
>-};
>-
>-/*
>- * Module signature information block.
>- *
>- * The constituents of the signature section are, in order:
>- *
>- * - Signer's name
>- * - Key identifier
>- * - Signature data
>- * - Information block
>- */
>-struct module_signature {
>- u8 algo; /* Public-key crypto algorithm [0] */
>- u8 hash; /* Digest algorithm [0] */
>- u8 id_type; /* Key identifier type [PKEY_ID_PKCS7] */
>- u8 signer_len; /* Length of signer's name [0] */
>- u8 key_id_len; /* Length of key identifier [0] */
>- u8 __pad[3];
>- __be32 sig_len; /* Length of signature data */
>-};
>-
> /*
> * Verify the signature on a module.
> */
>@@ -49,6 +25,7 @@ int mod_verify_sig(const void *mod, struct load_info *info)
> {
> struct module_signature ms;
> size_t sig_len, modlen = info->len;
>+ int ret;
>
> pr_devel("==>%s(,%zu)\n", __func__, modlen);
>
>@@ -56,32 +33,15 @@ int mod_verify_sig(const void *mod, struct load_info *info)
> return -EBADMSG;
>
> memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
>- modlen -= sizeof(ms);
>+
>+ ret = mod_check_sig(&ms, modlen, info->name);
>+ if (ret)
>+ return ret;
>
> sig_len = be32_to_cpu(ms.sig_len);
>- if (sig_len >= modlen)
>- return -EBADMSG;
>- modlen -= sig_len;
>+ modlen -= sig_len + sizeof(ms);
> info->len = modlen;
>
>- if (ms.id_type != PKEY_ID_PKCS7) {
>- pr_err("%s: Module is not signed with expected PKCS#7 message\n",
>- info->name);
>- return -ENOPKG;
>- }
>-
>- if (ms.algo != 0 ||
>- ms.hash != 0 ||
>- ms.signer_len != 0 ||
>- ms.key_id_len != 0 ||
>- ms.__pad[0] != 0 ||
>- ms.__pad[1] != 0 ||
>- ms.__pad[2] != 0) {
>- pr_err("%s: PKCS#7 signature info has unexpected non-zero params\n",
>- info->name);
>- return -EBADMSG;
>- }
>-
> return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
> VERIFY_USE_SECONDARY_KEYRING,
> VERIFYING_MODULE_SIGNATURE,
>diff --git a/scripts/Makefile b/scripts/Makefile
>index 9d442ee050bd..52098b080ab7 100644
>--- a/scripts/Makefile
>+++ b/scripts/Makefile
>@@ -17,7 +17,7 @@ hostprogs-$(CONFIG_VT) += conmakehash
> hostprogs-$(BUILD_C_RECORDMCOUNT) += recordmcount
> hostprogs-$(CONFIG_BUILDTIME_EXTABLE_SORT) += sortextable
> hostprogs-$(CONFIG_ASN1) += asn1_compiler
>-hostprogs-$(CONFIG_MODULE_SIG) += sign-file
>+hostprogs-$(CONFIG_MODULE_SIG_FORMAT) += sign-file
> hostprogs-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += extract-cert
> hostprogs-$(CONFIG_SYSTEM_EXTRA_CERTIFICATE) += insert-sys-cert
>
^ permalink raw reply
* Re: [PATCH 2/6] Adjust watch_queue documentation to mention mount and superblock watches. [ver #5]
From: Randy Dunlap @ 2019-07-01 14:52 UTC (permalink / raw)
To: David Howells
Cc: viro, Casey Schaufler, Stephen Smalley, Greg Kroah-Hartman,
nicolas.dichtel, raven, Christian Brauner, keyrings, linux-usb,
linux-security-module, linux-fsdevel, linux-api, linux-block,
linux-kernel
In-Reply-To: <8212.1561971170@warthog.procyon.org.uk>
On 7/1/19 1:52 AM, David Howells wrote:
> Randy Dunlap <rdunlap@infradead.org> wrote:
>
>> I'm having a little trouble parsing that sentence.
>> Could you clarify it or maybe rewrite/modify it?
>> Thanks.
>
> How about:
>
> * ``info_filter`` and ``info_mask`` act as a filter on the info field of the
> notification record. The notification is only written into the buffer if::
>
> (watch.info & info_mask) == info_filter
>
> This could be used, for example, to ignore events that are not exactly on
> the watched point in a mount tree by specifying NOTIFY_MOUNT_IN_SUBTREE
> must not be set, e.g.::
>
> {
> .type = WATCH_TYPE_MOUNT_NOTIFY,
> .info_filter = 0,
> .info_mask = NOTIFY_MOUNT_IN_SUBTREE,
> .subtype_filter = ...,
> }
>
> as an event would be only permissible with this filter if::
>
> (watch.info & NOTIFY_MOUNT_IN_SUBTREE) == 0
>
> David
>
Yes, better. Thanks.
--
~Randy
^ permalink raw reply
* RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Xing, Cedric @ 2019-07-01 17:11 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-sgx@vger.kernel.org, LSM List, selinux@vger.kernel.org,
Schaufler, Casey, James Morris, Jethro Beekman,
Dr. Greg Wettstein, Stephen Smalley, Jarkko Sakkinen,
Christopherson, Sean J
In-Reply-To: <CALCETrWQUEEwAAdPrVQMengKDhX3-fVrtJwaP2i=mWD+s+B8vg@mail.gmail.com>
Hi Andy,
> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Saturday, June 29, 2019 4:47 PM
>
> Just on a very cursory review, this seems like it's creating a bunch of
> complexity (a whole new library and data structure), and I'm not
> convinced the result is any better than Sean's version.
The new EMA data structure is to track enclave pages by range. Yes, Sean avoided that by storing similar information in the existing encl_page structure inside SGX subsystem. But as I pointed out, his code has to iterate through *every* page in range so mprotect() will be very slow if the range is large. So he would end up introducing something similar to achieve the same performance.
And that's not the most important point. The major problem in his patch lies in SGX2 support, as #PF driven EAUG cannot be supported (or he'd have to amend his code accordingly, which will add complexity and tip your scale).
Other weird things, such as mmap()'ing page by page vs. mmap()'ing the whole range will impact subsequent mprotect()'s as you have noticed, don't exist in my series.
^ permalink raw reply
* Re: [RFC PATCH v5 0/1] Add dm verity root hash pkcs7 sig validation.
From: Jaskaran Singh Khurana @ 2019-07-01 17:33 UTC (permalink / raw)
To: Milan Broz
Cc: James Morris, Eric Biggers, linux-security-module, linux-kernel,
linux-integrity, linux-fsdevel, agk, snitzer, dm-devel, scottsh,
mpatocka
In-Reply-To: <749ddf56-3cb6-42c8-9ccc-71e09558400f@gmail.com>
Hello Milan,
On Mon, 1 Jul 2019, Milan Broz wrote:
> On 29/06/2019 06:01, James Morris wrote:
>> On Thu, 27 Jun 2019, Eric Biggers wrote:
>>
>>> I don't understand your justification for this feature.
>>>
>>> If userspace has already been pwned severely enough for the attacker to be
>>> executing arbitrary code with CAP_SYS_ADMIN (which is what the device mapper
>>> ioctls need), what good are restrictions on loading more binaries from disk?
>>>
>>> Please explain your security model.
>>
>> Let's say the system has a policy where all code must be signed with a
>> valid key, and that one mechanism for enforcing this is via signed
>> dm-verity volumes. Validating the signature within the kernel provides
>> stronger assurance than userspace validation. The kernel validates and
>> executes the code, using kernel-resident keys, and does not need to rely
>> on validation which has occurred across a trust boundary.
>
> Yes, but as it is implemented in this patch, a certificate is provided as
> a binary blob by the (super)user that activates the dm-verity device.
>
> Actually, I can put there anything that looks like a correct signature (self-signed
> or so), and dm-verity code is happy because the root hash is now signed.
>
> Maybe could this concept be extended to support in-kernel compiled certificates?
>
> I like the idea of signed root hash, but the truth is that if you have access
> to device activation, it brings nothing, you can just put any cert in the keyring
> and use it.
>
> Milan
>
The signature needs to be trusted by the .builtin_trusted_keys which is
a read-only list of keys that were compiled into the kernel. The
verify_pkcs7_signature verifies trust against the builtin keyring so I
think what you are suggesting is covered here.
Regards,
Jaskaran.
^ permalink raw reply
* RE: [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation
From: Xing, Cedric @ 2019-07-01 17:46 UTC (permalink / raw)
To: Andy Lutomirski, Stephen Smalley
Cc: Christopherson, Sean J, Jarkko Sakkinen,
linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, Roberts, William C, Schaufler, Casey,
James Morris, Hansen, Dave, Jethro Beekman, Dr . Greg Wettstein
In-Reply-To: <CALCETrWPzSaFUWi4q4Vq_0RrtNMFZAKkwKkya=p6cfB50x2tMQ@mail.gmail.com>
> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Saturday, June 29, 2019 4:42 PM
>
> On Tue, Jun 25, 2019 at 2:09 PM Stephen Smalley <sds@tycho.nsa.gov>
> wrote:
> >
> > On 6/21/19 5:22 PM, Xing, Cedric wrote:
> > >> From: Christopherson, Sean J
> > >> Sent: Wednesday, June 19, 2019 3:24 PM
> > >>
> > >> Intended use of each permission:
> > >>
> > >> - SGX_EXECDIRTY: dynamically load code within the enclave itself
> > >> - SGX_EXECUNMR: load unmeasured code into the enclave, e.g.
> > >> Graphene
> > >
> > > Why does it matter whether a code page is measured or not?
> >
> > It won't be incorporated into an attestation?
> >
>
> Also, if there is, in parallel, a policy that limits the set of enclave
> SIGSTRUCTs that are accepted, requiring all code be measured makes it
> harder to subvert by writing incompetent or maliciously incompetent
> enclaves.
As analyzed in my reply to one of Stephen's comments, no executable page shall be "enclave only" as enclaves have access to host's memory, so if an executable page in regular memory is considered posting a threat to the process, it should be considered posting the same threat inside an enclave as well.
That said, every executable enclave page should have an executable source page (doesn’t have to executable, as long as mprotect(X) would succeed on it, as shown in my patch), hence any exploits mountable on the enclave page shall also be mountable using the source page. Given only the weakest link matters in security, I argue that SGX_EXECUNMR is unnecessary from the process's perspective.
SGX_EXECUNMR does impact security from the enclave's perspective, thus it is reflected in enclave's measurement, which is part of SGX ISA. It's the enclave vendor's responsibility to ensure code pages are properly measured and that's largely automated by tools. It's highly unlikely an ISV would "forget" to measure a page so I don't think SGX_EXECUNMR has much value for ISVs.
So the only case left is the enclave author left a page unmeasured with a malicious intent. As that's part of the enclave measurement, it would get caught at EINIT because of an untrusted/blacklisted signing key, or it doesn't because of the lack of whitelisting/blacklisting mechanism. But in the latter case, the adversary could just measure the malicious page as the final measurement or signing key doesn't matter anyway. Sean's series doesn't have an enclave_init() hook so it will always be the latter case, where the final measurement doesn't matter. Therefore, SGX_EXECUNMR doesn't have any value as adversaries could always measure all code pages to satisfy the policy without worrying about final measurements.
^ permalink raw reply
* Re: [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation
From: Andy Lutomirski @ 2019-07-01 17:53 UTC (permalink / raw)
To: Xing, Cedric
Cc: Andy Lutomirski, Stephen Smalley, Christopherson, Sean J,
Jarkko Sakkinen, linux-sgx@vger.kernel.org,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
Roberts, William C, Schaufler, Casey, James Morris, Hansen, Dave,
Jethro Beekman, Dr . Greg Wettstein
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F6551D558@ORSMSX116.amr.corp.intel.com>
On Mon, Jul 1, 2019 at 10:46 AM Xing, Cedric <cedric.xing@intel.com> wrote:
>
> > From: Andy Lutomirski [mailto:luto@kernel.org]
> > Sent: Saturday, June 29, 2019 4:42 PM
> >
> > On Tue, Jun 25, 2019 at 2:09 PM Stephen Smalley <sds@tycho.nsa.gov>
> > wrote:
> > >
> > > On 6/21/19 5:22 PM, Xing, Cedric wrote:
> > > >> From: Christopherson, Sean J
> > > >> Sent: Wednesday, June 19, 2019 3:24 PM
> > > >>
> > > >> Intended use of each permission:
> > > >>
> > > >> - SGX_EXECDIRTY: dynamically load code within the enclave itself
> > > >> - SGX_EXECUNMR: load unmeasured code into the enclave, e.g.
> > > >> Graphene
> > > >
> > > > Why does it matter whether a code page is measured or not?
> > >
> > > It won't be incorporated into an attestation?
> > >
> >
> > Also, if there is, in parallel, a policy that limits the set of enclave
> > SIGSTRUCTs that are accepted, requiring all code be measured makes it
> > harder to subvert by writing incompetent or maliciously incompetent
> > enclaves.
>
> As analyzed in my reply to one of Stephen's comments, no executable page shall be "enclave only" as enclaves have access to host's memory, so if an executable page in regular memory is considered posting a threat to the process, it should be considered posting the same threat inside an enclave as well.
Huh? The SDM (37.3 in whateve version I'm reading) says "Code fetches
from inside an enclave to a linear address outside that enclave result
in a #GP(0) exception." Enclaves execute enclave code only.
In any event, I believe we're discussing taking readable memory from
outside the enclave and copying it to an executable code inside the
enclave.
>
> That said, every executable enclave page should have an executable source page (doesn’t have to executable, as long as mprotect(X) would succeed on it, as shown in my patch)
Does Sean's series require this? I think that, if we can get away
with it, it's a lot nicer to *not* require user code to map the source
pages PROT_EXEC. Some policy may check that it's VM_MAYEXEC or check
some other attribute of the VMA, but actually requiring PROT_EXEC
seems like we're weakening existing hardening measures to enforce a
policy, which is a mistake.
^ permalink raw reply
* RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Xing, Cedric @ 2019-07-01 17:57 UTC (permalink / raw)
To: Casey Schaufler, Stephen Smalley
Cc: linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, Schaufler, Casey, jmorris@namei.org,
luto@kernel.org, jethro@fortanix.com, greg@enjellic.com,
sds@tycho.nsa.gov, jarkko.sakkinen@linux.intel.com,
Christopherson, Sean J
In-Reply-To: <18833f2e-9d18-1f39-6bc5-9242910ab25c@schaufler-ca.com>
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Casey Schaufler
>
> On 6/28/2019 6:37 PM, Stephen Smalley wrote:
> > On Fri, Jun 28, 2019 at 1:22 PM Casey Schaufler <casey@schaufler-
> ca.com> wrote:
> >> On 6/27/2019 5:47 PM, Xing, Cedric wrote:
> >>>> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
> >>>> Sent: Thursday, June 27, 2019 4:37 PM
> >>>>>> This code should not be mixed in with the LSM infrastructure.
> >>>>>> It should all be contained in its own module, under
> security/enclave.
> >>>>> lsm_ema is *intended* to be part of the LSM infrastructure.
> >>>> That's not going to fly, not for a minute.
> >>> Why not, if there's a need for it?
> >>>
> >>> And what's the concern here if it becomes part of the LSM
> infrastructure.
> >> The LSM infrastructure provides a framework for hooks and allocation
> >> of blobs. That's it. It's a layer for connecting system features like
> >> VFS, IPC and the IP stack to the security modules. It does not
> >> implement any policy of it's own. We are not going to implement SGX
> >> or any other mechanism within the LSM infrastructure.
> > I don't think you understand the purpose of this code. It isn't
> > implementing SGX, nor is it needed by SGX.
> > It is providing shared infrastructure for security modules, similar to
> > lsm_audit.c, so that security modules can enforce W^X or similar
> > memory protection guarantees for SGX enclave memory, which has unique
> > properties that render the existing mmap and mprotect hooks
> > insufficient. They can certainly implement it only for SELinux, but
> > then any other security module that wants to provide such guarantees
> > will have to replicate that code.
>
> I am not objecting to the purpose of the code.
> I *am* objecting to calling it part of the LSM infrastructure.
> It needs to be it's own thing, off somewhere else.
> It must not use the lsm_ prefix. That's namespace pollution.
> The code must not be embedded in the LSM infrastructure code, that
> breaks with how everything else works.
If you understand the purpose, then why are you objecting the lsm_ prefix as they are APIs to be used by all LSM modules? Or what should be the prefix in your mind? Or what kind of APIs do you think can qualify the lsm_ prefix?
And I'd like to clarify that it doesn't break anything, but is just a bit different, in that security_enclave_load() and security_file_free() call into those APIs. But there's a need for them because otherwise code/data would have to be duplicated among LSMs and the logic would be harder to comprehend. So that's a trade-off. Then what's the practical drawback of doing that? If no, why would we want to pay for the cost for not doing that?
>
> ... and the notion that you allocate data for one blob that gets freed
> relative to another breaks the data management model.
What do you mean here?
EMA blobs are allocated/freed *not* relative to any other blobs.
^ permalink raw reply
* Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Andy Lutomirski @ 2019-07-01 17:58 UTC (permalink / raw)
To: Xing, Cedric
Cc: Andy Lutomirski, linux-sgx@vger.kernel.org, LSM List,
selinux@vger.kernel.org, Schaufler, Casey, James Morris,
Jethro Beekman, Dr. Greg Wettstein, Stephen Smalley,
Jarkko Sakkinen, Christopherson, Sean J
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F6551D4B3@ORSMSX116.amr.corp.intel.com>
On Mon, Jul 1, 2019 at 10:11 AM Xing, Cedric <cedric.xing@intel.com> wrote:
>
> Hi Andy,
>
> > From: Andy Lutomirski [mailto:luto@kernel.org]
> > Sent: Saturday, June 29, 2019 4:47 PM
> >
> > Just on a very cursory review, this seems like it's creating a bunch of
> > complexity (a whole new library and data structure), and I'm not
> > convinced the result is any better than Sean's version.
>
> The new EMA data structure is to track enclave pages by range. Yes, Sean avoided that by storing similar information in the existing encl_page structure inside SGX subsystem. But as I pointed out, his code has to iterate through *every* page in range so mprotect() will be very slow if the range is large. So he would end up introducing something similar to achieve the same performance.
It seems odd to stick it in security/ if it only has one user, though.
Also, if it wasn't in security/, then the security folks would stop
complaining :)
>
> And that's not the most important point. The major problem in his patch lies in SGX2 support, as #PF driven EAUG cannot be supported (or he'd have to amend his code accordingly, which will add complexity and tip your scale).
>
Why can't it be?
^ permalink raw reply
* Re: [RFC PATCH v4 04/12] x86/sgx: Require userspace to define enclave pages' protection bits
From: Andy Lutomirski @ 2019-07-01 18:00 UTC (permalink / raw)
To: Sean Christopherson
Cc: Jarkko Sakkinen, linux-sgx, LSM List, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <20190619222401.14942-5-sean.j.christopherson@intel.com>
On Wed, Jun 19, 2019 at 3:24 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
> static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
> {
> struct sgx_encl *encl = file->private_data;
> + unsigned long allowed_rwx;
> int ret;
>
> + allowed_rwx = sgx_allowed_rwx(encl, vma);
> + if (vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC) & ~allowed_rwx)
> + return -EACCES;
> +
> ret = sgx_encl_mm_add(encl, vma->vm_mm);
> if (ret)
> return ret;
>
> + if (!(allowed_rwx & VM_READ))
> + vma->vm_flags &= ~VM_MAYREAD;
> + if (!(allowed_rwx & VM_WRITE))
> + vma->vm_flags &= ~VM_MAYWRITE;
> + if (!(allowed_rwx & VM_EXEC))
> + vma->vm_flags &= ~VM_MAYEXEC;
> +
I'm with Cedric here -- this is no good. The reason I think we need
.may_mprotect or similar is exactly to avoid doing this.
mmap() just needs to make the same type of VMA regardless of the pages
in the range.
^ permalink raw reply
* RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Xing, Cedric @ 2019-07-01 18:02 UTC (permalink / raw)
To: Stephen Smalley
Cc: Stephen Smalley, linux-sgx@vger.kernel.org,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
Schaufler, Casey, jmorris@namei.org, luto@kernel.org,
jethro@fortanix.com, greg@enjellic.com,
jarkko.sakkinen@linux.intel.com, Christopherson, Sean J
In-Reply-To: <CAB9W1A3Xusf77LZ44HjNYuuFPHv=vditqJZLu6yrStFG3OwHXQ@mail.gmail.com>
> From: Stephen Smalley [mailto:stephen.smalley@gmail.com]
> Sent: Friday, June 28, 2019 6:22 PM
>
> On Fri, Jun 28, 2019 at 5:54 PM Xing, Cedric <cedric.xing@intel.com>
> wrote:
> >
> > > From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> > > owner@vger.kernel.org] On Behalf Of Stephen Smalley
> > > Sent: Friday, June 28, 2019 9:37 AM
> > >
> > > > lsm.ema.cache_decisions is on by default and could be turned off
> > > > by appending “lsm.ema.cache_decisions=0” or
> > > > “lsm.ema.cache_decisions=off” to the kernel command line.
> > >
> > > This seems problematic on a few fronts:
> > >
> > > - Specifying it as a boot parameter requires teaching admins /
> > > policy developers to do something in addition to what they are
> > > already doing when they want to create policy,
> > >
> > > - Limiting it to a boot parameter requires a reboot to change the
> > > mode of operation, whereas SELinux offers runtime toggling of
> > > permissive mode and even per-process (domain) permissive mode (and
> > > so does AppArmor),
> >
> > How about making a variable tunable via sysctl?
>
> Better than a boot parameter but still not amenable to per-domain
> permissive and still requires admins to remember and perform an extra
> step before collecting denials.
>
> >
> > >
> > > - In the cache_decisions=1 case, do we get any auditing at all? If
> > > not, that's a problem. We want auditing not only when we are
> > > generating/learning policy but also in operation.
> >
> > Currently it doesn't generate audit log, but I could add it, except it
> couldn't point to the exact file. But I can use the sigstruct file
> instead so administrators can at least tell which enclave violates the
> policy. Do you think it acceptable?
>
> Seems prone to user confusion and lacks precision in why the denial
> occurred.
>
> >
> > >
> > > - There is the potential for inconsistencies to arise between the
> > > enforcement applied with different cache_decisions values.
> >
> > The enforcement will be consistent. The difference only lies in the
> logs.
> >
> > >
> > > I would suggest that we just never cache the decision and accept the
> > > cost if we are going to take this approach.
> >
> > This will also be a viable option. I don't think any enclaves would be
> comprised of a large number of files anyway. When SGX2 comes up, I think
> most enclaves will be instantiated from only one file and defer loading
> libraries at runtime. So in practice we are looking to keeping only one
> file open per enclave, which seems totally acceptable.
> >
> > Stephen (and everyone having an opinion on this), which way do you
> prefer? sysctl variable? Or never cache decisions?
>
> I'd favor never caching decisions.
Alright, I'll remove the boot parameter and never cache decisions.
^ permalink raw reply
* RE: [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation
From: Xing, Cedric @ 2019-07-01 18:14 UTC (permalink / raw)
To: Stephen Smalley
Cc: Stephen Smalley, Christopherson, Sean J, Jarkko Sakkinen,
linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, Roberts, William C, Schaufler, Casey,
James Morris, Hansen, Dave, Andy Lutomirski, Jethro Beekman,
Dr . Greg Wettstein
In-Reply-To: <CAB9W1A1D=uDEdfSjS9DDNbThA1_HwATJN3=BcxZdXX5qMGHFrA@mail.gmail.com>
> From: Stephen Smalley [mailto:stephen.smalley@gmail.com]
> Sent: Friday, June 28, 2019 6:16 PM
>
> On Fri, Jun 28, 2019 at 5:20 PM Xing, Cedric <cedric.xing@intel.com>
> wrote:
> >
> > > From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> > > owner@vger.kernel.org] On Behalf Of Stephen Smalley
> > > Sent: Friday, June 28, 2019 9:17 AM
> > >
> > > FWIW, adding new permissions only requires updating policy
> > > configuration, not userspace code/tools. But in any event, we can
> > > reuse the execute- related permissions if it makes sense but still
> > > consider introducing additional, new permissions, possibly in a
> > > separate "enclave" security class, if we want explicit control over
> enclave loading, e.g.
> > > ENCLAVE__LOAD, ENCLAVE__INIT, etc.
> >
> > I'm not so familiar with SELinux tools so my apology in advance if I
> end up mixing up things.
> >
> > I'm not only talking about the new permissions, but also how to apply
> them to enclave files. Intel SGX SDK packages enclaves as .so files, and
> I guess that's the most straight forward way that most others would do.
> So if different permissions are defined, then user mode tools would have
> to distinguish enclaves from regular .so files in order to grant them
> different permissions. Would that be something extra to existing tools?
>
> It doesn't require any userspace code changes. It is just a matter of
> defining some configuration data in the policy for the new permissions,
> one or more security labels (tags) for the SGX .so files, and rules
> allowing access where desired, and then setting those security labels on
> the SGX .so files (via the security.selinux extended attribute on the
> files). Even the last part is generally handled by updating a
> configuration specifying how files should be labeled and then rpm
> automatically labels the files when created, or you can manually
> restorecon them. If the new permissions are defined in their own
> security class rather than reusing existing ones, then they can even be
> defined entirely via a local or third party policy module separate from
> the distro policy if desired/needed.
I'm not objecting to what you proposed but just trying to understand more.
SGX enclaves don't look any different than regular shared objects except the meta data section, which is implementation dependent (all enclaves built by Intel's SDK have .note.sgxmeta sections but others could do something completely different and may not even use ELF sections). Then how does rpm tell whether a .so file is a regular shared object or an SGX enclave? My understanding is, rpm has to be able to distinguish those two in order to label them correctly (differently). Am I correct?
>
> >
> > >
> > > One residual concern I have with the reuse of FILE__EXECUTE is using
> > > it for the sigstruct file as the fallback case. If the sigstruct is
> > > always part of the same file as the code, then it probably doesn't
> > > matter. But otherwise, it is somewhat odd to have to allow the host
> > > process to execute from the sigstruct file if it is only data (the
> signature).
> >
> > I agree with you. But do you think it a practical problem today? As
> far as I know, no one is deploying sigstructs in dedicated files. I'm
> just trying to touch as few things as possible until there's definitely
> a need to do so.
>
> I don't know, and it wasn't clear to me from the earlier discussions.
> If not and if it is acceptable to require them to be in files in the
> first place, then perhaps it isn't necessary.
^ permalink raw reply
* [RFC PATCH v6 1/1] Add dm verity root hash pkcs7 sig validation.
From: Jaskaran Khurana @ 2019-07-01 18:19 UTC (permalink / raw)
To: gmazyland, ebiggers
Cc: linux-security-module, linux-kernel, linux-integrity,
linux-fsdevel, agk, snitzer, dm-devel, jmorris, scottsh, mdsakib,
mpatocka
In-Reply-To: <20190701181958.6493-1-jaskarankhurana@linux.microsoft.com>
The verification is to support cases where the roothash is not secured by
Trusted Boot, UEFI Secureboot or similar technologies.
One of the use cases for this is for dm-verity volumes mounted after boot,
the root hash provided during the creation of the dm-verity volume has to
be secure and thus in-kernel validation implemented here will be used
before we trust the root hash and allow the block device to be created.
The signature being provided for verification must verify the root hash and
must be trusted by the builtin keyring for verification to succeed.
The hash is added as a key of type "user" and the description is passed to the
kernel so it can look it up and use it for verification.
Adds CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG which can be turned on if root hash
verification is needed.
Kernel commandline parameter require_signatures will indicate whether to force
(for all dm verity volumes) roothash signature verification.
Signed-off-by: Jaskaran Khurana <jaskarankhurana@linux.microsoft.com>
---
Documentation/device-mapper/verity.txt | 7 ++
drivers/md/Kconfig | 12 +++
drivers/md/Makefile | 5 +
drivers/md/dm-verity-target.c | 43 +++++++-
drivers/md/dm-verity-verify-sig.c | 133 +++++++++++++++++++++++++
drivers/md/dm-verity-verify-sig.h | 60 +++++++++++
drivers/md/dm-verity.h | 2 +
7 files changed, 257 insertions(+), 5 deletions(-)
create mode 100644 drivers/md/dm-verity-verify-sig.c
create mode 100644 drivers/md/dm-verity-verify-sig.h
diff --git a/Documentation/device-mapper/verity.txt b/Documentation/device-mapper/verity.txt
index b3d2e4a42255..df7ef1d553cc 100644
--- a/Documentation/device-mapper/verity.txt
+++ b/Documentation/device-mapper/verity.txt
@@ -121,6 +121,13 @@ check_at_most_once
blocks, and a hash block will not be verified any more after all the data
blocks it covers have been verified anyway.
+root_hash_sig_key_desc <key_description>
+ This is the description of the USER_KEY that the kernel will lookup to get
+ the pkcs7 signature of the roothash. The pkcs7 signature is used to validate
+ the root hash during the creation of the device mapper block device.
+ Verification of roothash depends on the config DM_VERITY_VERIFY_ROOTHASH_SIG
+ being set in the kernel.
+
Theory of operation
===================
diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index db269a348b20..9d0bc609582c 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -489,6 +489,18 @@ config DM_VERITY
If unsure, say N.
+config DM_VERITY_VERIFY_ROOTHASH_SIG
+ def_bool n
+ bool "Verity data device root hash signature verification support"
+ depends on DM_VERITY
+ select SYSTEM_DATA_VERIFICATION
+ help
+ The device mapper target created by DM-VERITY can be validated if the
+ pre-generated tree of cryptographic checksums passed has a pkcs#7
+ signature file that can validate the roothash of the tree.
+
+ If unsure, say N.
+
config DM_VERITY_FEC
bool "Verity forward error correction support"
depends on DM_VERITY
diff --git a/drivers/md/Makefile b/drivers/md/Makefile
index be7a6eb92abc..fe756c0dca14 100644
--- a/drivers/md/Makefile
+++ b/drivers/md/Makefile
@@ -81,3 +81,8 @@ endif
ifeq ($(CONFIG_DM_VERITY_FEC),y)
dm-verity-objs += dm-verity-fec.o
endif
+
+ifeq ($(CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG),y)
+dm-verity-objs += dm-verity-verify-sig.o
+endif
+
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index f4c31ffaa88e..3dc7f6dcc337 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -16,7 +16,7 @@
#include "dm-verity.h"
#include "dm-verity-fec.h"
-
+#include "dm-verity-verify-sig.h"
#include <linux/module.h>
#include <linux/reboot.h>
@@ -34,7 +34,8 @@
#define DM_VERITY_OPT_IGN_ZEROES "ignore_zero_blocks"
#define DM_VERITY_OPT_AT_MOST_ONCE "check_at_most_once"
-#define DM_VERITY_OPTS_MAX (2 + DM_VERITY_OPTS_FEC)
+#define DM_VERITY_OPTS_MAX (2 + DM_VERITY_OPTS_FEC + \
+ DM_VERITY_ROOT_HASH_VERIFICATION_OPTS)
static unsigned dm_verity_prefetch_cluster = DM_VERITY_DEFAULT_PREFETCH_SIZE;
@@ -714,6 +715,8 @@ static void verity_status(struct dm_target *ti, status_type_t type,
args++;
if (v->validated_blocks)
args++;
+ if (v->signature_key_desc)
+ args += DM_VERITY_ROOT_HASH_VERIFICATION_OPTS;
if (!args)
return;
DMEMIT(" %u", args);
@@ -735,6 +738,9 @@ static void verity_status(struct dm_target *ti, status_type_t type,
if (v->validated_blocks)
DMEMIT(" " DM_VERITY_OPT_AT_MOST_ONCE);
sz = verity_fec_status_table(v, sz, result, maxlen);
+ if (v->signature_key_desc)
+ DMEMIT(" " DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY
+ " %s", v->signature_key_desc);
break;
}
}
@@ -800,6 +806,8 @@ static void verity_dtr(struct dm_target *ti)
verity_fec_dtr(v);
+ kfree(v->signature_key_desc);
+
kfree(v);
}
@@ -855,7 +863,8 @@ static int verity_alloc_zero_digest(struct dm_verity *v)
return r;
}
-static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
+static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
+ struct dm_verity_sig_opts *verify_args)
{
int r;
unsigned argc;
@@ -904,6 +913,14 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
if (r)
return r;
continue;
+ } else if (verity_verify_is_sig_opt_arg(arg_name)) {
+ r = verity_verify_sig_parse_opt_args(as, v,
+ verify_args,
+ &argc, arg_name);
+ if (r)
+ return r;
+ continue;
+
}
ti->error = "Unrecognized verity feature request";
@@ -930,6 +947,7 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
{
struct dm_verity *v;
+ struct dm_verity_sig_opts verify_args = {0};
struct dm_arg_set as;
unsigned int num;
unsigned long long num_ll;
@@ -937,6 +955,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
int i;
sector_t hash_position;
char dummy;
+ char *root_hash_digest_to_validate;
v = kzalloc(sizeof(struct dm_verity), GFP_KERNEL);
if (!v) {
@@ -1070,6 +1089,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
r = -EINVAL;
goto bad;
}
+ root_hash_digest_to_validate = argv[8];
if (strcmp(argv[9], "-")) {
v->salt_size = strlen(argv[9]) / 2;
@@ -1095,11 +1115,20 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
as.argc = argc;
as.argv = argv;
- r = verity_parse_opt_args(&as, v);
+ r = verity_parse_opt_args(&as, v, &verify_args);
if (r < 0)
goto bad;
}
+ /* Root hash signature is a optional parameter*/
+ r = verity_verify_root_hash(root_hash_digest_to_validate,
+ strlen(root_hash_digest_to_validate),
+ verify_args.sig,
+ verify_args.sig_size);
+ if (r < 0) {
+ ti->error = "Root hash verification failed";
+ goto bad;
+ }
v->hash_per_block_bits =
__fls((1 << v->hash_dev_block_bits) / v->digest_size);
@@ -1165,9 +1194,13 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
ti->per_io_data_size = roundup(ti->per_io_data_size,
__alignof__(struct dm_verity_io));
+ verity_verify_sig_opts_cleanup(&verify_args);
+
return 0;
bad:
+
+ verity_verify_sig_opts_cleanup(&verify_args);
verity_dtr(ti);
return r;
@@ -1175,7 +1208,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
static struct target_type verity_target = {
.name = "verity",
- .version = {1, 4, 0},
+ .version = {1, 5, 0},
.module = THIS_MODULE,
.ctr = verity_ctr,
.dtr = verity_dtr,
diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
new file mode 100644
index 000000000000..614e43db93aa
--- /dev/null
+++ b/drivers/md/dm-verity-verify-sig.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Microsoft Corporation.
+ *
+ * Author: Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
+ *
+ */
+#include <linux/device-mapper.h>
+#include <linux/verification.h>
+#include <keys/user-type.h>
+#include <linux/module.h>
+#include "dm-verity.h"
+#include "dm-verity-verify-sig.h"
+
+#define DM_VERITY_VERIFY_ERR(s) DM_VERITY_ROOT_HASH_VERIFICATION " " s
+
+static bool require_signatures;
+module_param(require_signatures, bool, false);
+MODULE_PARM_DESC(require_signatures,
+ "Verify the roothash of dm-verity hash tree");
+
+#define DM_VERITY_IS_SIG_FORCE_ENABLED() \
+ (require_signatures != false)
+
+bool verity_verify_is_sig_opt_arg(const char *arg_name)
+{
+ return (!strcasecmp(arg_name,
+ DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY));
+}
+
+static int verity_verify_get_sig_from_key(const char *key_desc,
+ struct dm_verity_sig_opts *sig_opts)
+{
+ struct key *key;
+ const struct user_key_payload *ukp;
+ int ret = 0;
+
+ key = request_key(&key_type_user,
+ key_desc, NULL);
+ if (IS_ERR(key))
+ return PTR_ERR(key);
+
+ down_read(&key->sem);
+
+ ukp = user_key_payload_locked(key);
+ if (!ukp) {
+ ret = -EKEYREVOKED;
+ goto end;
+ }
+
+ sig_opts->sig = kmalloc(ukp->datalen, GFP_KERNEL);
+ if (!sig_opts->sig) {
+ ret = -ENOMEM;
+ goto end;
+ }
+ sig_opts->sig_size = ukp->datalen;
+
+ memcpy(sig_opts->sig, ukp->data, sig_opts->sig_size);
+
+end:
+ up_read(&key->sem);
+ key_put(key);
+
+ return ret;
+}
+
+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as,
+ struct dm_verity *v,
+ struct dm_verity_sig_opts *sig_opts,
+ unsigned int *argc,
+ const char *arg_name)
+{
+ struct dm_target *ti = v->ti;
+ int ret = 0;
+ const char *sig_key = NULL;
+
+ if (!*argc) {
+ ti->error = DM_VERITY_VERIFY_ERR("Signature key not specified");
+ return -EINVAL;
+ }
+
+ sig_key = dm_shift_arg(as);
+ (*argc)--;
+
+ ret = verity_verify_get_sig_from_key(sig_key, sig_opts);
+ if (ret < 0)
+ ti->error = DM_VERITY_VERIFY_ERR("Invalid key specified");
+
+ v->signature_key_desc = kstrdup(sig_key, GFP_KERNEL);
+ if (!v->signature_key_desc)
+ return -ENOMEM;
+
+ return ret;
+}
+
+/*
+ * verify_verify_roothash - Verify the root hash of the verity hash device
+ * using builtin trusted keys.
+ *
+ * @root_hash: For verity, the roothash/data to be verified.
+ * @root_hash_len: Size of the roothash/data to be verified.
+ * @sig_data: The trusted signature that verifies the roothash/data.
+ * @sig_len: Size of the signature.
+ *
+ */
+int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
+ const void *sig_data, size_t sig_len)
+{
+ int ret;
+
+ if (!root_hash || root_hash_len == 0)
+ return -EINVAL;
+
+ if (!sig_data || sig_len == 0) {
+ if (DM_VERITY_IS_SIG_FORCE_ENABLED())
+ return -ENOKEY;
+ else
+ return 0;
+ }
+
+ ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
+ sig_len, NULL, VERIFYING_UNSPECIFIED_SIGNATURE,
+ NULL, NULL);
+
+ return ret;
+}
+
+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts)
+{
+ kfree(sig_opts->sig);
+ sig_opts->sig = NULL;
+ sig_opts->sig_size = 0;
+}
diff --git a/drivers/md/dm-verity-verify-sig.h b/drivers/md/dm-verity-verify-sig.h
new file mode 100644
index 000000000000..19b1547aa741
--- /dev/null
+++ b/drivers/md/dm-verity-verify-sig.h
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Microsoft Corporation.
+ *
+ * Author: Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
+ *
+ */
+#ifndef DM_VERITY_SIG_VERIFICATION_H
+#define DM_VERITY_SIG_VERIFICATION_H
+
+#define DM_VERITY_ROOT_HASH_VERIFICATION "DM Verity Sig Verification"
+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY "root_hash_sig_key_desc"
+
+struct dm_verity_sig_opts {
+ unsigned int sig_size;
+ u8 *sig;
+};
+
+#ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG
+
+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPTS 2
+
+int verity_verify_root_hash(const void *data, size_t data_len,
+ const void *sig_data, size_t sig_len);
+bool verity_verify_is_sig_opt_arg(const char *arg_name);
+
+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
+ struct dm_verity_sig_opts *sig_opts,
+ unsigned int *argc, const char *arg_name);
+
+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts);
+
+#else
+
+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPTS 0
+
+int verity_verify_root_hash(const void *data, size_t data_len,
+ const void *sig_data, size_t sig_len)
+{
+ return 0;
+}
+
+bool verity_verify_is_sig_opt_arg(const char *arg_name)
+{
+ return false;
+}
+
+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
+ struct dm_verity_sig_opts *sig_opts,
+ unsigned int *argc, const char *arg_name)
+{
+ return -EINVAL;
+}
+
+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts)
+{
+}
+
+#endif /* CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG */
+#endif /* DM_VERITY_SIG_VERIFICATION_H */
diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
index 3441c10b840c..42183a3903ae 100644
--- a/drivers/md/dm-verity.h
+++ b/drivers/md/dm-verity.h
@@ -64,6 +64,8 @@ struct dm_verity {
struct dm_verity_fec *fec; /* forward error correction */
unsigned long *validated_blocks; /* bitset blocks validated */
+
+ char *signature_key_desc; /* signature keyring reference */
};
struct dm_verity_io {
--
2.17.1
^ permalink raw reply related
* [RFC PATCH v6 0/1] Add dm verity root hash pkcs7 sig validation.
From: Jaskaran Khurana @ 2019-07-01 18:19 UTC (permalink / raw)
To: gmazyland, ebiggers
Cc: linux-security-module, linux-kernel, linux-integrity,
linux-fsdevel, agk, snitzer, dm-devel, jmorris, scottsh, mdsakib,
mpatocka
Changes in v6:
Address comments from Milan Broz and Eric Biggers on v5.
-Keep the verification code under config DM_VERITY_VERIFY_ROOTHASH_SIG.
-Change the command line parameter to requires_signatures(bool) which will
force root hash to be signed and trusted if specified.
-Fix the signature not being present in verity_status. Merged the
https://git.kernel.org/pub/scm/linux/kernel/git/mbroz/linux.git/commit/?h=dm-cryptsetup&id=a26c10806f5257e255b6a436713127e762935ad3
made by Milan Broz and tested it.
Jaskaran Khurana (1):
Add dm verity root hash pkcs7 sig validation.
Documentation/device-mapper/verity.txt | 7 ++
drivers/md/Kconfig | 12 +++
drivers/md/Makefile | 5 +
drivers/md/dm-verity-target.c | 43 +++++++-
drivers/md/dm-verity-verify-sig.c | 133 +++++++++++++++++++++++++
drivers/md/dm-verity-verify-sig.h | 60 +++++++++++
drivers/md/dm-verity.h | 2 +
7 files changed, 257 insertions(+), 5 deletions(-)
create mode 100644 drivers/md/dm-verity-verify-sig.c
create mode 100644 drivers/md/dm-verity-verify-sig.h
--
2.17.1
^ permalink raw reply
* RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Xing, Cedric @ 2019-07-01 18:31 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-sgx@vger.kernel.org, LSM List, selinux@vger.kernel.org,
Schaufler, Casey, James Morris, Jethro Beekman,
Dr. Greg Wettstein, Stephen Smalley, Jarkko Sakkinen,
Christopherson, Sean J
In-Reply-To: <CALCETrUR=W3rfmG+qHydm0FQvASSYRt_V72v5WeQ4KWT7tjEdA@mail.gmail.com>
> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Monday, July 01, 2019 10:58 AM
>
> On Mon, Jul 1, 2019 at 10:11 AM Xing, Cedric <cedric.xing@intel.com>
> wrote:
> >
> > Hi Andy,
> >
> > > From: Andy Lutomirski [mailto:luto@kernel.org]
> > > Sent: Saturday, June 29, 2019 4:47 PM
> > >
> > > Just on a very cursory review, this seems like it's creating a bunch
> > > of complexity (a whole new library and data structure), and I'm not
> > > convinced the result is any better than Sean's version.
> >
> > The new EMA data structure is to track enclave pages by range. Yes,
> Sean avoided that by storing similar information in the existing
> encl_page structure inside SGX subsystem. But as I pointed out, his code
> has to iterate through *every* page in range so mprotect() will be very
> slow if the range is large. So he would end up introducing something
> similar to achieve the same performance.
>
> It seems odd to stick it in security/ if it only has one user, though.
> Also, if it wasn't in security/, then the security folks would stop
> complaining :)
That's where I started. EMA (though named differently in my v1) was buried inside and used only by SELinux. But Stephen thought it useful for other LSMs as well, as it could be expected that other LSMs would also need to track enclave pages and end up duplicating what's done inside SELinux.
I'm ok either way, though I do agree with Stephen's assessment.
>
>
> >
> > And that's not the most important point. The major problem in his
> patch lies in SGX2 support, as #PF driven EAUG cannot be supported (or
> he'd have to amend his code accordingly, which will add complexity and
> tip your scale).
> >
>
> Why can't it be?
Let me take it back. It's important as it is where LSM folks are divided.
I intended to say the major reason I objected Sean's approach was its inability to support SGX2 smoothly - as #PF driven EAUG requires non-existent pages to be mmap()'ed, otherwise vm_ops->fault wouldn't be dispatched so EAUG couldn't be issued in response to #PF.
^ permalink raw reply
* RE: [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation
From: Xing, Cedric @ 2019-07-01 18:54 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stephen Smalley, Christopherson, Sean J, Jarkko Sakkinen,
linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, Roberts, William C, Schaufler, Casey,
James Morris, Hansen, Dave, Jethro Beekman, Dr . Greg Wettstein
In-Reply-To: <CALCETrXjq9JNjXZo3Va83Ca7fiAJx7ZM9VRWYebzpyH6ug0cKg@mail.gmail.com>
> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Monday, July 01, 2019 10:54 AM
>
> On Mon, Jul 1, 2019 at 10:46 AM Xing, Cedric <cedric.xing@intel.com>
> wrote:
> >
> > > From: Andy Lutomirski [mailto:luto@kernel.org]
> > > Sent: Saturday, June 29, 2019 4:42 PM
> > >
> > > On Tue, Jun 25, 2019 at 2:09 PM Stephen Smalley <sds@tycho.nsa.gov>
> > > wrote:
> > > >
> > > > On 6/21/19 5:22 PM, Xing, Cedric wrote:
> > > > >> From: Christopherson, Sean J
> > > > >> Sent: Wednesday, June 19, 2019 3:24 PM
> > > > >>
> > > > >> Intended use of each permission:
> > > > >>
> > > > >> - SGX_EXECDIRTY: dynamically load code within the enclave
> itself
> > > > >> - SGX_EXECUNMR: load unmeasured code into the enclave, e.g.
> > > > >> Graphene
> > > > >
> > > > > Why does it matter whether a code page is measured or not?
> > > >
> > > > It won't be incorporated into an attestation?
> > > >
> > >
> > > Also, if there is, in parallel, a policy that limits the set of
> > > enclave SIGSTRUCTs that are accepted, requiring all code be measured
> > > makes it harder to subvert by writing incompetent or maliciously
> > > incompetent enclaves.
> >
> > As analyzed in my reply to one of Stephen's comments, no executable
> page shall be "enclave only" as enclaves have access to host's memory,
> so if an executable page in regular memory is considered posting a
> threat to the process, it should be considered posting the same threat
> inside an enclave as well.
What I was trying to say was, an executable page, if considered a threat to the enclosing process, should always be considered a threat no matter it is in that process's memory or inside an enclave enclosed in that same process's address space.
Therefore, for a page in regular memory, if it is denied executable, it is because it is considered a potential security threat to the enclosing process, so it shall not be used as the source for an executable enclave page, as the same threat exists regardless it is in regular memory or EPC. Does that make more sense?
>
> Huh? The SDM (37.3 in whateve version I'm reading) says "Code fetches
> from inside an enclave to a linear address outside that enclave result
> in a #GP(0) exception." Enclaves execute enclave code only.
>
> In any event, I believe we're discussing taking readable memory from
> outside the enclave and copying it to an executable code inside the
> enclave.
You are correct. SGX ISA doesn't care the source page as it only takes care of the security the enclave itself.
But LSM on the other hand also takes care of the enclosing process. That said, a page, if denied executable because it is considered a potential threat to the process by LSM, should also be denied (by LSM) as the source for an executable enclave page because the same threat would exist even if it resides inside an enclave, for enclaves have access to all of the enclosing process's memory.
>
> >
> > That said, every executable enclave page should have an executable
> > source page (doesn’t have to executable, as long as mprotect(X) would
> > succeed on it, as shown in my patch)
>
> Does Sean's series require this? I think that, if we can get away with
> it, it's a lot nicer to *not* require user code to map the source pages
> PROT_EXEC. Some policy may check that it's VM_MAYEXEC or check some
> other attribute of the VMA, but actually requiring PROT_EXEC seems like
> we're weakening existing hardening measures to enforce a policy, which
> is a mistake.
My patch doesn't require X on source pages either. I said "would", meaning X *would* be granted but doesn't have to be granted. You can see this in selinux_enclave_load() calling selinux_file_mprotect() in my code. The purpose is to determine if X *would* be granted to the source pages without actually granting X.
^ permalink raw reply
* RE: [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation
From: Xing, Cedric @ 2019-07-01 19:03 UTC (permalink / raw)
To: Xing, Cedric, Andy Lutomirski
Cc: Stephen Smalley, Christopherson, Sean J, Jarkko Sakkinen,
linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, Roberts, William C, Schaufler, Casey,
James Morris, Hansen, Dave, Jethro Beekman, Dr . Greg Wettstein
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F6551D63B@ORSMSX116.amr.corp.intel.com>
Hi Andy,
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Xing, Cedric
> Sent: Monday, July 01, 2019 11:54 AM
> > >
> > > That said, every executable enclave page should have an executable
> > > source page (doesn’t have to executable, as long as mprotect(X)
> would
> > > succeed on it, as shown in my patch)
> >
> > Does Sean's series require this? I think that, if we can get away
> with
> > it, it's a lot nicer to *not* require user code to map the source
> pages
> > PROT_EXEC. Some policy may check that it's VM_MAYEXEC or check some
> > other attribute of the VMA, but actually requiring PROT_EXEC seems
> like
> > we're weakening existing hardening measures to enforce a policy, which
> > is a mistake.
>
> My patch doesn't require X on source pages either. I said "would",
> meaning X *would* be granted but doesn't have to be granted. You can see
> this in selinux_enclave_load() calling selinux_file_mprotect() in my
> code. The purpose is to determine if X *would* be granted to the source
> pages without actually granting X.
Forgot to conclude that we are on the same page for the requirement on the source pages.
And given that requirement (enclave page cannot be X unless source would also be allowed X), measuring enclave code pages or not doesn't make any difference from the enclosing process's perspective in terms of security. So it only makes a difference for the enclave, which however has been covered cryptographically by its measurement already. So SGX_EXECUNMR doesn't have any practical use, thus I don't think it should be added as a new permission.
^ permalink raw reply
* RE: [RFC PATCH v4 04/12] x86/sgx: Require userspace to define enclave pages' protection bits
From: Xing, Cedric @ 2019-07-01 19:22 UTC (permalink / raw)
To: Andy Lutomirski, Christopherson, Sean J
Cc: Jarkko Sakkinen, linux-sgx@vger.kernel.org, LSM List,
selinux@vger.kernel.org, Roberts, William C, Schaufler, Casey,
James Morris, Hansen, Dave, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <CALCETrXtVzCZW4mb994prdrzXxMP2T=xxU+fhy5k9N8AqjcqhA@mail.gmail.com>
> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Monday, July 01, 2019 11:00 AM
>
> On Wed, Jun 19, 2019 at 3:24 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> > static int sgx_mmap(struct file *file, struct vm_area_struct *vma) {
> > struct sgx_encl *encl = file->private_data;
> > + unsigned long allowed_rwx;
> > int ret;
> >
> > + allowed_rwx = sgx_allowed_rwx(encl, vma);
> > + if (vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC) &
> ~allowed_rwx)
> > + return -EACCES;
> > +
> > ret = sgx_encl_mm_add(encl, vma->vm_mm);
> > if (ret)
> > return ret;
> >
> > + if (!(allowed_rwx & VM_READ))
> > + vma->vm_flags &= ~VM_MAYREAD;
> > + if (!(allowed_rwx & VM_WRITE))
> > + vma->vm_flags &= ~VM_MAYWRITE;
> > + if (!(allowed_rwx & VM_EXEC))
> > + vma->vm_flags &= ~VM_MAYEXEC;
> > +
>
> I'm with Cedric here -- this is no good. The reason I think we
> need .may_mprotect or similar is exactly to avoid doing this.
>
> mmap() just needs to make the same type of VMA regardless of the pages
> in the range.
Instead of making decisions on its own, a more generic approach is for SGX subsystem/module to ask LSM for a decision, by calling security_file_mprotect() - as a new mapping could be considered as changing protection from PROT_NONE to (vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC)).
.may_mprotect() also solves part of the problem - i.e. VMAs will be created consistently but non-existent pages still cannot be mapped, which however is necessary for #PF driven EAUG in SGX2. Given that security_file_mprotect() is invoked by mprotect() syscall, it looks to me a more streamlined solution to call the same hook (security_file_mprotect) from both places (mmap and mprotect).
^ 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