linux-kernel - Re: [PATCH] efivars: prevent Oops if efi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOesGMi2nAecB4Lgti-JSrNTuV4MRA9GEQV1T=BrDxJ-QCq+cQ@mail.gmail.com>
Date:	Sun, 17 Jun 2012 16:00:45 -0700
From:	Olof Johansson <olof@...om.net>
To:	Marko Kohtala <marko.kohtala@...il.com>
Cc:	Matthew Garrett <mjg@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] efivars: prevent Oops if efi_enabled but no EFI runtime

On Thu, Jun 14, 2012 at 1:47 PM, Marko Kohtala <marko.kohtala@...il.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@...il.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/