Re: [PATCH v4 1/4] LoongArch: KVM: Set default MAILBOX access size with 8

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Guest-triggerable `WARN_ON_ONCE` via unaligned IOCSR address can crash the host, violating KVM guidelines.
- [High] `read_mailbox()` unconditionally performs an 8-byte read, leading to structural out-of-bounds array access and undefined behavior.
- [High] Read-modify-write cycles on IPI mailboxes are not protected by `ipi_state.lock`, leading to lost mailbox updates.
--

--- Patch [1]: [PATCH v4 1/4] LoongArch: KVM: Set default MAILBOX access size with 8 ---
commit ddb469fb4110b13e360da0bda699a489c854b92b
Author: Bibo Mao <maobibo@loongson.cn>

LoongArch: KVM: Set default MAILBOX access size with 8

This commit changes the default mailbox access size to 8 bytes in
read_mailbox() and write_mailbox() to handle IOCSR instruction emulation,
removing the previous error logging for unknown sizes.

> diff --git a/arch/loongarch/kvm/intc/ipi.c b/arch/loongarch/kvm/intc/ipi.c
> --- a/arch/loongarch/kvm/intc/ipi.c
> +++ b/arch/loongarch/kvm/intc/ipi.c
> @@ -67,11 +67,8 @@ static uint64_t read_mailbox(struct kvm_vcpu *vcpu, int offset, int len)
>  		return data & 0xffff;
>  	case 4:
>  		return data & 0xffffffff;
> -	case 8:
> -		return data;
>  	default:
> -		kvm_err("%s: unknown data len: %d\n", __func__, len);
> -		return 0;
> +		return data;
>  	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the unconditional 8-byte
read in read_mailbox() risk an out-of-bounds array access?

Looking at read_mailbox(), it computes a byte pointer into the 32-byte
ipi_state.buf array based on the offset, and unconditionally casts it to an
unsigned long pointer to read 8 bytes before evaluating the size cases:

    data = *(ulong *)((void *)vcpu->arch.ipi_state.buf + (offset - 0x20));

If a guest performs a valid 4-byte read at the maximum valid offset, the
function still reads 8 bytes. This means the read extends beyond
ipi_state.buf and bleeds into adjacent struct members. Additionally,
unconditionally reading an 8-byte integer from a potentially unaligned
pointer might result in undefined behavior.

[ ... ]

> @@ -92,11 +89,9 @@ static void write_mailbox(struct kvm_vcpu *vcpu, int offset, uint64_t data, int
>  	case 4:
>  		*(unsigned int *)pbuf = (unsigned int)data;
>  		break;
> -	case 8:
> +	default:
>  		*(unsigned long *)pbuf = (unsigned long)data;
>  		break;
> -	default:
> -		kvm_err("%s: unknown data len: %d\n", __func__, len);
>  	}
>  	spin_unlock(&vcpu->arch.ipi_state.lock);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but is there a risk of lost
mailbox updates due to unprotected read-modify-write cycles in mail_send()
and send_ipi_data()?

In mail_send(), the code performs a read-modify-write cycle to update IPI
mailboxes using byte bit-masks:

    val = read_mailbox(vcpu, offset, 4);
    for (i = 0; i < 4; i++)
        if (data & (BIT(27 + i)))
            mask |= (0xff << (i * 8));
    val &= mask;
    ...
    val |= ((uint32_t)(data >> 32) & ~mask);
    write_mailbox(vcpu, offset, val, 4);

Since read_mailbox() and write_mailbox() acquire and release ipi_state.lock
internally, the lock is not held across the entire modification cycle. If
multiple vCPUs send IPIs with masks to the same target concurrently, could
their operations interleave and overwrite each other's updates?

[Severity: High]
This isn't a bug introduced by this patch, but can a guest deliberately
trigger a WARN_ON_ONCE by providing an unaligned IOCSR address?

Looking at loongarch_ipi_readl() and loongarch_ipi_writel(), they
unconditionally assert WARN_ON_ONCE for unaligned offsets:

    WARN_ON_ONCE(offset & (len - 1));

A guest can trigger this by executing an unaligned IOCSR read/write
instruction within the IPI address range, or by writing an unaligned offset
payload to IOCSR_ANY_SEND which reaches kvm_io_bus_read() or
kvm_io_bus_write() in send_ipi_data().

Since KVM io bus operations do not validate alignment before routing to
these functions, an unprivileged guest could intentionally panic the host
kernel on systems configured with panic_on_warn.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709082109.1361767-1-maobibo@loongson.cn?part=1