Re: [PATCH 1/5] virtio-blk: enforce iothread-vq-mapping validation

[Hanna Czenczek]

On 05.02.24 18:26, Stefan Hajnoczi wrote:
> Hanna Czenczek <hreitz@redhat.com> noticed that the safety of
> `vq_aio_context[vq->value] = ctx;` with user-defined vq->value inputs is
> not obvious.
>
> The code is structured in validate() + apply() steps so input validation
> is there, but it happens way earlier and there is nothing that
> guarantees apply() can only be called with validated inputs.
>
> This patch moves the validate() call inside the apply() function so
> validation is guaranteed. I also added the bounds checking assertion
> that Hanna suggested.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   hw/block/virtio-blk.c | 192 +++++++++++++++++++++++-------------------
>   1 file changed, 107 insertions(+), 85 deletions(-)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 227d83569f..e8b37fd5f4 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c

[...]

> @@ -1660,6 +1681,14 @@ static bool virtio_blk_vq_aio_context_init(VirtIOBlock 
> *s, Error **errp)
>       BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
>       VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>   
> +    if (conf->iothread && conf->iothread_vq_mapping_list) {
> +        if (conf->iothread) {

This inner condition should probably be dropped.  With that done:

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

> +            error_setg(errp, "iothread and iothread-vq-mapping properties "
> +                             "cannot be set at the same time");
> +            return false;
> +        }
> +    }
> +
>       if (conf->iothread || conf->iothread_vq_mapping_list) {
>           if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
>               error_setg(errp,