* Re: [PATCH] efi_64: Fix a missing-check bug in arch/x86/platform/efi/efi_64.c of Linux 5.1 [not found] <20190517082633.GA3890@zhanggen-UX430UQ> @ 2019-05-17 8:41 ` Ard Biesheuvel [not found] ` <20190517090628.GA4162@zhanggen-UX430UQ> 0 siblings, 1 reply; 8+ messages in thread From: Ard Biesheuvel @ 2019-05-17 8:41 UTC (permalink / raw) To: Gen Zhang, linux-efi; +Cc: Darren Hart, Linux Kernel Mailing List Hello Gen, Thanks for the patch. On Fri, 17 May 2019 at 10:26, Gen Zhang <blackgod016574@gmail.com> wrote: > > save_pgd is allocated by kmalloc_array. And it is dereferenced in the > following codes. However, memory allocation functions such as > kmalloc_array may fail. Dereferencing this save_pgd null pointer may > cause the kernel go wrong. Thus we should check this allocation and add > error handling code. > > Signed-off-by: Gen Zhang <blackgod016574@gmail.com> > > --- > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c > index cf0347f..fb9ae57 100644 > --- a/arch/x86/platform/efi/efi_64.c > +++ b/arch/x86/platform/efi/efi_64.c > @@ -91,6 +91,8 @@ pgd_t * __init efi_call_phys_prolog(void) > > n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE); > save_pgd = kmalloc_array(n_pgds, sizeof(*save_pgd), GFP_KERNEL); > + if (!save_pgd) > + goto err; > > /* > * Build 1:1 identity mapping for efi=old_map usage. Note that > @@ -142,6 +144,9 @@ pgd_t * __init efi_call_phys_prolog(void) > __flush_tlb_all(); > > return save_pgd; > +err: > + __flush_tlb_all(); What is the point of the goto and the TLB flush? > + return ERR_PTR(-ENOMEM); Returning an error here is not going to make much difference, given that the caller of efi_call_phys_prolog() does not bother to check it, and passes the result straight into efi_call_phys_epilog(), which happily attempts to dereference it. So if you want to fix this properly, please fix it at the call site as well. I'd prefer to avoid ERR_PTR() and just return NULL for a failed allocation though. > } > > void __init efi_call_phys_epilog(pgd_t *save_pgd) > --- ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <20190517090628.GA4162@zhanggen-UX430UQ>]
* Re: [PATCH] efi_64: Fix a missing-check bug in arch/x86/platform/efi/efi_64.c of Linux 5.1 [not found] ` <20190517090628.GA4162@zhanggen-UX430UQ> @ 2019-05-17 9:24 ` Ard Biesheuvel 2019-05-17 9:43 ` Gen Zhang 2019-05-23 0:51 ` [PATCH v2] efi_64: Fix a missing-check bug in arch/x86/platform/efi/efi_64.c Gen Zhang 0 siblings, 2 replies; 8+ messages in thread From: Ard Biesheuvel @ 2019-05-17 9:24 UTC (permalink / raw) To: Gen Zhang, linux-efi; +Cc: Linux Kernel Mailing List On Fri, 17 May 2019 at 11:06, Gen Zhang <blackgod016574@gmail.com> wrote: > > On Fri, May 17, 2019 at 10:41:28AM +0200, Ard Biesheuvel wrote: > > Returning an error here is not going to make much difference, given > > that the caller of efi_call_phys_prolog() does not bother to check it, > > and passes the result straight into efi_call_phys_epilog(), which > > happily attempts to dereference it. > > > > So if you want to fix this properly, please fix it at the call site as > > well. I'd prefer to avoid ERR_PTR() and just return NULL for a failed > > allocation though. > Hi Ard, > Thanks for your timely reply! > I think returning NULL in efi_call_phys_prolog() and checking in > efi_call_phys_epilog() is much better. But I am confused what to return > in efi_call_phys_epilog() if save_pgd is NULL. Definitely not return > -ENOMEM, because efi_call_phys_epilog() returns unsigned long. Could > please light on me to fix this problem? If efi_call_phys_prolog() returns NULL, the calling function should abort and never call efi_call_phys_epilog(). ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] efi_64: Fix a missing-check bug in arch/x86/platform/efi/efi_64.c of Linux 5.1 2019-05-17 9:24 ` Ard Biesheuvel @ 2019-05-17 9:43 ` Gen Zhang 2019-05-23 0:51 ` [PATCH v2] efi_64: Fix a missing-check bug in arch/x86/platform/efi/efi_64.c Gen Zhang 1 sibling, 0 replies; 8+ messages in thread From: Gen Zhang @ 2019-05-17 9:43 UTC (permalink / raw) To: Ard Biesheuvel, linux-efi; +Cc: linux-kernel On Fri, May 17, 2019 at 11:24:27AM +0200, Ard Biesheuvel wrote: > If efi_call_phys_prolog() returns NULL, the calling function should > abort and never call efi_call_phys_epilog(). Hi Ard, I edit the patch and it is as following. Returning EFI_ABORTED would be proper, because the return value (status) is checked in __efi_enter_virtual_mode(). And returning EFI_ABORTED can abort the process. Thanks Gen save_pgd is allocated by kmalloc_array. And it is dereferenced in the following codes. However, memory allocation functions such as kmalloc_array may fail. Dereferencing this save_pgd null pointer may cause the kernel go wrong. Thus we should check this allocation. Signed-off-by: Gen Zhang <blackgod016574@gmail.com> --- diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index e1cb01a..a7189a3 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -85,6 +85,8 @@ static efi_status_t __init phys_efi_set_virtual_address_map( pgd_t *save_pgd; save_pgd = efi_call_phys_prolog(); + if (!save_pgd) + return EFI_ABORTED; /* Disable interrupts around EFI calls: */ local_irq_save(flags); diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c index cf0347f..828460a 100644 --- a/arch/x86/platform/efi/efi_64.c +++ b/arch/x86/platform/efi/efi_64.c @@ -91,6 +91,8 @@ pgd_t * __init efi_call_phys_prolog(void) n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE); save_pgd = kmalloc_array(n_pgds, sizeof(*save_pgd), GFP_KERNEL); + if (!save_pgd) + return NULL; /* * Build 1:1 identity mapping for efi=old_map usage. Note that --- ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2] efi_64: Fix a missing-check bug in arch/x86/platform/efi/efi_64.c 2019-05-17 9:24 ` Ard Biesheuvel 2019-05-17 9:43 ` Gen Zhang @ 2019-05-23 0:51 ` Gen Zhang 2019-05-24 16:07 ` Ard Biesheuvel 1 sibling, 1 reply; 8+ messages in thread From: Gen Zhang @ 2019-05-23 0:51 UTC (permalink / raw) To: Ard Biesheuvel, dvhart; +Cc: linux-efi, linux-kernel In efi_call_phys_prolog(), save_pgd is allocated by kmalloc_array(). And it is dereferenced in the following codes. However, memory allocation functions such as kmalloc_array() may fail. Dereferencing this save_pgd null pointer may cause the kernel go wrong. Thus we should check this allocation. Further, if efi_call_phys_prolog() returns NULL, we should abort the process in phys_efi_set_virtual_address_map(), and return EFI_ABORTED. Signed-off-by: Gen Zhang <blackgod016574@gmail.com> --- diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index e1cb01a..a7189a3 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -85,6 +85,8 @@ static efi_status_t __init phys_efi_set_virtual_address_map( pgd_t *save_pgd; save_pgd = efi_call_phys_prolog(); + if (!save_pgd) + return EFI_ABORTED; /* Disable interrupts around EFI calls: */ local_irq_save(flags); diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c index cf0347f..828460a 100644 --- a/arch/x86/platform/efi/efi_64.c +++ b/arch/x86/platform/efi/efi_64.c @@ -91,6 +91,8 @@ pgd_t * __init efi_call_phys_prolog(void) n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE); save_pgd = kmalloc_array(n_pgds, sizeof(*save_pgd), GFP_KERNEL); + if (!save_pgd) + return NULL; /* * Build 1:1 identity mapping for efi=old_map usage. Note that --- ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] efi_64: Fix a missing-check bug in arch/x86/platform/efi/efi_64.c 2019-05-23 0:51 ` [PATCH v2] efi_64: Fix a missing-check bug in arch/x86/platform/efi/efi_64.c Gen Zhang @ 2019-05-24 16:07 ` Ard Biesheuvel 2019-05-25 2:36 ` Gen Zhang 0 siblings, 1 reply; 8+ messages in thread From: Ard Biesheuvel @ 2019-05-24 16:07 UTC (permalink / raw) To: Gen Zhang; +Cc: Darren Hart, linux-efi, Linux Kernel Mailing List On Thu, 23 May 2019 at 02:51, Gen Zhang <blackgod016574@gmail.com> wrote: > > In efi_call_phys_prolog(), save_pgd is allocated by kmalloc_array(). > And it is dereferenced in the following codes. However, memory > allocation functions such as kmalloc_array() may fail. Dereferencing > this save_pgd null pointer may cause the kernel go wrong. Thus we > should check this allocation. > Further, if efi_call_phys_prolog() returns NULL, we should abort the > process in phys_efi_set_virtual_address_map(), and return EFI_ABORTED. > > Signed-off-by: Gen Zhang <blackgod016574@gmail.com> > > --- > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c > index e1cb01a..a7189a3 100644 > --- a/arch/x86/platform/efi/efi.c > +++ b/arch/x86/platform/efi/efi.c > @@ -85,6 +85,8 @@ static efi_status_t __init phys_efi_set_virtual_address_map( > pgd_t *save_pgd; > > save_pgd = efi_call_phys_prolog(); > + if (!save_pgd) > + return EFI_ABORTED; > > /* Disable interrupts around EFI calls: */ > local_irq_save(flags); > diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c > index cf0347f..828460a 100644 > --- a/arch/x86/platform/efi/efi_64.c > +++ b/arch/x86/platform/efi/efi_64.c > @@ -91,6 +91,8 @@ pgd_t * __init efi_call_phys_prolog(void) > > n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE); > save_pgd = kmalloc_array(n_pgds, sizeof(*save_pgd), GFP_KERNEL); > + if (!save_pgd) > + return NULL; > > /* > * Build 1:1 identity mapping for efi=old_map usage. Note that > --- Apologies for only spotting this now, but I seem to have given some bad advice. efi_call_phys_prolog() in efi_64.c will also return NULL if (!efi_enabled(EFI_OLD_MEMMAP)), but this is not an error condition. So that occurrence has to be updated: please return efi_mm.pgd instead. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] efi_64: Fix a missing-check bug in arch/x86/platform/efi/efi_64.c 2019-05-24 16:07 ` Ard Biesheuvel @ 2019-05-25 2:36 ` Gen Zhang 2019-05-25 9:18 ` Ard Biesheuvel 0 siblings, 1 reply; 8+ messages in thread From: Gen Zhang @ 2019-05-25 2:36 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Darren Hart, linux-efi, Linux Kernel Mailing List On Fri, May 24, 2019 at 06:07:10PM +0200, Ard Biesheuvel wrote: > Apologies for only spotting this now, but I seem to have given some bad advice. > > efi_call_phys_prolog() in efi_64.c will also return NULL if > (!efi_enabled(EFI_OLD_MEMMAP)), but this is not an error condition. So > that occurrence has to be updated: please return efi_mm.pgd instead. Thanks for your reply, Ard. You mean that we should return efi_mm.pgd when allcoation fails? And we should delete return EFI_ABORTED on the caller site, right? In that case, how should we handle the NULL pointer returned by condition if(!efi_enabled(EFI_OLD_MEMMAP)) on the caller site? Thanks Gen ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] efi_64: Fix a missing-check bug in arch/x86/platform/efi/efi_64.c 2019-05-25 2:36 ` Gen Zhang @ 2019-05-25 9:18 ` Ard Biesheuvel 2019-05-25 9:40 ` Gen Zhang 0 siblings, 1 reply; 8+ messages in thread From: Ard Biesheuvel @ 2019-05-25 9:18 UTC (permalink / raw) To: Gen Zhang; +Cc: Darren Hart, linux-efi, Linux Kernel Mailing List On Sat, 25 May 2019 at 04:36, Gen Zhang <blackgod016574@gmail.com> wrote: > > On Fri, May 24, 2019 at 06:07:10PM +0200, Ard Biesheuvel wrote: > > Apologies for only spotting this now, but I seem to have given some bad advice. > > > > efi_call_phys_prolog() in efi_64.c will also return NULL if > > (!efi_enabled(EFI_OLD_MEMMAP)), but this is not an error condition. So > > that occurrence has to be updated: please return efi_mm.pgd instead. > Thanks for your reply, Ard. You mean that we should return efi_mm.pgd > when allcoation fails? And we should delete return EFI_ABORTED on the > caller site, right? In that case, how should we handle the NULL pointer > returned by condition if(!efi_enabled(EFI_OLD_MEMMAP)) on the caller > site? > No, the other way around. I have already updated the patch, so don't worry about it. https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/commit/?h=urgent&id=d2dc2bc7b60b936b95da4b04c2912c02974c3e9f ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] efi_64: Fix a missing-check bug in arch/x86/platform/efi/efi_64.c 2019-05-25 9:18 ` Ard Biesheuvel @ 2019-05-25 9:40 ` Gen Zhang 0 siblings, 0 replies; 8+ messages in thread From: Gen Zhang @ 2019-05-25 9:40 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Darren Hart, linux-efi, Linux Kernel Mailing List On Sat, May 25, 2019 at 11:18:36AM +0200, Ard Biesheuvel wrote: > On Sat, 25 May 2019 at 04:36, Gen Zhang <blackgod016574@gmail.com> wrote: > > > > On Fri, May 24, 2019 at 06:07:10PM +0200, Ard Biesheuvel wrote: > > > Apologies for only spotting this now, but I seem to have given some bad advice. > > > > > > efi_call_phys_prolog() in efi_64.c will also return NULL if > > > (!efi_enabled(EFI_OLD_MEMMAP)), but this is not an error condition. So > > > that occurrence has to be updated: please return efi_mm.pgd instead. > > Thanks for your reply, Ard. You mean that we should return efi_mm.pgd > > when allcoation fails? And we should delete return EFI_ABORTED on the > > caller site, right? In that case, how should we handle the NULL pointer > > returned by condition if(!efi_enabled(EFI_OLD_MEMMAP)) on the caller > > site? > > > > No, the other way around. I have already updated the patch, so don't > worry about it. > > https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/commit/?h=urgent&id=d2dc2bc7b60b936b95da4b04c2912c02974c3e9f Thanks for your reply and update, Ard! That's really nice of you. Thanks Gen ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-05-25 9:41 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20190517082633.GA3890@zhanggen-UX430UQ> 2019-05-17 8:41 ` [PATCH] efi_64: Fix a missing-check bug in arch/x86/platform/efi/efi_64.c of Linux 5.1 Ard Biesheuvel [not found] ` <20190517090628.GA4162@zhanggen-UX430UQ> 2019-05-17 9:24 ` Ard Biesheuvel 2019-05-17 9:43 ` Gen Zhang 2019-05-23 0:51 ` [PATCH v2] efi_64: Fix a missing-check bug in arch/x86/platform/efi/efi_64.c Gen Zhang 2019-05-24 16:07 ` Ard Biesheuvel 2019-05-25 2:36 ` Gen Zhang 2019-05-25 9:18 ` Ard Biesheuvel 2019-05-25 9:40 ` Gen Zhang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).