| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-id: <551E993E.5060801@samsung.com>
Date: Fri, 03 Apr 2015 16:44:30 +0300
From: Andrey Ryabinin <a.ryabinin@...sung.com>
To: Catalin Marinas <catalin.marinas@....com>
Cc: linux-arm-kernel@...ts.infradead.org,
Will Deacon <will.deacon@....com>,
linux-kernel@...r.kernel.org, linux-mm@...ck.org,
Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 2/2] arm64: add KASan support
On 04/01/2015 03:28 PM, Catalin Marinas wrote:
> Hi Andrey,
>
Hi Catalin,
> On Tue, Mar 24, 2015 at 05:49:04PM +0300, Andrey Ryabinin wrote:
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 4085df1..10bbd71 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -41,6 +41,7 @@ config ARM64
>> select HAVE_ARCH_AUDITSYSCALL
>> select HAVE_ARCH_BITREVERSE
>> select HAVE_ARCH_JUMP_LABEL
>> + select HAVE_ARCH_KASAN if SPARSEMEM_VMEMMAP
>> select HAVE_ARCH_KGDB
>> select HAVE_ARCH_SECCOMP_FILTER
>> select HAVE_ARCH_TRACEHOOK
>> @@ -116,6 +117,12 @@ config GENERIC_CSUM
>> config GENERIC_CALIBRATE_DELAY
>> def_bool y
>>
>> +config KASAN_SHADOW_OFFSET
>> + hex
>> + default 0xdfff200000000000 if ARM64_VA_BITS_48
>> + default 0xdffffc8000000000 if ARM64_VA_BITS_42
>> + default 0xdfffff9000000000 if ARM64_VA_BITS_39
>
> Can we compute these at build time in some C header? Or they need to be
> passed to gcc when compiling the kernel so that it generates the right
> instrumentation?
>
Correct, this value passed to GCC.
> I'm not familiar with KASan but is the offset address supposed to be
> accessible? The addresses encoded above would always generate a fault
> (level 0 / address size fault).
>
It's fine. KASAN_SHADOW_OFFSET address is shadow address that corresponds to 0 address.
So KASAN_SHADOW_OFFSET could be dereferenced only if we have NULL-ptr derefernce in kernel.
Shadow for kernel addresses starts from KASAN_SHADOW_START constant,
which is defined in arch/arm64/include/asm/kasan.h. But since I forgot to 'git add' that file
it's not present in this patch.
arch/arm64/include/asm/kasan.h:
/*
* Compiler uses shadow offset assuming that addresses start
* from 0. Kernel addresses don't start from 0, so shadow
* for kernel really starts from 'compiler's shadow offset' +
* ('kernel address space start' >> KASAN_SHADOW_SCALE_SHIFT)
*/
#define KASAN_SHADOW_START (KASAN_SHADOW_OFFSET + \
((UL(0xffffffffffffffff) << (VA_BITS)) >> 3))
#define KASAN_SHADOW_END (KASAN_SHADOW_START + (1ULL << (VA_BITS - 3)))
>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>> index bd5db28..f5ce010 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -40,7 +40,7 @@
>> * fixed mappings and modules
>> */
>> #define VMEMMAP_SIZE ALIGN((1UL << (VA_BITS - PAGE_SHIFT)) * sizeof(struct page), PUD_SIZE)
>> -#define VMALLOC_START (UL(0xffffffffffffffff) << VA_BITS)
>> +#define VMALLOC_START ((UL(0xffffffffffffffff) << VA_BITS) + (UL(1) << (VA_BITS - 3)))
>
> I assume that's where you want to make room for KASan? Some comments and
> macros would be useful for why this is needed and how it is calculated.
> It also needs to be disabled when KASan is not enabled.
>
Ok.
>> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>> index 51c9811..1a99e95 100644
>> --- a/arch/arm64/kernel/head.S
>> +++ b/arch/arm64/kernel/head.S
>> @@ -482,6 +482,9 @@ __mmap_switched:
>> str_l x21, __fdt_pointer, x5 // Save FDT pointer
>> str_l x24, memstart_addr, x6 // Save PHYS_OFFSET
>> mov x29, #0
>> +#ifdef CONFIG_KASAN
>> + b kasan_early_init
>> +#endif
>
> Nitpick: tab between b and kasan_early_init.
>
>> diff --git a/arch/arm64/mm/kasan_init.c b/arch/arm64/mm/kasan_init.c
>> new file mode 100644
>> index 0000000..df537da
>> --- /dev/null
>> +++ b/arch/arm64/mm/kasan_init.c
>> @@ -0,0 +1,211 @@
>> +#include <linux/kasan.h>
>> +#include <linux/kernel.h>
>> +#include <linux/memblock.h>
>> +#include <linux/start_kernel.h>
>> +
>> +#include <asm/page.h>
>> +#include <asm/pgtable.h>
>> +#include <asm/tlbflush.h>
>> +
>> +static char kasan_zero_page[PAGE_SIZE] __page_aligned_bss;
>
> Can we not use the system's zero_page or it's not initialised yet?
>
System's zero page allocated in paging_init() and that is too late.
But I could put system's zero page into bss and use it here if you ok with this.
>> +static pgd_t tmp_page_table[PTRS_PER_PGD] __initdata __aligned(PAGE_SIZE);
>> +
>> +#if CONFIG_PGTABLE_LEVELS > 3
>> +static pud_t kasan_zero_pud[PTRS_PER_PUD] __page_aligned_bss;
>> +#endif
>> +#if CONFIG_PGTABLE_LEVELS > 2
>> +static pmd_t kasan_zero_pmd[PTRS_PER_PMD] __page_aligned_bss;
>> +#endif
>> +static pte_t kasan_zero_pte[PTRS_PER_PTE] __page_aligned_bss;
>> +
>> +static void __init init_kasan_page_tables(void)
>> +{
>> + int i;
>> +
>> +#if CONFIG_PGTABLE_LEVELS > 3
>> + for (i = 0; i < PTRS_PER_PUD; i++)
>> + set_pud(&kasan_zero_pud[i], __pud(__pa(kasan_zero_pmd)
>> + | PAGE_KERNEL));
>> +#endif
>> +#if CONFIG_PGTABLE_LEVELS > 2
>> + for (i = 0; i < PTRS_PER_PMD; i++)
>> + set_pmd(&kasan_zero_pmd[i], __pmd(__pa(kasan_zero_pte)
>> + | PAGE_KERNEL));
>> +#endif
>
> These don't look right. You are setting page attributes on table
> entries. You should use the standard pmd_populate etc. macros here, see
> early_fixmap_init() as an example.
>
Right. Will fix.
>> + for (i = 0; i < PTRS_PER_PTE; i++)
>> + set_pte(&kasan_zero_pte[i], __pte(__pa(kasan_zero_page)
>> + | PAGE_KERNEL));
>
> PAGE_KERNEL is pgprot_t, so you mix the types here. Just do something
> like:
>
> set_pte(..., pfn_pte(zero_pfn, PAGE_KERNEL_RO));
>
> (shouldn't it be read-only?)
>
It should be read-only, but only after kasan_init().
It should be writable earlier because stack instrumentation writes to shadow memory.
In function's prologue compiler writes to shadow to poison redzones around stack variables.
>> +void __init kasan_map_early_shadow(pgd_t *pgdp)
>> +{
>> + int i;
>> + unsigned long start = KASAN_SHADOW_START;
>> + unsigned long end = KASAN_SHADOW_END;
>> + pgd_t pgd;
>> +
>> +#if CONFIG_PGTABLE_LEVELS > 3
>> + pgd = __pgd(__pa(kasan_zero_pud) | PAGE_KERNEL);
>> +#elif CONFIG_PGTABLE_LEVELS > 2
>> + pgd = __pgd(__pa(kasan_zero_pmd) | PAGE_KERNEL);
>> +#else
>> + pgd = __pgd(__pa(kasan_zero_pte) | PAGE_KERNEL);
>> +#endif
>> +
>> + for (i = pgd_index(start); start < end; i++) {
>> + set_pgd(&pgdp[i], pgd);
>> + start += PGDIR_SIZE;
>> + }
>> +}
>
> Same problem as above with PAGE_KERNEL. You should just use
> pgd_populate().
>
Ok
>> +
>> +void __init kasan_early_init(void)
>> +{
>> + init_kasan_page_tables();
>> + kasan_map_early_shadow(swapper_pg_dir);
>> + kasan_map_early_shadow(idmap_pg_dir);
>> + flush_tlb_all();
>> + start_kernel();
>> +}
>
> Why do you need to map the kasan page tables into the idmap?
>
I don't need it. This is some left-over that should be removed.
>> +
>> +static void __init clear_pgds(unsigned long start,
>> + unsigned long end)
>> +{
>> + for (; start && start < end; start += PGDIR_SIZE)
>> + set_pgd(pgd_offset_k(start), __pgd(0));
>> +}
>
> We have dedicated pgd_clear() macro.
>
I need to remove references to kasan_zero_p* tables from swapper_pg_dir so
pgd_clear() will not work here because it's noop on CONFIG_PGTABLE_LEVEL <= 3.
[...]
>> +static int __init zero_pgd_populate(unsigned long addr, unsigned long end)
>> +{
>> + int ret = 0;
>> + pgd_t *pgd = pgd_offset_k(addr);
>> +
>> +#if CONFIG_PGTABLE_LEVELS > 3
>> + while (IS_ALIGNED(addr, PGDIR_SIZE) && addr + PGDIR_SIZE <= end) {
>> + set_pgd(pgd, __pgd(__pa(kasan_zero_pud)
>> + | PAGE_KERNEL_RO));
>> + addr += PGDIR_SIZE;
>> + pgd++;
>> + }
>> +#endif
>
> All these PAGE_KERNEL_RO on table entries are wrong. Please use the
> standard pgd/pud/pmd_populate macros.
>
> As for the while loops above, we have a standard way to avoid the
> #ifdef's by using pgd_addr_end() etc. See __create_mapping() as an
> example, there are a few others throughout the kernel.
>
Ok.
[...]
>> +static void cpu_set_ttbr1(unsigned long ttbr1)
>> +{
>> + asm(
>> + " msr ttbr1_el1, %0\n"
>> + " isb"
>> + :
>> + : "r" (ttbr1));
>> +}
>> +
>> +void __init kasan_init(void)
>> +{
>> + struct memblock_region *reg;
>> +
>> + memcpy(tmp_page_table, swapper_pg_dir, sizeof(tmp_page_table));
>> + cpu_set_ttbr1(__pa(tmp_page_table));
>
> Why is this needed? The code lacks comments in several places but here I
> couldn't figure out what the point is.
>
To setup shadow memory properly we need to unmap early shadow first (clear_pgds() in next line).
But instrumented kernel cannot run with unmaped shadow so this temporary
page table with early shadow until setting up shadow in swapper_pg_dir
will be finished.
I'll add comment about this here.
>> +
>> + clear_pgds(KASAN_SHADOW_START, KASAN_SHADOW_END);
>> +
>> + populate_zero_shadow(KASAN_SHADOW_START,
>> + (unsigned long)kasan_mem_to_shadow((void *)MODULES_VADDR));
>> +
>> + for_each_memblock(memory, reg) {
>> + void *start = (void *)__phys_to_virt(reg->base);
>> + void *end = (void *)__phys_to_virt(reg->base + reg->size);
>> +
>> + if (start >= end)
>> + break;
>> +
>> + vmemmap_populate((unsigned long)kasan_mem_to_shadow(start),
>> + (unsigned long)kasan_mem_to_shadow(end),
>> + pfn_to_nid(virt_to_pfn(start)));
>> + }
>> +
>> + memset(kasan_zero_page, 0, PAGE_SIZE);
>> + cpu_set_ttbr1(__pa(swapper_pg_dir));
>> + init_task.kasan_depth = 0;
>> +}
>
Thank you for detailed review.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists