[PATCH] kasan: arm64: support specialized outlined tag mismatch checks

[Mark Rutland]

Hi Peter,

On Thu, Oct 29, 2020 at 01:59:44PM -0700, Peter Collingbourne wrote:
> Outlined checks require a new runtime function with a custom calling
> convention. Add this function to arch/arm64/lib.

[...]

> +/*
> + * Report a tag mismatch detected by tag-based KASAN.
> + *
> + * This function has a custom calling convention in order to minimize the sizes
> + * of the compiler-generated thunks that call it. All registers except for x16
> + * and x17 must be preserved. This includes x0 and x1 which are used by the
> + * caller to pass arguments, and x29 (the frame pointer register). In order to
> + * allow these registers to be restored the caller spills them to stack. The 256
> + * bytes of stack space allocated by the caller must be deallocated on return.

This is a bit difficult to follow.

Am I right in understanding that the caller has pre-allocated 256 bytes
on the stack by decrementing the SP, yet it's up to the callee to undo
that? That seems bizarre to me, and it would be much easier (and more
generally useful) for the caller to leave it to the trampoline to
allocate/free its own space.

> + *
> + * This function takes care of transitioning to the standard AAPCS calling
> + * convention and calls the C function kasan_tag_mismatch to report the error.
> + *
> + * Parameters:
> + *	x0 - the fault address
> + *	x1 - an encoded description of the faulting access
> + */
> +SYM_FUNC_START(__hwasan_tag_mismatch)

For non-AAPCS code you should use SYM_CODE_*() rather than SYM_FUNC_*(),
and place any BTI instructions necessary manually.

We should probably add SYM_NONSTD_FUNC_*() helpers for trampolines, as
it looks like we didn't add BTIs into the ftrace trampolines (and module
PLTs can get there via indirect branches). I'll put together a
point-fix for ftrace.

> +	add	x29, sp, #232

Where has this offset come from? Is there already a frame record created
by the caller? If so, that should be mentioned in the description of the
calling convention. If not, I don't think this code works.

> +	stp	x2, x3, [sp, #16]
> +	stp	x4, x5, [sp, #32]
> +	stp	x6, x7, [sp, #48]
> +	stp	x8, x9, [sp, #64]
> +	stp	x10, x11, [sp, #80]
> +	stp	x12, x13, [sp, #96]
> +	stp	x14, x15, [sp, #112]

Is there a reason this starts at an offset of 16?

For offsets please use (16 * n) like we do in entry.S, e.g.

|	stp	x2, x3, [sp,  #16 * 0]
|	stp	x4, x5, [sp,  #16 * 1]
|	stp	x4, x5, [sp,  #16 * 2]

... as it makes the relationship clearer.

> +#ifndef CONFIG_SHADOW_CALL_STACK
> +	str	x18, [sp, #128]
> +#endif

This doesn't look right to me. The shadow call stack should remain
balanced across a call, so why do you need to save x18? any manipulation
within kasan_tag_mismatch should be balanced.

Did you mean to save the return address to the shadow stack, rather
than saving the shadow stack pointer?

> +	mov	x2, x30
> +	bl	kasan_tag_mismatch

We didn't preserve x0 and x1 above, so they can be clobbered here, but
the comment said we needed to preserve them.

> +	ldp	x29, x30, [sp, #232]

This doesn't look right. This function didn't save x29/x30 to the stack,
so the only way this could work is if the caller had a redundant frame
record pointing to itself.

> +#ifndef CONFIG_SHADOW_CALL_STACK
> +	ldr	x18, [sp, #128]
> +#endif

As above, this doesn't look right.

> +	ldp	x14, x15, [sp, #112]
> +	ldp	x12, x13, [sp, #96]
> +	ldp	x10, x11, [sp, #80]
> +	ldp	x8, x9, [sp, #64]
> +	ldp	x6, x7, [sp, #48]
> +	ldp	x4, x5, [sp, #32]
> +	ldp	x2, x3, [sp, #16]
> +	ldp	x0, x1, [sp], #256

As above, we didn't save x0 and x1 -- did the caller do that?

Likewise, did the caller allocate 256 bytes that we have to deallocate?
That's *really* confusing.

Thanks,
Mark.