[PATCH] efivars: prevent Oops if efi_enabled but no EFI runtime

[PATCH] efivars: prevent Oops if efi_enabled but no EFI runtime

[Marko Kohtala]

Since v3.3-rc4-5-g1adbfa3, there has been an Oops in register_efivars
calling ops->get_next_variable and ending up at EIP 0 during module init.

I boot 32-bit x86 kernel from 64-bit EFI bootloader.

The efi_enabled is true, but runtime is not available. The functions
are NULL due to 32/64-bit mismatch between kernel and EFI.

Signed-off-by: Marko Kohtala <marko.kohtala@gmail.com>
---
I currently see this on v3.4.2.

I could not figure out how I'm supposed to detect lack of runtime, so
I ended up with this quick and overly precise check that all required
functions are available. There may be other drivers that need to take
this new condition into account, so maybe Olof wants to make a better
fix.

 drivers/firmware/efivars.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 47408e8..612a097 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1223,12 +1223,16 @@ efivars_init(void)
 		return -ENOMEM;
 	}
 
-	ops.get_variable = efi.get_variable;
-	ops.set_variable = efi.set_variable;
-	ops.get_next_variable = efi.get_next_variable;
-	error = register_efivars(&__efivars, &ops, efi_kobj);
-	if (error)
-		goto err_put;
+	/* We may have efi_enabled for systab, but no runtime for variables.
+	 * Check the functions we need before proceeding. */
+	if (efi.get_variable && efi.set_variable && efi.get_next_variable) {
+		ops.get_variable = efi.get_variable;
+		ops.set_variable = efi.set_variable;
+		ops.get_next_variable = efi.get_next_variable;
+		error = register_efivars(&__efivars, &ops, efi_kobj);
+		if (error)
+			goto err_put;
+	}
 
 	/* Don't forget the systab entry */
 	error = sysfs_create_group(efi_kobj, &efi_subsys_attr_group);
@@ -1251,10 +1255,10 @@ err_put:
 static void __exit
 efivars_exit(void)
 {
-	if (efi_enabled) {
+	if (__efivars.kset)
 		unregister_efivars(&__efivars);
+	if (efi_kobj)
 		kobject_put(efi_kobj);
-	}
 }
 
 module_init(efivars_init);
-- 
1.7.10.4

Re: [PATCH] efivars: prevent Oops if efi_enabled but no EFI runtime

[Olof Johansson]

On Thu, Jun 14, 2012 at 1:47 PM, Marko Kohtala <marko.kohtala@gmail.com> wrote:
> Since v3.3-rc4-5-g1adbfa3, there has been an Oops in register_efivars
> calling ops->get_next_variable and ending up at EIP 0 during module init.
>
> I boot 32-bit x86 kernel from 64-bit EFI bootloader.
>
> The efi_enabled is true, but runtime is not available. The functions
> are NULL due to 32/64-bit mismatch between kernel and EFI.
>
> Signed-off-by: Marko Kohtala <marko.kohtala@gmail.com>
> ---
> I currently see this on v3.4.2.
>
> I could not figure out how I'm supposed to detect lack of runtime, so
> I ended up with this quick and overly precise check that all required
> functions are available. There may be other drivers that need to take
> this new condition into account, so maybe Olof wants to make a better
> fix.

I must not have tested with efivars enabled, or I would have seen this
when I did. Sigh.


> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 47408e8..612a097 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -1223,12 +1223,16 @@ efivars_init(void)
>                return -ENOMEM;
>        }
>
> -       ops.get_variable = efi.get_variable;
> -       ops.set_variable = efi.set_variable;
> -       ops.get_next_variable = efi.get_next_variable;
> -       error = register_efivars(&__efivars, &ops, efi_kobj);
> -       if (error)
> -               goto err_put;
> +       /* We may have efi_enabled for systab, but no runtime for variables.
> +        * Check the functions we need before proceeding. */
> +       if (efi.get_variable && efi.set_variable && efi.get_next_variable) {
> +               ops.get_variable = efi.get_variable;
> +               ops.set_variable = efi.set_variable;
> +               ops.get_next_variable = efi.get_next_variable;
> +               error = register_efivars(&__efivars, &ops, efi_kobj);
> +               if (error)
> +                       goto err_put;
> +       }

