[PATCH v1] KVM: s390: vsie: fix Do the CRYCB validation first

[PATCH v1] KVM: s390: vsie: fix Do the CRYCB validation first

[Pierre Morel]

This is a BUGFIX

Surely must be CC stable.



Pierre Morel (1):
  KVM: s390: vsie: fix Do the CRYCB validation first

 arch/s390/kvm/vsie.c | 3 +++
 1 file changed, 3 insertions(+)

-- 
2.7.4

[PATCH v1] KVM: s390: vsie: fix Do the CRYCB validation first

[Pierre Morel]

The case when the SIE for guest3 is not setup for using
encryption keys nor Adjunct processor but the guest2
does use these features was not properly handled.

This leads SIE entry for guest3 to crash with validity intercept
because the guest2, not having the use of encryption keys nor
Adjunct Processor did not initialize the CRYCB designation.

In the case where none of ECA_APIE, ECB3_AES or ECB3_DEA
are set in guest3 a format 0 CRYCB is allowed for guest3
and the CRYCB designation in the SIE for guest3 is not checked
on SIE entry.

Let's allow the CRYCD designation to be ignored when the
SIE for guest3 is not initialized for encryption key usage
nor AP.

Fixup: d6f6959 (KVM: s390: vsie: Do the CRYCB validation first)

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Reported-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/vsie.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index a153257..a748f76 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -300,6 +300,9 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 	if (!apie_h && !key_msk)
 		return 0;
 
+	if (!(scb_o->eca & ECA_APIE) && !(scb_o->ecb3 & (ECB3_AES | ECB3_DEA)))
+		return 0;
+
 	if (!crycb_addr)
 		return set_validity_icpt(scb_s, 0x0039U);
 
-- 
2.7.4

Re: [PATCH v1] KVM: s390: vsie: fix Do the CRYCB validation first

[Cornelia Huck]

On Fri,  1 Feb 2019 10:52:05 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> The case when the SIE for guest3 is not setup for using
> encryption keys nor Adjunct processor but the guest2
> does use these features was not properly handled.
> 
> This leads SIE entry for guest3 to crash with validity intercept
> because the guest2, not having the use of encryption keys nor
> Adjunct Processor did not initialize the CRYCB designation.
> 
> In the case where none of ECA_APIE, ECB3_AES or ECB3_DEA
> are set in guest3 a format 0 CRYCB is allowed for guest3
> and the CRYCB designation in the SIE for guest3 is not checked
> on SIE entry.
> 
> Let's allow the CRYCD designation to be ignored when the

s/CRYCD/CRYCB/

> SIE for guest3 is not initialized for encryption key usage
> nor AP.
> 
> Fixup: d6f6959 (KVM: s390: vsie: Do the CRYCB validation first)

I think the canonical format is

Fixes: d6f6959ac587 ("KVM: s390: vsie: Do the CRYCB validation first")

> 

CC: stable@vger.kernel.org # 4.20+

> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Reported-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  arch/s390/kvm/vsie.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index a153257..a748f76 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -300,6 +300,9 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  	if (!apie_h && !key_msk)
>  		return 0;
>  
> +	if (!(scb_o->eca & ECA_APIE) && !(scb_o->ecb3 & (ECB3_AES | ECB3_DEA)))
> +		return 0;
> +
>  	if (!crycb_addr)
>  		return set_validity_icpt(scb_s, 0x0039U);
>  

Looks sane to me, but I'll let vsie experts comment.

Re: [PATCH v1] KVM: s390: vsie: fix Do the CRYCB validation first

[Pierre Morel]

