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
