[PATCH 1/2] efi/arm64: fix potential NULL dereference of efi.systab

[PATCH 1/2] efi/arm64: fix potential NULL dereference of efi.systab

[Ard Biesheuvel]

We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but
then go on an dereference it anyway.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm64/kernel/efi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index 56c3327bbf79..e785f93f17cb 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -419,8 +419,11 @@ static int __init arm64_enter_virtual_mode(void)
 	}
 
 	efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table);
-	if (efi.systab)
-		set_bit(EFI_SYSTEM_TABLES, &efi.flags);
+	if (!efi.systab) {
+		pr_err("Failed to remap EFI System Table!\n");
+		return -1;
+	}
+	set_bit(EFI_SYSTEM_TABLES, &efi.flags);
 
 	local_irq_save(flags);
 	cpu_switch_mm(idmap_pg_dir, &init_mm);
-- 
1.8.3.2

[PATCH 1/2] efi/arm64: fix potential NULL dereference of efi.systab

[Ard Biesheuvel]

We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but
then go on an dereference it anyway.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/efi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index 56c3327bbf79..e785f93f17cb 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -419,8 +419,11 @@ static int __init arm64_enter_virtual_mode(void)
 	}
 
 	efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table);
-	if (efi.systab)
-		set_bit(EFI_SYSTEM_TABLES, &efi.flags);
+	if (!efi.systab) {
+		pr_err("Failed to remap EFI System Table!\n");
+		return -1;
+	}
+	set_bit(EFI_SYSTEM_TABLES, &efi.flags);
 
 	local_irq_save(flags);
 	cpu_switch_mm(idmap_pg_dir, &init_mm);
-- 
1.8.3.2

[PATCH 2/2] efi: implement mandatory locking for UEFI Runtime Services

[Ard Biesheuvel]

According to section 7.1 of the UEFI spec, Runtime Services are not fully
reentrant, and there are particular combinations of calls that need to be
serialized.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/runtime-wrappers.c | 109 +++++++++++++++++++++++++++++---
 1 file changed, 99 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 10daa4bbb258..6588d054af99 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -15,10 +15,50 @@
  */
 
 #include <linux/efi.h>
-#include <linux/spinlock.h>             /* spinlock_t */
+#include <linux/mutex.h>
+#include <linux/spinlock.h>
 #include <asm/efi.h>
 
 /*
+ * According to section 7.1 of the UEFI spec, Runtime Services are not fully
+ * reentrant, and there are particular combinations of calls that need to be
+ * serialized.
+ *
+ * Table 31. Rules for Reentry Into Runtime Services
+ * +------------------------------------+-------------------------------+
+ * | If previous call is busy in	| Forbidden to call		|
+ * +------------------------------------+-------------------------------+
+ * | Any				| SetVirtualAddressMap()	|
+ * +------------------------------------+-------------------------------+
+ * | ConvertPointer()			| ConvertPointer()		|
+ * +------------------------------------+-------------------------------+
+ * | SetVariable()			| ResetSystem()			|
+ * | UpdateCapsule()			|				|
+ * | SetTime()				|				|
+ * | SetWakeupTime()			|				|
+ * | GetNextHighMonotonicCount()	|				|
+ * +------------------------------------+-------------------------------+
+ * | GetVariable()			| GetVariable()			|
+ * | GetNextVariableName()		| GetNextVariableName()		|
+ * | SetVariable()			| SetVariable()			|
+ * | QueryVariableInfo()		| QueryVariableInfo()		|
+ * | UpdateCapsule()			| UpdateCapsule()		|
+ * | QueryCapsuleCapabilities()		| QueryCapsuleCapabilities()	|
+ * | GetNextHighMonotonicCount()	| GetNextHighMonotonicCount()	|
+ * +------------------------------------+-------------------------------+
+ * | GetTime()				| GetTime()			|
+ * | SetTime()				| SetTime()			|
+ * | GetWakeupTime()			| GetWakeupTime()		|
+ * | SetWakeupTime()			| SetWakeupTime()		|
+ * +------------------------------------+-------------------------------+
+ *
+ * The first two are disregarded here, as SetVirtualAddressMap() is called
+ * only once, and very early, and ConvertPointer() is not exposed at all.
+ */
+static DEFINE_MUTEX(var_ro_mutex);
+static DEFINE_MUTEX(var_rw_mutex);
+
+/*
  * As per commit ef68c8f87ed1 ("x86: Serialize EFI time accesses on rtc_lock"),
  * the EFI specification requires that callers of the time related runtime
  * functions serialize with other CMOS accesses in the kernel, as the EFI time
@@ -78,14 +118,25 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name,
 					  unsigned long *data_size,
 					  void *data)
 {
-	return efi_call_virt(get_variable, name, vendor, attr, data_size, data);
+	efi_status_t status;
+
+	mutex_lock(&var_ro_mutex);
+	status = efi_call_virt(get_variable, name, vendor, attr, data_size,
+			       data);
+	mutex_unlock(&var_ro_mutex);
+	return status;
 }
 
 static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
 					       efi_char16_t *name,
 					       efi_guid_t *vendor)
 {
-	return efi_call_virt(get_next_variable, name_size, name, vendor);
+	efi_status_t status;
+
+	mutex_lock(&var_ro_mutex);
+	status = efi_call_virt(get_next_variable, name_size, name, vendor);
+	mutex_unlock(&var_ro_mutex);
+	return status;
 }
 
 static efi_status_t virt_efi_set_variable(efi_char16_t *name,
@@ -94,7 +145,15 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
 					  unsigned long data_size,
 					  void *data)
 {
-	return efi_call_virt(set_variable, name, vendor, attr, data_size, data);
+	efi_status_t status;
+
+	mutex_lock(&var_ro_mutex);
+	mutex_lock(&var_rw_mutex);
+	status = efi_call_virt(set_variable, name, vendor, attr, data_size,
+			       data);
+	mutex_unlock(&var_rw_mutex);
+	mutex_unlock(&var_ro_mutex);
+	return status;
 }
 
 static efi_status_t virt_efi_query_variable_info(u32 attr,
@@ -102,16 +161,28 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 						 u64 *remaining_space,
 						 u64 *max_variable_size)
 {
+	efi_status_t status;
+
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	return efi_call_virt(query_variable_info, attr, storage_space,
-			     remaining_space, max_variable_size);
+	mutex_lock(&var_ro_mutex);
+	status = efi_call_virt(query_variable_info, attr, storage_space,
+			       remaining_space, max_variable_size);
+	mutex_unlock(&var_ro_mutex);
+	return status;
 }
 
 static efi_status_t virt_efi_get_next_high_mono_count(u32 *count)
 {
-	return efi_call_virt(get_next_high_mono_count, count);
+	efi_status_t status;
+
+	mutex_lock(&var_ro_mutex);
+	mutex_lock(&var_rw_mutex);
+	status = efi_call_virt(get_next_high_mono_count, count);
+	mutex_unlock(&var_rw_mutex);
+	mutex_unlock(&var_ro_mutex);
+	return status;
 }
 
 static void virt_efi_reset_system(int reset_type,
@@ -119,17 +190,30 @@ static void virt_efi_reset_system(int reset_type,
 				  unsigned long data_size,
 				  efi_char16_t *data)
 {
+	unsigned long flags;
+
+	mutex_lock(&var_rw_mutex);
+	spin_lock_irqsave(&rtc_lock, flags);
 	__efi_call_virt(reset_system, reset_type, status, data_size, data);
+	spin_unlock_irqrestore(&rtc_lock, flags);
+	mutex_unlock(&var_rw_mutex);
 }
 
 static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules,
 					    unsigned long count,
 					    unsigned long sg_list)
 {
+	efi_status_t status;
+
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	return efi_call_virt(update_capsule, capsules, count, sg_list);
+	mutex_lock(&var_ro_mutex);
+	mutex_lock(&var_rw_mutex);
+	status = efi_call_virt(update_capsule, capsules, count, sg_list);
+	mutex_unlock(&var_rw_mutex);
+	mutex_unlock(&var_ro_mutex);
+	return status;
 }
 
 static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules,
@@ -137,11 +221,16 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules,
 						u64 *max_size,
 						int *reset_type)
 {
+	efi_status_t status;
+
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	return efi_call_virt(query_capsule_caps, capsules, count, max_size,
-			     reset_type);
+	mutex_lock(&var_ro_mutex);
+	status = efi_call_virt(query_capsule_caps, capsules, count, max_size,
+			       reset_type);
+	mutex_unlock(&var_ro_mutex);
+	return status;
 }
 
 void efi_native_runtime_setup(void)
-- 
1.8.3.2

[PATCH 2/2] efi: implement mandatory locking for UEFI Runtime Services

[Ard Biesheuvel]

According to section 7.1 of the UEFI spec, Runtime Services are not fully
reentrant, and there are particular combinations of calls that need to be
serialized.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/runtime-wrappers.c | 109 +++++++++++++++++++++++++++++---
 1 file changed, 99 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 10daa4bbb258..6588d054af99 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -15,10 +15,50 @@
  */
 
 #include <linux/efi.h>