On 01/02/2019 11:50, Cornelia Huck wrote:
> On Fri,  1 Feb 2019 10:52:05 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> The case when the SIE for guest3 is not setup for using
>> encryption keys nor Adjunct processor but the guest2
>> does use these features was not properly handled.
>>
>> This leads SIE entry for guest3 to crash with validity intercept
>> because the guest2, not having the use of encryption keys nor
>> Adjunct Processor did not initialize the CRYCB designation.
>>
>> In the case where none of ECA_APIE, ECB3_AES or ECB3_DEA
>> are set in guest3 a format 0 CRYCB is allowed for guest3
>> and the CRYCB designation in the SIE for guest3 is not checked
>> on SIE entry.
>>
>> Let's allow the CRYCD designation to be ignored when the
> 
> s/CRYCD/CRYCB/
> 
>> SIE for guest3 is not initialized for encryption key usage
>> nor AP.
>>
>> Fixup: d6f6959 (KVM: s390: vsie: Do the CRYCB validation first)
> 
> I think the canonical format is
> 
> Fixes: d6f6959ac587 ("KVM: s390: vsie: Do the CRYCB validation first")
> 
>>
> 
> CC: stable@vger.kernel.org # 4.20+
> 
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reported-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>> ---
>>   arch/s390/kvm/vsie.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index a153257..a748f76 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -300,6 +300,9 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>   	if (!apie_h && !key_msk)
>>   		return 0;
>>   
>> +	if (!(scb_o->eca & ECA_APIE) && !(scb_o->ecb3 & (ECB3_AES | ECB3_DEA)))
>> +		return 0;
>> +
>>   	if (!crycb_addr)
>>   		return set_validity_icpt(scb_s, 0x0039U);
>>   
> 
> Looks sane to me, but I'll let vsie experts comment.
> 

Thanks
Pierre

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v1] KVM: s390: vsie: fix Do the CRYCB validation first

[David Hildenbrand]

On 01.02.19 10:52, Pierre Morel wrote:
> The case when the SIE for guest3 is not setup for using
> encryption keys nor Adjunct processor but the guest2
> does use these features was not properly handled.
> 
> This leads SIE entry for guest3 to crash with validity intercept
> because the guest2, not having the use of encryption keys nor
> Adjunct Processor did not initialize the CRYCB designation.
> 
> In the case where none of ECA_APIE, ECB3_AES or ECB3_DEA
> are set in guest3 a format 0 CRYCB is allowed for guest3
> and the CRYCB designation in the SIE for guest3 is not checked
> on SIE entry.
> 
> Let's allow the CRYCD designation to be ignored when the
> SIE for guest3 is not initialized for encryption key usage
> nor AP.
> 
> Fixup: d6f6959 (KVM: s390: vsie: Do the CRYCB validation first)
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Reported-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  arch/s390/kvm/vsie.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index a153257..a748f76 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -300,6 +300,9 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  	if (!apie_h && !key_msk)
>  		return 0;
>  
> +	if (!(scb_o->eca & ECA_APIE) && !(scb_o->ecb3 & (ECB3_AES | ECB3_DEA)))
> +		return 0;
> +
>  	if (!crycb_addr)
>  		return set_validity_icpt(scb_s, 0x0039U);
>  
> 

The original patch said

"We need to handle the validity checks for the crycb, no matter what the
 settings for the keywrappings are. So lets move the keywrapping checks
 after we have done the validy checks."

Can you explain why keywrapping now is important? These patches seem to
contradict.

-- 

Thanks,

David / dhildenb

Re: [PATCH v1] KVM: s390: vsie: fix Do the CRYCB validation first

[Pierre Morel]

On 01/02/2019 11:56, David Hildenbrand wrote:
> On 01.02.19 10:52, Pierre Morel wrote:
>> The case when the SIE for guest3 is not setup for using
>> encryption keys nor Adjunct processor but the guest2
>> does use these features was not properly handled.
>>
>> This leads SIE entry for guest3 to crash with validity intercept
>> because the guest2, not having the use of encryption keys nor
>> Adjunct Processor did not initialize the CRYCB designation.
>>
>> In the case where none of ECA_APIE, ECB3_AES or ECB3_DEA
>> are set in guest3 a format 0 CRYCB is allowed for guest3
>> and the CRYCB designation in the SIE for guest3 is not checked
>> on SIE entry.
>>
>> Let's allow the CRYCD designation to be ignored when the
>> SIE for guest3 is not initialized for encryption key usage
>> nor AP.
>>
>> Fixup: d6f6959 (KVM: s390: vsie: Do the CRYCB validation first)
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reported-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>> ---
>>   arch/s390/kvm/vsie.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index a153257..a748f76 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -300,6 +300,9 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>   	if (!apie_h && !key_msk)
>>   		return 0;
>>   
>> +	if (!(scb_o->eca & ECA_APIE) && !(scb_o->ecb3 & (ECB3_AES | ECB3_DEA)))
>> +		return 0;
>> +
>>   	if (!crycb_addr)
>>   		return set_validity_icpt(scb_s, 0x0039U);
>>   
>>
> 
> The original patch said
> 
> "We need to handle the validity checks for the crycb, no matter what the
>   settings for the keywrappings are. So lets move the keywrapping checks
>   after we have done the validy checks."
> 
> Can you explain why keywrapping now is important? These patches seem to
> contradict.
> 

