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

[Firo Yang <firogm@gmail.com>]

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