* [PATCH V6 4/4] efi: Attempt to get the TCG2 event log in the boot stub
From: Matthew Garrett @ 2019-05-17 21:39 UTC (permalink / raw)
To: linux-integrity
Cc: peterhuewe, jarkko.sakkinen, jgg, roberto.sassu, linux-efi,
linux-security-module, linux-kernel, tweek, bsz, Matthew Garrett
In-Reply-To: <20190517213918.26045-1-matthewgarrett@google.com>
From: Matthew Garrett <mjg59@google.com>
Right now we only attempt to obtain the SHA1-only event log. The
protocol also supports a crypto agile log format, which contains digests
for all algorithms in use. Attempt to obtain this first, and fall back
to obtaining the older format if the system doesn't support it. This is
lightly complicated by the event sizes being variable (as we don't know
in advance which algorithms are in use), and the interface giving us
back a pointer to the start of the final entry rather than a pointer to
the end of the log - as a result, we need to parse the final entry to
figure out its length in order to know how much data to copy up to the
OS.
Signed-off-by: Matthew Garrett <mjg59@google.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
drivers/firmware/efi/libstub/tpm.c | 57 ++++++++++++++++++++----------
1 file changed, 39 insertions(+), 18 deletions(-)
diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
index 5bd04f75d8d6..b3f30448e454 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -8,8 +8,13 @@
* Thiebaud Weksteen <tweek@google.com>
*/
#include <linux/efi.h>
-#include <linux/tpm_eventlog.h>
#include <asm/efi.h>
+/*
+ * KASAN redefines memcpy() in a way that isn't available in the EFI stub.
+ * We need to include asm/efi.h before linux/tpm_eventlog.h in order to avoid
+ * the wrong memcpy() being referenced.
+ */
+#include <linux/tpm_eventlog.h>
#include "efistub.h"
@@ -57,7 +62,7 @@ void efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg)
#endif
-static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
+void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg)
{
efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
efi_guid_t linux_eventlog_guid = LINUX_EFI_TPM_EVENT_LOG_GUID;
@@ -67,6 +72,7 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
unsigned long first_entry_addr, last_entry_addr;
size_t log_size, last_entry_size;
efi_bool_t truncated;
+ int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
void *tcg2_protocol = NULL;
status = efi_call_early(locate_protocol, &tcg2_guid, NULL,
@@ -74,14 +80,20 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
if (status != EFI_SUCCESS)
return;
- status = efi_call_proto(efi_tcg2_protocol, get_event_log, tcg2_protocol,
- EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2,
- &log_location, &log_last_entry, &truncated);
- if (status != EFI_SUCCESS)
- return;
+ status = efi_call_proto(efi_tcg2_protocol, get_event_log,
+ tcg2_protocol, version, &log_location,
+ &log_last_entry, &truncated);
+
+ if (status != EFI_SUCCESS || !log_location) {
+ version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
+ status = efi_call_proto(efi_tcg2_protocol, get_event_log,
+ tcg2_protocol, version, &log_location,
+ &log_last_entry, &truncated);
+ if (status != EFI_SUCCESS || !log_location)
+ return;
+
+ }
- if (!log_location)
- return;
first_entry_addr = (unsigned long) log_location;
/*
@@ -96,8 +108,23 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
* We need to calculate its size to deduce the full size of
* the logs.
*/
- last_entry_size = sizeof(struct tcpa_event) +
- ((struct tcpa_event *) last_entry_addr)->event_size;
+ if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) {
+ /*
+ * The TCG2 log format has variable length entries,
+ * and the information to decode the hash algorithms
+ * back into a size is contained in the first entry -
+ * pass a pointer to the final entry (to calculate its
+ * size) and the first entry (so we know how long each
+ * digest is)
+ */
+ last_entry_size =
+ __calc_tpm2_event_size((void *)last_entry_addr,
+ (void *)(long)log_location,
+ false);
+ } else {
+ last_entry_size = sizeof(struct tcpa_event) +
+ ((struct tcpa_event *) last_entry_addr)->event_size;
+ }
log_size = log_last_entry - log_location + last_entry_size;
}
@@ -114,7 +141,7 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
log_tbl->size = log_size;
- log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
+ log_tbl->version = version;
memcpy(log_tbl->log, (void *) first_entry_addr, log_size);
status = efi_call_early(install_configuration_table,
@@ -126,9 +153,3 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
err_free:
efi_call_early(free_pool, log_tbl);
}
-
-void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg)
-{
- /* Only try to retrieve the logs in 1.2 format. */
- efi_retrieve_tpm2_eventlog_1_2(sys_table_arg);
-}
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related
* [PATCH V6 3/4] tpm: Append the final event log to the TPM event log
From: Matthew Garrett @ 2019-05-17 21:39 UTC (permalink / raw)
To: linux-integrity
Cc: peterhuewe, jarkko.sakkinen, jgg, roberto.sassu, linux-efi,
linux-security-module, linux-kernel, tweek, bsz, Matthew Garrett
In-Reply-To: <20190517213918.26045-1-matthewgarrett@google.com>
From: Matthew Garrett <mjg59@google.com>
Any events that are logged after GetEventsLog() is called are logged to
the EFI Final Events table. These events are defined as being in the
crypto agile log format, so we can just append them directly to the
existing log if it's in the same format. In theory we can also construct
old-style SHA1 log entries for devices that only return logs in that
format, but EDK2 doesn't generate the final event log in that case so
it doesn't seem worth it at the moment.
Signed-off-by: Matthew Garrett <mjg59@google.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
drivers/char/tpm/eventlog/efi.c | 50 ++++++++++++++++++++++++++++-----
1 file changed, 43 insertions(+), 7 deletions(-)
diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
index 3e673ab22cb4..9179cf6bdee9 100644
--- a/drivers/char/tpm/eventlog/efi.c
+++ b/drivers/char/tpm/eventlog/efi.c
@@ -21,10 +21,13 @@
int tpm_read_log_efi(struct tpm_chip *chip)
{
+ struct efi_tcg2_final_events_table *final_tbl = NULL;
struct linux_efi_tpm_eventlog *log_tbl;
struct tpm_bios_log *log;
u32 log_size;
u8 tpm_log_version;
+ void *tmp;
+ int ret;
if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
return -ENODEV;
@@ -52,15 +55,48 @@ int tpm_read_log_efi(struct tpm_chip *chip)
/* malloc EventLog space */
log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL);
- if (!log->bios_event_log)
- goto err_memunmap;
- log->bios_event_log_end = log->bios_event_log + log_size;
+ if (!log->bios_event_log) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ log->bios_event_log_end = log->bios_event_log + log_size;
tpm_log_version = log_tbl->version;
- memunmap(log_tbl);
- return tpm_log_version;
-err_memunmap:
+ ret = tpm_log_version;
+
+ if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR ||
+ efi_tpm_final_log_size == 0 ||
+ tpm_log_version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
+ goto out;
+
+ final_tbl = memremap(efi.tpm_final_log,
+ sizeof(*final_tbl) + efi_tpm_final_log_size,
+ MEMREMAP_WB);
+ if (!final_tbl) {
+ pr_err("Could not map UEFI TPM final log\n");
+ kfree(log->bios_event_log);
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ tmp = krealloc(log->bios_event_log,
+ log_size + efi_tpm_final_log_size,
+ GFP_KERNEL);
+ if (!tmp) {
+ kfree(log->bios_event_log);
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ log->bios_event_log = tmp;
+ memcpy((void *)log->bios_event_log + log_size,
+ final_tbl->events, efi_tpm_final_log_size);
+ log->bios_event_log_end = log->bios_event_log +
+ log_size + efi_tpm_final_log_size;
+
+out:
+ memunmap(final_tbl);
memunmap(log_tbl);
- return -ENOMEM;
+ return ret;
}
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related
* [PATCH V6 2/4] tpm: Reserve the TPM final events table
From: Matthew Garrett @ 2019-05-17 21:39 UTC (permalink / raw)
To: linux-integrity
Cc: peterhuewe, jarkko.sakkinen, jgg, roberto.sassu, linux-efi,
linux-security-module, linux-kernel, tweek, bsz, Matthew Garrett
In-Reply-To: <20190517213918.26045-1-matthewgarrett@google.com>
From: Matthew Garrett <mjg59@google.com>
UEFI systems provide a boot services protocol for obtaining the TPM
event log, but this is unusable after ExitBootServices() is called.
Unfortunately ExitBootServices() itself triggers additional TPM events
that then can't be obtained using this protocol. The platform provides a
mechanism for the OS to obtain these events by recording them to a
separate UEFI configuration table which the OS can then map.
Unfortunately this table isn't self describing in terms of providing its
length, so we need to parse the events inside it to figure out how long
it is. Since the table isn't mapped at this point, we need to extend the
length calculation function to be able to map the event as it goes
along.
(Fixes by Bartosz Szczepanek <bsz@semihalf.com>)
Signed-off-by: Matthew Garrett <mjg59@google.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
drivers/char/tpm/eventlog/tpm2.c | 2 +-
drivers/firmware/efi/efi.c | 2 +
drivers/firmware/efi/tpm.c | 62 ++++++++++++++++++-
include/linux/efi.h | 9 +++
include/linux/tpm_eventlog.h | 102 ++++++++++++++++++++++++++++---
5 files changed, 164 insertions(+), 13 deletions(-)
diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c
index 1a977bdd3bd2..de1d9f7e5a92 100644
--- a/drivers/char/tpm/eventlog/tpm2.c
+++ b/drivers/char/tpm/eventlog/tpm2.c
@@ -40,7 +40,7 @@
static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
struct tcg_pcr_event *event_header)
{
- return __calc_tpm2_event_size(event, event_header);
+ return __calc_tpm2_event_size(event, event_header, false);
}
static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos)
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 55b77c576c42..6b11c41e0575 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -53,6 +53,7 @@ struct efi __read_mostly efi = {
.mem_attr_table = EFI_INVALID_TABLE_ADDR,
.rng_seed = EFI_INVALID_TABLE_ADDR,
.tpm_log = EFI_INVALID_TABLE_ADDR,
+ .tpm_final_log = EFI_INVALID_TABLE_ADDR,
.mem_reserve = EFI_INVALID_TABLE_ADDR,
};
EXPORT_SYMBOL(efi);
@@ -485,6 +486,7 @@ static __initdata efi_config_table_type_t common_tables[] = {
{EFI_MEMORY_ATTRIBUTES_TABLE_GUID, "MEMATTR", &efi.mem_attr_table},
{LINUX_EFI_RANDOM_SEED_TABLE_GUID, "RNG", &efi.rng_seed},
{LINUX_EFI_TPM_EVENT_LOG_GUID, "TPMEventLog", &efi.tpm_log},
+ {LINUX_EFI_TPM_FINAL_LOG_GUID, "TPMFinalLog", &efi.tpm_final_log},
{LINUX_EFI_MEMRESERVE_TABLE_GUID, "MEMRESERVE", &efi.mem_reserve},
{NULL_GUID, NULL, NULL},
};
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 3a689b40ccc0..2c912ea08166 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -4,34 +4,90 @@
* Thiebaud Weksteen <tweek@google.com>
*/
+#define TPM_MEMREMAP(start, size) early_memremap(start, size)
+#define TPM_MEMUNMAP(start, size) early_memunmap(start, size)
+
#include <linux/efi.h>
#include <linux/init.h>
#include <linux/memblock.h>
+#include <linux/tpm_eventlog.h>
#include <asm/early_ioremap.h>
+int efi_tpm_final_log_size;
+EXPORT_SYMBOL(efi_tpm_final_log_size);
+
+static int tpm2_calc_event_log_size(void *data, int count, void *size_info)
+{
+ struct tcg_pcr_event2_head *header;
+ int event_size, size = 0;
+
+ while (count > 0) {
+ header = data + size;
+ event_size = __calc_tpm2_event_size(header, size_info, true);
+ if (event_size == 0)
+ return -1;
+ size += event_size;
+ count--;
+ }
+
+ return size;
+}
+
/*
* Reserve the memory associated with the TPM Event Log configuration table.
*/
int __init efi_tpm_eventlog_init(void)
{
struct linux_efi_tpm_eventlog *log_tbl;
+ struct efi_tcg2_final_events_table *final_tbl;
unsigned int tbl_size;
+ int ret = 0;
- if (efi.tpm_log == EFI_INVALID_TABLE_ADDR)
+ if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) {
+ /*
+ * We can't calculate the size of the final events without the
+ * first entry in the TPM log, so bail here.
+ */
return 0;
+ }
log_tbl = early_memremap(efi.tpm_log, sizeof(*log_tbl));
if (!log_tbl) {
pr_err("Failed to map TPM Event Log table @ 0x%lx\n",
- efi.tpm_log);
+ efi.tpm_log);
efi.tpm_log = EFI_INVALID_TABLE_ADDR;
return -ENOMEM;
}
tbl_size = sizeof(*log_tbl) + log_tbl->size;
memblock_reserve(efi.tpm_log, tbl_size);
+
+ if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR)
+ goto out;
+
+ final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
+
+ if (!final_tbl) {
+ pr_err("Failed to map TPM Final Event Log table @ 0x%lx\n",
+ efi.tpm_final_log);
+ efi.tpm_final_log = EFI_INVALID_TABLE_ADDR;
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ tbl_size = tpm2_calc_event_log_size(efi.tpm_final_log
+ + sizeof(final_tbl->version)
+ + sizeof(final_tbl->nr_events),
+ final_tbl->nr_events,
+ log_tbl->log);
+ memblock_reserve((unsigned long)final_tbl,
+ tbl_size + sizeof(*final_tbl));
+ early_memunmap(final_tbl, sizeof(*final_tbl));
+ efi_tpm_final_log_size = tbl_size;
+
+out:
early_memunmap(log_tbl, sizeof(*log_tbl));
- return 0;
+ return ret;
}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 54357a258b35..e33c70a52a9d 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -689,6 +689,7 @@ void efi_native_runtime_setup(void);
#define LINUX_EFI_LOADER_ENTRY_GUID EFI_GUID(0x4a67b082, 0x0a4c, 0x41cf, 0xb6, 0xc7, 0x44, 0x0b, 0x29, 0xbb, 0x8c, 0x4f)
#define LINUX_EFI_RANDOM_SEED_TABLE_GUID EFI_GUID(0x1ce1e5bc, 0x7ceb, 0x42f2, 0x81, 0xe5, 0x8a, 0xad, 0xf1, 0x80, 0xf5, 0x7b)
#define LINUX_EFI_TPM_EVENT_LOG_GUID EFI_GUID(0xb7799cb0, 0xeca2, 0x4943, 0x96, 0x67, 0x1f, 0xae, 0x07, 0xb7, 0x47, 0xfa)
+#define LINUX_EFI_TPM_FINAL_LOG_GUID EFI_GUID(0x1e2ed096, 0x30e2, 0x4254, 0xbd, 0x89, 0x86, 0x3b, 0xbe, 0xf8, 0x23, 0x25)
#define LINUX_EFI_MEMRESERVE_TABLE_GUID EFI_GUID(0x888eb0c6, 0x8ede, 0x4ff5, 0xa8, 0xf0, 0x9a, 0xee, 0x5c, 0xb9, 0x77, 0xc2)
typedef struct {
@@ -996,6 +997,7 @@ extern struct efi {
unsigned long mem_attr_table; /* memory attributes table */
unsigned long rng_seed; /* UEFI firmware random seed */
unsigned long tpm_log; /* TPM2 Event Log table */
+ unsigned long tpm_final_log; /* TPM2 Final Events Log table */
unsigned long mem_reserve; /* Linux EFI memreserve table */
efi_get_time_t *get_time;
efi_set_time_t *set_time;
@@ -1707,6 +1709,13 @@ struct linux_efi_tpm_eventlog {
extern int efi_tpm_eventlog_init(void);
+struct efi_tcg2_final_events_table {
+ u64 version;
+ u64 nr_events;
+ u8 events[];
+};
+extern int efi_tpm_final_log_size;
+
/*
* efi_runtime_service() function identifiers.
* "NONE" is used by efi_recover_from_page_fault() to check if the page
diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index 6a86144e13f1..63238c84dc0b 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -112,10 +112,35 @@ struct tcg_pcr_event2_head {
struct tpm_digest digests[];
} __packed;
+struct tcg_algorithm_size {
+ u16 algorithm_id;
+ u16 algorithm_size;
+};
+
+struct tcg_algorithm_info {
+ u8 signature[16];
+ u32 platform_class;
+ u8 spec_version_minor;
+ u8 spec_version_major;
+ u8 spec_errata;
+ u8 uintn_size;
+ u32 number_of_algorithms;
+ struct tcg_algorithm_size digest_sizes[];
+};
+
+#ifndef TPM_MEMREMAP
+#define TPM_MEMREMAP(start, size) NULL
+#endif
+
+#ifndef TPM_MEMUNMAP
+#define TPM_MEMUNMAP(start, size) do{} while(0)
+#endif
+
/**
* __calc_tpm2_event_size - calculate the size of a TPM2 event log entry
* @event: Pointer to the event whose size should be calculated
* @event_header: Pointer to the initial event containing the digest lengths
+ * @do_mapping: Whether or not the event needs to be mapped
*
* The TPM2 event log format can contain multiple digests corresponding to
* separate PCR banks, and also contains a variable length of the data that
@@ -131,10 +156,13 @@ struct tcg_pcr_event2_head {
*/
static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
- struct tcg_pcr_event *event_header)
+ struct tcg_pcr_event *event_header,
+ bool do_mapping)
{
struct tcg_efi_specid_event_head *efispecid;
struct tcg_event_field *event_field;
+ void *mapping = NULL;
+ int mapping_size;
void *marker;
void *marker_start;
u32 halg_size;
@@ -148,16 +176,49 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type)
+ sizeof(event->count);
+ /* Map the event header */
+ if (do_mapping) {
+ mapping_size = marker - marker_start;
+ mapping = TPM_MEMREMAP((unsigned long)marker_start,
+ mapping_size);
+ if (!mapping) {
+ size = 0;
+ goto out;
+ }
+ } else {
+ mapping = marker_start;
+ }
+
+ event = (struct tcg_pcr_event2_head *)mapping;
+
efispecid = (struct tcg_efi_specid_event_head *)event_header->event;
/* Check if event is malformed. */
- if (event->count > efispecid->num_algs)
- return 0;
+ if (event->count > efispecid->num_algs) {
+ size = 0;
+ goto out;
+ }
for (i = 0; i < event->count; i++) {
halg_size = sizeof(event->digests[i].alg_id);
- memcpy(&halg, marker, halg_size);
+
+ /* Map the digest's algorithm identifier */
+ if (do_mapping) {
+ TPM_MEMUNMAP(mapping, mapping_size);
+ mapping_size = halg_size;
+ mapping = TPM_MEMREMAP((unsigned long)marker,
+ mapping_size);
+ if (!mapping) {
+ size = 0;
+ goto out;
+ }
+ } else {
+ mapping = marker;
+ }
+
+ memcpy(&halg, mapping, halg_size);
marker = marker + halg_size;
+
for (j = 0; j < efispecid->num_algs; j++) {
if (halg == efispecid->digest_sizes[j].alg_id) {
marker +=
@@ -166,18 +227,41 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
}
}
/* Algorithm without known length. Such event is unparseable. */
- if (j == efispecid->num_algs)
- return 0;
+ if (j == efispecid->num_algs) {
+ size = 0;
+ goto out;
+ }
+ }
+
+ /*
+ * Map the event size - we don't read from the event itself, so
+ * we don't need to map it
+ */
+ if (do_mapping) {
+ TPM_MEMUNMAP(mapping, mapping_size);
+ mapping_size += sizeof(event_field->event_size);
+ mapping = TPM_MEMREMAP((unsigned long)marker,
+ mapping_size);
+ if (!mapping) {
+ size = 0;
+ goto out;
+ }
+ } else {
+ mapping = marker;
}
- event_field = (struct tcg_event_field *)marker;
+ event_field = (struct tcg_event_field *)mapping;
+
marker = marker + sizeof(event_field->event_size)
+ event_field->event_size;
size = marker - marker_start;
if ((event->event_type == 0) && (event_field->event_size == 0))
- return 0;
-
+ size = 0;
+out:
+ if (do_mapping)
+ TPM_MEMUNMAP(mapping, mapping_size);
return size;
}
+
#endif
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related
* [PATCH V6 1/4] tpm: Abstract crypto agile event size calculations
From: Matthew Garrett @ 2019-05-17 21:39 UTC (permalink / raw)
To: linux-integrity
Cc: peterhuewe, jarkko.sakkinen, jgg, roberto.sassu, linux-efi,
linux-security-module, linux-kernel, tweek, bsz, Matthew Garrett
In-Reply-To: <20190517213918.26045-1-matthewgarrett@google.com>
From: Matthew Garrett <mjg59@google.com>
We need to calculate the size of crypto agile events in multiple
locations, including in the EFI boot stub. The easiest way to do this is
to put it in a header file as an inline and leave a wrapper to ensure we
don't end up with multiple copies of it embedded in the existing code.
Signed-off-by: Matthew Garrett <mjg59@google.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
drivers/char/tpm/eventlog/tpm2.c | 47 +---------------------
include/linux/tpm_eventlog.h | 68 ++++++++++++++++++++++++++++++++
2 files changed, 69 insertions(+), 46 deletions(-)
diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c
index f824563fc28d..1a977bdd3bd2 100644
--- a/drivers/char/tpm/eventlog/tpm2.c
+++ b/drivers/char/tpm/eventlog/tpm2.c
@@ -40,52 +40,7 @@
static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
struct tcg_pcr_event *event_header)
{
- struct tcg_efi_specid_event_head *efispecid;
- struct tcg_event_field *event_field;
- void *marker;
- void *marker_start;
- u32 halg_size;
- size_t size;
- u16 halg;
- int i;
- int j;
-
- marker = event;
- marker_start = marker;
- marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type)
- + sizeof(event->count);
-
- efispecid = (struct tcg_efi_specid_event_head *)event_header->event;
-
- /* Check if event is malformed. */
- if (event->count > efispecid->num_algs)
- return 0;
-
- for (i = 0; i < event->count; i++) {
- halg_size = sizeof(event->digests[i].alg_id);
- memcpy(&halg, marker, halg_size);
- marker = marker + halg_size;
- for (j = 0; j < efispecid->num_algs; j++) {
- if (halg == efispecid->digest_sizes[j].alg_id) {
- marker +=
- efispecid->digest_sizes[j].digest_size;
- break;
- }
- }
- /* Algorithm without known length. Such event is unparseable. */
- if (j == efispecid->num_algs)
- return 0;
- }
-
- event_field = (struct tcg_event_field *)marker;
- marker = marker + sizeof(event_field->event_size)
- + event_field->event_size;
- size = marker - marker_start;
-
- if ((event->event_type == 0) && (event_field->event_size == 0))
- return 0;
-
- return size;
+ return __calc_tpm2_event_size(event, event_header);
}
static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos)
diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index 81519f163211..6a86144e13f1 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -112,4 +112,72 @@ struct tcg_pcr_event2_head {
struct tpm_digest digests[];
} __packed;
+/**
+ * __calc_tpm2_event_size - calculate the size of a TPM2 event log entry
+ * @event: Pointer to the event whose size should be calculated
+ * @event_header: Pointer to the initial event containing the digest lengths
+ *
+ * The TPM2 event log format can contain multiple digests corresponding to
+ * separate PCR banks, and also contains a variable length of the data that
+ * was measured. This requires knowledge of how long each digest type is,
+ * and this information is contained within the first event in the log.
+ *
+ * We calculate the length by examining the number of events, and then looking
+ * at each event in turn to determine how much space is used for events in
+ * total. Once we've done this we know the offset of the data length field,
+ * and can calculate the total size of the event.
+ *
+ * Return: size of the event on success, <0 on failure
+ */
+
+static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
+ struct tcg_pcr_event *event_header)
+{
+ struct tcg_efi_specid_event_head *efispecid;
+ struct tcg_event_field *event_field;
+ void *marker;
+ void *marker_start;
+ u32 halg_size;
+ size_t size;
+ u16 halg;
+ int i;
+ int j;
+
+ marker = event;
+ marker_start = marker;
+ marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type)
+ + sizeof(event->count);
+
+ efispecid = (struct tcg_efi_specid_event_head *)event_header->event;
+
+ /* Check if event is malformed. */
+ if (event->count > efispecid->num_algs)
+ return 0;
+
+ for (i = 0; i < event->count; i++) {
+ halg_size = sizeof(event->digests[i].alg_id);
+ memcpy(&halg, marker, halg_size);
+ marker = marker + halg_size;
+ for (j = 0; j < efispecid->num_algs; j++) {
+ if (halg == efispecid->digest_sizes[j].alg_id) {
+ marker +=
+ efispecid->digest_sizes[j].digest_size;
+ break;
+ }
+ }
+ /* Algorithm without known length. Such event is unparseable. */
+ if (j == efispecid->num_algs)
+ return 0;
+ }
+
+ event_field = (struct tcg_event_field *)marker;
+ marker = marker + sizeof(event_field->event_size)
+ + event_field->event_size;
+ size = marker - marker_start;
+
+ if ((event->event_type == 0) && (event_field->event_size == 0))
+ return 0;
+
+ return size;
+}
#endif
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related
* [PATCH V6 0/4] Add support for crypto agile TPM event logs
From: Matthew Garrett @ 2019-05-17 21:39 UTC (permalink / raw)
To: linux-integrity
Cc: peterhuewe, jarkko.sakkinen, jgg, roberto.sassu, linux-efi,
linux-security-module, linux-kernel, tweek, bsz
Updated with the fixes from Bartosz and the header fixes folded in.
Bartosz, my machine still doesn't generate any final event log entries -
are you able to give this a test and make sure it's good?
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-17 21:36 UTC (permalink / raw)
To: Stephen Smalley
Cc: Andy Lutomirski, Xing, Cedric, Andy Lutomirski, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jarkko Sakkinen, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <c901ea99-5b43-a25d-03e8-55b4fce9c466@tycho.nsa.gov>
On Fri, May 17, 2019 at 04:09:22PM -0400, Stephen Smalley wrote:
> On 5/17/19 3:28 PM, Sean Christopherson wrote:
> >On Fri, May 17, 2019 at 02:05:39PM -0400, Stephen Smalley wrote:
> >Yep, and that's by design in the overall proposal. The trick is that
> >ENCLAVE_ADD takes a source VMA and copies the contents *and* the
> >permissions from the source VMA. The source VMA points at regular memory
> >that was mapped and populated using existing mechanisms for loading DSOs.
> >
> >E.g. at a high level:
> >
> >source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
> >for_each_chunk {
> > <hand waving - mmap()/mprotect() the enclave file into regular memory>
> >}
> >
> >enclave_fd = open("/dev/sgx/enclave", O_RDWR); /* allocs anon inode */
> >enclave_addr = mmap(NULL, size, PROT_READ, MAP_SHARED, enclave_fd, 0);
> >
> >ioctl(enclave_fd, ENCLAVE_CREATE, {enclave_addr});
> >for_each_chunk {
> > struct sgx_enclave_add ioctlargs = {
> > .offset = chunk.offset,
> > .source = chunk.addr,
> > .size = chunk.size,
> > .type = chunk.type, /* SGX specific metadata */
> > }
> > ioctl(fd, ENCLAVE_ADD, &ioctlargs); /* modifies enclave's VMAs */
> >}
> >ioctl(fd, ENCLAVE_INIT, ...);
> >
> >
> >Userspace never explicitly requests PROT_EXEC on enclave_fd, but SGX also
> >ensures userspace isn't bypassing LSM policies by virtue of copying the
> >permissions for EPC VMAs from regular VMAs that have already gone through
> >LSM checks.
>
> Is O_RDWR required for /dev/sgx/enclave or would O_RDONLY suffice? Do you
> do anything other than ioctl() calls on it?
Hmm, in the current implementation, yes, O_RDWR is required. An enclave
and its associated EPC memory are represented and referenced by its fd,
which is backed by /dev/sgx/enclave. An enclave is not just code, e.g.
also has a heap, stack, variables, etc..., which need to be mapped
accordingly. In the current implementation, userspace directly does
mprotect() or mmap() on EPC VMAs, and so setting PROT_WRITE for the heap
and whatnot requires opening /dev/sgx/enclave with O_RDWR.
I *think* /dev/sgx/enclave could be opened O_RDONLY if ENCLAVE_ADD stuffed
the EPC VMA permissions, assuming the use case doesn't require changing
permissions after the enclave has been created.
The other reason userspace would need to open /dev/sgx/enclave O_RDWR
would be to debug an enclave, e.g. pwrite() works on the enclave fd due
to SGX restrictions on modifying EPC memory from outside the enclave.
But that's an obvious case where FILE__WRITE should be required.
> What's the advantage of allocating an anon inode in the above? At present
> anon inodes are exempted from inode-based checking, thereby losing the
> ability to perform SELinux ioctl whitelisting, unlike the file-backed
> /dev/sgx/enclave inode.
Purely to trigger the EXECMEM check on any PROT_EXEC mapping. However,
the motiviation for that was due to my bad assumption that FILE__WRITE
and FILE__EXECUTE are global and not per process. If we can do as you
suggest and allow creation of enclaves with O_RDONLY, then keeping a
file-backed inode is definitely better as it means most processes only
need FILE__READ and FILE__* in general has actual meaning.
Thanks a bunch for your help!
^ permalink raw reply
* Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
From: Rob Landley @ 2019-05-17 21:17 UTC (permalink / raw)
To: hpa, Roberto Sassu, viro
Cc: linux-security-module, linux-integrity, initramfs, linux-api,
linux-fsdevel, linux-kernel, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, arnd, james.w.mcmechan,
niveditas98
In-Reply-To: <CD9A4F89-7CA5-4329-A06A-F8DEB87905A5@zytor.com>
On 5/17/19 3:18 PM, hpa@zytor.com wrote:
> Ok... I just realized this does not work for a modular initramfs, composed at load time from multiple files, which is a very real problem. Should be easy enough to deal with: instead of one large file, use one companion file per source file, perhaps something like filename..xattrs (suggesting double dots to make it less likely to conflict with a "real" file.) No leading dot, as it makes it more likely that archivers will sort them before the file proper.
>
> A side benefit is that the format can be simpler as there is no need to encode the filename.
>
> A technically cleaner solution still, but which would need archiver modifications, would be to encode the xattrs as an optionally nameless file (just an empty string) with a new file mode value, immediately following the original file. The advantage there is that the archiver itself could support xattrs and other extended metadata (which has been requested elsewhere); the disadvantage obviously is that that it requires new support in the archiver. However, at least it ought to be simpler since it is still a higher protocol level than the cpio archive itself.
>
> There's already one special case in cpio, which is the "!!!TRAILER!!!" filename; although I don't think it is part of the formal spec, to the extent there is one, I would expect that in practice it is always encoded with a mode of 0, which incidentally could be used to unbreak the case where such a filename actually exists. So one way to support such extended metadata would be to set mode to 0 and use the filename to encode the type of metadata. I wonder how existing GNU or BSD cpio (the BSD one is better maintained these days) would deal with reading such a file; it would at least not be a regression if it just read it still, possibly with warnings. It could also be possible to use bits 17:16 in the mode, which are traditionally always zero (mode_t being 16 bits), but I believe are present in most or all of the cpio formats for historical reasons. It might be accepted better by existing implementations to use one of these high bits combined with S_IFREG, I dont know.
>
I'll happily modify toybox cpio to understand xattrs (compress and decompress),
the android guys do a lot with xattrs already. I tapped out of _this_ discussion
from disgust with the proposed encoding.
Rob
^ permalink raw reply
* Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
From: Arvind Sankar @ 2019-05-17 21:10 UTC (permalink / raw)
To: Arvind Sankar
Cc: hpa, Roberto Sassu, viro, linux-security-module, linux-integrity,
initramfs, linux-api, linux-fsdevel, linux-kernel, zohar,
silviu.vlasceanu, dmitry.kasatkin, takondra, kamensky, arnd, rob,
james.w.mcmechan, niveditas98
In-Reply-To: <20190517210219.GA5998@rani.riverdale.lan>
On Fri, May 17, 2019 at 05:02:20PM -0400, Arvind Sankar wrote:
> On Fri, May 17, 2019 at 01:18:11PM -0700, hpa@zytor.com wrote:
> >
> > Ok... I just realized this does not work for a modular initramfs, composed at load time from multiple files, which is a very real problem. Should be easy enough to deal with: instead of one large file, use one companion file per source file, perhaps something like filename..xattrs (suggesting double dots to make it less likely to conflict with a "real" file.) No leading dot, as it makes it more likely that archivers will sort them before the file proper.
> This version of the patch was changed from the previous one exactly to deal with this case --
> it allows for the bootloader to load multiple initramfs archives, each
> with its own .xattr-list file, and to have that work properly.
> Could you elaborate on the issue that you see?
Roberto, are you missing a changelog entry for v2->v3 change?
^ permalink raw reply
* Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
From: Arvind Sankar @ 2019-05-17 21:02 UTC (permalink / raw)
To: hpa
Cc: Roberto Sassu, viro, linux-security-module, linux-integrity,
initramfs, linux-api, linux-fsdevel, linux-kernel, zohar,
silviu.vlasceanu, dmitry.kasatkin, takondra, kamensky, arnd, rob,
james.w.mcmechan, niveditas98
In-Reply-To: <CD9A4F89-7CA5-4329-A06A-F8DEB87905A5@zytor.com>
On Fri, May 17, 2019 at 01:18:11PM -0700, hpa@zytor.com wrote:
>
> Ok... I just realized this does not work for a modular initramfs, composed at load time from multiple files, which is a very real problem. Should be easy enough to deal with: instead of one large file, use one companion file per source file, perhaps something like filename..xattrs (suggesting double dots to make it less likely to conflict with a "real" file.) No leading dot, as it makes it more likely that archivers will sort them before the file proper.
This version of the patch was changed from the previous one exactly to deal with this case --
it allows for the bootloader to load multiple initramfs archives, each
with its own .xattr-list file, and to have that work properly.
Could you elaborate on the issue that you see?
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Stephen Smalley @ 2019-05-17 20:34 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Sean Christopherson, Xing, Cedric, Andy Lutomirski, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jarkko Sakkinen, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <ED98AEC9-FFA3-4DA4-9B86-11D8AADC9151@amacapital.net>
On 5/17/19 4:14 PM, Andy Lutomirski wrote:
>
>> On May 17, 2019, at 1:09 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>
>>> On 5/17/19 3:28 PM, Sean Christopherson wrote:
>>>> On Fri, May 17, 2019 at 02:05:39PM -0400, Stephen Smalley wrote:
>>>>> On 5/17/19 1:12 PM, Andy Lutomirski wrote:
>>>>>
>>>>> How can that work? Unless the API changes fairly radically, users
>>>>> fundamentally need to both write and execute the enclave. Some of it will
>>>>> be written only from already executable pages, and some privilege should be
>>>>> needed to execute any enclave page that was not loaded like this.
>>>>
>>>> I'm not sure what the API is. Let's say they do something like this:
>>>>
>>>> fd = open("/dev/sgx/enclave", O_RDONLY);
>>>> addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
>>>> stuff addr into ioctl args
>>>> ioctl(fd, ENCLAVE_CREATE, &ioctlargs);
>>>> ioctl(fd, ENCLAVE_ADD_PAGE, &ioctlargs);
>>>> ioctl(fd, ENCLAVE_INIT, &ioctlargs);
>>> That's rougly the flow, except that that all enclaves need to have RW and
>>> X EPC pages.
>>>> The important points are that they do not open /dev/sgx/enclave with write
>>>> access (otherwise they will trigger FILE__WRITE at open time, and later
>>>> encounter FILE__EXECUTE as well during mmap, thereby requiring both to be
>>>> allowed to /dev/sgx/enclave), and that they do not request PROT_WRITE to the
>>>> resulting mapping (otherwise they will trigger FILE__WRITE at mmap time).
>>>> Then only FILE__READ and FILE__EXECUTE are required to /dev/sgx/enclave in
>>>> policy.
>>>>
>>>> If they switch to an anon inode, then any mmap PROT_EXEC of the opened file
>>>> will trigger an EXECMEM check, at least as currently implemented, as we have
>>>> no useful backing inode information.
>>> Yep, and that's by design in the overall proposal. The trick is that
>>> ENCLAVE_ADD takes a source VMA and copies the contents *and* the
>>> permissions from the source VMA. The source VMA points at regular memory
>>> that was mapped and populated using existing mechanisms for loading DSOs.
>>> E.g. at a high level:
>>> source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
>>> for_each_chunk {
>>> <hand waving - mmap()/mprotect() the enclave file into regular memory>
>>> }
>>> enclave_fd = open("/dev/sgx/enclave", O_RDWR); /* allocs anon inode */
>>> enclave_addr = mmap(NULL, size, PROT_READ, MAP_SHARED, enclave_fd, 0);
>>> ioctl(enclave_fd, ENCLAVE_CREATE, {enclave_addr});
>>> for_each_chunk {
>>> struct sgx_enclave_add ioctlargs = {
>>> .offset = chunk.offset,
>>> .source = chunk.addr,
>>> .size = chunk.size,
>>> .type = chunk.type, /* SGX specific metadata */
>>> }
>>> ioctl(fd, ENCLAVE_ADD, &ioctlargs); /* modifies enclave's VMAs */
>>> }
>>> ioctl(fd, ENCLAVE_INIT, ...);
>>> Userspace never explicitly requests PROT_EXEC on enclave_fd, but SGX also
>>> ensures userspace isn't bypassing LSM policies by virtue of copying the
>>> permissions for EPC VMAs from regular VMAs that have already gone through
>>> LSM checks.
>>
>> Is O_RDWR required for /dev/sgx/enclave or would O_RDONLY suffice? Do you do anything other than ioctl() calls on it?
>>
>> What's the advantage of allocating an anon inode in the above? At present anon inodes are exempted from inode-based checking, thereby losing the ability to perform SELinux ioctl whitelisting, unlike the file-backed /dev/sgx/enclave inode.
>>
>> How would SELinux (or other security modules) restrict the authorized enclaves that can be loaded via this interface? Would the sgx driver invoke a new LSM hook with the regular/source VMAs as parameters and allow the security module to reject the ENCLAVE_ADD operation? That could be just based on the vm_file (e.g. whitelist what enclave files are permitted in general) or it could be based on both the process and the vm_file (e.g. only allow specific enclaves to be loaded into specific processes).
>
> This is the idea behind the .sigstruct file. The driver could call a new hook to approve or reject the .sigstruct. The sigstruct contains a hash of the whole enclave and a signature by the author.
Ok, so same idea but moved to ENCLAVE_INIT and passing the vma or file
for the sigstruct instead of the enclave.
^ permalink raw reply
* Re: [PATCH v3 2/2] initramfs: introduce do_readxattrs()
From: hpa @ 2019-05-17 20:18 UTC (permalink / raw)
To: Roberto Sassu, viro
Cc: linux-security-module, linux-integrity, initramfs, linux-api,
linux-fsdevel, linux-kernel, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, arnd, rob, james.w.mcmechan,
niveditas98
In-Reply-To: <20190517165519.11507-3-roberto.sassu@huawei.com>
On May 17, 2019 9:55:19 AM PDT, Roberto Sassu <roberto.sassu@huawei.com> wrote:
>This patch adds support for an alternative method to add xattrs to
>files in
>the rootfs filesystem. Instead of extracting them directly from the ram
>disk image, they are extracted from a regular file called .xattr-list,
>that
>can be added by any ram disk generator available today. The file format
>is:
>
><file #N data len (ASCII, 10 chars)><file #N path>\0
><xattr #N data len (ASCII, 8 chars)><xattr #N name>\0<xattr #N value>
>
>.xattr-list can be generated by executing:
>
>$ getfattr --absolute-names -d -h -R -e hex -m - \
> <file list> | xattr.awk -b > ${initdir}/.xattr-list
>
>where the content of the xattr.awk script is:
>
>#! /usr/bin/awk -f
>{
> if (!length($0)) {
> printf("%.10x%s\0", len, file);
> for (x in xattr) {
> printf("%.8x%s\0", xattr_len[x], x);
> for (i = 0; i < length(xattr[x]) / 2; i++) {
> printf("%c", strtonum("0x"substr(xattr[x], i * 2 + 1, 2)));
> }
> }
> i = 0;
> delete xattr;
> delete xattr_len;
> next;
> };
> if (i == 0) {
> file=$3;
> len=length(file) + 8 + 1;
> }
> if (i > 0) {
> split($0, a, "=");
> xattr[a[1]]=substr(a[2], 3);
> xattr_len[a[1]]=length(a[1]) + 1 + 8 + length(xattr[a[1]]) / 2;
> len+=xattr_len[a[1]];
> };
> i++;
>}
>
>Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>---
> init/initramfs.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 99 insertions(+)
>
>diff --git a/init/initramfs.c b/init/initramfs.c
>index 0c6dd1d5d3f6..6ec018c6279a 100644
>--- a/init/initramfs.c
>+++ b/init/initramfs.c
>@@ -13,6 +13,8 @@
> #include <linux/namei.h>
> #include <linux/xattr.h>
>
>+#define XATTR_LIST_FILENAME ".xattr-list"
>+
> static ssize_t __init xwrite(int fd, const char *p, size_t count)
> {
> ssize_t out = 0;
>@@ -382,6 +384,97 @@ static int __init __maybe_unused do_setxattrs(char
>*pathname)
> return 0;
> }
>
>+struct path_hdr {
>+ char p_size[10]; /* total size including p_size field */
>+ char p_data[]; /* <path>\0<xattrs> */
>+};
>+
>+static int __init do_readxattrs(void)
>+{
>+ struct path_hdr hdr;
>+ char *path = NULL;
>+ char str[sizeof(hdr.p_size) + 1];
>+ unsigned long file_entry_size;
>+ size_t size, path_size, total_size;
>+ struct kstat st;
>+ struct file *file;
>+ loff_t pos;
>+ int ret;
>+
>+ ret = vfs_lstat(XATTR_LIST_FILENAME, &st);
>+ if (ret < 0)
>+ return ret;
>+
>+ total_size = st.size;
>+
>+ file = filp_open(XATTR_LIST_FILENAME, O_RDONLY, 0);
>+ if (IS_ERR(file))
>+ return PTR_ERR(file);
>+
>+ pos = file->f_pos;
>+
>+ while (total_size) {
>+ size = kernel_read(file, (char *)&hdr, sizeof(hdr), &pos);
>+ if (size != sizeof(hdr)) {
>+ ret = -EIO;
>+ goto out;
>+ }
>+
>+ total_size -= size;
>+
>+ str[sizeof(hdr.p_size)] = 0;
>+ memcpy(str, hdr.p_size, sizeof(hdr.p_size));
>+ ret = kstrtoul(str, 16, &file_entry_size);
>+ if (ret < 0)
>+ goto out;
>+
>+ file_entry_size -= sizeof(sizeof(hdr.p_size));
>+ if (file_entry_size > total_size) {
>+ ret = -EINVAL;
>+ goto out;
>+ }
>+
>+ path = vmalloc(file_entry_size);
>+ if (!path) {
>+ ret = -ENOMEM;
>+ goto out;
>+ }
>+
>+ size = kernel_read(file, path, file_entry_size, &pos);
>+ if (size != file_entry_size) {
>+ ret = -EIO;
>+ goto out_free;
>+ }
>+
>+ total_size -= size;
>+
>+ path_size = strnlen(path, file_entry_size);
>+ if (path_size == file_entry_size) {
>+ ret = -EINVAL;
>+ goto out_free;
>+ }
>+
>+ xattr_buf = path + path_size + 1;
>+ xattr_len = file_entry_size - path_size - 1;
>+
>+ ret = do_setxattrs(path);
>+ vfree(path);
>+ path = NULL;
>+
>+ if (ret < 0)
>+ break;
>+ }
>+out_free:
>+ vfree(path);
>+out:
>+ fput(file);
>+
>+ if (ret < 0)
>+ error("Unable to parse xattrs");
>+
>+ return ret;
>+}
>+
> static __initdata int wfd;
>
> static int __init do_name(void)
>@@ -391,6 +484,11 @@ static int __init do_name(void)
> if (strcmp(collected, "TRAILER!!!") == 0) {
> free_hash();
> return 0;
>+ } else if (strcmp(collected, XATTR_LIST_FILENAME) == 0) {
>+ struct kstat st;
>+
>+ if (!vfs_lstat(collected, &st))
>+ do_readxattrs();
> }
> clean_path(collected, mode);
> if (S_ISREG(mode)) {
>@@ -562,6 +660,7 @@ static char * __init unpack_to_rootfs(char *buf,
>unsigned long len)
> buf += my_inptr;
> len -= my_inptr;
> }
>+ do_readxattrs();
> dir_utime();
> kfree(name_buf);
> kfree(symlink_buf);
Ok... I just realized this does not work for a modular initramfs, composed at load time from multiple files, which is a very real problem. Should be easy enough to deal with: instead of one large file, use one companion file per source file, perhaps something like filename..xattrs (suggesting double dots to make it less likely to conflict with a "real" file.) No leading dot, as it makes it more likely that archivers will sort them before the file proper.
A side benefit is that the format can be simpler as there is no need to encode the filename.
A technically cleaner solution still, but which would need archiver modifications, would be to encode the xattrs as an optionally nameless file (just an empty string) with a new file mode value, immediately following the original file. The advantage there is that the archiver itself could support xattrs and other extended metadata (which has been requested elsewhere); the disadvantage obviously is that that it requires new support in the archiver. However, at least it ought to be simpler since it is still a higher protocol level than the cpio archive itself.
There's already one special case in cpio, which is the "!!!TRAILER!!!" filename; although I don't think it is part of the formal spec, to the extent there is one, I would expect that in practice it is always encoded with a mode of 0, which incidentally could be used to unbreak the case where such a filename actually exists. So one way to support such extended metadata would be to set mode to 0 and use the filename to encode the type of metadata. I wonder how existing GNU or BSD cpio (the BSD one is better maintained these days) would deal with reading such a file; it would at least not be a regression if it just read it still, possibly with warnings. It could also be possible to use bits 17:16 in the mode, which are traditionally always zero (mode_t being 16 bits), but I believe are present in most or all of the cpio formats for historical reasons. It might be accepted better by existing implementations to use one of these high bits combined with S_IFREG, I dont know.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-17 20:14 UTC (permalink / raw)
To: Stephen Smalley
Cc: Sean Christopherson, Xing, Cedric, Andy Lutomirski, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jarkko Sakkinen, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <c901ea99-5b43-a25d-03e8-55b4fce9c466@tycho.nsa.gov>
> On May 17, 2019, at 1:09 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
>> On 5/17/19 3:28 PM, Sean Christopherson wrote:
>>> On Fri, May 17, 2019 at 02:05:39PM -0400, Stephen Smalley wrote:
>>>> On 5/17/19 1:12 PM, Andy Lutomirski wrote:
>>>>
>>>> How can that work? Unless the API changes fairly radically, users
>>>> fundamentally need to both write and execute the enclave. Some of it will
>>>> be written only from already executable pages, and some privilege should be
>>>> needed to execute any enclave page that was not loaded like this.
>>>
>>> I'm not sure what the API is. Let's say they do something like this:
>>>
>>> fd = open("/dev/sgx/enclave", O_RDONLY);
>>> addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
>>> stuff addr into ioctl args
>>> ioctl(fd, ENCLAVE_CREATE, &ioctlargs);
>>> ioctl(fd, ENCLAVE_ADD_PAGE, &ioctlargs);
>>> ioctl(fd, ENCLAVE_INIT, &ioctlargs);
>> That's rougly the flow, except that that all enclaves need to have RW and
>> X EPC pages.
>>> The important points are that they do not open /dev/sgx/enclave with write
>>> access (otherwise they will trigger FILE__WRITE at open time, and later
>>> encounter FILE__EXECUTE as well during mmap, thereby requiring both to be
>>> allowed to /dev/sgx/enclave), and that they do not request PROT_WRITE to the
>>> resulting mapping (otherwise they will trigger FILE__WRITE at mmap time).
>>> Then only FILE__READ and FILE__EXECUTE are required to /dev/sgx/enclave in
>>> policy.
>>>
>>> If they switch to an anon inode, then any mmap PROT_EXEC of the opened file
>>> will trigger an EXECMEM check, at least as currently implemented, as we have
>>> no useful backing inode information.
>> Yep, and that's by design in the overall proposal. The trick is that
>> ENCLAVE_ADD takes a source VMA and copies the contents *and* the
>> permissions from the source VMA. The source VMA points at regular memory
>> that was mapped and populated using existing mechanisms for loading DSOs.
>> E.g. at a high level:
>> source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
>> for_each_chunk {
>> <hand waving - mmap()/mprotect() the enclave file into regular memory>
>> }
>> enclave_fd = open("/dev/sgx/enclave", O_RDWR); /* allocs anon inode */
>> enclave_addr = mmap(NULL, size, PROT_READ, MAP_SHARED, enclave_fd, 0);
>> ioctl(enclave_fd, ENCLAVE_CREATE, {enclave_addr});
>> for_each_chunk {
>> struct sgx_enclave_add ioctlargs = {
>> .offset = chunk.offset,
>> .source = chunk.addr,
>> .size = chunk.size,
>> .type = chunk.type, /* SGX specific metadata */
>> }
>> ioctl(fd, ENCLAVE_ADD, &ioctlargs); /* modifies enclave's VMAs */
>> }
>> ioctl(fd, ENCLAVE_INIT, ...);
>> Userspace never explicitly requests PROT_EXEC on enclave_fd, but SGX also
>> ensures userspace isn't bypassing LSM policies by virtue of copying the
>> permissions for EPC VMAs from regular VMAs that have already gone through
>> LSM checks.
>
> Is O_RDWR required for /dev/sgx/enclave or would O_RDONLY suffice? Do you do anything other than ioctl() calls on it?
>
> What's the advantage of allocating an anon inode in the above? At present anon inodes are exempted from inode-based checking, thereby losing the ability to perform SELinux ioctl whitelisting, unlike the file-backed /dev/sgx/enclave inode.
>
> How would SELinux (or other security modules) restrict the authorized enclaves that can be loaded via this interface? Would the sgx driver invoke a new LSM hook with the regular/source VMAs as parameters and allow the security module to reject the ENCLAVE_ADD operation? That could be just based on the vm_file (e.g. whitelist what enclave files are permitted in general) or it could be based on both the process and the vm_file (e.g. only allow specific enclaves to be loaded into specific processes).
This is the idea behind the .sigstruct file. The driver could call a new hook to approve or reject the .sigstruct. The sigstruct contains a hash of the whole enclave and a signature by the author.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Stephen Smalley @ 2019-05-17 20:09 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Xing, Cedric, Andy Lutomirski, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jarkko Sakkinen, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <20190517192823.GG15006@linux.intel.com>
On 5/17/19 3:28 PM, Sean Christopherson wrote:
> On Fri, May 17, 2019 at 02:05:39PM -0400, Stephen Smalley wrote:
>> On 5/17/19 1:12 PM, Andy Lutomirski wrote:
>>>
>>> How can that work? Unless the API changes fairly radically, users
>>> fundamentally need to both write and execute the enclave. Some of it will
>>> be written only from already executable pages, and some privilege should be
>>> needed to execute any enclave page that was not loaded like this.
>>
>> I'm not sure what the API is. Let's say they do something like this:
>>
>> fd = open("/dev/sgx/enclave", O_RDONLY);
>> addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
>> stuff addr into ioctl args
>> ioctl(fd, ENCLAVE_CREATE, &ioctlargs);
>> ioctl(fd, ENCLAVE_ADD_PAGE, &ioctlargs);
>> ioctl(fd, ENCLAVE_INIT, &ioctlargs);
>
> That's rougly the flow, except that that all enclaves need to have RW and
> X EPC pages.
>
>> The important points are that they do not open /dev/sgx/enclave with write
>> access (otherwise they will trigger FILE__WRITE at open time, and later
>> encounter FILE__EXECUTE as well during mmap, thereby requiring both to be
>> allowed to /dev/sgx/enclave), and that they do not request PROT_WRITE to the
>> resulting mapping (otherwise they will trigger FILE__WRITE at mmap time).
>> Then only FILE__READ and FILE__EXECUTE are required to /dev/sgx/enclave in
>> policy.
>>
>> If they switch to an anon inode, then any mmap PROT_EXEC of the opened file
>> will trigger an EXECMEM check, at least as currently implemented, as we have
>> no useful backing inode information.
>
> Yep, and that's by design in the overall proposal. The trick is that
> ENCLAVE_ADD takes a source VMA and copies the contents *and* the
> permissions from the source VMA. The source VMA points at regular memory
> that was mapped and populated using existing mechanisms for loading DSOs.
>
> E.g. at a high level:
>
> source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
> for_each_chunk {
> <hand waving - mmap()/mprotect() the enclave file into regular memory>
> }
>
> enclave_fd = open("/dev/sgx/enclave", O_RDWR); /* allocs anon inode */
> enclave_addr = mmap(NULL, size, PROT_READ, MAP_SHARED, enclave_fd, 0);
>
> ioctl(enclave_fd, ENCLAVE_CREATE, {enclave_addr});
> for_each_chunk {
> struct sgx_enclave_add ioctlargs = {
> .offset = chunk.offset,
> .source = chunk.addr,
> .size = chunk.size,
> .type = chunk.type, /* SGX specific metadata */
> }
> ioctl(fd, ENCLAVE_ADD, &ioctlargs); /* modifies enclave's VMAs */
> }
> ioctl(fd, ENCLAVE_INIT, ...);
>
>
> Userspace never explicitly requests PROT_EXEC on enclave_fd, but SGX also
> ensures userspace isn't bypassing LSM policies by virtue of copying the
> permissions for EPC VMAs from regular VMAs that have already gone through
> LSM checks.
Is O_RDWR required for /dev/sgx/enclave or would O_RDONLY suffice? Do
you do anything other than ioctl() calls on it?
What's the advantage of allocating an anon inode in the above? At
present anon inodes are exempted from inode-based checking, thereby
losing the ability to perform SELinux ioctl whitelisting, unlike the
file-backed /dev/sgx/enclave inode.
How would SELinux (or other security modules) restrict the authorized
enclaves that can be loaded via this interface? Would the sgx driver
invoke a new LSM hook with the regular/source VMAs as parameters and
allow the security module to reject the ENCLAVE_ADD operation? That
could be just based on the vm_file (e.g. whitelist what enclave files
are permitted in general) or it could be based on both the process and
the vm_file (e.g. only allow specific enclaves to be loaded into
specific processes).
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-17 19:28 UTC (permalink / raw)
To: Stephen Smalley
Cc: Andy Lutomirski, Xing, Cedric, Andy Lutomirski, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jarkko Sakkinen, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <6d083885-1880-f33d-a54f-23518d56b714@tycho.nsa.gov>
On Fri, May 17, 2019 at 02:05:39PM -0400, Stephen Smalley wrote:
> On 5/17/19 1:12 PM, Andy Lutomirski wrote:
> >
> >How can that work? Unless the API changes fairly radically, users
> >fundamentally need to both write and execute the enclave. Some of it will
> >be written only from already executable pages, and some privilege should be
> >needed to execute any enclave page that was not loaded like this.
>
> I'm not sure what the API is. Let's say they do something like this:
>
> fd = open("/dev/sgx/enclave", O_RDONLY);
> addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
> stuff addr into ioctl args
> ioctl(fd, ENCLAVE_CREATE, &ioctlargs);
> ioctl(fd, ENCLAVE_ADD_PAGE, &ioctlargs);
> ioctl(fd, ENCLAVE_INIT, &ioctlargs);
That's rougly the flow, except that that all enclaves need to have RW and
X EPC pages.
> The important points are that they do not open /dev/sgx/enclave with write
> access (otherwise they will trigger FILE__WRITE at open time, and later
> encounter FILE__EXECUTE as well during mmap, thereby requiring both to be
> allowed to /dev/sgx/enclave), and that they do not request PROT_WRITE to the
> resulting mapping (otherwise they will trigger FILE__WRITE at mmap time).
> Then only FILE__READ and FILE__EXECUTE are required to /dev/sgx/enclave in
> policy.
>
> If they switch to an anon inode, then any mmap PROT_EXEC of the opened file
> will trigger an EXECMEM check, at least as currently implemented, as we have
> no useful backing inode information.
Yep, and that's by design in the overall proposal. The trick is that
ENCLAVE_ADD takes a source VMA and copies the contents *and* the
permissions from the source VMA. The source VMA points at regular memory
that was mapped and populated using existing mechanisms for loading DSOs.
E.g. at a high level:
source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
for_each_chunk {
<hand waving - mmap()/mprotect() the enclave file into regular memory>
}
enclave_fd = open("/dev/sgx/enclave", O_RDWR); /* allocs anon inode */
enclave_addr = mmap(NULL, size, PROT_READ, MAP_SHARED, enclave_fd, 0);
ioctl(enclave_fd, ENCLAVE_CREATE, {enclave_addr});
for_each_chunk {
struct sgx_enclave_add ioctlargs = {
.offset = chunk.offset,
.source = chunk.addr,
.size = chunk.size,
.type = chunk.type, /* SGX specific metadata */
}
ioctl(fd, ENCLAVE_ADD, &ioctlargs); /* modifies enclave's VMAs */
}
ioctl(fd, ENCLAVE_INIT, ...);
Userspace never explicitly requests PROT_EXEC on enclave_fd, but SGX also
ensures userspace isn't bypassing LSM policies by virtue of copying the
permissions for EPC VMAs from regular VMAs that have already gone through
LSM checks.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Stephen Smalley @ 2019-05-17 19:20 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Sean Christopherson, Xing, Cedric, Andy Lutomirski, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jarkko Sakkinen, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <6d083885-1880-f33d-a54f-23518d56b714@tycho.nsa.gov>
On 5/17/19 2:05 PM, Stephen Smalley wrote:
> On 5/17/19 1:12 PM, Andy Lutomirski wrote:
>>
>>
>>> On May 17, 2019, at 9:37 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>
>>>> On 5/17/19 12:20 PM, Stephen Smalley wrote:
>>>>> On 5/17/19 11:09 AM, Sean Christopherson wrote:
>>>>>> On Fri, May 17, 2019 at 09:53:06AM -0400, Stephen Smalley wrote:
>>>>>>> On 5/16/19 6:23 PM, Xing, Cedric wrote:
>>>>>>> I thought EXECMOD applied to files (and memory mappings backed by
>>>>>>> them) but
>>>>>>> I was probably wrong. It sounds like EXECMOD applies to the whole
>>>>>>> process so
>>>>>>> would allow all pages within a process's address space to be
>>>>>>> modified then
>>>>>>> executed, regardless the backing files. Am I correct this time?
>>>>>>
>>>>>> No, you were correct the first time I think; EXECMOD is used to
>>>>>> control
>>>>>> whether a process can make executable a private file mapping that has
>>>>>> previously been modified (e.g. text relocation); it is a special
>>>>>> case to
>>>>>> support text relocations without having to allow full EXECMEM
>>>>>> (i.e. execute
>>>>>> arbitrary memory).
>>>>>>
>>>>>> SELinux checks relevant to W^X include:
>>>>>>
>>>>>> - EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping
>>>>>> (regardless of
>>>>>> PROT_WRITE, since we know the content has to have been written at
>>>>>> some
>>>>>> point) or a private file mapping that is also PROT_WRITE.
>>>>>> - EXECMOD: mprotect PROT_EXEC a private file mapping that has been
>>>>>> previously modified, typically for text relocations,
>>>>>> - FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping,
>>>>>> - FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping.
>>>>>>
>>>>>> (ignoring EXECSTACK and EXECHEAP here since they aren't really
>>>>>> relevant to
>>>>>> this discussion)
>>>>>>
>>>>>> So if you want to ensure W^X, then you wouldn't allow EXECMEM for the
>>>>>> process, EXECMOD by the process to any file, and the combination
>>>>>> of both
>>>>>> FILE__WRITE and FILE__EXECUTE by the process to any file.
>>>>>>
>>>>>> If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't
>>>>>> using an
>>>>>> anonymous inode, then I would expect that only the FILE__WRITE and
>>>>>> FILE__EXECUTE checks are relevant.
>>>>>
>>>>> Yep, I was just typing this up in a different thread:
>>>>>
>>>>> I think we may want to change the SGX API to alloc an anon inode
>>>>> for each
>>>>> enclave instead of hanging every enclave off of the
>>>>> /dev/sgx/enclave inode.
>>>>> Because /dev/sgx/enclave is NOT private, SELinux's
>>>>> file_map_prot_check()
>>>>> will only require FILE__WRITE and FILE__EXECUTE to mprotect()
>>>>> enclave VMAs
>>>>> to RWX. Backing each enclave with an anon inode will make SELinux
>>>>> treat
>>>>> EPC memory like anonymous mappings, which is what we want (I
>>>>> think), e.g.
>>>>> making *any* EPC page executable will require PROCESS__EXECMEM (SGX is
>>>>> 64-bit only at this point, so SELinux will always have
>>>>> default_noexec).
>>>> I don't think we want to require EXECMEM (or equivalently both
>>>> FILE__WRITE and FILE__EXECUTE to /dev/sgx/enclave) for making any
>>>> EPC page executable, only if the page is also writable or previously
>>>> modified. The intent is to prevent arbitrary code execution without
>>>> EXECMEM (or FILE__WRITE|FILE__EXECUTE), while still allowing
>>>> enclaves to be created without EXECMEM as long as the EPC page
>>>> mapping is only ever mapped RX and its initial contents came from an
>>>> unmodified file mapping that was PROT_EXEC (and hence already
>>>> checked via FILE__EXECUTE).
>>>
>>> Also, just to be clear, there is nothing inherently better about
>>> checking EXECMEM instead of checking both FILE__WRITE and
>>> FILE__EXECUTE to the /dev/sgx/enclave inode, so I wouldn't switch to
>>> using anon inodes for that reason. Using anon inodes also
>>> unfortunately disables SELinux inode-based checking since we no
>>> longer have any useful inode information, so you'd lose out on
>>> SELinux ioctl whitelisting on those enclave inodes if that matters.
>>
>> How can that work? Unless the API changes fairly radically, users
>> fundamentally need to both write and execute the enclave. Some of it
>> will be written only from already executable pages, and some privilege
>> should be needed to execute any enclave page that was not loaded like
>> this.
>
> I'm not sure what the API is. Let's say they do something like this:
>
> fd = open("/dev/sgx/enclave", O_RDONLY);
> addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
> stuff addr into ioctl args
> ioctl(fd, ENCLAVE_CREATE, &ioctlargs);
> ioctl(fd, ENCLAVE_ADD_PAGE, &ioctlargs);
> ioctl(fd, ENCLAVE_INIT, &ioctlargs);
>
> The important points are that they do not open /dev/sgx/enclave with
> write access (otherwise they will trigger FILE__WRITE at open time, and
> later encounter FILE__EXECUTE as well during mmap, thereby requiring
> both to be allowed to /dev/sgx/enclave), and that they do not request
> PROT_WRITE to the resulting mapping (otherwise they will trigger
> FILE__WRITE at mmap time). Then only FILE__READ and FILE__EXECUTE are
> required to /dev/sgx/enclave in policy.
>
> If they switch to an anon inode, then any mmap PROT_EXEC of the opened
> file will trigger an EXECMEM check, at least as currently implemented,
> as we have no useful backing inode information.
FWIW, looking at the selftest for SGX in the patch series, they open
/dev/sgx/enclave O_RDWR (probably not necessary?) and mmap the open file
RWX. If that is necessary then I'd rather it show up as FILE__WRITE and
FILE__EXECUTE to /dev/sgx/enclave instead of EXECMEM, so that we can
allow the process the ability to perform that mmap without allowing it
to make other mappings WX. So staying with the single /dev/sgx/enclave
inode is better in that regard.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-17 18:53 UTC (permalink / raw)
To: Sean Christopherson
Cc: Linus Torvalds, Stephen Smalley, Xing, Cedric, Andy Lutomirski,
James Morris, Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jarkko Sakkinen, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <20190517182124.GF15006@linux.intel.com>
> On May 17, 2019, at 11:21 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>
>> On Fri, May 17, 2019 at 11:04:22AM -0700, Linus Torvalds wrote:
>> On Fri, May 17, 2019 at 10:55 AM Sean Christopherson
>> <sean.j.christopherson@intel.com> wrote:
>>>
>>> In this snippet, IS_PRIVATE() is true for anon inodes, false for
>>> /dev/sgx/enclave. Because EPC memory is always shared, SELinux will never
>>> check PROCESS__EXECMEM for mprotect() on/dev/sgx/enclave.
>>
>> Why _does_ the memory have to be shared? Shared mmap() is
>> fundamentally less secure than private mmap, since by definition it
>> means "oh, somebody else has access to it too and might modify it
>> under us".
>>
>> Why does the SGX logic care about things like that? Normal executables
>> are just private mappings of an underlying file, I'm not sure why the
>> SGX interface has to have that shared thing, and why the interface has
>> to have a device node in the first place when you have system calls
>> for setup anyway.
>>
>> So why don't the system calls just work on perfectly normal anonymous
>> mmap's? Why a device node, and why must it be shared to begin with?
>
> I agree that conceptually EPC is private memory, but because EPC is
> managed as a separate memory pool, SGX tags it VM_PFNMAP and manually
> inserts PFNs, i.e. EPC effectively it gets classified as IO memory.
>
> And vmf_insert_pfn_prot() doesn't like writable private IO mappings:
>
> BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags));
I don’t see how it could be anonymous even in principle. The kernel can’t *read* the memory — how could we possibly CoW it? And we can’t share an RO backing pages between two different enclaves because the CPU won’t let us — each EPC page belongs to a particular enclave. And fork()ing an enclave is right out.
So I agree that MAP_ANONYMOUS would be nice conceptually, but I don’t see how it would work.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-17 18:52 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, Stephen Smalley, Xing, Cedric, Andy Lutomirski,
James Morris, Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jarkko Sakkinen, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <CAHk-=wi6N1ckASALGDSydzj+YXwAUq26uVPZD0r2q4Mjwss7hw@mail.gmail.com>
On Fri, May 17, 2019 at 11:33:30AM -0700, Linus Torvalds wrote:
> On Fri, May 17, 2019 at 11:21 AM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > I agree that conceptually EPC is private memory, but because EPC is
> > managed as a separate memory pool, SGX tags it VM_PFNMAP and manually
> > inserts PFNs, i.e. EPC effectively it gets classified as IO memory.
> >
> > And vmf_insert_pfn_prot() doesn't like writable private IO mappings:
> >
> > BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags));
>
> Hmm. I haven't looked into why you want to do your own page insertion
> and not just "use existing pages", but I'm sure there's some reason.
Outside of the SGX subsystem, the kernel is unaware of EPC memory, e.g.
BIOS enumerates it as reserved memory in the e820 tables, or not at all.
On current hardware, EPC is backed by system memory, but it's protected
by a range registers (and other stuff) and can't be accessed directly
except when the CPU is in "enclave mode", i.e. executing an enclave in
CPL3. To execute an enclave it must first be built, and because EPC
memory can't be written outside of enclave mode, the only way to build
the enclave is via dedicated CPL0 ISA, e.g. ENCLS[EADD].
> It looks like the "shared vs private" inode part is a red herring,
> though. You might as well give each opener of the sgx node its own
> inode - and you probably should. Then you can keep track of the pages
> that have been added in the inode->i_mapping, and you could avoid the
> whole PFN thing entirely. I still am not a huge fan of the device node
> in the first place, but I guess it's just one more place where a
> system admin can then give (or deny) access to a kernel feature from
> users. I guess the kvm people do the same thing, for not necessarily
> any better reasons.
>
> With the PFNMAP model I guess the SGX memory ends up being unswappable
> - at least done the obvious way.
EPC memory is swappable in it's own terms, e.g. pages can be swapped
from EPC to system RAM and vice versa, but again moving pages in and out
of the EPC can only be done through dedicated CPL0 ISA. And there are
additional TLB flushing requirements, evicted pages need to be refcounted
against the enclave, evicted pages need an anchor in the EPC to ensure
freshness, etc...
Long story short, we decided to manage EPC in the SGX subsystem as a
separate memory pool rather than modify the kernel's MMU to teach it
how to deal with EPC.
> Again, the way I'd expect it to be done is as a shmem inode - that
> would I think be a better model. But I think that's a largely internal
> design decision, and the device node could just do that eventually
> (and the mmap could just map the populated shmem information into
> memory, no PFNMAP needed - the inode and the mapping could be
> "read-only" as far as the _user_ is concerned, but the i_mapping then
> gets populated by the ioctl's).
>
> I have not actually looked at any of the SGX patches, so maybe you're
> already doing something like that (although the PFNMAP comment makes
> me think not), and quite possibly there's some fundamental reason why
> you can't just use the shmem approach.
>
> So my high-level reaction here may be just the rantings of somebody
> who just isn't familiar with what you do. My "why not shmem and
> regular mmap" questions come from a 30000ft view without knowing any
> of the details.
>
> Linus
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Linus Torvalds @ 2019-05-17 18:33 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Stephen Smalley, Xing, Cedric, Andy Lutomirski,
James Morris, Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jarkko Sakkinen, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <20190517182124.GF15006@linux.intel.com>
On Fri, May 17, 2019 at 11:21 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> I agree that conceptually EPC is private memory, but because EPC is
> managed as a separate memory pool, SGX tags it VM_PFNMAP and manually
> inserts PFNs, i.e. EPC effectively it gets classified as IO memory.
>
> And vmf_insert_pfn_prot() doesn't like writable private IO mappings:
>
> BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags));
Hmm. I haven't looked into why you want to do your own page insertion
and not just "use existing pages", but I'm sure there's some reason.
It looks like the "shared vs private" inode part is a red herring,
though. You might as well give each opener of the sgx node its own
inode - and you probably should. Then you can keep track of the pages
that have been added in the inode->i_mapping, and you could avoid the
whole PFN thing entirely. I still am not a huge fan of the device node
in the first place, but I guess it's just one more place where a
system admin can then give (or deny) access to a kernel feature from
users. I guess the kvm people do the same thing, for not necessarily
any better reasons.
With the PFNMAP model I guess the SGX memory ends up being unswappable
- at least done the obvious way.
Again, the way I'd expect it to be done is as a shmem inode - that
would I think be a better model. But I think that's a largely internal
design decision, and the device node could just do that eventually
(and the mmap could just map the populated shmem information into
memory, no PFNMAP needed - the inode and the mapping could be
"read-only" as far as the _user_ is concerned, but the i_mapping then
gets populated by the ioctl's).
I have not actually looked at any of the SGX patches, so maybe you're
already doing something like that (although the PFNMAP comment makes
me think not), and quite possibly there's some fundamental reason why
you can't just use the shmem approach.
So my high-level reaction here may be just the rantings of somebody
who just isn't familiar with what you do. My "why not shmem and
regular mmap" questions come from a 30000ft view without knowing any
of the details.
Linus
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-17 18:21 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, Stephen Smalley, Xing, Cedric, Andy Lutomirski,
James Morris, Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jarkko Sakkinen, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <CAHk-=wgH2FBzBG3_RZSuatpYCj8DCQZipJYp9vh3Wy_S3Qt4-g@mail.gmail.com>
On Fri, May 17, 2019 at 11:04:22AM -0700, Linus Torvalds wrote:
> On Fri, May 17, 2019 at 10:55 AM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > In this snippet, IS_PRIVATE() is true for anon inodes, false for
> > /dev/sgx/enclave. Because EPC memory is always shared, SELinux will never
> > check PROCESS__EXECMEM for mprotect() on/dev/sgx/enclave.
>
> Why _does_ the memory have to be shared? Shared mmap() is
> fundamentally less secure than private mmap, since by definition it
> means "oh, somebody else has access to it too and might modify it
> under us".
>
> Why does the SGX logic care about things like that? Normal executables
> are just private mappings of an underlying file, I'm not sure why the
> SGX interface has to have that shared thing, and why the interface has
> to have a device node in the first place when you have system calls
> for setup anyway.
>
> So why don't the system calls just work on perfectly normal anonymous
> mmap's? Why a device node, and why must it be shared to begin with?
I agree that conceptually EPC is private memory, but because EPC is
managed as a separate memory pool, SGX tags it VM_PFNMAP and manually
inserts PFNs, i.e. EPC effectively it gets classified as IO memory.
And vmf_insert_pfn_prot() doesn't like writable private IO mappings:
BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags));
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Stephen Smalley @ 2019-05-17 18:16 UTC (permalink / raw)
To: Sean Christopherson
Cc: Xing, Cedric, Andy Lutomirski, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux@vger.kernel.org,
Jarkko Sakkinen, Jethro Beekman, Hansen, Dave, Thomas Gleixner,
Dr. Greg, Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <20190517175036.GD15006@linux.intel.com>
On 5/17/19 1:50 PM, Sean Christopherson wrote:
> On Fri, May 17, 2019 at 01:42:50PM -0400, Stephen Smalley wrote:
>> On 5/17/19 1:29 PM, Sean Christopherson wrote:
>>> AIUI, having FILE__WRITE and FILE__EXECUTE on /dev/sgx/enclave would allow
>>> *any* enclave/process to map EPC as RWX. Moving to anon inodes and thus
>>> PROCESS__EXECMEM achieves per-process granularity.
>>>
>>
>> No, FILE__WRITE and FILE__EXECUTE are a check between a process and a file,
>> so you can ensure that only whitelisted processes are allowed both to
>> /dev/sgx/enclave.
>
> Ah, so each process has its own FILE__* permissions for a specific set of
> files?
That's correct.
> Does that allow differentiating between a process making an EPC page RWX
> and a process making two separate EPC pages RW and RX?
Not if they are backed by the same inode, nor if they are all backed by
anon inodes, at least not as currently implemented.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Linus Torvalds @ 2019-05-17 18:04 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Stephen Smalley, Xing, Cedric, Andy Lutomirski,
James Morris, Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jarkko Sakkinen, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <20190517175500.GE15006@linux.intel.com>
On Fri, May 17, 2019 at 10:55 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> In this snippet, IS_PRIVATE() is true for anon inodes, false for
> /dev/sgx/enclave. Because EPC memory is always shared, SELinux will never
> check PROCESS__EXECMEM for mprotect() on/dev/sgx/enclave.
Why _does_ the memory have to be shared? Shared mmap() is
fundamentally less secure than private mmap, since by definition it
means "oh, somebody else has access to it too and might modify it
under us".
Why does the SGX logic care about things like that? Normal executables
are just private mappings of an underlying file, I'm not sure why the
SGX interface has to have that shared thing, and why the interface has
to have a device node in the first place when you have system calls
for setup anyway.
So why don't the system calls just work on perfectly normal anonymous
mmap's? Why a device node, and why must it be shared to begin with?
Linus
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Stephen Smalley @ 2019-05-17 18:05 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Sean Christopherson, Xing, Cedric, Andy Lutomirski, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jarkko Sakkinen, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <837CE33B-A636-4BF8-B46E-0A8A40C5A563@amacapital.net>
On 5/17/19 1:12 PM, Andy Lutomirski wrote:
>
>
>> On May 17, 2019, at 9:37 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>
>>> On 5/17/19 12:20 PM, Stephen Smalley wrote:
>>>> On 5/17/19 11:09 AM, Sean Christopherson wrote:
>>>>> On Fri, May 17, 2019 at 09:53:06AM -0400, Stephen Smalley wrote:
>>>>>> On 5/16/19 6:23 PM, Xing, Cedric wrote:
>>>>>> I thought EXECMOD applied to files (and memory mappings backed by them) but
>>>>>> I was probably wrong. It sounds like EXECMOD applies to the whole process so
>>>>>> would allow all pages within a process's address space to be modified then
>>>>>> executed, regardless the backing files. Am I correct this time?
>>>>>
>>>>> No, you were correct the first time I think; EXECMOD is used to control
>>>>> whether a process can make executable a private file mapping that has
>>>>> previously been modified (e.g. text relocation); it is a special case to
>>>>> support text relocations without having to allow full EXECMEM (i.e. execute
>>>>> arbitrary memory).
>>>>>
>>>>> SELinux checks relevant to W^X include:
>>>>>
>>>>> - EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping (regardless of
>>>>> PROT_WRITE, since we know the content has to have been written at some
>>>>> point) or a private file mapping that is also PROT_WRITE.
>>>>> - EXECMOD: mprotect PROT_EXEC a private file mapping that has been
>>>>> previously modified, typically for text relocations,
>>>>> - FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping,
>>>>> - FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping.
>>>>>
>>>>> (ignoring EXECSTACK and EXECHEAP here since they aren't really relevant to
>>>>> this discussion)
>>>>>
>>>>> So if you want to ensure W^X, then you wouldn't allow EXECMEM for the
>>>>> process, EXECMOD by the process to any file, and the combination of both
>>>>> FILE__WRITE and FILE__EXECUTE by the process to any file.
>>>>>
>>>>> If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't using an
>>>>> anonymous inode, then I would expect that only the FILE__WRITE and
>>>>> FILE__EXECUTE checks are relevant.
>>>>
>>>> Yep, I was just typing this up in a different thread:
>>>>
>>>> I think we may want to change the SGX API to alloc an anon inode for each
>>>> enclave instead of hanging every enclave off of the /dev/sgx/enclave inode.
>>>> Because /dev/sgx/enclave is NOT private, SELinux's file_map_prot_check()
>>>> will only require FILE__WRITE and FILE__EXECUTE to mprotect() enclave VMAs
>>>> to RWX. Backing each enclave with an anon inode will make SELinux treat
>>>> EPC memory like anonymous mappings, which is what we want (I think), e.g.
>>>> making *any* EPC page executable will require PROCESS__EXECMEM (SGX is
>>>> 64-bit only at this point, so SELinux will always have default_noexec).
>>> I don't think we want to require EXECMEM (or equivalently both FILE__WRITE and FILE__EXECUTE to /dev/sgx/enclave) for making any EPC page executable, only if the page is also writable or previously modified. The intent is to prevent arbitrary code execution without EXECMEM (or FILE__WRITE|FILE__EXECUTE), while still allowing enclaves to be created without EXECMEM as long as the EPC page mapping is only ever mapped RX and its initial contents came from an unmodified file mapping that was PROT_EXEC (and hence already checked via FILE__EXECUTE).
>>
>> Also, just to be clear, there is nothing inherently better about checking EXECMEM instead of checking both FILE__WRITE and FILE__EXECUTE to the /dev/sgx/enclave inode, so I wouldn't switch to using anon inodes for that reason. Using anon inodes also unfortunately disables SELinux inode-based checking since we no longer have any useful inode information, so you'd lose out on SELinux ioctl whitelisting on those enclave inodes if that matters.
>
> How can that work? Unless the API changes fairly radically, users fundamentally need to both write and execute the enclave. Some of it will be written only from already executable pages, and some privilege should be needed to execute any enclave page that was not loaded like this.
I'm not sure what the API is. Let's say they do something like this:
fd = open("/dev/sgx/enclave", O_RDONLY);
addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
stuff addr into ioctl args
ioctl(fd, ENCLAVE_CREATE, &ioctlargs);
ioctl(fd, ENCLAVE_ADD_PAGE, &ioctlargs);
ioctl(fd, ENCLAVE_INIT, &ioctlargs);
The important points are that they do not open /dev/sgx/enclave with
write access (otherwise they will trigger FILE__WRITE at open time, and
later encounter FILE__EXECUTE as well during mmap, thereby requiring
both to be allowed to /dev/sgx/enclave), and that they do not request
PROT_WRITE to the resulting mapping (otherwise they will trigger
FILE__WRITE at mmap time). Then only FILE__READ and FILE__EXECUTE are
required to /dev/sgx/enclave in policy.
If they switch to an anon inode, then any mmap PROT_EXEC of the opened
file will trigger an EXECMEM check, at least as currently implemented,
as we have no useful backing inode information.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-17 17:55 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stephen Smalley, Xing, Cedric, Andy Lutomirski, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jarkko Sakkinen, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <DFE03E0C-694A-4289-B416-29CDC2644F94@amacapital.net>
On Fri, May 17, 2019 at 10:43:01AM -0700, Andy Lutomirski wrote:
>
> > On May 17, 2019, at 10:29 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> >
> > AIUI, having FILE__WRITE and FILE__EXECUTE on /dev/sgx/enclave would allow
> > *any* enclave/process to map EPC as RWX. Moving to anon inodes and thus
> > PROCESS__EXECMEM achieves per-process granularity.
>
> How does anon_inode make any difference? Anon_inode is not the same thing as
> anon_vma.
In this snippet, IS_PRIVATE() is true for anon inodes, false for
/dev/sgx/enclave. Because EPC memory is always shared, SELinux will never
check PROCESS__EXECMEM for mprotect() on/dev/sgx/enclave.
static int file_map_prot_check(struct file *file, unsigned long prot, int shared)
{
const struct cred *cred = current_cred();
u32 sid = cred_sid(cred);
int rc = 0;
if (default_noexec &&
(prot & PROT_EXEC) && (!file || IS_PRIVATE(file_inode(file)) ||
(!shared && (prot & PROT_WRITE)))) {
/*
* We are making executable an anonymous mapping or a
* private file mapping that will also be writable.
* This has an additional check.
*/
rc = avc_has_perm(&selinux_state,
sid, sid, SECCLASS_PROCESS,
PROCESS__EXECMEM, NULL);
if (rc)
goto error;
}
...
}
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-17 17:50 UTC (permalink / raw)
To: Stephen Smalley
Cc: Xing, Cedric, Andy Lutomirski, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux@vger.kernel.org,
Jarkko Sakkinen, Jethro Beekman, Hansen, Dave, Thomas Gleixner,
Dr. Greg, Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <7de94229-f223-64bd-de11-7a601ec26938@tycho.nsa.gov>
On Fri, May 17, 2019 at 01:42:50PM -0400, Stephen Smalley wrote:
> On 5/17/19 1:29 PM, Sean Christopherson wrote:
> >AIUI, having FILE__WRITE and FILE__EXECUTE on /dev/sgx/enclave would allow
> >*any* enclave/process to map EPC as RWX. Moving to anon inodes and thus
> >PROCESS__EXECMEM achieves per-process granularity.
> >
>
> No, FILE__WRITE and FILE__EXECUTE are a check between a process and a file,
> so you can ensure that only whitelisted processes are allowed both to
> /dev/sgx/enclave.
Ah, so each process has its own FILE__* permissions for a specific set of
files?
Does that allow differentiating between a process making an EPC page RWX
and a process making two separate EPC pages RW and RX?
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Stephen Smalley @ 2019-05-17 17:42 UTC (permalink / raw)
To: Sean Christopherson
Cc: Xing, Cedric, Andy Lutomirski, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux@vger.kernel.org,
Jarkko Sakkinen, Jethro Beekman, Hansen, Dave, Thomas Gleixner,
Dr. Greg, Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <20190517172953.GC15006@linux.intel.com>
On 5/17/19 1:29 PM, Sean Christopherson wrote:
> On Fri, May 17, 2019 at 12:37:40PM -0400, Stephen Smalley wrote:
>> On 5/17/19 12:20 PM, Stephen Smalley wrote:
>>> On 5/17/19 11:09 AM, Sean Christopherson wrote:
>>>> I think we may want to change the SGX API to alloc an anon inode for each
>>>> enclave instead of hanging every enclave off of the /dev/sgx/enclave
>>>> inode.
>>>> Because /dev/sgx/enclave is NOT private, SELinux's file_map_prot_check()
>>>> will only require FILE__WRITE and FILE__EXECUTE to mprotect() enclave
>>>> VMAs
>>>> to RWX. Backing each enclave with an anon inode will make SELinux treat
>>>> EPC memory like anonymous mappings, which is what we want (I think), e.g.
>>>> making *any* EPC page executable will require PROCESS__EXECMEM (SGX is
>>>> 64-bit only at this point, so SELinux will always have default_noexec).
>>>
>>> I don't think we want to require EXECMEM (or equivalently both FILE__WRITE
>>> and FILE__EXECUTE to /dev/sgx/enclave) for making any EPC page executable,
>>> only if the page is also writable or previously modified. The intent is
>>> to prevent arbitrary code execution without EXECMEM (or
>>> FILE__WRITE|FILE__EXECUTE), while still allowing enclaves to be created
>>> without EXECMEM as long as the EPC page mapping is only ever mapped RX and
>>> its initial contents came from an unmodified file mapping that was
>>> PROT_EXEC (and hence already checked via FILE__EXECUTE).
>
> The idea is that by providing an SGX ioctl() to propagate VMA permissions
> from a source VMA, EXECMEM wouldn't be required to make an EPC page
> executable. E.g. userspace establishes an enclave in non-EPC memory from
> an unmodified file (with FILE__EXECUTE perms), and the uses the SGX ioctl()
> to copy the contents and permissions into EPC memory.
>
>> Also, just to be clear, there is nothing inherently better about checking
>> EXECMEM instead of checking both FILE__WRITE and FILE__EXECUTE to the
>> /dev/sgx/enclave inode, so I wouldn't switch to using anon inodes for that
>> reason. Using anon inodes also unfortunately disables SELinux inode-based
>> checking since we no longer have any useful inode information, so you'd lose
>> out on SELinux ioctl whitelisting on those enclave inodes if that matters.
>
> The problem is that all enclaves are associated with a single inode, i.e.
> /dev/sgx/enclave. /dev/sgx/enclave is a char device whose purpose is to
> provide ioctls() and to allow mmap()'ing EPC memory. In no way is it
> associated with the content that actually gets loaded into EPC memory.
>
> The actual file that contains the enclave's contents (assuming the enclave
> came from a file) is a separate regular file that the SGX subsystem never
> sees.
>
> AIUI, having FILE__WRITE and FILE__EXECUTE on /dev/sgx/enclave would allow
> *any* enclave/process to map EPC as RWX. Moving to anon inodes and thus
> PROCESS__EXECMEM achieves per-process granularity.
>
No, FILE__WRITE and FILE__EXECUTE are a check between a process and a
file, so you can ensure that only whitelisted processes are allowed both
to /dev/sgx/enclave.
^ 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