Re: [PATCH] KVM: arm64: Handle CMOs on Read Only memslots

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Drew,

On 2/17/21 10:43 AM, Andrew Jones wrote:
> On Tue, Feb 16, 2021 at 12:18:31PM +0000, Alexandru Elisei wrote:
>> Hi Marc,
>>
>> Played with this for a bit to try to understand the problem better, wrote a simple
>> MMIO device in kvmtool which maps the memory as a read-only memslot [1] and poked
>> it with kvm-unit-tests [2].
>>
>> [1] https://gitlab.arm.com/linux-arm/kvmtool-ae/-/tree/mmiodev-wip1
>>
>> [2] https://gitlab.arm.com/linux-arm/kvm-unit-tests-ae/-/tree/mmiodev-wip1
> Looks like you forgot to add arm/mmiodev.c to your commit.

Fixed, thanks for pointing that out!

Thanks,

Alex

>
> Thanks,
> drew
>
>> On 2/11/21 2:27 PM, Marc Zyngier wrote:
>>> It appears that when a guest traps into KVM because it is
>>> performing a CMO on a Read Only memslot, our handling of
>>> this operation is "slightly suboptimal", as we treat it as
>>> an MMIO access without a valid syndrome.
>>>
>>> The chances that userspace is adequately equiped to deal
>>> with such an exception being slim, it would be better to
>>> handle it in the kernel.
>>>
>>> What we need to provide is roughly as follows:
>>>
>>> (a) if a CMO hits writeable memory, handle it as a normal memory acess
>>> (b) if a CMO hits non-memory, skip it
>>> (c) if a CMO hits R/O memory, that's where things become fun:
>>>   (1) if the CMO is DC IVAC, the architecture says this should result
>>>       in a permission fault
>>>   (2) if the CMO is DC CIVAC, it should work similarly to (a)
>>>
>>> We already perform (a) and (b) correctly, but (c) is a total mess.
>>> Hence we need to distinguish between IVAC (c.1) and CIVAC (c.2).
>>>
>>> One way to do it is to treat CMOs generating a translation fault as
>>> a *read*, even when they are on a RW memslot. This allows us to
>>> further triage things:
>>>
>>> If they come back with a permission fault, that is because this is
>>> a DC IVAC instruction:
>>> - inside a RW memslot: no problem, treat it as a write (a)(c.2)
>>> - inside a RO memslot: inject a data abort in the guest (c.1)
>>>
>>> The only drawback is that DC IVAC on a yet unmapped page faults
>>> twice: one for the initial translation fault that result in a RO
>>> mapping, and once for the permission fault. I think we can live with
>>> that.
>>>
>>> Reported-by: Jianyong Wu <jianyong.wu@arm.com>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>
>>> Notes:
>>>     I have taken the option to inject an abort in the guest when
>>>     it issues a DC IVAC on a R/O memslot, but another option would
>>>     be to just perform the invalidation ourselves as a DC CIAVAC.
>>>     
>>>     This would have the advantage of being consistent with what we
>>>     do for emulated MMIO.
>>>
>>>  arch/arm64/kvm/mmu.c | 53 ++++++++++++++++++++++++++++++++++----------
>>>  1 file changed, 41 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>>> index 7d2257cc5438..c7f4388bea45 100644
>>> --- a/arch/arm64/kvm/mmu.c
>>> +++ b/arch/arm64/kvm/mmu.c
>>> @@ -760,7 +760,17 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>>  	struct kvm_pgtable *pgt;
>>>  
>>>  	fault_granule = 1UL << ARM64_HW_PGTABLE_LEVEL_SHIFT(fault_level);
>>> -	write_fault = kvm_is_write_fault(vcpu);
>>> +	/*
>>> +	 * Treat translation faults on CMOs as read faults. Should
>>> +	 * this further generate a permission fault on a R/O memslot,
>>> +	 * it will be caught in kvm_handle_guest_abort(), with
>>> +	 * prejudice. Permission faults on non-R/O memslot will be
>>> +	 * gracefully handled as writes.
>>> +	 */
>>> +	if (fault_status == FSC_FAULT && kvm_vcpu_dabt_is_cm(vcpu))
>>> +		write_fault = false;
>> This means that every DC CIVAC will map the IPA with read permissions in the stage
>> 2 tables, regardless of the IPA being already mapped. It's harmless, but a bit
>> unexpected.
>>
>>> +	else
>>> +		write_fault = kvm_is_write_fault(vcpu);
>>>  	exec_fault = kvm_vcpu_trap_is_exec_fault(vcpu);
>>>  	VM_BUG_ON(write_fault && exec_fault);
>>>  
>>> @@ -1013,19 +1023,37 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
>>>  		}
>>>  
>>>  		/*
>>> -		 * Check for a cache maintenance operation. Since we
>>> -		 * ended-up here, we know it is outside of any memory
>>> -		 * slot. But we can't find out if that is for a device,
>>> -		 * or if the guest is just being stupid. The only thing
>>> -		 * we know for sure is that this range cannot be cached.
>>> +		 * Check for a cache maintenance operation. Three cases:
>>> +		 *
>>> +		 * - It is outside of any memory slot. But we can't find out
>>> +		 *   if that is for a device, or if the guest is just being
>>> +		 *   stupid. The only thing we know for sure is that this
>>> +		 *   range cannot be cached.  So let's assume that the guest
>>> +		 *   is just being cautious, and skip the instruction.
>>> +		 *
>>> +		 * - Otherwise, check whether this is a permission fault.
>>> +		 *   If so, that's a DC IVAC on a R/O memslot, which is a
>>> +		 *   pretty bad idea, and we tell the guest so.
>>>  		 *
>>> -		 * So let's assume that the guest is just being
>>> -		 * cautious, and skip the instruction.
>>> +		 * - If this wasn't a permission fault, pass it along for
>>> +		 *   further handling (including faulting the page in if it
>>> +		 *   was a translation fault).
>>>  		 */
>>> -		if (kvm_is_error_hva(hva) && kvm_vcpu_dabt_is_cm(vcpu)) {
>>> -			kvm_incr_pc(vcpu);
>>> -			ret = 1;
>>> -			goto out_unlock;
>>> +		if (kvm_vcpu_dabt_is_cm(vcpu)) {
>>> +			if (kvm_is_error_hva(hva)) {
>>> +				kvm_incr_pc(vcpu);
>>> +				ret = 1;
>>> +				goto out_unlock;
>>> +			}
>>> +
>>> +			if (fault_status == FSC_PERM) {
>>> +				/* DC IVAC on a R/O memslot */
>>> +				kvm_inject_dabt(vcpu, kvm_vcpu_get_hfar(vcpu));
>>> +				ret = 1;
>>> +				goto out_unlock;
>>> +			}
>> I don't like the inconsistency. We go from exiting to userspace for both DC
>> IVAC/DC CIVAC to mapping the IPA with read permissions for DC CIVAC, but injecting
>> a DABT for a DC IVAC. DC IVAC acts just like a DC CIVAC and requires the same
>> permissions when executed by a guest, so I'm not sure we should be handling them
>> differently.
>>
>> Thanks,
>>
>> Alex
>>
>>> +
>>> +			goto handle_access;
>>>  		}
>>>  
>>>  		/*
>>> @@ -1039,6 +1067,7 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
>>>  		goto out_unlock;
>>>  	}
>>>  
>>> +handle_access:
>>>  	/* Userspace should not be able to register out-of-bounds IPAs */
>>>  	VM_BUG_ON(fault_ipa >= kvm_phys_size(vcpu->kvm));
>>>  
>> _______________________________________________
>> kvmarm mailing list
>> kvmarm@lists.cs.columbia.edu
>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel