[PATCH v3 36/40] KVM: arm64: gic-v5: Implement save/restore mechanisms for ISTs

[Fuad Tabba]

Hi Sascha,

Two things about the new KVM_DEV_ARM_VGIC_GRP_IST interface. Both are
about the userspace ABI rather than the mechanism.

...

> +int vgic_v5_save_spi_ist(struct kvm *kvm, struct kvm_device_attr *attr)
> +{
> +    u32 __user *uaddr = (u32 __user *)(unsigned long)attr->addr;
...
> +    for (unsigned int i = 0; i < kvm->arch.vgic.nr_spis; ++i) {
...
> +        __le32 *h_iste_addr = ist.base + i * ist.iste_size;
> +
> +        h_iste = READ_ONCE(*h_iste_addr);
> +        ret = put_user(h_iste, uaddr);
> +        if (ret)
> +            return ret;
> +
> +        uaddr++;
> +    }

The kernel writes nr_spis * 4 bytes into attr->addr, but struct
kvm_device_attr has no length field, so userspace has no way to
declare how large its buffer is and the kernel has no way to check it.
The size is implied by nr_spis, which userspace set earlier through
KVM_DEV_ARM_VGIC_GRP_NR_IRQS, or never set, in which case it defaulted
to 32.

put_user() has no idea how large the caller's buffer is. It only fails
if the address itself is not writable, so writes past the end of a
short buffer land in whatever happens to be next, with no error
reported. The documentation states the required size, but nothing
enforces it.

It is also the only variable length buffer in the VGICv5 interface.
GRP_IRS_REGS is a __u64, USERSPACE_PPIS is a __u64[2], GRP_NR_IRQS is
a __u32, and none of those can be got wrong.

kvm_device_attr itself is fixed, so no length can be added there.
Changing what addr points at is still possible while this is unmerged,
but would not be afterwards. Would it make sense for addr to point at
a small structure carrying an explicit size, so that the kernel can
reject a mismatch?

The second point is the LPI IST going into guest memory. I understand
from the commit message that the shadowing is deliberate and that the
guest has already allocated the storage, so this is about the
interface rather than that decision.

Saving the ISTs is a KVM_GET_DEVICE_ATTR, and it writes into guest
memory. Several of the documented ordering constraints are enforced,
with -EBUSY when the VGIC is not initialised or when a restore is
attempted after the VM has run. This one cannot be, because it is
about when the VMM serialises guest memory:

> These two steps may be performed in either order. However, the guest memory
> must be serialised after the ISTs have been saved, as saving the LPI IST writes
> the IST state back into guest memory.

A VMM that snapshots guest memory before saving the ISTs gets no
error, and the IST contents simply never reach the destination. Live
migration looks fine to me, since kvm_write_guest() marks the pages
dirty and a later pass picks them up, but a stop and copy save done in
the natural order would lose them silently.

Was a userspace buffer considered for the LPI IST as well, the way the
SPI IST already works? That would remove the ordering requirement
entirely, and by my reading the worst case is 65536 entries at 32
bits, so 256 KiB, given the 16 bit clamp on the guest's LPI ID bits.

Cheers,
/fuad