Re: [PATCH v4 07/25] KVM: arm64: Prevent the donation of no-map pages

[Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>]

Hi Will & Quentin,

On 17/10/22 13:51, Will Deacon wrote:
> From: Quentin Perret <qperret@xxxxxxxxxx>
>
> Memory regions marked as "no-map" in the host device-tree routinely
> include TrustZone carevouts and DMA pools. Although donating such pages

Typo "carve-outs"?

> to the hypervisor may not breach confidentiality, it could be used to
> corrupt its state in uncontrollable ways. To prevent this, let's block
> host-initiated memory transitions targeting "no-map" pages altogether in
> nVHE protected mode as there should be no valid reason to do this in
> current operation.
>
> Thankfully, the pKVM EL2 hypervisor has a full copy of the host's list
> of memblock regions, so we can easily check for the presence of the
> MEMBLOCK_NOMAP flag on a region containing pages being donated from the
> host.
>
> Tested-by: Vincent Donnefort <vdonnefort@xxxxxxxxxx>
> Signed-off-by: Quentin Perret <qperret@xxxxxxxxxx>
> Signed-off-by: Will Deacon <will@xxxxxxxxxx>
> ---
>   arch/arm64/kvm/hyp/nvhe/mem_protect.c | 22 ++++++++++++++++------
>   1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> index c30402737548..a7156fd13bc8 100644
> --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> @@ -193,7 +193,7 @@ struct kvm_mem_range {
>   	u64 end;
>   };

>   bool addr_is_memory(phys_addr_t phys)
>   {
>   	struct kvm_mem_range range;
>   
> -	return find_mem_range(phys, &range);
> +	return !!find_mem_range(phys, &range);
> +}
> +
> +static bool addr_is_allowed_memory(phys_addr_t phys)
> +{
> +	struct memblock_region *reg;
> +	struct kvm_mem_range range;
> +
> +	reg = find_mem_range(phys, &range);
> +
> +	return reg && !(reg->flags & MEMBLOCK_NOMAP);
>   }
>   
>   static bool is_in_mem_range(u64 addr, struct kvm_mem_range *range)
> @@ -346,7 +356,7 @@ static bool host_stage2_force_pte_cb(u64 addr, u64 end, enum kvm_pgtable_prot pr
>   static int host_stage2_idmap(u64 addr)
>   {
>   	struct kvm_mem_range range;
> -	bool is_memory = find_mem_range(addr, &range);
> +	bool is_memory = !!find_mem_range(addr, &range);

We don't replace by addr_is_allowed_memory() because we still use
&range, OK.

>   	enum kvm_pgtable_prot prot;
>   	int ret;
>   
> @@ -424,7 +434,7 @@ static int __check_page_state_visitor(u64 addr, u64 end, u32 level,
>   	struct check_walk_data *d = arg;
>   	kvm_pte_t pte = *ptep;
>   
> -	if (kvm_pte_valid(pte) && !addr_is_memory(kvm_pte_to_phys(pte)))
> +	if (kvm_pte_valid(pte) && !addr_is_allowed_memory(kvm_pte_to_phys(pte)))
>   		return -EINVAL;

Reviewed-by: Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>