I think it would make more sense to return -ENODEV when the function
pointers aren't set, that way the exit function won't ever be called
either, so no need to add the checks there.

So, instead of current efi_enabled check:

if (!efi_enabled ||
    !efi.get_variable ||
    !efi.set_variable ||
    !efi.get_next_variable) {
    return -ENODEV;
}

-Olof

Re: [PATCH] efivars: prevent Oops if efi_enabled but no EFI runtime

[Marko Kohtala]

2012/6/18 Olof Johansson <olof@lixom.net>:
> On Thu, Jun 14, 2012 at 1:47 PM, Marko Kohtala <marko.kohtala@gmail.com> wrote:
>> Since v3.3-rc4-5-g1adbfa3, there has been an Oops in register_efivars
>> calling ops->get_next_variable and ending up at EIP 0 during module init.
>>
>> I boot 32-bit x86 kernel from 64-bit EFI bootloader.
>>
>> The efi_enabled is true, but runtime is not available. The functions
>> are NULL due to 32/64-bit mismatch between kernel and EFI.
>>
>> Signed-off-by: Marko Kohtala <marko.kohtala@gmail.com>
>> ---
>> I currently see this on v3.4.2.
>>
>> I could not figure out how I'm supposed to detect lack of runtime, so
>> I ended up with this quick and overly precise check that all required
>> functions are available. There may be other drivers that need to take
>> this new condition into account, so maybe Olof wants to make a better
>> fix.
>
> I must not have tested with efivars enabled, or I would have seen this
> when I did. Sigh.

I looked around a little, and it seems there is drivers/scsi/isci
driver using get_variable. arch/x86/kernel/reboot.c also calls
efi.reset_system if reboot=efi is given on command line. Some RTC
drivers call EFI runtime but are only for IA64 and this problem should
not be there.

>
>
>> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
>> index 47408e8..612a097 100644
>> --- a/drivers/firmware/efivars.c
>> +++ b/drivers/firmware/efivars.c
>> @@ -1223,12 +1223,16 @@ efivars_init(void)
>>                return -ENOMEM;
>>        }
>>
>> -       ops.get_variable = efi.get_variable;
>> -       ops.set_variable = efi.set_variable;
>> -       ops.get_next_variable = efi.get_next_variable;
>> -       error = register_efivars(&__efivars, &ops, efi_kobj);
>> -       if (error)
>> -               goto err_put;
>> +       /* We may have efi_enabled for systab, but no runtime for variables.
>> +        * Check the functions we need before proceeding. */
>> +       if (efi.get_variable && efi.set_variable && efi.get_next_variable) {
>> +               ops.get_variable = efi.get_variable;
>> +               ops.set_variable = efi.set_variable;
>> +               ops.get_next_variable = efi.get_next_variable;
>> +               error = register_efivars(&__efivars, &ops, efi_kobj);
>> +               if (error)
>> +                       goto err_put;
>> +       }
>
> I think it would make more sense to return -ENODEV when the function
> pointers aren't set, that way the exit function won't ever be called
> either, so no need to add the checks there.
>
> So, instead of current efi_enabled check:
>
> if (!efi_enabled ||
>    !efi.get_variable ||
>    !efi.set_variable ||
>    !efi.get_next_variable) {
>    return -ENODEV;
> }

This was the first thought I had. Looking more closely there is a
reason why efivars returns 0 if !efi_enabled. The module exports
functions to export other variables as well (used by
drivers/firmware/google). Also, it is loaded (bloated?) with a feature
to show /sys/firmware/efi/systab. Systab is there in case of
efi_enabled but no runtime.

So there are three modes to work with:
- !efi_enabled
- efi_enabled, but no runtime
- efi_enabled and runtime

I was wondering if you had some feelings for handling the difference
between the last two modes. I have no objection to checking the
pointers, but somehow it seems excessive checking many pointers when
really they are either all set or unset.

Maybe efi_enabled could have a third value for the new mode so other
modules like efivars can easily skip some useless initialization.

