On Tue, 2015-07-21 at 17:22 +0800, Dave Young wrote:
> On 07/15/15 at 07:36pm, Ricardo Neri wrote:
> > Even though it is documented how to specifiy efi parameters, it is
> > possible to cause a kernel panic due to a derreference of a NULL pointer when
> > parsing such parameters if "efi" alone is given:
> >
> > PANIC: early exception 0e rip 10:ffffffff812fb361 error 0 cr2 0
> > [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.2.0-rc1+ #450
> > [ 0.000000] ffffffff81fe20a9 ffffffff81e03d50 ffffffff8184bb0f 00000000000003f8
> > [ 0.000000] 0000000000000000 ffffffff81e03e08 ffffffff81f371a1 64656c62616e6520
> > [ 0.000000] 0000000000000069 000000000000005f 0000000000000000 0000000000000000
> > [ 0.000000] Call Trace:
> > [ 0.000000] [<ffffffff8184bb0f>] dump_stack+0x45/0x57
> > [ 0.000000] [<ffffffff81f371a1>] early_idt_handler_common+0x81/0xae
> > [ 0.000000] [<ffffffff812fb361>] ? parse_option_str+0x11/0x90
> > [ 0.000000] [<ffffffff81f4dd69>] arch_parse_efi_cmdline+0x15/0x42
> > [ 0.000000] [<ffffffff81f376e1>] do_early_param+0x50/0x8a
> > [ 0.000000] [<ffffffff8106b1b3>] parse_args+0x1e3/0x400
> > [ 0.000000] [<ffffffff81f37a43>] parse_early_options+0x24/0x28
> > [ 0.000000] [<ffffffff81f37691>] ? loglevel+0x31/0x31
> > [ 0.000000] [<ffffffff81f37a78>] parse_early_param+0x31/0x3d
> > [ 0.000000] [<ffffffff81f3ae98>] setup_arch+0x2de/0xc08
> > [ 0.000000] [<ffffffff8109629a>] ? vprintk_default+0x1a/0x20
> > [ 0.000000] [<ffffffff81f37b20>] start_kernel+0x90/0x423
> > [ 0.000000] [<ffffffff81f37495>] x86_64_start_reservations+0x2a/0x2c
> > [ 0.000000] [<ffffffff81f37582>] x86_64_start_kernel+0xeb/0xef
> > [ 0.000000] RIP 0xffffffff81ba2efc
> >
> > This panic is not reproducible with "efi=" as this will result in a non-NULL
> > zero-lenght string.
> >
> > Thus, verify that the pointer to the parameter string is not NULL. This is
> > consistent with other parameter-parsing functions which check for NULL pointers.
> >
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@xxxxxxxxxxxxxxx>
> > ---
> >
> > Changes since v1:
> >
> > - Include a stackdump showing the code path to the panic
> > - Describe further in which scenarios the panic can be reproduced
> > - Describe more accurately the fix.
>
> Ricardo, any reason not change in parse_option_str general function?
> I prefer to fix it in parse_option_str, because it is for checking
> if str contains an option string, if str is NULL then it should
> return false.
Yes, I thought about it as well. I thought that this implementation
aligns with what the majority of the param functions does. That was the
main reason. But having this in parse_option_str might seem a better
alternative if more param functions use it.
>
> But since it is only used in efi code for now so I have no strong opinion.
Matt, do you have any specific preference?
>
> >
> > arch/x86/platform/efi/efi.c | 4 ++++
> > drivers/firmware/efi/efi.c | 4 ++++
> > 2 files changed, 8 insertions(+)
> >
> > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> > index cfba30f..d323e3a 100644
> > --- a/arch/x86/platform/efi/efi.c
> > +++ b/arch/x86/platform/efi/efi.c
> > @@ -972,6 +972,10 @@ u64 efi_mem_attributes(unsigned long phys_addr)
> >
> > static int __init arch_parse_efi_cmdline(char *str)
> > {
> > + if (!str) {
> > + pr_warn("need at least one option\n");
> > + return -EINVAL;
> > + }
> > if (parse_option_str(str, "old_map"))
> > set_bit(EFI_OLD_MEMMAP, &efi.flags);
> > if (parse_option_str(str, "debug"))
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 46eb8a6..9816e3d 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -58,6 +58,10 @@ bool efi_runtime_disabled(void)
> >
> > static int __init parse_efi_cmdline(char *str)
> > {
> > + if (!str) {
> > + pr_warn("need at least one option\n");
> > + return -EINVAL;
> > + }
> > if (parse_option_str(str, "noruntime"))
> > disable_runtime = true;
> >
> Thanks
> Dave
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
