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@xxxxxxxxx>
<> ---
<> 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 kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
