linux-kernel - RE: [PATCH] vfio/type1: fix cap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BN9PR11MB5276CEA18E764411F6E755D98C08A@BN9PR11MB5276.namprd11.prod.outlook.com>
Date:   Thu, 3 Aug 2023 05:19:04 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     Stefan Hajnoczi <stefanha@...hat.com>,
        Alex Williamson <alex.williamson@...hat.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>
Subject: RE: [PATCH] vfio/type1: fix cap_migration information leak

> From: Stefan Hajnoczi <stefanha@...hat.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@...hat.com>

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

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