Re: [PATCH v5 03/10] KVM: arm/arm64: vgic-its: Check CBASER/BASER validity before enabling the ITS

[Marc Zyngier]

On Wed, Oct 25 2017 at 10:38:13 am BST, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> On Mon, Oct 23 2017 at  4:08:22 pm BST, Eric Auger <eric.auger@xxxxxxxxxx> wrote:
>> The spec says it is UNPREDICTABLE to enable the ITS
>> if any of the following conditions are true:
>>
>> - GITS_CBASER.Valid == 0.
>> - GITS_BASER<n>.Valid == 0, for any GITS_BASER<n> register
>>   where the Type field indicates Device.
>> - GITS_BASER<n>.Valid == 0, for any GITS_BASER<n> register
>>   where the Type field indicates Interrupt Collection and
>>   GITS_TYPER.HCC == 0.
>>
>> In that case, let's keep the ITS disabled.
>>
>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>> Reported-by: Andre Przywara <andre.przywara@xxxxxxx>
>>
>> ---
>>
>> need to be CC'ed stable
>>
>> v4 -> v5:
>> - check the condition before updating its->enabled and
>>   fix its->cbaser && GITS_CBASER_VALID
>>
>> v3: creation
>> ---
>>  virt/kvm/arm/vgic/vgic-its.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index b0ba80f..1eb355e 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -1466,6 +1466,16 @@ static void vgic_mmio_write_its_ctlr(struct kvm *kvm, struct vgic_its *its,
>>  {
>>  	mutex_lock(&its->cmd_lock);
>>  
>> +	/*
>> +	 * It is UNPREDICTABLE to enable the ITS if any of the CBASER or
>> +	 * device/collection BASER are invalid
>> +	 */
>> +	if (!its->enabled && (val & GITS_CTLR_ENABLE) &&
>> +		(!(its->baser_device_table & GITS_BASER_VALID) ||
>> +		 !(its->baser_coll_table & GITS_BASER_VALID) ||
>> +		 !(its->cbaser & GITS_CBASER_VALID)))
>> +		goto out;
>> +
>>  	its->enabled = !!(val & GITS_CTLR_ENABLE);
>>  
>>  	/*
>> @@ -1474,6 +1484,7 @@ static void vgic_mmio_write_its_ctlr(struct kvm *kvm, struct vgic_its *its,
>>  	 */
>>  	vgic_its_process_commands(kvm, its);
>>  
>> +out:
>>  	mutex_unlock(&its->cmd_lock);
>>  }
>
> While this is definitely a good hardening of the implementation, I don't
> think it fixes anything for the guest which is already misbehaving and
> would just not get anything out of this misconfigurarion (in line with
> the UNPRED requirement).
>
> So I don't think we need to Cc stable for this.

I'm having second thoughts. If the guest has written junk in one of the
BASER registers, enabled the ITS (which won't work), and is then
save/restored, userspace is going to get an -EFAULT as part of the
restore process. Not great.

So cc-stable is probably justified here.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.