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:   Thu, 10 Aug 2023 10:24:20 -0400
From:   Stefan Hajnoczi <stefanha@redhat.com>
To:     "Tian, Kevin" <kevin.tian@intel.com>
Cc:     "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        Jason Gunthorpe <jgg@ziepe.ca>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Alex Williamson <alex.williamson@redhat.com>
Subject: Re: [PATCH 2/4] vfio: use __aligned_u64 in struct
 vfio_device_gfx_plane_info
Message-ID: <20230810142420.GC2931656@fedora>
References: <20230809210248.2898981-1-stefanha@redhat.com>
 <20230809210248.2898981-3-stefanha@redhat.com>
 <BN9PR11MB5276BC132EAE86D470B886528C13A@BN9PR11MB5276.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="O6bfz7OyLmHoLzjF"
Content-Disposition: inline
In-Reply-To: <BN9PR11MB5276BC132EAE86D470B886528C13A@BN9PR11MB5276.namprd11.prod.outlook.com>
Precedence: bulk


--O6bfz7OyLmHoLzjF
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Aug 10, 2023 at 03:22:56AM +0000, Tian, Kevin wrote:
> > From: Stefan Hajnoczi <stefanha@redhat.com>
> > Sent: Thursday, August 10, 2023 5:03 AM
> >=20
> > The memory layout of struct vfio_device_gfx_plane_info is
> > architecture-dependent due to a u64 field and a struct size that is not
> > a multiple of 8 bytes:
> > - On x86_64 the struct size is padded to a multiple of 8 bytes.
> > - On x32 the struct size is only a multiple of 4 bytes, not 8.
> > - Other architectures may vary.
> >=20
> > Use __aligned_u64 to make memory layout consistent. This reduces the
> > chance of holes that result in an information leak and the chance that
>=20
> I didn't quite get this. The leak example [1] from your earlier fix is re=
ally
> not caused by the use of __u64. Instead it's a counter example that on
> x32 there is no hole with 4byte alignment for __u64.
>=20
> I'd remove the hole part and just keep the compat reason.
>=20
> [1] https://lore.kernel.org/lkml/20230801103114.757d7992.alex.williamson@=
redhat.com/T/

Okay.

>=20
> > @@ -1392,6 +1392,8 @@ static long intel_vgpu_ioctl(struct vfio_device
> > *vfio_dev, unsigned int cmd,
> >  		if (dmabuf.argsz < minsz)
> >  			return -EINVAL;
> >=20
> > +		minsz =3D min(minsz, sizeof(dmabuf));
> > +
>=20
> Is there a case where minsz could be greater than sizeof(dmabuf)?

Thanks for spotting this, it's a bug in the patch. It should be
min(dmabuf.argsz, sizeof(dmabuf)).

Stefan

--O6bfz7OyLmHoLzjF
Content-Type: application/pgp-signature; name="signature.asc"

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

iQEzBAEBCAAdFiEEhpWov9P5fNqsNXdanKSrs4Grc8gFAmTU8xQACgkQnKSrs4Gr
c8jKDgf/RLZgbtwtI72CjE2KiK0+kuRGCyKg6g5IlhEVfIhGpdr+MMHc4q2uUi6b
2pXvDCegcThH0hDv66q2HW9pGdv6CKSTuUuCv0TpKqocxTMImMTWPxBIsN7oslpJ
oOxe3PPzC2jtFQyZkwmsP2yjekvq0e6tPOmaYJMFflBnR1o3SncsLP/Z3TyInMQr
Sron45lvEXLQAH/xHjguvNzACgVLZiJB0/weTWJqi03KEqFw2/YSxX85M1yRLYhp
e/Lfk7FSBAYoNKUXm6pJM/ULEKTbxkH4zb1JwejCkzJ4PPYXsl/MLZZhXnNFPvJ0
/oDPG/KSh9YMo6iS2wQ2Fl8yN50BBQ==
=ESeR
-----END PGP SIGNATURE-----

--O6bfz7OyLmHoLzjF--