linux-kernel - Re: [patch 00/37] PNP resource

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]
Message-ID: <47F4FDCB.2000803@keyaccess.nl>
Date:	Thu, 03 Apr 2008 17:54:51 +0200
From:	Rene Herman <rene.herman@...access.nl>
To:	Bjorn Helgaas <bjorn.helgaas@...com>
CC:	Rene Herman <rene.herman@...il.com>, Len Brown <lenb@...nel.org>,
	linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
	Adam Belay <ambx1@....rr.com>,
	Li Shaohua <shaohua.li@...el.com>,
	Matthieu Castet <castet.matthieu@...e.fr>,
	Thomas Renninger <trenn@...e.de>,
	Jaroslav Kysela <perex@...ex.cz>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [patch 00/37] PNP resource_table cleanups, v2

On 02-04-08 23:35, Bjorn Helgaas wrote:

>>    By the way, the original isapnp_parse_id explicitly encodes the top _6_
>>    bits in str[0] (& 0x3f) which seems odd. Bit 31 had better be 0 indeed,
>>    but I wonder why the original didn't just assume such.
> 
> Yes, I wonder about that, too.  Including that bit would mean that
> the first character of PNP IDs could include characters at offsets
> 0x20-0x3f, i.e., "`a..z{|}~" and DEL.  I poked around and found
> some IDs that seem to depend on that, e.g., "nEC8241" in the 8250_pnp
> serial driver.

Oh well, PC hardware...

> I changed this to include six bits for the first character, and
> masked off the top bit in PNPBIOS.  I think that should preserve
> the previous behavior; see what you think.

Yes, it should. I checked the ISAPnP specification and it explicitly fixes 
bit 31 at 0 (and defines the "compressed ASCII" as 5 bits). Given what you 
describe you probably don't have a good place to stash a comment but with 6 
bits being non-spec something like "appease broken ISAPnP hardware" would 
probably be good.

>>    2: There are 4 tests for ACPI_READ_WRITE_MEMORY here which are turned
>>       into IORESOURCE_MEM_WRITEABLE or 0. Not sure, but should they be
>>       turned into IORESOURCE_MEM_WRITEABLE or IORESOURCE_READONLY?
> 
> One would expect that "mem.write_protect == 1" would mean read-only.
> Unfortunately, I'm too lazy to un-obfuscate the ACPI CA logic that
> deals with mem.write_protect, since it seems to be all table-driven.
> In the absence of understanding, I tried to preserve the existing
> behavior.  I think I did, i.e., if "write_protect == ACPI_READ_WRITE_MEMORY",
> we add in IORESOURCE_MEM_WRITEABLE, otherwise do nothing.  If I
> goofed that up, let me know.

No, you didn't, is fine.

> I have an ISA question here, too: previously isapnp_read_resources()
> set only res.start for IO and MMIO resources and left res.end unset
> (should be zero, I think).  I don't think ISA tells you the size, so
> I assumed "1", but I don't know if that's the right thing to do.  My
> reasoning was "zero is obviously wrong, two could be too big and
> generate bogus conflicts, so one is the only possible choice."

Yes, as far as I'm aware the actual value is of no consequence. The size is 
not a setable parameter; to hardware they're only base address registers, It 
used to be kept simply at -1 (in an unsigned sort of way) and as far as I'm 
aware, we're also not interested yet at this level.

However, now that you made me look closer and in context -- there's actually 
a possibly somewhat serious problem here.

isapnp_read_resources() stores the resources as read from the hardware at 
the index in the table that matches the actual index in the hardware and 
isapnp_set_resources() stores them back into those same hardware indices.

Now by using pnp_add_foo_resource() which just scans for the first _UNSET 
resource, the resources might not end up in the same linear position in 
table/list if intermediate resources were unset in hardware (!ret). A 
subsequent isapnp_set_resources() would them restore the value to the wrong 
hardware index.

The IORESOURCE_ flags currently reserve too few bits (IORESOURCE_BITS,  8) 
to be able to store the hardware index: IORESOURCE_MEM and IORESOURCE_DMA 
need 2 and 1 respectively and there are 1 and 0 available respectively. It's 
ofcourse possible to hijack a few more bits in IORESOURCE_ flags but you're 
turning this into a list. I suppose the idea is to make it a simple list of 
struct resource, but perhaps a resource-private "driver_data" sort of field 
comes in handy for more than this already? Swiping more of IORESOURCE_ is a 
bit ugly...

In any case, I missed this, but ISAPnP is still (at least in principle) 
broken with the current set therefore.

> I also made the _len() functions inlines and restructured the logic in
> both _len() and _valid() functions.  I couldn't stand the thought of all
> those extra list traversals in there :-)  I'd appreciate a double-check
> of that.

Will do.

Rene.
--
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/