Re: [PATCH v6 02/14] KVM: s390: Fix kvm_s390_vcpu_unsetup_cmma()

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

Pre-existing issues:
- [Critical] Unprivileged local memory corruption via `sca_dispose(NULL)` leading to freeing physical pages 1, 2, and 3.
- [High] Memory leak of `vcpu->arch.mc` in the error path of `kvm_arch_vcpu_create()`.
- [High] Missing lock acquisition when removing a child gmap in the vCPU creation error path.
- [High] When `uv_call` fails during `kvm_s390_pv_create_cpu()`, newly allocated `stor_base` and `sida_addr` pages are permanently leaked.
--

--- Patch [2]: [PATCH v6 02/14] KVM: s390: Fix kvm_s390_vcpu_unsetup_cmma() ---
commit b2abac5b0352ced2b250d10ad5dfc60b18cae87c
Author: Claudio Imbrenda <[email protected]>

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.

Fixes: b31605c12f4e ("KVM: s390: make cmma usage conditionally")
Fixes: 29b40f105ec8 ("KVM: s390: protvirt: Add initial vm and cpu lifecycle handling")

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index f86b4b0b356f4..9ec5833a09732 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3677,7 +3677,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;
>  }

While reviewing this code, a few other things were noticed nearby.

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);

[Severity: High]
This is a pre-existing issue, but is it possible we are missing a lock here?

In kvm_arch_vcpu_create(), if a ucontrol VM's vCPU setup fails, it calls
gmap_remove_child(vcpu->arch.gmap).

The gmap_remove_child() function expects the parent gmap's children_lock to
be held and concurrently modifies the parent gmap's children list. However,
this error path appears to omit acquiring the children_lock.

Could this lead to a data race and list corruption?


		vcpu->arch.gmap = gmap_put(vcpu->arch.gmap);
	}
out_free_sie_block:
	free_page((unsigned long)(vcpu->arch.sie_block));
	return rc;
}

[Severity: High]
This is a pre-existing issue, but what happens to the vcpu->arch.mc memory
when we take this error path?

When an error occurs in kvm_arch_vcpu_create() after vcpu->arch.mc
allocation, the code jumps to out_ucontrol_uninit and out_free_sie_block.

While it frees the sie_block and removes the gmap child, it seems to never
free the memory allocated by kvm_s390_new_mmu_cache(vcpu->arch.mc).

Can userspace repeatedly trigger vCPU creation failures to leak memory and
cause a local denial of service?


arch/s390/kvm/kvm-s390.c:kvm_arch_init_vm() {
[ ... ]
out_err:
	free_page((unsigned long)kvm->arch.sie_page2);
	debug_unregister(kvm->arch.dbf);
	sca_dispose(kvm);
[ ... ]
}

arch/s390/kvm/kvm-s390.c:sca_dispose() {
	free_pages_exact(kvm->arch.sca, sizeof(*kvm->arch.sca));
	kvm->arch.sca = NULL;
}

[Severity: Critical]
This is a pre-existing issue, but is there a risk of memory corruption here?

In kvm_arch_init_vm(), if VM creation fails early, the code jumps to out_err
before kvm->arch.sca is allocated.

Since the kvm structure is zero-allocated, kvm->arch.sca is NULL.

The sca_dispose() function calls
free_pages_exact(kvm->arch.sca, sizeof(*kvm->arch.sca)). The size of
struct esca_block is roughly 16KB.

The free_pages_exact(virt, size) function loops from addr = virt to
addr + PAGE_ALIGN(size), incrementing by PAGE_SIZE, and calls
free_page(addr).

For virt == NULL, it iterates through addresses 0, 4096, 8192, and 12288.

While free_page(0) is safely ignored, free_page(4096) translates to
__free_pages(virt_to_page(4096), 0) and incorrectly frees host physical pages
1, 2, and 3 into the buddy allocator.

Can an unprivileged user trigger this trivially by calling
ioctl(KVM_CREATE_VM, 1)?


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() {
	int cc;

	if (!kvm_s390_pv_cpu_get_handle(vcpu))
		return 0;
[ ... ]
}

[Severity: High]
This is a pre-existing issue, but does this error handling permanently leak
the newly allocated stor_base and sida_addr pages?

The kvm_s390_pv_create_cpu() function allocates vcpu->arch.pv.stor_base
and sida_addr. If uv_call fails, it jumps to error handling which
calls kvm_s390_pv_destroy_cpu(vcpu).

However, kvm_s390_pv_destroy_cpu() begins with an early return:
if (!kvm_s390_pv_cpu_get_handle(vcpu)) return 0;

Because vcpu->arch.pv.handle is only set upon successful uv_call completion,
the handle is 0 here. Thus, the destruction routine does nothing and bypasses
the free_pages() blocks entirely.

Could a compromised or malicious userspace trigger failures in uv_call to
repeatedly leak host memory?

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