From patchwork Mon Apr 18 23:34:57 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jason Gunthorpe X-Patchwork-Id: 611950 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.sourceforge.net (lists.sourceforge.net [216.34.181.88]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3qpkyv3Jqnz9t3c for ; Tue, 19 Apr 2016 09:35:19 +1000 (AEST) Received: from localhost ([127.0.0.1] helo=sfs-ml-2.v29.ch3.sourceforge.com) by sfs-ml-2.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1asIhG-00066r-6O; Mon, 18 Apr 2016 23:35:14 +0000 Received: from sog-mx-1.v43.ch3.sourceforge.com ([172.29.43.191] helo=mx.sourceforge.net) by sfs-ml-2.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1asIhF-00066j-7V for tpmdd-devel@lists.sourceforge.net; Mon, 18 Apr 2016 23:35:13 +0000 Received-SPF: pass (sog-mx-1.v43.ch3.sourceforge.com: domain of obsidianresearch.com designates 184.70.90.242 as permitted sender) client-ip=184.70.90.242; envelope-from=jgunthorpe@obsidianresearch.com; helo=quartz.orcorp.ca; Received: from quartz.orcorp.ca ([184.70.90.242]) by sog-mx-1.v43.ch3.sourceforge.com with esmtps (TLSv1:AES128-SHA:128) (Exim 4.76) id 1asIhE-0006kz-1B for tpmdd-devel@lists.sourceforge.net; Mon, 18 Apr 2016 23:35:13 +0000 Received: from [10.0.0.160] (helo=jggl.edm.orcorp.ca) by quartz.orcorp.ca with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84) (envelope-from ) id 1asIgz-0003zQ-Jm; Mon, 18 Apr 2016 17:34:57 -0600 Received: from jgg by jggl.edm.orcorp.ca with local (Exim 4.84) (envelope-from ) id 1asIgz-0005Q5-Er; Mon, 18 Apr 2016 17:34:57 -0600 Date: Mon, 18 Apr 2016 17:34:57 -0600 From: Jason Gunthorpe To: Jarkko Sakkinen Message-ID: <20160418233457.GA20319@obsidianresearch.com> References: <1461020880-10914-1-git-send-email-jarkko.sakkinen@linux.intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1461020880-10914-1-git-send-email-jarkko.sakkinen@linux.intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Broken-Reverse-DNS: no host name found for IP address 10.0.0.160 X-Spam-Score: -1.6 (-) X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. -1.5 SPF_CHECK_PASS SPF reports sender host as permitted sender for sender-domain -0.0 SPF_HELO_PASS SPF: HELO matches SPF record -0.0 SPF_PASS SPF: sender matches SPF record -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature X-Headers-End: 1asIhE-0006kz-1B Cc: open list , stable@vger.kernel.org, linux-security-module@vger.kernel.org, "moderated list:TPM DEVICE DRIVER" Subject: Re: [tpmdd-devel] [PATCH] tpm_crb: fix mapping of the buffers X-BeenThere: tpmdd-devel@lists.sourceforge.net X-Mailman-Version: 2.1.9 Precedence: list List-Id: Tpm Device Driver maintainance List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: tpmdd-devel-bounces@lists.sourceforge.net On Tue, Apr 19, 2016 at 02:08:00AM +0300, Jarkko Sakkinen wrote: > On my Lenovo x250 the following situation occurs: > > [18697.813871] tpm_crb MSFT0101:00: can't request region for resource > [mem 0xacdff080-0xacdfffff] Sigh, the BIOS vendors seem to be screwing this up a lot.. No doubt because the spec doesn't really say what to do very well... > The mapping of the control area interleaves the mapping of the command > buffer. The control area is mapped over page, which is not right. It > should mapped over sizeof(struct crb_control_area). Good > Fixing this issue unmasks another issue. Command and response buffers > can interleave and they do interleave on this machine. Do they 100% overlap because one is 'read' and the other is 'write'? Or did the BIOS guys screw up the length of one of the regions, and they were supposed to be back to back? In which case it is just luck this proposed patch solves the issue :( The request_io stuff is there specifically to prevent two peices of code from trying to control the same registers, I'm really reluctant to work-around it like this, though granted, crazy BIOS is a fine good reason. Is this patch below enough to deal with it sanely? If you do stick with this then: > -static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv, > - struct resource *io_res, u64 start, u32 size) > +static int crb_map_res(struct device *dev, struct crb_priv *priv, > + int res_i, u64 start, u32 size) I wouldn't change the signature at all, just add a counter to the priv and 'append to the list' This change is creating a lot of needless churn which is not good at all for the stable rules. Removing the pointer return is not an improvement.. > { > + u8 __iomem *ptr; > + int i; > + > struct resource new_res = { > .start = start, > .end = start + size - 1, > @@ -245,12 +257,25 @@ static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv, > > /* Detect a 64 bit address on a 32 bit system */ > if (start != new_res.start) > - return ERR_PTR(-EINVAL); > + return -EINVAL; > > - if (!resource_contains(io_res, &new_res)) > - return devm_ioremap_resource(dev, &new_res); > + for (i = 0; i < CRB_NR_RESOURCES; i++) { > + if (resource_contains(&priv->res[i], &new_res)) { > + priv->res[res_i] = new_res; > + priv->res_ptr[res_i] = priv->res_ptr[i] + > + (new_res.start - priv->res[i].start); > + return 0; > + } > + } Just add: id = priv->num_res++; priv->res[id] = *io_res; priv->res_ptr[id] = priv->iobase + (new_res.start - io_res->start); return priv->res_ptr[id]; And drop all the other hunks, except for the sizeof change and the above loop. Maybe print a FW bug if it overlaps with id != 0, that is just crazy beans. Jason ------------------------------------------------------------------------------ Find and fix application performance issues faster with Applications Manager Applications Manager provides deep performance insights into multiple tiers of your business applications. It resolves application problems quickly and reduces your MTTR. Get your free trial! https://ad.doubleclick.net/ddm/clk/302982198;130105516;z diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index 20155d55a62b..0a87c813d004 100644 --- a/drivers/char/tpm/tpm_crb.c +++ b/drivers/char/tpm/tpm_crb.c @@ -253,7 +253,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, struct list_head resources; struct resource io_res; struct device *dev = &device->dev; - u64 pa; + u64 cmd_pa,rsp_pa; int ret; INIT_LIST_HEAD(&resources); @@ -274,22 +274,33 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, return PTR_ERR(priv->iobase); priv->cca = crb_map_res(dev, priv, &io_res, buf->control_address, - 0x1000); + sizeof(*priv->cca)); if (IS_ERR(priv->cca)) return PTR_ERR(priv->cca); - pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | - (u64) ioread32(&priv->cca->cmd_pa_low); - priv->cmd = crb_map_res(dev, priv, &io_res, pa, - ioread32(&priv->cca->cmd_size)); - if (IS_ERR(priv->cmd)) - return PTR_ERR(priv->cmd); - + cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | + (u64) ioread32(&priv->cca->cmd_pa_low); memcpy_fromio(&pa, &priv->cca->rsp_pa, 8); - pa = le64_to_cpu(pa); - priv->rsp = crb_map_res(dev, priv, &io_res, pa, - ioread32(&priv->cca->rsp_size)); - return PTR_ERR_OR_ZERO(priv->rsp); + rsp_pa = le64_to_cpu(pa); + + if (cmd_pa == rsp_pa) { + u32 len = max_t(size_t,ioread32(&priv->cca->cmd_size), + ioread32(&priv->cca->rsp_size)); + priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, len); + if (IS_ERR(priv->cmd)) + return PTR_ERR(priv->cmd); + priv->rsp = priv->cmd; + } else { + priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, + ioread32(&priv->cca->rsp_size)); + if (IS_ERR(priv->cmd)) + return PTR_ERR(priv->cmd); + priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, + ioread32(&priv->cca->rsp_size)); + if (IS_ERR(priv->rsp)) + return PTR_ERR(priv->rsp); + } + return 0; } static int crb_acpi_add(struct acpi_device *device)