linux-kernel - Re: [RFC][Patch] IBM Real-Time "SMI Free" mode driver

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: <20090211100249.1ce0a170@lxorguk.ukuu.org.uk>
Date:	Wed, 11 Feb 2009 10:02:49 +0000
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	Keith Mannthey <kmannth@...ibm.com>
Cc:	lkml <linux-kernel@...r.kernel.org>,
	John Stultz <johnstul@...ibm.com>
Subject: Re: [RFC][Patch] IBM Real-Time "SMI Free" mode driver

> +	if (readb(rtl+RTL_STATE) != value) {
> +		if (value == 1)
> +			writeb(1, (rtl+RTL_CMD));
> +		else
> +			writeb(2, (rtl+RTL_CMD));

		writeb( 1 + value , ... ) ??

> +	rtl = ioremap(table_addr, RTL_TABLE_SIZE);
> +
> +	if (!rtl) {
> +		ret = -ENOMEM;
> +		goto err_out;

Would it be better to eliminate all the ioremap special test casing and
just remap it once at load time, or at least use a single function so the
tests/printks don't all get duplicated ? That would also eliminate the
fact you can get multiple mappings of the same thing if there are
multiple parallel users.

I note also there is no locking versus multiple accessors - eg when
writing the RTL_CMD registers


> +	/*get the address for the RTL table from the EBDA */
> +	ebda_addr = *(unsigned short *)phys_to_virt(0x40E);
> +	ebda_addr <<= 4;
> +	ebda_size = 64*1024;
> +
> +	data = ioremap(ebda_addr, ebda_size);

Careful - it is perfectly valid for a BIOS EBDA table to be zero
indicating no EBDA is present, and at this point you've not checked if
the driver is being loaded on a relevant IBM platform.

> +		unsigned int *tmp = (unsigned int *) data++;

Use u32 - it makea it obvious what is going on here.

> +/*needed to decode CmdPortAddress and CmdPortValue*/
> +#define bios_to_value(rtl, offset) (u32)((readb(rtl+(offset+1)) << 8) + \
> +							readb(rtl+offset))

What is wrong with readw() ?


Alan
--
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/