| Message ID | 20230801155352.1391945-1-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/type1: fix cap_migration information leak | expand |
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>
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;
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;
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(-)