If there is no "final" solution quickly available, then maybe my patch
should be applied now to stable 3.4.y and replaced later with a better
solution for 3.5 or something.

>
> -Olof

Marko

[PATCH] efi: add efi_runtime state checking

[Olof Johansson]

This adds an efi_runtime variable indicating whether the
efi runtime services are available. The only time they are
expected to not be available is when a 32-bit kernel has been
booted using 64-but EFI and vice versa.

It also adds checking to the two locations where functions are
called; x86 reboot and efivars.

Signed-off-by: Olof Johansson <olof@lixom.net>
Cc: stable@kernel.org # 3.4
---

Marko,

This should solve the issue for you, I hope. I've verified that it works
here on my 64-bit EFI / 32-bit kernel systems (first-gen Chromebook).

Peter, let me know if you want this split up across several patches. I
preserved the previous probe/return behaviour for efivars, I suppose
it could be switched to -ENXIO return instead but this smaller change
seemed more suitable for -stable.


-Olof

 arch/x86/kernel/reboot.c    | 2 +-
 arch/x86/platform/efi/efi.c | 6 ++++++
 drivers/firmware/efivars.c  | 2 +-
 include/linux/efi.h         | 3 +++
 4 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 52190a9..fa5bb6c 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -592,7 +592,7 @@ static void native_machine_emergency_restart(void)
 			break;
 
 		case BOOT_EFI:
