x86/efi: fix potential NULL pointer dereference
Subsystems (3)
PATCH 1/1 x86/efi: fix potential NULL pointer dereference
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.
--- arch/x86/platform/efi/efi_64.c | 2 ++ 1 file changed, 2 insertions(+)
arch/x86/platform/efi/efi_64.c +2 -0
@@ -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 );
Signature · 1 line
2.1.0
A message that is not in the archive (sent off-list or not yet received): 1429855639-14706-1-git-send-email-firogm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
arch/x86/platform/efi/efi_64.c pgd_t * __init efi_call_phys_prolog(void) · line 94
Quoted 10 lines · Firo Yang
arch/x86/platform/efi/efi_64.c +2
-0@@ -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
Quoted 16 lines · 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_pgdwill 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(+)arch/x86/platform/efi/efi_64.c +0
-0
arch/x86/platform/efi/efi_64.c pgd_t * __init efi_call_phys_prolog(void) · line 97
Quoted 8 lines
arch/x86/platform/efi/efi_64.c +2
-0@@ -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
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