No it does not, having the flags set or not is part of the validity check.
but, I acted too fast.

The problem seems to be even clearer:
key_msk is defined as
    int key_msk = test_kvm_facility(vcpu->kvm, 76);

If it is defined, as it should for a mask, as
	(scb_o->ecb3 & (ECB3_AES | ECB3_DEA))

all is clear..., key_msk is not use but for this test, so I do not 
understand why it is set as facility 76.

so I think I better do:


diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index a153257..30843a8 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -289,7 +289,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, 
struct vsie_page *vsie_page)
         unsigned long *b1, *b2;
         u8 ecb3_flags;
         int apie_h;
-       int key_msk = test_kvm_facility(vcpu->kvm, 76);
+       int key_msk = scb_o->ecb3 & (ECB3_AES | ECB3_DEA);
         int fmt_o = crycbd_o & CRYCB_FORMAT_MASK;
         int fmt_h = vcpu->arch.sie_block->crycbd & CRYCB_FORMAT_MASK;
         int ret = 0;


So just define a mask a mask.
I verify the functionality and test on Monday  and if in between it 
seems better to you so too I post the patch.

Thanks,
Pierre

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v1] KVM: s390: vsie: fix Do the CRYCB validation first

[Christian Borntraeger]

On 01.02.2019 14:37, Pierre Morel wrote:
> On 01/02/2019 11:56, David Hildenbrand wrote:
>> On 01.02.19 10:52, Pierre Morel wrote:
>>> The case when the SIE for guest3 is not setup for using
>>> encryption keys nor Adjunct processor but the guest2
>>> does use these features was not properly handled.
>>>
>>> This leads SIE entry for guest3 to crash with validity intercept
>>> because the guest2, not having the use of encryption keys nor
>>> Adjunct Processor did not initialize the CRYCB designation.
>>>
>>> In the case where none of ECA_APIE, ECB3_AES or ECB3_DEA
>>> are set in guest3 a format 0 CRYCB is allowed for guest3
>>> and the CRYCB designation in the SIE for guest3 is not checked
>>> on SIE entry.
>>>
>>> Let's allow the CRYCD designation to be ignored when the
>>> SIE for guest3 is not initialized for encryption key usage
>>> nor AP.
>>>
>>> Fixup: d6f6959 (KVM: s390: vsie: Do the CRYCB validation first)
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> Reported-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>>> ---
>>>   arch/s390/kvm/vsie.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>> index a153257..a748f76 100644
>>> --- a/arch/s390/kvm/vsie.c
>>> +++ b/arch/s390/kvm/vsie.c
>>> @@ -300,6 +300,9 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>       if (!apie_h && !key_msk)
>>>           return 0;
>>>   +    if (!(scb_o->eca & ECA_APIE) && !(scb_o->ecb3 & (ECB3_AES | ECB3_DEA)))
>>> +        return 0;
>>> +
>>>       if (!crycb_addr)
>>>           return set_validity_icpt(scb_s, 0x0039U);
>>>  
>>
>> The original patch said
>>
>> "We need to handle the validity checks for the crycb, no matter what the
>>   settings for the keywrappings are. So lets move the keywrapping checks
>>   after we have done the validy checks."
>>
>> Can you explain why keywrapping now is important? These patches seem to
>> contradict.
>>
> 
> No it does not, having the flags set or not is part of the validity check.
> but, I acted too fast.
> 
> The problem seems to be even clearer:
> key_msk is defined as
>    int key_msk = test_kvm_facility(vcpu->kvm, 76);
> 
> If it is defined, as it should for a mask, as
>     (scb_o->ecb3 & (ECB3_AES | ECB3_DEA))
> 
> all is clear..., key_msk is not use but for this test, so I do not understand why it is set as facility 76.
> 
> so I think I better do:
> 
> 
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index a153257..30843a8 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -289,7 +289,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>         unsigned long *b1, *b2;
>         u8 ecb3_flags;
>         int apie_h;
> -       int key_msk = test_kvm_facility(vcpu->kvm, 76);
> +       int key_msk = scb_o->ecb3 & (ECB3_AES | ECB3_DEA);
>         int fmt_o = crycbd_o & CRYCB_FORMAT_MASK;
>         int fmt_h = vcpu->arch.sie_block->crycbd & CRYCB_FORMAT_MASK;
>         int ret = 0;
> 
> 
> So just define a mask a mask.
> I verify the functionality and test on Monday  and if in between it seems better to you so too I post the patch.

