Re: [GIT PULL v1 05/12] KVM: s390: Fix dat_peek_cmma() overflow

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Uninitialized kernel memory from `vmalloc` is copied to userspace when CMMA attributes are retrieved because skipped pages or unmapped CRSTEs leave uninitialized gaps in the buffer.
- [Medium] The `KVM_S390_SET_CMMA_BITS` ioctl incorrectly returns a positive program exception code (`PGM_ADDRESSING` = 5) on out-of-bounds access instead of a standard negative errno.
--

commit 8344f31bea75084c2603931b9dc4f97ab83d6263
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix dat_peek_cmma() overflow

This commit fixes an underflow issue in dat_peek_cmma() when userspace
provides an out-of-bounds start address. It ensures the count is correctly
zeroed out instead of underflowing, so the function correctly returns an error
instead of erroneously reporting success.

Message-ID: <20260623153331.233784-3-imbrenda@linux.ibm.com>

> 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, but since we are modifying how count is
calculated for the values array, does this code leak uninitialized kernel
memory to userspace?

If KVM_S390_GET_CMMA_BITS is called on a memory range containing clean pages
or unmapped holes, it allocates the values buffer using vmalloc() which does
not zero-initialize memory:

arch/s390/kvm/kvm-s390.c:kvm_s390_get_cmma_bits() {
    ...
	values = vmalloc(args->count);
	if (!values)
		return -ENOMEM;
    ...
	if (copy_to_user((void __user *)args->values, values, args->count))
    ...
}

During the walk, dat_peek_cmma() and dat_get_cmma() can skip clean PTEs and
empty CRSTEs without writing to the values array. For example:

arch/s390/kvm/dat.c:__dat_peek_cmma_crste() {
    ...
	if (crstep->h.i)
		state->end = min(walk->end, next);
    ...
}

arch/s390/kvm/dat.c:__dat_get_cmma_pte() {
    ...
	if (!READ_ONCE(*pgste_of(ptep)).cmma_d)
		return 0;
    ...
}

Since the gap in GFNs is included in the returned count, does the
copy_to_user() call in kvm_s390_get_cmma_bits() end up copying the entire
length and leaking those uninitialized vmalloc gaps to userspace?

[Severity: Medium]
This isn't a bug introduced by this patch, but while looking at the CMMA
addressing failure paths, is there an issue with how out-of-bounds accesses
are reported by KVM_S390_SET_CMMA_BITS?

If an invalid start_gfn is provided, dat_entry_walk() returns a positive
program exception code:

arch/s390/kvm/dat.c:dat_entry_walk() {
    ...
	if (!asce_contains_gfn(asce, gfn))
		return PGM_ADDRESSING;
    ...
}

This positive value (5) appears to be propagated directly back to the ioctl
result in kvm_s390_set_cmma_bits():

arch/s390/kvm/kvm-s390.c:kvm_s390_set_cmma_bits() {
    ...
			r = dat_set_cmma_bits(mc, kvm->arch.gmap->asce, args->start_gfn,
					      args->count, args->mask, bits);
		}
	} while (r == -ENOMEM);

	set_bit(GMAP_FLAG_USES_CMM, &kvm->arch.gmap->flags);

	return r;
}

Could this cause userspace checking for standard Linux error codes
(ioctl() < 0) to incorrectly assume the KVM_S390_SET_CMMA_BITS ioctl
succeeded when it actually failed?

>  	/* 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/20260624111928.144283-1-imbrenda@linux.ibm.com?part=5