Re: [PATCH v5 2/9] KVM: s390: Fix dat_peek_cmma() overflow

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Uninitialized kernel memory is leaked to userspace during CMMA peek and get operations due to gaps left in the vmalloc'd buffer.
--

commit 7a3f41eb2c82217db779d886fe0375b646e0440a
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix dat_peek_cmma() overflow

This commit addresses an integer underflow in dat_peek_cmma() where an
out-of-bounds start address returns -EFAULT but leaves state.end at 0. It
corrects the behavior by ensuring the returned count is only updated if
the end address is greater than or equal to the starting address.

> diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
> index 4a41c0247ffa2..cffac7782c4bf 100644
> --- a/arch/s390/kvm/dat.c
> +++ b/arch/s390/kvm/dat.c
> @@ -1209,7 +1209,7 @@ int dat_peek_cmma(gfn_t start, union asce asce, unsigned int *count, u8 *values)
>  	int rc;
>  
>  	rc = _dat_walk_gfn_range(start, start + *count, asce, &ops, DAT_WALK_DEFAULT, &state);
> -	*count = state.end - start;
> +	*count = state.end >= start ? state.end - start : 0;

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but does
this leave uninitialized kernel memory in the values array that gets copied
to userspace?

Looking at kvm_s390_get_cmma_bits() in arch/s390/kvm/kvm-s390.c, the values
buffer is allocated with vmalloc() instead of vzalloc():

kvm_s390_get_cmma_bits() {
    ...
    values = vmalloc(args->count);
    ...
}

During the page table walk, if __dat_peek_cmma_crste() encounters an
invalid PMD, it advances state->end without populating state->values:

__dat_peek_cmma_crste() {
    ...
    state->end = min(walk->end, next);
    ...
}

Similarly, in __dat_get_cmma_pte(), clean pages are skipped without writing
to the values buffer:

__dat_get_cmma_pte() {
    ...
    if (!READ_ONCE(*pgste_of(ptep)).cmma_d)
        return 0;
    ...
}

Because the count returned by dat_peek_cmma() and dat_get_cmma() represents
the span from the start to state.end, any unpopulated gaps in the values
array will contain whatever uninitialized kernel heap memory vmalloc()
provided.

When kvm_s390_get_cmma_bits() later copies this buffer:

kvm_s390_get_cmma_bits() {
    ...
    if (copy_to_user((void __user *)args->values, values, args->count))
    ...
}

Can these gaps leak uninitialized memory to userspace? Should the buffer be
allocated with vzalloc() to ensure any gaps left by the walk are safely
zeroed out?

>  	/* Return success if at least one value was saved, otherwise an error. */
>  	return (rc == -EFAULT && *count > 0) ? 0 : rc;
>  }

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