vfio/type1: fix cap_migration information leak

  Fix an information leak where an uninitialized hole in struct
vfio_iommu_type1_info_cap_migration on the stack is exposed to userspace.

The definition of struct vfio_iommu_type1_info_cap_migration contains a hole as
shown in this pahole(1) output:

  struct vfio_iommu_type1_info_cap_migration {
          struct vfio_info_cap_header header;              /*     0     8 */
          __u32                      flags;                /*     8     4 */

          /* XXX 4 bytes hole, try to pack */

          __u64                      pgsize_bitmap;        /*    16     8 */
          __u64                      max_dirty_bitmap_size; /*    24     8 */

          /* size: 32, cachelines: 1, members: 4 */
          /* sum members: 28, holes: 1, sum holes: 4 */
          /* last cacheline: 32 bytes */
  };

The cap_mig variable is filled in without initializing the hole:

  static int vfio_iommu_migration_build_caps(struct vfio_iommu *iommu,
                         struct vfio_info_cap *caps)
  {
      struct vfio_iommu_type1_info_cap_migration cap_mig;

      cap_mig.header.id = VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION;
      cap_mig.header.version = 1;

      cap_mig.flags = 0;
      /* support minimum pgsize */
      cap_mig.pgsize_bitmap = (size_t)1 << __ffs(iommu->pgsize_bitmap);
      cap_mig.max_dirty_bitmap_size = DIRTY_BITMAP_SIZE_MAX;

      return vfio_info_add_capability(caps, &cap_mig.header, sizeof(cap_mig));
  }

The structure is then copied to a temporary location on the heap. At this point
it's already too late and ioctl(VFIO_IOMMU_GET_INFO) copies it to userspace
later:

  int vfio_info_add_capability(struct vfio_info_cap *caps,
                   struct vfio_info_cap_header *cap, size_t size)
  {
      struct vfio_info_cap_header *header;

      header = vfio_info_cap_add(caps, size, cap->id, cap->version);
      if (IS_ERR(header))
          return PTR_ERR(header);

      memcpy(header + 1, cap + 1, size - sizeof(*header));

      return 0;
  }

This issue was found by code inspection.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 drivers/vfio/vfio_iommu_type1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

On Tue,  1 Aug 2023 11:53:52 -0400
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> Fix an information leak where an uninitialized hole in struct
> vfio_iommu_type1_info_cap_migration on the stack is exposed to userspace.
> 
> The definition of struct vfio_iommu_type1_info_cap_migration contains a hole as
> shown in this pahole(1) output:
> 
>   struct vfio_iommu_type1_info_cap_migration {
>           struct vfio_info_cap_header header;              /*     0     8 */
>           __u32                      flags;                /*     8     4 */
> 
>           /* XXX 4 bytes hole, try to pack */
> 
>           __u64                      pgsize_bitmap;        /*    16     8 */
>           __u64                      max_dirty_bitmap_size; /*    24     8 */
> 
>           /* size: 32, cachelines: 1, members: 4 */
>           /* sum members: 28, holes: 1, sum holes: 4 */
>           /* last cacheline: 32 bytes */
>   };
> 
> The cap_mig variable is filled in without initializing the hole:
> 
>   static int vfio_iommu_migration_build_caps(struct vfio_iommu *iommu,
>                          struct vfio_info_cap *caps)
>   {
>       struct vfio_iommu_type1_info_cap_migration cap_mig;
> 
>       cap_mig.header.id = VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION;
>       cap_mig.header.version = 1;
> 
>       cap_mig.flags = 0;
>       /* support minimum pgsize */
>       cap_mig.pgsize_bitmap = (size_t)1 << __ffs(iommu->pgsize_bitmap);
>       cap_mig.max_dirty_bitmap_size = DIRTY_BITMAP_SIZE_MAX;
> 
>       return vfio_info_add_capability(caps, &cap_mig.header, sizeof(cap_mig));
>   }
> 
> The structure is then copied to a temporary location on the heap. At this point
> it's already too late and ioctl(VFIO_IOMMU_GET_INFO) copies it to userspace
> later:
> 
>   int vfio_info_add_capability(struct vfio_info_cap *caps,
>                    struct vfio_info_cap_header *cap, size_t size)
>   {
>       struct vfio_info_cap_header *header;
> 
>       header = vfio_info_cap_add(caps, size, cap->id, cap->version);
>       if (IS_ERR(header))
>           return PTR_ERR(header);
> 
>       memcpy(header + 1, cap + 1, size - sizeof(*header));
> 
>       return 0;
>   }
> 
> This issue was found by code inspection.

LGTM, but missing:

Fixes: ad721705d09c ("vfio iommu: Add migration capability to report supported features")

I'll give a bit for further comments/reviews and queue it for v6.6 with
the above update.  Thanks,

Alex


> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index ebe0ad31d0b0..d662aa9d1b4b 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -2732,7 +2732,7 @@ static int vfio_iommu_iova_build_caps(struct vfio_iommu *iommu,
>  static int vfio_iommu_migration_build_caps(struct vfio_iommu *iommu,
>  					   struct vfio_info_cap *caps)
>  {
> -	struct vfio_iommu_type1_info_cap_migration cap_mig;
> +	struct vfio_iommu_type1_info_cap_migration cap_mig = {};
>  
>  	cap_mig.header.id = VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION;
>  	cap_mig.header.version = 1;
  

On Tue, Aug 01, 2023 at 10:31:14AM -0600, Alex Williamson wrote:
> On Tue,  1 Aug 2023 11:53:52 -0400
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > Fix an information leak where an uninitialized hole in struct
> > vfio_iommu_type1_info_cap_migration on the stack is exposed to userspace.
> > 
> > The definition of struct vfio_iommu_type1_info_cap_migration contains a hole as
> > shown in this pahole(1) output:
> > 
> >   struct vfio_iommu_type1_info_cap_migration {
> >           struct vfio_info_cap_header header;              /*     0     8 */
> >           __u32                      flags;                /*     8     4 */
> > 
> >           /* XXX 4 bytes hole, try to pack */
> > 
> >           __u64                      pgsize_bitmap;        /*    16     8 */
> >           __u64                      max_dirty_bitmap_size; /*    24     8 */
> > 
> >           /* size: 32, cachelines: 1, members: 4 */
> >           /* sum members: 28, holes: 1, sum holes: 4 */
> >           /* last cacheline: 32 bytes */
> >   };
> > 
> > The cap_mig variable is filled in without initializing the hole:
> > 
> >   static int vfio_iommu_migration_build_caps(struct vfio_iommu *iommu,
> >                          struct vfio_info_cap *caps)
> >   {
> >       struct vfio_iommu_type1_info_cap_migration cap_mig;
> > 
> >       cap_mig.header.id = VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION;
> >       cap_mig.header.version = 1;
> > 
> >       cap_mig.flags = 0;
> >       /* support minimum pgsize */
> >       cap_mig.pgsize_bitmap = (size_t)1 << __ffs(iommu->pgsize_bitmap);
> >       cap_mig.max_dirty_bitmap_size = DIRTY_BITMAP_SIZE_MAX;
> > 
> >       return vfio_info_add_capability(caps, &cap_mig.header, sizeof(cap_mig));
> >   }
> > 
> > The structure is then copied to a temporary location on the heap. At this point
> > it's already too late and ioctl(VFIO_IOMMU_GET_INFO) copies it to userspace
> > later:
> > 
> >   int vfio_info_add_capability(struct vfio_info_cap *caps,
> >                    struct vfio_info_cap_header *cap, size_t size)
> >   {
> >       struct vfio_info_cap_header *header;
> > 
> >       header = vfio_info_cap_add(caps, size, cap->id, cap->version);
> >       if (IS_ERR(header))
> >           return PTR_ERR(header);
> > 
> >       memcpy(header + 1, cap + 1, size - sizeof(*header));
> > 
> >       return 0;
> >   }
> > 
> > This issue was found by code inspection.
> 
> LGTM, but missing:
> 
> Fixes: ad721705d09c ("vfio iommu: Add migration capability to report supported features")
> 
> I'll give a bit for further comments/reviews and queue it for v6.6 with
> the above update.  Thanks,

Great, thanks for squashing in the "Fixes" line that I forgot.

Stefan
  

> From: Stefan Hajnoczi <stefanha@redhat.com>
> Sent: Tuesday, August 1, 2023 11:54 PM
> 
> Fix an information leak where an uninitialized hole in struct
> vfio_iommu_type1_info_cap_migration on the stack is exposed to userspace.
> 
> The definition of struct vfio_iommu_type1_info_cap_migration contains a
> hole as
> shown in this pahole(1) output:
> 
>   struct vfio_iommu_type1_info_cap_migration {
>           struct vfio_info_cap_header header;              /*     0     8 */
>           __u32                      flags;                /*     8     4 */
> 
>           /* XXX 4 bytes hole, try to pack */
> 
>           __u64                      pgsize_bitmap;        /*    16     8 */
>           __u64                      max_dirty_bitmap_size; /*    24     8 */
> 
>           /* size: 32, cachelines: 1, members: 4 */
>           /* sum members: 28, holes: 1, sum holes: 4 */
>           /* last cacheline: 32 bytes */
>   };
> 
> The cap_mig variable is filled in without initializing the hole:
> 
>   static int vfio_iommu_migration_build_caps(struct vfio_iommu *iommu,
>                          struct vfio_info_cap *caps)
>   {
>       struct vfio_iommu_type1_info_cap_migration cap_mig;
> 
>       cap_mig.header.id = VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION;
>       cap_mig.header.version = 1;
> 
>       cap_mig.flags = 0;
>       /* support minimum pgsize */
>       cap_mig.pgsize_bitmap = (size_t)1 << __ffs(iommu->pgsize_bitmap);
>       cap_mig.max_dirty_bitmap_size = DIRTY_BITMAP_SIZE_MAX;
> 
>       return vfio_info_add_capability(caps, &cap_mig.header, sizeof(cap_mig));
>   }
> 
> The structure is then copied to a temporary location on the heap. At this
> point
> it's already too late and ioctl(VFIO_IOMMU_GET_INFO) copies it to userspace
> later:
> 
>   int vfio_info_add_capability(struct vfio_info_cap *caps,
>                    struct vfio_info_cap_header *cap, size_t size)
>   {
>       struct vfio_info_cap_header *header;
> 
>       header = vfio_info_cap_add(caps, size, cap->id, cap->version);
>       if (IS_ERR(header))
>           return PTR_ERR(header);
> 
>       memcpy(header + 1, cap + 1, size - sizeof(*header));
> 
>       return 0;
>   }
> 
> This issue was found by code inspection.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

nit. better also add a 'reserved' field in cap_migration to
mark the hole.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>