Re: [PATCH v4 22/27] KVM: s390: Add basic arm64 kvm module

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Integer underflow in `kvm_max_guest_address()` allows bypassing IPA limit checks, leading to shift-out-of-bounds undefined behavior.
- [Medium] Falsely advertises support for `KVM_CAP_ARM_USER_IRQ` while silently dropping interrupt injections.
--

commit cf17b7229eed87d0fba2ede0e60b154b44cdfc5e
Author: Steffen Eiden <seiden@linux.ibm.com>

KVM: s390: Add basic arm64 kvm module
    
Add basic code for the new arm64 on s390 KVM implementation.
Add kernel module boilerplate code and trivial functions.

> diff --git a/arch/s390/kvm/arm64/arm.c b/arch/s390/kvm/arm64/arm.c
> new file mode 100644
> index 0000000000000..19249ec5c539f
> --- /dev/null
> +++ b/arch/s390/kvm/arm64/arm.c
[ ... ]
> @@ -20,172 +20,192 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  {
>  	int ret;
>  
>  	switch (ext) {
>  	case KVM_CAP_NR_VCPUS:
>  	case KVM_CAP_MAX_VCPUS:
>  	case KVM_CAP_MAX_VCPU_ID:
>  		ret = KVM_MAX_VCPUS;
>  		break;
>  	case KVM_CAP_ARM_VM_IPA_SIZE:
>  		ret = get_kvm_ipa_limit();
>  		break;
>  	case KVM_CAP_IOEVENTFD:
>  	case KVM_CAP_ARM_USER_IRQ:
>  		ret = 1;

[Severity: Medium]
Since kvm_vm_ioctl_irq_line() silently drops interrupts and returns success,
does advertising support for KVM_CAP_ARM_USER_IRQ here cause userspace to
believe the capability works, leading to lost interrupts and guest hangs?

>  		break;
>  	default:
>  		ret = 0;
>  	}
>  
>  	return ret;
>  }
>  
>  static u64 kvm_max_guest_address(void)
>  {
>  	u64 max_addr;
>  
>  	if (sclp.hamax == U64_MAX)
>  		max_addr = TASK_SIZE_MAX;
>  	else
>  		max_addr = min_t(u64, TASK_SIZE_MAX, sclp.hamax);
>  	return ALIGN_DOWN(max_addr + 1, 1 << 30) - 1;

[Severity: Medium]
Can this underflow if max_addr + 1 is less than 1GB? 

If sclp.hamax restricts the maximum address to less than 1GB, 
ALIGN_DOWN(max_addr + 1, 1 << 30) will evaluate to 0. Subtracting 1 would
then underflow to U64_MAX.

>  }
>  
>  vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
>  {
>  	return VM_FAULT_SIGBUS;
>  }
>  
>  long kvm_arch_dev_ioctl(struct file *filp,
>  			unsigned int ioctl, unsigned long arg)
>  {
>  	return -EINVAL;
>  }
>  
>  u32 get_kvm_ipa_limit(void)
>  {
>  	return fls64(kvm_max_guest_address() + 1) - 1;

[Severity: Medium]
If kvm_max_guest_address() returns U64_MAX due to the underflow above, this
will evaluate to U32_MAX. 

Could this large value cause the IPA limit validation checks to be bypassed,
allowing untrusted userspace to provide a large shift value and trigger a
shift-out-of-bounds undefined behavior?

>  }
[ ... ]
>  int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level,
>  			  bool line_status)
>  {
>  	return 0;

[Severity: Medium]
This isn't a bug in the implementation of the stub, but since
KVM_CAP_ARM_USER_IRQ is advertised as supported, will returning success here
without injecting the interrupt deceive userspace and silently drop the
interrupts?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706085229.979525-1-seiden@linux.ibm.com?part=22