Re: [PATCH] KVM: s390: vsie: Consolidate CRYCB validation

[David Hildenbrand <david@redhat.com>]

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@linux.ibm.com>
> ---
>  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