[PATCH 00/10] efi: Constify all the things

[PATCH 00/10] efi: Constify all the things

[Lukas Wunner]

When disassembling the EFI stub I noticed that GUIDs and efi_char16_t
strings are not placed in rodata but generated on the stack at runtime.
GUIDs occupy up to 80 bytes of text instead of just 16 bytes rodata
which annoyed me enough to go down the rabbit hole of fixing it.
This series constifies all GUIDs and EFI-related strings I could find.

The most important patch is [10/10] to constify the EFI_GUID() macro.
This has to be at the end of the series to avoid compiler warnings.
Reviewers may want to read its commit message first as it contains
background information.  In particular it explains why this series
only fixes the kernel side of things.  Additional work is necessary
on the compiler side to actually have all the GUIDs and strings in
rodata.  At the moment both gcc and clang achieve that only partially.

Ard Biesheuvel expressed an interest in these patches in November but
stressed the importance that arguments to EFI boot/runtime/protocol
services may only be declared const if the spec marks them "IN":
https://lkml.org/lkml/2016/11/21/459

Hence I've split the series by boot/runtime/protocol services to allow
for easy verification of this fact.  Constification of GetVariable() /
SetVariable() is further split in one patch for the function signatures
and another for the actual arguments because changing the function
signatures is quite involved in this case.

