Re: [PATCH v2 2/3] vfio: use __aligned_u64 in struct vfio_device_gfx_plane_info

[Stefan Hajnoczi]

On Fri, Sep 15, 2023 at 02:04:58PM -0600, Alex Williamson wrote:
> On Tue, 29 Aug 2023 14:27:19 -0400
> Stefan Hajnoczi <stefanha@xxxxxxxxxx> wrote:
> 
> > The memory layout of struct vfio_device_gfx_plane_info is
> > architecture-dependent due to a u64 field and a struct size that is not
> > a multiple of 8 bytes:
> > - On x86_64 the struct size is padded to a multiple of 8 bytes.
> > - On x32 the struct size is only a multiple of 4 bytes, not 8.
> > - Other architectures may vary.
> > 
> > Use __aligned_u64 to make memory layout consistent. This reduces the
> > chance of 32-bit userspace on a 64-bit kernel breakage.
> > 
> > This patch increases the struct size on x32 but this is safe because of
> > the struct's argsz field. The kernel may grow the struct as long as it
> > still supports smaller argsz values from userspace (e.g. applications
> > compiled against older kernel headers).
> > 
> > Suggested-by: Jason Gunthorpe <jgg@xxxxxxxx>
> > Signed-off-by: Stefan Hajnoczi <stefanha@xxxxxxxxxx>
> > ---
> >  include/uapi/linux/vfio.h        | 3 ++-
> >  drivers/gpu/drm/i915/gvt/kvmgt.c | 4 +++-
> >  samples/vfio-mdev/mbochs.c       | 6 ++++--
> >  samples/vfio-mdev/mdpy.c         | 4 +++-
> >  4 files changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 94007ca348ed..777374dd7725 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -816,7 +816,7 @@ struct vfio_device_gfx_plane_info {
> >  	__u32 drm_plane_type;	/* type of plane: DRM_PLANE_TYPE_* */
> >  	/* out */
> >  	__u32 drm_format;	/* drm format of plane */
> > -	__u64 drm_format_mod;   /* tiled mode */
> > +	__aligned_u64 drm_format_mod;   /* tiled mode */
> >  	__u32 width;	/* width of plane */
> >  	__u32 height;	/* height of plane */
> >  	__u32 stride;	/* stride of plane */
> > @@ -829,6 +829,7 @@ struct vfio_device_gfx_plane_info {
> >  		__u32 region_index;	/* region index */
> >  		__u32 dmabuf_id;	/* dma-buf id */
> >  	};
> > +	__u32 reserved;
> >  };
> >  
> >  #define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14)
> > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > index 9cd9e9da60dd..813cfef23453 100644
> > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > @@ -1382,7 +1382,7 @@ static long intel_vgpu_ioctl(struct vfio_device *vfio_dev, unsigned int cmd,
> >  		intel_gvt_reset_vgpu(vgpu);
> >  		return 0;
> >  	} else if (cmd == VFIO_DEVICE_QUERY_GFX_PLANE) {
> > -		struct vfio_device_gfx_plane_info dmabuf;
> > +		struct vfio_device_gfx_plane_info dmabuf = {};
> >  		int ret = 0;
> >  
> >  		minsz = offsetofend(struct vfio_device_gfx_plane_info,
> > @@ -1392,6 +1392,8 @@ static long intel_vgpu_ioctl(struct vfio_device *vfio_dev, unsigned int cmd,
> >  		if (dmabuf.argsz < minsz)
> >  			return -EINVAL;
> >  
> > +		minsz = min(dmabuf.argsz, sizeof(dmabuf));
> > +
> >  		ret = intel_vgpu_query_plane(vgpu, &dmabuf);
> >  		if (ret != 0)
> >  			return ret;
> > diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
> > index 3764d1911b51..78aa977ae597 100644
> > --- a/samples/vfio-mdev/mbochs.c
> > +++ b/samples/vfio-mdev/mbochs.c
> > @@ -1262,7 +1262,7 @@ static long mbochs_ioctl(struct vfio_device *vdev, unsigned int cmd,
> >  
> >  	case VFIO_DEVICE_QUERY_GFX_PLANE:
> >  	{
> > -		struct vfio_device_gfx_plane_info plane;
> > +		struct vfio_device_gfx_plane_info plane = {};
> >  
> >  		minsz = offsetofend(struct vfio_device_gfx_plane_info,
> >  				    region_index);
> > @@ -1273,11 +1273,13 @@ static long mbochs_ioctl(struct vfio_device *vdev, unsigned int cmd,
> >  		if (plane.argsz < minsz)
> >  			return -EINVAL;
> >  
> > +		outsz = min_t(unsigned long, plane.argsz, sizeof(plane));
> 
> Sorry, I'm struggling with why these two sample drivers use min_t()
> when passed the exact same args as kvmgt above which just uses min().

min() would work fine here, too.

> But more importantly I'm also confused why we need this at all.  The
> buffer we're copying to is provided by the user, so what's wrong with
> leaving the user provided reserved data?  Are we just trying to return
> a zero'd reserved field if argsz allows for it?
> 
> Any use of the reserved field other than as undefined data would need
> to be associated with a flags bit, so I don't think it's buying us
> anything to return it zero'd.  What am I missing?  Thanks,

I don't remember anymore and what you've described makes sense to me.
I'll remove this in the next revision.

Stefan

> 
> Alex
> 
> > +
> >  		ret = mbochs_query_gfx_plane(mdev_state, &plane);
> >  		if (ret)
> >  			return ret;
> >  
> > -		if (copy_to_user((void __user *)arg, &plane, minsz))
> > +		if (copy_to_user((void __user *)arg, &plane, outsz))
> >  			return -EFAULT;
> >  
> >  		return 0;
> > diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
> > index 064e1c0a7aa8..f5c2effc1cec 100644
> > --- a/samples/vfio-mdev/mdpy.c
> > +++ b/samples/vfio-mdev/mdpy.c
> > @@ -591,7 +591,7 @@ static long mdpy_ioctl(struct vfio_device *vdev, unsigned int cmd,
> >  
> >  	case VFIO_DEVICE_QUERY_GFX_PLANE:
> >  	{
> > -		struct vfio_device_gfx_plane_info plane;
> > +		struct vfio_device_gfx_plane_info plane = {};
> >  
> >  		minsz = offsetofend(struct vfio_device_gfx_plane_info,
> >  				    region_index);
> > @@ -602,6 +602,8 @@ static long mdpy_ioctl(struct vfio_device *vdev, unsigned int cmd,
> >  		if (plane.argsz < minsz)
> >  			return -EINVAL;
> >  
> > +		minsz = min_t(unsigned long, plane.argsz, sizeof(plane));
> > +
> >  		ret = mdpy_query_gfx_plane(mdev_state, &plane);
> >  		if (ret)
> >  			return ret;
>

Attachment: signature.asc
Description: PGP signature