| 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
| ||
|
Message-Id: <20060731223559.2ac944e8.akpm@osdl.org>
Date: Mon, 31 Jul 2006 22:35:59 -0700
From: Andrew Morton <akpm@...l.org>
To: Jan Beulich <jbeulich@...ell.com>
Cc: jgarzik@...ox.com, linux-kernel@...r.kernel.org, mb@...sch.de
Subject: Re: [PATCH] fix Intel RNG detection (take 2)
On Mon, 31 Jul 2006 12:41:36 +0200
Jan Beulich <jbeulich@...ell.com> wrote:
> Previously, since determination whether there was an Intel random
> number generator was based on a single bit, on systems with a matching
> bridge device but without a firmware hub, there was a 50% chance that
> the code would incorrectly decide that the system had an RNG. This
> patch adds detection of the firmware hub to better qualify the
> existence of an RNG.
>
> There is one issue with the patch: I was unable to determine the LPC
> equivalent for the PCI bridge 8086:2430 (since the old code didn't
> care about which of the many devices provided by the ICH/ESB it was
> chose to use the PCI bridge device, but the FWH settings live in the
> LPC device, so the device list needed to be changed).
>
>
> ...
>
> +#ifdef CONFIG_SMP
> +static char __initdata waitflag;
> +
> +static void __init intel_init_wait (void *unused)
No space before the (, please.
> +{
> + while (waitflag) {
> + cpu_relax();
> + smp_rmb();
> + }
I believe we decided that cpu_relax() implies a barrier.
> +#endif
> +
> static int __init mod_init(void)
> {
> int err = -ENODEV;
> + unsigned i;
> + struct pci_dev *dev = NULL;
> void __iomem *mem;
> - u8 hw_status;
> -
> - if (!pci_dev_present(pci_tbl))
> + unsigned long flags;
> + u8 bios_cntl_off, fwh_dec_en1_off;
> + u8 bios_cntl_val = 0xff, fwh_dec_en1_val = 0xff;
> + u8 hw_status, mfc, dvc;
> +
> + for (i = 0; !dev && pci_tbl[i].vendor; ++i)
> + dev = pci_get_device(pci_tbl[i].vendor, pci_tbl[i].device, NULL);
> +
> + if (!dev)
> goto out; /* Device not found. */
>
> + /* Check for Intel 82802 */
> + if (dev->device < 0x2640) {
> + fwh_dec_en1_off = FWH_DEC_EN1_REG_OLD;
> + bios_cntl_off = BIOS_CNTL_REG_OLD;
> + }
> + else {
Please do
} else {
> + fwh_dec_en1_off = FWH_DEC_EN1_REG_NEW;
> + bios_cntl_off = BIOS_CNTL_REG_NEW;
> + }
> +
> + pci_read_config_byte(dev, fwh_dec_en1_off, &fwh_dec_en1_val);
> + pci_read_config_byte(dev, bios_cntl_off, &bios_cntl_val);
> +
> + mem = ioremap_nocache(INTEL_FWH_ADDR, INTEL_FWH_ADDR_LEN);
> + if (mem == NULL) {
> + pci_dev_put(dev);
> + err = -EBUSY;
> + goto out;
> + }
> +
> +#ifdef CONFIG_SMP
> + waitflag = 1;
> + smp_wmb();
> + if (smp_call_function(intel_init_wait, NULL, 1, 0) != 0) {
> + waitflag = 0;
> + smp_wmb();
> + pci_dev_put(dev);
> + printk(KERN_ERR PFX "cannot run on all processors\n");
> + err = -EAGAIN;
> + goto err_unmap;
> + }
> +#else
> +#define waitflag err
> +#endif
awww man, this is nasty. We're #defining the name of a file-global
variable to that it puns a local? On uniproc only?
Please, not in Linux. Let's find a better way.
> + local_irq_save(flags);
I think the code needs a comment explaining the local_irq_save(). Its
reasoning is not apparent from reading the implementation.
> + if (!(fwh_dec_en1_val & FWH_F8_EN_MASK))
> + pci_write_config_byte(dev,
> + fwh_dec_en1_off,
> + fwh_dec_en1_val | FWH_F8_EN_MASK);
> + if (!(bios_cntl_val
> + & (BIOS_CNTL_LOCK_ENABLE_MASK|BIOS_CNTL_WRITE_ENABLE_MASK)))
> + pci_write_config_byte(dev,
> + bios_cntl_off,
> + bios_cntl_val | BIOS_CNTL_WRITE_ENABLE_MASK);
> +
> + writeb(INTEL_FWH_RESET_CMD, mem);
> + writeb(INTEL_FWH_READ_ID_CMD, mem);
> + mfc = readb(mem + INTEL_FWH_MANUFACTURER_CODE_ADDRESS);
> + dvc = readb(mem + INTEL_FWH_DEVICE_CODE_ADDRESS);
> + writeb(INTEL_FWH_RESET_CMD, mem);
> +
> + if (!(bios_cntl_val
> + & (BIOS_CNTL_LOCK_ENABLE_MASK|BIOS_CNTL_WRITE_ENABLE_MASK)))
It would be (a little) more conventional to do
if (!(bios_cntl_val &
> + pci_write_config_byte(dev, bios_cntl_off, bios_cntl_val);
> + if (!(fwh_dec_en1_val & FWH_F8_EN_MASK))
> + pci_write_config_byte(dev, fwh_dec_en1_off, fwh_dec_en1_val);
> +
> + local_irq_restore(flags);
> + waitflag = 0;
> + smp_wmb();
Again, the barrier is hard to understand. Pretty much all open-coded
barriers should have an explanatory comment.
> + iounmap(mem);
> + pci_dev_put(dev);
> +
> + if (mfc != INTEL_FWH_MANUFACTURER_CODE
> + || (dvc != INTEL_FWH_DEVICE_CODE_8M
> + && dvc != INTEL_FWH_DEVICE_CODE_4M)) {
Again,
if (mfc != INTEL_FWH_MANUFACTURER_CODE ||
(dvc != INTEL_FWH_DEVICE_CODE_8M &&
dvc != INTEL_FWH_DEVICE_CODE_4M)) {
would be more typical.
-
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