Re: [PATCH v5 02/14] KVM: s390: pv: avoid double free of sida page

Janosch Frank
2021-10-05T13:12:02Z

On 9/20/21 15:24, 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.
>
> This can happen, for example, if userspace calls the KVM_PV_DISABLE
> IOCTL, and it fails, and then userspace calls the same IOCTL again.
> This scenario is only possible if KVM has some serious bug or if the
> hardware is broken.
>
> The solution is to simply return successfully immediately if the vCPU
> was already non secure.
>
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Fixes: 19e1227768863a1469797c13ef8fea1af7beac2c ("KVM: S390: protvirt: Introduce instruction data area bounce buffer")

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

> ---
> 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,
>