Re: [PATCH v2 3/9] KVM: s390: Fix get_all_floating_irqs()

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

Pre-existing issues:
- [High] Potential NULL pointer dereference due to a TOCTOU data race on `gi->origin`.
--

--- Patch [3]: [PATCH v2 3/9] KVM: s390: Fix get_all_floating_irqs() ---
commit b1b05a4cfd3db133f1eddf2ecf286b6b12e10c50
Author: Claudio Imbrenda <[email protected]>

KVM: s390: Fix get_all_floating_irqs()

This commit modifies the reporting of floating interrupts to userspace 
to use a non-destructive test. It ensures that the GISA IPM bits are 
not inadvertently cleared and lost if the userspace output buffer is 
too small.

> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c

[ ... ]

> @@ -2242,7 +2247,7 @@ static int get_all_floating_irqs(struct kvm *kvm, u8 __user *usrbuf, u64 len)
>  				ret = -ENOMEM;
>  				goto out_nolock;
>  			}
> -			if (gisa_tac_ipm_gisc(gi->origin, i)) {
> +			if (gisa_test_ipm_gisc(gi->origin, i)) {

[Severity: High]
This is a pre-existing issue, but is it possible for gi->origin to be
dereferenced as NULL here?

Looking at get_all_floating_irqs(), gi->origin is accessed multiple times
without lock protection or READ_ONCE(). The pointer is initially checked
earlier in the function:

    if (gi->origin && gisa_get_ipm(gi->origin)) {

and then accessed repeatedly inside the loop for each ISC.

Could the compiler reload the gi->origin pointer from memory between the
initial check and these loop iterations?

If a concurrent KVM_PV_ENABLE ioctl is invoked, it can execute
kvm_s390_pv_init_vm() -> kvm_s390_cpus_to_pv() -> kvm_s390_gisa_disable() ->
kvm_s390_gisa_destroy(), which sets gi->origin = NULL.

Since the KVM_DEV_FLIC_GET_ALL_IRQS ioctl does not hold kvm->lock, could
this concurrent clearing lead to a NULL pointer dereference if the compiler
reloads gi->origin inside the loop?

>  				irq = (struct kvm_s390_irq *) &buf[n];
>  				irq->type = KVM_S390_INT_IO(1, 0, 0, 0);
>  				irq->u.io.io_int_word = isc_to_int_word(i);

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