On Mon, Jan 23, 2017 at 08:44:07AM +0100, Alexander Gordeev wrote:
> Currently pci_bar_is_valid() returns the value stored
> in a BAR. That is wrong, because the function fails if
> the returned value is zero. But that is wrong, because:
> - an address in PIO address space may be zero;
> - an upper part of 64-bit BAR may be zero;
>
> The right way of validating a BAR is checking its size -
> for implemented BARs the size is non-zero.
>
> It might look strange, but the function checks not just the
> queried BAR, but sanity of all six BARs of the device.
>
> If we checked only requested BAR the function could have
> succeeded with inconsistent BAR sizes combinations. I.e.
> imagine a corrupted BARs reported by a device:
> BAR 0 - 64 bit lower
> BAR 1 - 64 bit lower
> BAR 2 - 64 bit upper
> ...
> If the function is asked about BAR 1 it would succeed, even
> though in fact the layout is ambiguous.
Is it possible to have bars messed up like this? I think I'd
just assume that sane layouts will exist and that we need to
avoid misinterpreting a high bar. So we just need to know a
high bar is a high bar, and in that case it's valid.
>
> As result, the function is expected to be called only with
> valid BAR numbers and BARs layout should be consistent.
> Otherwise, an assert is raised.
>
> The BAR number parameter bar_num is only allowed to refer
> a 32-bit BAR or lower part of a 64-bit BAR, as it does not
> make sense checking validity of high part of a 64-bit BAR.
> Thus, the caller is expected to know the device BARs layout.
I think the caller should be allowed to make this call on a
high bar.
>
> Cc: Thomas Huth <thuth@xxxxxxxxxx>
> Cc: Andrew Jones <drjones@xxxxxxxxxx>
> Signed-off-by: Alexander Gordeev <agordeev@xxxxxxxxxx>
> ---
> lib/pci.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/lib/pci.c b/lib/pci.c
> index fdd88296f0ae..00fba6e07860 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -117,7 +117,24 @@ bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)
>
> bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
> {
> - return pci_bar_get(dev, bar_num);
> + bool is64 = false;
> + int i;
> +
> + assert(bar_num >= 0 && bar_num < 6);
> +
> + for (i = 0; i < 6; i++) {
We have PCI_BAR_NUM now to use instead of 6, but don't you just want
to loop up to bar_num?
> + if (is64) {
> + assert(i == bar_num); /* high part of 64-bit BAR */
This assert looks wrong. Let's assume we want to check bar_num == 2,
but bar0 is 64-bit. In that case wouldn't you hit it?
> + assert(pci_bar_is64(dev, i));
Won't this assert always fire on the high bar?
> +
> + is64 = false;
> + } else {
> + is64 = pci_bar_is64(dev, i);
> + }
> + }
> + assert(!is64); /* incomplete 64-bit BAR */
> +
> + return pci_bar_size(dev, bar_num);
> }
>
> bool pci_bar_is64(pcidevaddr_t dev, int bar_num)
> --
> 1.8.3.1
>
How about just this instead (untested)
bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
{
assert(bar_num >= 0 && bar_num < PCI_BAR_NUM);
if (bar_num > 0 && pci_bar_is64(dev, bar_num - 1))
return pci_bar_is_valid(dev, bar_num - 1);
return pci_bar_size(dev, bar_num);
}
Thanks,
drew
