[PATCH] x86/efi: fix potential NULL pointer dereference

[PATCH] x86/efi: fix potential NULL pointer dereference

[Firo Yang]

In this patch, I add error-handing code for kmalloc() in
arch/x86/platform/efi/efi_64.c::efi_call_phys_prolog().

If kmalloc() failed to alloc memroy, save_pgd will be a NULL
pointer dereferenced by subsequent codes.

Signed-off-by: Firo Yang <firogm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/x86/platform/efi/efi_64.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index a0ac0f9..62326c4 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -90,6 +90,8 @@ pgd_t * __init efi_call_phys_prolog(void)
 
 	n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
 	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);
+	if (unlikely(!save_pgd))
+		return NULL;
 
 	for (pgd = 0; pgd < n_pgds; pgd++) {
 		save_pgd[pgd] = *pgd_offset_k(pgd * PGDIR_SIZE);
-- 
2.1.0

Re: [PATCH] x86/efi: fix potential NULL pointer dereference

[Dan Carpenter]

On Fri, Apr 24, 2015 at 02:07:19PM +0800, Firo Yang wrote:
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index a0ac0f9..62326c4 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -90,6 +90,8 @@ pgd_t * __init efi_call_phys_prolog(void)
>  
>  	n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
>  	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);
> +	if (unlikely(!save_pgd))
> +		return NULL;

A bunch of init code doesn't check for NULL because it won't happen in
real life.  It makes my life a little bit harder because it introduces
meaningless static checker warnings...  Oh well.

Don't add unlikely() here because it won't help with benchmarks
and it makes the code harder to read.

regards,
dan carpenter

Re: [PATCH] x86/efi: fix potential NULL pointer dereference

[James Bottomley]

On Fri, 2015-04-24 at 14:07 +0800, Firo Yang wrote:
> In this patch, I add error-handing code for kmalloc() in
> arch/x86/platform/efi/efi_64.c::efi_call_phys_prolog().
> 
> If kmalloc() failed to alloc memroy, save_pgd will be a NULL
> pointer dereferenced by subsequent codes.
> 
> Signed-off-by: Firo Yang <firogm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  arch/x86/platform/efi/efi_64.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index a0ac0f9..62326c4 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -90,6 +90,8 @@ pgd_t * __init efi_call_phys_prolog(void)
>  
>  	n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
>  	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);
> +	if (unlikely(!save_pgd))
> +		return NULL;
>  
>  	for (pgd = 0; pgd < n_pgds; pgd++) {
>  		save_pgd[pgd] = *pgd_offset_k(pgd * PGDIR_SIZE);

First explain what you're trying to achieve?  I'm asking because the
code you've introduced is actively harmful: the return from
efi_call_phys_prolog() isn't checked for errors, so the error you return
won't be handled and we'll likely trigger a much harder to detect error
if the allocation you're checking fails.  Effectively the prolog/epilog
do nothing and we never restore the pgds we hijacked, yet the system
continues on with the memory map in an unexpected state.

In my opinion the NULL deref we get further down in the prolog code is
actually a far better indicator of failure than what you're proposing.

James

Re: [PATCH] x86/efi: fix potential NULL pointer dereference

[Firo Yang]

On Fri, Apr 24, 2015 at 08:33:29AM -0700, James Bottomley wrote:
<On Fri, 2015-04-24 at 14:07 +0800, Firo Yang wrote:
<> In this patch, I add error-handing code for kmalloc() in
<> arch/x86/platform/efi/efi_64.c::efi_call_phys_prolog().
<> 
<> If kmalloc() failed to alloc memroy, save_pgd will be a NULL
<> pointer dereferenced by subsequent codes.
<> 
<> Signed-off-by: Firo Yang <firogm@gmail.com>
<> ---
<>  arch/x86/platform/efi/efi_64.c | 2 ++
<>  1 file changed, 2 insertions(+)
<> 
<> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
<> index a0ac0f9..62326c4 100644
<> --- a/arch/x86/platform/efi/efi_64.c
<> +++ b/arch/x86/platform/efi/efi_64.c
<> @@ -90,6 +90,8 @@ pgd_t * __init efi_call_phys_prolog(void)
<>  
<>  	n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
<>  	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);
<> +	if (unlikely(!save_pgd))
<> +		return NULL;
<>  
<>  	for (pgd = 0; pgd < n_pgds; pgd++) {
<>  		save_pgd[pgd] = *pgd_offset_k(pgd * PGDIR_SIZE);
<
<First explain what you're trying to achieve?  I'm asking because the
<code you've introduced is actively harmful: the return from
<efi_call_phys_prolog() isn't checked for errors, so the error you return
<won't be handled and we'll likely trigger a much harder to detect error
<if the allocation you're checking fails.  Effectively the prolog/epilog
<do nothing and we never restore the pgds we hijacked, yet the system
<continues on with the memory map in an unexpected state.
<
<In my opinion the NULL deref we get further down in the prolog code is
<actually a far better indicator of failure than what you're proposing.
<
<James
<
I feel sorry for introducing this problematic and harmful patch.
I will do more checks and tests before submitting patch next time.

The deep reason is that I did not fully understand these code and
add the error-handing code roughly.

Regards
Firo