Re: [PATCH] hw/9pfs: virtio-9p: Ensure config space is a multiple of 4 bytes

[Michael S. Tsirkin]

On Thu, Oct 29, 2020 at 04:25:41PM +0800, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> At present the virtio device config space access is handled by the
> virtio_config_readX() and virtio_config_writeX() APIs. They perform
> a sanity check on the result of address plus size against the config
> space size before the access occurs.
> 
> For unaligned access, the last converted naturally aligned access
> will fail the sanity check on 9pfs. For example, with a mount_tag
> `p9fs`, if guest software tries to read the mount_tag via a 4 byte
> read at the mount_tag offset which is not 4 byte aligned, the read
> result will be `p9\377\377`, which is wrong.
> 
> This changes the size of device config space to be a multiple of 4
> bytes so that correct result can be returned in all circumstances.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>



The patch is ok, but I'd like to clarify the commit log.

If I understand correctly, what happens is:
- tag is set to a value that is not a multiple of 4 bytes
- guest attempts to read the last 4 bytes of the tag
- access returns -1


What I find confusing in the above description:
- reference to unaligned access - I don't think these
  are legal or allowed by QEMU
- reference to `p9\377\377` - I think returned value will be -1

thanks!

> ---
> 
>  hw/9pfs/virtio-9p-device.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 14371a7..e6a1432 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -201,6 +201,7 @@ static void virtio_9p_device_realize(DeviceState *dev, 
> Error **errp)
>      V9fsVirtioState *v = VIRTIO_9P(dev);
>      V9fsState *s = &v->state;
>      FsDriverEntry *fse = get_fsdev_fsentry(s->fsconf.fsdev_id);
> +    size_t config_size;
>  
>      if (qtest_enabled() && fse) {
>          fse->export_flags |= V9FS_NO_PERF_WARN;
> @@ -211,7 +212,8 @@ static void virtio_9p_device_realize(DeviceState *dev, 
> Error **errp)
>      }
>  
>      v->config_size = sizeof(struct virtio_9p_config) + strlen(s->fsconf.tag);
> -    virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P, v->config_size);
> +    config_size = ROUND_UP(v->config_size, 4);
> +    virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P, config_size);
>      v->vq = virtio_add_queue(vdev, MAX_REQ, handle_9p_output);
>  }
>  
> -- 
> 2.7.4