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@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/