[PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[James Bottomley]

From: James Bottomley <JBottomley@Parallels.com>

The object here is to make the NV+BS variables accessible (at least read only)
at runtime so we can get a full picture of the state of the EFI variables for
debugging and secure boot purposes.

The way this is done is to get the efi stub to pull all the NV+BS
(i.e. variables without the RT flag) out into setup_data records which can
then be read back in at runtime.  the EFI calls get_next_variable() and
get_variable() are modified to run through these setup records when the real
calls fail.

Signed-off-by: James Bottomley <JBottomley@Parallels.com>

---

This same mechanism should work for all architectures, but the method of
transferring information from boot to runtime differs, so it can't be
done generically

James

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index c205035..6bbbb2e 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -251,6 +251,85 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size)
 	*size = len;
 }
 
+static efi_status_t setup_efi_bootvars(struct boot_params *params)
+{
+	const int len = 1024;
+	efi_char16_t *str;
+	efi_status_t status;
+	struct setup_data **data = (struct setup_data **)&params->hdr.setup_data;
+
+	status = efi_call_phys3(sys_table->boottime->allocate_pool,
+				EFI_LOADER_DATA, len, &str);
+	if (status != EFI_SUCCESS)
+		return status;
+
+	/*
+	 * Note: this trick only works on Little Endian if hdr.setup_data
+	 * is u64 on both 64 and 32 bit
+	 */
+	while (*data)
+		data = (struct setup_data **)&(*data)->next;
+
+	memset(str, 0, len);
+
+	for (;;) {
+		unsigned long size = len, str_len;
+		efi_guid_t guid;
+		struct efi_setup_bootvars *bvs;
+		unsigned int attributes;
+
+		status = efi_call_phys3(sys_table->runtime->get_next_variable,
+					&size, str, &guid);
+		if (status != EFI_SUCCESS)
+			break;
+
+		str_len = size;
+		size = 0;
+		status = efi_call_phys5(sys_table->runtime->get_variable,
+					str, &guid, &attributes, &size, NULL);
+		if (status != EFI_SUCCESS && status != EFI_BUFFER_TOO_SMALL)
+			break;
+
+
+		/*
+		 * Please don't be tempted to optimise here: some
+		 * UEFI implementations fail to set attributes if
+		 * they return any error (including EFI_BUFFER_TOO_SMALL)
+		 */
+		status = efi_call_phys3(sys_table->boottime->allocate_pool,
+					EFI_LOADER_DATA,
+					size + sizeof(*bvs) + str_len,
+					&bvs);
+		if (status != EFI_SUCCESS)
+			continue;
+		memset(bvs, 0 , sizeof(bvs));
+		status = efi_call_phys5(sys_table->runtime->get_variable,
+					str, &guid, &attributes,
+					&size, bvs->data);
+
+		if (status != EFI_SUCCESS
+		    || (attributes & 0x07) != (EFI_VARIABLE_BOOTSERVICE_ACCESS |
+					       EFI_VARIABLE_NON_VOLATILE)) {
+			efi_call_phys1(sys_table->boottime->free_pool, bvs);
+			continue;
+		}
+		memcpy(bvs->data + size, str, str_len);
+		bvs->name_size = str_len;
+		bvs->s_d.type = SETUP_EFIVAR;
+		bvs->s_d.len = size + sizeof(*bvs) + str_len;
+		bvs->guid = guid;
+		bvs->size = size;
+		bvs->attributes = attributes;
+		*data = (struct setup_data *)bvs;
+		data = (struct setup_data **)&(*data)->next;
+	}
+	*data = NULL;
+	status = EFI_SUCCESS;
+ out:
+	efi_call_phys1(sys_table->boottime->free_pool, str);
+	return status;
+}
+
 static efi_status_t setup_efi_pci(struct boot_params *params)
 {
 	efi_pci_io_protocol *pci;
@@ -1159,6 +1238,8 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table,
 
 	setup_efi_pci(boot_params);
 
+	setup_efi_bootvars(boot_params);
+
 	status = efi_call_phys3(sys_table->boottime->allocate_pool,
 				EFI_LOADER_DATA, sizeof(*gdt),
 				(void **)&gdt);
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 60c89f3..2dadecd 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -122,4 +122,13 @@ static inline bool efi_is_native(void)
 #define efi_call6(_f, _a1, _a2, _a3, _a4, _a5, _a6)	(-ENOSYS)
 #endif /* CONFIG_EFI */
 
+struct efi_setup_bootvars {
+	struct setup_data s_d;
+	unsigned int attributes;
+	efi_guid_t guid;
+	unsigned long name_size;
+	unsigned long size;	/* size of the variable data (name follows) */
+	unsigned char data[0];
+};
+
 #endif /* _ASM_X86_EFI_H */
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index c15ddaf..e864d23 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -6,6 +6,7 @@
 #define SETUP_E820_EXT			1
 #define SETUP_DTB			2
 #define SETUP_PCI			3
+#define SETUP_EFIVAR			4
 
 /* ram_size flags */
 #define RAMDISK_IMAGE_START_MASK	0x07FF
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index fff986d..931060a 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -153,17 +153,103 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name,
 					  unsigned long *data_size,
 					  void *data)
 {
-	return efi_call_virt5(get_variable,
-			      name, vendor, attr,
-			      data_size, data);
+	efi_status_t status = efi_call_virt5(get_variable,
+					     name, vendor, attr,
+					     data_size, data);
+	u64 pa_data;
+	struct setup_data *s_d;
+
+	if (status != EFI_NOT_FOUND)
+		return status;
+
+
+	for (pa_data = boot_params.hdr.setup_data; pa_data;
+	     pa_data = s_d->next) {
+		struct efi_setup_bootvars *bvs;
+		efi_char16_t *bvs_name;
+		int i = 0;
+
+		s_d = phys_to_virt(pa_data);
+
+		if (s_d->type != SETUP_EFIVAR)
+			continue;
+
+		bvs = (struct efi_setup_bootvars *)s_d;
+		bvs_name = (efi_char16_t *)(bvs->data + bvs->size);
+
+		for (i = 0; bvs_name[i] != 0 && name[i] != 0; i++)
+			if (bvs_name[i] != name[i])
+				break;
+
+		if (bvs_name[i] != 0 || name[i] != 0)
+			continue;
+
+		if (memcmp(&bvs->guid, vendor, sizeof(*vendor)) != 0)
+			continue;
+
+		if (attr)
+			*attr = bvs->attributes;
+
+		if (*data_size < bvs->size) {
+			*data_size = bvs->size;
+			return EFI_BUFFER_TOO_SMALL;
+		}
+		*data_size = bvs->size;
+		memcpy(data, bvs->data, bvs->size);
+
+		return EFI_SUCCESS;
+	}
+	return EFI_NOT_FOUND;
 }
 
 static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
 					       efi_char16_t *name,
 					       efi_guid_t *vendor)
 {
-	return efi_call_virt3(get_next_variable,
-			      name_size, name, vendor);
+	static int use_bootvars = 0;
+	efi_status_t status;
+	u64 pa_data = boot_params.hdr.setup_data;
+	int found = 0;
+	struct setup_data *data;
+
+	if (!use_bootvars) {
+		status = efi_call_virt3(get_next_variable,
+					name_size, name, vendor);
+		if (status == EFI_NOT_FOUND)
+			use_bootvars = 1;
+		else
+			return status;
+	}
+
+	for (pa_data = boot_params.hdr.setup_data; pa_data;
+	     pa_data = data->next) {
+		struct efi_setup_bootvars *bvs;
+		efi_char16_t *bvs_name;
+
+		data = phys_to_virt(pa_data);
+
+		if (data->type != SETUP_EFIVAR)
+			continue;
+
+		bvs = (struct efi_setup_bootvars *)data;
+		bvs_name = (efi_char16_t *)(bvs->data + bvs->size);
+
+		if (use_bootvars != 1 && !found) {
+			unsigned long size = min(bvs->name_size, *name_size);
+
+			if (memcmp(bvs_name, name, size) == 0)
+				found = 1;
+		} else {
+			memcpy(name, bvs_name, bvs->name_size);
+			*name_size = bvs->name_size;
+			*vendor = bvs->guid;
+			use_bootvars = 2;
+			return EFI_SUCCESS;
+		}
+	}
+	use_bootvars = 0;
+
+	return EFI_NOT_FOUND;
 }
 
 static efi_status_t virt_efi_set_variable(efi_char16_t *name,

[PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[James Bottomley]

From: James Bottomley <JBottomley-MU7nAjRaF3makBO8gow8eQ@public.gmane.org>

The object here is to make the NV+BS variables accessible (at least read only)
at runtime so we can get a full picture of the state of the EFI variables for
debugging and secure boot purposes.

The way this is done is to get the efi stub to pull all the NV+BS
(i.e. variables without the RT flag) out into setup_data records which can
then be read back in at runtime.  the EFI calls get_next_variable() and
get_variable() are modified to run through these setup records when the real
calls fail.

Signed-off-by: James Bottomley <JBottomley-MU7nAjRaF3makBO8gow8eQ@public.gmane.org>

---

This same mechanism should work for all architectures, but the method of
transferring information from boot to runtime differs, so it can't be
done generically

James

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index c205035..6bbbb2e 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -251,6 +251,85 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size)
 	*size = len;
 }
 
