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 <[email protected]> <> --- <> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
