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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <86y3nzef0w.fsf@arm.com>
Date:   Wed, 25 Oct 2017 10:46:23 +0100
From:   Marc Zyngier <marc.zyngier@....com>
To:     Eric Auger <eric.auger@...hat.com>
Cc:     <eric.auger.pro@...il.com>, <linux-kernel@...r.kernel.org>,
        <kvm@...r.kernel.org>, <kvmarm@...ts.cs.columbia.edu>,
        <cdall@...aro.org>, <peter.maydell@...aro.org>,
        <andre.przywara@....com>, <wanghaibin.wang@...wei.com>,
        <wu.wubin@...wei.com>, <drjones@...hat.com>, <wei@...hat.com>
Subject: Re: [PATCH v5 03/10] KVM: arm/arm64: vgic-its: Check CBASER/BASER validity before enabling the ITS

On Wed, Oct 25 2017 at 10:38:13 am BST, Marc Zyngier <marc.zyngier@....com> wrote:
> On Mon, Oct 23 2017 at  4:08:22 pm BST, Eric Auger <eric.auger@...hat.com> 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@...hat.com>
>> Reported-by: Andre Przywara <andre.przywara@....com>
>>
>> ---
>>
>> 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.