Pager with address sanitizer #1856

jenswi-linaro · 2017-10-04T05:45:55Z

No description provided.

etienne-lms

Commit "core: arm: kern.ld.S: put constructors in init":

Acked-by: Etienne Carriere <etienne.carriere@linaro.org>

Commit " core: generic boot: tag paging access"
request changes. see comments.

Others: Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
despite few minor comments.
Also commit "core; add MEM_AREA_TEE_ASAN": s/;/:/ + some comment on asan/emul_rsam.
Comment

etienne-lms · 2017-10-04T11:14:19Z

core/arch/arm/mm/tee_pager.c


 if (!size)
 return NULL;

 mm = tee_mm_alloc(&tee_mm_vcore, ROUNDUP(size, SMALL_PAGE_SIZE));
 if (!mm)
 return NULL;
+ bytes = tee_mm_get_bytes(mm);
+
+ tee_pager_add_core_area(tee_mm_get_smem(mm), bytes, f, NULL, NULL);


minor :

smem = tee_mm_get_smem(mm); bytes = tee_mm_get_bytes(mm); tee_pager_add_core_area(smem, bytes, f, NULL, NULL); asan_tag_access(smem, smem + bytes); return smem;

etienne-lms · 2017-10-04T11:18:25Z

core/arch/arm/kernel/generic_boot.c

+static void init_vcore(tee_mm_pool_t *mm_vcore)
+{
+ vaddr_t begin = TEE_RAM_VA_START;
+ vaddr_t end = TEE_RAM_VA_START + CFG_TEE_RAM_VA_SIZE;


minor: double space

etienne-lms · 2017-10-04T11:19:32Z

core/arch/arm/kernel/generic_boot.c

+ vaddr_t end = TEE_RAM_VA_START + CFG_TEE_RAM_VA_SIZE;
+
+#ifdef CFG_CORE_SANITIZE_KADDRESS
+ if (end > ASAN_SHADOW_PA)


comment: /* carveout asan memory, flat maped after core memory */

etienne-lms · 2017-10-04T14:56:19Z

core/arch/arm/kernel/generic_boot.c

+ asan_tag_access((void *)VCORE_INIT_RO_PA,
+ (void *)(VCORE_INIT_RO_PA + VCORE_INIT_RO_SZ));
+ asan_tag_access(__rodata_pageable_start, __rodata_pageable_end);
+#else


tagging INIT_RX and text_pageable is unconditional

Hmm, it seems everything could be reduced to
asan_tag_access(__pageable_start, __pageable_end);

ok when no RONOEXE. Else there are holes in due to rx/ro split.

etienne-lms · 2017-10-04T14:58:08Z

core/arch/arm/kernel/generic_boot.c

+#else
+ asan_tag_access((void *)VCORE_INIT_RX_PA,
+ (void *)(VCORE_INIT_RX_PA + VCORE_INIT_RX_SZ));
+ asan_tag_access(__rodata_pageable_start, __text_pageable_end);


typo: s/rodata/text/

etienne-lms · 2017-10-04T14:59:30Z

core/arch/arm/kernel/generic_boot.c

+#ifdef CFG_CORE_SANITIZE_KADDRESS
+static void carve_out_asan_mem(tee_mm_pool_t *pool)
+{
+ size_t s = pool->hi - pool->lo;


minor const

etienne-lms · 2017-10-04T15:01:55Z

core/arch/arm/kernel/generic_boot.c


 /*
 * Need tee_mm_sec_ddr initialized to be able to allocate secure
 * DDR below.
 */
 teecore_init_ta_ram();

+#ifdef CFG_CORE_SANITIZE_KADDRESS
+ carve_out_asan_mem(&tee_mm_sec_ddr);
+#endif


minor: suggest to remove the #ifdef and use a empty definition.

jenswi-linaro · 2017-10-05T12:29:39Z

Comments addressed, some discovered problems addressed.

etienne-lms · 2017-10-10T14:32:14Z

core/arch/arm/kernel/generic_boot.c

+#ifdef CFG_WITH_PAGER
+ asan_tag_access(__pageable_start, __pageable_end);
+#endif /*CFG_WITH_PAGER*/
+ asan_tag_access(__nozi_start, __nozi_end);


note: this change discards the asan_tag_access() on exidx and extab sections. Seems unwinding related. These do not need a manual access tagging ?

Thanks, I'll fix

etienne-lms · 2017-10-10T14:36:12Z

core/arch/arm/kernel/generic_boot.c

+ * ASAN buffer is overlapping with the end of the
+ * pool.
+ */
+ asz = apa - pool->hi;


asz = pool->hi - apa;

etienne-lms · 2017-10-10T14:38:39Z

core/arch/arm/kernel/generic_boot.c

+ */
+ asz = apa - pool->hi;
+ }
+ }


