On Tue, 31 Aug 2021 15:55:07 +0200
Christian Borntraeger <borntraeger@xxxxxxxxxx> wrote:
> On 18.08.21 15:26, Claudio Imbrenda wrote:
> > If kvm_s390_pv_destroy_cpu is called more than once, we risk calling
> > free_page on a random page, since the sidad field is aliased with the
> > gbea, which is not guaranteed to be zero.
> >
> > The solution is to simply return successfully immediately if the vCPU
> > was already non secure.
> >
> > Signed-off-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
> > Fixes: 19e1227768863a1469797c13ef8fea1af7beac2c ("KVM: S390: protvirt: Introduce instruction data area bounce buffer")
>
> Patch looks good. Do we have any potential case where we call this twice? In other words,
> do we need the Fixes tag with the code as of today or not?
I think so.
if QEMU calls KVM_PV_DISABLE, and it fails, some VCPUs might have been
made non secure, but the VM itself still counts as secure. QEMU can
then call KVM_PV_DISABLE again, which will try to convert all VCPUs to
non secure again, triggering this bug.
this scenario will not happen in practice (unless the hardware is
broken)
> > ---
> > arch/s390/kvm/pv.c | 19 +++++++++----------
> > 1 file changed, 9 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
> > index c8841f476e91..0a854115100b 100644
> > --- a/arch/s390/kvm/pv.c
> > +++ b/arch/s390/kvm/pv.c
> > @@ -16,18 +16,17 @@
> >
> > int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc)
> > {
> > - int cc = 0;
> > + int cc;
> >
> > - if (kvm_s390_pv_cpu_get_handle(vcpu)) {
> > - cc = uv_cmd_nodata(kvm_s390_pv_cpu_get_handle(vcpu),
> > - UVC_CMD_DESTROY_SEC_CPU, rc, rrc);
> > + if (!kvm_s390_pv_cpu_get_handle(vcpu))
> > + return 0;
> > +
> > + cc = uv_cmd_nodata(kvm_s390_pv_cpu_get_handle(vcpu), UVC_CMD_DESTROY_SEC_CPU, rc, rrc);
> > +
> > + KVM_UV_EVENT(vcpu->kvm, 3, "PROTVIRT DESTROY VCPU %d: rc %x rrc %x",
> > + vcpu->vcpu_id, *rc, *rrc);
> > + WARN_ONCE(cc, "protvirt destroy cpu failed rc %x rrc %x", *rc, *rrc);
> >
> > - KVM_UV_EVENT(vcpu->kvm, 3,
> > - "PROTVIRT DESTROY VCPU %d: rc %x rrc %x",
> > - vcpu->vcpu_id, *rc, *rrc);
> > - WARN_ONCE(cc, "protvirt destroy cpu failed rc %x rrc %x",
> > - *rc, *rrc);
> > - }
> > /* Intended memory leak for something that should never happen. */
> > if (!cc)
> > free_pages(vcpu->arch.pv.stor_base,
> >
