Re: [PATCH] KVM: s390: vsie: Consolidate CRYCB validation
From: David Hildenbrand
Date: Wed Aug 22 2018 - 04:25:18 EST
On 22.08.2018 10:08, Pierre Morel wrote:
> Currently when shadowing the CRYCB on SIE entrance, the validation
> tests the following:
> - accept only FORMAT1 or FORMAT2
> - test if MSAext facility (76) is installed
> - accept the CRYCB if no keys are used
> - verifies that the CRYCB format1 is inside a page
> - verifies that the CRYCB origin is not 0
>
> This is not following the architecture.
I have to trust you on that :)
>
> On SIE entrance, the CRYCB must be validated before accepting
> any of its entries.
>
> Let's do the validation in the right order and also verify
> correctly the FORMAT2 CRYCB.
With which facility was FORMAT2 introduced?
Does MSA3 imply that FORMAT2 can be used? (even if AP is absent)
FORMAT2 is backwards compatible to FORMAT1,
>
> The testing of facility MSAext3 (76) is not useful as it is
> already tested by kvm_crypto_init() to set FORMAT1.
Indeed, having FORMAT1 in g1 implies that.
>
> The testing of a null CRYCB origin must be done what ever
> the format of the guest3 CRYCB is.
>
> The CRYCB must be contained inside a page, but the CRYCB size
> depends on the CRYCB format.
> Lets test what the guest2 initialized, we can not trust it to have
> done things right.
>
> Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
> ---
> arch/s390/kvm/vsie.c | 35 +++++++++++++++++++++++++----------
> 1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index a2b28cd..35c3907 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -158,28 +158,43 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> scb_s->crycbd = 0;
> if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
> return 0;
> - /* format-1 is supported with message-security-assist extension 3 */
> - if (!test_kvm_facility(vcpu->kvm, 76))
> - return 0;
> + /*
> + * If APIE is set or it the CRYCB Format is FORMAT1 or FORMAT2 with
> + * APXA installed, the machine checks the validity of crycb origin.
> + * KVM kvm_s390_crypto_init() makes sure that FORMAT2 is only used
> + * if APXA is installed.
> + * The guest2 hypervizor could have set APIE and Format2 so let's
> + * test all these points.
> + * We here have always a CRYCB FORMAT1 or FORMAT2 (FORMAT0 was
> + * refused in previous test).
Can you shorten that comment and leave out all stuff to be added next?
(APIE, APXA ...). I guess this whole comment is to be left out of this
patch.
> + */
> + if (!crycb_addr)
> + return set_validity_icpt(scb_s, 0x0039U);
> +
> + if ((crycbd_o & 0x03) == CRYCB_FORMAT1)
Can you instead of 0x03 define CRYCB_FORMAT_MASK
> + if ((crycb_addr & PAGE_MASK) !=
> + ((crycb_addr + 128) & PAGE_MASK))
please add one space in front of the second line to properly indent
> + return set_validity_icpt(scb_s, 0x003CU);
> +
> + if ((crycbd_o & 0x03) == CRYCB_FORMAT2)
> + if ((crycb_addr & PAGE_MASK) !=
> + ((crycb_addr + 256) & PAGE_MASK))
dito
> + return set_validity_icpt(scb_s, 0x003CU);
> +
> /* we may only allow it if enabled for guest 2 */
> ecb3_flags = scb_o->ecb3 & vcpu->arch.sie_block->ecb3 &
> (ECB3_AES | ECB3_DEA);
> if (!ecb3_flags)
> return 0;
>
> - if ((crycb_addr & PAGE_MASK) != ((crycb_addr + 128) & PAGE_MASK))
> - return set_validity_icpt(scb_s, 0x003CU);
> - else if (!crycb_addr)
> - return set_validity_icpt(scb_s, 0x0039U);
> -
> /* copy only the wrapping keys */
> if (read_guest_real(vcpu, crycb_addr + 72,
> vsie_page->crycb.dea_wrapping_key_mask, 56))
> return set_validity_icpt(scb_s, 0x0035U);
>
> scb_s->ecb3 |= ecb3_flags;
> - scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT1 |
> - CRYCB_FORMAT2;
> + /* Set the shadow CRYCB format to format 2 */
I don't consider this comment helpful (CRYCB_FORMAT2 below is at least
obvious to me) - CRYCB_FORMAT2 implies CRYCB_FORMAT1 (what the existing
code did not consider)
> + scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT2;
>
> /* xor both blocks in one run */
> b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask;
>
Thanks for looking into this.
--
Thanks,
David / dhildenb