-			if (efi_enabled)
+			if (efi_runtime)
 				efi.reset_system(reboot_mode ?
 						 EFI_RESET_WARM :
 						 EFI_RESET_COLD,
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 2dc29f5..cc2bc12 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -53,6 +53,9 @@
 int efi_enabled;
 EXPORT_SYMBOL(efi_enabled);
 
+bool efi_runtime;
+EXPORT_SYMBOL(efi_runtime);
+
 struct efi __read_mostly efi = {
 	.mps        = EFI_INVALID_TABLE_ADDR,
 	.acpi       = EFI_INVALID_TABLE_ADDR,
@@ -77,6 +80,7 @@ static efi_system_table_t efi_systab __initdata;
 static int __init setup_noefi(char *arg)
 {
 	efi_enabled = 0;
+	efi_runtime = 0;
 	return 0;
 }
 early_param("noefi", setup_noefi);
@@ -615,6 +619,8 @@ static int __init efi_runtime_init(void)
 
 	early_iounmap(runtime, sizeof(efi_runtime_services_t));
 
+	efi_runtime = 1;
+
 	return 0;
 }
 
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 47408e8..265449a 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1213,7 +1213,7 @@ efivars_init(void)
 	printk(KERN_INFO "EFI Variables Facility v%s %s\n", EFIVARS_VERSION,
 	       EFIVARS_DATE);
 
-	if (!efi_enabled)
+	if (!efi_runtime)
 		return 0;
 
 	/* For now we'll register the efi directory at /sys/firmware/efi */
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 103adc6..ac9c28d 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -539,11 +539,14 @@ extern int __init efi_setup_pcdp_console(char *);
 # ifdef CONFIG_X86
    extern int efi_enabled;
    extern bool efi_64bit;
+   extern bool efi_runtime;
 # else
 #  define efi_enabled 1
+#  define efi_runtime 1
 # endif
 #else
 # define efi_enabled 0
+# define efi_runtime 0
 #endif
 
 /*
-- 
1.7.10.1.488.g05fbf7a

Re: [PATCH] efi: add efi_runtime state checking

[H. Peter Anvin]

On 06/27/2012 02:35 PM, Olof Johansson wrote:
> This adds an efi_runtime variable indicating whether the
> efi runtime services are available. The only time they are
> expected to not be available is when a 32-bit kernel has been
> booted using 64-but EFI and vice versa.
>
> It also adds checking to the two locations where functions are
> called; x86 reboot and efivars.
>

OK, stupid question:

Why is this different from the efi_enabled variable, or rather: why is 
it different from what the efi_enabled variable *should* be?  If runtime 
services aren't available the only "EFI" that is available to the kernel 
are the data structures passed in, and those can be checked directly...

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] efi: add efi_runtime state checking

[Olof Johansson]

On Wed, Jun 27, 2012 at 2:52 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 06/27/2012 02:35 PM, Olof Johansson wrote:
>>
>> This adds an efi_runtime variable indicating whether the
>> efi runtime services are available. The only time they are
>> expected to not be available is when a 32-bit kernel has been
>> booted using 64-but EFI and vice versa.
>>
>> It also adds checking to the two locations where functions are
>> called; x86 reboot and efivars.
>>
>
> OK, stupid question:
>
> Why is this different from the efi_enabled variable, or rather: why is it
> different from what the efi_enabled variable *should* be?  If runtime
> services aren't available the only "EFI" that is available to the kernel are
> the data structures passed in, and those can be checked directly...

Excellent question, and I think it would work to turn off efi_enabled
towards the end of setup_arch() for non-native boots. That'd solve all
these problems, I believe.

I'll try it out and revise this patch. It might take me a day or two
to get cycles for it.


-Olof

Re: [PATCH] efi: add efi_runtime state checking

[H. Peter Anvin]

On 06/28/2012 10:02 AM, Olof Johansson wrote:
> On Wed, Jun 27, 2012 at 2:52 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> On 06/27/2012 02:35 PM, Olof Johansson wrote:
>>>
>>> This adds an efi_runtime variable indicating whether the
>>> efi runtime services are available. The only time they are
>>> expected to not be available is when a 32-bit kernel has been
>>> booted using 64-but EFI and vice versa.
>>>
>>> It also adds checking to the two locations where functions are
>>> called; x86 reboot and efivars.
>>>
>>
>> OK, stupid question:
>>
>> Why is this different from the efi_enabled variable, or rather: why is it
>> different from what the efi_enabled variable *should* be?  If runtime
>> services aren't available the only "EFI" that is available to the kernel are
>> the data structures passed in, and those can be checked directly...
>
> Excellent question, and I think it would work to turn off efi_enabled
> towards the end of setup_arch() for non-native boots. That'd solve all
> these problems, I believe.
>
> I'll try it out and revise this patch. It might take me a day or two
> to get cycles for it.
>

Ping?

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] efi: add efi_runtime state checking

[Matt Fleming]

On Tue, 2012-08-14 at 17:51 -0700, H. Peter Anvin wrote:
> On 06/28/2012 10:02 AM, Olof Johansson wrote:
> > On Wed, Jun 27, 2012 at 2:52 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> >> On 06/27/2012 02:35 PM, Olof Johansson wrote:
> >>>
> >>> This adds an efi_runtime variable indicating whether the
> >>> efi runtime services are available. The only time they are
> >>> expected to not be available is when a 32-bit kernel has been
> >>> booted using 64-but EFI and vice versa.
> >>>
> >>> It also adds checking to the two locations where functions are
> >>> called; x86 reboot and efivars.
> >>>
> >>
> >> OK, stupid question:
> >>
> >> Why is this different from the efi_enabled variable, or rather: why is it
> >> different from what the efi_enabled variable *should* be?  If runtime
> >> services aren't available the only "EFI" that is available to the kernel are
> >> the data structures passed in, and those can be checked directly...
> >
> > Excellent question, and I think it would work to turn off efi_enabled
> > towards the end of setup_arch() for non-native boots. That'd solve all
> > these problems, I believe.
> >
> > I'll try it out and revise this patch. It might take me a day or two
> > to get cycles for it.
> >
> 
> Ping?

There's now a bugzilla report for tracking this,

	https://bugzilla.kernel.org/show_bug.cgi?id=45991

Re: [PATCH] efivars: prevent Oops if efi_enabled but no EFI runtime

[H. Peter Anvin]

On 06/17/2012 04:00 PM, Olof Johansson wrote:
> 
> I think it would make more sense to return -ENODEV when the function
> pointers aren't set, that way the exit function won't ever be called
> either, so no need to add the checks there.
> 

-ENIXO?  ENODEV is usually some form of "wrong type of device"; ENXIO is
"device is not there."

	-hpa