kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] vfio/pci: Sanity check IGD OpRegion Size
@ 2021-05-07 18:53 Alex Williamson
  2021-05-08  4:50 ` Zhenyu Wang
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Alex Williamson @ 2021-05-07 18:53 UTC (permalink / raw)
  To: alex.williamson; +Cc: tkffaul, kvm, linux-kernel

The size field of the IGD OpRegion table is supposed to indicate table
size in KB, but we've seen at least one report of a BIOS that appears
to incorrectly report size in bytes.  The default size is 8 (*1024 =
8KB), but an incorrect implementation may report 8192 (*1024 = 8MB)
and can cause a variety of mapping errors.

It's believed that 8MB would be an implausible, if not absurd, actual
size, so we can probably be pretty safe in assuming this is a BIOS bug
where the intended size is likely 8KB.

Reported-by: Travis Faulhaber <tkffaul@outlook.com>
Tested-by: Travis Faulhaber <tkffaul@outlook.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/vfio/pci/vfio_pci_igd.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/pci/vfio_pci_igd.c b/drivers/vfio/pci/vfio_pci_igd.c
index 228df565e9bc..c89a4797cd18 100644
--- a/drivers/vfio/pci/vfio_pci_igd.c
+++ b/drivers/vfio/pci/vfio_pci_igd.c
@@ -86,7 +86,16 @@ static int vfio_pci_igd_opregion_init(struct vfio_pci_device *vdev)
 		return -EINVAL;
 	}
 
-	size *= 1024; /* In KB */
+	/*
+	 * The OpRegion size field is specified as size in KB, but there have been
+	 * user reports where this field appears to report size in bytes.  If we
+	 * read 8192, assume this is the case.
+	 */
+	if (size == OPREGION_SIZE)
+		pci_warn(vdev->pdev,
+			 "BIOS Bug, IGD OpRegion reports invalid size, assuming default 8KB\n");
+	else
+		size *= 1024; /* In KB */
 
 	/*
 	 * Support opregion v2.1+



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] vfio/pci: Sanity check IGD OpRegion Size
  2021-05-07 18:53 [PATCH] vfio/pci: Sanity check IGD OpRegion Size Alex Williamson
@ 2021-05-08  4:50 ` Zhenyu Wang
  2021-05-10  1:10 ` Yuan Yao
  2021-05-10  6:12 ` Christoph Hellwig
  2 siblings, 0 replies; 7+ messages in thread
From: Zhenyu Wang @ 2021-05-08  4:50 UTC (permalink / raw)
  To: Alex Williamson; +Cc: tkffaul, kvm, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1750 bytes --]

