efi/arm64: fix potential NULL dereference of efi.systab

PATCH New Ard Biesheuvel 4 participants 14 messages Jul 2, 2014 – Jul 8, 2014

Subsystems (2)

arm64 EFI

lore

Share & tools

lore

Ard Biesheuvel

author

2014-07-02 10:10 UTC #0 lore raw

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

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 <[email protected]>

---
 arch/arm64/kernel/efi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

arch/arm64/kernel/efi.c +5 -2

index 56c3327bbf79..e785f93f17cb 100644
@@ -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);

Signature · 1 line

1.8.3.2

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().

Ard Biesheuvel

author

2014-07-03 16:04 UTC #3 lore raw

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

Quoted 26 lines · Mark Salter

Quoted 16 lines · Ard Biesheuvel
Quoted 16 lines · 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 <[email protected]>
---
 arch/arm64/kernel/efi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
arch/arm64/kernel/efi.c +0 -0
index 56c3327bbf79..e785f93f17cb 100644
@@ -419,8 +419,11 @@ static int __init arm64_enter_virtual_mode(void)
        }
 
Quoted 8 lines
arch/arm64/kernel/efi.c static int __init arm64_enter_virtual_mode(void) · line 423
Quoted 5 lines
arch/arm64/kernel/efi.c +2 -2
        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.

Quoted 1 line

And probably should unmap all the remapped regions in virtmap first.

Yes.

Quoted 9 lines

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

Signature · 1 line

Ard.

Ard Biesheuvel

author

2014-07-02 10:10 UTC #4 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/firmware/efi/runtime-wrappers.c | 109 +++++++++++++++++++++++++++++---
 1 file changed, 99 insertions(+), 10 deletions(-)

drivers/firmware/efi/runtime-wrappers.c +99 ~~-10~~

index 10daa4bbb258..6588d054af99 100644
@@ -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)

Signature · 1 line

1.8.3.2

Matt Fleming

2014-07-07 20:29 UTC #5 lore raw

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

Quoted 8 lines · 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 <[email protected]>
---
 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().

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Ard Biesheuvel

author

2014-07-07 20:43 UTC #6 lore raw

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

Quoted 10 lines · Matt Fleming

Quoted 8 lines · 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 <[email protected]>
---
 drivers/firmware/efi/runtime-wrappers.c | 109 +++++++++++++++++++++++++++++---
 1 file changed, 99 insertions(+), 10 deletions(-)

Quoted 9 lines

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 ...

Ard Biesheuvel

author

2014-07-08 08:54 UTC #7 lore raw

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

Quoted 21 lines · Ard Biesheuvel

Quoted 10 lines · Matt Fleming
Quoted 8 lines · 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 <[email protected]>
---
 drivers/firmware/efi/runtime-wrappers.c | 109 +++++++++++++++++++++++++++++---
 1 file changed, 99 insertions(+), 10 deletions(-)
Quoted 9 lines
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().

Quoted 3 lines

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?

Signature · 1 line

Ard.

Matt Fleming

2014-07-08 09:29 UTC #8 lore raw

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

Quoted 4 lines · Ard Biesheuvel

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.

Quoted 3 lines

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.

Quoted 3 lines

- 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?

Quoted 3 lines

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.

Quoted 5 lines

- 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.

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Ard Biesheuvel

author

2014-07-08 09:45 UTC #9 lore raw

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

Quoted 10 lines · Matt Fleming

Quoted 4 lines · Ard Biesheuvel
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.

Quoted 9 lines

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.

Quoted 7 lines

Quoted 3 lines
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 ...

Quoted 7 lines

Quoted 3 lines
- 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.

Quoted 8 lines

Quoted 3 lines
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.

Quoted 10 lines

Quoted 5 lines
- 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.

Quoted 5 lines

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)

Signature · 1 line

Ard.

Matt Fleming

2014-07-08 09:52 UTC #10 lore raw

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

Quoted 5 lines · Ard Biesheuvel

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.

Quoted 4 lines

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!

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Catalin Marinas

maintainer

2014-07-04 13:38 UTC #11 lore raw

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

In reply to Ard Biesheuvel

Quoted 4 lines · 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 <[email protected]>

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

Signature · 1 line

Catalin

Ard Biesheuvel

author

2014-07-04 13:54 UTC #12 lore raw

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

Quoted 9 lines · Catalin Marinas

Quoted 4 lines · 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 <[email protected]>
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

Signature · 1 line

Ard.

Catalin Marinas

maintainer

2014-07-04 15:32 UTC #13 lore raw

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

Quoted 7 lines · Ard Biesheuvel

Quoted 6 lines · Catalin Marinas
Quoted 4 lines · 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 <[email protected]>

Quoted 8 lines

Quoted 3 lines
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.

Signature · 1 line

Catalin