[PATCH] efi: Check for null efi kernel parameters

[PATCH] efi: Check for null efi kernel parameters

[Ricardo Neri]

Even though it is documented how to specify efi parameters,
it is possible to cause a kernel panic due to a NULL pointer
derreference if no efi parameter is passed. Thus, check whether
there is any parameters at all before parsing and warn the user.

Signed-off-by: Ricardo Neri <ricardo.neri-calderon-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 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 51a689d..f60af0c 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -948,6 +948,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 3c457db..c1b3fdb 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;
 
-- 
1.9.1

Re: [PATCH] efi: Check for null efi kernel parameters

[Matt Fleming]

(Pulling in Dave because he wrote parse_option_str())

On Tue, 30 Jun, at 04:13:33PM, Ricardo Neri wrote:
> Even though it is documented how to specify efi parameters,
> it is possible to cause a kernel panic due to a NULL pointer
> derreference if no efi parameter is passed. Thus, check whether
> there is any parameters at all before parsing and warn the user.
> 
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  arch/x86/platform/efi/efi.c | 4 ++++
>  drivers/firmware/efi/efi.c  | 4 ++++
>  2 files changed, 8 insertions(+)

Did you hit this by passing "efi=" on the kernel command line?

I would have assumed that the generic early param parsing code would
have caught this problem since it's obviously a potential issue for
every other early_param() function, and so needs to be solved in a more
general way.
 
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index 51a689d..f60af0c 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -948,6 +948,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 3c457db..c1b3fdb 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;
>  
> -- 
> 1.9.1
> 

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] efi: Check for null efi kernel parameters

[Ricardo Neri]

On Wed, 2015-07-01 at 14:19 +0100, Matt Fleming wrote:
> Did you hit this by passing "efi=" on the kernel command line?

I hit this by passing "efi" alone. "efi=" works well because the pointer
points to a valid string that only contains \0.
> 
> I would have assumed that the generic early param parsing code would
> have caught this problem since it's obviously a potential issue for
> every other early_param() function, and so needs to be solved in a
> more
> general way.

Yes, I was dubious on where to apply the patch. I saw that
parse_option_str does not check for the validity of the pointers. That
might seem to be a more general fix. I'll take that route.

Thanks and BR,
Ricardo

Re: [PATCH] efi: Check for null efi kernel parameters

[Dave Young]

Matt/Ricardo, thanks for ccing me, and sorry for late response I thought
to reply but I was busy on other things..

On 07/01/15 at 10:37am, Ricardo Neri wrote:
> On Wed, 2015-07-01 at 14:19 +0100, Matt Fleming wrote:
> > Did you hit this by passing "efi=" on the kernel command line?
> 
> I hit this by passing "efi" alone. "efi=" works well because the pointer
> points to a valid string that only contains \0.
> > 
> > I would have assumed that the generic early param parsing code would
> > have caught this problem since it's obviously a potential issue for
> > every other early_param() function, and so needs to be solved in a
> > more
> > general way.
> 
> Yes, I was dubious on where to apply the patch. I saw that
> parse_option_str does not check for the validity of the pointers. That
> might seem to be a more general fix. I'll take that route.

Ricardo, I agree to fix it in parse_option_str. Do you have sent any
update ?

Thanks
Dave

Re: [PATCH] efi: Check for null efi kernel parameters

[Matt Fleming]

On Mon, 20 Jul, at 05:34:01PM, Dave Young wrote:
> 
> Ricardo, I agree to fix it in parse_option_str. Do you have sent any
> update ?

Ricardo actually went with a different solution to fixing
parse_option_str(),

  http://article.gmane.org/gmane.linux.kernel.efi/5865

-- 
Matt Fleming, Intel Open Source Technology Center