Re: [PATCH v7 06/14] KVM: s390: Fix leaking of PGM_ADDRESSING to userspace

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

Pre-existing issues:
- [High] Uninitialized stack variable `prev.zero` is leaked to userspace.
--

--- Patch [6]: [PATCH v7 06/14] KVM: s390: Fix leaking of PGM_ADDRESSING to userspace ---
commit ed708d75cfaadb270c9e236e10c87fa7cbbb9d4f
Author: Claudio Imbrenda <[email protected]>

KVM: s390: Fix leaking of PGM_ADDRESSING to userspace

This commit modifies KVM memory ioctls (such as setting CMMA bits or storage
keys) to return -EFAULT instead of a positive PGM_ADDRESSING error code when
addressing outside a memslot. This restores the expected behavior from before
the gmap rewrite.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 21574f57be72e..e162efaa35b8f 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -571,7 +571,7 @@ static int kvm_s390_keyop(struct kvm_s390_mmu_cache *mc, struct kvm *kvm, int op
>  	switch (op) {
>  	case KVM_S390_KEYOP_SSKE:
>  		r = dat_cond_set_storage_key(mc, asce, gfn, skey, &skey, 0, 0, 0);

[Severity: High]
This is a pre-existing issue, but does dat_cond_set_storage_key() leak
uninitialized kernel stack memory to userspace here?

When the KVM_S390_KEYOP ioctl is called with KVM_S390_KEYOP_SSKE, it calls
dat_cond_set_storage_key(), which populates a local stack variable:

arch/s390/kvm/dat.c:dat_cond_set_storage_key() {
    ...
	} else {
		prev.acc = old.acc;
		prev.fp = old.fp;
		prev.c = old.gc;
		prev.r = old.gr;
	}
	if (oldkey)
		*oldkey = prev;
    ...
}

The 1-bit prev.zero bitfield is left uninitialized in this branch. The entire
1-byte union is then copied into *oldkey and returned to kvm_s390_keyop().
Could this value then be copied to userspace, successfully leaking 1 bit of
kernel stack memory?

> -		if (r >= 0)
> +		if (r == 0 || r == 1)
>  			return skey.skey;
>  		break;

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