+static efi_status_t setup_efi_bootvars(struct boot_params *params)
+{
+	const int len = 1024;
+	efi_char16_t *str;
+	efi_status_t status;
+	struct setup_data **data = (struct setup_data **)&params->hdr.setup_data;
+
+	status = efi_call_phys3(sys_table->boottime->allocate_pool,
+				EFI_LOADER_DATA, len, &str);
+	if (status != EFI_SUCCESS)
+		return status;
+
+	/*
+	 * Note: this trick only works on Little Endian if hdr.setup_data
+	 * is u64 on both 64 and 32 bit
+	 */
+	while (*data)
+		data = (struct setup_data **)&(*data)->next;
+
+	memset(str, 0, len);
+
+	for (;;) {
+		unsigned long size = len, str_len;
+		efi_guid_t guid;
+		struct efi_setup_bootvars *bvs;
+		unsigned int attributes;
+
+		status = efi_call_phys3(sys_table->runtime->get_next_variable,
+					&size, str, &guid);
+		if (status != EFI_SUCCESS)
+			break;
+
+		str_len = size;
+		size = 0;
+		status = efi_call_phys5(sys_table->runtime->get_variable,
+					str, &guid, &attributes, &size, NULL);
+		if (status != EFI_SUCCESS && status != EFI_BUFFER_TOO_SMALL)
+			break;
+
+
+		/*
+		 * Please don't be tempted to optimise here: some
+		 * UEFI implementations fail to set attributes if
+		 * they return any error (including EFI_BUFFER_TOO_SMALL)
+		 */
+		status = efi_call_phys3(sys_table->boottime->allocate_pool,
+					EFI_LOADER_DATA,
+					size + sizeof(*bvs) + str_len,
+					&bvs);
+		if (status != EFI_SUCCESS)
+			continue;
+		memset(bvs, 0 , sizeof(bvs));
+		status = efi_call_phys5(sys_table->runtime->get_variable,
+					str, &guid, &attributes,
+					&size, bvs->data);
+
+		if (status != EFI_SUCCESS
+		    || (attributes & 0x07) != (EFI_VARIABLE_BOOTSERVICE_ACCESS |
+					       EFI_VARIABLE_NON_VOLATILE)) {
+			efi_call_phys1(sys_table->boottime->free_pool, bvs);
+			continue;
+		}
+		memcpy(bvs->data + size, str, str_len);
+		bvs->name_size = str_len;
+		bvs->s_d.type = SETUP_EFIVAR;
+		bvs->s_d.len = size + sizeof(*bvs) + str_len;
+		bvs->guid = guid;
+		bvs->size = size;
+		bvs->attributes = attributes;
+		*data = (struct setup_data *)bvs;
+		data = (struct setup_data **)&(*data)->next;
+	}
+	*data = NULL;
+	status = EFI_SUCCESS;
+ out:
+	efi_call_phys1(sys_table->boottime->free_pool, str);
+	return status;
+}
+
 static efi_status_t setup_efi_pci(struct boot_params *params)
 {
 	efi_pci_io_protocol *pci;
@@ -1159,6 +1238,8 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table,
 
 	setup_efi_pci(boot_params);
 
+	setup_efi_bootvars(boot_params);
+
 	status = efi_call_phys3(sys_table->boottime->allocate_pool,
 				EFI_LOADER_DATA, sizeof(*gdt),
 				(void **)&gdt);
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 60c89f3..2dadecd 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -122,4 +122,13 @@ static inline bool efi_is_native(void)
 #define efi_call6(_f, _a1, _a2, _a3, _a4, _a5, _a6)	(-ENOSYS)
 #endif /* CONFIG_EFI */
 
+struct efi_setup_bootvars {
+	struct setup_data s_d;
+	unsigned int attributes;
+	efi_guid_t guid;
+	unsigned long name_size;
+	unsigned long size;	/* size of the variable data (name follows) */
+	unsigned char data[0];
+};
+
 #endif /* _ASM_X86_EFI_H */
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index c15ddaf..e864d23 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -6,6 +6,7 @@
 #define SETUP_E820_EXT			1
 #define SETUP_DTB			2
 #define SETUP_PCI			3
+#define SETUP_EFIVAR			4
 
 /* ram_size flags */
 #define RAMDISK_IMAGE_START_MASK	0x07FF
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index fff986d..931060a 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -153,17 +153,103 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name,
 					  unsigned long *data_size,
 					  void *data)
 {
-	return efi_call_virt5(get_variable,
-			      name, vendor, attr,
-			      data_size, data);
+	efi_status_t status = efi_call_virt5(get_variable,
+					     name, vendor, attr,
+					     data_size, data);
+	u64 pa_data;
+	struct setup_data *s_d;
+
+	if (status != EFI_NOT_FOUND)
+		return status;
+
+
+	for (pa_data = boot_params.hdr.setup_data; pa_data;
+	     pa_data = s_d->next) {
+		struct efi_setup_bootvars *bvs;
+		efi_char16_t *bvs_name;
+		int i = 0;
+
+		s_d = phys_to_virt(pa_data);
+
+		if (s_d->type != SETUP_EFIVAR)
+			continue;
+
+		bvs = (struct efi_setup_bootvars *)s_d;
+		bvs_name = (efi_char16_t *)(bvs->data + bvs->size);
+
+		for (i = 0; bvs_name[i] != 0 && name[i] != 0; i++)
+			if (bvs_name[i] != name[i])
+				break;
+
+		if (bvs_name[i] != 0 || name[i] != 0)
+			continue;
+
+		if (memcmp(&bvs->guid, vendor, sizeof(*vendor)) != 0)
+			continue;
+
+		if (attr)
+			*attr = bvs->attributes;
+
+		if (*data_size < bvs->size) {
+			*data_size = bvs->size;
+			return EFI_BUFFER_TOO_SMALL;
+		}
+		*data_size = bvs->size;
+		memcpy(data, bvs->data, bvs->size);
+
+		return EFI_SUCCESS;
+	}
+	return EFI_NOT_FOUND;
 }
 
 static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
 					       efi_char16_t *name,
 					       efi_guid_t *vendor)
 {
-	return efi_call_virt3(get_next_variable,
-			      name_size, name, vendor);
+	static int use_bootvars = 0;
+	efi_status_t status;
+	u64 pa_data = boot_params.hdr.setup_data;
+	int found = 0;
+	struct setup_data *data;
+
+	if (!use_bootvars) {
+		status = efi_call_virt3(get_next_variable,
+					name_size, name, vendor);
+		if (status == EFI_NOT_FOUND)
+			use_bootvars = 1;
+		else
+			return status;
+	}
+
+	for (pa_data = boot_params.hdr.setup_data; pa_data;
+	     pa_data = data->next) {
+		struct efi_setup_bootvars *bvs;
+		efi_char16_t *bvs_name;
+
+		data = phys_to_virt(pa_data);
+
+		if (data->type != SETUP_EFIVAR)
+			continue;
+
+		bvs = (struct efi_setup_bootvars *)data;
+		bvs_name = (efi_char16_t *)(bvs->data + bvs->size);
+
+		if (use_bootvars != 1 && !found) {
+			unsigned long size = min(bvs->name_size, *name_size);
+
+			if (memcmp(bvs_name, name, size) == 0)
+				found = 1;
+		} else {
+			memcpy(name, bvs_name, bvs->name_size);
+			*name_size = bvs->name_size;
+			*vendor = bvs->guid;
+			use_bootvars = 2;
+			return EFI_SUCCESS;
+		}
+	}
+	use_bootvars = 0;
+
+	return EFI_NOT_FOUND;
 }
 
 static efi_status_t virt_efi_set_variable(efi_char16_t *name,

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[Matthew Garrett]

On Mon, Mar 18, 2013 at 08:40:14AM +0000, James Bottomley wrote:

> The object here is to make the NV+BS variables accessible (at least read only)
> at runtime so we can get a full picture of the state of the EFI variables for
> debugging and secure boot purposes.

I'd really prefer not to do this - the reason these aren't flagged as RT 
is that they're not supposed to be visible at runtime and may break 
certain security assumptions. If there's a real development purpose to 
this then it ought to be guarded as a config option.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[Matthew Garrett]

On Mon, Mar 18, 2013 at 08:40:14AM +0000, James Bottomley wrote:

> The object here is to make the NV+BS variables accessible (at least read only)
> at runtime so we can get a full picture of the state of the EFI variables for
> debugging and secure boot purposes.

I'd really prefer not to do this - the reason these aren't flagged as RT 
is that they're not supposed to be visible at runtime and may break 
certain security assumptions. If there's a real development purpose to 
this then it ought to be guarded as a config option.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[James Bottomley]

On Tue, 2013-03-19 at 01:48 +0000, Matthew Garrett wrote:
> On Mon, Mar 18, 2013 at 08:40:14AM +0000, James Bottomley wrote:
> 
> > The object here is to make the NV+BS variables accessible (at least read only)
> > at runtime so we can get a full picture of the state of the EFI variables for
> > debugging and secure boot purposes.
> 
> I'd really prefer not to do this - the reason these aren't flagged as RT 
> is that they're not supposed to be visible at runtime and may break 
> certain security assumptions.

I suppose they could be flagged as read only by root, but I haven't seen
anything non-innocuous in them yet.  It's mostly shell redirects and a
bit more useful information about the secure boot configuration.

Any security assumptions that rely on inability to read certain
information aren't really going to be that secure.  Inability to modify,
sure, but inability to read, not really.

>  If there's a real development purpose to 
> this then it ought to be guarded as a config option.

I've no objection to them going under the secure boot lockdown config
option if that's what you're thinking.

James

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[James Bottomley]

On Tue, 2013-03-19 at 01:48 +0000, Matthew Garrett wrote:
> On Mon, Mar 18, 2013 at 08:40:14AM +0000, James Bottomley wrote:
> 
> > The object here is to make the NV+BS variables accessible (at least read only)
> > at runtime so we can get a full picture of the state of the EFI variables for
> > debugging and secure boot purposes.
> 
> I'd really prefer not to do this - the reason these aren't flagged as RT 
> is that they're not supposed to be visible at runtime and may break 
> certain security assumptions.

I suppose they could be flagged as read only by root, but I haven't seen
anything non-innocuous in them yet.  It's mostly shell redirects and a
bit more useful information about the secure boot configuration.

Any security assumptions that rely on inability to read certain
information aren't really going to be that secure.  Inability to modify,
sure, but inability to read, not really.

>  If there's a real development purpose to 
> this then it ought to be guarded as a config option.

I've no objection to them going under the secure boot lockdown config
option if that's what you're thinking.

James

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[Matthew Garrett]

On Tue, Mar 19, 2013 at 08:14:45AM +0000, James Bottomley wrote:

> Any security assumptions that rely on inability to read certain
> information aren't really going to be that secure.  Inability to modify,
> sure, but inability to read, not really.

Well, I guess that's public/private key cryptography screwed.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[Matthew Garrett]

On Tue, Mar 19, 2013 at 08:14:45AM +0000, James Bottomley wrote:

> Any security assumptions that rely on inability to read certain
> information aren't really going to be that secure.  Inability to modify,
> sure, but inability to read, not really.

Well, I guess that's public/private key cryptography screwed.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[James Bottomley]

On Tue, 2013-03-19 at 16:35 +0000, Matthew Garrett wrote:
> On Tue, Mar 19, 2013 at 08:14:45AM +0000, James Bottomley wrote:
> 
> > Any security assumptions that rely on inability to read certain
> > information aren't really going to be that secure.  Inability to modify,
> > sure, but inability to read, not really.
> 
> Well, I guess that's public/private key cryptography screwed.

Well, OK, it's ex-BIOS writers we're dealing with, so I won't say no-one
would be stupid enough to come up with a security scheme embedding
Private Keys in BS+NV variables, but I would have thought the fact that
Linux would blow the lid off it might be a good incentive not to do it
and thus a plus point for this patch.

James

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[Matthew Garrett]

On Tue, Mar 19, 2013 at 05:17:27PM +0000, James Bottomley wrote:
> On Tue, 2013-03-19 at 16:35 +0000, Matthew Garrett wrote:
> > On Tue, Mar 19, 2013 at 08:14:45AM +0000, James Bottomley wrote:
> > 
> > > Any security assumptions that rely on inability to read certain
> > > information aren't really going to be that secure.  Inability to modify,
> > > sure, but inability to read, not really.
> > 
> > Well, I guess that's public/private key cryptography screwed.
> 
> Well, OK, it's ex-BIOS writers we're dealing with, so I won't say no-one
> would be stupid enough to come up with a security scheme embedding
> Private Keys in BS+NV variables, but I would have thought the fact that
> Linux would blow the lid off it might be a good incentive not to do it
> and thus a plus point for this patch.

The hibernation scheme we'd discussed involved having the first stage 
loader generating a keypair and handing half of it to the OS for 
encryption of the hibernation partition, then handing the other half to 
the OS on the next boot so it can decrypt it. That requires non-RT 
variables to be restricted from OS visibility.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[Matthew Garrett]

On Tue, Mar 19, 2013 at 05:17:27PM +0000, James Bottomley wrote:
> On Tue, 2013-03-19 at 16:35 +0000, Matthew Garrett wrote:
> > On Tue, Mar 19, 2013 at 08:14:45AM +0000, James Bottomley wrote:
> > 
> > > Any security assumptions that rely on inability to read certain
> > > information aren't really going to be that secure.  Inability to modify,
> > > sure, but inability to read, not really.
> > 
> > Well, I guess that's public/private key cryptography screwed.
> 
> Well, OK, it's ex-BIOS writers we're dealing with, so I won't say no-one
> would be stupid enough to come up with a security scheme embedding
> Private Keys in BS+NV variables, but I would have thought the fact that
> Linux would blow the lid off it might be a good incentive not to do it
> and thus a plus point for this patch.

The hibernation scheme we'd discussed involved having the first stage 
loader generating a keypair and handing half of it to the OS for 
encryption of the hibernation partition, then handing the other half to 
the OS on the next boot so it can decrypt it. That requires non-RT 
variables to be restricted from OS visibility.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[James Bottomley]

On Tue, 2013-03-19 at 17:25 +0000, Matthew Garrett wrote:
> On Tue, Mar 19, 2013 at 05:17:27PM +0000, James Bottomley wrote:
> > On Tue, 2013-03-19 at 16:35 +0000, Matthew Garrett wrote:
> > > On Tue, Mar 19, 2013 at 08:14:45AM +0000, James Bottomley wrote:
> > > 
> > > > Any security assumptions that rely on inability to read certain
> > > > information aren't really going to be that secure.  Inability to modify,
> > > > sure, but inability to read, not really.
> > > 
> > > Well, I guess that's public/private key cryptography screwed.
> > 
> > Well, OK, it's ex-BIOS writers we're dealing with, so I won't say no-one
> > would be stupid enough to come up with a security scheme embedding
> > Private Keys in BS+NV variables, but I would have thought the fact that
> > Linux would blow the lid off it might be a good incentive not to do it
> > and thus a plus point for this patch.
> 
> The hibernation scheme we'd discussed involved having the first stage 
> loader generating a keypair and handing half of it to the OS for 
> encryption of the hibernation partition, then handing the other half to 
> the OS on the next boot so it can decrypt it. That requires non-RT 
> variables to be restricted from OS visibility.

The scheme we discussed, unless something radically changed, was to
convey a temporary key pair via a mechanism to later verify the
hybernate kernel on a resume.  That only requires reboot safe knowledge
of the public key.  The private key can be conveyed in BS only (not NV),
and should be consumed (as in deleted) by the OS when it receives it, so
it wouldn't be exposed by this patch.

James

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[James Bottomley]

On Tue, 2013-03-19 at 17:25 +0000, Matthew Garrett wrote:
> On Tue, Mar 19, 2013 at 05:17:27PM +0000, James Bottomley wrote:
> > On Tue, 2013-03-19 at 16:35 +0000, Matthew Garrett wrote:
> > > On Tue, Mar 19, 2013 at 08:14:45AM +0000, James Bottomley wrote:
> > > 
> > > > Any security assumptions that rely on inability to read certain
> > > > information aren't really going to be that secure.  Inability to modify,
> > > > sure, but inability to read, not really.
> > > 
> > > Well, I guess that's public/private key cryptography screwed.
> > 
> > Well, OK, it's ex-BIOS writers we're dealing with, so I won't say no-one
> > would be stupid enough to come up with a security scheme embedding
> > Private Keys in BS+NV variables, but I would have thought the fact that
> > Linux would blow the lid off it might be a good incentive not to do it
> > and thus a plus point for this patch.
> 
> The hibernation scheme we'd discussed involved having the first stage 
> loader generating a keypair and handing half of it to the OS for 
> encryption of the hibernation partition, then handing the other half to 
> the OS on the next boot so it can decrypt it. That requires non-RT 
> variables to be restricted from OS visibility.

The scheme we discussed, unless something radically changed, was to
convey a temporary key pair via a mechanism to later verify the
hybernate kernel on a resume.  That only requires reboot safe knowledge
of the public key.  The private key can be conveyed in BS only (not NV),
and should be consumed (as in deleted) by the OS when it receives it, so
it wouldn't be exposed by this patch.

James

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[Matthew Garrett]

On Tue, Mar 19, 2013 at 06:23:31PM +0000, James Bottomley wrote:

> The scheme we discussed, unless something radically changed, was to
> convey a temporary key pair via a mechanism to later verify the
> hybernate kernel on a resume.  That only requires reboot safe knowledge
> of the public key.  The private key can be conveyed in BS only (not NV),
> and should be consumed (as in deleted) by the OS when it receives it, so
> it wouldn't be exposed by this patch.

It requires the key to survive the system being entirely powered down, 
which means it needs to be BS+NV. It shouldn't be possible for userspace 
to access this key.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[Matthew Garrett]

On Tue, Mar 19, 2013 at 06:23:31PM +0000, James Bottomley wrote:

> The scheme we discussed, unless something radically changed, was to
> convey a temporary key pair via a mechanism to later verify the
> hybernate kernel on a resume.  That only requires reboot safe knowledge
> of the public key.  The private key can be conveyed in BS only (not NV),
> and should be consumed (as in deleted) by the OS when it receives it, so
> it wouldn't be exposed by this patch.

It requires the key to survive the system being entirely powered down, 
which means it needs to be BS+NV. It shouldn't be possible for userspace 
to access this key.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[James Bottomley]

On Tue, 2013-03-19 at 18:28 +0000, Matthew Garrett wrote:
> On Tue, Mar 19, 2013 at 06:23:31PM +0000, James Bottomley wrote:
> 
> > The scheme we discussed, unless something radically changed, was to
> > convey a temporary key pair via a mechanism to later verify the
> > hybernate kernel on a resume.  That only requires reboot safe knowledge
> > of the public key.  The private key can be conveyed in BS only (not NV),
> > and should be consumed (as in deleted) by the OS when it receives it, so
> > it wouldn't be exposed by this patch.
> 
> It requires the key to survive the system being entirely powered down, 
> which means it needs to be BS+NV. It shouldn't be possible for userspace 
> to access this key.

It requires the *public* key to survive power down, certainly.  The
private key can be thrown away once the hibernate image is signed.  I
think the scheme can be constructed so the private key is never in NV
storage ... that also makes it more secure against tampering.

James

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[James Bottomley]

On Tue, 2013-03-19 at 18:28 +0000, Matthew Garrett wrote:
> On Tue, Mar 19, 2013 at 06:23:31PM +0000, James Bottomley wrote:
> 
> > The scheme we discussed, unless something radically changed, was to
> > convey a temporary key pair via a mechanism to later verify the
> > hybernate kernel on a resume.  That only requires reboot safe knowledge
> > of the public key.  The private key can be conveyed in BS only (not NV),
> > and should be consumed (as in deleted) by the OS when it receives it, so
> > it wouldn't be exposed by this patch.
> 
> It requires the key to survive the system being entirely powered down, 
> which means it needs to be BS+NV. It shouldn't be possible for userspace 
> to access this key.

It requires the *public* key to survive power down, certainly.  The
private key can be thrown away once the hibernate image is signed.  I
think the scheme can be constructed so the private key is never in NV
storage ... that also makes it more secure against tampering.

James

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[Matthew Garrett]

On Tue, Mar 19, 2013 at 06:40:56PM +0000, James Bottomley wrote:
> On Tue, 2013-03-19 at 18:28 +0000, Matthew Garrett wrote:
> > It requires the key to survive the system being entirely powered down, 
> > which means it needs to be BS+NV. It shouldn't be possible for userspace 
> > to access this key.
> 
> It requires the *public* key to survive power down, certainly.  The
> private key can be thrown away once the hibernate image is signed.  I
> think the scheme can be constructed so the private key is never in NV
> storage ... that also makes it more secure against tampering.

Well, that somewhat complicates implementation - we'd be encrypting the 
entire contents of memory except for the key that we're using to encrypt 
memory. Keeping the public key away from userspace avoids having to care 
about that.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[Matthew Garrett]

On Tue, Mar 19, 2013 at 06:40:56PM +0000, James Bottomley wrote:
> On Tue, 2013-03-19 at 18:28 +0000, Matthew Garrett wrote:
> > It requires the key to survive the system being entirely powered down, 
> > which means it needs to be BS+NV. It shouldn't be possible for userspace 
> > to access this key.
> 
> It requires the *public* key to survive power down, certainly.  The
> private key can be thrown away once the hibernate image is signed.  I
> think the scheme can be constructed so the private key is never in NV
> storage ... that also makes it more secure against tampering.

Well, that somewhat complicates implementation - we'd be encrypting the 
entire contents of memory except for the key that we're using to encrypt 
memory. Keeping the public key away from userspace avoids having to care 
about that.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[James Bottomley]

On Tue, 2013-03-19 at 18:50 +0000, Matthew Garrett wrote:
> On Tue, Mar 19, 2013 at 06:40:56PM +0000, James Bottomley wrote:
> > On Tue, 2013-03-19 at 18:28 +0000, Matthew Garrett wrote:
> > > It requires the key to survive the system being entirely powered down, 
> > > which means it needs to be BS+NV. It shouldn't be possible for userspace 
> > > to access this key.
> > 
> > It requires the *public* key to survive power down, certainly.  The
> > private key can be thrown away once the hibernate image is signed.  I
> > think the scheme can be constructed so the private key is never in NV
> > storage ... that also makes it more secure against tampering.
> 
> Well, that somewhat complicates implementation - we'd be encrypting the 
> entire contents of memory except for the key that we're using to encrypt 
> memory. Keeping the public key away from userspace avoids having to care 
> about that.

I don't quite understand what you're getting at: the principle of public
key cryptography is that you can make the public key, well public.  You
only need to guard the private key.

James

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[James Bottomley]

On Tue, 2013-03-19 at 18:50 +0000, Matthew Garrett wrote:
> On Tue, Mar 19, 2013 at 06:40:56PM +0000, James Bottomley wrote:
> > On Tue, 2013-03-19 at 18:28 +0000, Matthew Garrett wrote:
> > > It requires the key to survive the system being entirely powered down, 
> > > which means it needs to be BS+NV. It shouldn't be possible for userspace 
> > > to access this key.
> > 
> > It requires the *public* key to survive power down, certainly.  The
> > private key can be thrown away once the hibernate image is signed.  I
> > think the scheme can be constructed so the private key is never in NV
> > storage ... that also makes it more secure against tampering.
> 
> Well, that somewhat complicates implementation - we'd be encrypting the 
> entire contents of memory except for the key that we're using to encrypt 
> memory. Keeping the public key away from userspace avoids having to care 
> about that.

I don't quite understand what you're getting at: the principle of public
key cryptography is that you can make the public key, well public.  You
only need to guard the private key.

James

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[Matthew Garrett]

On Tue, Mar 19, 2013 at 11:00:31PM +0000, James Bottomley wrote:
> On Tue, 2013-03-19 at 18:50 +0000, Matthew Garrett wrote:
> > Well, that somewhat complicates implementation - we'd be encrypting the 
> > entire contents of memory except for the key that we're using to encrypt 
> > memory. Keeping the public key away from userspace avoids having to care 
> > about that.
> 
> I don't quite understand what you're getting at: the principle of public
> key cryptography is that you can make the public key, well public.  You
> only need to guard the private key.

Ok, so let's just rephrase it as asymmetric cryptography. The aim is to 
ensure that there's never a situation where userspace can decrypt a 
hibernation file, modify it and reencrypt it. So, shim (or whatever) 
generates a keypair. The encryption key is passed to the kernel being 
booted. The decryption key is stashed in a variable in order to be 
passed to the resume kernel.

If the decryption key is available to userspace then the kernel needs to 
discard the encryption key during image write-out - otherwise the 
encryption key will be in the encrypted image. If the decryption key 
isn't available to userspace then this isn't a concern.

If the decryption key *is* available to userspace (as it would be in 
your case), there's a requirement to discard the encryption key during 
the hibernation process. This isn't impossible, but it does add a little 
to the complexity.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[Matthew Garrett]

On Tue, Mar 19, 2013 at 11:00:31PM +0000, James Bottomley wrote:
> On Tue, 2013-03-19 at 18:50 +0000, Matthew Garrett wrote:
> > Well, that somewhat complicates implementation - we'd be encrypting the 
> > entire contents of memory except for the key that we're using to encrypt 
> > memory. Keeping the public key away from userspace avoids having to care 
> > about that.
> 
> I don't quite understand what you're getting at: the principle of public
> key cryptography is that you can make the public key, well public.  You
> only need to guard the private key.

Ok, so let's just rephrase it as asymmetric cryptography. The aim is to 
ensure that there's never a situation where userspace can decrypt a 
hibernation file, modify it and reencrypt it. So, shim (or whatever) 
generates a keypair. The encryption key is passed to the kernel being 
booted. The decryption key is stashed in a variable in order to be 
passed to the resume kernel.

If the decryption key is available to userspace then the kernel needs to 
discard the encryption key during image write-out - otherwise the 
encryption key will be in the encrypted image. If the decryption key 
isn't available to userspace then this isn't a concern.

If the decryption key *is* available to userspace (as it would be in 
your case), there's a requirement to discard the encryption key during 
the hibernation process. This isn't impossible, but it does add a little 
to the complexity.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[James Bottomley]

On Tue, 2013-03-19 at 23:17 +0000, Matthew Garrett wrote:
> On Tue, Mar 19, 2013 at 11:00:31PM +0000, James Bottomley wrote:
> > On Tue, 2013-03-19 at 18:50 +0000, Matthew Garrett wrote:
> > > Well, that somewhat complicates implementation - we'd be encrypting the 
> > > entire contents of memory except for the key that we're using to encrypt 
> > > memory. Keeping the public key away from userspace avoids having to care 
> > > about that.
> > 
> > I don't quite understand what you're getting at: the principle of public
> > key cryptography is that you can make the public key, well public.  You
> > only need to guard the private key.
> 
> Ok, so let's just rephrase it as asymmetric cryptography. The aim is to 
> ensure that there's never a situation where userspace can decrypt a 
> hibernation file, modify it and reencrypt it. So, shim (or whatever) 
> generates a keypair. The encryption key is passed to the kernel being 
> booted. The decryption key is stashed in a variable in order to be 
> passed to the resume kernel.
> 
> If the decryption key is available to userspace then the kernel needs to 
> discard the encryption key during image write-out - otherwise the 
> encryption key will be in the encrypted image. If the decryption key 
> isn't available to userspace then this isn't a concern.
> 
> If the decryption key *is* available to userspace (as it would be in 
> your case), there's a requirement to discard the encryption key during 
> the hibernation process. This isn't impossible, but it does add a little 
> to the complexity.

I agree with this.  But I do think the volatile secret key scheme, where
you discard the key immediately after use is the more secure one because
it relies on fewer secrets (and, indeed, no secrets at all after the
event).  It's a case where transparency encourages better security
architecture.

James

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[James Bottomley]

On Tue, 2013-03-19 at 23:17 +0000, Matthew Garrett wrote:
> On Tue, Mar 19, 2013 at 11:00:31PM +0000, James Bottomley wrote:
> > On Tue, 2013-03-19 at 18:50 +0000, Matthew Garrett wrote:
> > > Well, that somewhat complicates implementation - we'd be encrypting the 
> > > entire contents of memory except for the key that we're using to encrypt 
> > > memory. Keeping the public key away from userspace avoids having to care 
> > > about that.
> > 
> > I don't quite understand what you're getting at: the principle of public
> > key cryptography is that you can make the public key, well public.  You
> > only need to guard the private key.
> 
> Ok, so let's just rephrase it as asymmetric cryptography. The aim is to 
> ensure that there's never a situation where userspace can decrypt a 
> hibernation file, modify it and reencrypt it. So, shim (or whatever) 
> generates a keypair. The encryption key is passed to the kernel being 
> booted. The decryption key is stashed in a variable in order to be 
> passed to the resume kernel.
> 
> If the decryption key is available to userspace then the kernel needs to 
> discard the encryption key during image write-out - otherwise the 
> encryption key will be in the encrypted image. If the decryption key 
> isn't available to userspace then this isn't a concern.
> 
> If the decryption key *is* available to userspace (as it would be in 
> your case), there's a requirement to discard the encryption key during 
> the hibernation process. This isn't impossible, but it does add a little 
> to the complexity.

I agree with this.  But I do think the volatile secret key scheme, where
you discard the key immediately after use is the more secure one because
it relies on fewer secrets (and, indeed, no secrets at all after the
event).  It's a case where transparency encourages better security
architecture.

James

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[Matt Fleming]

On 03/18/2013 08:40 AM, James Bottomley wrote:
> From: James Bottomley <JBottomley@Parallels.com>
> 
> The object here is to make the NV+BS variables accessible (at least read only)
> at runtime so we can get a full picture of the state of the EFI variables for
> debugging and secure boot purposes.

This should definitely be guarded with a debug config option and off by
default. It needs to be made really clear that this code is for
debugging purposes.

[...]

> +static efi_status_t setup_efi_bootvars(struct boot_params *params)
> +{
> +	const int len = 1024;
> +	efi_char16_t *str;
> +	efi_status_t status;
> +	struct setup_data **data = (struct setup_data **)&params->hdr.setup_data;
> +
> +	status = efi_call_phys3(sys_table->boottime->allocate_pool,
> +				EFI_LOADER_DATA, len, &str);
> +	if (status != EFI_SUCCESS)
> +		return status;
> +
> +	/*
> +	 * Note: this trick only works on Little Endian if hdr.setup_data
> +	 * is u64 on both 64 and 32 bit
> +	 */
> +	while (*data)
> +		data = (struct setup_data **)&(*data)->next;
> +
> +	memset(str, 0, len);
> +
> +	for (;;) {
> +		unsigned long size = len, str_len;
> +		efi_guid_t guid;
> +		struct efi_setup_bootvars *bvs;
> +		unsigned int attributes;
> +
> +		status = efi_call_phys3(sys_table->runtime->get_next_variable,
> +					&size, str, &guid);
> +		if (status != EFI_SUCCESS)
> +			break;
> +
> +		str_len = size;

Beware of these recent debacles,

    http://article.gmane.org/gmane.linux.kernel.efi/905
    http://article.gmane.org/gmane.linux.kernel.efi/909

I'm not saying you need to handle them explicitly in this patch, at
least not if this is all under a config option with a big fat warning.

> +		size = 0;
> +		status = efi_call_phys5(sys_table->runtime->get_variable,
> +					str, &guid, &attributes, &size, NULL);
> +		if (status != EFI_SUCCESS && status != EFI_BUFFER_TOO_SMALL)
> +			break;
> +
> +
> +		/*
> +		 * Please don't be tempted to optimise here: some
> +		 * UEFI implementations fail to set attributes if
> +		 * they return any error (including EFI_BUFFER_TOO_SMALL)
> +		 */
> +		status = efi_call_phys3(sys_table->boottime->allocate_pool,
> +					EFI_LOADER_DATA,
> +					size + sizeof(*bvs) + str_len,
> +					&bvs);
> +		if (status != EFI_SUCCESS)
> +			continue;

Allocating things as EFI_LOADER_DATA is fine but you need to be sure to
reserve these memory regions so that they don't get used by any of the
memory allocators. See

    http://article.gmane.org/gmane.linux.kernel.efi/580

(yes, setup_efi_pci() suffers from the same problem)

> +		memset(bvs, 0 , sizeof(bvs));
> +		status = efi_call_phys5(sys_table->runtime->get_variable,
> +					str, &guid, &attributes,
> +					&size, bvs->data);
> +
> +		if (status != EFI_SUCCESS
> +		    || (attributes & 0x07) != (EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +					       EFI_VARIABLE_NON_VOLATILE)) {
> +			efi_call_phys1(sys_table->boottime->free_pool, bvs);
> +			continue;
> +		}

Please use the EFI_VARIABLE_* constants instead of the magic 0x07.

[...]

> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index fff986d..931060a 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -153,17 +153,103 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name,
>  					  unsigned long *data_size,
>  					  void *data)
>  {
> -	return efi_call_virt5(get_variable,
> -			      name, vendor, attr,
> -			      data_size, data);
> +	efi_status_t status = efi_call_virt5(get_variable,
> +					     name, vendor, attr,
> +					     data_size, data);
> +	u64 pa_data;
> +	struct setup_data *s_d;
> +
> +	if (status != EFI_NOT_FOUND)
> +		return status;
> +
> +
> +	for (pa_data = boot_params.hdr.setup_data; pa_data;
> +	     pa_data = s_d->next) {
> +		struct efi_setup_bootvars *bvs;
> +		efi_char16_t *bvs_name;
> +		int i = 0;
> +
> +		s_d = phys_to_virt(pa_data);
> +
> +		if (s_d->type != SETUP_EFIVAR)
> +			continue;
> +
> +		bvs = (struct efi_setup_bootvars *)s_d;
> +		bvs_name = (efi_char16_t *)(bvs->data + bvs->size);
> +
> +		for (i = 0; bvs_name[i] != 0 && name[i] != 0; i++)
> +			if (bvs_name[i] != name[i])
> +				break;
> +
> +		if (bvs_name[i] != 0 || name[i] != 0)
> +			continue;
> +

This would be cleaner if you pulled some of the utf16_str* functions out
of drivers/firmware/efivars.c and stuck them in include/linux/efi.h.
Then you could do something like,

		if (utf16_strncmp(bvs_name, name, utf16_strlen(name)) != 0)
			continue;

> +		if (memcmp(&bvs->guid, vendor, sizeof(*vendor)) != 0)
> +			continue;
> +
> +		if (attr)
> +			*attr = bvs->attributes;
> +
> +		if (*data_size < bvs->size) {
> +			*data_size = bvs->size;
> +			return EFI_BUFFER_TOO_SMALL;
> +		}
> +		*data_size = bvs->size;
> +		memcpy(data, bvs->data, bvs->size);
> +
> +		return EFI_SUCCESS;
> +	}
> +	return EFI_NOT_FOUND;
>  }
>  
>  static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
>  					       efi_char16_t *name,
>  					       efi_guid_t *vendor)
>  {
> -	return efi_call_virt3(get_next_variable,
> -			      name_size, name, vendor);
> +	static int use_bootvars = 0;
> +	efi_status_t status;
> +	u64 pa_data = boot_params.hdr.setup_data;
> +	int found = 0;

Why not make found a boolean?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[Matt Fleming]

On 03/18/2013 08:40 AM, James Bottomley wrote:
> From: James Bottomley <JBottomley-MU7nAjRaF3makBO8gow8eQ@public.gmane.org>
> 
> The object here is to make the NV+BS variables accessible (at least read only)
> at runtime so we can get a full picture of the state of the EFI variables for
> debugging and secure boot purposes.

This should definitely be guarded with a debug config option and off by
default. It needs to be made really clear that this code is for
debugging purposes.

[...]

> +static efi_status_t setup_efi_bootvars(struct boot_params *params)
> +{
> +	const int len = 1024;
> +	efi_char16_t *str;
> +	efi_status_t status;
> +	struct setup_data **data = (struct setup_data **)&params->hdr.setup_data;
> +
> +	status = efi_call_phys3(sys_table->boottime->allocate_pool,
> +				EFI_LOADER_DATA, len, &str);
> +	if (status != EFI_SUCCESS)
> +		return status;
> +
> +	/*
> +	 * Note: this trick only works on Little Endian if hdr.setup_data
> +	 * is u64 on both 64 and 32 bit
> +	 */
> +	while (*data)
> +		data = (struct setup_data **)&(*data)->next;
> +
> +	memset(str, 0, len);
> +
> +	for (;;) {
> +		unsigned long size = len, str_len;
> +		efi_guid_t guid;
> +		struct efi_setup_bootvars *bvs;
> +		unsigned int attributes;
> +
> +		status = efi_call_phys3(sys_table->runtime->get_next_variable,
> +					&size, str, &guid);
> +		if (status != EFI_SUCCESS)
> +			break;
> +
> +		str_len = size;

Beware of these recent debacles,

    http://article.gmane.org/gmane.linux.kernel.efi/905
    http://article.gmane.org/gmane.linux.kernel.efi/909

I'm not saying you need to handle them explicitly in this patch, at
least not if this is all under a config option with a big fat warning.

> +		size = 0;
> +		status = efi_call_phys5(sys_table->runtime->get_variable,
> +					str, &guid, &attributes, &size, NULL);
> +		if (status != EFI_SUCCESS && status != EFI_BUFFER_TOO_SMALL)
> +			break;
> +
> +
> +		/*
> +		 * Please don't be tempted to optimise here: some
> +		 * UEFI implementations fail to set attributes if
> +		 * they return any error (including EFI_BUFFER_TOO_SMALL)
> +		 */
> +		status = efi_call_phys3(sys_table->boottime->allocate_pool,
> +					EFI_LOADER_DATA,
> +					size + sizeof(*bvs) + str_len,
> +					&bvs);
> +		if (status != EFI_SUCCESS)
> +			continue;

Allocating things as EFI_LOADER_DATA is fine but you need to be sure to
reserve these memory regions so that they don't get used by any of the
memory allocators. See

    http://article.gmane.org/gmane.linux.kernel.efi/580

(yes, setup_efi_pci() suffers from the same problem)

> +		memset(bvs, 0 , sizeof(bvs));
> +		status = efi_call_phys5(sys_table->runtime->get_variable,
> +					str, &guid, &attributes,
> +					&size, bvs->data);
> +
> +		if (status != EFI_SUCCESS
> +		    || (attributes & 0x07) != (EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +					       EFI_VARIABLE_NON_VOLATILE)) {
> +			efi_call_phys1(sys_table->boottime->free_pool, bvs);
> +			continue;
> +		}

Please use the EFI_VARIABLE_* constants instead of the magic 0x07.

[...]

> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index fff986d..931060a 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -153,17 +153,103 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name,
>  					  unsigned long *data_size,
>  					  void *data)
>  {
> -	return efi_call_virt5(get_variable,
> -			      name, vendor, attr,
> -			      data_size, data);
> +	efi_status_t status = efi_call_virt5(get_variable,
> +					     name, vendor, attr,
> +					     data_size, data);
> +	u64 pa_data;
> +	struct setup_data *s_d;
> +
> +	if (status != EFI_NOT_FOUND)
> +		return status;
> +
> +
> +	for (pa_data = boot_params.hdr.setup_data; pa_data;
> +	     pa_data = s_d->next) {
> +		struct efi_setup_bootvars *bvs;
> +		efi_char16_t *bvs_name;
> +		int i = 0;
> +
> +		s_d = phys_to_virt(pa_data);
> +
> +		if (s_d->type != SETUP_EFIVAR)
> +			continue;
> +
> +		bvs = (struct efi_setup_bootvars *)s_d;
> +		bvs_name = (efi_char16_t *)(bvs->data + bvs->size);
> +
> +		for (i = 0; bvs_name[i] != 0 && name[i] != 0; i++)
> +			if (bvs_name[i] != name[i])
> +				break;
> +
> +		if (bvs_name[i] != 0 || name[i] != 0)
> +			continue;
> +

This would be cleaner if you pulled some of the utf16_str* functions out
of drivers/firmware/efivars.c and stuck them in include/linux/efi.h.
Then you could do something like,

		if (utf16_strncmp(bvs_name, name, utf16_strlen(name)) != 0)
			continue;

> +		if (memcmp(&bvs->guid, vendor, sizeof(*vendor)) != 0)
> +			continue;
> +
> +		if (attr)
> +			*attr = bvs->attributes;
> +
> +		if (*data_size < bvs->size) {
> +			*data_size = bvs->size;
> +			return EFI_BUFFER_TOO_SMALL;
> +		}
> +		*data_size = bvs->size;
> +		memcpy(data, bvs->data, bvs->size);
> +
> +		return EFI_SUCCESS;
> +	}
> +	return EFI_NOT_FOUND;
>  }
>  
>  static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
>  					       efi_char16_t *name,
>  					       efi_guid_t *vendor)
>  {
> -	return efi_call_virt3(get_next_variable,
> -			      name_size, name, vendor);
> +	static int use_bootvars = 0;
> +	efi_status_t status;
> +	u64 pa_data = boot_params.hdr.setup_data;
> +	int found = 0;

Why not make found a boolean?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[Matthew Garrett]

On Wed, Mar 20, 2013 at 08:00:03AM +0000, James Bottomley wrote:

> I agree with this.  But I do think the volatile secret key scheme, where
> you discard the key immediately after use is the more secure one because
> it relies on fewer secrets (and, indeed, no secrets at all after the
> event).  It's a case where transparency encourages better security
> architecture.

That somewhat depends what you're securing against. There's an argument 
that reusing this key is actually sufficiently secure so long as the 
only way to obtain it is with physical access to the machine - that way 
you don't have an nvram update cycle every boot. Exposing these 
variables to userspace doesn't make it impossible to produce a secure 
system, but it does remove some choices that were previously available.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] x86/efi: pull NV+BS variables out before we exit boot services

[Matthew Garrett]

On Wed, Mar 20, 2013 at 08:00:03AM +0000, James Bottomley wrote:

> I agree with this.  But I do think the volatile secret key scheme, where
> you discard the key immediately after use is the more secure one because
> it relies on fewer secrets (and, indeed, no secrets at all after the
> event).  It's a case where transparency encourages better security
> architecture.

That somewhat depends what you're securing against. There's an argument 
that reusing this key is actually sufficiently secure so long as the 
only way to obtain it is with physical access to the machine - that way 
you don't have an nvram update cycle every boot. Exposing these 
variables to userspace doesn't make it impossible to produce a secure 
system, but it does remove some choices that were previously available.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org