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

[Christian Schoenebeck]

On Mittwoch, 4. November 2020 08:44:44 CET Bin Meng wrote:
> Hi Michael,
> 
> On Tue, Nov 3, 2020 at 8:05 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > 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.
> 
> Correct.
> 
> > 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.
> Yes.
> 
> > 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?
> 
> The obvious fix can be made to the guest which exposed this issue, but
> I was wondering whether we should enforce all virtio devices' config
> space size to be a multiple of 4 bytes which sounds more natural.
> 
> Regards,
> Bin

Personally I am not opposed for this to be addressed in qemu, but Michael 
should decide that.

But even if it would be addressed in qemu, I still think this would better be 
addressed on virtio side, not on virtio user side (9pfs, etc.), because that's 
a virtio implementation detail that might change in future.

What I definitely don't want to see though, is this alignment issue being 
handled with a hard coded value on user (9pfs) side as this patch does right 
now. Because that smells like it would be overseen if something changes on 
virtio side one day.

Best regards,
Christian Schoenebeck