On 2021.05.07 12:53:17 -0600, Alex Williamson wrote:
> The size field of the IGD OpRegion table is supposed to indicate table
> size in KB, but we've seen at least one report of a BIOS that appears
> to incorrectly report size in bytes.  The default size is 8 (*1024 =
> 8KB), but an incorrect implementation may report 8192 (*1024 = 8MB)
> and can cause a variety of mapping errors.
> 
> It's believed that 8MB would be an implausible, if not absurd, actual
> size, so we can probably be pretty safe in assuming this is a BIOS bug
> where the intended size is likely 8KB.
> 
> Reported-by: Travis Faulhaber <tkffaul@outlook.com>
> Tested-by: Travis Faulhaber <tkffaul@outlook.com>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  drivers/vfio/pci/vfio_pci_igd.c |   11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_igd.c b/drivers/vfio/pci/vfio_pci_igd.c
> index 228df565e9bc..c89a4797cd18 100644
> --- a/drivers/vfio/pci/vfio_pci_igd.c
> +++ b/drivers/vfio/pci/vfio_pci_igd.c
> @@ -86,7 +86,16 @@ static int vfio_pci_igd_opregion_init(struct vfio_pci_device *vdev)
>  		return -EINVAL;
>  	}
>  
> -	size *= 1024; /* In KB */
> +	/*
> +	 * The OpRegion size field is specified as size in KB, but there have been
> +	 * user reports where this field appears to report size in bytes.  If we
> +	 * read 8192, assume this is the case.
> +	 */
> +	if (size == OPREGION_SIZE)
> +		pci_warn(vdev->pdev,
> +			 "BIOS Bug, IGD OpRegion reports invalid size, assuming default 8KB\n");
> +	else
> +		size *= 1024; /* In KB */
>  
>  	/*
>  	 * Support opregion v2.1+
> 

Reviewed-by: Zhenyu Wang <zhenyuw@linux.intel.com>

thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] vfio/pci: Sanity check IGD OpRegion Size
  2021-05-07 18:53 [PATCH] vfio/pci: Sanity check IGD OpRegion Size Alex Williamson
  2021-05-08  4:50 ` Zhenyu Wang
@ 2021-05-10  1:10 ` Yuan Yao
  2021-05-10  1:34   ` Alex Williamson
  2021-05-10  6:12 ` Christoph Hellwig
  2 siblings, 1 reply; 7+ messages in thread
From: Yuan Yao @ 2021-05-10  1:10 UTC (permalink / raw)
  To: Alex Williamson; +Cc: tkffaul, kvm, linux-kernel

On Fri, May 07, 2021 at 12:53:17PM -0600, Alex Williamson wrote:
> The size field of the IGD OpRegion table is supposed to indicate table
> size in KB, but we've seen at least one report of a BIOS that appears
> to incorrectly report size in bytes.  The default size is 8 (*1024 =
> 8KB), but an incorrect implementation may report 8192 (*1024 = 8MB)
> and can cause a variety of mapping errors.
> 
> It's believed that 8MB would be an implausible, if not absurd, actual
> size, so we can probably be pretty safe in assuming this is a BIOS bug
> where the intended size is likely 8KB.
> 
> Reported-by: Travis Faulhaber <tkffaul@outlook.com>
> Tested-by: Travis Faulhaber <tkffaul@outlook.com>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  drivers/vfio/pci/vfio_pci_igd.c |   11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_igd.c b/drivers/vfio/pci/vfio_pci_igd.c
> index 228df565e9bc..c89a4797cd18 100644
> --- a/drivers/vfio/pci/vfio_pci_igd.c
> +++ b/drivers/vfio/pci/vfio_pci_igd.c
> @@ -86,7 +86,16 @@ static int vfio_pci_igd_opregion_init(struct vfio_pci_device *vdev)
>  		return -EINVAL;
>  	}
>  
> -	size *= 1024; /* In KB */
> +	/*
> +	 * The OpRegion size field is specified as size in KB, but there have been
> +	 * user reports where this field appears to report size in bytes.  If we
> +	 * read 8192, assume this is the case.
> +	 */
> +	if (size == OPREGION_SIZE)

Is "size >= OPREGION_SIZE" or "size >= smaller but still implausible value
(like 4096)" better for covering more bad BIOS implementation cases ?

