| 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
| ||
|
Message-ID: <a4939aeb-ed9d-a6af-1c70-c6c2513e86e2@redhat.com>
Date: Tue, 21 Apr 2020 10:39:19 +0800
From: Jason Wang <jasowang@...hat.com>
To: "Michael S. Tsirkin" <mst@...hat.com>, linux-kernel@...r.kernel.org
Cc: kvm@...r.kernel.org, virtualization@...ts.linux-foundation.org,
netdev@...r.kernel.org
Subject: Re: [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@...hat.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
Powered by blists - more mailing lists