Hi Martin,
On Mon, May 09, 2022 at 11:39:36PM +0300, Martin Radev wrote:
> This patch verifies that adding the addr and length arguments
> from an MMIO op do not overflow. This is necessary because the
> arguments are controlled by the VM. The length may be set to
> an arbitrary value by using the rep prefix.
>
> Reviewed-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
> Signed-off-by: Martin Radev <martin.b.radev@xxxxxxxxx>
> ---
> mmio.c | 4 ++++
> virtio/mmio.c | 6 ++++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/mmio.c b/mmio.c
> index a6dd3aa..5a114e9 100644
> --- a/mmio.c
> +++ b/mmio.c
> @@ -32,6 +32,10 @@ static struct mmio_mapping *mmio_search(struct rb_root *root, u64 addr, u64 len)
> {
> struct rb_int_node *node;
>
> + /* If len is zero or if there's an overflow, the MMIO op is invalid. */
> + if (addr + len <= addr)
> + return NULL;
> +
> node = rb_int_search_range(root, addr, addr + len);
> if (node == NULL)
> return NULL;
> diff --git a/virtio/mmio.c b/virtio/mmio.c
> index 875a288..979fa8c 100644
> --- a/virtio/mmio.c
> +++ b/virtio/mmio.c
> @@ -105,6 +105,12 @@ static void virtio_mmio_device_specific(struct kvm_cpu *vcpu,
> struct virtio_mmio *vmmio = vdev->virtio;
> u32 i;
>
> + /* Check for wrap-around and zero length. */
> + if (addr + len <= addr) {
> + WARN_ONCE(1, "addr (%llu) + length (%u) wraps-around.\n", addr, len);
> + return;
> + }
This is _NOT_ needed.
When a VCPU exits with exit_reason set to KVM_EXIT_MMIO, kvmtool searches
for the virtio-mmio callback (which ends up calling
virtio_mmio_device_specific) in kvm_cpu__emulate_mmio() ->
kvm__emulate_mmio() -> mmio_search(), which already contains the overflow
check. The virtio_mmio_device_specific() checks above is redundant.
Please remove the above 5 lines of code.
Thanks,
Alex
> +
> for (i = 0; i < len; i++) {
> if (is_write)
> vdev->ops->get_config(vmmio->kvm, vmmio->dev)[addr + i] =
> --
> 2.25.1
>
