Before adapting the CRYCB shadowing for a guest supporting
the AP instructions we want to clean the CRYCB shadowing code.
First patch seem obvious.
Second patch introduce a change in the behavior of
the virtual machine in that the CRYCB is validated
whenever we use or not the key masks to mimic the
real machine.
Patch 3 does not correct the compiled code but make
more clear what is done concerning the formating of
the CRYCB in the guest original CRYCB and in the
shadow CRYCB.
Pierre Morel (3):
KVM: s390: vsie: copy wrapping keys to right place
KVM: s390: vsie: Do the CRYCB validation first
KVM: s390: vsie: Make use of CRYCB FORMAT2 clear
arch/s390/kvm/vsie.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
--
2.7.4
Changelog from V2:
- Suppress two useless/broken patches
- Change a patch on CRYCB format to a modification of the comment
- modified the first patch title as expected
- added R-Bs
Changelog from v1
- split into severall small patches
- insert the first cleaning patch ahead
- add a new patch to allow a host not having AP
instructions
Copy the key mask to the right offset inside the shadow CRYCB
Signed-off-by: Pierre Morel <[email protected]>
Reviewed-by: David Hildenbrand <[email protected]>
Reviewed-by: Cornelia Huck <[email protected]>
Reviewed-by: Janosch Frank <[email protected]>
---
arch/s390/kvm/vsie.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 9175518..12b9707 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -173,7 +173,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
return set_validity_icpt(scb_s, 0x0039U);
/* copy only the wrapping keys */
- if (read_guest_real(vcpu, crycb_addr + 72, &vsie_page->crycb, 56))
+ 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;
--
2.7.4
The comment preceding the shadow_crycb function is
misleading, we effectively accept FORMAT2 CRYCB in the
guest.
When using FORMAT2 in the host we do not need to or with
FORMAT1.
Signed-off-by: Pierre Morel <[email protected]>
---
arch/s390/kvm/vsie.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 38ea5da..e0e6fbf 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -140,7 +140,8 @@ static int prepare_cpuflags(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
* Create a shadow copy of the crycb block and setup key wrapping, if
* requested for guest 3 and enabled for guest 2.
*
- * We only accept format-1 (no AP in g2), but convert it into format-2
+ * We accept format-1 or format-2, but we treat it as a format-1 (no AP in g2),
+ * and we convert it into format-2 in the shadow CRYCB.
* There is nothing to do for format-0.
*
* Returns: - 0 if shadowed or nothing to do
@@ -179,8 +180,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
return set_validity_icpt(scb_s, 0x0035U);
scb_s->ecb3 |= ecb3_flags;
- scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT1 |
- CRYCB_FORMAT2;
+ 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;
--
2.7.4
When entering the SIE the CRYCB validation better
be done independently of the instruction's
availability.
Signed-off-by: Pierre Morel <[email protected]>
---
arch/s390/kvm/vsie.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 12b9707..38ea5da 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -161,17 +161,18 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
/* format-1 is supported with message-security-assist extension 3 */
if (!test_kvm_facility(vcpu->kvm, 76))
return 0;
- /* 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);
+ /* 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;
+
/* copy only the wrapping keys */
if (read_guest_real(vcpu, crycb_addr + 72,
vsie_page->crycb.dea_wrapping_key_mask, 56))
--
2.7.4
On 8/23/18 12:25 PM, Pierre Morel wrote:
> The comment preceding the shadow_crycb function is
> misleading, we effectively accept FORMAT2 CRYCB in the
> guest.
I beg to differ:
if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
return 0;
>
> When using FORMAT2 in the host we do not need to or with
> FORMAT1.
>
> Signed-off-by: Pierre Morel <[email protected]>
> ---
> arch/s390/kvm/vsie.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 38ea5da..e0e6fbf 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -140,7 +140,8 @@ static int prepare_cpuflags(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> * Create a shadow copy of the crycb block and setup key wrapping, if
> * requested for guest 3 and enabled for guest 2.
> *
> - * We only accept format-1 (no AP in g2), but convert it into format-2
> + * We accept format-1 or format-2, but we treat it as a format-1 (no AP in g2),
> + * and we convert it into format-2 in the shadow CRYCB.
> * There is nothing to do for format-0.
> *
> * Returns: - 0 if shadowed or nothing to do
> @@ -179,8 +180,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> return set_validity_icpt(scb_s, 0x0035U);
>
> scb_s->ecb3 |= ecb3_flags;
> - scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT1 |
> - CRYCB_FORMAT2;
> + scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT2;
That's purely cosmetic but valid.
>
> /* xor both blocks in one run */
> b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask;
>
On 08/23/2018 12:25 PM, Pierre Morel wrote:
> Copy the key mask to the right offset inside the shadow CRYCB
>
> Signed-off-by: Pierre Morel <[email protected]>
> Reviewed-by: David Hildenbrand <[email protected]>
> Reviewed-by: Cornelia Huck <[email protected]>
> Reviewed-by: Janosch Frank <[email protected]>
This should be cc stable I guess?
> ---
> arch/s390/kvm/vsie.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 9175518..12b9707 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -173,7 +173,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> return set_validity_icpt(scb_s, 0x0039U);
>
> /* copy only the wrapping keys */
> - if (read_guest_real(vcpu, crycb_addr + 72, &vsie_page->crycb, 56))
> + 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;
>
On 08/23/2018 12:25 PM, Pierre Morel wrote:
> When entering the SIE the CRYCB validation better
> be done independently of the instruction's
> availability.
Maybe something like
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.
?
>
> Signed-off-by: Pierre Morel <[email protected]>
> ---
> arch/s390/kvm/vsie.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 12b9707..38ea5da 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -161,17 +161,18 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> /* format-1 is supported with message-security-assist extension 3 */
> if (!test_kvm_facility(vcpu->kvm, 76))
> return 0;
> - /* 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);
>
> + /* 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;
> +
> /* copy only the wrapping keys */
> if (read_guest_real(vcpu, crycb_addr + 72,
> vsie_page->crycb.dea_wrapping_key_mask, 56))
>
On 23.08.2018 13:17, Christian Borntraeger wrote:
>
>
> On 08/23/2018 12:25 PM, Pierre Morel wrote:
>> When entering the SIE the CRYCB validation better
>> be done independently of the instruction's
>> availability.
>
> Maybe something like
>
> 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.
>
> ?
With that
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On 23.08.2018 13:05, Janosch Frank wrote:
> On 8/23/18 12:25 PM, Pierre Morel wrote:
>> The comment preceding the shadow_crycb function is
>> misleading, we effectively accept FORMAT2 CRYCB in the
>> guest.
>
> I beg to differ:
>
> if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
> return 0;
FORMAT2 includes bit FORMAT1 (backwards compatible)
>
>>
>> When using FORMAT2 in the host we do not need to or with
>> FORMAT1.
>>
>> Signed-off-by: Pierre Morel <[email protected]>
>> ---
>> arch/s390/kvm/vsie.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index 38ea5da..e0e6fbf 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -140,7 +140,8 @@ static int prepare_cpuflags(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>> * Create a shadow copy of the crycb block and setup key wrapping, if
>> * requested for guest 3 and enabled for guest 2.
>> *
>> - * We only accept format-1 (no AP in g2), but convert it into format-2
>> + * We accept format-1 or format-2, but we treat it as a format-1 (no AP in g2),
>> + * and we convert it into format-2 in the shadow CRYCB.
>> * There is nothing to do for format-0.
>> *
>> * Returns: - 0 if shadowed or nothing to do
>> @@ -179,8 +180,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>> return set_validity_icpt(scb_s, 0x0035U);
>>
>> scb_s->ecb3 |= ecb3_flags;
>> - scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT1 |
>> - CRYCB_FORMAT2;
>> + scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT2;
>
> That's purely cosmetic but valid.
>
>>
>> /* xor both blocks in one run */
>> b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask;
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On 23.08.2018 13:07, Christian Borntraeger wrote:
>
>
> On 08/23/2018 12:25 PM, Pierre Morel wrote:
>> Copy the key mask to the right offset inside the shadow CRYCB
>>
>> Signed-off-by: Pierre Morel <[email protected]>
>> Reviewed-by: David Hildenbrand <[email protected]>
>> Reviewed-by: Cornelia Huck <[email protected]>
>> Reviewed-by: Janosch Frank <[email protected]>
>
> This should be cc stable I guess?
>
Yes, g3 guests will be run with same wrapping keys.
--
Thanks,
David / dhildenb
On 8/23/18 1:21 PM, David Hildenbrand wrote:
> On 23.08.2018 13:05, Janosch Frank wrote:
>> On 8/23/18 12:25 PM, Pierre Morel wrote:
>>> The comment preceding the shadow_crycb function is
>>> misleading, we effectively accept FORMAT2 CRYCB in the
>>> guest.
>>
>> I beg to differ:
>>
>> if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
>> return 0;
>
> FORMAT2 includes bit FORMAT1 (backwards compatible)
Right, this check is very misleading because of the constant, we
effectively test against Format 0 and Format 2.
Can we make this clearer by explicitly ANDing 0x01 or adding a comment?
Code makes sense:
Reviewed-by: Janosch Frank <[email protected]>
>
>>
>>>
>>> When using FORMAT2 in the host we do not need to or with
>>> FORMAT1.
>>>
>>> Signed-off-by: Pierre Morel <[email protected]>
>>> ---
>>> arch/s390/kvm/vsie.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>> index 38ea5da..e0e6fbf 100644
>>> --- a/arch/s390/kvm/vsie.c
>>> +++ b/arch/s390/kvm/vsie.c
>>> @@ -140,7 +140,8 @@ static int prepare_cpuflags(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>> * Create a shadow copy of the crycb block and setup key wrapping, if
>>> * requested for guest 3 and enabled for guest 2.
>>> *
>>> - * We only accept format-1 (no AP in g2), but convert it into format-2
>>> + * We accept format-1 or format-2, but we treat it as a format-1 (no AP in g2),
>>> + * and we convert it into format-2 in the shadow CRYCB.
>>> * There is nothing to do for format-0.
>>> *
>>> * Returns: - 0 if shadowed or nothing to do
>>> @@ -179,8 +180,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>> return set_validity_icpt(scb_s, 0x0035U);
>>>
>>> scb_s->ecb3 |= ecb3_flags;
>>> - scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT1 |
>>> - CRYCB_FORMAT2;
>>> + scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT2;
>>
>> That's purely cosmetic but valid.
>>
>>>
>>> /* xor both blocks in one run */
>>> b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask;
>
> Reviewed-by: David Hildenbrand <[email protected]>
>
On 23/08/2018 13:17, Christian Borntraeger wrote:
>
>
> On 08/23/2018 12:25 PM, Pierre Morel wrote:
>> When entering the SIE the CRYCB validation better
>> be done independently of the instruction's
>> availability.
>
> Maybe something like
>
> 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.
>
> ?
OK thanks, I use this.
regards,
Pierre
>
>>
>> Signed-off-by: Pierre Morel <[email protected]>
>> ---
>> arch/s390/kvm/vsie.c | 11 ++++++-----
>> 1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index 12b9707..38ea5da 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -161,17 +161,18 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>> /* format-1 is supported with message-security-assist extension 3 */
>> if (!test_kvm_facility(vcpu->kvm, 76))
>> return 0;
>> - /* 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);
>>
>> + /* 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;
>> +
>> /* copy only the wrapping keys */
>> if (read_guest_real(vcpu, crycb_addr + 72,
>> vsie_page->crycb.dea_wrapping_key_mask, 56))
>>
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
On 23/08/2018 13:21, David Hildenbrand wrote:
> On 23.08.2018 13:05, Janosch Frank wrote:
>> On 8/23/18 12:25 PM, Pierre Morel wrote:
>>> The comment preceding the shadow_crycb function is
>>> misleading, we effectively accept FORMAT2 CRYCB in the
>>> guest.
>>
>> I beg to differ:
>>
>> if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
>> return 0;
>
> FORMAT2 includes bit FORMAT1 (backwards compatible)
>
>>
>>>
>>> When using FORMAT2 in the host we do not need to or with
>>> FORMAT1.
>>>
>>> Signed-off-by: Pierre Morel <[email protected]>
>>> ---
>>> arch/s390/kvm/vsie.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>> index 38ea5da..e0e6fbf 100644
>>> --- a/arch/s390/kvm/vsie.c
>>> +++ b/arch/s390/kvm/vsie.c
>>> @@ -140,7 +140,8 @@ static int prepare_cpuflags(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>> * Create a shadow copy of the crycb block and setup key wrapping, if
>>> * requested for guest 3 and enabled for guest 2.
>>> *
>>> - * We only accept format-1 (no AP in g2), but convert it into format-2
>>> + * We accept format-1 or format-2, but we treat it as a format-1 (no AP in g2),
>>> + * and we convert it into format-2 in the shadow CRYCB.
>>> * There is nothing to do for format-0.
>>> *
>>> * Returns: - 0 if shadowed or nothing to do
>>> @@ -179,8 +180,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>> return set_validity_icpt(scb_s, 0x0035U);
>>>
>>> scb_s->ecb3 |= ecb3_flags;
>>> - scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT1 |
>>> - CRYCB_FORMAT2;
>>> + scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT2;
>>
>> That's purely cosmetic but valid.
>>
>>>
>>> /* xor both blocks in one run */
>>> b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask;
>
> Reviewed-by: David Hildenbrand <[email protected]>
>
Thanks,
regards,
Pierre
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
On 23/08/2018 13:19, David Hildenbrand wrote:
> On 23.08.2018 13:17, Christian Borntraeger wrote:
>>
>>
>> On 08/23/2018 12:25 PM, Pierre Morel wrote:
>>> When entering the SIE the CRYCB validation better
>>> be done independently of the instruction's
>>> availability.
>>
>> Maybe something like
>>
>> 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.
>>
>> ?
>
> With that
>
> Reviewed-by: David Hildenbrand <[email protected]>
>
>
Thanks, I use the description proposed by Christian.
regards,
Pierre
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
On 23/08/2018 13:33, Janosch Frank wrote:
> On 8/23/18 1:21 PM, David Hildenbrand wrote:
>> On 23.08.2018 13:05, Janosch Frank wrote:
>>> On 8/23/18 12:25 PM, Pierre Morel wrote:
>>>> The comment preceding the shadow_crycb function is
>>>> misleading, we effectively accept FORMAT2 CRYCB in the
>>>> guest.
>>>
>>> I beg to differ:
>>>
>>> if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
>>> return 0;
>>
>> FORMAT2 includes bit FORMAT1 (backwards compatible)
>
> Right, this check is very misleading because of the constant, we
> effectively test against Format 0 and Format 2.
>
> Can we make this clearer by explicitly ANDing 0x01 or adding a comment?
yes, done, I modified the comment in front of the function.
>
> Code makes sense:
> Reviewed-by: Janosch Frank <[email protected]>
Thanks,
regards,
Pierre
>
>>
>>>
>>>>
>>>> When using FORMAT2 in the host we do not need to or with
>>>> FORMAT1.
>>>>
>>>> Signed-off-by: Pierre Morel <[email protected]>
>>>> ---
>>>> arch/s390/kvm/vsie.c | 6 +++---
>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>>> index 38ea5da..e0e6fbf 100644
>>>> --- a/arch/s390/kvm/vsie.c
>>>> +++ b/arch/s390/kvm/vsie.c
>>>> @@ -140,7 +140,8 @@ static int prepare_cpuflags(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>> * Create a shadow copy of the crycb block and setup key wrapping, if
>>>> * requested for guest 3 and enabled for guest 2.
>>>> *
>>>> - * We only accept format-1 (no AP in g2), but convert it into format-2
>>>> + * We accept format-1 or format-2, but we treat it as a format-1 (no AP in g2),
>>>> + * and we convert it into format-2 in the shadow CRYCB.
>>>> * There is nothing to do for format-0.
>>>> *
>>>> * Returns: - 0 if shadowed or nothing to do
>>>> @@ -179,8 +180,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>> return set_validity_icpt(scb_s, 0x0035U);
>>>>
>>>> scb_s->ecb3 |= ecb3_flags;
>>>> - scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT1 |
>>>> - CRYCB_FORMAT2;
>>>> + scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT2;
>>>
>>> That's purely cosmetic but valid.
>>>
>>>>
>>>> /* xor both blocks in one run */
>>>> b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask;
>>
>> Reviewed-by: David Hildenbrand <[email protected]>
>>
>
>
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
On 8/23/18 1:47 PM, Pierre Morel wrote:
> On 23/08/2018 13:33, Janosch Frank wrote:
>> On 8/23/18 1:21 PM, David Hildenbrand wrote:
>>> On 23.08.2018 13:05, Janosch Frank wrote:
>>>> On 8/23/18 12:25 PM, Pierre Morel wrote:
>>>>> The comment preceding the shadow_crycb function is
>>>>> misleading, we effectively accept FORMAT2 CRYCB in the
>>>>> guest.
>>>>
>>>> I beg to differ:
>>>>
>>>> if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
>>>> return 0;
>>>
>>> FORMAT2 includes bit FORMAT1 (backwards compatible)
>>
>> Right, this check is very misleading because of the constant, we
>> effectively test against Format 0 and Format 2.
>>
>> Can we make this clearer by explicitly ANDing 0x01 or adding a comment?
>
> yes, done, I modified the comment in front of the function.
Which is not what I want, what I want is:
/* CRYCB_FORMAT2 includes the bit for CRYCB_FORMAT1, so we allow both
formats here */
if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
return 0;
>
>>
>> Code makes sense:
>> Reviewed-by: Janosch Frank <[email protected]>
>
> Thanks,
>
> regards,
> Pierre
>
>>
>>>
>>>>
>>>>>
>>>>> When using FORMAT2 in the host we do not need to or with
>>>>> FORMAT1.
>>>>>
>>>>> Signed-off-by: Pierre Morel <[email protected]>
>>>>> ---
>>>>> arch/s390/kvm/vsie.c | 6 +++---
>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>>>> index 38ea5da..e0e6fbf 100644
>>>>> --- a/arch/s390/kvm/vsie.c
>>>>> +++ b/arch/s390/kvm/vsie.c
>>>>> @@ -140,7 +140,8 @@ static int prepare_cpuflags(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>> * Create a shadow copy of the crycb block and setup key wrapping, if
>>>>> * requested for guest 3 and enabled for guest 2.
>>>>> *
>>>>> - * We only accept format-1 (no AP in g2), but convert it into format-2
>>>>> + * We accept format-1 or format-2, but we treat it as a format-1 (no AP in g2),
>>>>> + * and we convert it into format-2 in the shadow CRYCB.
>>>>> * There is nothing to do for format-0.
>>>>> *
>>>>> * Returns: - 0 if shadowed or nothing to do
>>>>> @@ -179,8 +180,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>> return set_validity_icpt(scb_s, 0x0035U);
>>>>>
>>>>> scb_s->ecb3 |= ecb3_flags;
>>>>> - scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT1 |
>>>>> - CRYCB_FORMAT2;
>>>>> + scb_s->crycbd = ((__u32)(__u64) &vsie_page->crycb) | CRYCB_FORMAT2;
>>>>
>>>> That's purely cosmetic but valid.
>>>>
>>>>>
>>>>> /* xor both blocks in one run */
>>>>> b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask;
>>>
>>> Reviewed-by: David Hildenbrand <[email protected]>
>>>
>>
>>
>
>
On 23.08.2018 13:53, Janosch Frank wrote:
> On 8/23/18 1:47 PM, Pierre Morel wrote:
>> On 23/08/2018 13:33, Janosch Frank wrote:
>>> On 8/23/18 1:21 PM, David Hildenbrand wrote:
>>>> On 23.08.2018 13:05, Janosch Frank wrote:
>>>>> On 8/23/18 12:25 PM, Pierre Morel wrote:
>>>>>> The comment preceding the shadow_crycb function is
>>>>>> misleading, we effectively accept FORMAT2 CRYCB in the
>>>>>> guest.
>>>>>
>>>>> I beg to differ:
>>>>>
>>>>> if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
>>>>> return 0;
>>>>
>>>> FORMAT2 includes bit FORMAT1 (backwards compatible)
>>>
>>> Right, this check is very misleading because of the constant, we
>>> effectively test against Format 0 and Format 2.
>>>
>>> Can we make this clearer by explicitly ANDing 0x01 or adding a comment?
>>
>> yes, done, I modified the comment in front of the function.
>
> Which is not what I want, what I want is:
>
> /* CRYCB_FORMAT2 includes the bit for CRYCB_FORMAT1, so we allow both
> formats here */
> if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
> return 0;
While it's not wrong, it is also not required. And it might soon be
obsolete again (with APXA, as you said, there we always have to check).
But I'll leave that to you
--
Thanks,
David / dhildenb
On 8/23/18 2:03 PM, David Hildenbrand wrote:
> On 23.08.2018 13:53, Janosch Frank wrote:
>> On 8/23/18 1:47 PM, Pierre Morel wrote:
>>> On 23/08/2018 13:33, Janosch Frank wrote:
>>>> On 8/23/18 1:21 PM, David Hildenbrand wrote:
>>>>> On 23.08.2018 13:05, Janosch Frank wrote:
>>>>>> On 8/23/18 12:25 PM, Pierre Morel wrote:
>>>>>>> The comment preceding the shadow_crycb function is
>>>>>>> misleading, we effectively accept FORMAT2 CRYCB in the
>>>>>>> guest.
>>>>>>
>>>>>> I beg to differ:
>>>>>>
>>>>>> if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
>>>>>> return 0;
>>>>>
>>>>> FORMAT2 includes bit FORMAT1 (backwards compatible)
>>>>
>>>> Right, this check is very misleading because of the constant, we
>>>> effectively test against Format 0 and Format 2.
>>>>
>>>> Can we make this clearer by explicitly ANDing 0x01 or adding a comment?
>>>
>>> yes, done, I modified the comment in front of the function.
>>
>> Which is not what I want, what I want is:
>>
>> /* CRYCB_FORMAT2 includes the bit for CRYCB_FORMAT1, so we allow both
>> formats here */
>> if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
>> return 0;
>
> While it's not wrong, it is also not required. And it might soon be
> obsolete again (with APXA, as you said, there we always have to check).
>
> But I'll leave that to you
>
I have not checked the vfio-ap patches, Pierre just told me that it goes
away in a few weeks anyway, so let's leave it out.
On 23/08/2018 14:43, Christian Borntraeger wrote:
>
>
> On 08/23/2018 01:41 PM, Pierre Morel wrote:
>> On 23/08/2018 13:19, David Hildenbrand wrote:
>>> On 23.08.2018 13:07, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 08/23/2018 12:25 PM, Pierre Morel wrote:
>>>>> Copy the key mask to the right offset inside the shadow CRYCB
>>>>>
>>>>> Signed-off-by: Pierre Morel <[email protected]>
>>>>> Reviewed-by: David Hildenbrand <[email protected]>
>>>>> Reviewed-by: Cornelia Huck <[email protected]>
>>>>> Reviewed-by: Janosch Frank <[email protected]>
>>>>
>>>> This should be cc stable I guess?
>>>>
>>>
>>> Yes, g3 guests will be run with same wrapping keys.
>>>
>>
>> OK I do send it separated from the other patches.
>
> No need to do that. I can add that when picking up.
>
OK
Thank
Pierre
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
On 08/23/2018 01:41 PM, Pierre Morel wrote:
> On 23/08/2018 13:19, David Hildenbrand wrote:
>> On 23.08.2018 13:07, Christian Borntraeger wrote:
>>>
>>>
>>> On 08/23/2018 12:25 PM, Pierre Morel wrote:
>>>> Copy the key mask to the right offset inside the shadow CRYCB
>>>>
>>>> Signed-off-by: Pierre Morel <[email protected]>
>>>> Reviewed-by: David Hildenbrand <[email protected]>
>>>> Reviewed-by: Cornelia Huck <[email protected]>
>>>> Reviewed-by: Janosch Frank <[email protected]>
>>>
>>> This should be cc stable I guess?
>>>
>>
>> Yes, g3 guests will be run with same wrapping keys.
>>
>
> OK I do send it separated from the other patches.
No need to do that. I can add that when picking up.
On 08/23/2018 12:25 PM, Pierre Morel wrote:
> Copy the key mask to the right offset inside the shadow CRYCB
>
> Signed-off-by: Pierre Morel <[email protected]>
> Reviewed-by: David Hildenbrand <[email protected]>
> Reviewed-by: Cornelia Huck <[email protected]>
> Reviewed-by: Janosch Frank <[email protected]>
> ---
> arch/s390/kvm/vsie.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 9175518..12b9707 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -173,7 +173,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> return set_validity_icpt(scb_s, 0x0039U);
>
> /* copy only the wrapping keys */
> - if (read_guest_real(vcpu, crycb_addr + 72, &vsie_page->crycb, 56))
> + if (read_guest_real(vcpu, crycb_addr + 72,
> + vsie_page->crycb.dea_wrapping_key_mask, 56))
When we fix that code.., dont we have to XOR the guest3 wrapping keys with
the guest2 ones?
> return set_validity_icpt(scb_s, 0x0035U);
>
> scb_s->ecb3 |= ecb3_flags;
>
On 8/23/18 12:25 PM, Pierre Morel wrote:
> Before adapting the CRYCB shadowing for a guest supporting
> the AP instructions we want to clean the CRYCB shadowing code.
>
> First patch seem obvious.
>
> Second patch introduce a change in the behavior of
> the virtual machine in that the CRYCB is validated
> whenever we use or not the key masks to mimic the
> real machine.
>
> Patch 3 does not correct the compiled code but make
> more clear what is done concerning the formating of
> the CRYCB in the guest original CRYCB and in the
> shadow CRYCB.
>
Thanks, applied.
On 23.08.2018 15:12, Christian Borntraeger wrote:
>
>
> On 08/23/2018 12:25 PM, Pierre Morel wrote:
>> Copy the key mask to the right offset inside the shadow CRYCB
>>
>> Signed-off-by: Pierre Morel <[email protected]>
>> Reviewed-by: David Hildenbrand <[email protected]>
>> Reviewed-by: Cornelia Huck <[email protected]>
>> Reviewed-by: Janosch Frank <[email protected]>
>> ---
>> arch/s390/kvm/vsie.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index 9175518..12b9707 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -173,7 +173,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>> return set_validity_icpt(scb_s, 0x0039U);
>>
>> /* copy only the wrapping keys */
>> - if (read_guest_real(vcpu, crycb_addr + 72, &vsie_page->crycb, 56))
>> + if (read_guest_real(vcpu, crycb_addr + 72,
>> + vsie_page->crycb.dea_wrapping_key_mask, 56))
>
> When we fix that code.., dont we have to XOR the guest3 wrapping keys with
> the guest2 ones?
That's done further down in that function.
>
>
>> return set_validity_icpt(scb_s, 0x0035U);
>>
>> scb_s->ecb3 |= ecb3_flags;
>>
>
--
Thanks,
David / dhildenb
On 8/23/18 12:25 PM, Pierre Morel wrote:
> When entering the SIE the CRYCB validation better
> be done independently of the instruction's
> availability.
>
> Signed-off-by: Pierre Morel <[email protected]>
> ---
> arch/s390/kvm/vsie.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 12b9707..38ea5da 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -161,17 +161,18 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> /* format-1 is supported with message-security-assist extension 3 */
> if (!test_kvm_facility(vcpu->kvm, 76))
> return 0;
> - /* 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);
>
> + /* 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;
> +
> /* copy only the wrapping keys */
> if (read_guest_real(vcpu, crycb_addr + 72,
> vsie_page->crycb.dea_wrapping_key_mask, 56))
>
I seemed to have forgotten to add this while being preoccupied with the
search for the validity discussion in #3.
Reviewed-by: Janosch Frank <[email protected]>
On 23/08/2018 13:19, David Hildenbrand wrote:
> On 23.08.2018 13:07, Christian Borntraeger wrote:
>>
>>
>> On 08/23/2018 12:25 PM, Pierre Morel wrote:
>>> Copy the key mask to the right offset inside the shadow CRYCB
>>>
>>> Signed-off-by: Pierre Morel <[email protected]>
>>> Reviewed-by: David Hildenbrand <[email protected]>
>>> Reviewed-by: Cornelia Huck <[email protected]>
>>> Reviewed-by: Janosch Frank <[email protected]>
>>
>> This should be cc stable I guess?
>>
>
> Yes, g3 guests will be run with same wrapping keys.
>
OK I do send it separated from the other patches.
regard,
Pierre
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany