linux-kernel - Re: [PATCH] vfio/pci: Sanity check IGD OpRegion Size

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] [day] [month] [year] [list]

Date:   Thu, 13 May 2021 08:43:59 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     tkffaul@...look.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Yuan Yao <yuan.yao@...ux.intel.com>
Subject: Re: [PATCH] vfio/pci: Sanity check IGD OpRegion Size

On Mon, 10 May 2021 07:12:46 +0100
Christoph Hellwig <hch@...radead.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