Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752226AbcJIScf (ORCPT ); Sun, 9 Oct 2016 14:32:35 -0400 Received: from mga07.intel.com ([134.134.136.100]:44046 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752084AbcJISce (ORCPT ); Sun, 9 Oct 2016 14:32:34 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,467,1473145200"; d="scan'208";a="1051491637" Date: Sun, 9 Oct 2016 21:32:32 +0300 From: Jarkko Sakkinen To: Jason Gunthorpe Cc: Peter Huewe , Marcel Selhorst , "moderated list:TPM DEVICE DRIVER" , open list Subject: Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs Message-ID: <20161009183232.GA27764@intel.com> References: <1475972112-2819-1-git-send-email-jarkko.sakkinen@linux.intel.com> <1475972112-2819-2-git-send-email-jarkko.sakkinen@linux.intel.com> <20161009014256.GA8210@obsidianresearch.com> <20161009093818.GG31891@intel.com> <20161009164905.GA12551@obsidianresearch.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161009164905.GA12551@obsidianresearch.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2486 Lines: 61 On Sun, Oct 09, 2016 at 10:49:05AM -0600, Jason Gunthorpe wrote: > On Sun, Oct 09, 2016 at 12:38:18PM +0300, Jarkko Sakkinen wrote: > > On Sat, Oct 08, 2016 at 07:42:56PM -0600, Jason Gunthorpe wrote: > > > 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)); > > > > That would be wrong address for the control area as it does not start > > from the beginning of CRB registers. > > Of course, I just pointed out what the map call should look like > > Something like this > > priv->regs = crb_map_res(dev, priv, &io_res, buf->control_address - > offsetof(struct crb_regs, ctrl_req), > sizeof(struct crb_regs)); > ctrl = &priv->regs.ctrl_req; Sorry I missed this part. Here are the constraints for existing hardware: 1. All the existing CRB start only hardware has the iomem covering the control area and registers for multiple localities. 2. All the existing ACPI start hardware has only the control area. If you assume that SSDT does not have malicous behavior caused by either a BIOS bug or maybe a rootkit, then the current patch works for all the existing hardware. To counter-measure for unexpected behavior in non-existing hardware and buggy or malicious firmware it probably make sense to use crb_map_res to validate the part of the CRB registers that is not part of the control area. Doing it in the way you proposed does not work for ACPI start devices. For them it should be done in the same way as I'm doing in the existing patch as for ACPI start devices the address below the control area are never accessed. Having a separate crb_map_res for CRB start only devices is sane thing to do for validation. /Jarkko