Re: [PATCH v5 14/23] arm64: KVM: Introduce EL2 VA randomisation

[James Morse <james.morse@arm.com>]

[Resending with Marc's email address fixed, looks like I miss-operated the mouse
and pasted some stray text in the address...]

Hi Marc,

On 01/03/18 15:55, Marc Zyngier wrote:
> The main idea behind randomising the EL2 VA is that we usually have
> a few spare bits between the most significant bit of the VA mask
> and the most significant bit of the linear mapping.
>
> Those bits could be a bunch of zeroes, and could be useful
> to move things around a bit. Of course, the more memory you have,
> the less randomisation you get...
>
> Alternatively, these bits could be the result of KASLR, in which
> case they are already random. But it would be nice to have a
> *different* randomization, just to make the job of a potential
> attacker a bit more difficult.
>
> Inserting these random bits is a bit involved. We don't have a spare
> register (short of rewriting all the kern_hyp_va call sites), and
> the immediate we want to insert is too random to be used with the
> ORR instruction. The best option I could come up with is the following
> sequence:
>
> 	and x0, x0, #va_mask
> 	ror x0, x0, #first_random_bit
> 	add x0, x0, #(random & 0xfff)
> 	add x0, x0, #(random >> 12), lsl #12
> 	ror x0, x0, #(63 - first_random_bit)
>
> making it a fairly long sequence, but one that a decent CPU should
> be able to execute without breaking a sweat. It is of course NOPed
> out on VHE. The last 4 instructions can also be turned into NOPs
> if it appears that there is no free bits to use.

> diff --git a/arch/arm64/kvm/va_layout.c b/arch/arm64/kvm/va_layout.c
> index 2bcbc1d33b64..a73e47804972 100644
> --- a/arch/arm64/kvm/va_layout.c
> +++ b/arch/arm64/kvm/va_layout.c
> @@ -16,24 +16,62 @@
>   */
>  
>  #include <linux/kvm_host.h>
> +#include <linux/random.h>
> +#include <linux/memblock.h>
>  #include <asm/alternative.h>
>  #include <asm/debug-monitors.h>
>  #include <asm/insn.h>
>  #include <asm/kvm_mmu.h>
>  
> +/*
> + * The LSB of the random hyp VA tag or 0 if no randomization is used.
> + */
> +static u8 tag_lsb;
> +/*
> + * The random hyp VA tag value with the region bit if hyp randomization is used
> + */
> +static u64 tag_val;
>  static u64 va_mask;
>  
>  static void compute_layout(void)
>  {
>  	phys_addr_t idmap_addr = __pa_symbol(__hyp_idmap_text_start);
>  	u64 hyp_va_msb;
> +	int kva_msb;
>  
>  	/* Where is my RAM region? */
>  	hyp_va_msb  = idmap_addr & BIT(VA_BITS - 1);
>  	hyp_va_msb ^= BIT(VA_BITS - 1);
>  
> -	va_mask  = GENMASK_ULL(VA_BITS - 2, 0);
> -	va_mask |= hyp_va_msb;
> +	kva_msb = fls64((u64)phys_to_virt(memblock_start_of_DRAM()) ^
> +			(u64)(high_memory - 1));
> +	if (kva_msb == (VA_BITS - 1)) {
> +		/*
> +		 * No space in the address, let's compute the mask so
> +		 * that it covers (VA_BITS - 1) bits, and the region
> +		 * bit. The tag stays set to zero.
> +		 */
> +		va_mask  = BIT(VA_BITS - 1) - 1;
> +		va_mask |= hyp_va_msb;
> +	} else {
> +		/*
> +		 * We do have some free bits to insert a random tag.
> +		 * Hyp VAs are now created from kernel linear map VAs
> +		 * using the following formula (with V == VA_BITS):
> +		 *
> +		 *  63 ... V |     V-1    | V-2 .. tag_lsb | tag_lsb - 1 .. 0
> +		 *  ---------------------------------------------------------
> +		 * | 0000000 | hyp_va_msb |    random tag  |  kern linear VA |
> +		 */
> +		u64 mask = GENMASK_ULL(VA_BITS - 2, tag_lsb);

(tag_lsb is 0 at this point, but you shift out the bits you didn't want later)


> +
> +		tag_lsb = kva_msb;
> +		va_mask = GENMASK_ULL(tag_lsb - 1, 0);
> +		tag_val  = get_random_long() & mask;
> +		tag_val |= hyp_va_msb;
> +		tag_val >>= tag_lsb;
> +	}
>  }

>  static u32 compute_instruction(int n, u32 rd, u32 rn)
> @@ -46,6 +84,33 @@ static u32 compute_instruction(int n, u32 rd, u32 rn)
>  							  AARCH64_INSN_VARIANT_64BIT,
>  							  rn, rd, va_mask);
>  		break;
> +
> +	case 1:
> +		/* ROR is a variant of EXTR with Rm = Rn */
> +		insn = aarch64_insn_gen_extr(AARCH64_INSN_VARIANT_64BIT,
> +					     rn, rn, rd,
> +					     tag_lsb);
> +		break;
> +
> +	case 2:
> +		insn = aarch64_insn_gen_add_sub_imm(rd, rn,
> +						    tag_val & (SZ_4K - 1),
> +						    AARCH64_INSN_VARIANT_64BIT,
> +						    AARCH64_INSN_ADSB_ADD);
> +		break;
> +
> +	case 3:
> +		insn = aarch64_insn_gen_add_sub_imm(rd, rn,
> +						    tag_val & GENMASK(23, 12),
> +						    AARCH64_INSN_VARIANT_64BIT,
> +						    AARCH64_INSN_ADSB_ADD);
> +		break;

(This mix of GENMASK() and (SZ_4K - 1) feels a bit odd)


> @@ -68,8 +132,12 @@ void __init kvm_update_va_mask(struct alt_instr *alt,
>  		/*
>  		 * VHE doesn't need any address translation, let's NOP
>  		 * everything.
> +		 *
> +		 * Alternatively, if we don't have any spare bits in
> +		 * the address, NOP everything after masking that
> +		 * kernel VA.
>  		 */

> -		if (has_vhe()) {
> +		if (has_vhe() || (!tag_lsb && i > 1)) {
>  			updptr[i] = aarch64_insn_gen_nop();
>  			continue;
>  		}

Typo? Won't this keep the first ror too, instead of just the 'and'?
| (!tag_lsb && i > 0)

(I agree 'ROR #0' is still a NOP)

Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James