Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 1 Aug 2023 15:23:45 -0400
From:   Stefan Hajnoczi <stefanha@redhat.com>
To:     Alex Williamson <alex.williamson@redhat.com>
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [PATCH] vfio/type1: fix cap_migration information leak
Message-ID: <20230801192345.GA1414936@fedora>
References: <20230801155352.1391945-1-stefanha@redhat.com>
 <20230801103114.757d7992.alex.williamson@redhat.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="0CjF++Sq7XzfS4wJ"
Content-Disposition: inline
In-Reply-To: <20230801103114.757d7992.alex.williamson@redhat.com>
Precedence: bulk


--0CjF++Sq7XzfS4wJ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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:
>=20
> > Fix an information leak where an uninitialized hole in struct
> > vfio_iommu_type1_info_cap_migration on the stack is exposed to userspac=
e.
> >=20
> > The definition of struct vfio_iommu_type1_info_cap_migration contains a=
 hole as
> > shown in this pahole(1) output:
> >=20
> >   struct vfio_iommu_type1_info_cap_migration {
> >           struct vfio_info_cap_header header;              /*     0    =
 8 */
> >           __u32                      flags;                /*     8    =
 4 */
> >=20
> >           /* XXX 4 bytes hole, try to pack */
> >=20
> >           __u64                      pgsize_bitmap;        /*    16    =
 8 */
> >           __u64                      max_dirty_bitmap_size; /*    24   =
  8 */
> >=20
> >           /* size: 32, cachelines: 1, members: 4 */
> >           /* sum members: 28, holes: 1, sum holes: 4 */
> >           /* last cacheline: 32 bytes */
> >   };
> >=20
> > The cap_mig variable is filled in without initializing the hole:
> >=20
> >   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;
> >=20
> >       cap_mig.header.id =3D VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION;
> >       cap_mig.header.version =3D 1;
> >=20
> >       cap_mig.flags =3D 0;
> >       /* support minimum pgsize */
> >       cap_mig.pgsize_bitmap =3D (size_t)1 << __ffs(iommu->pgsize_bitmap=
);
> >       cap_mig.max_dirty_bitmap_size =3D DIRTY_BITMAP_SIZE_MAX;
> >=20
> >       return vfio_info_add_capability(caps, &cap_mig.header, sizeof(cap=
_mig));
> >   }
> >=20
> > The structure is then copied to a temporary location on the heap. At th=
is point
> > it's already too late and ioctl(VFIO_IOMMU_GET_INFO) copies it to users=
pace
> > later:
> >=20
> >   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;
> >=20
> >       header =3D vfio_info_cap_add(caps, size, cap->id, cap->version);
> >       if (IS_ERR(header))
> >           return PTR_ERR(header);
> >=20
> >       memcpy(header + 1, cap + 1, size - sizeof(*header));
> >=20
> >       return 0;
> >   }
> >=20
> > This issue was found by code inspection.
>=20
> LGTM, but missing:
>=20
> Fixes: ad721705d09c ("vfio iommu: Add migration capability to report supp=
orted features")
>=20
> 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

--0CjF++Sq7XzfS4wJ
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEhpWov9P5fNqsNXdanKSrs4Grc8gFAmTJW8EACgkQnKSrs4Gr
c8gIVAf8DE2gcidYmywXZWL207MyVOVVJNJ59lICK3bNWX3DfaBlXF67fKU8zzZM
2Mi55gSKvhPLcluyAiSuLnKlB00TKRATlrBTxXtG4q5Ps6SxbE+tzc0KNZVvSNTl
9eK+at3FddLX0nbLKO/rkNFcK74rP20crr6W/Hy6SnHAI8KRYd3LO7+bfg+kgq8g
a0AeCk5iwL73lEJrXZaVo6loDBq7H0JUoupt5j87HtO+XYXRzfFZ/vCci0LyEFI4
n+WO3DUXGALN4Z4rUyDaG0oZkC30sz7hmsP71FtT4uEAqeY8Qi3BufVFM2IghXT4
FKOs9Z49Jw7JfYDp1EhkbdeAas9nmg==
=e8Ic
-----END PGP SIGNATURE-----

--0CjF++Sq7XzfS4wJ--