-#include <linux/spinlock.h>             /* spinlock_t */
+#include <linux/mutex.h>
+#include <linux/spinlock.h>
 #include <asm/efi.h>
 
 /*
+ * According to section 7.1 of the UEFI spec, Runtime Services are not fully
+ * reentrant, and there are particular combinations of calls that need to be
+ * serialized.
+ *
+ * Table 31. Rules for Reentry Into Runtime Services
+ * +------------------------------------+-------------------------------+
+ * | If previous call is busy in	| Forbidden to call		|
+ * +------------------------------------+-------------------------------+
+ * | Any				| SetVirtualAddressMap()	|
+ * +------------------------------------+-------------------------------+
+ * | ConvertPointer()			| ConvertPointer()		|
+ * +------------------------------------+-------------------------------+
+ * | SetVariable()			| ResetSystem()			|
+ * | UpdateCapsule()			|				|
+ * | SetTime()				|				|
+ * | SetWakeupTime()			|				|
+ * | GetNextHighMonotonicCount()	|				|
+ * +------------------------------------+-------------------------------+
+ * | GetVariable()			| GetVariable()			|
+ * | GetNextVariableName()		| GetNextVariableName()		|
+ * | SetVariable()			| SetVariable()			|
+ * | QueryVariableInfo()		| QueryVariableInfo()		|
+ * | UpdateCapsule()			| UpdateCapsule()		|
+ * | QueryCapsuleCapabilities()		| QueryCapsuleCapabilities()	|
+ * | GetNextHighMonotonicCount()	| GetNextHighMonotonicCount()	|
+ * +------------------------------------+-------------------------------+
+ * | GetTime()				| GetTime()			|
+ * | SetTime()				| SetTime()			|
+ * | GetWakeupTime()			| GetWakeupTime()		|
+ * | SetWakeupTime()			| SetWakeupTime()		|
+ * +------------------------------------+-------------------------------+
+ *
+ * The first two are disregarded here, as SetVirtualAddressMap() is called
+ * only once, and very early, and ConvertPointer() is not exposed at all.
+ */
+static DEFINE_MUTEX(var_ro_mutex);
+static DEFINE_MUTEX(var_rw_mutex);
+
+/*
  * As per commit ef68c8f87ed1 ("x86: Serialize EFI time accesses on rtc_lock"),
  * the EFI specification requires that callers of the time related runtime
  * functions serialize with other CMOS accesses in the kernel, as the EFI time
@@ -78,14 +118,25 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name,
 					  unsigned long *data_size,
 					  void *data)
 {
-	return efi_call_virt(get_variable, name, vendor, attr, data_size, data);
+	efi_status_t status;
+
+	mutex_lock(&var_ro_mutex);
+	status = efi_call_virt(get_variable, name, vendor, attr, data_size,
+			       data);
+	mutex_unlock(&var_ro_mutex);
+	return status;
 }
 
 static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
 					       efi_char16_t *name,
 					       efi_guid_t *vendor)
 {
-	return efi_call_virt(get_next_variable, name_size, name, vendor);
+	efi_status_t status;
+
+	mutex_lock(&var_ro_mutex);
+	status = efi_call_virt(get_next_variable, name_size, name, vendor);
+	mutex_unlock(&var_ro_mutex);
+	return status;
 }
 
 static efi_status_t virt_efi_set_variable(efi_char16_t *name,
@@ -94,7 +145,15 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
 					  unsigned long data_size,
 					  void *data)
 {
-	return efi_call_virt(set_variable, name, vendor, attr, data_size, data);
+	efi_status_t status;
+
+	mutex_lock(&var_ro_mutex);
+	mutex_lock(&var_rw_mutex);
+	status = efi_call_virt(set_variable, name, vendor, attr, data_size,
+			       data);
+	mutex_unlock(&var_rw_mutex);
+	mutex_unlock(&var_ro_mutex);
+	return status;
 }
 
 static efi_status_t virt_efi_query_variable_info(u32 attr,
@@ -102,16 +161,28 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 						 u64 *remaining_space,
 						 u64 *max_variable_size)
 {
+	efi_status_t status;
+
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	return efi_call_virt(query_variable_info, attr, storage_space,
-			     remaining_space, max_variable_size);
+	mutex_lock(&var_ro_mutex);
+	status = efi_call_virt(query_variable_info, attr, storage_space,
+			       remaining_space, max_variable_size);
+	mutex_unlock(&var_ro_mutex);
+	return status;
 }
 
 static efi_status_t virt_efi_get_next_high_mono_count(u32 *count)
 {
-	return efi_call_virt(get_next_high_mono_count, count);
+	efi_status_t status;
+
+	mutex_lock(&var_ro_mutex);
+	mutex_lock(&var_rw_mutex);
+	status = efi_call_virt(get_next_high_mono_count, count);
+	mutex_unlock(&var_rw_mutex);
+	mutex_unlock(&var_ro_mutex);
+	return status;
 }
 
 static void virt_efi_reset_system(int reset_type,
@@ -119,17 +190,30 @@ static void virt_efi_reset_system(int reset_type,
 				  unsigned long data_size,
 				  efi_char16_t *data)
 {
+	unsigned long flags;
+
+	mutex_lock(&var_rw_mutex);
+	spin_lock_irqsave(&rtc_lock, flags);
 	__efi_call_virt(reset_system, reset_type, status, data_size, data);
+	spin_unlock_irqrestore(&rtc_lock, flags);
+	mutex_unlock(&var_rw_mutex);
 }
 
 static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules,
 					    unsigned long count,
 					    unsigned long sg_list)
 {
+	efi_status_t status;
+
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	return efi_call_virt(update_capsule, capsules, count, sg_list);
+	mutex_lock(&var_ro_mutex);
+	mutex_lock(&var_rw_mutex);
+	status = efi_call_virt(update_capsule, capsules, count, sg_list);
+	mutex_unlock(&var_rw_mutex);
+	mutex_unlock(&var_ro_mutex);
+	return status;
 }
 
 static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules,
@@ -137,11 +221,16 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules,
 						u64 *max_size,
 						int *reset_type)
 {
+	efi_status_t status;
+
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	return efi_call_virt(query_capsule_caps, capsules, count, max_size,
-			     reset_type);
+	mutex_lock(&var_ro_mutex);
+	status = efi_call_virt(query_capsule_caps, capsules, count, max_size,
+			       reset_type);
+	mutex_unlock(&var_ro_mutex);
+	return status;
 }
 
 void efi_native_runtime_setup(void)
-- 
1.8.3.2

Re: [PATCH 2/2] efi: implement mandatory locking for UEFI Runtime Services

[Matt Fleming]

On Wed, 02 Jul, at 12:10:02PM, Ard Biesheuvel wrote:
> According to section 7.1 of the UEFI spec, Runtime Services are not fully
> reentrant, and there are particular combinations of calls that need to be
> serialized.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/runtime-wrappers.c | 109 +++++++++++++++++++++++++++++---
>  1 file changed, 99 insertions(+), 10 deletions(-)

Ard, what's going on with this one? I didn't see it resubmitted along
with v3 of "efi/arm64: handle missing virtual mapping for UEFI System
Table".

Note that we already have a lock to serialize access to the UEFI
variable services in the form of __efivars->lock, see
drivers/firmware/efi/vars.c. It's a spinlock because of the context we
may need to create variables in from efi_pstore_write().

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH 2/2] efi: implement mandatory locking for UEFI Runtime Services

[Matt Fleming]

On Wed, 02 Jul, at 12:10:02PM, Ard Biesheuvel wrote:
> According to section 7.1 of the UEFI spec, Runtime Services are not fully
> reentrant, and there are particular combinations of calls that need to be
> serialized.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/firmware/efi/runtime-wrappers.c | 109 +++++++++++++++++++++++++++++---
>  1 file changed, 99 insertions(+), 10 deletions(-)

Ard, what's going on with this one? I didn't see it resubmitted along
with v3 of "efi/arm64: handle missing virtual mapping for UEFI System
Table".

Note that we already have a lock to serialize access to the UEFI
variable services in the form of __efivars->lock, see
drivers/firmware/efi/vars.c. It's a spinlock because of the context we
may need to create variables in from efi_pstore_write().

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/2] efi: implement mandatory locking for UEFI Runtime Services

[Ard Biesheuvel]

On 7 July 2014 22:29, Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org> wrote:
> On Wed, 02 Jul, at 12:10:02PM, Ard Biesheuvel wrote:
>> According to section 7.1 of the UEFI spec, Runtime Services are not fully
>> reentrant, and there are particular combinations of calls that need to be
>> serialized.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  drivers/firmware/efi/runtime-wrappers.c | 109 +++++++++++++++++++++++++++++---
>>  1 file changed, 99 insertions(+), 10 deletions(-)
>
> Ard, what's going on with this one? I didn't see it resubmitted along
> with v3 of "efi/arm64: handle missing virtual mapping for UEFI System
> Table".
>
> Note that we already have a lock to serialize access to the UEFI
> variable services in the form of __efivars->lock, see
> drivers/firmware/efi/vars.c. It's a spinlock because of the context we
> may need to create variables in from efi_pstore_write().
>

As the patch says, the UEFI spec is very clear on which combinations
of calls are not reentrant.
I don't think the rtc_lock and the efivars->lock quite cover that completely ...

[PATCH 2/2] efi: implement mandatory locking for UEFI Runtime Services

[Ard Biesheuvel]

On 7 July 2014 22:29, Matt Fleming <matt@console-pimps.org> wrote:
> On Wed, 02 Jul, at 12:10:02PM, Ard Biesheuvel wrote:
>> According to section 7.1 of the UEFI spec, Runtime Services are not fully
>> reentrant, and there are particular combinations of calls that need to be
>> serialized.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  drivers/firmware/efi/runtime-wrappers.c | 109 +++++++++++++++++++++++++++++---
>>  1 file changed, 99 insertions(+), 10 deletions(-)
>
> Ard, what's going on with this one? I didn't see it resubmitted along
> with v3 of "efi/arm64: handle missing virtual mapping for UEFI System
> Table".
>
> Note that we already have a lock to serialize access to the UEFI
> variable services in the form of __efivars->lock, see
> drivers/firmware/efi/vars.c. It's a spinlock because of the context we
> may need to create variables in from efi_pstore_write().
>

As the patch says, the UEFI spec is very clear on which combinations
of calls are not reentrant.
I don't think the rtc_lock and the efivars->lock quite cover that completely ...

Re: [PATCH 2/2] efi: implement mandatory locking for UEFI Runtime Services

[Ard Biesheuvel]

On 7 July 2014 22:43, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 7 July 2014 22:29, Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org> wrote:
>> On Wed, 02 Jul, at 12:10:02PM, Ard Biesheuvel wrote:
>>> According to section 7.1 of the UEFI spec, Runtime Services are not fully
>>> reentrant, and there are particular combinations of calls that need to be
>>> serialized.
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> ---
>>>  drivers/firmware/efi/runtime-wrappers.c | 109 +++++++++++++++++++++++++++++---
>>>  1 file changed, 99 insertions(+), 10 deletions(-)
>>
>> Ard, what's going on with this one? I didn't see it resubmitted along
>> with v3 of "efi/arm64: handle missing virtual mapping for UEFI System
>> Table".
>>
>> Note that we already have a lock to serialize access to the UEFI
>> variable services in the form of __efivars->lock, see
>> drivers/firmware/efi/vars.c. It's a spinlock because of the context we
>> may need to create variables in from efi_pstore_write().
>>
>
> As the patch says, the UEFI spec is very clear on which combinations
> of calls are not reentrant.
> I don't think the rtc_lock and the efivars->lock quite cover that completely ...

After doing a bit more research, I still think there is work needed if
we aim to adhere to the UEFI spec, or at least be safe from the
hazards it points out.

So the current status is:
- get/set time calls are serialized with respect to one another using
rtc_lock at the wrapper level
- get/set variable calls are serialized using the efivars->lock in the
efivars module
- get_next_variable() calls use the BKL

The two things I am most concerned with are:
- reset system while other calls are in flight; is this handled
implicitly in some other way?
- things like settime()/setwakeuptime() and setvariable() poking into
the flash at the same time.

Perhaps it would be sufficient to have a single spinlock cover all
these cases? Or just let efivars grab the rtc_lock as well?

-- 
Ard.

[PATCH 2/2] efi: implement mandatory locking for UEFI Runtime Services

[Ard Biesheuvel]

On 7 July 2014 22:43, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 7 July 2014 22:29, Matt Fleming <matt@console-pimps.org> wrote:
>> On Wed, 02 Jul, at 12:10:02PM, Ard Biesheuvel wrote:
>>> According to section 7.1 of the UEFI spec, Runtime Services are not fully
>>> reentrant, and there are particular combinations of calls that need to be
>>> serialized.
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  drivers/firmware/efi/runtime-wrappers.c | 109 +++++++++++++++++++++++++++++---
>>>  1 file changed, 99 insertions(+), 10 deletions(-)
>>
>> Ard, what's going on with this one? I didn't see it resubmitted along
>> with v3 of "efi/arm64: handle missing virtual mapping for UEFI System
>> Table".
>>
>> Note that we already have a lock to serialize access to the UEFI
>> variable services in the form of __efivars->lock, see
>> drivers/firmware/efi/vars.c. It's a spinlock because of the context we
>> may need to create variables in from efi_pstore_write().
>>
>
> As the patch says, the UEFI spec is very clear on which combinations
> of calls are not reentrant.
> I don't think the rtc_lock and the efivars->lock quite cover that completely ...

After doing a bit more research, I still think there is work needed if
we aim to adhere to the UEFI spec, or at least be safe from the
hazards it points out.

So the current status is:
- get/set time calls are serialized with respect to one another using
rtc_lock at the wrapper level
- get/set variable calls are serialized using the efivars->lock in the
efivars module
- get_next_variable() calls use the BKL

The two things I am most concerned with are:
- reset system while other calls are in flight; is this handled
implicitly in some other way?
- things like settime()/setwakeuptime() and setvariable() poking into
the flash at the same time.

Perhaps it would be sufficient to have a single spinlock cover all
these cases? Or just let efivars grab the rtc_lock as well?

-- 
Ard.

Re: [PATCH 2/2] efi: implement mandatory locking for UEFI Runtime Services

[Matt Fleming]

On Tue, 08 Jul, at 10:54:13AM, Ard Biesheuvel wrote:
> 
> After doing a bit more research, I still think there is work needed if
> we aim to adhere to the UEFI spec, or at least be safe from the
> hazards it points out.
 
Note that I never claimed there wasn't a need for an EFI runtime lock, I
was just pointing out that you need to consider the efi-pstore scenario,
and that a mutex isn't suitable in that case.

I did in fact make a half-arsed attempt at introducing a runtime lock
here,

  http://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=capsule-blkdev&id=c0a88ac5b21f3c837121748be2e59e995126a6cb

Provided we can get away with a single EFI runtime lock like that patch,
your recent efi_call_virt() changes actually make the required patch
much simpler, at least for arm64 and x86.

> So the current status is:
> - get/set time calls are serialized with respect to one another using
> rtc_lock at the wrapper level

The time functions are completely unused on x86, which is why no proper
runtime locking exists. It's basically dead code.

> - get/set variable calls are serialized using the efivars->lock in the
> efivars module
> - get_next_variable() calls use the BKL

It uses __efivars->lock just like the other variable services. Is that
what you meant by BKL?
 
> The two things I am most concerned with are:
> - reset system while other calls are in flight; is this handled
> implicitly in some other way?

No, it isn't handled, so yeah, it needs fixing. I think on x86 we
actually wait for other cpus to shutdown before issuing the reset but
it's obviously not possible to make that guarantee across architectures.

> - things like settime()/setwakeuptime() and setvariable() poking into
> the flash at the same time.
> 
> Perhaps it would be sufficient to have a single spinlock cover all
> these cases? Or just let efivars grab the rtc_lock as well?

I think we need to introduce a separate lock, logically below all other
subsystem specific ones (rtc_lock, __efivars->lock, etc). It needs to be
the final lock you grab before invoking the runtime services.

I don't think the additional complexity of introducing multiple locks to
parallelise access to, say, GetTime() and GetVariable(), is really worth
the headache. Definitely not without someone making a really compelling
case for why they need to squeeze every ounce of performance out of that
scenario.

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH 2/2] efi: implement mandatory locking for UEFI Runtime Services

[Matt Fleming]

On Tue, 08 Jul, at 10:54:13AM, Ard Biesheuvel wrote:
> 
> After doing a bit more research, I still think there is work needed if
> we aim to adhere to the UEFI spec, or at least be safe from the
> hazards it points out.
 
Note that I never claimed there wasn't a need for an EFI runtime lock, I
was just pointing out that you need to consider the efi-pstore scenario,
and that a mutex isn't suitable in that case.

I did in fact make a half-arsed attempt at introducing a runtime lock
here,

  http://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=capsule-blkdev&id=c0a88ac5b21f3c837121748be2e59e995126a6cb

Provided we can get away with a single EFI runtime lock like that patch,
your recent efi_call_virt() changes actually make the required patch
much simpler, at least for arm64 and x86.

> So the current status is:
> - get/set time calls are serialized with respect to one another using
> rtc_lock at the wrapper level

The time functions are completely unused on x86, which is why no proper
runtime locking exists. It's basically dead code.

> - get/set variable calls are serialized using the efivars->lock in the
> efivars module
> - get_next_variable() calls use the BKL

It uses __efivars->lock just like the other variable services. Is that
what you meant by BKL?
 
> The two things I am most concerned with are:
> - reset system while other calls are in flight; is this handled
> implicitly in some other way?

No, it isn't handled, so yeah, it needs fixing. I think on x86 we
actually wait for other cpus to shutdown before issuing the reset but
it's obviously not possible to make that guarantee across architectures.

> - things like settime()/setwakeuptime() and setvariable() poking into
> the flash at the same time.
> 
> Perhaps it would be sufficient to have a single spinlock cover all
> these cases? Or just let efivars grab the rtc_lock as well?

I think we need to introduce a separate lock, logically below all other
subsystem specific ones (rtc_lock, __efivars->lock, etc). It needs to be
the final lock you grab before invoking the runtime services.

I don't think the additional complexity of introducing multiple locks to
parallelise access to, say, GetTime() and GetVariable(), is really worth
the headache. Definitely not without someone making a really compelling
case for why they need to squeeze every ounce of performance out of that
scenario.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/2] efi: implement mandatory locking for UEFI Runtime Services

[Ard Biesheuvel]

On 8 July 2014 11:29, Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org> wrote:
> On Tue, 08 Jul, at 10:54:13AM, Ard Biesheuvel wrote:
>>
>> After doing a bit more research, I still think there is work needed if
>> we aim to adhere to the UEFI spec, or at least be safe from the
>> hazards it points out.
>
> Note that I never claimed there wasn't a need for an EFI runtime lock, I
> was just pointing out that you need to consider the efi-pstore scenario,
> and that a mutex isn't suitable in that case.
>
> I did in fact make a half-arsed attempt at introducing a runtime lock
> here,
>
>   http://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=capsule-blkdev&id=c0a88ac5b21f3c837121748be2e59e995126a6cb
>
> Provided we can get away with a single EFI runtime lock like that patch,
> your recent efi_call_virt() changes actually make the required patch
> much simpler, at least for arm64 and x86.
>

Indeed.

>> So the current status is:
>> - get/set time calls are serialized with respect to one another using
>> rtc_lock at the wrapper level
>
> The time functions are completely unused on x86, which is why no proper
> runtime locking exists. It's basically dead code.
>

OK. That may be different on ARM, though ...

>> - get/set variable calls are serialized using the efivars->lock in the
>> efivars module
>> - get_next_variable() calls use the BKL
>
> It uses __efivars->lock just like the other variable services. Is that
> what you meant by BKL?
>

Well, that is what is says in the comment:
         * ops.get_next_variable() is only called from register_efivars()
         * or efivar_update_sysfs_entries(),
         * which is protected by the BKL, so that path is safe.

>> The two things I am most concerned with are:
>> - reset system while other calls are in flight; is this handled
>> implicitly in some other way?
>
> No, it isn't handled, so yeah, it needs fixing. I think on x86 we
> actually wait for other cpus to shutdown before issuing the reset but
> it's obviously not possible to make that guarantee across architectures.
>

Exactly.

>> - things like settime()/setwakeuptime() and setvariable() poking into
>> the flash at the same time.
>>
>> Perhaps it would be sufficient to have a single spinlock cover all
>> these cases? Or just let efivars grab the rtc_lock as well?
>
> I think we need to introduce a separate lock, logically below all other
> subsystem specific ones (rtc_lock, __efivars->lock, etc). It needs to be
> the final lock you grab before invoking the runtime services.
>
> I don't think the additional complexity of introducing multiple locks to
> parallelise access to, say, GetTime() and GetVariable(), is really worth
> the headache. Definitely not without someone making a really compelling
> case for why they need to squeeze every ounce of performance out of that
> scenario.


I agree. So shall I update my patch to add a single spin_lock that is
used by all wrappers?
We will likely need the wrappers on ARM as well, only ia64 needs
another approach (if desired)

-- 
Ard.

[PATCH 2/2] efi: implement mandatory locking for UEFI Runtime Services

[Ard Biesheuvel]

On 8 July 2014 11:29, Matt Fleming <matt@console-pimps.org> wrote:
> On Tue, 08 Jul, at 10:54:13AM, Ard Biesheuvel wrote:
>>
>> After doing a bit more research, I still think there is work needed if
>> we aim to adhere to the UEFI spec, or at least be safe from the
>> hazards it points out.
>
> Note that I never claimed there wasn't a need for an EFI runtime lock, I
> was just pointing out that you need to consider the efi-pstore scenario,
> and that a mutex isn't suitable in that case.
>
> I did in fact make a half-arsed attempt at introducing a runtime lock
> here,
>
>   http://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=capsule-blkdev&id=c0a88ac5b21f3c837121748be2e59e995126a6cb
>
> Provided we can get away with a single EFI runtime lock like that patch,
> your recent efi_call_virt() changes actually make the required patch
> much simpler, at least for arm64 and x86.
>

Indeed.

>> So the current status is:
>> - get/set time calls are serialized with respect to one another using
>> rtc_lock at the wrapper level
>
> The time functions are completely unused on x86, which is why no proper
> runtime locking exists. It's basically dead code.
>

OK. That may be different on ARM, though ...

>> - get/set variable calls are serialized using the efivars->lock in the
>> efivars module
>> - get_next_variable() calls use the BKL
>
> It uses __efivars->lock just like the other variable services. Is that
> what you meant by BKL?
>

Well, that is what is says in the comment:
         * ops.get_next_variable() is only called from register_efivars()
         * or efivar_update_sysfs_entries(),
         * which is protected by the BKL, so that path is safe.

>> The two things I am most concerned with are:
>> - reset system while other calls are in flight; is this handled
>> implicitly in some other way?
>
> No, it isn't handled, so yeah, it needs fixing. I think on x86 we
> actually wait for other cpus to shutdown before issuing the reset but
> it's obviously not possible to make that guarantee across architectures.
>

Exactly.

>> - things like settime()/setwakeuptime() and setvariable() poking into
>> the flash at the same time.
>>
>> Perhaps it would be sufficient to have a single spinlock cover all
>> these cases? Or just let efivars grab the rtc_lock as well?
>
> I think we need to introduce a separate lock, logically below all other
> subsystem specific ones (rtc_lock, __efivars->lock, etc). It needs to be
> the final lock you grab before invoking the runtime services.
>
> I don't think the additional complexity of introducing multiple locks to
> parallelise access to, say, GetTime() and GetVariable(), is really worth
> the headache. Definitely not without someone making a really compelling
> case for why they need to squeeze every ounce of performance out of that
> scenario.


I agree. So shall I update my patch to add a single spin_lock that is
used by all wrappers?
We will likely need the wrappers on ARM as well, only ia64 needs
another approach (if desired)

-- 
Ard.

Re: [PATCH 2/2] efi: implement mandatory locking for UEFI Runtime Services

[Matt Fleming]

On Tue, 08 Jul, at 11:45:03AM, Ard Biesheuvel wrote:
> 
> Well, that is what is says in the comment:
>          * ops.get_next_variable() is only called from register_efivars()
>          * or efivar_update_sysfs_entries(),
>          * which is protected by the BKL, so that path is safe.
 
Oops, so it does. That's a stale comment. I'll update it.

> I agree. So shall I update my patch to add a single spin_lock that is
> used by all wrappers?
> We will likely need the wrappers on ARM as well, only ia64 needs
> another approach (if desired)

Please do, that would be great!

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH 2/2] efi: implement mandatory locking for UEFI Runtime Services

[Matt Fleming]

On Tue, 08 Jul, at 11:45:03AM, Ard Biesheuvel wrote:
> 
> Well, that is what is says in the comment:
>          * ops.get_next_variable() is only called from register_efivars()
>          * or efivar_update_sysfs_entries(),
>          * which is protected by the BKL, so that path is safe.
 
Oops, so it does. That's a stale comment. I'll update it.

> I agree. So shall I update my patch to add a single spin_lock that is
> used by all wrappers?
> We will likely need the wrappers on ARM as well, only ia64 needs
> another approach (if desired)

Please do, that would be great!

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 1/2] efi/arm64: fix potential NULL dereference of efi.systab

[Ard Biesheuvel]

> On 2 July 2014 12:10, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but
> then go on an dereference it anyway.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm64/kernel/efi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index 56c3327bbf79..e785f93f17cb 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -419,8 +419,11 @@ static int __init arm64_enter_virtual_mode(void)
>         }
>
>         efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table);
> -       if (efi.systab)
> -               set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> +       if (!efi.systab) {
> +               pr_err("Failed to remap EFI System Table!\n");

... this needs a kfree(virtmap) as well.

> +               return -1;
> +       }
> +       set_bit(EFI_SYSTEM_TABLES, &efi.flags);
>
>         local_irq_save(flags);
>         cpu_switch_mm(idmap_pg_dir, &init_mm);
> --
> 1.8.3.2
>

[PATCH 1/2] efi/arm64: fix potential NULL dereference of efi.systab

[Ard Biesheuvel]

> On 2 July 2014 12:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but
> then go on an dereference it anyway.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/efi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index 56c3327bbf79..e785f93f17cb 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -419,8 +419,11 @@ static int __init arm64_enter_virtual_mode(void)
>         }
>
>         efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table);
> -       if (efi.systab)
> -               set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> +       if (!efi.systab) {
> +               pr_err("Failed to remap EFI System Table!\n");

... this needs a kfree(virtmap) as well.

> +               return -1;
> +       }
> +       set_bit(EFI_SYSTEM_TABLES, &efi.flags);
>
>         local_irq_save(flags);
>         cpu_switch_mm(idmap_pg_dir, &init_mm);
> --
> 1.8.3.2
>

Re: [PATCH 1/2] efi/arm64: fix potential NULL dereference of efi.systab

[Mark Salter]

On Wed, 2014-07-02 at 12:13 +0200, Ard Biesheuvel wrote:
> > On 2 July 2014 12:10, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but
> > then go on an dereference it anyway.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > ---
> >  arch/arm64/kernel/efi.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> > index 56c3327bbf79..e785f93f17cb 100644
> > --- a/arch/arm64/kernel/efi.c
> > +++ b/arch/arm64/kernel/efi.c
> > @@ -419,8 +419,11 @@ static int __init arm64_enter_virtual_mode(void)
> >         }
> >
> >         efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table);
> > -       if (efi.systab)
> > -               set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> > +       if (!efi.systab) {
> > +               pr_err("Failed to remap EFI System Table!\n");
> 
> ... this needs a kfree(virtmap) as well.
> 

And probably should unmap all the remapped regions in virtmap first.
Presumably the systab will be in a runtime memory region which gets
virtual mapping from remap_region(). If that succeeds, then the
efi_lookup_mapped_addr should always succeed. But to be careful,
we should probably bail out earlier if remap_region() ever returns
zero. If all remaps work and efi_lookup_mapped_addr() fails, then
we should try ioremap_cache() directly. Or do what x86 does and make
a local copy of the table earlier when we early_memremap() it in
uefi_init().

[PATCH 1/2] efi/arm64: fix potential NULL dereference of efi.systab

[Mark Salter]

On Wed, 2014-07-02 at 12:13 +0200, Ard Biesheuvel wrote:
> > On 2 July 2014 12:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but
> > then go on an dereference it anyway.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  arch/arm64/kernel/efi.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> > index 56c3327bbf79..e785f93f17cb 100644
> > --- a/arch/arm64/kernel/efi.c
> > +++ b/arch/arm64/kernel/efi.c
> > @@ -419,8 +419,11 @@ static int __init arm64_enter_virtual_mode(void)
> >         }
> >
> >         efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table);
> > -       if (efi.systab)
> > -               set_bit(EFI_SYSTEM_TABLES, &efi.flags);
> > +       if (!efi.systab) {
> > +               pr_err("Failed to remap EFI System Table!\n");
> 
> ... this needs a kfree(virtmap) as well.
> 

And probably should unmap all the remapped regions in virtmap first.
Presumably the systab will be in a runtime memory region which gets
virtual mapping from remap_region(). If that succeeds, then the
efi_lookup_mapped_addr should always succeed. But to be careful,
we should probably bail out earlier if remap_region() ever returns
zero. If all remaps work and efi_lookup_mapped_addr() fails, then
we should try ioremap_cache() directly. Or do what x86 does and make
a local copy of the table earlier when we early_memremap() it in
uefi_init().

Re: [PATCH 1/2] efi/arm64: fix potential NULL dereference of efi.systab

[Ard Biesheuvel]

On 2 July 2014 16:29, Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Wed, 2014-07-02 at 12:13 +0200, Ard Biesheuvel wrote:
>> > On 2 July 2014 12:10, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> > We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but
>> > then go on an dereference it anyway.
>> >
>> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> > ---
>> >  arch/arm64/kernel/efi.c | 7 +++++--
>> >  1 file changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
>> > index 56c3327bbf79..e785f93f17cb 100644
>> > --- a/arch/arm64/kernel/efi.c
>> > +++ b/arch/arm64/kernel/efi.c
>> > @@ -419,8 +419,11 @@ static int __init arm64_enter_virtual_mode(void)
>> >         }
>> >
>> >         efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table);
>> > -       if (efi.systab)
>> > -               set_bit(EFI_SYSTEM_TABLES, &efi.flags);
>> > +       if (!efi.systab) {
>> > +               pr_err("Failed to remap EFI System Table!\n");
>>
>> ... this needs a kfree(virtmap) as well.
>>
>
> And probably should unmap all the remapped regions in virtmap first.

Yes.

> Presumably the systab will be in a runtime memory region which gets
> virtual mapping from remap_region(). If that succeeds, then the
> efi_lookup_mapped_addr should always succeed. But to be careful,
> we should probably bail out earlier if remap_region() ever returns
> zero. If all remaps work and efi_lookup_mapped_addr() fails, then
> we should try ioremap_cache() directly. Or do what x86 does and make
> a local copy of the table earlier when we early_memremap() it in
> uefi_init().
>

Bailing early (and cleaning up) if any remap_region() call fails seems
like a wise thing to do.
But if they all succeed, and efi_lookup_mapped_addr() then fails to
resolve efi_system_table, it means it lives in a region which will
become inaccessible to the runtime services themselves after
set_virtual_address_map() has been called. So doing any kind of
fallback masks a severe firmware bug, which makes me think we should
just complain loudly and not enable UEFI bits any further

-- 
Ard.

[PATCH 1/2] efi/arm64: fix potential NULL dereference of efi.systab

[Ard Biesheuvel]

On 2 July 2014 16:29, Mark Salter <msalter@redhat.com> wrote:
> On Wed, 2014-07-02 at 12:13 +0200, Ard Biesheuvel wrote:
>> > On 2 July 2014 12:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> > We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but
>> > then go on an dereference it anyway.
>> >
>> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> > ---
>> >  arch/arm64/kernel/efi.c | 7 +++++--
>> >  1 file changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
>> > index 56c3327bbf79..e785f93f17cb 100644
>> > --- a/arch/arm64/kernel/efi.c
>> > +++ b/arch/arm64/kernel/efi.c
>> > @@ -419,8 +419,11 @@ static int __init arm64_enter_virtual_mode(void)
>> >         }
>> >
>> >         efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table);
>> > -       if (efi.systab)
>> > -               set_bit(EFI_SYSTEM_TABLES, &efi.flags);
>> > +       if (!efi.systab) {
>> > +               pr_err("Failed to remap EFI System Table!\n");
>>
>> ... this needs a kfree(virtmap) as well.
>>
>
> And probably should unmap all the remapped regions in virtmap first.

Yes.

> Presumably the systab will be in a runtime memory region which gets
> virtual mapping from remap_region(). If that succeeds, then the
> efi_lookup_mapped_addr should always succeed. But to be careful,
> we should probably bail out earlier if remap_region() ever returns
> zero. If all remaps work and efi_lookup_mapped_addr() fails, then
> we should try ioremap_cache() directly. Or do what x86 does and make
> a local copy of the table earlier when we early_memremap() it in
> uefi_init().
>

Bailing early (and cleaning up) if any remap_region() call fails seems
like a wise thing to do.
But if they all succeed, and efi_lookup_mapped_addr() then fails to
resolve efi_system_table, it means it lives in a region which will
become inaccessible to the runtime services themselves after
set_virtual_address_map() has been called. So doing any kind of
fallback masks a severe firmware bug, which makes me think we should
just complain loudly and not enable UEFI bits any further

-- 
Ard.

Re: [PATCH 1/2] efi/arm64: fix potential NULL dereference of efi.systab

[Catalin Marinas]

On Wed, Jul 02, 2014 at 11:10:01AM +0100, Ard Biesheuvel wrote:
> We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but
> then go on an dereference it anyway.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Thanks. I applied this one (the other patch needs to go in via a
different route).

-- 
Catalin

[PATCH 1/2] efi/arm64: fix potential NULL dereference of efi.systab

[Catalin Marinas]

On Wed, Jul 02, 2014 at 11:10:01AM +0100, Ard Biesheuvel wrote:
> We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but
> then go on an dereference it anyway.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Thanks. I applied this one (the other patch needs to go in via a
different route).

-- 
Catalin

Re: [PATCH 1/2] efi/arm64: fix potential NULL dereference of efi.systab

[Ard Biesheuvel]

On 4 July 2014 15:38, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> On Wed, Jul 02, 2014 at 11:10:01AM +0100, Ard Biesheuvel wrote:
>> We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but
>> then go on an dereference it anyway.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> Thanks. I applied this one (the other patch needs to go in via a
> different route).
>

Eh, actually, I proposed a v2 based on Mark's feedback, but I now see
that I accidentally removed you from the To list.

http://marc.info/?l=linux-arm-kernel&m=140446911506688&w=2

-- 
Ard.

[PATCH 1/2] efi/arm64: fix potential NULL dereference of efi.systab

[Ard Biesheuvel]

On 4 July 2014 15:38, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Wed, Jul 02, 2014 at 11:10:01AM +0100, Ard Biesheuvel wrote:
>> We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but
>> then go on an dereference it anyway.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Thanks. I applied this one (the other patch needs to go in via a
> different route).
>

Eh, actually, I proposed a v2 based on Mark's feedback, but I now see
that I accidentally removed you from the To list.

http://marc.info/?l=linux-arm-kernel&m=140446911506688&w=2

-- 
Ard.

Re: [PATCH 1/2] efi/arm64: fix potential NULL dereference of efi.systab

[Catalin Marinas]

On Fri, Jul 04, 2014 at 02:54:19PM +0100, Ard Biesheuvel wrote:
> On 4 July 2014 15:38, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> > On Wed, Jul 02, 2014 at 11:10:01AM +0100, Ard Biesheuvel wrote:
> >> We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but
> >> then go on an dereference it anyway.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >
> > Thanks. I applied this one (the other patch needs to go in via a
> > different route).
> >
> 
> Eh, actually, I proposed a v2 based on Mark's feedback, but I now see
> that I accidentally removed you from the To list.
> 
> http://marc.info/?l=linux-arm-kernel&m=140446911506688&w=2

OK. I'll wait for acks on the other version then.

-- 
Catalin

[PATCH 1/2] efi/arm64: fix potential NULL dereference of efi.systab

[Catalin Marinas]

On Fri, Jul 04, 2014 at 02:54:19PM +0100, Ard Biesheuvel wrote:
> On 4 July 2014 15:38, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Wed, Jul 02, 2014 at 11:10:01AM +0100, Ard Biesheuvel wrote:
> >> We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but
> >> then go on an dereference it anyway.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >
> > Thanks. I applied this one (the other patch needs to go in via a
> > different route).
> >
> 
> Eh, actually, I proposed a v2 based on Mark's feedback, but I now see
> that I accidentally removed you from the To list.
> 
> http://marc.info/?l=linux-arm-kernel&m=140446911506688&w=2

OK. I'll wait for acks on the other version then.

-- 
Catalin