linux-kernel - Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb

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:   Sat, 8 Oct 2016 19:42:56 -0600
From:   Jason Gunthorpe <jgunthorpe@...idianresearch.com>
To:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
Cc:     Peter Huewe <peterhuewe@....de>,
        Marcel Selhorst <tpmdd@...horst.net>,
        "moderated list:TPM DEVICE DRIVER" 
        <tpmdd-devel@...ts.sourceforge.net>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to
 struct crb_regs

On Sun, Oct 09, 2016 at 03:15:09AM +0300, Jarkko Sakkinen wrote:
> +	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address,
> +			   sizeof(struct crb_regs) -
> +			   offsetof(struct crb_regs, ctrl_req));
> +	if (IS_ERR(ctrl))
> +		return PTR_ERR(ctrl);
> +
> +	/* The control area always overrlaps IO memory mapped from the ACPI
> +	 * object with CRB start only devices. Thus, this is perfectly safe.
> +	 */
> +	priv->regs = (void *)((unsigned long)ctrl -
> +		offsetof(struct crb_regs, ctrl_req));

Hum. No, this makes bad assumptions about the structure of iomapping.

The map itself needs to be done with the adjustment:

	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address -
		offsetof(struct crb_regs, ctrl_req),
	   	sizeof(struct crb_regs));

.. and nothing actually proves that control_address follows anything
in the driver, so this seems like a terrifying blind assumption, but
everything about the iomap in this ACPI binding seems totally bonkers
so that is in good company I guess.

.. and the comment says this only holds for 'crb start only' devices,
but the code doesn't actually act differently based on what sort of
device we have..

Your commit message also seems to imply the new registers are only on
newer hardware, but nothing seems to check for that before acessing
them?  Confusing.

Jason