Michael S. Tsirkin
2020-Apr-20 20:46 UTC
[PATCH v3] virtio: force spec specified alignment on types
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. Note that userspace that allocates the memory is actually OK and does not need to be fixed, but userspace that gets it from guest or another process does need to be fixed. The later doesn't generally talk to the kernel so while it might be buggy it's not talking to the kernel in the buggy way - it's just using the header in the buggy way - so fixing header and asking userspace to recompile is the best we can do. I verified that the produced kernel binary on x86 is exactly identical before and after the change. Signed-off-by: Michael S. Tsirkin <mst at redhat.com> --- changes from v2: add vring_used_elem_t to ensure alignment for substructures changes from v1: swicth all __user to the new typedefs drivers/vhost/vhost.c | 8 +++--- drivers/vhost/vhost.h | 6 ++--- drivers/vhost/vringh.c | 6 ++--- include/linux/vringh.h | 6 ++--- include/uapi/linux/virtio_ring.h | 43 ++++++++++++++++++++++++-------- 5 files changed, 45 insertions(+), 24 deletions(-) diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index d450e16c5c25..bc77b0f465fd 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -1244,9 +1244,9 @@ static int vhost_iotlb_miss(struct vhost_virtqueue *vq, u64 iova, int access) } static bool vq_access_ok(struct vhost_virtqueue *vq, 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) { return access_ok(desc, vhost_get_desc_size(vq, num)) && @@ -2301,7 +2301,7 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads, unsigned count) { - struct vring_used_elem __user *used; + vring_used_elem_t __user *used; u16 old, new; int start; diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h index f8403bd46b85..60cab4c78229 100644 --- a/drivers/vhost/vhost.h +++ b/drivers/vhost/vhost.h @@ -67,9 +67,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 file *kick; struct eventfd_ctx *call_ctx; diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c index ba8e0d6cfd97..e059a9a47cdf 100644 --- a/drivers/vhost/vringh.c +++ b/drivers/vhost/vringh.c @@ -620,9 +620,9 @@ static inline int xfer_to_user(const struct vringh *vrh, */ int vringh_init_user(struct vringh *vrh, u64 features, unsigned int num, bool weak_barriers, - 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) { /* Sane power of 2 please! */ if (!num || num > 0xffff || (num & (num - 1))) { diff --git a/include/linux/vringh.h b/include/linux/vringh.h index 9e2763d7c159..59bd50f99291 100644 --- a/include/linux/vringh.h +++ b/include/linux/vringh.h @@ -105,9 +105,9 @@ struct vringh_kiov { /* Helpers for userspace vrings. */ int vringh_init_user(struct vringh *vrh, u64 features, unsigned int num, bool weak_barriers, - 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); static inline void vringh_iov_init(struct vringh_iov *iov, struct iovec *iovec, unsigned num) diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h index 9223c3a5c46a..b2c20f794472 100644 --- a/include/uapi/linux/virtio_ring.h +++ b/include/uapi/linux/virtio_ring.h @@ -86,6 +86,13 @@ * at the end of the used ring. Guest should ignore the used->flags field. */ #define VIRTIO_RING_F_EVENT_IDX 29 +/* Alignment requirements for vring elements. + * When using pre-virtio 1.0 layout, these fall out naturally. + */ +#define VRING_AVAIL_ALIGN_SIZE 2 +#define VRING_USED_ALIGN_SIZE 4 +#define VRING_DESC_ALIGN_SIZE 16 + /* Virtio ring descriptors: 16 bytes. These can chain together via "next". */ struct vring_desc { /* Address (guest-physical). */ @@ -112,29 +119,43 @@ struct vring_used_elem { __virtio32 len; }; +typedef struct vring_used_elem __aligned(VRING_USED_ALIGN_SIZE) + vring_used_elem_t; + struct vring_used { __virtio16 flags; __virtio16 idx; - struct vring_used_elem ring[]; + vring_used_elem_t ring[]; }; +/* + * 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 __aligned(VRING_DESC_ALIGN_SIZE) vring_desc_t; +typedef struct vring_avail __aligned(VRING_AVAIL_ALIGN_SIZE) vring_avail_t; +typedef struct vring_used __aligned(VRING_USED_ALIGN_SIZE) vring_used_t; + struct vring { unsigned int num; - struct vring_desc *desc; + vring_desc_t *desc; - struct vring_avail *avail; + vring_avail_t *avail; - struct vring_used *used; + vring_used_t *used; }; -/* Alignment requirements for vring elements. - * When using pre-virtio 1.0 layout, these fall out naturally. - */ -#define VRING_AVAIL_ALIGN_SIZE 2 -#define VRING_USED_ALIGN_SIZE 4 -#define VRING_DESC_ALIGN_SIZE 16 - #ifndef VIRTIO_RING_NO_LEGACY /* The standard layout for the ring is a continuous chunk of memory which looks -- MST
Jason Wang
2020-Apr-21 02:39 UTC
[PATCH v3] virtio: force spec specified alignment on types
On 2020/4/21 ??4:46, 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. > > Note that userspace that allocates the memory is actually OK and does > not need to be fixed, but userspace that gets it from guest or another > process does need to be fixed. The later doesn't generally talk to the > kernel so while it might be buggy it's not talking to the kernel in the > buggy way - it's just using the header in the buggy way - so fixing > header and asking userspace to recompile is the best we can do. > > I verified that the produced kernel binary on x86 is exactly identical > before and after the change. > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > > changes from v2: > add vring_used_elem_t to ensure alignment for substructures > changes from v1: > swicth all __user to the new typedefs > > drivers/vhost/vhost.c | 8 +++--- > drivers/vhost/vhost.h | 6 ++--- > drivers/vhost/vringh.c | 6 ++--- > include/linux/vringh.h | 6 ++--- > include/uapi/linux/virtio_ring.h | 43 ++++++++++++++++++++++++-------- > 5 files changed, 45 insertions(+), 24 deletions(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index d450e16c5c25..bc77b0f465fd 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -1244,9 +1244,9 @@ static int vhost_iotlb_miss(struct vhost_virtqueue *vq, u64 iova, int access) > } > > static bool vq_access_ok(struct vhost_virtqueue *vq, 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) > > { > return access_ok(desc, vhost_get_desc_size(vq, num)) && > @@ -2301,7 +2301,7 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq, > struct vring_used_elem *heads, > unsigned count) > { > - struct vring_used_elem __user *used; > + vring_used_elem_t __user *used; > u16 old, new; > int start; > > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h > index f8403bd46b85..60cab4c78229 100644 > --- a/drivers/vhost/vhost.h > +++ b/drivers/vhost/vhost.h > @@ -67,9 +67,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 file *kick; > struct eventfd_ctx *call_ctx; > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c > index ba8e0d6cfd97..e059a9a47cdf 100644 > --- a/drivers/vhost/vringh.c > +++ b/drivers/vhost/vringh.c > @@ -620,9 +620,9 @@ static inline int xfer_to_user(const struct vringh *vrh, > */ > int vringh_init_user(struct vringh *vrh, u64 features, > unsigned int num, bool weak_barriers, > - 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) > { > /* Sane power of 2 please! */ > if (!num || num > 0xffff || (num & (num - 1))) { > diff --git a/include/linux/vringh.h b/include/linux/vringh.h > index 9e2763d7c159..59bd50f99291 100644 > --- a/include/linux/vringh.h > +++ b/include/linux/vringh.h > @@ -105,9 +105,9 @@ struct vringh_kiov { > /* Helpers for userspace vrings. */ > int vringh_init_user(struct vringh *vrh, u64 features, > unsigned int num, bool weak_barriers, > - 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); > > static inline void vringh_iov_init(struct vringh_iov *iov, > struct iovec *iovec, unsigned num) > diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h > index 9223c3a5c46a..b2c20f794472 100644 > --- a/include/uapi/linux/virtio_ring.h > +++ b/include/uapi/linux/virtio_ring.h > @@ -86,6 +86,13 @@ > * at the end of the used ring. Guest should ignore the used->flags field. */ > #define VIRTIO_RING_F_EVENT_IDX 29 > > +/* Alignment requirements for vring elements. > + * When using pre-virtio 1.0 layout, these fall out naturally. > + */ > +#define VRING_AVAIL_ALIGN_SIZE 2 > +#define VRING_USED_ALIGN_SIZE 4 > +#define VRING_DESC_ALIGN_SIZE 16 > + > /* Virtio ring descriptors: 16 bytes. These can chain together via "next". */ > struct vring_desc { > /* Address (guest-physical). */ > @@ -112,29 +119,43 @@ struct vring_used_elem { > __virtio32 len; > }; > > +typedef struct vring_used_elem __aligned(VRING_USED_ALIGN_SIZE) > + vring_used_elem_t; > + > struct vring_used { > __virtio16 flags; > __virtio16 idx; > - struct vring_used_elem ring[]; > + vring_used_elem_t ring[]; > }; > > +/* > + * 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 __aligned(VRING_DESC_ALIGN_SIZE) vring_desc_t; > +typedef struct vring_avail __aligned(VRING_AVAIL_ALIGN_SIZE) vring_avail_t; > +typedef struct vring_used __aligned(VRING_USED_ALIGN_SIZE) vring_used_t;I wonder whether we can simply use __attribute__(packed) instead? Thanks> + > struct vring { > unsigned int num; > > - struct vring_desc *desc; > + vring_desc_t *desc; > > - struct vring_avail *avail; > + vring_avail_t *avail; > > - struct vring_used *used; > + vring_used_t *used; > }; > > -/* Alignment requirements for vring elements. > - * When using pre-virtio 1.0 layout, these fall out naturally. > - */ > -#define VRING_AVAIL_ALIGN_SIZE 2 > -#define VRING_USED_ALIGN_SIZE 4 > -#define VRING_DESC_ALIGN_SIZE 16 > - > #ifndef VIRTIO_RING_NO_LEGACY > > /* The standard layout for the ring is a continuous chunk of memory which looks
Michael S. Tsirkin
2020-Apr-21 13:25 UTC
[PATCH v3] virtio: force spec specified alignment on types
On Tue, Apr 21, 2020 at 10:39:19AM +0800, Jason Wang wrote:> > On 2020/4/21 ??4:46, 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. > > > > Note that userspace that allocates the memory is actually OK and does > > not need to be fixed, but userspace that gets it from guest or another > > process does need to be fixed. The later doesn't generally talk to the > > kernel so while it might be buggy it's not talking to the kernel in the > > buggy way - it's just using the header in the buggy way - so fixing > > header and asking userspace to recompile is the best we can do. > > > > I verified that the produced kernel binary on x86 is exactly identical > > before and after the change. > > > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > > --- > > > > changes from v2: > > add vring_used_elem_t to ensure alignment for substructures > > changes from v1: > > swicth all __user to the new typedefs > > > > drivers/vhost/vhost.c | 8 +++--- > > drivers/vhost/vhost.h | 6 ++--- > > drivers/vhost/vringh.c | 6 ++--- > > include/linux/vringh.h | 6 ++--- > > include/uapi/linux/virtio_ring.h | 43 ++++++++++++++++++++++++-------- > > 5 files changed, 45 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > index d450e16c5c25..bc77b0f465fd 100644 > > --- a/drivers/vhost/vhost.c > > +++ b/drivers/vhost/vhost.c > > @@ -1244,9 +1244,9 @@ static int vhost_iotlb_miss(struct vhost_virtqueue *vq, u64 iova, int access) > > } > > static bool vq_access_ok(struct vhost_virtqueue *vq, 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) > > { > > return access_ok(desc, vhost_get_desc_size(vq, num)) && > > @@ -2301,7 +2301,7 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq, > > struct vring_used_elem *heads, > > unsigned count) > > { > > - struct vring_used_elem __user *used; > > + vring_used_elem_t __user *used; > > u16 old, new; > > int start; > > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h > > index f8403bd46b85..60cab4c78229 100644 > > --- a/drivers/vhost/vhost.h > > +++ b/drivers/vhost/vhost.h > > @@ -67,9 +67,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 file *kick; > > struct eventfd_ctx *call_ctx; > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c > > index ba8e0d6cfd97..e059a9a47cdf 100644 > > --- a/drivers/vhost/vringh.c > > +++ b/drivers/vhost/vringh.c > > @@ -620,9 +620,9 @@ static inline int xfer_to_user(const struct vringh *vrh, > > */ > > int vringh_init_user(struct vringh *vrh, u64 features, > > unsigned int num, bool weak_barriers, > > - 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) > > { > > /* Sane power of 2 please! */ > > if (!num || num > 0xffff || (num & (num - 1))) { > > diff --git a/include/linux/vringh.h b/include/linux/vringh.h > > index 9e2763d7c159..59bd50f99291 100644 > > --- a/include/linux/vringh.h > > +++ b/include/linux/vringh.h > > @@ -105,9 +105,9 @@ struct vringh_kiov { > > /* Helpers for userspace vrings. */ > > int vringh_init_user(struct vringh *vrh, u64 features, > > unsigned int num, bool weak_barriers, > > - 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); > > static inline void vringh_iov_init(struct vringh_iov *iov, > > struct iovec *iovec, unsigned num) > > diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h > > index 9223c3a5c46a..b2c20f794472 100644 > > --- a/include/uapi/linux/virtio_ring.h > > +++ b/include/uapi/linux/virtio_ring.h > > @@ -86,6 +86,13 @@ > > * at the end of the used ring. Guest should ignore the used->flags field. */ > > #define VIRTIO_RING_F_EVENT_IDX 29 > > +/* Alignment requirements for vring elements. > > + * When using pre-virtio 1.0 layout, these fall out naturally. > > + */ > > +#define VRING_AVAIL_ALIGN_SIZE 2 > > +#define VRING_USED_ALIGN_SIZE 4 > > +#define VRING_DESC_ALIGN_SIZE 16 > > + > > /* Virtio ring descriptors: 16 bytes. These can chain together via "next". */ > > struct vring_desc { > > /* Address (guest-physical). */ > > @@ -112,29 +119,43 @@ struct vring_used_elem { > > __virtio32 len; > > }; > > +typedef struct vring_used_elem __aligned(VRING_USED_ALIGN_SIZE) > > + vring_used_elem_t; > > + > > struct vring_used { > > __virtio16 flags; > > __virtio16 idx; > > - struct vring_used_elem ring[]; > > + vring_used_elem_t ring[]; > > }; > > +/* > > + * 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 __aligned(VRING_DESC_ALIGN_SIZE) vring_desc_t; > > +typedef struct vring_avail __aligned(VRING_AVAIL_ALIGN_SIZE) vring_avail_t; > > +typedef struct vring_used __aligned(VRING_USED_ALIGN_SIZE) vring_used_t; > > > I wonder whether we can simply use __attribute__(packed) instead? > > ThanksPacked is 1 byte alignment. As such generates very bad code for accesses.> > > + > > struct vring { > > unsigned int num; > > - struct vring_desc *desc; > > + vring_desc_t *desc; > > - struct vring_avail *avail; > > + vring_avail_t *avail; > > - struct vring_used *used; > > + vring_used_t *used; > > }; > > -/* Alignment requirements for vring elements. > > - * When using pre-virtio 1.0 layout, these fall out naturally. > > - */ > > -#define VRING_AVAIL_ALIGN_SIZE 2 > > -#define VRING_USED_ALIGN_SIZE 4 > > -#define VRING_DESC_ALIGN_SIZE 16 > > - > > #ifndef VIRTIO_RING_NO_LEGACY > > /* The standard layout for the ring is a continuous chunk of memory which looks
Possibly Parallel Threads
- [PATCH v3] virtio: force spec specified alignment on types
- [PATCH v4] virtio: force spec specified alignment on types
- [PATCH v4] virtio: force spec specified alignment on types
- [PATCH] vhost: force spec specified alignment on types
- [PATCH] vhost: force spec specified alignment on types