arm64: add KASAN support

[kevans]

Summary

This entails:

• Marking some obvious candidates for __nosanitizeaddress
• Similar trap frame markings as amd64, for similar reasons
• Shadow map implementation

The shadow map implementation is roughly similar to what was done on
amd64, with some exceptions. Attempting to use available space at
preinit_map_va + PMAP_PREINIT_MAPPING_SIZE (up to the end of that range,
as depicted in the physmap) results in odd failures, so we instead
search the physmap for free regions that we can carve out, fragmenting
the shadow map as necessary to try and fit as much as we need for the
initial kernel map. pmap_bootstrap_san() is thus after
pmap_bootstrap(), which still included some technically reserved areas
of the memory map that needed to be included in the DMAP.

The odd failure noted above may be a bug, but I haven't investigated it
all that much.

Initial work by: mhorne
Sponsored by: Juniper Networks, Inc.
Sponsored by: Klara, Inc.

[kevans]

I also still haven't put too much thought into why arm64 KASAN seems to use way more stack than amd64 KASAN.

[andrew]

sys/arm64/arm64/pmap.c

1246

This is broken with a 16k PAGE_SIZE as there are only 2 L0 entries with both already created.

1260–1261

I don't think you need PMAP_SAN_PTE_BITS on table entries. It just sets bits in ignored fields.

I also wonder if we could reuse pmap_bootstrap_dmap_l1_table here. I don't think it's quite correct as it assume there are no page tables present so the second user will fail, but should allow KASAN + 16k PAGE_SIZE

1429

I'm not sure we need this (or other cases in pmap.c). We should never be modifying a valid entry in the bootstrap process.

sys/arm64/arm64/unwind.c

37 ↗

