Re: [PATCH 1/2] KVM: s390: Limit adapter indicator access to mapped page

[Matthew Rosato <mjrosato@linux.ibm.com>]

On 2/17/26 3:54 AM, Janosch Frank wrote:
> While we check the address for errors, we don't seem to check the bit
> offsets and since they are 32 and 64 bits a lot of memory can be
> reached indirectly via those offsets.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Fixes: 84223598778b ("KVM: s390: irq routing for adapter interrupts.")
> ---
>  arch/s390/kvm/interrupt.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 1c2bb5cd7e12..cd4851e33a5b 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -2724,6 +2724,9 @@ static unsigned long get_ind_bit(__u64 addr, unsigned long bit_nr, bool swap)
>  
>  	bit = bit_nr + (addr % PAGE_SIZE) * 8;
>  
> +	/* kvm_set_routing_entry() should never allow this to happen */
> +	WARN_ON_ONCE(bit > (PAGE_SIZE * BITS_PER_BYTE - 1));
> +
>  	return swap ? (bit ^ (BITS_PER_LONG - 1)) : bit;
>  }
>  
> @@ -2852,6 +2855,7 @@ int kvm_set_routing_entry(struct kvm *kvm,
>  			  struct kvm_kernel_irq_routing_entry *e,
>  			  const struct kvm_irq_routing_entry *ue)
>  {
> +	const struct kvm_irq_routing_s390_adapter *adapter;
>  	u64 uaddr_s, uaddr_i;
>  	int idx;
>  
> @@ -2862,6 +2866,14 @@ int kvm_set_routing_entry(struct kvm *kvm,
>  			return -EINVAL;
>  		e->set = set_adapter_int;
>  
> +		adapter = &ue->u.adapter;
> +		if (adapter->summary_addr + BITS_TO_BYTES(adapter->summary_offset) >=
> +		    (adapter->summary_addr & PAGE_MASK) + PAGE_SIZE)
> +			return -EINVAL;
> +		if (adapter->ind_addr + BITS_TO_BYTES(adapter->ind_offset) >=
> +		    (adapter->ind_addr & PAGE_MASK) + PAGE_SIZE)
> +			return -EINVAL;
> +


I think this is slightly off.

The offset should indicate a bit offset from the beginning of the byte, so offsets 0-7 are all within the same byte as the specified address, 8-15 are in the next byte, etc.

I hacked QEMU and tested something like...
1) addr 8126efff offset 7 -- this would be the very last bit in the page.
2) addr 8126efff offset 8 -- this would be the very first bit in the next page.  
3) addr 8126efff offset 9 -- this would be the 2nd bit in the next page.

I expected (1) to pass while (2) and (3) were rejected, but all 3 were rejected by your check.

I think the problem is that BITS_TO_BYTES rounds up.  So:
BITS_TO_BYTES(0) = 0
BITS_TO_BYTES(1..8) = 1
BITS_TO_BYTES(9..16) = 2
and so on.

But your offset check expects
0..7 = 0
8..15 = 1
and so on.

AFAICT replacing BITS_TO_BYTES(offset) with (offset / 8) would work.


>  		idx = srcu_read_lock(&kvm->srcu);
>  		uaddr_s = gpa_to_hva(kvm, ue->u.adapter.summary_addr);
>  		uaddr_i = gpa_to_hva(kvm, ue->u.adapter.ind_addr);