Can you spin a v2 with an improved patch description outlining the problem according
to this discussion?

Re: [PATCH v1] KVM: s390: vsie: fix Do the CRYCB validation first

[Pierre Morel]

On 04/02/2019 15:48, Christian Borntraeger wrote:
> 
> 
> On 01.02.2019 14:37, Pierre Morel wrote:
>> On 01/02/2019 11:56, David Hildenbrand wrote:
>>> On 01.02.19 10:52, Pierre Morel wrote:
>>>> The case when the SIE for guest3 is not setup for using
>>>> encryption keys nor Adjunct processor but the guest2
>>>> does use these features was not properly handled.
>>>>
>>>> This leads SIE entry for guest3 to crash with validity intercept
>>>> because the guest2, not having the use of encryption keys nor
>>>> Adjunct Processor did not initialize the CRYCB designation.
>>>>
>>>> In the case where none of ECA_APIE, ECB3_AES or ECB3_DEA
>>>> are set in guest3 a format 0 CRYCB is allowed for guest3
>>>> and the CRYCB designation in the SIE for guest3 is not checked
>>>> on SIE entry.
>>>>
>>>> Let's allow the CRYCD designation to be ignored when the
>>>> SIE for guest3 is not initialized for encryption key usage
>>>> nor AP.
>>>>
>>>> Fixup: d6f6959 (KVM: s390: vsie: Do the CRYCB validation first)
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> Reported-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>>>> ---
>>>>    arch/s390/kvm/vsie.c | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>>> index a153257..a748f76 100644
>>>> --- a/arch/s390/kvm/vsie.c
>>>> +++ b/arch/s390/kvm/vsie.c
>>>> @@ -300,6 +300,9 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>        if (!apie_h && !key_msk)
>>>>            return 0;
>>>>    +    if (!(scb_o->eca & ECA_APIE) && !(scb_o->ecb3 & (ECB3_AES | ECB3_DEA)))
>>>> +        return 0;
>>>> +
>>>>        if (!crycb_addr)
>>>>            return set_validity_icpt(scb_s, 0x0039U);
>>>>   
>>>
>>> The original patch said
>>>
>>> "We need to handle the validity checks for the crycb, no matter what the
>>>    settings for the keywrappings are. So lets move the keywrapping checks
>>>    after we have done the validy checks."
>>>
>>> Can you explain why keywrapping now is important? These patches seem to
>>> contradict.
>>>
>>
>> No it does not, having the flags set or not is part of the validity check.
>> but, I acted too fast.
>>
>> The problem seems to be even clearer:
>> key_msk is defined as
>>     int key_msk = test_kvm_facility(vcpu->kvm, 76);
>>
>> If it is defined, as it should for a mask, as
>>      (scb_o->ecb3 & (ECB3_AES | ECB3_DEA))
>>
>> all is clear..., key_msk is not use but for this test, so I do not understand why it is set as facility 76.
>>
>> so I think I better do:
>>
>>
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index a153257..30843a8 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -289,7 +289,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>          unsigned long *b1, *b2;
>>          u8 ecb3_flags;
>>          int apie_h;
>> -       int key_msk = test_kvm_facility(vcpu->kvm, 76);
>> +       int key_msk = scb_o->ecb3 & (ECB3_AES | ECB3_DEA);
>>          int fmt_o = crycbd_o & CRYCB_FORMAT_MASK;
>>          int fmt_h = vcpu->arch.sie_block->crycbd & CRYCB_FORMAT_MASK;
>>          int ret = 0;
>>
>>
>> So just define a mask a mask.
>> I verify the functionality and test on Monday  and if in between it seems better to you so too I post the patch.
> 
> Can you spin a v2 with an improved patch description outlining the problem according
> to this discussion?
> 
Yes I will do.
Seems I need more analyzing of the problem

