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] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 3 Mar 2008 11:12:27 -0800
From:	Jesse Barnes <jbarnes@...tuousgeek.org>
To:	benh@...nel.crashing.org
Cc:	linux-pci@...ey.karlin.mff.cuni.cz,
	Linux Kernel list <linux-kernel@...r.kernel.org>,
	Matthew Wilcox <matthew@....cx>
Subject: Re: Weirdness in pci_read_bases()

On Thursday, February 28, 2008 6:37 pm Benjamin Herrenschmidt wrote:
> Hi !
>
> There is something dodgy going on in pci_read_bases().
>
> In addition to the fact (or related to it) that we tend to treat
> res->start == 0 as meaning "unassigned" (which at this stage is mostly
> incorrect, but let's say I can cope),

Yeah, that sounds like trouble.  I expect this may become a problem even for 
PC architectures in the future (IOMMUs on multiple busses for example), so 
maybe it makes sense to change the core to not assume 0 == unassigned?

> > however, there is some bit of code  
> that I think is just plain wrong:
>
> 		reg = PCI_BASE_ADDRESS_0 + (pos << 2);
> 		pci_read_config_dword(dev, reg, &l);
> 		pci_write_config_dword(dev, reg, ~0);
> 		pci_read_config_dword(dev, reg, &sz);
> 		pci_write_config_dword(dev, reg, l);
> 		if (!sz || sz == 0xffffffff)
> 			continue;
> 		if (l == 0xffffffff)
> 			l = 0;
>
> So here, we read the BAR value, which at this stage could either be some
> assigned address or some ff's from a freshly unassigned BAR. _however_
> that test against 0xffffffff looks totally bogus to me.
>
> If we read that value, we write 0 in l. Which means that if we read some
> unassigned resource that had the address space bits set to IO, we
> overwrite this with a bit encoding that means Memory, and further along,
> start sizing & treating this as a memory resource.

But we should never hit the l == 0xffffffff case if it's a memory BAR, since 
the low bit will be 0, right?

I think Grant is probably right that the only time we'd hit this is if the bus 
is down and we're getting back all 1s for every read (though that's a PC 
specific assumption).

> This isn't a problem I'm encountering in practice. I just noticed that
> while trying to audit what it would take to fix the code to stop using
> res->start = 0 as a way to mark unassigned resources (unrelated), so it
> doesn't -need- to be fixed per-se, but I'm curious where that came from
> in the first place.

Yeah, it seems like it could be safely removed, but this is an area where we 
should probably move slowly (i.e. one, very small change to the probing code 
at a time) or it may be difficult to isolate any bugs in the wild.

Jesse
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists