| Message ID | 20190821183204.23576-6-pasha.tatashin@soleen.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: MMU enabled kexec relocation | expand |
Hi Pavel, On 21/08/2019 19:31, Pavel Tatashin wrote: > There is a bug in create_safe_exec_page(), when page table is allocated > it is not checked that table is allocated successfully: > > But it is dereferenced in: pgd_none(READ_ONCE(*pgdp)). If there is a bug, it shouldn't be fixed part way through a series. This makes it difficult to backport the fix. Please split this out as an independent patch with a 'Fixes:' tag for the commit that introduced the bug. > Another issue, So this patch does two things? That is rarely a good idea. Again, this makes it difficult to backport the fix. > is that phys_to_ttbr() uses an offset in page table instead > of pgd directly. If you were going to reuse this, that would be a bug. But because the only page that is being mapped, is mapped to PAGE_SIZE, all the top bits will be 0. The offset calls are boiler-plate. It doesn't look intentional, but its harmless. Please separate out the potential NULL-dereference bits so there is a clean stand-alone fix that can be sent to the stable trees. Thanks, James
On Fri, Sep 6, 2019 at 11:17 AM James Morse <james.morse@arm.com> wrote: > > Hi Pavel, > > On 21/08/2019 19:31, Pavel Tatashin wrote: > > There is a bug in create_safe_exec_page(), when page table is allocated > > it is not checked that table is allocated successfully: > > > > But it is dereferenced in: pgd_none(READ_ONCE(*pgdp)). > > If there is a bug, it shouldn't be fixed part way through a series. This makes it > difficult to backport the fix. > > Please split this out as an independent patch with a 'Fixes:' tag for the commit that > introduced the bug. > > > > Another issue, > > So this patch does two things? That is rarely a good idea. Again, this makes it difficult > to backport the fix. > > > > is that phys_to_ttbr() uses an offset in page table instead > > of pgd directly. > > If you were going to reuse this, that would be a bug. But because the only page that is > being mapped, is mapped to PAGE_SIZE, all the top bits will be 0. The offset calls are > boiler-plate. It doesn't look intentional, but its harmless. Yes, it is harmless otherwise the code would not work. But it is confusing to read, and looks broken. I will separate this change as two patches as you suggested. Thank you, Pasha
diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c index ee34a06d8a35..750ecc7f2cbe 100644 --- a/arch/arm64/kernel/hibernate.c +++ b/arch/arm64/kernel/hibernate.c @@ -199,6 +199,7 @@ static int create_safe_exec_page(void *src_start, size_t length, phys_addr_t *phys_dst_addr) { void *page = (void *)get_safe_page(GFP_ATOMIC); + pgd_t *trans_pgd; pgd_t *pgdp; pud_t *pudp; pmd_t *pmdp; @@ -210,7 +211,11 @@ static int create_safe_exec_page(void *src_start, size_t length, memcpy(page, src_start, length); __flush_icache_range((unsigned long)page, (unsigned long)page + length); - pgdp = pgd_offset_raw((void *)get_safe_page(GFP_ATOMIC), dst_addr); + trans_pgd = (void *)get_safe_page(GFP_ATOMIC); + if (!trans_pgd) + return -ENOMEM; + + pgdp = pgd_offset_raw(trans_pgd, dst_addr); if (pgd_none(READ_ONCE(*pgdp))) { pudp = (void *)get_safe_page(GFP_ATOMIC); if (!pudp) @@ -251,7 +256,7 @@ static int create_safe_exec_page(void *src_start, size_t length, */ cpu_set_reserved_ttbr0(); local_flush_tlb_all(); - write_sysreg(phys_to_ttbr(virt_to_phys(pgdp)), ttbr0_el1); + write_sysreg(phys_to_ttbr(virt_to_phys(trans_pgd)), ttbr0_el1); isb(); *phys_dst_addr = virt_to_phys(page);
There is a bug in create_safe_exec_page(), when page table is allocated it is not checked that table is allocated successfully: But it is dereferenced in: pgd_none(READ_ONCE(*pgdp)). Another issue, is that phys_to_ttbr() uses an offset in page table instead of pgd directly. So, allocate page table, check that allocation was successful, and use it directly to set ttbr0_el1. Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com> --- arch/arm64/kernel/hibernate.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)