Regards,
Pierre

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v1] KVM: s390: vsie: fix Do the CRYCB validation first

[David Hildenbrand]

On 01.02.19 14:37, Pierre Morel wrote:
> On 01/02/2019 11:56, David Hildenbrand wrote:
>> On 01.02.19 10:52, Pierre Morel wrote:
>>> The case when the SIE for guest3 is not setup for using
>>> encryption keys nor Adjunct processor but the guest2
>>> does use these features was not properly handled.
>>>
>>> This leads SIE entry for guest3 to crash with validity intercept
>>> because the guest2, not having the use of encryption keys nor
>>> Adjunct Processor did not initialize the CRYCB designation.
>>>
>>> In the case where none of ECA_APIE, ECB3_AES or ECB3_DEA
>>> are set in guest3 a format 0 CRYCB is allowed for guest3
>>> and the CRYCB designation in the SIE for guest3 is not checked
>>> on SIE entry.
>>>
>>> Let's allow the CRYCD designation to be ignored when the
>>> SIE for guest3 is not initialized for encryption key usage
>>> nor AP.
>>>
>>> Fixup: d6f6959 (KVM: s390: vsie: Do the CRYCB validation first)
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> Reported-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>>> ---
>>>   arch/s390/kvm/vsie.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>> index a153257..a748f76 100644
>>> --- a/arch/s390/kvm/vsie.c
>>> +++ b/arch/s390/kvm/vsie.c
>>> @@ -300,6 +300,9 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>   	if (!apie_h && !key_msk)
>>>   		return 0;
>>>   
>>> +	if (!(scb_o->eca & ECA_APIE) && !(scb_o->ecb3 & (ECB3_AES | ECB3_DEA)))
>>> +		return 0;
>>> +
>>>   	if (!crycb_addr)
>>>   		return set_validity_icpt(scb_s, 0x0039U);
>>>   
>>>
>>
>> The original patch said
>>
>> "We need to handle the validity checks for the crycb, no matter what the
>>   settings for the keywrappings are. So lets move the keywrapping checks
>>   after we have done the validy checks."
>>
>> Can you explain why keywrapping now is important? These patches seem to
>> contradict.
>>
> 
> No it does not, having the flags set or not is part of the validity check.
> but, I acted too fast.
> 
> The problem seems to be even clearer:
> key_msk is defined as
>     int key_msk = test_kvm_facility(vcpu->kvm, 76);
> 
> If it is defined, as it should for a mask, as
> 	(scb_o->ecb3 & (ECB3_AES | ECB3_DEA))
> 
> all is clear..., key_msk is not use but for this test, so I do not 
> understand why it is set as facility 76.
> 
> so I think I better do:
> 
> 
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index a153257..30843a8 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -289,7 +289,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, 
> struct vsie_page *vsie_page)
>          unsigned long *b1, *b2;
>          u8 ecb3_flags;
>          int apie_h;
> -       int key_msk = test_kvm_facility(vcpu->kvm, 76);
> +       int key_msk = scb_o->ecb3 & (ECB3_AES | ECB3_DEA);
>          int fmt_o = crycbd_o & CRYCB_FORMAT_MASK;
>          int fmt_h = vcpu->arch.sie_block->crycbd & CRYCB_FORMAT_MASK;
>          int ret = 0;
> 
> 
> So just define a mask a mask.
> I verify the functionality and test on Monday  and if in between it 
> seems better to you so too I post the patch.

Without documentation at hand I cannot really judge what is the right
approach. You have to read the architecture description very carefully
and thimk about the different scenarios. I cannot help here.

test_kvm_facility(vcpu->kvm, 76) means "hardware knows key wrapping exists"

scb_o->ecb3 & (ECB3_AES | ECB3_DEA) means "key wrapping was actually
enabled for AES or DEA"

> 
> Thanks,
> Pierre
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v1] KVM: s390: vsie: fix Do the CRYCB validation first