> +		pci_warn(vdev->pdev,
> +			 "BIOS Bug, IGD OpRegion reports invalid size, assuming default 8KB\n");
> +	else
> +		size *= 1024; /* In KB */
>  
>  	/*
>  	 * Support opregion v2.1+
> 
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] vfio/pci: Sanity check IGD OpRegion Size
  2021-05-10  1:10 ` Yuan Yao
@ 2021-05-10  1:34   ` Alex Williamson
  2021-05-10  3:14     ` Yuan Yao
  0 siblings, 1 reply; 7+ messages in thread
From: Alex Williamson @ 2021-05-10  1:34 UTC (permalink / raw)
  To: Yuan Yao; +Cc: tkffaul, kvm, linux-kernel

On Mon, 10 May 2021 09:10:14 +0800
Yuan Yao <yuan.yao@linux.intel.com> wrote:

> On Fri, May 07, 2021 at 12:53:17PM -0600, Alex Williamson wrote:
> > The size field of the IGD OpRegion table is supposed to indicate table
> > size in KB, but we've seen at least one report of a BIOS that appears
> > to incorrectly report size in bytes.  The default size is 8 (*1024 =
> > 8KB), but an incorrect implementation may report 8192 (*1024 = 8MB)
> > and can cause a variety of mapping errors.
> > 
> > It's believed that 8MB would be an implausible, if not absurd, actual
> > size, so we can probably be pretty safe in assuming this is a BIOS bug
> > where the intended size is likely 8KB.
> > 
> > Reported-by: Travis Faulhaber <tkffaul@outlook.com>
> > Tested-by: Travis Faulhaber <tkffaul@outlook.com>
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >  drivers/vfio/pci/vfio_pci_igd.c |   11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/vfio/pci/vfio_pci_igd.c b/drivers/vfio/pci/vfio_pci_igd.c
> > index 228df565e9bc..c89a4797cd18 100644
> > --- a/drivers/vfio/pci/vfio_pci_igd.c
> > +++ b/drivers/vfio/pci/vfio_pci_igd.c
> > @@ -86,7 +86,16 @@ static int vfio_pci_igd_opregion_init(struct vfio_pci_device *vdev)
> >  		return -EINVAL;
> >  	}
> >  
> > -	size *= 1024; /* In KB */
> > +	/*
> > +	 * The OpRegion size field is specified as size in KB, but there have been
> > +	 * user reports where this field appears to report size in bytes.  If we
> > +	 * read 8192, assume this is the case.
> > +	 */
> > +	if (size == OPREGION_SIZE)  
> 
> Is "size >= OPREGION_SIZE" or "size >= smaller but still implausible value
> (like 4096)" better for covering more bad BIOS implementation cases ?

We haven't seen such cases and it seems like a BIOS implementation
competent enough to use something other than the default size, probably
might get the units correct for this field.  Our footing for assuming
this specific implementation error gets shakier if we try to apply it
beyond the default size, imo.  Thanks,

Alex


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] vfio/pci: Sanity check IGD OpRegion Size
  2021-05-10  1:34   ` Alex Williamson
@ 2021-05-10  3:14     ` Yuan Yao
  0 siblings, 0 replies; 7+ messages in thread
From: Yuan Yao @ 2021-05-10  3:14 UTC (permalink / raw)
  To: Alex Williamson; +Cc: tkffaul, kvm, linux-kernel

On Sun, May 09, 2021 at 07:34:08PM -0600, Alex Williamson wrote:
> On Mon, 10 May 2021 09:10:14 +0800
> Yuan Yao <yuan.yao@linux.intel.com> wrote:
> 
> > On Fri, May 07, 2021 at 12:53:17PM -0600, Alex Williamson wrote:
> > > The size field of the IGD OpRegion table is supposed to indicate table
> > > size in KB, but we've seen at least one report of a BIOS that appears
> > > to incorrectly report size in bytes.  The default size is 8 (*1024 =
> > > 8KB), but an incorrect implementation may report 8192 (*1024 = 8MB)
> > > and can cause a variety of mapping errors.
> > > 
> > > It's believed that 8MB would be an implausible, if not absurd, actual
> > > size, so we can probably be pretty safe in assuming this is a BIOS bug
> > > where the intended size is likely 8KB.
> > > 
> > > Reported-by: Travis Faulhaber <tkffaul@outlook.com>
> > > Tested-by: Travis Faulhaber <tkffaul@outlook.com>
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > ---
> > >  drivers/vfio/pci/vfio_pci_igd.c |   11 ++++++++++-
> > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/vfio/pci/vfio_pci_igd.c b/drivers/vfio/pci/vfio_pci_igd.c
> > > index 228df565e9bc..c89a4797cd18 100644
> > > --- a/drivers/vfio/pci/vfio_pci_igd.c
> > > +++ b/drivers/vfio/pci/vfio_pci_igd.c
> > > @@ -86,7 +86,16 @@ static int vfio_pci_igd_opregion_init(struct vfio_pci_device *vdev)
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	size *= 1024; /* In KB */
> > > +	/*
> > > +	 * The OpRegion size field is specified as size in KB, but there have been
> > > +	 * user reports where this field appears to report size in bytes.  If we
> > > +	 * read 8192, assume this is the case.
> > > +	 */
> > > +	if (size == OPREGION_SIZE)  
> > 
> > Is "size >= OPREGION_SIZE" or "size >= smaller but still implausible value
> > (like 4096)" better for covering more bad BIOS implementation cases ?
> 
> We haven't seen such cases and it seems like a BIOS implementation
> competent enough to use something other than the default size, probably
> might get the units correct for this field.  Our footing for assuming
> this specific implementation error gets shakier if we try to apply it
> beyond the default size, imo.  Thanks,

