Re: [PATCH v7 03/14] KVM: s390: Fix kvm_s390_vcpu_unsetup_cmma()

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

Pre-existing issues:
- [High] The `kvm_arch_vcpu_create` function leaks the MMU cache on error paths.
- [High] Memory leak of protected VCPU storage base (`stor_base`) and SIDA pages when `uv_call` fails in `kvm_s390_pv_create_cpu()`.
--

--- Patch [3]: [PATCH v7 03/14] KVM: s390: Fix kvm_s390_vcpu_unsetup_cmma() ---
commit a13468185076d541edaaa78721cdc3f5e4915351
Author: Claudio Imbrenda <[email protected]>
KVM: s390: Fix kvm_s390_vcpu_unsetup_cmma()

This patch fixes an issue in kvm_s390_vcpu_unsetup_cmma() where a 0
cbrlo could cause free_page() to attempt to free physical page 0 when
running with V != R. It adds a check to only free cbrlo if it is 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 regression introduced by this patch, but the kvm_arch_vcpu_create
function appears to leak the MMU cache on error paths:

arch/s390/kvm/kvm-s390.c:kvm_arch_vcpu_create() {
...
	if (kvm_is_ucontrol(vcpu->kvm)) {
		rc = -ENOMEM;
		vcpu->arch.gmap = gmap_new_child(vcpu->kvm->arch.gmap, -1UL);
		if (!vcpu->arch.gmap)
			goto out_free_sie_block;
	}
...
out_free_sie_block:
	free_page((unsigned long)(vcpu->arch.sie_block));
	return rc;
}

Can this leak vcpu->arch.mc? If vcpu creation fails after it is allocated,
the error path jumps to out_free_sie_block, but it doesn't seem to free
vcpu->arch.mc. Since kvm->created_vcpus is decremented upon failure,
could an unprivileged user repeatedly call KVM_CREATE_VCPU to trigger
failures and exhaust kernel memory?

[Severity: High]
This is a pre-existing regression, but there is also a potential memory leak
of the protected vcpu storage base (stor_base) and SIDA pages when
uv_call() fails:

arch/s390/kvm/pv.c:kvm_s390_pv_create_cpu() {
...
	cc = uv_call(0, (u64)&uvcb);
...
	if (cc) {
		u16 dummy;

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

arch/s390/kvm/pv.c:kvm_s390_pv_destroy_cpu() {
...
	if (!kvm_s390_pv_cpu_get_handle(vcpu))
		return 0;
...
}

Since the handle is only set after a successful uv_call(), does the early
return in kvm_s390_pv_destroy_cpu() silently skip freeing
vcpu->arch.pv.stor_base and the sidad page? Could this be reached by a
user repeatedly calling KVM_CREATE_VCPU on a protected VM to trigger
failures, leading to a host denial of service?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3