[PATCH 0/1] KVM: arm64: Make nVHE ASLR conditional on cmdline nokaslr

[PATCH 0/1] KVM: arm64: Make nVHE ASLR conditional on cmdline nokaslr

[qixiang.xu]

From: Qxiang Xu <qixiang.xu@outlook.com>

The nokaslr option in the kernel command line cannot disable the hyp VA
random tag. This patch allows nokaslr to also disable the use of a
random tag for hyp VA.

Qxiang Xu (1):
  KVM: arm64: Make nVHE ASLR conditional on nokaslr

 arch/arm64/kvm/va_layout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.25.1

[PATCH 1/1] KVM: arm64: Make nVHE ASLR conditional on nokaslr

[qixiang.xu]

From: Qxiang Xu <qixiang.xu@outlook.com>

The random tag of hyp VA is determined by the `CONFIG_RANDOMIZE_BASE`
option, so even if `nokaslr` is set in the cmdline, KASLR cannot be
disabled for hyp VA. To align with kernel behavior, disable KASLR if
the kernel cmdline includes `nokaslr`.

Link: https://lore.kernel.org/r/20240905061659.3410362-1-qixiang.xu@outlook.com
Signed-off-by: Qxiang Xu <qixiang.xu@outlook.com>
---
 arch/arm64/kvm/va_layout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/va_layout.c b/arch/arm64/kvm/va_layout.c
index 91b22a014610..bebb4b1ddc82 100644
--- a/arch/arm64/kvm/va_layout.c
+++ b/arch/arm64/kvm/va_layout.c
@@ -72,7 +72,7 @@ __init void kvm_compute_layout(void)
 	va_mask = GENMASK_ULL(tag_lsb - 1, 0);
 	tag_val = hyp_va_msb;
 
-	if (IS_ENABLED(CONFIG_RANDOMIZE_BASE) && tag_lsb != (vabits_actual - 1)) {
+	if (kaslr_enabled() && tag_lsb != (vabits_actual - 1)) {
 		/* We have some free bits to insert a random tag. */
 		tag_val |= get_random_long() & GENMASK_ULL(vabits_actual - 2, tag_lsb);
 	}
-- 
2.25.1

Re: [PATCH 1/1] KVM: arm64: Make nVHE ASLR conditional on nokaslr

[Marc Zyngier]

On Thu, 05 Sep 2024 07:30:26 +0100,
qixiang.xu@outlook.com wrote:
> 
> From: Qxiang Xu <qixiang.xu@outlook.com>
> 
> The random tag of hyp VA is determined by the `CONFIG_RANDOMIZE_BASE`
> option, so even if `nokaslr` is set in the cmdline, KASLR cannot be
> disabled for hyp VA. To align with kernel behavior, disable KASLR if
> the kernel cmdline includes `nokaslr`.
> 
> Link: https://lore.kernel.org/r/20240905061659.3410362-1-qixiang.xu@outlook.com

I get a 404.

> Signed-off-by: Qxiang Xu <qixiang.xu@outlook.com>
> ---
>  arch/arm64/kvm/va_layout.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/va_layout.c b/arch/arm64/kvm/va_layout.c
> index 91b22a014610..bebb4b1ddc82 100644
> --- a/arch/arm64/kvm/va_layout.c
> +++ b/arch/arm64/kvm/va_layout.c
> @@ -72,7 +72,7 @@ __init void kvm_compute_layout(void)
>  	va_mask = GENMASK_ULL(tag_lsb - 1, 0);
>  	tag_val = hyp_va_msb;
>  
> -	if (IS_ENABLED(CONFIG_RANDOMIZE_BASE) && tag_lsb != (vabits_actual - 1)) {
> +	if (kaslr_enabled() && tag_lsb != (vabits_actual - 1)) {
>  		/* We have some free bits to insert a random tag. */
>  		tag_val |= get_random_long() & GENMASK_ULL(vabits_actual - 2, tag_lsb);
>  	}

This is a change in behaviour that would leave the 2 implementations
affected by Spectre-v3a unmitigated and leaking information to
*guests*, while they would have been safe until this change. Is this
what we really want to do?

This is also not disabling the whole thing, since we still do the
indirect vector dance.

So while I'm not opposed to having an option that disables the
randomisation, it has to match two requirements:

- it has to be a *new* option -- changing an existing behaviour is not
  acceptable,

- it has to disable both the VA alteration and the vector indirection.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 1/1] KVM: arm64: Make nVHE ASLR conditional on nokaslr

[Qixiang Xu]

Marc,

Thanks for your reply. 

> This is a change in behaviour that would leave the 2 implementations
> affected by Spectre-v3a unmitigated and leaking information to
> *guests*, while they would have been safe until this change. Is this
> what we really want to do?

The reason for adding this is to make debugging nvhe hyp code easier. 
Otherwise, we would need to calculate the offset every time.
Do you have any better suggestions for the debugging?


> This is also not disabling the whole thing, since we still do the
> indirect vector dance.

I'm not sure if my understanding is correct, but based on 
the hyp_map_vectors function, the address of the indirect vector
is only related to __io_map_base and is not random.


Thanks. 
Qixiang Xu

Re: [PATCH 1/1] KVM: arm64: Make nVHE ASLR conditional on nokaslr

[Marc Zyngier]

On Fri, 06 Sep 2024 08:19:07 +0100,
Qixiang Xu <qixiang.xu@outlook.com> wrote:
> 
> Marc,
> 
> Thanks for your reply. 
> 
> > This is a change in behaviour that would leave the 2 implementations
> > affected by Spectre-v3a unmitigated and leaking information to
> > *guests*, while they would have been safe until this change. Is this
> > what we really want to do?
> 
> The reason for adding this is to make debugging nvhe hyp code easier. 
> Otherwise, we would need to calculate the offset every time.
> Do you have any better suggestions for the debugging?

You already have facilities to dump stacktraces from the HYP code, and
Vincent's tracing infrastructure is available on the list (feel free
to review it!).

And as I said, I'm not opposed to disabling the randomisation with a
command-line option. I oppose to using 'nokaslr' for this, as it
changes the existing behaviour.

> > This is also not disabling the whole thing, since we still do the
> > indirect vector dance.
> 
> I'm not sure if my understanding is correct, but based on 
> the hyp_map_vectors function, the address of the indirect vector
> is only related to __io_map_base and is not random.

Of course it isn't random. It is in the idmap, since VBAR_EL2 can be
leaked to EL1, and that's the whole point that the only thing you can
leak isn't random.

But when you decide to disable randomisation, you might as well
disable the indirection, which adds extra complexity for no benefit.

You may want to read [1] to get the context of what you are changing.

	M.

[1] https://lore.kernel.org/all/20180314165049.30105-1-marc.zyngier@arm.com/

-- 
Without deviation from the norm, progress is not possible.