netdev - Re: [PATCH] vhost: force spec specified alignment on types

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d171447e-eabc-60ab-6de7-41ac9b82d7d1@redhat.com>
Date:   Mon, 6 Apr 2020 22:09:27 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org
Subject: Re: [PATCH] vhost: force spec specified alignment on types


On 2020/4/6 下午9:55, Michael S. Tsirkin wrote:
> On Mon, Apr 06, 2020 at 09:34:00PM +0800, Jason Wang wrote:
>> On 2020/4/6 下午8:50, Michael S. Tsirkin wrote:
>>> The ring element addresses are passed between components with different
>>> alignments assumptions. Thus, if guest/userspace selects a pointer and
>>> host then gets and dereferences it, we might need to decrease the
>>> compiler-selected alignment to prevent compiler on the host from
>>> assuming pointer is aligned.
>>>
>>> This actually triggers on ARM with -mabi=apcs-gnu - which is a
>>> deprecated configuration, but it seems safer to handle this
>>> generally.
>>>
>>> I verified that the produced binary is exactly identical on x86.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@...hat.com>
>>> ---
>>>
>>> This is my preferred way to handle the ARM incompatibility issues
>>> (in preference to kconfig hacks).
>>> I will push this into next now.
>>> Comments?
>>
>> I'm not sure if it's too late to fix. It would still be still problematic
>> for the userspace that is using old uapi headers?
>>
>> Thanks
> It's not a problem in userspace. The problem is when
> userspace/guest uses 2 byte alignment and passes it to kernel
> assuming 8 byte alignment. The fix is for host not to
> make these assumptions.


Yes, but I meant when userspace is complied with apcs-gnu, then it still 
assumes 8 byte alignment?

Thanks


>
>>>    drivers/vhost/vhost.h            |  6 ++---
>>>    include/uapi/linux/virtio_ring.h | 41 ++++++++++++++++++++++++--------
>>>    2 files changed, 34 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
>>> index cc82918158d2..a67bda9792ec 100644
>>> --- a/drivers/vhost/vhost.h
>>> +++ b/drivers/vhost/vhost.h
>>> @@ -74,9 +74,9 @@ struct vhost_virtqueue {
>>>    	/* The actual ring of buffers. */
>>>    	struct mutex mutex;
>>>    	unsigned int num;
>>> -	struct vring_desc __user *desc;
>>> -	struct vring_avail __user *avail;
>>> -	struct vring_used __user *used;
>>> +	vring_desc_t __user *desc;
>>> +	vring_avail_t __user *avail;
>>> +	vring_used_t __user *used;
>>>    	const struct vhost_iotlb_map *meta_iotlb[VHOST_NUM_ADDRS];
>>>    	struct vhost_desc *descs;
>>> diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
>>> index 559f42e73315..cd6e0b2eaf2f 100644
>>> --- a/include/uapi/linux/virtio_ring.h
>>> +++ b/include/uapi/linux/virtio_ring.h
>>> @@ -118,16 +118,6 @@ struct vring_used {
>>>    	struct vring_used_elem ring[];
>>>    };
>>> -struct vring {
>>> -	unsigned int num;
>>> -
>>> -	struct vring_desc *desc;
>>> -
>>> -	struct vring_avail *avail;
>>> -
>>> -	struct vring_used *used;
>>> -};
>>> -
>>>    /* Alignment requirements for vring elements.
>>>     * When using pre-virtio 1.0 layout, these fall out naturally.
>>>     */
>>> @@ -164,6 +154,37 @@ struct vring {
>>>    #define vring_used_event(vr) ((vr)->avail->ring[(vr)->num])
>>>    #define vring_avail_event(vr) (*(__virtio16 *)&(vr)->used->ring[(vr)->num])
>>> +/*
>>> + * The ring element addresses are passed between components with different
>>> + * alignments assumptions. Thus, we might need to decrease the compiler-selected
>>> + * alignment, and so must use a typedef to make sure the __aligned attribute
>>> + * actually takes hold:
>>> + *
>>> + * https://gcc.gnu.org/onlinedocs//gcc/Common-Type-Attributes.html#Common-Type-Attributes
>>> + *
>>> + * When used on a struct, or struct member, the aligned attribute can only
>>> + * increase the alignment; in order to decrease it, the packed attribute must
>>> + * be specified as well. When used as part of a typedef, the aligned attribute
>>> + * can both increase and decrease alignment, and specifying the packed
>>> + * attribute generates a warning.
>>> + */
>>> +typedef struct vring_desc __attribute__((aligned(VRING_DESC_ALIGN_SIZE)))
>>> +	vring_desc_t;
>>> +typedef struct vring_avail __attribute__((aligned(VRING_AVAIL_ALIGN_SIZE)))
>>> +	vring_avail_t;
>>> +typedef struct vring_used __attribute__((aligned(VRING_USED_ALIGN_SIZE)))
>>> +	vring_used_t;
>>> +
>>> +struct vring {
>>> +	unsigned int num;
>>> +
>>> +	vring_desc_t *desc;
>>> +
>>> +	vring_avail_t *avail;
>>> +
>>> +	vring_used_t *used;
>>> +};
>>> +
>>>    static inline void vring_init(struct vring *vr, unsigned int num, void *p,
>>>    			      unsigned long align)
>>>    {