(On Diff #110954)

I wonder if we should just build this file with compile-with "${NORMAL_C:N-fsanitize*}" given it's used in the sanitizers to get a stack trace

sys/arm64/include/asan.h

2

Missing license

23

Why the commented out return statement?

sys/arm64/include/atomic.h

57

Why is this needed?

sys/arm64/include/param.h

103

Why so many? We use 6 on amd64.

[kevans]

Address review feedback.

This incorporates a version of andy's suggestion;
dmap_bootstrap_state->bootstrapped was added to skip earlier stages of table
bootstrap and applied to all uses of the state, just to keep the semantics
consistent across the board (but I'm not tied to it...). I didn't rename
pmap_bootstrap_dmap_l1_table, either. It works with 4k pages, but I can't test
16k pages because that seems to put the kernel up to 37M which can't fit in the
single L2 table that we have at this point (it blows up in .bss zeroing).

[andrew]

Having thought about it a bit more I think we should make the struct dmap_bootstrap_state global and to share it between the different callers (probably renaming it to pmap_bootstrap_state). The pmap_bootstrap_dmap_* functions could then be used by pmap_bootstrap_l2 and pmap_bootstrap_l3 with some work to check they're not trashing an existing mappin (easy after the DMAP region is avilable).

I marked a few changes that could be committed before the main KASAN change. They are also needed by KCSAN which is buildable with one compiler change to allow the thread sanitizer to be used on arm64.

sys/arm64/include/bus.h

283 ↗

(On Diff #110969)

Can you split this out to a new review as we need it for other sanitizers, e.g. KCSAN

489 ↗

(On Diff #110969)

Can you split the bus_space_set_multi_stream and bus_space_set_region_stream bits to a new review as they don't depend on KASAN

sys/dev/usb/controller/musb_otg_allwinner.c

249 ↗

(On Diff #110969)

Can you split this out to a new review as we need it for other sanitizers, e.g. KCSAN

[markj]

sys/arm64/arm64/stack_machdep.c

59 ↗

(On Diff #110969)

Do we really need this annotation anywhere except stack_capture()?

sys/arm64/include/pmap.h

186

Elsewhere you write defined(KASAN) || defined(KMSAN), which I think is ok, but to be consistent we should use that condition here too.

192

Extra newline.

sys/kern/subr_asan.c

261 ↗

(On Diff #110969)

I think this bit could be committed as-is.

[kevans]

sys/arm64/arm64/unwind.c

37 ↗

(On Diff #110954)

That seems reasonable.

sys/arm64/include/atomic.h

57

We still want to build sys/arm64/arm64/support_ifunc.c, but otherwise the symbol's only declared here in the !sanitizer branch below.

sys/arm64/include/param.h

103

This is what I haven't really spent too much time digging into; with fewer than 12 it crashes and burns elsewhere, so this is perhaps papering over something else.

[kevans]

Second round of review feedback; rebased on top of Andy's pmap bootstrap cleanup
branch.

[kevans]

sys/arm64/arm64/stack_machdep.c

59 ↗

(On Diff #110969)

We discussed this a bit OOB and I removed the annotation from all three of these, as stack_save_td() doesn't support capturing a running thread. I may add back in a comment to note that x86 prevents KASAN/KMSAN there potentially due to inconsistencies arising from that scenario, but it's unclear if there's any plan/desire to change this.

sys/dev/usb/controller/musb_otg_allwinner.c

249 ↗

(On Diff #110969)

Sorry, I had actually forgotten this in the first round, but this is done now as well.

sys/kern/subr_asan.c

261 ↗

(On Diff #110969)

Landed early in f2963b530e17

[kevans]

sys/arm64/include/param.h

103

I'm still tracking down an issue here. With these values, I see:

• 6: Panics (fault in a nofault region) as sp drops below the thread's stack allocation under load in random processes; sometimes this is just a couple pages below (always past the guard page), other times it's many many pages.
• 8: Panics become much rarer, but processes are randomly blown away under load somewhat infrequently (either SIGABRT or SIGSEGV)
• 12: Generally fine

The behavior at 12 pages would lead me to think it's just higher stack usasge, but jumping 3+ pages below the stack seems suspicious enough to warrant further investigation.

When I examine the stack to try and figure out where we may have been when things blew up, the results are... not great. The only addresses I can find are for data_abort and something else along the trap path, and the stack is otherwise sprayed with 0x5a ('Z'?) repeatedly in between other occurrences of data_abort and <mumble>'s addresses. I couldn't track down where that pattern might be coming from.

I hacked in a quick td_faultcnt to track recursion into data_abort to try and see if we were actually recursively faulting, asserting that it not raise higher than 2 (user trap into kernel trap) -- this is not the case.

[markj]

sys/arm64/arm64/pmap.c

7807

All of the leaf entries in the shadow map should also have ATTR_SW_DBM set. That bit effectively marks the mapping as dirty, since shadow map entries are always writeable. Without it, the first write to the mapping will cause the DBM bit to be set; if the platform performs DBM management in software, it'll trap and pmap_fault() will fail to fix up the mapping.

7817

Extra newline.

[kevans]

Incorporate further fixes, notably:

• Fix an off-by-one L2 block in pmap_bootstrap_allocate_kasan_l2 (kevans)
• Correctly round up for number of shadow pages, not down (markj)
• Several unused variables (markj)
• Dropped the stack size requirement back down to a reasonable value with previously encountered crashes fixed (markj)
• Fixed shadowing of the boot stack (markj)
• Added a missing store barrier after updating translation tables in pmap_san_enter (markj)
• Only set attributes on leaf entries in pmap_san_enter (markj)

[markj]

This looks ok to me modulo the one comment.

sys/arm64/include/atomic.h

58

A problem with this is that we rely on the sys/types.h include in atomic_common.h/atomic_san.h to provide a typedef for bool. This appears to break at least linux_mmap.c. One workaround is to define it as a _Bool instead.

[kevans]

sys/arm64/arm64/pmap.c

7807

We discussed this out-of-band, markj concluded that it's sufficient to make sure the RO bit isn't set

sys/arm64/include/atomic.h

58

In the interest of not adding any arch-specific header pollution, I've staged the change to _Bool locally for push or to roll into another revision.

[andrew]

sys/arm64/include/pmap.h

191

Different indentation?