efi.git/next needs to be rebased on v4.10-rc1 before this series can be
applied because it requires commits f6697df36bdf ("x86/efi: Prevent mixed
mode boot corruption with CONFIG_VMAP_STACK=y") and 2fbadc3002c5
("arm/arm64: xen: Move shared architecture headers to include/xen/arm").

Unfortunately I was not able to compile-test on ia64, there's no cross-
compiler package available on Debian. :-(  I did not receive complaints
from 0-day, so I'm cautiously optimistic that I didn't cause breakage
there.

I've pushed the patches to GitHub to ease reviewing/fetching:
https://github.com/l1k/linux/commits/efi_const_v1

Thanks,

Lukas


Ard Biesheuvel (1):
  efi: use typed function pointers for runtime services table

Lukas Wunner (9):
  efi: Constify GetVariable() / SetVariable() signatures
  efi: Constify GetVariable() / SetVariable() arguments
  efi: Constify HandleProtocol() / LocateHandle() / LocateProtocol()
  efi: Constify InstallConfigurationTable()
  efi: Constify EFI_FILE_PROTOCOL.GetInfo()
  efi: Constify EFI_RNG_PROTOCOL.GetRNG()
  efi: Constify EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.OutputString()
  efi: Constify efi_guidcmp() arguments
  uuid: Constify UUID compound literals

 arch/ia64/kernel/efi.c                         | 24 ++++------
 arch/x86/boot/compressed/eboot.c               | 50 ++++++++++-----------
 arch/x86/include/asm/xen/interface.h           |  1 +
 arch/x86/platform/efi/efi_64.c                 | 18 ++++----
 arch/x86/platform/efi/quirks.c                 |  2 +-
 drivers/firmware/efi/efi-pstore.c              | 10 ++---
 drivers/firmware/efi/efibc.c                   |  3 +-
 drivers/firmware/efi/libstub/arm-stub.c        | 17 +++----
 drivers/firmware/efi/libstub/arm32-stub.c      |  2 +-
 drivers/firmware/efi/libstub/efi-stub-helper.c |  7 ++-
 drivers/firmware/efi/libstub/efistub.h         |  2 +-
 drivers/firmware/efi/libstub/fdt.c             |  3 +-
 drivers/firmware/efi/libstub/gop.c             | 12 +++--
 drivers/firmware/efi/libstub/random.c          | 16 +++----
 drivers/firmware/efi/runtime-wrappers.c        | 15 ++++---
 drivers/firmware/google/gsmi.c                 | 10 ++---
 drivers/infiniband/hw/hfi1/efivar.c            |  6 +--
 drivers/scsi/isci/probe_roms.c                 |  2 +-
 drivers/xen/efi.c                              | 12 ++---
 include/linux/efi.h                            | 61 ++++++++++++++------------
 include/uapi/linux/uuid.h                      |  4 +-
 include/xen/arm/interface.h                    |  1 +
 include/xen/interface/platform.h               | 11 ++++-
 include/xen/xen-ops.h                          | 12 ++---
 24 files changed, 143 insertions(+), 158 deletions(-)

-- 
2.11.0

[PATCH 01/10] efi: use typed function pointers for runtime services table

[Lukas Wunner]

From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Instead of using void pointers, and casting them to correctly typed
function pointers upon use, declare the runtime services pointers
as function pointers using their respective prototypes, for which
typedefs are already available.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 include/linux/efi.h | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 712a3aa..889d40d 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -509,24 +509,6 @@ struct efi_boot_memmap {
 	u64 query_variable_info;
 } efi_runtime_services_64_t;
 
-typedef struct {
-	efi_table_hdr_t hdr;
-	void *get_time;
-	void *set_time;
-	void *get_wakeup_time;
-	void *set_wakeup_time;
-	void *set_virtual_address_map;
-	void *convert_pointer;
-	void *get_variable;
-	void *get_next_variable;
-	void *set_variable;
-	void *get_next_high_mono_count;
-	void *reset_system;
-	void *update_capsule;
-	void *query_capsule_caps;
-	void *query_variable_info;
-} efi_runtime_services_t;
-
 typedef efi_status_t efi_get_time_t (efi_time_t *tm, efi_time_cap_t *tc);
 typedef efi_status_t efi_set_time_t (efi_time_t *tm);
 typedef efi_status_t efi_get_wakeup_time_t (efi_bool_t *enabled, efi_bool_t *pending,
@@ -561,6 +543,24 @@ typedef efi_status_t efi_query_variable_store_t(u32 attributes,
 						unsigned long size,
 						bool nonblocking);
 
+typedef struct {
+	efi_table_hdr_t			hdr;
+	efi_get_time_t			*get_time;
+	efi_set_time_t			*set_time;
+	efi_get_wakeup_time_t		*get_wakeup_time;
+	efi_set_wakeup_time_t		*set_wakeup_time;
+	efi_set_virtual_address_map_t	*set_virtual_address_map;
+	void				*convert_pointer;
+	efi_get_variable_t		*get_variable;
+	efi_get_next_variable_t		*get_next_variable;
+	efi_set_variable_t		*set_variable;
+	efi_get_next_high_mono_count_t	*get_next_high_mono_count;
+	efi_reset_system_t		*reset_system;
+	efi_update_capsule_t		*update_capsule;
+	efi_query_capsule_caps_t	*query_capsule_caps;
+	efi_query_variable_info_t	*query_variable_info;
+} efi_runtime_services_t;
+
 void efi_native_runtime_setup(void);
 
 /*
-- 
2.11.0

[PATCH 02/10] efi: Constify GetVariable() / SetVariable() signatures

[Lukas Wunner]

GetVariable() / SetVariable() is typically called with immutable
VendorGuid and VariableName arguments.  SetVariable() is also often
called with an immutable Data argument.  However declaring these const
in the caller results in a compiler warning since they're not marked
const in the function signatures.  Rectify that, in keeping with the EFI
spec which marks these arguments "IN".

Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Tony Luck <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Fenghua Yu <fenghua.yu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Boris Ostrovsky <boris.ostrovsky-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Cc: David Vrabel <david.vrabel-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
Cc: Juergen Gross <jgross-IBi9RG/b67k@public.gmane.org>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 arch/ia64/kernel/efi.c                  |  9 ++++-----
 arch/x86/include/asm/xen/interface.h    |  1 +
 arch/x86/platform/efi/efi_64.c          | 18 +++++++++---------
 drivers/firmware/efi/runtime-wrappers.c | 15 ++++++++-------
 drivers/firmware/google/gsmi.c          | 10 +++++-----
 drivers/xen/efi.c                       | 12 ++++++------
 include/linux/efi.h                     | 10 ++++++----
 include/xen/arm/interface.h             |  1 +
 include/xen/interface/platform.h        | 11 +++++++++--
 include/xen/xen-ops.h                   | 12 ++++++------
 10 files changed, 55 insertions(+), 44 deletions(-)

diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
index 1212956..f57d089 100644
--- a/arch/ia64/kernel/efi.c
+++ b/arch/ia64/kernel/efi.c
@@ -125,8 +125,8 @@
 
 #define STUB_GET_VARIABLE(prefix, adjust_arg)				       \
 static efi_status_t							       \
-prefix##_get_variable (efi_char16_t *name, efi_guid_t *vendor, u32 *attr,      \
-		       unsigned long *data_size, void *data)		       \
+prefix##_get_variable (const efi_char16_t *name, const efi_guid_t *vendor,     \
+		       u32 *attr, unsigned long *data_size, void *data)        \
 {									       \
 	struct ia64_fpreg fr[6];					       \
 	u32 *aattr = NULL;						       \
@@ -161,9 +161,8 @@
 
 #define STUB_SET_VARIABLE(prefix, adjust_arg)				       \
 static efi_status_t							       \
-prefix##_set_variable (efi_char16_t *name, efi_guid_t *vendor,		       \
-		       u32 attr, unsigned long data_size,		       \
-		       void *data)					       \
+prefix##_set_variable (const efi_char16_t *name, const efi_guid_t *vendor,     \
+		       u32 attr, unsigned long data_size, const void *data)    \
 {									       \
 	struct ia64_fpreg fr[6];					       \
 	efi_status_t ret;						       \
diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h
index 62ca03e..543716c 100644
--- a/arch/x86/include/asm/xen/interface.h
+++ b/arch/x86/include/asm/xen/interface.h
@@ -86,6 +86,7 @@
 /* Guest handles for primitive C types. */
 __DEFINE_GUEST_HANDLE(uchar, unsigned char);
 __DEFINE_GUEST_HANDLE(uint,  unsigned int);
+__DEFINE_GUEST_HANDLE(cvoid, const void);
 DEFINE_GUEST_HANDLE(char);
 DEFINE_GUEST_HANDLE(int);
 DEFINE_GUEST_HANDLE(void);
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index ee966aa..2a7d423 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -632,13 +632,13 @@ static efi_status_t efi_thunk_set_time(efi_time_t *tm)
 	return status;
 }
 
-static unsigned long efi_name_size(efi_char16_t *name)
+static unsigned long efi_name_size(const efi_char16_t *name)
 {
 	return ucs2_strsize(name, EFI_VAR_NAME_LEN) + 1;
 }
 
 static efi_status_t
-efi_thunk_get_variable(efi_char16_t *name, efi_guid_t *vendor,
+efi_thunk_get_variable(const efi_char16_t *name, const efi_guid_t *vendor,
 		       u32 *attr, unsigned long *data_size, void *data)
 {
 	efi_status_t status;
@@ -646,8 +646,8 @@ static unsigned long efi_name_size(efi_char16_t *name)
 	u32 phys_data_size, phys_data;
 
 	phys_data_size = virt_to_phys_or_null(data_size);
-	phys_vendor = virt_to_phys_or_null(vendor);
-	phys_name = virt_to_phys_or_null_size(name, efi_name_size(name));
+	phys_vendor = virt_to_phys_or_null((void *)vendor);
+	phys_name = virt_to_phys_or_null_size((void *)name, efi_name_size(name));
 	phys_attr = virt_to_phys_or_null(attr);
 	phys_data = virt_to_phys_or_null_size(data, *data_size);
 
@@ -658,15 +658,15 @@ static unsigned long efi_name_size(efi_char16_t *name)
 }
 
 static efi_status_t
-efi_thunk_set_variable(efi_char16_t *name, efi_guid_t *vendor,
-		       u32 attr, unsigned long data_size, void *data)
+efi_thunk_set_variable(const efi_char16_t *name, const efi_guid_t *vendor,
+		       u32 attr, unsigned long data_size, const void *data)
 {
 	u32 phys_name, phys_vendor, phys_data;
 	efi_status_t status;
 
-	phys_name = virt_to_phys_or_null_size(name, efi_name_size(name));
-	phys_vendor = virt_to_phys_or_null(vendor);
-	phys_data = virt_to_phys_or_null_size(data, data_size);
+	phys_name = virt_to_phys_or_null_size((void *)name, efi_name_size(name));
+	phys_vendor = virt_to_phys_or_null((void *)vendor);
+	phys_data = virt_to_phys_or_null_size((void *)data, data_size);
 
 	/* If data_size is > sizeof(u32) we've got problems */
 	status = efi_thunk(set_variable, phys_name, phys_vendor,
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index ae54870b..a98de14 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -136,8 +136,8 @@ static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
 	return status;
 }
 
-static efi_status_t virt_efi_get_variable(efi_char16_t *name,
-					  efi_guid_t *vendor,
+static efi_status_t virt_efi_get_variable(const efi_char16_t *name,
+					  const efi_guid_t *vendor,
 					  u32 *attr,
 					  unsigned long *data_size,
 					  void *data)
@@ -165,11 +165,11 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
 	return status;
 }
 
-static efi_status_t virt_efi_set_variable(efi_char16_t *name,
-					  efi_guid_t *vendor,
+static efi_status_t virt_efi_set_variable(const efi_char16_t *name,
+					  const efi_guid_t *vendor,
 					  u32 attr,
 					  unsigned long data_size,
-					  void *data)
+					  const void *data)
 {
 	efi_status_t status;
 
@@ -182,9 +182,10 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
 }
 
 static efi_status_t
-virt_efi_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor,
+virt_efi_set_variable_nonblocking(const efi_char16_t *name,
+				  const efi_guid_t *vendor,
 				  u32 attr, unsigned long data_size,
-				  void *data)
+				  const void *data)
 {
 	efi_status_t status;
 
diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c
index c463871..a1b3cd9 100644
--- a/drivers/firmware/google/gsmi.c
+++ b/drivers/firmware/google/gsmi.c
@@ -289,8 +289,8 @@ static int gsmi_exec(u8 func, u8 sub)
 	return rc;
 }
 
-static efi_status_t gsmi_get_variable(efi_char16_t *name,
-				      efi_guid_t *vendor, u32 *attr,
+static efi_status_t gsmi_get_variable(const efi_char16_t *name,
+				      const efi_guid_t *vendor, u32 *attr,
 				      unsigned long *data_size,
 				      void *data)
 {
@@ -410,11 +410,11 @@ static efi_status_t gsmi_get_next_variable(unsigned long *name_size,
 	return ret;
 }
 
-static efi_status_t gsmi_set_variable(efi_char16_t *name,
-				      efi_guid_t *vendor,
+static efi_status_t gsmi_set_variable(const efi_char16_t *name,
+				      const efi_guid_t *vendor,
 				      u32 attr,
 				      unsigned long data_size,
-				      void *data)
+				      const void *data)
 {
 	struct gsmi_nvram_var_param param = {
 		.name_ptr = gsmi_dev.name_buf->address,
diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
index 22f71ff..248740a 100644
--- a/drivers/xen/efi.c
+++ b/drivers/xen/efi.c
@@ -117,9 +117,9 @@ efi_status_t xen_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
 }
 EXPORT_SYMBOL_GPL(xen_efi_set_wakeup_time);
 
-efi_status_t xen_efi_get_variable(efi_char16_t *name, efi_guid_t *vendor,
-				  u32 *attr, unsigned long *data_size,
-				  void *data)
+efi_status_t xen_efi_get_variable(const efi_char16_t *name,
+				  const efi_guid_t *vendor, u32 *attr,
+				  unsigned long *data_size, void *data)
 {
 	struct xen_platform_op op = INIT_EFI_OP(get_variable);
 
@@ -165,9 +165,9 @@ efi_status_t xen_efi_get_next_variable(unsigned long *name_size,
 }
 EXPORT_SYMBOL_GPL(xen_efi_get_next_variable);
 
-efi_status_t xen_efi_set_variable(efi_char16_t *name, efi_guid_t *vendor,
-				 u32 attr, unsigned long data_size,
-				 void *data)
+efi_status_t xen_efi_set_variable(const efi_char16_t *name,
+				  const efi_guid_t *vendor, u32 attr,
+				  unsigned long data_size, const void *data)
 {
 	struct xen_platform_op op = INIT_EFI_OP(set_variable);
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 889d40d..6ade102 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -514,13 +514,15 @@ struct efi_boot_memmap {
 typedef efi_status_t efi_get_wakeup_time_t (efi_bool_t *enabled, efi_bool_t *pending,
 					    efi_time_t *tm);
 typedef efi_status_t efi_set_wakeup_time_t (efi_bool_t enabled, efi_time_t *tm);
-typedef efi_status_t efi_get_variable_t (efi_char16_t *name, efi_guid_t *vendor, u32 *attr,
+typedef efi_status_t efi_get_variable_t (const efi_char16_t *name,
+					 const efi_guid_t *vendor, u32 *attr,
 					 unsigned long *data_size, void *data);
 typedef efi_status_t efi_get_next_variable_t (unsigned long *name_size, efi_char16_t *name,
 					      efi_guid_t *vendor);
-typedef efi_status_t efi_set_variable_t (efi_char16_t *name, efi_guid_t *vendor, 
-					 u32 attr, unsigned long data_size,
-					 void *data);
+typedef efi_status_t efi_set_variable_t (const efi_char16_t *name,
+					 const efi_guid_t *vendor, u32 attr,
+					 unsigned long data_size,
+					 const void *data);
 typedef efi_status_t efi_get_next_high_mono_count_t (u32 *count);
 typedef void efi_reset_system_t (int reset_type, efi_status_t status,
 				 unsigned long data_size, efi_char16_t *data);
diff --git a/include/xen/arm/interface.h b/include/xen/arm/interface.h
index 75d5968..daa2956 100644
--- a/include/xen/arm/interface.h
+++ b/include/xen/arm/interface.h
@@ -47,6 +47,7 @@
 /* Guest handles for primitive C types. */
 __DEFINE_GUEST_HANDLE(uchar, unsigned char);
 __DEFINE_GUEST_HANDLE(uint,  unsigned int);
+__DEFINE_GUEST_HANDLE(cvoid, const void);
 DEFINE_GUEST_HANDLE(char);
 DEFINE_GUEST_HANDLE(int);
 DEFINE_GUEST_HANDLE(void);
diff --git a/include/xen/interface/platform.h b/include/xen/interface/platform.h
index 732efb0..b7e249c 100644
--- a/include/xen/interface/platform.h
+++ b/include/xen/interface/platform.h
@@ -171,7 +171,7 @@ struct xenpf_efi_runtime_call {
 #define XEN_EFI_VARIABLE_BOOTSERVICE_ACCESS 0x00000002
 #define XEN_EFI_VARIABLE_RUNTIME_ACCESS     0x00000004
 		struct {
-			GUEST_HANDLE(void) name;  /* UCS-2/UTF-16 string */
+			GUEST_HANDLE(cvoid) name;  /* UCS-2/UTF-16 string */
 			xen_ulong_t size;
 			GUEST_HANDLE(void) data;
 			struct xenpf_efi_guid {
@@ -180,7 +180,14 @@ struct xenpf_efi_runtime_call {
 				uint16_t data3;
 				uint8_t data4[8];
 			} vendor_guid;
-		} get_variable, set_variable;
+		} get_variable;
+
+		struct {
+			GUEST_HANDLE(cvoid) name;  /* UCS-2/UTF-16 string */
+			xen_ulong_t size;
+			GUEST_HANDLE(cvoid) data;
+			struct xenpf_efi_guid vendor_guid;
+		} set_variable;
 
 		struct {
 			xen_ulong_t size;
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index b5486e6..b118f56 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -103,14 +103,14 @@ int xen_xlate_map_ballooned_pages(xen_pfn_t **pfns, void **vaddr,
 efi_status_t xen_efi_get_wakeup_time(efi_bool_t *enabled, efi_bool_t *pending,
 				     efi_time_t *tm);
 efi_status_t xen_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm);
-efi_status_t xen_efi_get_variable(efi_char16_t *name, efi_guid_t *vendor,
-				  u32 *attr, unsigned long *data_size,
-				  void *data);
+efi_status_t xen_efi_get_variable(const efi_char16_t *name,
+				  const efi_guid_t *vendor, u32 *attr,
+				  unsigned long *data_size, void *data);
 efi_status_t xen_efi_get_next_variable(unsigned long *name_size,
 				       efi_char16_t *name, efi_guid_t *vendor);
-efi_status_t xen_efi_set_variable(efi_char16_t *name, efi_guid_t *vendor,
-				  u32 attr, unsigned long data_size,
-				  void *data);
+efi_status_t xen_efi_set_variable(const efi_char16_t *name,
+				  const efi_guid_t *vendor, u32 attr,
+				  unsigned long data_size, const void *data);
 efi_status_t xen_efi_query_variable_info(u32 attr, u64 *storage_space,
 					 u64 *remaining_space,
 					 u64 *max_variable_size);
-- 
2.11.0

[PATCH 03/10] efi: Constify GetVariable() / SetVariable() arguments

[Lukas Wunner]

GUIDs and variable names passed to GetVariable() / SetVariable() are not
modified, so declare them const.  This allows the compiler to place them
in the rodata section instead of generating them on the stack at
runtime.  Pass GUIDs as literals rather than assigning them to temporary
variables to save a bit on code.

Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Tony Luck <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Fenghua Yu <fenghua.yu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Anton Vorontsov <anton-9xeibp6oKSgdnm+yROfE0A@public.gmane.org>
Cc: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Artur Paszkiewicz <artur.paszkiewicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 arch/ia64/kernel/efi.c                  | 15 ++++-----------
 arch/x86/platform/efi/quirks.c          |  2 +-
 drivers/firmware/efi/efi-pstore.c       |  4 ++--
 drivers/firmware/efi/efibc.c            |  3 +--
 drivers/firmware/efi/libstub/arm-stub.c |  5 ++---
 drivers/infiniband/hw/hfi1/efivar.c     |  6 +-----
 drivers/scsi/isci/probe_roms.c          |  2 +-
 7 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
index f57d089..f1264ab 100644
--- a/arch/ia64/kernel/efi.c
+++ b/arch/ia64/kernel/efi.c
@@ -919,22 +919,15 @@ static void __init handle_palo(unsigned long phys_addr)
 efi_uart_console_only(void)
 {
 	efi_status_t status;
-	char *s, name[] = "ConOut";
-	efi_guid_t guid = EFI_GLOBAL_VARIABLE_GUID;
-	efi_char16_t *utf16, name_utf16[32];
+	const char name[] = "ConOut";
+	const efi_char16_t name_utf16[] = { 'C', 'o', 'n', 'O', 'u', 't', 0 };
 	unsigned char data[1024];
 	unsigned long size = sizeof(data);
 	struct efi_generic_dev_path *hdr, *end_addr;
 	int uart = 0;
 
-	/* Convert to UTF-16 */
-	utf16 = name_utf16;
-	s = name;
-	while (*s)
-		*utf16++ = *s++ & 0x7f;
-	*utf16 = 0;
-
-	status = efi.get_variable(name_utf16, &guid, NULL, &size, data);
+	status = efi.get_variable(name_utf16, &EFI_GLOBAL_VARIABLE_GUID,
+				  NULL, &size, data);
 	if (status != EFI_SUCCESS) {
 		printk(KERN_ERR "No EFI %s variable?\n", name);
 		return 0;
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 10aca63..aad3701 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -19,7 +19,7 @@
 #define EFI_DUMMY_GUID \
 	EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e, 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92, 0xa9)
 
-static efi_char16_t efi_dummy_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };
+static const efi_char16_t efi_dummy_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };
 
 static bool efi_no_storage_paranoia;
 
diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index f402ba2..0911449 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -265,7 +265,6 @@ static int efi_pstore_write(enum pstore_type_id type,
 {
 	char name[DUMP_NAME_LEN];
 	efi_char16_t efi_name[DUMP_NAME_LEN];
-	efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
 	int i, ret = 0;
 
 	sprintf(name, "dump-type%u-%u-%d-%lu-%c", type, part, count,
@@ -274,7 +273,8 @@ static int efi_pstore_write(enum pstore_type_id type,
 	for (i = 0; i < DUMP_NAME_LEN; i++)
 		efi_name[i] = name[i];
 
-	efivar_entry_set_safe(efi_name, vendor, PSTORE_EFI_ATTRIBUTES,
+	efivar_entry_set_safe(efi_name, LINUX_EFI_CRASH_GUID,
+			      PSTORE_EFI_ATTRIBUTES,
 			      !pstore_cannot_block_path(reason),
 			      size, psi->buf);
 
diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
index 503bbe2..57ddf8c 100644
--- a/drivers/firmware/efi/efibc.c
+++ b/drivers/firmware/efi/efibc.c
@@ -32,7 +32,6 @@ static void efibc_str_to_str16(const char *str, efi_char16_t *str16)
 static int efibc_set_variable(const char *name, const char *value)
 {
 	int ret;
-	efi_guid_t guid = LINUX_EFI_LOADER_ENTRY_GUID;
 	struct efivar_entry *entry;
 	size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);
 
@@ -49,7 +48,7 @@ static int efibc_set_variable(const char *name, const char *value)
 
 	efibc_str_to_str16(name, entry->var.VariableName);
 	efibc_str_to_str16(value, (efi_char16_t *)entry->var.Data);
-	memcpy(&entry->var.VendorGuid, &guid, sizeof(guid));
+	entry->var.VendorGuid = LINUX_EFI_LOADER_ENTRY_GUID;
 
 	ret = efivar_entry_set(entry,
 			       EFI_VARIABLE_NON_VOLATILE
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index 6fca48c..da72bfb 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -27,13 +27,12 @@ static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
 	static efi_char16_t const sm_var_name[] = {
 		'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
 
-	efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
 	efi_get_variable_t *f_getvar = sys_table_arg->runtime->get_variable;
 	u8 val;
 	unsigned long size = sizeof(val);
 	efi_status_t status;
 
-	status = f_getvar((efi_char16_t *)sb_var_name, (efi_guid_t *)&var_guid,
+	status = f_getvar(sb_var_name, &EFI_GLOBAL_VARIABLE_GUID,
 			  NULL, &size, &val);
 
 	if (status != EFI_SUCCESS)
@@ -42,7 +41,7 @@ static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
 	if (val == 0)
 		return 0;
 
-	status = f_getvar((efi_char16_t *)sm_var_name, (efi_guid_t *)&var_guid,
+	status = f_getvar(sm_var_name, &EFI_GLOBAL_VARIABLE_GUID,
 			  NULL, &size, &val);
 
 	if (status != EFI_SUCCESS)
diff --git a/drivers/infiniband/hw/hfi1/efivar.c b/drivers/infiniband/hw/hfi1/efivar.c
index 106349f..f769cc2 100644
--- a/drivers/infiniband/hw/hfi1/efivar.c
+++ b/drivers/infiniband/hw/hfi1/efivar.c
@@ -66,7 +66,6 @@ static int read_efi_var(const char *name, unsigned long *size,
 {
 	efi_status_t status;
 	efi_char16_t *uni_name;
-	efi_guid_t guid;
 	unsigned long temp_size;
 	void *temp_buffer;
 	void *data;
@@ -95,13 +94,10 @@ static int read_efi_var(const char *name, unsigned long *size,
 	for (i = 0; name[i]; i++)
 		uni_name[i] = name[i];
 
-	/* need a variable for our GUID */
-	guid = HFI1_EFIVAR_GUID;
-
 	/* call into EFI runtime services */
 	status = efi.get_variable(
 			uni_name,
-			&guid,
+			&HFI1_EFIVAR_GUID,
 			NULL,
 			&temp_size,
 			temp_buffer);
diff --git a/drivers/scsi/isci/probe_roms.c b/drivers/scsi/isci/probe_roms.c
index 8ac646e..d2c17c1 100644
--- a/drivers/scsi/isci/probe_roms.c
+++ b/drivers/scsi/isci/probe_roms.c
@@ -34,7 +34,7 @@
 #include "task.h"
 #include "probe_roms.h"
 
-static efi_char16_t isci_efivar_name[] = {
+static const efi_char16_t isci_efivar_name[] = {
 	'R', 's', 't', 'S', 'c', 'u', 'O'
 };
 
-- 
2.11.0

[PATCH 04/10] efi: Constify HandleProtocol() / LocateHandle() / LocateProtocol()

[Lukas Wunner]

GUIDs passed to HandleProtocol() / LocateHandle() / LocateProtocol() are
not modified, so declare them const.  This allows the compiler to place
them in the rodata section instead of generating them on the stack at
runtime.  Pass GUIDs as literals rather than assigning them to temporary
variables to save a bit on code.  (Except for UGA and GOP GUIDs as they
are passed around between functions.)  Constification of the GUIDs is
in compliance with the EFI spec which marks them "IN" for said boot
services.

Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 arch/x86/boot/compressed/eboot.c        | 48 ++++++++++++++++-----------------
 drivers/firmware/efi/libstub/arm-stub.c | 10 +++----
 drivers/firmware/efi/libstub/gop.c      | 12 ++++-----
 drivers/firmware/efi/libstub/random.c   |  6 ++---
 include/linux/efi.h                     |  9 ++++---
 5 files changed, 39 insertions(+), 46 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 6d3aeab..61014f5 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -43,13 +43,12 @@ static inline efi_status_t __open_volume32(void *__image, void **__fh)
 	efi_file_io_interface_t *io;
 	efi_loaded_image_32_t *image = __image;
 	efi_file_handle_32_t *fh;
-	efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID;
 	efi_status_t status;
 	void *handle = (void *)(unsigned long)image->device_handle;
 	unsigned long func;
 
 	status = efi_call_early(handle_protocol, handle,
-				&fs_proto, (void **)&io);
+				&EFI_FILE_SYSTEM_GUID, (void **)&io);
 	if (status != EFI_SUCCESS) {
 		efi_printk(sys_table, "Failed to handle fs_proto\n");
 		return status;
@@ -69,13 +68,12 @@ static inline efi_status_t __open_volume64(void *__image, void **__fh)
 	efi_file_io_interface_t *io;
 	efi_loaded_image_64_t *image = __image;
 	efi_file_handle_64_t *fh;
-	efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID;
 	efi_status_t status;
 	void *handle = (void *)(unsigned long)image->device_handle;
 	unsigned long func;
 
 	status = efi_call_early(handle_protocol, handle,
-				&fs_proto, (void **)&io);
+				&EFI_FILE_SYSTEM_GUID, (void **)&io);
 	if (status != EFI_SUCCESS) {
 		efi_printk(sys_table, "Failed to handle fs_proto\n");
 		return status;
@@ -173,7 +171,6 @@ void efi_char16_printk(efi_system_table_t *table, efi_char16_t *str)
 		unsigned long size)
 {
 	efi_pci_io_protocol_32 *pci = NULL;
-	efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
 	u32 *handles = (u32 *)(unsigned long)pci_handle;
 	efi_status_t status;
 	unsigned long nr_pci;
@@ -191,7 +188,8 @@ void efi_char16_printk(efi_system_table_t *table, efi_char16_t *str)
 		u32 h = handles[i];
 
 		status = efi_call_early(handle_protocol, h,
-					&pci_proto, (void **)&pci);
+					&EFI_PCI_IO_PROTOCOL_GUID,
+					(void **)&pci);
 
 		if (status != EFI_SUCCESS)
 			continue;
@@ -280,7 +278,6 @@ void efi_char16_printk(efi_system_table_t *table, efi_char16_t *str)
 		unsigned long size)
 {
 	efi_pci_io_protocol_64 *pci = NULL;
-	efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
 	u64 *handles = (u64 *)(unsigned long)pci_handle;
 	efi_status_t status;
 	unsigned long nr_pci;
@@ -298,7 +295,8 @@ void efi_char16_printk(efi_system_table_t *table, efi_char16_t *str)
 		u64 h = handles[i];
 
 		status = efi_call_early(handle_protocol, h,
-					&pci_proto, (void **)&pci);
+					&EFI_PCI_IO_PROTOCOL_GUID,
+					(void **)&pci);
 
 		if (status != EFI_SUCCESS)
 			continue;
@@ -333,12 +331,12 @@ static void setup_efi_pci(struct boot_params *params)
 {
 	efi_status_t status;
 	void **pci_handle = NULL;
-	efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
 	unsigned long size = 0;
 
 	status = efi_call_early(locate_handle,
 				EFI_LOCATE_BY_PROTOCOL,
-				&pci_proto, NULL, &size, pci_handle);
+				&EFI_PCI_IO_PROTOCOL_GUID,
+				NULL, &size, pci_handle);
 
 	if (status == EFI_BUFFER_TOO_SMALL) {
 		status = efi_call_early(allocate_pool,
@@ -351,7 +349,8 @@ static void setup_efi_pci(struct boot_params *params)
 		}
 
 		status = efi_call_early(locate_handle,
-					EFI_LOCATE_BY_PROTOCOL, &pci_proto,
+					EFI_LOCATE_BY_PROTOCOL,
+					&EFI_PCI_IO_PROTOCOL_GUID,
 					NULL, &size, pci_handle);
 	}
 
@@ -369,13 +368,13 @@ static void setup_efi_pci(struct boot_params *params)
 
 static void retrieve_apple_device_properties(struct boot_params *boot_params)
 {
-	efi_guid_t guid = APPLE_PROPERTIES_PROTOCOL_GUID;
 	struct setup_data *data, *new;
 	efi_status_t status;
 	u32 size = 0;
 	void *p;
 
-	status = efi_call_early(locate_protocol, &guid, NULL, &p);
+	status = efi_call_early(locate_protocol,
+				&APPLE_PROPERTIES_PROTOCOL_GUID, NULL, &p);
 	if (status != EFI_SUCCESS)
 		return;
 
@@ -434,7 +433,7 @@ static void setup_quirks(struct boot_params *boot_params)
 setup_uga32(void **uga_handle, unsigned long size, u32 *width, u32 *height)
 {
 	struct efi_uga_draw_protocol *uga = NULL, *first_uga;
-	efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID;
+	const efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID;
 	unsigned long nr_ugas;
 	u32 *handles = (u32 *)uga_handle;;
 	efi_status_t status = EFI_INVALID_PARAMETER;
@@ -443,7 +442,6 @@ static void setup_quirks(struct boot_params *boot_params)
 	first_uga = NULL;
 	nr_ugas = size / sizeof(u32);
 	for (i = 0; i < nr_ugas; i++) {
-		efi_guid_t pciio_proto = EFI_PCI_IO_PROTOCOL_GUID;
 		u32 w, h, depth, refresh;
 		void *pciio;
 		u32 handle = handles[i];
@@ -453,7 +451,8 @@ static void setup_quirks(struct boot_params *boot_params)
 		if (status != EFI_SUCCESS)
 			continue;
 
-		efi_call_early(handle_protocol, handle, &pciio_proto, &pciio);
+		efi_call_early(handle_protocol, handle,
+			       &EFI_PCI_IO_PROTOCOL_GUID, &pciio);
 
 		status = efi_early->call((unsigned long)uga->get_mode, uga,
 					 &w, &h, &depth, &refresh);
@@ -479,7 +478,7 @@ static void setup_quirks(struct boot_params *boot_params)
 setup_uga64(void **uga_handle, unsigned long size, u32 *width, u32 *height)
 {
 	struct efi_uga_draw_protocol *uga = NULL, *first_uga;
-	efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID;
+	const efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID;
 	unsigned long nr_ugas;
 	u64 *handles = (u64 *)uga_handle;;
 	efi_status_t status = EFI_INVALID_PARAMETER;
@@ -488,7 +487,6 @@ static void setup_quirks(struct boot_params *boot_params)
 	first_uga = NULL;
 	nr_ugas = size / sizeof(u64);
 	for (i = 0; i < nr_ugas; i++) {
-		efi_guid_t pciio_proto = EFI_PCI_IO_PROTOCOL_GUID;
 		u32 w, h, depth, refresh;
 		void *pciio;
 		u64 handle = handles[i];
@@ -498,7 +496,8 @@ static void setup_quirks(struct boot_params *boot_params)
 		if (status != EFI_SUCCESS)
 			continue;
 
-		efi_call_early(handle_protocol, handle, &pciio_proto, &pciio);
+		efi_call_early(handle_protocol, handle,
+			       &EFI_PCI_IO_PROTOCOL_GUID, &pciio);
 
 		status = efi_early->call((unsigned long)uga->get_mode, uga,
 					 &w, &h, &depth, &refresh);
@@ -523,8 +522,8 @@ static void setup_quirks(struct boot_params *boot_params)
 /*
  * See if we have Universal Graphics Adapter (UGA) protocol
  */
-static efi_status_t setup_uga(struct screen_info *si, efi_guid_t *uga_proto,
-			      unsigned long size)
+static efi_status_t setup_uga(struct screen_info *si,
+			      const efi_guid_t *uga_proto, unsigned long size)
 {
 	efi_status_t status;
 	u32 width, height;
@@ -575,9 +574,9 @@ static efi_status_t setup_uga(struct screen_info *si, efi_guid_t *uga_proto,
 
 void setup_graphics(struct boot_params *boot_params)
 {
-	efi_guid_t graphics_proto = EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID;
+	const efi_guid_t graphics_proto = EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID;
 	struct screen_info *si;
-	efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID;
+	const efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID;
 	efi_status_t status;
 	unsigned long size;
 	void **gop_handle = NULL;
@@ -618,7 +617,6 @@ struct boot_params *make_boot_params(struct efi_config *c)
 	struct setup_header *hdr;
 	efi_loaded_image_t *image;
 	void *options, *handle;
-	efi_guid_t proto = LOADED_IMAGE_PROTOCOL_GUID;
 	int options_size = 0;
 	efi_status_t status;
 	char *cmdline_ptr;
@@ -642,7 +640,7 @@ struct boot_params *make_boot_params(struct efi_config *c)
 		setup_boot_services32(efi_early);
 
 	status = efi_call_early(handle_protocol, handle,
-				&proto, (void *)&image);
+				&LOADED_IMAGE_PROTOCOL_GUID, (void *)&image);
 	if (status != EFI_SUCCESS) {
 		efi_printk(sys_table, "Failed to get handle for LOADED_IMAGE_PROTOCOL\n");
 		return NULL;
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index da72bfb..70d6722 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -71,12 +71,11 @@ efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg,
 	efi_file_io_interface_t *io;
 	efi_loaded_image_t *image = __image;
 	efi_file_handle_t *fh;
-	efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID;
 	efi_status_t status;
 	void *handle = (void *)(unsigned long)image->device_handle;
 
-	status = sys_table_arg->boottime->handle_protocol(handle,
-				 &fs_proto, (void **)&io);
+	status = efi_call_early(handle_protocol, handle,
+				&EFI_FILE_SYSTEM_GUID, (void **)&io);
 	if (status != EFI_SUCCESS) {
 		efi_printk(sys_table_arg, "Failed to handle fs_proto\n");
 		return status;
@@ -101,7 +100,7 @@ void efi_char16_printk(efi_system_table_t *sys_table_arg,
 
 static struct screen_info *setup_graphics(efi_system_table_t *sys_table_arg)
 {
-	efi_guid_t gop_proto = EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID;
+	const efi_guid_t gop_proto = EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID;
 	efi_status_t status;
 	unsigned long size;
 	void **gop_handle = NULL;
@@ -153,7 +152,6 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
 	char *cmdline_ptr = NULL;
 	int cmdline_size = 0;
 	unsigned long new_fdt_addr;
-	efi_guid_t loaded_image_proto = LOADED_IMAGE_PROTOCOL_GUID;
 	unsigned long reserve_addr = 0;
 	unsigned long reserve_size = 0;
 	int secure_boot = 0;
@@ -175,7 +173,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
 	 * line.
 	 */
 	status = sys_table->boottime->handle_protocol(handle,
-					&loaded_image_proto, (void *)&image);
+				&LOADED_IMAGE_PROTOCOL_GUID, (void *)&image);
 	if (status != EFI_SUCCESS) {
 		pr_efi_err(sys_table, "Failed to get loaded image protocol\n");
 		goto fail;
diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
index 932742e..9513a98 100644
--- a/drivers/firmware/efi/libstub/gop.c
+++ b/drivers/firmware/efi/libstub/gop.c
@@ -111,7 +111,7 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size)
 
 static efi_status_t
 setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
-            efi_guid_t *proto, unsigned long size, void **gop_handle)
+	    const efi_guid_t *proto, unsigned long size, void **gop_handle)
 {
 	struct efi_graphics_output_protocol_32 *gop32, *first_gop;
 	unsigned long nr_gops;
@@ -131,7 +131,6 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size)
 	nr_gops = size / sizeof(u32);
 	for (i = 0; i < nr_gops; i++) {
 		struct efi_graphics_output_mode_info *info = NULL;
-		efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID;
 		bool conout_found = false;
 		void *dummy = NULL;
 		efi_handle_t h = (efi_handle_t)(unsigned long)handles[i];
@@ -143,7 +142,7 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size)
 			continue;
 
 		status = efi_call_early(handle_protocol, h,
-					&conout_proto, &dummy);
+					&EFI_CONSOLE_OUT_DEVICE_GUID, &dummy);
 		if (status == EFI_SUCCESS)
 			conout_found = true;
 
@@ -228,7 +227,7 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size)
 
 static efi_status_t
 setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
-	    efi_guid_t *proto, unsigned long size, void **gop_handle)
+	    const efi_guid_t *proto, unsigned long size, void **gop_handle)
 {
 	struct efi_graphics_output_protocol_64 *gop64, *first_gop;
 	unsigned long nr_gops;
@@ -248,7 +247,6 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size)
 	nr_gops = size / sizeof(u64);
 	for (i = 0; i < nr_gops; i++) {
 		struct efi_graphics_output_mode_info *info = NULL;
-		efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID;
 		bool conout_found = false;
 		void *dummy = NULL;
 		efi_handle_t h = (efi_handle_t)(unsigned long)handles[i];
@@ -260,7 +258,7 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size)
 			continue;
 
 		status = efi_call_early(handle_protocol, h,
-					&conout_proto, &dummy);
+					&EFI_CONSOLE_OUT_DEVICE_GUID, &dummy);
 		if (status == EFI_SUCCESS)
 			conout_found = true;
 
@@ -323,7 +321,7 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size)
  * See if we have Graphics Output Protocol
  */
 efi_status_t efi_setup_gop(efi_system_table_t *sys_table_arg,
-			   struct screen_info *si, efi_guid_t *proto,
+			   struct screen_info *si, const efi_guid_t *proto,
 			   unsigned long size)
 {
 	efi_status_t status;
diff --git a/drivers/firmware/efi/libstub/random.c b/drivers/firmware/efi/libstub/random.c
index 7e72954..ffb1130 100644
--- a/drivers/firmware/efi/libstub/random.c
+++ b/drivers/firmware/efi/libstub/random.c
@@ -23,11 +23,10 @@ struct efi_rng_protocol {
 efi_status_t efi_get_random_bytes(efi_system_table_t *sys_table_arg,
 				  unsigned long size, u8 *out)
 {
-	efi_guid_t rng_proto = EFI_RNG_PROTOCOL_GUID;
 	efi_status_t status;
 	struct efi_rng_protocol *rng;
 
-	status = efi_call_early(locate_protocol, &rng_proto, NULL,
+	status = efi_call_early(locate_protocol, &EFI_RNG_PROTOCOL_GUID, NULL,
 				(void **)&rng);
 	if (status != EFI_SUCCESS)
 		return status;
@@ -149,14 +148,13 @@ efi_status_t efi_random_alloc(efi_system_table_t *sys_table_arg,
 
 efi_status_t efi_random_get_seed(efi_system_table_t *sys_table_arg)
 {
-	efi_guid_t rng_proto = EFI_RNG_PROTOCOL_GUID;
 	efi_guid_t rng_algo_raw = EFI_RNG_ALGORITHM_RAW;
 	efi_guid_t rng_table_guid = LINUX_EFI_RANDOM_SEED_TABLE_GUID;
 	struct efi_rng_protocol *rng;
 	struct linux_efi_random_seed *seed;
 	efi_status_t status;
 
-	status = efi_call_early(locate_protocol, &rng_proto, NULL,
+	status = efi_call_early(locate_protocol, &EFI_RNG_PROTOCOL_GUID, NULL,
 				(void **)&rng);
 	if (status != EFI_SUCCESS)
 		return status;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 6ade102..c4923c1f 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -293,10 +293,11 @@ struct efi_boot_memmap {
 	void *install_protocol_interface;
 	void *reinstall_protocol_interface;
 	void *uninstall_protocol_interface;
-	efi_status_t (*handle_protocol)(efi_handle_t, efi_guid_t *, void **);
+	efi_status_t (*handle_protocol)(efi_handle_t, const efi_guid_t *,
+					void **);
 	void *__reserved;
 	void *register_protocol_notify;
-	efi_status_t (*locate_handle)(int, efi_guid_t *, void *,
+	efi_status_t (*locate_handle)(int, const efi_guid_t *, void *,
 				      unsigned long *, efi_handle_t *);
 	void *locate_device_path;
 	efi_status_t (*install_configuration_table)(efi_guid_t *, void *);
@@ -315,7 +316,7 @@ struct efi_boot_memmap {
 	void *open_protocol_information;
 	void *protocols_per_handle;
 	void *locate_handle_buffer;
-	efi_status_t (*locate_protocol)(efi_guid_t *, void *, void **);
+	efi_status_t (*locate_protocol)(const efi_guid_t *, void *, void **);
 	void *install_multiple_protocol_interfaces;
 	void *uninstall_multiple_protocol_interfaces;
 	void *calculate_crc32;
@@ -1472,7 +1473,7 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
 efi_status_t efi_parse_options(char *cmdline);
 
 efi_status_t efi_setup_gop(efi_system_table_t *sys_table_arg,
-			   struct screen_info *si, efi_guid_t *proto,
+			   struct screen_info *si, const efi_guid_t *proto,
 			   unsigned long size);
 
 bool efi_runtime_disabled(void);
-- 
2.11.0

[PATCH 05/10] efi: Constify InstallConfigurationTable()

[Lukas Wunner]

GUIDs passed to InstallConfigurationTable() are not modified, so declare
them const.  This allows the compiler to place them in the rodata
section instead of generating them on the stack at runtime.  Pass GUIDs
as literals rather than assigning them to temporary variables to save a
bit on code.  Constification of the GUIDs is in compliance with the EFI
spec which marks them "IN" for this boot service.

Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 drivers/firmware/efi/libstub/arm32-stub.c | 2 +-
 drivers/firmware/efi/libstub/random.c     | 5 ++---
 include/linux/efi.h                       | 2 +-
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/libstub/arm32-stub.c b/drivers/firmware/efi/libstub/arm32-stub.c
index e1f0b28..1be40b6 100644
--- a/drivers/firmware/efi/libstub/arm32-stub.c
+++ b/drivers/firmware/efi/libstub/arm32-stub.c
@@ -26,7 +26,7 @@ efi_status_t check_platform_features(efi_system_table_t *sys_table_arg)
 	return EFI_SUCCESS;
 }
 
-static efi_guid_t screen_info_guid = LINUX_EFI_ARM_SCREEN_INFO_TABLE_GUID;
+static const efi_guid_t screen_info_guid = LINUX_EFI_ARM_SCREEN_INFO_TABLE_GUID;
 
 struct screen_info *alloc_screen_info(efi_system_table_t *sys_table_arg)
 {
diff --git a/drivers/firmware/efi/libstub/random.c b/drivers/firmware/efi/libstub/random.c
index ffb1130..c29a722 100644
--- a/drivers/firmware/efi/libstub/random.c
+++ b/drivers/firmware/efi/libstub/random.c
@@ -149,7 +149,6 @@ efi_status_t efi_random_alloc(efi_system_table_t *sys_table_arg,
 efi_status_t efi_random_get_seed(efi_system_table_t *sys_table_arg)
 {
 	efi_guid_t rng_algo_raw = EFI_RNG_ALGORITHM_RAW;
-	efi_guid_t rng_table_guid = LINUX_EFI_RANDOM_SEED_TABLE_GUID;
 	struct efi_rng_protocol *rng;
 	struct linux_efi_random_seed *seed;
 	efi_status_t status;
@@ -179,8 +178,8 @@ efi_status_t efi_random_get_seed(efi_system_table_t *sys_table_arg)
 		goto err_freepool;
 
 	seed->size = RANDOM_SEED_SIZE;
-	status = efi_call_early(install_configuration_table, &rng_table_guid,
-				seed);
+	status = efi_call_early(install_configuration_table,
+				&LINUX_EFI_RANDOM_SEED_TABLE_GUID, seed);
 	if (status != EFI_SUCCESS)
 		goto err_freepool;
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index c4923c1f..0c260f0 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -300,7 +300,7 @@ struct efi_boot_memmap {
 	efi_status_t (*locate_handle)(int, const efi_guid_t *, void *,
 				      unsigned long *, efi_handle_t *);
 	void *locate_device_path;
-	efi_status_t (*install_configuration_table)(efi_guid_t *, void *);
+	efi_status_t (*install_configuration_table)(const efi_guid_t *, void *);
 	void *load_image;
 	void *start_image;
 	void *exit;
-- 
2.11.0

[PATCH 06/10] efi: Constify EFI_FILE_PROTOCOL.GetInfo()

[Lukas Wunner]

GUIDs passed to EFI_FILE_PROTOCOL.GetInfo() are not modified, so declare
them const.  This allows the compiler to place them in the rodata
section instead of generating them on the stack at runtime.  Pass GUIDs
as literals rather than assigning them to temporary variables to save a
bit on code.  Constification of the GUIDs is in compliance with the EFI
spec which marks them "IN" for this protocol function.

Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 drivers/firmware/efi/libstub/efi-stub-helper.c | 5 ++---
 include/linux/efi.h                            | 2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 6ee9164..8e06307 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -345,7 +345,6 @@ static efi_status_t efi_file_size(efi_system_table_t *sys_table_arg, void *__fh,
 	efi_file_handle_t *h, *fh = __fh;
 	efi_file_info_t *info;
 	efi_status_t status;
-	efi_guid_t info_guid = EFI_FILE_INFO_ID;
 	unsigned long info_sz;
 
 	status = efi_call_proto(efi_file_handle, open, fh, &h, filename_16,
@@ -360,7 +359,7 @@ static efi_status_t efi_file_size(efi_system_table_t *sys_table_arg, void *__fh,
 	*handle = h;
 
 	info_sz = 0;
-	status = efi_call_proto(efi_file_handle, get_info, h, &info_guid,
+	status = efi_call_proto(efi_file_handle, get_info, h, &EFI_FILE_INFO_ID,
 				&info_sz, NULL);
 	if (status != EFI_BUFFER_TOO_SMALL) {
 		efi_printk(sys_table_arg, "Failed to get file info size\n");
@@ -375,7 +374,7 @@ static efi_status_t efi_file_size(efi_system_table_t *sys_table_arg, void *__fh,
 		return status;
 	}
 
-	status = efi_call_proto(efi_file_handle, get_info, h, &info_guid,
+	status = efi_call_proto(efi_file_handle, get_info, h, &EFI_FILE_INFO_ID,
 				&info_sz, info);
 	if (status == EFI_BUFFER_TOO_SMALL) {
 		efi_call_early(free_pool, info);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 0c260f0..6ed28e1 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -838,7 +838,7 @@ struct efi_fdt_params {
 	void *write;
 	void *get_position;
 	void *set_position;
-	efi_status_t (*get_info)(struct _efi_file_handle *, efi_guid_t *,
+	efi_status_t (*get_info)(struct _efi_file_handle *, const efi_guid_t *,
 			unsigned long *, void *);
 	void *set_info;
 	void *flush;
-- 
2.11.0

[PATCH 07/10] efi: Constify EFI_RNG_PROTOCOL.GetRNG()

[Lukas Wunner]

GUIDs passed to EFI_RNG_PROTOCOL.GetRNG() are not modified, so declare
them const.  This allows the compiler to place them in the rodata
section instead of generating them on the stack at runtime.  Pass GUIDs
as literals rather than assigning them to temporary variables to save a
bit on code.  Constification of the GUIDs is in compliance with the EFI
spec which marks them "IN" for this protocol function.

Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 drivers/firmware/efi/libstub/random.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/efi/libstub/random.c b/drivers/firmware/efi/libstub/random.c
index c29a722..844bdc9 100644
--- a/drivers/firmware/efi/libstub/random.c
+++ b/drivers/firmware/efi/libstub/random.c
@@ -17,7 +17,7 @@ struct efi_rng_protocol {
 	efi_status_t (*get_info)(struct efi_rng_protocol *,
 				 unsigned long *, efi_guid_t *);
 	efi_status_t (*get_rng)(struct efi_rng_protocol *,
-				efi_guid_t *, unsigned long, u8 *out);
+				const efi_guid_t *, unsigned long, u8 *out);
 };
 
 efi_status_t efi_get_random_bytes(efi_system_table_t *sys_table_arg,
@@ -148,7 +148,6 @@ efi_status_t efi_random_alloc(efi_system_table_t *sys_table_arg,
 
 efi_status_t efi_random_get_seed(efi_system_table_t *sys_table_arg)
 {
-	efi_guid_t rng_algo_raw = EFI_RNG_ALGORITHM_RAW;
 	struct efi_rng_protocol *rng;
 	struct linux_efi_random_seed *seed;
 	efi_status_t status;
@@ -164,7 +163,7 @@ efi_status_t efi_random_get_seed(efi_system_table_t *sys_table_arg)
 	if (status != EFI_SUCCESS)
 		return status;
 
-	status = rng->get_rng(rng, &rng_algo_raw, RANDOM_SEED_SIZE,
+	status = rng->get_rng(rng, &EFI_RNG_ALGORITHM_RAW, RANDOM_SEED_SIZE,
 			      seed->bits);
 	if (status == EFI_UNSUPPORTED)
 		/*
-- 
2.11.0

[PATCH 08/10] efi: Constify EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.OutputString()

[Lukas Wunner]

UCS-2 strings passed to EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.OutputString()
are not modified, so can be declared const.  This allows the compiler to
place them in the rodata section instead of generating them on the stack
at runtime, such as with the '\r' string passed from efi_printk() to
efi_char16_printk().  Constification of such strings is in compliance
with the EFI spec which marks them "IN" for this protocol function.

Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 arch/x86/boot/compressed/eboot.c               | 2 +-
 drivers/firmware/efi/libstub/arm-stub.c        | 2 +-
 drivers/firmware/efi/libstub/efi-stub-helper.c | 2 +-
 drivers/firmware/efi/libstub/efistub.h         | 2 +-
 include/linux/efi.h                            | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 61014f5..fee5693 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -97,7 +97,7 @@ static inline efi_status_t __open_volume64(void *__image, void **__fh)
 	return __open_volume32(__image, __fh);
 }
 
-void efi_char16_printk(efi_system_table_t *table, efi_char16_t *str)
+void efi_char16_printk(efi_system_table_t *table, const efi_char16_t *str)
 {
 	efi_call_proto(efi_simple_text_output_protocol, output_string,
 		       efi_early->text_output, str);
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index 70d6722..1053ec2 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -90,7 +90,7 @@ efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg,
 }
 
 void efi_char16_printk(efi_system_table_t *sys_table_arg,
-			      efi_char16_t *str)
+		       const efi_char16_t *str)
 {
 	struct efi_simple_text_output_protocol *out;
 
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 8e06307..a9b6e4d 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -48,7 +48,7 @@ void efi_printk(efi_system_table_t *sys_table_arg, char *str)
 
 		ch[0] = *s8;
 		if (*s8 == '\n') {
-			efi_char16_t nl[2] = { '\r', 0 };
+			const efi_char16_t nl[2] = { '\r', 0 };
 			efi_char16_printk(sys_table_arg, nl);
 		}
 
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 2fe7746..12dee6b 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -24,7 +24,7 @@
 #define EFI_ALLOC_ALIGN		EFI_PAGE_SIZE
 #endif
 
-void efi_char16_printk(efi_system_table_t *, efi_char16_t *);
+void efi_char16_printk(efi_system_table_t *, const efi_char16_t *);
 
 efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg, void *__image,
 			     void **__fh);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 6ed28e1..24a3e55 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1257,7 +1257,7 @@ struct efivar_entry {
 
 struct efi_simple_text_output_protocol {
 	void *reset;
-	efi_status_t (*output_string)(void *, void *);
+	efi_status_t (*output_string)(void *, const void *);
 	void *test_string;
 };
 
-- 
2.11.0

[PATCH 09/10] efi: Constify efi_guidcmp() arguments

[Lukas Wunner]

GUIDs passed to efi_guidcmp() are not modified, so declare them const.
This allows the compiler to place them in the rodata section instead of
generating them on the stack at runtime.  Pass GUIDs as literals rather
than assigning them to temporary variables to save a bit on code.

Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Anton Vorontsov <anton-9xeibp6oKSgdnm+yROfE0A@public.gmane.org>
Cc: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Tony Luck <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 drivers/firmware/efi/efi-pstore.c  | 6 ++----
 drivers/firmware/efi/libstub/fdt.c | 3 +--
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index 0911449..e7c9fd1 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -46,7 +46,6 @@ static inline u64 generic_id(unsigned long timestamp,
 
 static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
 {
-	efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
 	struct pstore_read_data *cb_data = data;
 	char name[DUMP_NAME_LEN], data_type;
 	int i;
@@ -54,7 +53,7 @@ static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
 	unsigned int part;
 	unsigned long time, size;
 
-	if (efi_guidcmp(entry->var.VendorGuid, vendor))
+	if (efi_guidcmp(entry->var.VendorGuid, LINUX_EFI_CRASH_GUID))
 		return 0;
 
 	for (i = 0; i < DUMP_NAME_LEN; i++)
@@ -299,14 +298,13 @@ struct pstore_erase_data {
 static int efi_pstore_erase_func(struct efivar_entry *entry, void *data)
 {
 	struct pstore_erase_data *ed = data;
-	efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
 	efi_char16_t efi_name_old[DUMP_NAME_LEN];
 	efi_char16_t *efi_name = ed->name;
 	unsigned long ucs2_len = ucs2_strlen(ed->name);
 	char name_old[DUMP_NAME_LEN];
 	int i;
 
-	if (efi_guidcmp(entry->var.VendorGuid, vendor))
+	if (efi_guidcmp(entry->var.VendorGuid, LINUX_EFI_CRASH_GUID))
 		return 0;
 
 	if (ucs2_strncmp(entry->var.VariableName,
diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index a6a9311..6ebc3bb 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -332,7 +332,6 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 
 void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
 {
-	efi_guid_t fdt_guid = DEVICE_TREE_GUID;
 	efi_config_table_t *tables;
 	void *fdt;
 	int i;
@@ -341,7 +340,7 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
 	fdt = NULL;
 
 	for (i = 0; i < sys_table->nr_tables; i++)
-		if (efi_guidcmp(tables[i].guid, fdt_guid) == 0) {
+		if (efi_guidcmp(tables[i].guid, DEVICE_TREE_GUID) == 0) {
 			fdt = (void *) tables[i].table;
 			if (fdt_check_header(fdt) != 0) {
 				pr_efi_err(sys_table, "Invalid header detected on UEFI supplied FDT, ignoring ...\n");
-- 
2.11.0

[PATCH 10/10] uuid: Constify UUID compound literals

[Lukas Wunner]

The UUID_BE() and UUID_LE() macros (as well as the derived EFI_GUID())
generate a compound literal for a 128-bit ID.  By default, these are not
stored in the rodata section but generated on the stack at runtime in a
very time- and space-inefficient manner.

E.g. EFI_FILE_SYSTEM_GUID (964E5B22-6459-11D2-8E39-00A0C969723B) results
in x86_64 code which requires 80 bytes text instead of 16 bytes rodata:

    c6 44 24 20 22                   movb   $0x22,0x20(%rsp)
    c6 44 24 21 5b                   movb   $0x5b,0x21(%rsp)
    c6 44 24 22 4e                   movb   $0x4e,0x22(%rsp)
    c6 44 24 23 96                   movb   $0x96,0x23(%rsp)
    c6 44 24 24 59                   movb   $0x59,0x24(%rsp)
    c6 44 24 25 64                   movb   $0x64,0x25(%rsp)
    c6 44 24 26 d2                   movb   $0xd2,0x26(%rsp)
    c6 44 24 27 11                   movb   $0x11,0x27(%rsp)
    c6 44 24 28 8e                   movb   $0x8e,0x28(%rsp)
    c6 44 24 29 39                   movb   $0x39,0x29(%rsp)
    c6 44 24 2a 00                   movb   $0x0,0x2a(%rsp)
    c6 44 24 2b a0                   movb   $0xa0,0x2b(%rsp)
    c6 44 24 2c c9                   movb   $0xc9,0x2c(%rsp)
    c6 44 24 2d 69                   movb   $0x69,0x2d(%rsp)
    c6 44 24 2e 72                   movb   $0x72,0x2e(%rsp)
    c6 44 24 2f 3b                   movb   $0x3b,0x2f(%rsp)

gcc 7 improves on this by using 2x movabs + 2x mov instructions,
amounting to 30 bytes text instead of 16 bytes rodata.  That's more
space-efficient than 80 bytes but still undesirable from a performance
and security perspective:

    48 b8 22 5b 4e 96 59 64 d2 11    movabs $0x11d26459964e5b,%rax
    48 89 44 24 20                   mov    %rax,0x20(%rsp)
    48 b8 8e 39 00 a0 c9 69 72 3b    movabs $0x3b7269c9a00039,%rax
    48 89 44 24 28                   mov    %rax,0x28(%rsp)

The reason for generating compound literals on the stack is that they're
allowed to be modified.  E.g. a pointer to a UUID might be used to
change some of its 16 bytes.  In the context of UUIDs this seems silly
though:  The uuid_be, uuid_le and efi_guid_t data types hide the
distinct bytes making up the ID, the bytes are not meant to be modified.
Moreover, the UUIDs are usually pre-defined in a spec such as UEFI and
there's no reason at all to modify them.

Thus, change the UUID_BE and UUID_LE macros to declare the generated
compound literal const, allowing the compiler to put it in rodata.

When using the compound literal as an initializer for a variable, that
variable has to be declared const as well for the UUID to become rodata.

Is that all?  Unfortunately not.  Under certain conditions gcc refuses
to store compound literals in rodata despite them being declared const.
Rasmus Villemoes opened a bugzilla entry for this in December 2015:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68725

The upshot is that if a UUID is passed by value to a function (either
directly or as a variable), it is stored in rodata.  This is the case
for efi_guidcmp().  But if the UUID is passed by reference, it is
generated on the stack, in spite of it being declared const and the
function parameter being declared const.  This is the case for the
various EFI boot and runtime services calls such as GetVariable.
A workaround is to declare a *static* const variable, assign the UUID to
it and pass the variable by reference to the function.

This issue also affects efi_char16_t strings which are only declared
const (and not static const).  They're likewise generated on the stack
at runtime.

clang 3.9 is not better than gcc:  It does the exact opposite by putting
UUIDs in rodata if they're passed by reference, and generating them on
the stack if they're passed by value.

Cc: Huang Ying <ying.huang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Rasmus Villemoes <linux-qQsb+v5E8BnlAoU/VqSP6n9LOBIZ5rWg@public.gmane.org>
Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 include/uapi/linux/uuid.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/uuid.h b/include/uapi/linux/uuid.h
index 3738e5f..9184f01 100644
--- a/include/uapi/linux/uuid.h
+++ b/include/uapi/linux/uuid.h
@@ -29,14 +29,14 @@
 } uuid_be;
 
 #define UUID_LE(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)		\
-((uuid_le)								\
+((const uuid_le)							\
 {{ (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, ((a) >> 24) & 0xff, \
    (b) & 0xff, ((b) >> 8) & 0xff,					\
    (c) & 0xff, ((c) >> 8) & 0xff,					\
    (d0), (d1), (d2), (d3), (d4), (d5), (d6), (d7) }})
 
 #define UUID_BE(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)		\
-((uuid_be)								\
+((const uuid_be)							\
 {{ ((a) >> 24) & 0xff, ((a) >> 16) & 0xff, ((a) >> 8) & 0xff, (a) & 0xff, \
    ((b) >> 8) & 0xff, (b) & 0xff,					\
    ((c) >> 8) & 0xff, (c) & 0xff,					\
-- 
2.11.0

Re: [PATCH 02/10] efi: Constify GetVariable() / SetVariable() signatures

[Juergen Gross]

On 02/01/17 11:24, Lukas Wunner wrote:
> GetVariable() / SetVariable() is typically called with immutable
> VendorGuid and VariableName arguments.  SetVariable() is also often
> called with an immutable Data argument.  However declaring these const
> in the caller results in a compiler warning since they're not marked
> const in the function signatures.  Rectify that, in keeping with the EFI
> spec which marks these arguments "IN".
> 
> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Tony Luck <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Fenghua Yu <fenghua.yu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Boris Ostrovsky <boris.ostrovsky-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Cc: David Vrabel <david.vrabel-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
> Cc: Juergen Gross <jgross-IBi9RG/b67k@public.gmane.org>
> Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>

Xen parts:

Acked-by: Juergen Gross <jgross-IBi9RG/b67k@public.gmane.org>


Juergen

Re: [PATCH 03/10] efi: Constify GetVariable() / SetVariable() arguments

[Dennis Dalessandro]

For the hfi1 bits below:

Acked-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

On 01/02/2017 05:24 AM, Lukas Wunner wrote:
> iff --git a/drivers/infiniband/hw/hfi1/efivar.c b/drivers/infiniband/hw/hfi1/efivar.c
> index 106349f..f769cc2 100644
> --- a/drivers/infiniband/hw/hfi1/efivar.c
> +++ b/drivers/infiniband/hw/hfi1/efivar.c
> @@ -66,7 +66,6 @@ static int read_efi_var(const char *name, unsigned long *size,
>  {
>  	efi_status_t status;
>  	efi_char16_t *uni_name;
> -	efi_guid_t guid;
>  	unsigned long temp_size;
>  	void *temp_buffer;
>  	void *data;
> @@ -95,13 +94,10 @@ static int read_efi_var(const char *name, unsigned long *size,
>  	for (i = 0; name[i]; i++)
>  		uni_name[i] = name[i];
>
> -	/* need a variable for our GUID */
> -	guid = HFI1_EFIVAR_GUID;
> -
>  	/* call into EFI runtime services */
>  	status = efi.get_variable(
>  			uni_name,
> -			&guid,
> +			&HFI1_EFIVAR_GUID,
>  			NULL,
>  			&temp_size,
>  			temp_buffer);

Re: [PATCH 00/10] efi: Constify all the things

[Kees Cook]

On Mon, Jan 2, 2017 at 2:24 AM, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
> When disassembling the EFI stub I noticed that GUIDs and efi_char16_t
> strings are not placed in rodata but generated on the stack at runtime.
> GUIDs occupy up to 80 bytes of text instead of just 16 bytes rodata
> which annoyed me enough to go down the rabbit hole of fixing it.
> This series constifies all GUIDs and EFI-related strings I could find.
>
> The most important patch is [10/10] to constify the EFI_GUID() macro.
> This has to be at the end of the series to avoid compiler warnings.
> Reviewers may want to read its commit message first as it contains
> background information.  In particular it explains why this series
> only fixes the kernel side of things.  Additional work is necessary
> on the compiler side to actually have all the GUIDs and strings in
> rodata.  At the moment both gcc and clang achieve that only partially.
>
> Ard Biesheuvel expressed an interest in these patches in November but
> stressed the importance that arguments to EFI boot/runtime/protocol
> services may only be declared const if the spec marks them "IN":
> https://lkml.org/lkml/2016/11/21/459
>
> Hence I've split the series by boot/runtime/protocol services to allow
> for easy verification of this fact.  Constification of GetVariable() /
> SetVariable() is further split in one patch for the function signatures
> and another for the actual arguments because changing the function
> signatures is quite involved in this case.
>
> efi.git/next needs to be rebased on v4.10-rc1 before this series can be
> applied because it requires commits f6697df36bdf ("x86/efi: Prevent mixed
> mode boot corruption with CONFIG_VMAP_STACK=y") and 2fbadc3002c5
> ("arm/arm64: xen: Move shared architecture headers to include/xen/arm").
>
> Unfortunately I was not able to compile-test on ia64, there's no cross-
> compiler package available on Debian. :-(  I did not receive complaints
> from 0-day, so I'm cautiously optimistic that I didn't cause breakage
> there.
>
> I've pushed the patches to GitHub to ease reviewing/fetching:
> https://github.com/l1k/linux/commits/efi_const_v1
>
> Thanks,
>
> Lukas
>
>
> Ard Biesheuvel (1):
>   efi: use typed function pointers for runtime services table
>
> Lukas Wunner (9):
>   efi: Constify GetVariable() / SetVariable() signatures
>   efi: Constify GetVariable() / SetVariable() arguments
>   efi: Constify HandleProtocol() / LocateHandle() / LocateProtocol()
>   efi: Constify InstallConfigurationTable()
>   efi: Constify EFI_FILE_PROTOCOL.GetInfo()
>   efi: Constify EFI_RNG_PROTOCOL.GetRNG()
>   efi: Constify EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.OutputString()
>   efi: Constify efi_guidcmp() arguments
>   uuid: Constify UUID compound literals

Cool! Thanks for doing this!

Reviewed-by: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

-Kees

-- 
Kees Cook
Nexus Security

Re: [PATCH 00/10] efi: Constify all the things

[Ard Biesheuvel]

Hi Lukas,

Thanks for putting this together.

On 2 January 2017 at 10:24, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
> When disassembling the EFI stub I noticed that GUIDs and efi_char16_t
> strings are not placed in rodata but generated on the stack at runtime.
> GUIDs occupy up to 80 bytes of text instead of just 16 bytes rodata
> which annoyed me enough to go down the rabbit hole of fixing it.
> This series constifies all GUIDs and EFI-related strings I could find.
>
> The most important patch is [10/10] to constify the EFI_GUID() macro.
> This has to be at the end of the series to avoid compiler warnings.
> Reviewers may want to read its commit message first as it contains
> background information.  In particular it explains why this series
> only fixes the kernel side of things.  Additional work is necessary
> on the compiler side to actually have all the GUIDs and strings in
> rodata.  At the moment both gcc and clang achieve that only partially.
>
> Ard Biesheuvel expressed an interest in these patches in November but
> stressed the importance that arguments to EFI boot/runtime/protocol
> services may only be declared const if the spec marks them "IN":
> https://lkml.org/lkml/2016/11/21/459
>

Indeed. I think it is reasonable to infer constness from the IN (and
not OUT) annotation of by-reference arguments. In general, I think the
complete lack of distinction between read-write and read-execute is a
huge oversight, and anything that improves the situation on either
side of the firmware-OS boundary is a welcome change IMO.

> Hence I've split the series by boot/runtime/protocol services to allow
> for easy verification of this fact.  Constification of GetVariable() /
> SetVariable() is further split in one patch for the function signatures
> and another for the actual arguments because changing the function
> signatures is quite involved in this case.
>
> efi.git/next needs to be rebased on v4.10-rc1 before this series can be
> applied because it requires commits f6697df36bdf ("x86/efi: Prevent mixed
> mode boot corruption with CONFIG_VMAP_STACK=y") and 2fbadc3002c5
> ("arm/arm64: xen: Move shared architecture headers to include/xen/arm").
>
> Unfortunately I was not able to compile-test on ia64, there's no cross-
> compiler package available on Debian. :-(  I did not receive complaints
> from 0-day, so I'm cautiously optimistic that I didn't cause breakage
> there.
>
> I've pushed the patches to GitHub to ease reviewing/fetching:
> https://github.com/l1k/linux/commits/efi_const_v1
>
> Thanks,
>
> Lukas
>
>
> Ard Biesheuvel (1):
>   efi: use typed function pointers for runtime services table
>
> Lukas Wunner (9):
>   efi: Constify GetVariable() / SetVariable() signatures
>   efi: Constify GetVariable() / SetVariable() arguments
>   efi: Constify HandleProtocol() / LocateHandle() / LocateProtocol()
>   efi: Constify InstallConfigurationTable()
>   efi: Constify EFI_FILE_PROTOCOL.GetInfo()
>   efi: Constify EFI_RNG_PROTOCOL.GetRNG()
>   efi: Constify EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL.OutputString()
>   efi: Constify efi_guidcmp() arguments
>   uuid: Constify UUID compound literals
>
>  arch/ia64/kernel/efi.c                         | 24 ++++------
>  arch/x86/boot/compressed/eboot.c               | 50 ++++++++++-----------
>  arch/x86/include/asm/xen/interface.h           |  1 +
>  arch/x86/platform/efi/efi_64.c                 | 18 ++++----
>  arch/x86/platform/efi/quirks.c                 |  2 +-
>  drivers/firmware/efi/efi-pstore.c              | 10 ++---
>  drivers/firmware/efi/efibc.c                   |  3 +-
>  drivers/firmware/efi/libstub/arm-stub.c        | 17 +++----
>  drivers/firmware/efi/libstub/arm32-stub.c      |  2 +-
>  drivers/firmware/efi/libstub/efi-stub-helper.c |  7 ++-
>  drivers/firmware/efi/libstub/efistub.h         |  2 +-
>  drivers/firmware/efi/libstub/fdt.c             |  3 +-
>  drivers/firmware/efi/libstub/gop.c             | 12 +++--
>  drivers/firmware/efi/libstub/random.c          | 16 +++----
>  drivers/firmware/efi/runtime-wrappers.c        | 15 ++++---
>  drivers/firmware/google/gsmi.c                 | 10 ++---
>  drivers/infiniband/hw/hfi1/efivar.c            |  6 +--
>  drivers/scsi/isci/probe_roms.c                 |  2 +-
>  drivers/xen/efi.c                              | 12 ++---
>  include/linux/efi.h                            | 61 ++++++++++++++------------
>  include/uapi/linux/uuid.h                      |  4 +-
>  include/xen/arm/interface.h                    |  1 +
>  include/xen/interface/platform.h               | 11 ++++-
>  include/xen/xen-ops.h                          | 12 ++---
>  24 files changed, 143 insertions(+), 158 deletions(-)
>
> --
> 2.11.0
>

Re: [PATCH 03/10] efi: Constify GetVariable() / SetVariable() arguments

[Ard Biesheuvel]

On 2 January 2017 at 10:24, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
> GUIDs and variable names passed to GetVariable() / SetVariable() are not
> modified, so declare them const.  This allows the compiler to place them
> in the rodata section instead of generating them on the stack at
> runtime.  Pass GUIDs as literals rather than assigning them to temporary
> variables to save a bit on code.
>
> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Tony Luck <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Fenghua Yu <fenghua.yu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Anton Vorontsov <anton-9xeibp6oKSgdnm+yROfE0A@public.gmane.org>
> Cc: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Artur Paszkiewicz <artur.paszkiewicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> ---
>  arch/ia64/kernel/efi.c                  | 15 ++++-----------
>  arch/x86/platform/efi/quirks.c          |  2 +-
>  drivers/firmware/efi/efi-pstore.c       |  4 ++--
>  drivers/firmware/efi/efibc.c            |  3 +--
>  drivers/firmware/efi/libstub/arm-stub.c |  5 ++---
>  drivers/infiniband/hw/hfi1/efivar.c     |  6 +-----
>  drivers/scsi/isci/probe_roms.c          |  2 +-
>  7 files changed, 12 insertions(+), 25 deletions(-)
>
> diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
> index f57d089..f1264ab 100644
> --- a/arch/ia64/kernel/efi.c
> +++ b/arch/ia64/kernel/efi.c
> @@ -919,22 +919,15 @@ static void __init handle_palo(unsigned long phys_addr)
>  efi_uart_console_only(void)
>  {
>         efi_status_t status;
> -       char *s, name[] = "ConOut";
> -       efi_guid_t guid = EFI_GLOBAL_VARIABLE_GUID;
> -       efi_char16_t *utf16, name_utf16[32];
> +       const char name[] = "ConOut";
> +       const efi_char16_t name_utf16[] = { 'C', 'o', 'n', 'O', 'u', 't', 0 };

I know you mention 'static' in 10/10, but I don't understand why you
don't make these static to begin with. That also allows you to
annotate them as __initconst, which is not possible for compound
initializers (including the GUID)

>         unsigned char data[1024];
>         unsigned long size = sizeof(data);
>         struct efi_generic_dev_path *hdr, *end_addr;
>         int uart = 0;
>
> -       /* Convert to UTF-16 */
> -       utf16 = name_utf16;
> -       s = name;
> -       while (*s)
> -               *utf16++ = *s++ & 0x7f;
> -       *utf16 = 0;
> -
> -       status = efi.get_variable(name_utf16, &guid, NULL, &size, data);
> +       status = efi.get_variable(name_utf16, &EFI_GLOBAL_VARIABLE_GUID,
> +                                 NULL, &size, data);
>         if (status != EFI_SUCCESS) {
>                 printk(KERN_ERR "No EFI %s variable?\n", name);
>                 return 0;
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 10aca63..aad3701 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -19,7 +19,7 @@
>  #define EFI_DUMMY_GUID \
>         EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e, 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92, 0xa9)
>
> -static efi_char16_t efi_dummy_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };
> +static const efi_char16_t efi_dummy_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };
>
>  static bool efi_no_storage_paranoia;
>
> diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
> index f402ba2..0911449 100644
> --- a/drivers/firmware/efi/efi-pstore.c
> +++ b/drivers/firmware/efi/efi-pstore.c
> @@ -265,7 +265,6 @@ static int efi_pstore_write(enum pstore_type_id type,
>  {
>         char name[DUMP_NAME_LEN];
>         efi_char16_t efi_name[DUMP_NAME_LEN];
> -       efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
>         int i, ret = 0;
>
>         sprintf(name, "dump-type%u-%u-%d-%lu-%c", type, part, count,
> @@ -274,7 +273,8 @@ static int efi_pstore_write(enum pstore_type_id type,
>         for (i = 0; i < DUMP_NAME_LEN; i++)
>                 efi_name[i] = name[i];
>
> -       efivar_entry_set_safe(efi_name, vendor, PSTORE_EFI_ATTRIBUTES,
> +       efivar_entry_set_safe(efi_name, LINUX_EFI_CRASH_GUID,
> +                             PSTORE_EFI_ATTRIBUTES,
>                               !pstore_cannot_block_path(reason),
>                               size, psi->buf);
>
> diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
> index 503bbe2..57ddf8c 100644
> --- a/drivers/firmware/efi/efibc.c
> +++ b/drivers/firmware/efi/efibc.c
> @@ -32,7 +32,6 @@ static void efibc_str_to_str16(const char *str, efi_char16_t *str16)
>  static int efibc_set_variable(const char *name, const char *value)
>  {
>         int ret;
> -       efi_guid_t guid = LINUX_EFI_LOADER_ENTRY_GUID;
>         struct efivar_entry *entry;
>         size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);
>
> @@ -49,7 +48,7 @@ static int efibc_set_variable(const char *name, const char *value)
>
>         efibc_str_to_str16(name, entry->var.VariableName);
>         efibc_str_to_str16(value, (efi_char16_t *)entry->var.Data);
> -       memcpy(&entry->var.VendorGuid, &guid, sizeof(guid));
> +       entry->var.VendorGuid = LINUX_EFI_LOADER_ENTRY_GUID;
>
>         ret = efivar_entry_set(entry,
>                                EFI_VARIABLE_NON_VOLATILE
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index 6fca48c..da72bfb 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -27,13 +27,12 @@ static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
>         static efi_char16_t const sm_var_name[] = {
>                 'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
>
> -       efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
>         efi_get_variable_t *f_getvar = sys_table_arg->runtime->get_variable;
>         u8 val;
>         unsigned long size = sizeof(val);
>         efi_status_t status;
>
> -       status = f_getvar((efi_char16_t *)sb_var_name, (efi_guid_t *)&var_guid,
> +       status = f_getvar(sb_var_name, &EFI_GLOBAL_VARIABLE_GUID,
>                           NULL, &size, &val);
>
>         if (status != EFI_SUCCESS)
> @@ -42,7 +41,7 @@ static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
>         if (val == 0)
>                 return 0;
>
> -       status = f_getvar((efi_char16_t *)sm_var_name, (efi_guid_t *)&var_guid,
> +       status = f_getvar(sm_var_name, &EFI_GLOBAL_VARIABLE_GUID,
>                           NULL, &size, &val);
>
>         if (status != EFI_SUCCESS)
> diff --git a/drivers/infiniband/hw/hfi1/efivar.c b/drivers/infiniband/hw/hfi1/efivar.c
> index 106349f..f769cc2 100644
> --- a/drivers/infiniband/hw/hfi1/efivar.c
> +++ b/drivers/infiniband/hw/hfi1/efivar.c
> @@ -66,7 +66,6 @@ static int read_efi_var(const char *name, unsigned long *size,
>  {
>         efi_status_t status;
>         efi_char16_t *uni_name;
> -       efi_guid_t guid;
>         unsigned long temp_size;
>         void *temp_buffer;
>         void *data;
> @@ -95,13 +94,10 @@ static int read_efi_var(const char *name, unsigned long *size,
>         for (i = 0; name[i]; i++)
>                 uni_name[i] = name[i];
>
> -       /* need a variable for our GUID */
> -       guid = HFI1_EFIVAR_GUID;
> -
>         /* call into EFI runtime services */
>         status = efi.get_variable(
>                         uni_name,
> -                       &guid,
> +                       &HFI1_EFIVAR_GUID,
>                         NULL,
>                         &temp_size,
>                         temp_buffer);
> diff --git a/drivers/scsi/isci/probe_roms.c b/drivers/scsi/isci/probe_roms.c
> index 8ac646e..d2c17c1 100644
> --- a/drivers/scsi/isci/probe_roms.c
> +++ b/drivers/scsi/isci/probe_roms.c
> @@ -34,7 +34,7 @@
>  #include "task.h"
>  #include "probe_roms.h"
>
> -static efi_char16_t isci_efivar_name[] = {
> +static const efi_char16_t isci_efivar_name[] = {
>         'R', 's', 't', 'S', 'c', 'u', 'O'
>  };
>
> --
> 2.11.0
>

Re: [PATCH 10/10] uuid: Constify UUID compound literals

[Ard Biesheuvel]

Hi Lukas,

On 2 January 2017 at 10:24, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
> The UUID_BE() and UUID_LE() macros (as well as the derived EFI_GUID())
> generate a compound literal for a 128-bit ID.  By default, these are not
> stored in the rodata section but generated on the stack at runtime in a
> very time- and space-inefficient manner.
>

This is not true if you declare those variables static const (in
function scope).

In general, I think we should avoid touching uapi headers for this, so
I would like to explore if we can reach the same result without this
patch. Another objection I have to this patch is that it is context
dependent whether the UUID_[LE_BE]() macros in fact generate compound
literals: this is true for an assignment to a pointer variable, but
when they are used in initializers this is not the case, and so adding
'const' to their definitions is not always appropriate imo.

Are there any instances where simply turning those automatic variables
into static const doesn't work?

> E.g. EFI_FILE_SYSTEM_GUID (964E5B22-6459-11D2-8E39-00A0C969723B) results
> in x86_64 code which requires 80 bytes text instead of 16 bytes rodata:
>
>     c6 44 24 20 22                   movb   $0x22,0x20(%rsp)
>     c6 44 24 21 5b                   movb   $0x5b,0x21(%rsp)
>     c6 44 24 22 4e                   movb   $0x4e,0x22(%rsp)
>     c6 44 24 23 96                   movb   $0x96,0x23(%rsp)
>     c6 44 24 24 59                   movb   $0x59,0x24(%rsp)
>     c6 44 24 25 64                   movb   $0x64,0x25(%rsp)
>     c6 44 24 26 d2                   movb   $0xd2,0x26(%rsp)
>     c6 44 24 27 11                   movb   $0x11,0x27(%rsp)
>     c6 44 24 28 8e                   movb   $0x8e,0x28(%rsp)
>     c6 44 24 29 39                   movb   $0x39,0x29(%rsp)
>     c6 44 24 2a 00                   movb   $0x0,0x2a(%rsp)
>     c6 44 24 2b a0                   movb   $0xa0,0x2b(%rsp)
>     c6 44 24 2c c9                   movb   $0xc9,0x2c(%rsp)
>     c6 44 24 2d 69                   movb   $0x69,0x2d(%rsp)
>     c6 44 24 2e 72                   movb   $0x72,0x2e(%rsp)
>     c6 44 24 2f 3b                   movb   $0x3b,0x2f(%rsp)
>
> gcc 7 improves on this by using 2x movabs + 2x mov instructions,
> amounting to 30 bytes text instead of 16 bytes rodata.  That's more
> space-efficient than 80 bytes but still undesirable from a performance
> and security perspective:
>
>     48 b8 22 5b 4e 96 59 64 d2 11    movabs $0x11d26459964e5b,%rax
>     48 89 44 24 20                   mov    %rax,0x20(%rsp)
>     48 b8 8e 39 00 a0 c9 69 72 3b    movabs $0x3b7269c9a00039,%rax
>     48 89 44 24 28                   mov    %rax,0x28(%rsp)
>
> The reason for generating compound literals on the stack is that they're
> allowed to be modified.  E.g. a pointer to a UUID might be used to
> change some of its 16 bytes.  In the context of UUIDs this seems silly
> though:  The uuid_be, uuid_le and efi_guid_t data types hide the
> distinct bytes making up the ID, the bytes are not meant to be modified.
> Moreover, the UUIDs are usually pre-defined in a spec such as UEFI and
> there's no reason at all to modify them.
>
> Thus, change the UUID_BE and UUID_LE macros to declare the generated
> compound literal const, allowing the compiler to put it in rodata.
>
> When using the compound literal as an initializer for a variable, that
> variable has to be declared const as well for the UUID to become rodata.
>
> Is that all?  Unfortunately not.  Under certain conditions gcc refuses
> to store compound literals in rodata despite them being declared const.
> Rasmus Villemoes opened a bugzilla entry for this in December 2015:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68725
>
> The upshot is that if a UUID is passed by value to a function (either
> directly or as a variable), it is stored in rodata.  This is the case
> for efi_guidcmp().  But if the UUID is passed by reference, it is
> generated on the stack, in spite of it being declared const and the
> function parameter being declared const.  This is the case for the
> various EFI boot and runtime services calls such as GetVariable.
> A workaround is to declare a *static* const variable, assign the UUID to
> it and pass the variable by reference to the function.
>
> This issue also affects efi_char16_t strings which are only declared
> const (and not static const).  They're likewise generated on the stack
> at runtime.
>
> clang 3.9 is not better than gcc:  It does the exact opposite by putting
> UUIDs in rodata if they're passed by reference, and generating them on
> the stack if they're passed by value.
>
> Cc: Huang Ying <ying.huang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Rasmus Villemoes <linux-qQsb+v5E8BnlAoU/VqSP6n9LOBIZ5rWg@public.gmane.org>
> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> ---
>  include/uapi/linux/uuid.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/uuid.h b/include/uapi/linux/uuid.h
> index 3738e5f..9184f01 100644
> --- a/include/uapi/linux/uuid.h
> +++ b/include/uapi/linux/uuid.h
> @@ -29,14 +29,14 @@
>  } uuid_be;
>
>  #define UUID_LE(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)               \
> -((uuid_le)                                                             \
> +((const uuid_le)                                                       \
>  {{ (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, ((a) >> 24) & 0xff, \
>     (b) & 0xff, ((b) >> 8) & 0xff,                                      \
>     (c) & 0xff, ((c) >> 8) & 0xff,                                      \
>     (d0), (d1), (d2), (d3), (d4), (d5), (d6), (d7) }})
>
>  #define UUID_BE(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)               \
> -((uuid_be)                                                             \
> +((const uuid_be)                                                       \
>  {{ ((a) >> 24) & 0xff, ((a) >> 16) & 0xff, ((a) >> 8) & 0xff, (a) & 0xff, \
>     ((b) >> 8) & 0xff, (b) & 0xff,                                      \
>     ((c) >> 8) & 0xff, (c) & 0xff,                                      \
> --
> 2.11.0
>

Re: [PATCH 10/10] uuid: Constify UUID compound literals

[Lukas Wunner]

On Wed, Jan 04, 2017 at 05:54:08PM +0000, Ard Biesheuvel wrote:
> On 2 January 2017 at 10:24, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
> > The UUID_BE() and UUID_LE() macros (as well as the derived EFI_GUID())
> > generate a compound literal for a 128-bit ID.  By default, these are not
> > stored in the rodata section but generated on the stack at runtime in a
> > very time- and space-inefficient manner.
> 
> This is not true if you declare those variables static const (in
> function scope).
> 
> In general, I think we should avoid touching uapi headers for this, so
> I would like to explore if we can reach the same result without this
> patch. Another objection I have to this patch is that it is context
> dependent whether the UUID_[LE_BE]() macros in fact generate compound
> literals: this is true for an assignment to a pointer variable, but
> when they are used in initializers this is not the case, and so adding
> 'const' to their definitions is not always appropriate imo.
> 
> Are there any instances where simply turning those automatic variables
> into static const doesn't work?

The number of GUIDs used in the kernel is vast, it's many more than this
patch set makes it appear.  In a lot of cases, they're used as literals.
I would have to declare a temporary "static const" variable for each of
them and this patch set would become much larger and more intrusive.
Additionally the necessity to use a static const variable instead of a
literal seems clumsy and ugly.

As an example, consider arch/ia64/kernel/mca_drv.c:mca_make_slidx() which
contains an if-else-if ladder with 9 entries, each using a GUID literal.
I would have expected objections on grounds of severe ugliness if I had
changed all of these to temporary "static const" variables.

By contrast, just changing the UUID_LE() and UUID_BE() macros causes these
literals to be placed in rodata automatically.

It seems that the only reason for uuid.h to be in uapi is its usage by
two other uapi headers, hyperv.h and mei.h.  The two don't even use the
UUID_LE() and UUID_BE() macros.

The impact of adding const to these macros is limited:  If the macros are
used as an initializer in a user space application and the variable they're
assigned to isn't declared const, gcc will silently make them non-const.

Worst thing that can happen is that compiling the user space application
results in a compiler warning.  This only occurs if the UUID is passed by
reference into a function.  E.g. if this patch wasn't at the end of the
series, these 3 warnings would occur:

arch/x86/platform/efi/quirks.c: In function 'efi_delete_dummy_variable':
arch/x86/platform/efi/quirks.c:52:35: warning: passing argument 2 of 'efi.set_variable' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
  efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
                                   ^
arch/x86/platform/efi/quirks.c:52:35: note: expected 'efi_guid_t * {aka struct <anonymous> *}' but argument is of type 'const uuid_le * {aka const struct <anonymous> *}'

arch/x86/platform/efi/quirks.c: In function 'efi_query_variable_store':
arch/x86/platform/efi/quirks.c:129:45: warning: passing argument 2 of 'efi.set_variable' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
   status = efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
                                             ^
arch/x86/platform/efi/quirks.c:129:45: note: expected 'efi_guid_t * {aka struct <anonymous> *}' but argument is of type 'const uuid_le * {aka const struct <anonymous> *}'

drivers/scsi/isci/probe_roms.c: In function 'isci_get_efi_var':
drivers/scsi/isci/probe_roms.c:190:8: warning: passing argument 2 of 'get_efi()->get_variable' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
        &ISCI_EFI_VENDOR_GUID,
        ^
drivers/scsi/isci/probe_roms.c:190:8: note: expected 'efi_guid_t * {aka struct <anonymous> *}' but argument is of type 'const uuid_le * {aka const struct <anonymous> *}'

This can easily be fixed in the user space application by adding "const"
to function declarations.

To reiterate what I wrote in the commit message, I don't see why anyone
would want to modify the distinct bytes making up the UUID after declaring
it, so marking UUIDs const seems natural.

Thanks,

Lukas

> 
> > E.g. EFI_FILE_SYSTEM_GUID (964E5B22-6459-11D2-8E39-00A0C969723B) results
> > in x86_64 code which requires 80 bytes text instead of 16 bytes rodata:
> >
> >     c6 44 24 20 22                   movb   $0x22,0x20(%rsp)
> >     c6 44 24 21 5b                   movb   $0x5b,0x21(%rsp)
> >     c6 44 24 22 4e                   movb   $0x4e,0x22(%rsp)
> >     c6 44 24 23 96                   movb   $0x96,0x23(%rsp)
> >     c6 44 24 24 59                   movb   $0x59,0x24(%rsp)
> >     c6 44 24 25 64                   movb   $0x64,0x25(%rsp)
> >     c6 44 24 26 d2                   movb   $0xd2,0x26(%rsp)
> >     c6 44 24 27 11                   movb   $0x11,0x27(%rsp)
> >     c6 44 24 28 8e                   movb   $0x8e,0x28(%rsp)
> >     c6 44 24 29 39                   movb   $0x39,0x29(%rsp)
> >     c6 44 24 2a 00                   movb   $0x0,0x2a(%rsp)
> >     c6 44 24 2b a0                   movb   $0xa0,0x2b(%rsp)
> >     c6 44 24 2c c9                   movb   $0xc9,0x2c(%rsp)
> >     c6 44 24 2d 69                   movb   $0x69,0x2d(%rsp)
> >     c6 44 24 2e 72                   movb   $0x72,0x2e(%rsp)
> >     c6 44 24 2f 3b                   movb   $0x3b,0x2f(%rsp)
> >
> > gcc 7 improves on this by using 2x movabs + 2x mov instructions,
> > amounting to 30 bytes text instead of 16 bytes rodata.  That's more
> > space-efficient than 80 bytes but still undesirable from a performance
> > and security perspective:
> >
> >     48 b8 22 5b 4e 96 59 64 d2 11    movabs $0x11d26459964e5b,%rax
> >     48 89 44 24 20                   mov    %rax,0x20(%rsp)
> >     48 b8 8e 39 00 a0 c9 69 72 3b    movabs $0x3b7269c9a00039,%rax
> >     48 89 44 24 28                   mov    %rax,0x28(%rsp)
> >
> > The reason for generating compound literals on the stack is that they're
> > allowed to be modified.  E.g. a pointer to a UUID might be used to
> > change some of its 16 bytes.  In the context of UUIDs this seems silly
> > though:  The uuid_be, uuid_le and efi_guid_t data types hide the
> > distinct bytes making up the ID, the bytes are not meant to be modified.
> > Moreover, the UUIDs are usually pre-defined in a spec such as UEFI and
> > there's no reason at all to modify them.
> >
> > Thus, change the UUID_BE and UUID_LE macros to declare the generated
> > compound literal const, allowing the compiler to put it in rodata.
> >
> > When using the compound literal as an initializer for a variable, that
> > variable has to be declared const as well for the UUID to become rodata.
> >
> > Is that all?  Unfortunately not.  Under certain conditions gcc refuses
> > to store compound literals in rodata despite them being declared const.
> > Rasmus Villemoes opened a bugzilla entry for this in December 2015:
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68725
> >
> > The upshot is that if a UUID is passed by value to a function (either
> > directly or as a variable), it is stored in rodata.  This is the case
> > for efi_guidcmp().  But if the UUID is passed by reference, it is
> > generated on the stack, in spite of it being declared const and the
> > function parameter being declared const.  This is the case for the
> > various EFI boot and runtime services calls such as GetVariable.
> > A workaround is to declare a *static* const variable, assign the UUID to
> > it and pass the variable by reference to the function.
> >
> > This issue also affects efi_char16_t strings which are only declared
> > const (and not static const).  They're likewise generated on the stack
> > at runtime.
> >
> > clang 3.9 is not better than gcc:  It does the exact opposite by putting
> > UUIDs in rodata if they're passed by reference, and generating them on
> > the stack if they're passed by value.
> >
> > Cc: Huang Ying <ying.huang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Cc: Rasmus Villemoes <linux-qQsb+v5E8BnlAoU/VqSP6n9LOBIZ5rWg@public.gmane.org>
> > Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> > ---
> >  include/uapi/linux/uuid.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/uuid.h b/include/uapi/linux/uuid.h
> > index 3738e5f..9184f01 100644
> > --- a/include/uapi/linux/uuid.h
> > +++ b/include/uapi/linux/uuid.h
> > @@ -29,14 +29,14 @@
> >  } uuid_be;
> >
> >  #define UUID_LE(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)               \
> > -((uuid_le)                                                             \
> > +((const uuid_le)                                                       \
> >  {{ (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, ((a) >> 24) & 0xff, \
> >     (b) & 0xff, ((b) >> 8) & 0xff,                                      \
> >     (c) & 0xff, ((c) >> 8) & 0xff,                                      \
> >     (d0), (d1), (d2), (d3), (d4), (d5), (d6), (d7) }})
> >
> >  #define UUID_BE(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)               \
> > -((uuid_be)                                                             \
> > +((const uuid_be)                                                       \
> >  {{ ((a) >> 24) & 0xff, ((a) >> 16) & 0xff, ((a) >> 8) & 0xff, (a) & 0xff, \
> >     ((b) >> 8) & 0xff, (b) & 0xff,                                      \
> >     ((c) >> 8) & 0xff, (c) & 0xff,                                      \
> > --
> > 2.11.0
> >

Re: [PATCH 03/10] efi: Constify GetVariable() / SetVariable() arguments

[Lukas Wunner]

On Wed, Jan 04, 2017 at 05:38:13PM +0000, Ard Biesheuvel wrote:
> On 2 January 2017 at 10:24, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
> > GUIDs and variable names passed to GetVariable() / SetVariable() are not
> > modified, so declare them const.  This allows the compiler to place them
> > in the rodata section instead of generating them on the stack at
> > runtime.  Pass GUIDs as literals rather than assigning them to temporary
> > variables to save a bit on code.
> >
> > Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > Cc: Tony Luck <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Cc: Fenghua Yu <fenghua.yu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Cc: Anton Vorontsov <anton-9xeibp6oKSgdnm+yROfE0A@public.gmane.org>
> > Cc: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> > Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > Cc: Artur Paszkiewicz <artur.paszkiewicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Cc: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Cc: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> > ---
> >  arch/ia64/kernel/efi.c                  | 15 ++++-----------
> >  arch/x86/platform/efi/quirks.c          |  2 +-
> >  drivers/firmware/efi/efi-pstore.c       |  4 ++--
> >  drivers/firmware/efi/efibc.c            |  3 +--
> >  drivers/firmware/efi/libstub/arm-stub.c |  5 ++---
> >  drivers/infiniband/hw/hfi1/efivar.c     |  6 +-----
> >  drivers/scsi/isci/probe_roms.c          |  2 +-
> >  7 files changed, 12 insertions(+), 25 deletions(-)
> >
> > diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
> > index f57d089..f1264ab 100644
> > --- a/arch/ia64/kernel/efi.c
> > +++ b/arch/ia64/kernel/efi.c
> > @@ -919,22 +919,15 @@ static void __init handle_palo(unsigned long phys_addr)
> >  efi_uart_console_only(void)
> >  {
> >         efi_status_t status;
> > -       char *s, name[] = "ConOut";
> > -       efi_guid_t guid = EFI_GLOBAL_VARIABLE_GUID;
> > -       efi_char16_t *utf16, name_utf16[32];
> > +       const char name[] = "ConOut";
> > +       const efi_char16_t name_utf16[] = { 'C', 'o', 'n', 'O', 'u', 't', 0 };
> 
> I know you mention 'static' in 10/10, but I don't understand why you
> don't make these static to begin with. That also allows you to
> annotate them as __initconst, which is not possible for compound
> initializers (including the GUID)

Right, I hadn't thought of __initconst.  In this particular case indeed
it might make sense to use "static const efi_guid_t guid __initconst = ...",
and arguably for the strings as well.  But where should I draw the line?

There are a handful of other GUIDs used in __init functions, e.g. in
arch/ia64/ there's ESI_TABLE_GUID in kernel/esi.c:esi_init() and
SAL_SYSTEM_TABLE_GUID in sn/kernel/setup.c:early_sn_setup().
This patch set results in them being placed in rodata, but not
init.rodata.  Should I declare a temporary "static const __initconst"
variable for each of them?  That would give us 16 bytes in each of the
two cases being freed after kernel initialization.

However string literals aren't put in init.rodata either even though
they're declared in an __init function.  E.g. early_sn_setup() contains
a string literal "failed to find SAL entry point\n".  That's 31 bytes.
esi_init() contains even 103 bytes of string literals.

So I'm not sure if it's worth to move the GUIDs to init.rodata (at the
expense of an additional LoC to declare a temporary variable) even
though we waste more space for string literals.

Thanks,

Lukas

> 
> >         unsigned char data[1024];
> >         unsigned long size = sizeof(data);
> >         struct efi_generic_dev_path *hdr, *end_addr;
> >         int uart = 0;
> >
> > -       /* Convert to UTF-16 */
> > -       utf16 = name_utf16;
> > -       s = name;
> > -       while (*s)
> > -               *utf16++ = *s++ & 0x7f;
> > -       *utf16 = 0;
> > -
> > -       status = efi.get_variable(name_utf16, &guid, NULL, &size, data);
> > +       status = efi.get_variable(name_utf16, &EFI_GLOBAL_VARIABLE_GUID,
> > +                                 NULL, &size, data);
> >         if (status != EFI_SUCCESS) {
> >                 printk(KERN_ERR "No EFI %s variable?\n", name);
> >                 return 0;
> > diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> > index 10aca63..aad3701 100644
> > --- a/arch/x86/platform/efi/quirks.c
> > +++ b/arch/x86/platform/efi/quirks.c
> > @@ -19,7 +19,7 @@
> >  #define EFI_DUMMY_GUID \
> >         EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e, 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92, 0xa9)
> >
> > -static efi_char16_t efi_dummy_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };
> > +static const efi_char16_t efi_dummy_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };
> >
> >  static bool efi_no_storage_paranoia;
> >
> > diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
> > index f402ba2..0911449 100644
> > --- a/drivers/firmware/efi/efi-pstore.c
> > +++ b/drivers/firmware/efi/efi-pstore.c
> > @@ -265,7 +265,6 @@ static int efi_pstore_write(enum pstore_type_id type,
> >  {
> >         char name[DUMP_NAME_LEN];
> >         efi_char16_t efi_name[DUMP_NAME_LEN];
> > -       efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
> >         int i, ret = 0;
> >
> >         sprintf(name, "dump-type%u-%u-%d-%lu-%c", type, part, count,
> > @@ -274,7 +273,8 @@ static int efi_pstore_write(enum pstore_type_id type,
> >         for (i = 0; i < DUMP_NAME_LEN; i++)
> >                 efi_name[i] = name[i];
> >
> > -       efivar_entry_set_safe(efi_name, vendor, PSTORE_EFI_ATTRIBUTES,
> > +       efivar_entry_set_safe(efi_name, LINUX_EFI_CRASH_GUID,
> > +                             PSTORE_EFI_ATTRIBUTES,
> >                               !pstore_cannot_block_path(reason),
> >                               size, psi->buf);
> >
> > diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
> > index 503bbe2..57ddf8c 100644
> > --- a/drivers/firmware/efi/efibc.c
> > +++ b/drivers/firmware/efi/efibc.c
> > @@ -32,7 +32,6 @@ static void efibc_str_to_str16(const char *str, efi_char16_t *str16)
> >  static int efibc_set_variable(const char *name, const char *value)
> >  {
> >         int ret;
> > -       efi_guid_t guid = LINUX_EFI_LOADER_ENTRY_GUID;
> >         struct efivar_entry *entry;
> >         size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);
> >
> > @@ -49,7 +48,7 @@ static int efibc_set_variable(const char *name, const char *value)
> >
> >         efibc_str_to_str16(name, entry->var.VariableName);
> >         efibc_str_to_str16(value, (efi_char16_t *)entry->var.Data);
> > -       memcpy(&entry->var.VendorGuid, &guid, sizeof(guid));
> > +       entry->var.VendorGuid = LINUX_EFI_LOADER_ENTRY_GUID;
> >
> >         ret = efivar_entry_set(entry,
> >                                EFI_VARIABLE_NON_VOLATILE
> > diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> > index 6fca48c..da72bfb 100644
> > --- a/drivers/firmware/efi/libstub/arm-stub.c
> > +++ b/drivers/firmware/efi/libstub/arm-stub.c
> > @@ -27,13 +27,12 @@ static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
> >         static efi_char16_t const sm_var_name[] = {
> >                 'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
> >
> > -       efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
> >         efi_get_variable_t *f_getvar = sys_table_arg->runtime->get_variable;
> >         u8 val;
> >         unsigned long size = sizeof(val);
> >         efi_status_t status;
> >
> > -       status = f_getvar((efi_char16_t *)sb_var_name, (efi_guid_t *)&var_guid,
> > +       status = f_getvar(sb_var_name, &EFI_GLOBAL_VARIABLE_GUID,
> >                           NULL, &size, &val);
> >
> >         if (status != EFI_SUCCESS)
> > @@ -42,7 +41,7 @@ static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
> >         if (val == 0)
> >                 return 0;
> >
> > -       status = f_getvar((efi_char16_t *)sm_var_name, (efi_guid_t *)&var_guid,
> > +       status = f_getvar(sm_var_name, &EFI_GLOBAL_VARIABLE_GUID,
> >                           NULL, &size, &val);
> >
> >         if (status != EFI_SUCCESS)
> > diff --git a/drivers/infiniband/hw/hfi1/efivar.c b/drivers/infiniband/hw/hfi1/efivar.c
> > index 106349f..f769cc2 100644
> > --- a/drivers/infiniband/hw/hfi1/efivar.c
> > +++ b/drivers/infiniband/hw/hfi1/efivar.c
> > @@ -66,7 +66,6 @@ static int read_efi_var(const char *name, unsigned long *size,
> >  {
> >         efi_status_t status;
> >         efi_char16_t *uni_name;
> > -       efi_guid_t guid;
> >         unsigned long temp_size;
> >         void *temp_buffer;
> >         void *data;
> > @@ -95,13 +94,10 @@ static int read_efi_var(const char *name, unsigned long *size,
> >         for (i = 0; name[i]; i++)
> >                 uni_name[i] = name[i];
> >
> > -       /* need a variable for our GUID */
> > -       guid = HFI1_EFIVAR_GUID;
> > -
> >         /* call into EFI runtime services */
> >         status = efi.get_variable(
> >                         uni_name,
> > -                       &guid,
> > +                       &HFI1_EFIVAR_GUID,
> >                         NULL,
> >                         &temp_size,
> >                         temp_buffer);
> > diff --git a/drivers/scsi/isci/probe_roms.c b/drivers/scsi/isci/probe_roms.c
> > index 8ac646e..d2c17c1 100644
> > --- a/drivers/scsi/isci/probe_roms.c
> > +++ b/drivers/scsi/isci/probe_roms.c
> > @@ -34,7 +34,7 @@
> >  #include "task.h"
> >  #include "probe_roms.h"
> >
> > -static efi_char16_t isci_efivar_name[] = {
> > +static const efi_char16_t isci_efivar_name[] = {
> >         'R', 's', 't', 'S', 'c', 'u', 'O'
> >  };
> >
> > --
> > 2.11.0
> >

Re: [PATCH 10/10] uuid: Constify UUID compound literals

[Ard Biesheuvel]

On 6 January 2017 at 09:57, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
> On Wed, Jan 04, 2017 at 05:54:08PM +0000, Ard Biesheuvel wrote:
>> On 2 January 2017 at 10:24, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
>> > The UUID_BE() and UUID_LE() macros (as well as the derived EFI_GUID())
>> > generate a compound literal for a 128-bit ID.  By default, these are not
>> > stored in the rodata section but generated on the stack at runtime in a
>> > very time- and space-inefficient manner.
>>
>> This is not true if you declare those variables static const (in
>> function scope).
>>
>> In general, I think we should avoid touching uapi headers for this, so
>> I would like to explore if we can reach the same result without this
>> patch. Another objection I have to this patch is that it is context
>> dependent whether the UUID_[LE_BE]() macros in fact generate compound
>> literals: this is true for an assignment to a pointer variable, but
>> when they are used in initializers this is not the case, and so adding
>> 'const' to their definitions is not always appropriate imo.
>>
>> Are there any instances where simply turning those automatic variables
>> into static const doesn't work?
>
> The number of GUIDs used in the kernel is vast, it's many more than this
> patch set makes it appear.  In a lot of cases, they're used as literals.
> I would have to declare a temporary "static const" variable for each of
> them and this patch set would become much larger and more intrusive.
> Additionally the necessity to use a static const variable instead of a
> literal seems clumsy and ugly.
>
> As an example, consider arch/ia64/kernel/mca_drv.c:mca_make_slidx() which
> contains an if-else-if ladder with 9 entries, each using a GUID literal.
> I would have expected objections on grounds of severe ugliness if I had
> changed all of these to temporary "static const" variables.
>

Yes, but we can macroize that: something like

#define __efiguid_ref(x) ({static const efi_guid_t __x = (x); __x; })

and

#define __efiguid_initref(x) ({static const __initconst efi_guid_t __x
= (x); __x; })

should do the trick I think?


> By contrast, just changing the UUID_LE() and UUID_BE() macros causes these
> literals to be placed in rodata automatically.
>
> It seems that the only reason for uuid.h to be in uapi is its usage by
> two other uapi headers, hyperv.h and mei.h.  The two don't even use the
> UUID_LE() and UUID_BE() macros.
>
> The impact of adding const to these macros is limited:  If the macros are
> used as an initializer in a user space application and the variable they're
> assigned to isn't declared const, gcc will silently make them non-const.
>
> Worst thing that can happen is that compiling the user space application
> results in a compiler warning.  This only occurs if the UUID is passed by
> reference into a function.  E.g. if this patch wasn't at the end of the
> series, these 3 warnings would occur:
>
> arch/x86/platform/efi/quirks.c: In function 'efi_delete_dummy_variable':
> arch/x86/platform/efi/quirks.c:52:35: warning: passing argument 2 of 'efi.set_variable' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>   efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
>                                    ^
> arch/x86/platform/efi/quirks.c:52:35: note: expected 'efi_guid_t * {aka struct <anonymous> *}' but argument is of type 'const uuid_le * {aka const struct <anonymous> *}'
>
> arch/x86/platform/efi/quirks.c: In function 'efi_query_variable_store':
> arch/x86/platform/efi/quirks.c:129:45: warning: passing argument 2 of 'efi.set_variable' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>    status = efi.set_variable(efi_dummy_name, &EFI_DUMMY_GUID,
>                                              ^
> arch/x86/platform/efi/quirks.c:129:45: note: expected 'efi_guid_t * {aka struct <anonymous> *}' but argument is of type 'const uuid_le * {aka const struct <anonymous> *}'
>
> drivers/scsi/isci/probe_roms.c: In function 'isci_get_efi_var':
> drivers/scsi/isci/probe_roms.c:190:8: warning: passing argument 2 of 'get_efi()->get_variable' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>         &ISCI_EFI_VENDOR_GUID,
>         ^
> drivers/scsi/isci/probe_roms.c:190:8: note: expected 'efi_guid_t * {aka struct <anonymous> *}' but argument is of type 'const uuid_le * {aka const struct <anonymous> *}'
>
> This can easily be fixed in the user space application by adding "const"
> to function declarations.
>
> To reiterate what I wrote in the commit message, I don't see why anyone
> would want to modify the distinct bytes making up the UUID after declaring
> it, so marking UUIDs const seems natural.
>
> Thanks,
>
> Lukas
>
>>
>> > E.g. EFI_FILE_SYSTEM_GUID (964E5B22-6459-11D2-8E39-00A0C969723B) results
>> > in x86_64 code which requires 80 bytes text instead of 16 bytes rodata:
>> >
>> >     c6 44 24 20 22                   movb   $0x22,0x20(%rsp)
>> >     c6 44 24 21 5b                   movb   $0x5b,0x21(%rsp)
>> >     c6 44 24 22 4e                   movb   $0x4e,0x22(%rsp)
>> >     c6 44 24 23 96                   movb   $0x96,0x23(%rsp)
>> >     c6 44 24 24 59                   movb   $0x59,0x24(%rsp)
>> >     c6 44 24 25 64                   movb   $0x64,0x25(%rsp)
>> >     c6 44 24 26 d2                   movb   $0xd2,0x26(%rsp)
>> >     c6 44 24 27 11                   movb   $0x11,0x27(%rsp)
>> >     c6 44 24 28 8e                   movb   $0x8e,0x28(%rsp)
>> >     c6 44 24 29 39                   movb   $0x39,0x29(%rsp)
>> >     c6 44 24 2a 00                   movb   $0x0,0x2a(%rsp)
>> >     c6 44 24 2b a0                   movb   $0xa0,0x2b(%rsp)
>> >     c6 44 24 2c c9                   movb   $0xc9,0x2c(%rsp)
>> >     c6 44 24 2d 69                   movb   $0x69,0x2d(%rsp)
>> >     c6 44 24 2e 72                   movb   $0x72,0x2e(%rsp)
>> >     c6 44 24 2f 3b                   movb   $0x3b,0x2f(%rsp)
>> >
>> > gcc 7 improves on this by using 2x movabs + 2x mov instructions,
>> > amounting to 30 bytes text instead of 16 bytes rodata.  That's more
>> > space-efficient than 80 bytes but still undesirable from a performance
>> > and security perspective:
>> >
>> >     48 b8 22 5b 4e 96 59 64 d2 11    movabs $0x11d26459964e5b,%rax
>> >     48 89 44 24 20                   mov    %rax,0x20(%rsp)
>> >     48 b8 8e 39 00 a0 c9 69 72 3b    movabs $0x3b7269c9a00039,%rax
>> >     48 89 44 24 28                   mov    %rax,0x28(%rsp)
>> >
>> > The reason for generating compound literals on the stack is that they're
>> > allowed to be modified.  E.g. a pointer to a UUID might be used to
>> > change some of its 16 bytes.  In the context of UUIDs this seems silly
>> > though:  The uuid_be, uuid_le and efi_guid_t data types hide the
>> > distinct bytes making up the ID, the bytes are not meant to be modified.
>> > Moreover, the UUIDs are usually pre-defined in a spec such as UEFI and
>> > there's no reason at all to modify them.
>> >
>> > Thus, change the UUID_BE and UUID_LE macros to declare the generated
>> > compound literal const, allowing the compiler to put it in rodata.
>> >
>> > When using the compound literal as an initializer for a variable, that
>> > variable has to be declared const as well for the UUID to become rodata.
>> >
>> > Is that all?  Unfortunately not.  Under certain conditions gcc refuses
>> > to store compound literals in rodata despite them being declared const.
>> > Rasmus Villemoes opened a bugzilla entry for this in December 2015:
>> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68725
>> >
>> > The upshot is that if a UUID is passed by value to a function (either
>> > directly or as a variable), it is stored in rodata.  This is the case
>> > for efi_guidcmp().  But if the UUID is passed by reference, it is
>> > generated on the stack, in spite of it being declared const and the
>> > function parameter being declared const.  This is the case for the
>> > various EFI boot and runtime services calls such as GetVariable.
>> > A workaround is to declare a *static* const variable, assign the UUID to
>> > it and pass the variable by reference to the function.
>> >
>> > This issue also affects efi_char16_t strings which are only declared
>> > const (and not static const).  They're likewise generated on the stack
>> > at runtime.
>> >
>> > clang 3.9 is not better than gcc:  It does the exact opposite by putting
>> > UUIDs in rodata if they're passed by reference, and generating them on
>> > the stack if they're passed by value.
>> >
>> > Cc: Huang Ying <ying.huang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> > Cc: Rasmus Villemoes <linux-qQsb+v5E8BnlAoU/VqSP6n9LOBIZ5rWg@public.gmane.org>
>> > Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
>> > Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> > Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
>> > ---
>> >  include/uapi/linux/uuid.h | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/include/uapi/linux/uuid.h b/include/uapi/linux/uuid.h
>> > index 3738e5f..9184f01 100644
>> > --- a/include/uapi/linux/uuid.h
>> > +++ b/include/uapi/linux/uuid.h
>> > @@ -29,14 +29,14 @@
>> >  } uuid_be;
>> >
>> >  #define UUID_LE(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)               \
>> > -((uuid_le)                                                             \
>> > +((const uuid_le)                                                       \
>> >  {{ (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, ((a) >> 24) & 0xff, \
>> >     (b) & 0xff, ((b) >> 8) & 0xff,                                      \
>> >     (c) & 0xff, ((c) >> 8) & 0xff,                                      \
>> >     (d0), (d1), (d2), (d3), (d4), (d5), (d6), (d7) }})
>> >
>> >  #define UUID_BE(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)               \
>> > -((uuid_be)                                                             \
>> > +((const uuid_be)                                                       \
>> >  {{ ((a) >> 24) & 0xff, ((a) >> 16) & 0xff, ((a) >> 8) & 0xff, (a) & 0xff, \
>> >     ((b) >> 8) & 0xff, (b) & 0xff,                                      \
>> >     ((c) >> 8) & 0xff, (c) & 0xff,                                      \
>> > --
>> > 2.11.0
>> >

Re: [PATCH 03/10] efi: Constify GetVariable() / SetVariable() arguments

[Ard Biesheuvel]

On 6 January 2017 at 10:16, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
> On Wed, Jan 04, 2017 at 05:38:13PM +0000, Ard Biesheuvel wrote:
>> On 2 January 2017 at 10:24, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
>> > GUIDs and variable names passed to GetVariable() / SetVariable() are not
>> > modified, so declare them const.  This allows the compiler to place them
>> > in the rodata section instead of generating them on the stack at
>> > runtime.  Pass GUIDs as literals rather than assigning them to temporary
>> > variables to save a bit on code.
>> >
>> > Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
>> > Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> > Cc: Tony Luck <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> > Cc: Fenghua Yu <fenghua.yu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> > Cc: Anton Vorontsov <anton-9xeibp6oKSgdnm+yROfE0A@public.gmane.org>
>> > Cc: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>> > Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> > Cc: Artur Paszkiewicz <artur.paszkiewicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> > Cc: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> > Cc: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> > Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
>> > ---
>> >  arch/ia64/kernel/efi.c                  | 15 ++++-----------
>> >  arch/x86/platform/efi/quirks.c          |  2 +-
>> >  drivers/firmware/efi/efi-pstore.c       |  4 ++--
>> >  drivers/firmware/efi/efibc.c            |  3 +--
>> >  drivers/firmware/efi/libstub/arm-stub.c |  5 ++---
>> >  drivers/infiniband/hw/hfi1/efivar.c     |  6 +-----
>> >  drivers/scsi/isci/probe_roms.c          |  2 +-
>> >  7 files changed, 12 insertions(+), 25 deletions(-)
>> >
>> > diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
>> > index f57d089..f1264ab 100644
>> > --- a/arch/ia64/kernel/efi.c
>> > +++ b/arch/ia64/kernel/efi.c
>> > @@ -919,22 +919,15 @@ static void __init handle_palo(unsigned long phys_addr)
>> >  efi_uart_console_only(void)
>> >  {
>> >         efi_status_t status;
>> > -       char *s, name[] = "ConOut";
>> > -       efi_guid_t guid = EFI_GLOBAL_VARIABLE_GUID;
>> > -       efi_char16_t *utf16, name_utf16[32];
>> > +       const char name[] = "ConOut";
>> > +       const efi_char16_t name_utf16[] = { 'C', 'o', 'n', 'O', 'u', 't', 0 };
>>
>> I know you mention 'static' in 10/10, but I don't understand why you
>> don't make these static to begin with. That also allows you to
>> annotate them as __initconst, which is not possible for compound
>> initializers (including the GUID)
>
> Right, I hadn't thought of __initconst.  In this particular case indeed
> it might make sense to use "static const efi_guid_t guid __initconst = ...",
> and arguably for the strings as well.  But where should I draw the line?
>
> There are a handful of other GUIDs used in __init functions, e.g. in
> arch/ia64/ there's ESI_TABLE_GUID in kernel/esi.c:esi_init() and
> SAL_SYSTEM_TABLE_GUID in sn/kernel/setup.c:early_sn_setup().
> This patch set results in them being placed in rodata, but not
> init.rodata.  Should I declare a temporary "static const __initconst"
> variable for each of them?  That would give us 16 bytes in each of the
> two cases being freed after kernel initialization.
>
> However string literals aren't put in init.rodata either even though
> they're declared in an __init function.  E.g. early_sn_setup() contains
> a string literal "failed to find SAL entry point\n".  That's 31 bytes.
> esi_init() contains even 103 bytes of string literals.
>
> So I'm not sure if it's worth to move the GUIDs to init.rodata (at the
> expense of an additional LoC to declare a temporary variable) even
> though we waste more space for string literals.
>

For the string literals I think there are some solutions possible
involving compiler plugins. Kees?

>>
>> >         unsigned char data[1024];
>> >         unsigned long size = sizeof(data);
>> >         struct efi_generic_dev_path *hdr, *end_addr;
>> >         int uart = 0;
>> >
>> > -       /* Convert to UTF-16 */
>> > -       utf16 = name_utf16;
>> > -       s = name;
>> > -       while (*s)
>> > -               *utf16++ = *s++ & 0x7f;
>> > -       *utf16 = 0;
>> > -
>> > -       status = efi.get_variable(name_utf16, &guid, NULL, &size, data);
>> > +       status = efi.get_variable(name_utf16, &EFI_GLOBAL_VARIABLE_GUID,
>> > +                                 NULL, &size, data);
>> >         if (status != EFI_SUCCESS) {
>> >                 printk(KERN_ERR "No EFI %s variable?\n", name);
>> >                 return 0;
>> > diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>> > index 10aca63..aad3701 100644
>> > --- a/arch/x86/platform/efi/quirks.c
>> > +++ b/arch/x86/platform/efi/quirks.c
>> > @@ -19,7 +19,7 @@
>> >  #define EFI_DUMMY_GUID \
>> >         EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e, 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92, 0xa9)
>> >
>> > -static efi_char16_t efi_dummy_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };
>> > +static const efi_char16_t efi_dummy_name[6] = { 'D', 'U', 'M', 'M', 'Y', 0 };
>> >
>> >  static bool efi_no_storage_paranoia;
>> >
>> > diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
>> > index f402ba2..0911449 100644
>> > --- a/drivers/firmware/efi/efi-pstore.c
>> > +++ b/drivers/firmware/efi/efi-pstore.c
>> > @@ -265,7 +265,6 @@ static int efi_pstore_write(enum pstore_type_id type,
>> >  {
>> >         char name[DUMP_NAME_LEN];
>> >         efi_char16_t efi_name[DUMP_NAME_LEN];
>> > -       efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
>> >         int i, ret = 0;
>> >
>> >         sprintf(name, "dump-type%u-%u-%d-%lu-%c", type, part, count,
>> > @@ -274,7 +273,8 @@ static int efi_pstore_write(enum pstore_type_id type,
>> >         for (i = 0; i < DUMP_NAME_LEN; i++)
>> >                 efi_name[i] = name[i];
>> >
>> > -       efivar_entry_set_safe(efi_name, vendor, PSTORE_EFI_ATTRIBUTES,
>> > +       efivar_entry_set_safe(efi_name, LINUX_EFI_CRASH_GUID,
>> > +                             PSTORE_EFI_ATTRIBUTES,
>> >                               !pstore_cannot_block_path(reason),
>> >                               size, psi->buf);
>> >
>> > diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
>> > index 503bbe2..57ddf8c 100644
>> > --- a/drivers/firmware/efi/efibc.c
>> > +++ b/drivers/firmware/efi/efibc.c
>> > @@ -32,7 +32,6 @@ static void efibc_str_to_str16(const char *str, efi_char16_t *str16)
>> >  static int efibc_set_variable(const char *name, const char *value)
>> >  {
>> >         int ret;
>> > -       efi_guid_t guid = LINUX_EFI_LOADER_ENTRY_GUID;
>> >         struct efivar_entry *entry;
>> >         size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);
>> >
>> > @@ -49,7 +48,7 @@ static int efibc_set_variable(const char *name, const char *value)
>> >
>> >         efibc_str_to_str16(name, entry->var.VariableName);
>> >         efibc_str_to_str16(value, (efi_char16_t *)entry->var.Data);
>> > -       memcpy(&entry->var.VendorGuid, &guid, sizeof(guid));
>> > +       entry->var.VendorGuid = LINUX_EFI_LOADER_ENTRY_GUID;
>> >
>> >         ret = efivar_entry_set(entry,
>> >                                EFI_VARIABLE_NON_VOLATILE
>> > diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
>> > index 6fca48c..da72bfb 100644
>> > --- a/drivers/firmware/efi/libstub/arm-stub.c
>> > +++ b/drivers/firmware/efi/libstub/arm-stub.c
>> > @@ -27,13 +27,12 @@ static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
>> >         static efi_char16_t const sm_var_name[] = {
>> >                 'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
>> >
>> > -       efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
>> >         efi_get_variable_t *f_getvar = sys_table_arg->runtime->get_variable;
>> >         u8 val;
>> >         unsigned long size = sizeof(val);
>> >         efi_status_t status;
>> >
>> > -       status = f_getvar((efi_char16_t *)sb_var_name, (efi_guid_t *)&var_guid,
>> > +       status = f_getvar(sb_var_name, &EFI_GLOBAL_VARIABLE_GUID,
>> >                           NULL, &size, &val);
>> >
>> >         if (status != EFI_SUCCESS)
>> > @@ -42,7 +41,7 @@ static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
>> >         if (val == 0)
>> >                 return 0;
>> >
>> > -       status = f_getvar((efi_char16_t *)sm_var_name, (efi_guid_t *)&var_guid,
>> > +       status = f_getvar(sm_var_name, &EFI_GLOBAL_VARIABLE_GUID,
>> >                           NULL, &size, &val);
>> >
>> >         if (status != EFI_SUCCESS)
>> > diff --git a/drivers/infiniband/hw/hfi1/efivar.c b/drivers/infiniband/hw/hfi1/efivar.c
>> > index 106349f..f769cc2 100644
>> > --- a/drivers/infiniband/hw/hfi1/efivar.c
>> > +++ b/drivers/infiniband/hw/hfi1/efivar.c
>> > @@ -66,7 +66,6 @@ static int read_efi_var(const char *name, unsigned long *size,
>> >  {
>> >         efi_status_t status;
>> >         efi_char16_t *uni_name;
>> > -       efi_guid_t guid;
>> >         unsigned long temp_size;
>> >         void *temp_buffer;
>> >         void *data;
>> > @@ -95,13 +94,10 @@ static int read_efi_var(const char *name, unsigned long *size,
>> >         for (i = 0; name[i]; i++)
>> >                 uni_name[i] = name[i];
>> >
>> > -       /* need a variable for our GUID */
>> > -       guid = HFI1_EFIVAR_GUID;
>> > -
>> >         /* call into EFI runtime services */
>> >         status = efi.get_variable(
>> >                         uni_name,
>> > -                       &guid,
>> > +                       &HFI1_EFIVAR_GUID,
>> >                         NULL,
>> >                         &temp_size,
>> >                         temp_buffer);
>> > diff --git a/drivers/scsi/isci/probe_roms.c b/drivers/scsi/isci/probe_roms.c
>> > index 8ac646e..d2c17c1 100644
>> > --- a/drivers/scsi/isci/probe_roms.c
>> > +++ b/drivers/scsi/isci/probe_roms.c
>> > @@ -34,7 +34,7 @@
>> >  #include "task.h"
>> >  #include "probe_roms.h"
>> >
>> > -static efi_char16_t isci_efivar_name[] = {
>> > +static const efi_char16_t isci_efivar_name[] = {
>> >         'R', 's', 't', 'S', 'c', 'u', 'O'
>> >  };
>> >
>> > --
>> > 2.11.0
>> >

Re: [PATCH 03/10] efi: Constify GetVariable() / SetVariable() arguments

[Kees Cook]

On Fri, Jan 6, 2017 at 8:28 AM, Ard Biesheuvel
<ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 6 January 2017 at 10:16, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
>> On Wed, Jan 04, 2017 at 05:38:13PM +0000, Ard Biesheuvel wrote:
>>> On 2 January 2017 at 10:24, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
>>> > GUIDs and variable names passed to GetVariable() / SetVariable() are not
>>> > modified, so declare them const.  This allows the compiler to place them
>>> > in the rodata section instead of generating them on the stack at
>>> > runtime.  Pass GUIDs as literals rather than assigning them to temporary
>>> > variables to save a bit on code.
>>> >
>>> > Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
>>> > Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> > Cc: Tony Luck <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>> > Cc: Fenghua Yu <fenghua.yu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>> > Cc: Anton Vorontsov <anton-9xeibp6oKSgdnm+yROfE0A@public.gmane.org>
>>> > Cc: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>>> > Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>> > Cc: Artur Paszkiewicz <artur.paszkiewicz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>> > Cc: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>> > Cc: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>> > Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
>>> > ---
>>> >  arch/ia64/kernel/efi.c                  | 15 ++++-----------
>>> >  arch/x86/platform/efi/quirks.c          |  2 +-
>>> >  drivers/firmware/efi/efi-pstore.c       |  4 ++--
>>> >  drivers/firmware/efi/efibc.c            |  3 +--
>>> >  drivers/firmware/efi/libstub/arm-stub.c |  5 ++---
>>> >  drivers/infiniband/hw/hfi1/efivar.c     |  6 +-----
>>> >  drivers/scsi/isci/probe_roms.c          |  2 +-
>>> >  7 files changed, 12 insertions(+), 25 deletions(-)
>>> >
>>> > diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
>>> > index f57d089..f1264ab 100644
>>> > --- a/arch/ia64/kernel/efi.c
>>> > +++ b/arch/ia64/kernel/efi.c
>>> > @@ -919,22 +919,15 @@ static void __init handle_palo(unsigned long phys_addr)
>>> >  efi_uart_console_only(void)
>>> >  {
>>> >         efi_status_t status;
>>> > -       char *s, name[] = "ConOut";
>>> > -       efi_guid_t guid = EFI_GLOBAL_VARIABLE_GUID;
>>> > -       efi_char16_t *utf16, name_utf16[32];
>>> > +       const char name[] = "ConOut";
>>> > +       const efi_char16_t name_utf16[] = { 'C', 'o', 'n', 'O', 'u', 't', 0 };
>>>
>>> I know you mention 'static' in 10/10, but I don't understand why you
>>> don't make these static to begin with. That also allows you to
>>> annotate them as __initconst, which is not possible for compound
>>> initializers (including the GUID)
>>
>> Right, I hadn't thought of __initconst.  In this particular case indeed
>> it might make sense to use "static const efi_guid_t guid __initconst = ...",
>> and arguably for the strings as well.  But where should I draw the line?
>>
>> There are a handful of other GUIDs used in __init functions, e.g. in
>> arch/ia64/ there's ESI_TABLE_GUID in kernel/esi.c:esi_init() and
>> SAL_SYSTEM_TABLE_GUID in sn/kernel/setup.c:early_sn_setup().
>> This patch set results in them being placed in rodata, but not
>> init.rodata.  Should I declare a temporary "static const __initconst"
>> variable for each of them?  That would give us 16 bytes in each of the
>> two cases being freed after kernel initialization.
>>
>> However string literals aren't put in init.rodata either even though
>> they're declared in an __init function.  E.g. early_sn_setup() contains
>> a string literal "failed to find SAL entry point\n".  That's 31 bytes.
>> esi_init() contains even 103 bytes of string literals.
>>
>> So I'm not sure if it's worth to move the GUIDs to init.rodata (at the
>> expense of an additional LoC to declare a temporary variable) even
>> though we waste more space for string literals.
>>
>
> For the string literals I think there are some solutions possible
> involving compiler plugins. Kees?

Not yet, but coming. The "initify" plugin will move string literals to
init, IIUC. There are still some false-positive warnings generated by
modpost when initify is enabled, so I haven't push it into my -next
tree yet.

-Kees

-- 
Kees Cook
Nexus Security