| Message ID | 1404295802-28030-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
> On 2 July 2014 12:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but > then go on an dereference it anyway. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm64/kernel/efi.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c > index 56c3327bbf79..e785f93f17cb 100644 > --- a/arch/arm64/kernel/efi.c > +++ b/arch/arm64/kernel/efi.c > @@ -419,8 +419,11 @@ static int __init arm64_enter_virtual_mode(void) > } > > efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table); > - if (efi.systab) > - set_bit(EFI_SYSTEM_TABLES, &efi.flags); > + if (!efi.systab) { > + pr_err("Failed to remap EFI System Table!\n"); ... this needs a kfree(virtmap) as well. > + return -1; > + } > + set_bit(EFI_SYSTEM_TABLES, &efi.flags); > > local_irq_save(flags); > cpu_switch_mm(idmap_pg_dir, &init_mm); > -- > 1.8.3.2 >
On Wed, 2014-07-02 at 12:13 +0200, Ard Biesheuvel wrote: > > On 2 July 2014 12:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but > > then go on an dereference it anyway. > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > --- > > arch/arm64/kernel/efi.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c > > index 56c3327bbf79..e785f93f17cb 100644 > > --- a/arch/arm64/kernel/efi.c > > +++ b/arch/arm64/kernel/efi.c > > @@ -419,8 +419,11 @@ static int __init arm64_enter_virtual_mode(void) > > } > > > > efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table); > > - if (efi.systab) > > - set_bit(EFI_SYSTEM_TABLES, &efi.flags); > > + if (!efi.systab) { > > + pr_err("Failed to remap EFI System Table!\n"); > > ... this needs a kfree(virtmap) as well. > And probably should unmap all the remapped regions in virtmap first. Presumably the systab will be in a runtime memory region which gets virtual mapping from remap_region(). If that succeeds, then the efi_lookup_mapped_addr should always succeed. But to be careful, we should probably bail out earlier if remap_region() ever returns zero. If all remaps work and efi_lookup_mapped_addr() fails, then we should try ioremap_cache() directly. Or do what x86 does and make a local copy of the table earlier when we early_memremap() it in uefi_init().
On 2 July 2014 16:29, Mark Salter <msalter@redhat.com> wrote: > On Wed, 2014-07-02 at 12:13 +0200, Ard Biesheuvel wrote: >> > On 2 July 2014 12:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> > We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but >> > then go on an dereference it anyway. >> > >> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> > --- >> > arch/arm64/kernel/efi.c | 7 +++++-- >> > 1 file changed, 5 insertions(+), 2 deletions(-) >> > >> > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c >> > index 56c3327bbf79..e785f93f17cb 100644 >> > --- a/arch/arm64/kernel/efi.c >> > +++ b/arch/arm64/kernel/efi.c >> > @@ -419,8 +419,11 @@ static int __init arm64_enter_virtual_mode(void) >> > } >> > >> > efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table); >> > - if (efi.systab) >> > - set_bit(EFI_SYSTEM_TABLES, &efi.flags); >> > + if (!efi.systab) { >> > + pr_err("Failed to remap EFI System Table!\n"); >> >> ... this needs a kfree(virtmap) as well. >> > > And probably should unmap all the remapped regions in virtmap first. Yes. > Presumably the systab will be in a runtime memory region which gets > virtual mapping from remap_region(). If that succeeds, then the > efi_lookup_mapped_addr should always succeed. But to be careful, > we should probably bail out earlier if remap_region() ever returns > zero. If all remaps work and efi_lookup_mapped_addr() fails, then > we should try ioremap_cache() directly. Or do what x86 does and make > a local copy of the table earlier when we early_memremap() it in > uefi_init(). > Bailing early (and cleaning up) if any remap_region() call fails seems like a wise thing to do. But if they all succeed, and efi_lookup_mapped_addr() then fails to resolve efi_system_table, it means it lives in a region which will become inaccessible to the runtime services themselves after set_virtual_address_map() has been called. So doing any kind of fallback masks a severe firmware bug, which makes me think we should just complain loudly and not enable UEFI bits any further
On Wed, Jul 02, 2014 at 11:10:01AM +0100, Ard Biesheuvel wrote: > We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but > then go on an dereference it anyway. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Thanks. I applied this one (the other patch needs to go in via a different route).
On 4 July 2014 15:38, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Wed, Jul 02, 2014 at 11:10:01AM +0100, Ard Biesheuvel wrote: >> We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but >> then go on an dereference it anyway. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Thanks. I applied this one (the other patch needs to go in via a > different route). > Eh, actually, I proposed a v2 based on Mark's feedback, but I now see that I accidentally removed you from the To list. http://marc.info/?l=linux-arm-kernel&m=140446911506688&w=2
On Fri, Jul 04, 2014 at 02:54:19PM +0100, Ard Biesheuvel wrote: > On 4 July 2014 15:38, Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Wed, Jul 02, 2014 at 11:10:01AM +0100, Ard Biesheuvel wrote: > >> We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but > >> then go on an dereference it anyway. > >> > >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > > Thanks. I applied this one (the other patch needs to go in via a > > different route). > > > > Eh, actually, I proposed a v2 based on Mark's feedback, but I now see > that I accidentally removed you from the To list. > > http://marc.info/?l=linux-arm-kernel&m=140446911506688&w=2 OK. I'll wait for acks on the other version then.
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index 56c3327bbf79..e785f93f17cb 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -419,8 +419,11 @@ static int __init arm64_enter_virtual_mode(void) } efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table); - if (efi.systab) - set_bit(EFI_SYSTEM_TABLES, &efi.flags); + if (!efi.systab) { + pr_err("Failed to remap EFI System Table!\n"); + return -1; + } + set_bit(EFI_SYSTEM_TABLES, &efi.flags); local_irq_save(flags); cpu_switch_mm(idmap_pg_dir, &init_mm);
We assign efi.systab using efi_lookup_mapped_addr(), and test for !NULL, but then go on an dereference it anyway. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/arm64/kernel/efi.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)