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

[Michael S. Tsirkin]

On Tue, Nov 03, 2020 at 02:26:10PM +0800, Bin Meng wrote:
> Hi Michael,
> 
> On Fri, Oct 30, 2020 at 5:29 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > 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.
> 
> Thanks for the review.
> 
> >
> > If I understand correctly, what happens is:
> > - tag is set to a value that is not a multiple of 4 bytes
> 
> It's not about the mount_tag value, but the length of the mount_tag is 4.
> 
> > - guest attempts to read the last 4 bytes of the tag
> 
> Yep. So the config space of a 9pfs looks like the following:
> 
> offset: 0x14, size: 2 bytes indicating the length of the following mount_tag
> offset: 0x16, size: value of (offset 0x14).
> 
> When a 4-byte mount_tag is given, guest software is subject to read 4
> bytes (value read from offset 0x14) at offset 0x16.


Well looking at Linux guest code:


static inline void __virtio_cread_many(struct virtio_device *vdev,
                                       unsigned int offset,
                                       void *buf, size_t count, size_t bytes)
{
        u32 old, gen = vdev->config->generation ?
                vdev->config->generation(vdev) : 0;
        int i;  
                                   
        might_sleep();             
        do {
                old = gen;

                for (i = 0; i < count; i++)
                        vdev->config->get(vdev, offset + bytes * i,
                                          buf + i * bytes, bytes);
        
                gen = vdev->config->generation ?
                        vdev->config->generation(vdev) : 0;
        } while (gen != old);
}
        


static inline void virtio_cread_bytes(struct virtio_device *vdev,
                                      unsigned int offset,
                                      void *buf, size_t len) 
{           
        __virtio_cread_many(vdev, offset, buf, len, 1);
}

and:


        virtio_cread_bytes(vdev, offsetof(struct virtio_9p_config, tag),
                           tag, tag_len);



So guest is doing multiple 1-byte reads.


Spec actually says:
        For device configuration access, the driver MUST use 8-bit wide 
accesses for 8-bit wide fields, 16-bit wide

        and aligned accesses for 16-bit wide fields and 32-bit wide and aligned 
accesses for 32-bit and 64-bit wide

        fields. For 64-bit fields, the driver MAY access each of the high and 
low 32-bit parts of the field independently.

9p was never standardized, but the linux header at least lists it as
follows:

struct virtio_9p_config {
        /* length of the tag name */
        __virtio16 tag_len;
        /* non-NULL terminated tag name */
        __u8 tag[0];
} __attribute__((packed));

In that sense tag is an 8 byte field.

So which guest reads tag using a 32 bit read, and why?



> > - access returns -1
> >
> 
> The access will be split into 2 accesses, either by hardware or
> software. On RISC-V such unaligned access is emulated by M-mode
> firmware. On ARM I believe it's supported by the CPU. So the first
> converted aligned access is to read 4 byte at 0x14 and the second
> converted aligned access is to read 4 byte at 0x16, and drop the bytes
> that are not needed, assemble the remaining bytes and return the
> result to the guest software. The second aligned access will fail the
> sanity check and return -1, but not the first access, hence the result
> will be `p9\377\377`.
> 
> >
> > 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
> >
> 
> Regards,
> Bin