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