* Re: [PATCH] vfio/type1: fix cap_migration information leak
2023-08-01 15:53 [PATCH] vfio/type1: fix cap_migration information leak Stefan Hajnoczi
@ 2023-08-01 16:31 ` Alex Williamson
2023-08-01 19:23 ` Stefan Hajnoczi
2023-08-03 5:19 ` Tian, Kevin
2023-08-17 17:55 ` Alex Williamson
2 siblings, 1 reply; 5+ messages in thread
From: Alex Williamson @ 2023-08-01 16:31 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: linux-kernel, kvm
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;
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] vfio/type1: fix cap_migration information leak
2023-08-01 16:31 ` Alex Williamson
@ 2023-08-01 19:23 ` Stefan Hajnoczi
0 siblings, 0 replies; 5+ messages in thread
From: Stefan Hajnoczi @ 2023-08-01 19:23 UTC (permalink / raw)
To: Alex Williamson; +Cc: linux-kernel, kvm
[-- Attachment #1: Type: text/plain, Size: 2777 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] vfio/type1: fix cap_migration information leak
2023-08-01 15:53 [PATCH] vfio/type1: fix cap_migration information leak Stefan Hajnoczi
2023-08-01 16:31 ` Alex Williamson
@ 2023-08-03 5:19 ` Tian, Kevin
2023-08-17 17:55 ` Alex Williamson
2 siblings, 0 replies; 5+ messages in thread
From: Tian, Kevin @ 2023-08-03 5:19 UTC (permalink / raw)
To: Stefan Hajnoczi, Alex Williamson
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org
> 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>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] vfio/type1: fix cap_migration information leak
2023-08-01 15:53 [PATCH] vfio/type1: fix cap_migration information leak Stefan Hajnoczi
2023-08-01 16:31 ` Alex Williamson
2023-08-03 5:19 ` Tian, Kevin
@ 2023-08-17 17:55 ` Alex Williamson
2 siblings, 0 replies; 5+ messages in thread
From: Alex Williamson @ 2023-08-17 17:55 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: linux-kernel, kvm
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.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> drivers/vfio/vfio_iommu_type1.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied to vfio next branch for v6.6. I'll give Jason a little more
time to ack "[PATCH v3] vfio: align capability structures". Thanks!
Alex
>
> 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;
^ permalink raw reply [flat|nested] 5+ messages in thread