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@xxxxxxxxxxxxx>
> > > > Reported-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
> > > > ---
> > > >  Â 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