OK, make sense to me, thanks.

> 
> Alex
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] vfio/pci: Sanity check IGD OpRegion Size
  2021-05-07 18:53 [PATCH] vfio/pci: Sanity check IGD OpRegion Size Alex Williamson
  2021-05-08  4:50 ` Zhenyu Wang
  2021-05-10  1:10 ` Yuan Yao
@ 2021-05-10  6:12 ` Christoph Hellwig
  2021-05-13 14:43   ` Alex Williamson
  2 siblings, 1 reply; 7+ messages in thread
From: Christoph Hellwig @ 2021-05-10  6:12 UTC (permalink / raw)
  To: Alex Williamson; +Cc: tkffaul, kvm, linux-kernel

On Fri, May 07, 2021 at 12:53:17PM -0600, Alex Williamson wrote:
> +	/*
> +	 * The OpRegion size field is specified as size in KB, but there have been
> +	 * user reports where this field appears to report size in bytes.  If we
> +	 * read 8192, assume this is the case.
> +	 */

Please avoid pointlesly spilling the comment line over 80 chars.


> +	if (size == OPREGION_SIZE)

Shouldn't this be a range tests, i.e. >= ?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] vfio/pci: Sanity check IGD OpRegion Size
  2021-05-10  6:12 ` Christoph Hellwig
@ 2021-05-13 14:43   ` Alex Williamson
  0 siblings, 0 replies; 7+ messages in thread
From: Alex Williamson @ 2021-05-13 14:43 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: tkffaul, kvm, linux-kernel, Yuan Yao

On Mon, 10 May 2021 07:12:46 +0100
Christoph Hellwig <hch@infradead.org> wrote:

> On Fri, May 07, 2021 at 12:53:17PM -0600, Alex Williamson wrote:
> > +	/*
> > +	 * The OpRegion size field is specified as size in KB, but there have been
> > +	 * user reports where this field appears to report size in bytes.  If we
> > +	 * read 8192, assume this is the case.
> > +	 */  
> 
> Please avoid pointlesly spilling the comment line over 80 chars.

Oops, I didn't notice I was using a wider terminal.  Fixed.

> > +	if (size == OPREGION_SIZE)  
> 
> Shouldn't this be a range tests, i.e. >= ?

My concern here is how far we go down the path of trying to figure out
what a sane size range is for this table an how/if we try to assume the
BIOS intentions.  The precise value of 8192 is not only absurdly large,
but happens to coincide with the default table size, so it seems likely
that we can infer this specific misinterpretation.  If the BIOS has
used a different value, suggesting they're trying to do something more
extensive than a basic implementation, but still managed to botch the
units for the size field, we should probably disregard it entirely.  We
can probably do that for smaller values as well, but I don't know where
the line between reasonable and absurd is crossed.

Would it make more sense to export e820__get_entry_type() so that we
can validate that the full range of the table fits within an e820
mapping, which I understand should be ACPI NVS in this case?  Thanks,

Alex


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-05-13 14:44 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-07 18:53 [PATCH] vfio/pci: Sanity check IGD OpRegion Size Alex Williamson
2021-05-08  4:50 ` Zhenyu Wang
2021-05-10  1:10 ` Yuan Yao
2021-05-10  1:34   ` Alex Williamson
2021-05-10  3:14     ` Yuan Yao
2021-05-10  6:12 ` Christoph Hellwig
2021-05-13 14:43   ` Alex Williamson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).