do we need to assert asan map is not fully overlapping the target pool ?
assert(apa >= pool.lo || (apa + asz) <= pool->hi);
(unlikely to happen)

That's taking it a bit too far in my opinion.

etienne-lms · 2017-10-10T14:40:25Z

core/arch/arm/kernel/generic_boot.c

+static void init_vcore(tee_mm_pool_t *mm_vcore)
+{
+ vaddr_t begin = TEE_RAM_VA_START;
+ vaddr_t end = TEE_RAM_VA_START + CFG_TEE_RAM_VA_SIZE;


minor: trailing space. add const to begin definition.

jenswi-linaro · 2017-10-12T06:44:06Z

Comments addressed
Added a commit fixing __asan_handle_no_return()

etienne-lms · 2017-10-12T07:46:47Z

core/arch/arm/mm/core_mmu.c

 #endif /*CFG_WITH_PAGER*/
 #else /*!CFG_CORE_RWDATA_NOEXEC*/
 register_phys_mem(MEM_AREA_TEE_RAM, CFG_TEE_RAM_START, CFG_TEE_RAM_PH_SIZE);
 #endif /*!CFG_CORE_RWDATA_NOEXEC*/

+#if defined(CFG_CORE_SANITIZE_KADDRESS) && defined(CFG_WITH_PAGER)
+register_phys_mem(MEM_AREA_TEE_ASAN, ASAN_MAP_PA, ASAN_MAP_SZ);


A comment to state /* Asan ram is part of MEM_AREA_TEE_RAM_RW when pager is disable */

etienne-lms · 2017-10-12T07:52:43Z

core/kernel/asan.c

 {
- panic();


no kind of panicking ?

No, it's not entirely clear to me what we're supposed to do here. The Linux kernel does nothing though.

etienne-lms · 2017-10-12T07:56:04Z

core/arch/arm/mm/core_mmu.c

+#endif /*!CFG_CORE_RWDATA_NOEXEC*/
+
+#if defined(CFG_CORE_SANITIZE_KADDRESS) && defined(CFG_WITH_PAGER)
+register_phys_mem(MEM_AREA_TEE_ASAN, ASAN_MAP_PA, ASAN_MAP_SZ);


A comment to state /* Asan ram is part of MEM_AREA_TEE_RAM_RW when pager is disable */ ?

jenswi-linaro · 2017-10-12T08:35:21Z

Update

jenswi-linaro · 2017-11-01T10:29:01Z

Ping?

etienne-lms · 2017-11-02T10:46:48Z

Typo in generic_boot.c.
Aside this and rebase requirement, Acked-by: Etienne Carriere <etienne.carriere@linaro.org> for "core: arm: kern.ld.S: put constructors in init", Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> for the others.

etienne-lms

Sorry... this comment will still pending, not submited: here it is: typo in generic_boot.c.

etienne-lms · 2017-11-02T10:41:17Z

core/arch/arm/kernel/generic_boot.c

+ return;
+
+ /* Reserve the shadow area */
+ if (!core_is_buffer_inside(asz, asz, pool->lo, s)) {


typo on 1st arg: s/asz/apa.

Fixes set_alias_area() to only take the supplied area, prior to this the final page would have been included too. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Adds MEM_AREA_TEE_ASAN which is used when pager is enabled to map the memory used by the address sanitizer if enabled. Currently this only works in configurations with the pager where emulated SRAM is used. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Provides asan_memcpy_unchecked() which does a memcpy() that isn't checked against the tagging in the ASAN shadow area. If ASAN isn't enabled it's replaced by a direct call to memcpy(). Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Makes sure that constructor functions are in the init section to be available during initialization of OP-TEE. Acked-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Tag temporary or allocated memory ranges to allow new accesses. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Makes sure that __asan_register_globals is available during init. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Carves out address sanitizer range used for bookkeeping. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Tags paged stacks as accessible. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Moves the section covered by #ifdef CFG_CORE_SANITIZE_KADDRESS to above the #ifdef CFG_WITH_PAGER section to be able to later initialize address sanitizer with pager enabled. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

When pager is enabled tag needed ranges accordingly. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Enables address sanitizer when pager is enabled. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

jenswi-linaro · 2017-11-09T09:42:22Z

Rebased, tags applied

jforissier · 2017-11-09T10:24:02Z

There is one long line in the commit description of "qemu_virt: fix memory configuration". Can you fix it please?

Fixes memory configuration inconsistency introduced with the coherent memory area for QEMU virt with pager enabled. Fixes: 5402a9f ("qemu_virt: enable smp boot") Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Fixes version-o-cflags by adding $(cflagscore) to make sure that the address sanitizer flags are used for this object file too. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

asan_tag_access() should ignore null ranges to make tagging of areas easier. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

It seems __asan_handle_no_return() isn't called when a __noreturn function returns, instead it's called before the function is called. So empty the __asan_handle_no_return() function to let __noreturn function be called. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

jenswi-linaro · 2017-11-09T11:26:38Z

Updated

jforissier · 2017-11-09T11:56:17Z

Thanks

etienne-lms reviewed Oct 4, 2017

View reviewed changes

etienne-lms reviewed Oct 10, 2017

View reviewed changes

etienne-lms reviewed Oct 12, 2017

View reviewed changes

etienne-lms reviewed Nov 3, 2017

View reviewed changes

jenswi-linaro mentioned this pull request Nov 8, 2017

Replace struct prng_ops with function interface #1923

Merged

jenswi-linaro added 11 commits November 9, 2017 08:18

core: pager: bugfix set_alias_area()

59d22d5

Fixes set_alias_area() to only take the supplied area, prior to this the final page would have been included too. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

core: arm: kern.ld.S: put constructors in init

7fbd59d

Makes sure that constructor functions are in the init section to be available during initialization of OP-TEE. Acked-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

core: pager: asan adoptions

3687e42

Tag temporary or allocated memory ranges to allow new accesses. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

core: asan: pager adoptions

ecf50b9

Makes sure that __asan_register_globals is available during init. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

core: pager: carve out asan shadow range

367cf89

Carves out address sanitizer range used for bookkeeping. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

core: thread: asan tag paged stacks

b4c4a44

Tags paged stacks as accessible. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

core: generic boot: tag paging access

b61c3a9

When pager is enabled tag needed ranges accordingly. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

core: pager: enable address sanitizer

a925260

Enables address sanitizer when pager is enabled. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

jenswi-linaro force-pushed the pager_asan branch from ce7eed5 to 1b0d5a9 Compare November 9, 2017 09:41

jenswi-linaro added 4 commits November 9, 2017 12:25

core: fix version-o-cflags

b0992bf

Fixes version-o-cflags by adding $(cflagscore) to make sure that the address sanitizer flags are used for this object file too. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

core: asan_tag_access() ignore null ranges

7ea7c69

asan_tag_access() should ignore null ranges to make tagging of areas easier. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

jenswi-linaro force-pushed the pager_asan branch from 1b0d5a9 to b8fb11c Compare November 9, 2017 11:26

jforissier merged commit ce553c8 into OP-TEE:master Nov 9, 2017

jenswi-linaro deleted the pager_asan branch November 9, 2017 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pager with address sanitizer #1856

Pager with address sanitizer #1856

jenswi-linaro commented Oct 4, 2017

etienne-lms left a comment

etienne-lms Oct 4, 2017

etienne-lms Oct 4, 2017

etienne-lms Oct 4, 2017

etienne-lms Oct 4, 2017

jenswi-linaro Oct 5, 2017

etienne-lms Oct 5, 2017

etienne-lms Oct 4, 2017

etienne-lms Oct 4, 2017

etienne-lms Oct 4, 2017

jenswi-linaro commented Oct 5, 2017

etienne-lms Oct 10, 2017

jenswi-linaro Oct 12, 2017

etienne-lms Oct 10, 2017

jenswi-linaro Oct 12, 2017

etienne-lms Oct 10, 2017

jenswi-linaro Oct 12, 2017

etienne-lms Oct 10, 2017

jenswi-linaro commented Oct 12, 2017

etienne-lms Oct 12, 2017

etienne-lms Oct 12, 2017

jenswi-linaro Oct 12, 2017

etienne-lms Oct 12, 2017

jenswi-linaro commented Oct 12, 2017

jenswi-linaro commented Nov 1, 2017

etienne-lms commented Nov 2, 2017

etienne-lms left a comment

etienne-lms Nov 2, 2017

jenswi-linaro commented Nov 9, 2017

jforissier commented Nov 9, 2017

jenswi-linaro commented Nov 9, 2017

jforissier commented Nov 9, 2017

Pager with address sanitizer #1856

Pager with address sanitizer #1856

Conversation

jenswi-linaro commented Oct 4, 2017

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Oct 12, 2017

jenswi-linaro commented Nov 1, 2017

etienne-lms commented Nov 2, 2017

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Nov 9, 2017

jforissier commented Nov 9, 2017

jenswi-linaro commented Nov 9, 2017

jforissier commented Nov 9, 2017