Re: [PATCH v8 03/13] KVM: s390: Fix kvm_s390_vcpu_unsetup_cmma()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The MMU cache (`vcpu->arch.mc`) is permanently leaked when `kvm_arch_vcpu_create()` fails during vCPU initialization.
- [High] Protected vCPU creation failure leaks `pv.stor_base` and `sidad` pages.
--

--- Patch [3]: [PATCH v8 03/13] KVM: s390: Fix kvm_s390_vcpu_unsetup_cmma() ---
commit 17f441be24aaabde7dd38b196f4dba98cf6e898d
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix kvm_s390_vcpu_unsetup_cmma()

In some cases kvm_s390_vcpu_unsetup_cmma() can be called with a 0
cbrlo; in such cases, if running with V != R, free_page() will attempt
to free physical page 0.

Fix by freeing cbrlo only if it's non-zero.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 1b3290a5ad1ae..9be27db0a21e6 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3678,7 +3678,8 @@ static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu)
>  
>  void kvm_s390_vcpu_unsetup_cmma(struct kvm_vcpu *vcpu)
>  {
> -	free_page((unsigned long)phys_to_virt(vcpu->arch.sie_block->cbrlo));
> +	if (vcpu->arch.sie_block->cbrlo)
> +		free_page((unsigned long)phys_to_virt(vcpu->arch.sie_block->cbrlo));
>  	vcpu->arch.sie_block->cbrlo = 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but it looks like the MMU cache
(vcpu->arch.mc) might be permanently leaked when kvm_arch_vcpu_create() fails
during vCPU initialization.

kvm_arch_vcpu_create() allocates memory for vcpu->arch.mc via
kvm_s390_new_mmu_cache(). If a subsequent initialization step fails, the error
paths free vcpu->arch.gmap and vcpu->arch.sie_block, but appear to omit
calling kvm_s390_free_mmu_cache(vcpu->arch.mc):

arch/s390/kvm/kvm-s390.c:kvm_arch_vcpu_create() {
    ...
out_ucontrol_uninit:
	if (kvm_is_ucontrol(vcpu->kvm)) {
		gmap_remove_child(vcpu->arch.gmap);
		vcpu->arch.gmap = gmap_put(vcpu->arch.gmap);
	}
out_free_sie_block:
	free_page((unsigned long)(vcpu->arch.sie_block));
	return rc;
}

Since the KVM core does not call kvm_arch_vcpu_destroy() when
kvm_arch_vcpu_create() returns an error, could this leak the MMU cache pages
and allow a local user to exhaust kernel memory?


[Severity: High]
This is a pre-existing issue, but I noticed another potential leak in
kvm_s390_pv_create_cpu().

When creating a protected vCPU, pages are allocated for
vcpu->arch.pv.stor_base and the sida_addr. If the ultravisor call uv_call()
fails, the code attempts cleanup using kvm_s390_pv_destroy_cpu():

arch/s390/kvm/pv.c:kvm_s390_pv_create_cpu() {
    ...
	if (cc) {
		u16 dummy;

		kvm_s390_pv_destroy_cpu(vcpu, &dummy, &dummy);
		return -EIO;
	}

	/* Output */
	vcpu->arch.pv.handle = uvcb.cpu_handle;
    ...
}

However, kvm_s390_pv_destroy_cpu() begins with a check for the handle:

    if (!kvm_s390_pv_cpu_get_handle(vcpu))
        return 0;

Since vcpu->arch.pv.handle is strictly populated after a successful uv_call(),
doesn't it evaluate to 0 during a failure?

Does this mean kvm_s390_pv_destroy_cpu() will return immediately without
freeing the stor_base and sidad pages, allowing a local user to repeatedly
fail protected vCPU creation and exhaust kernel memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803124040.126471-1-imbrenda@linux.ibm.com?part=3