linux-kernel - Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method

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:   Fri, 25 Aug 2017 19:21:39 +0300
From:   Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:     Jiandi An <anjiandi@...eaurora.org>
Cc:     peterhuewe@....de, tpmdd@...horst.net,
        jgunthorpe@...idianresearch.com, tpmdd-devel@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC
 start method

On Thu, Aug 24, 2017 at 12:20:35PM -0500, Jiandi An wrote:
> I know they don't change the logic.  This was to address comment from Jason.
> He wanted to express the exact condition which crb_go_idle(),
> crb_cmd_ready(), and the check for "Bad ACPI memory layout" in
> crb_map_io() should run, instead of the if not the condition, return.
> Since you want it as is, I'll change it back. It's already excluding
> CRB_FL_CRB_SMC_START in addition to CRB_FL_ACPI_START which is what's
> intended.

I think this very in simple terms. It does not change *anything*.

> Like I said the goal for this patch is to really further exclude
> CRB_FL_CRB_SMC_START from the check for "Bad ACPI memory layout"
> in crb_map_io() in the code below.
> 
> @@ -458,7 +455,7 @@ static int crb_map_io(struct acpi_device *device, struct
> crb_priv *priv,
>   * the control area, as one nice sane region except for some older
>   * stuff that puts the control area outside the ACPI IO region.
>   */
>  -if (!(priv->flags & CRB_FL_ACPI_START)) {
>  +if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0) {
>   		if (buf->control_address == io_res.start +
>   		    sizeof(*priv->regs_h))
>   			priv->regs_h = priv->iobase;
>   		else
>   		    dev_warn(dev, FW_BUG "Bad ACPI memory layout");
>   }
> 
> I'll submit v2 with only this change.  Are you okay which this change?

I'm not OK with those parts that do nothing except shuffle the code.

As I said before it would make much more sense to make code always deal
with sm and remove flags completely. That would help maintaining code
easier as new logic for TZ is introduced.

> Thanks
> - Jiandi

/Jarkko