[Pierre Morel]

On 04/02/2019 16:15, David Hildenbrand wrote:
> On 01.02.19 14:37, Pierre Morel wrote:
>> On 01/02/2019 11:56, David Hildenbrand wrote:
>>> On 01.02.19 10:52, Pierre Morel wrote:
>>>> The case when the SIE for guest3 is not setup for using
>>>> encryption keys nor Adjunct processor but the guest2
>>>> does use these features was not properly handled.
>>>>
>>>> This leads SIE entry for guest3 to crash with validity intercept
>>>> because the guest2, not having the use of encryption keys nor
>>>> Adjunct Processor did not initialize the CRYCB designation.
>>>>
>>>> In the case where none of ECA_APIE, ECB3_AES or ECB3_DEA
>>>> are set in guest3 a format 0 CRYCB is allowed for guest3
>>>> and the CRYCB designation in the SIE for guest3 is not checked
>>>> on SIE entry.
>>>>
>>>> Let's allow the CRYCD designation to be ignored when the
>>>> SIE for guest3 is not initialized for encryption key usage
>>>> nor AP.
>>>>
>>>> Fixup: d6f6959 (KVM: s390: vsie: Do the CRYCB validation first)
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> Reported-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>>>> ---
>>>>    arch/s390/kvm/vsie.c | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>>> index a153257..a748f76 100644
>>>> --- a/arch/s390/kvm/vsie.c
>>>> +++ b/arch/s390/kvm/vsie.c
>>>> @@ -300,6 +300,9 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>    	if (!apie_h && !key_msk)
>>>>    		return 0;
>>>>    
>>>> +	if (!(scb_o->eca & ECA_APIE) && !(scb_o->ecb3 & (ECB3_AES | ECB3_DEA)))
>>>> +		return 0;
>>>> +
>>>>    	if (!crycb_addr)
>>>>    		return set_validity_icpt(scb_s, 0x0039U);
>>>>    
>>>>
>>>
>>> The original patch said
>>>
>>> "We need to handle the validity checks for the crycb, no matter what the
>>>    settings for the keywrappings are. So lets move the keywrapping checks
>>>    after we have done the validy checks."
>>>
>>> Can you explain why keywrapping now is important? These patches seem to
>>> contradict.
>>>
>>
>> No it does not, having the flags set or not is part of the validity check.
>> but, I acted too fast.
>>
>> The problem seems to be even clearer:
>> key_msk is defined as
>>      int key_msk = test_kvm_facility(vcpu->kvm, 76);
>>
>> If it is defined, as it should for a mask, as
>> 	(scb_o->ecb3 & (ECB3_AES | ECB3_DEA))
>>
>> all is clear..., key_msk is not use but for this test, so I do not
>> understand why it is set as facility 76.
>>
>> so I think I better do:
>>
>>
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index a153257..30843a8 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -289,7 +289,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu,
>> struct vsie_page *vsie_page)
>>           unsigned long *b1, *b2;
>>           u8 ecb3_flags;
>>           int apie_h;
>> -       int key_msk = test_kvm_facility(vcpu->kvm, 76);
>> +       int key_msk = scb_o->ecb3 & (ECB3_AES | ECB3_DEA);
>>           int fmt_o = crycbd_o & CRYCB_FORMAT_MASK;
>>           int fmt_h = vcpu->arch.sie_block->crycbd & CRYCB_FORMAT_MASK;
>>           int ret = 0;
>>
>>
>> So just define a mask a mask.
>> I verify the functionality and test on Monday  and if in between it
>> seems better to you so too I post the patch.
> 
> Without documentation at hand I cannot really judge what is the right
> approach. You have to read the architecture description very carefully
> and thimk about the different scenarios. I cannot help here.
> 
> test_kvm_facility(vcpu->kvm, 76) means "hardware knows key wrapping exists"
> 
> scb_o->ecb3 & (ECB3_AES | ECB3_DEA) means "key wrapping was actually
> enabled for AES or DEA"


Right.
We also must take care of the format.
So not sure my propositions are right.
And helas I have the documentation :)
So ... I have to read it again again.


Regards,
Pierre

> 
>>
>> Thanks,
>> Pierre
>>
> 
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany