From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932648AbcHYWcB (ORCPT ); Thu, 25 Aug 2016 18:32:01 -0400 Received: from quartz.orcorp.ca ([184.70.90.242]:37887 "EHLO quartz.orcorp.ca" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758734AbcHYWb7 (ORCPT ); Thu, 25 Aug 2016 18:31:59 -0400 Date: Thu, 25 Aug 2016 15:09:42 -0600 From: Jason Gunthorpe To: Jarkko Sakkinen Cc: Peter Huewe , linux-security-module@vger.kernel.org, stable@vger.kernel.org, Marcel Selhorst , "moderated list:TPM DEVICE DRIVER" , open list Subject: Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted() Message-ID: <20160825210942.GA8502@obsidianresearch.com> References: <1472000243-7088-1-git-send-email-jarkko.sakkinen@linux.intel.com> <20160825183059.GB1142@obsidianresearch.com> <20160825210437.GA8658@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160825210437.GA8658@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.151 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Aug 25, 2016 at 05:06:10PM -0400, Jarkko Sakkinen wrote: > On Thu, Aug 25, 2016 at 12:30:59PM -0600, Jason Gunthorpe wrote: > > On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote: > > > > > + if (flags & TPM_TRANSMIT_LOCK) > > > + mutex_lock(&chip->tpm_mutex); > > > > I think I would invert this. UNLOCKED is the exceptional case, so I'd > > make the 0 flags lock. If we see UNLOCKED in the caller then we know > > to audit for locking, 0 is much less obvious. > > I'm fine with either way. > > > > @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip, > > > goto out; > > > } > > > > > > - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob"); > > > + rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0); > > > > All these points should accept a flags too and the caller should pass > > in the TPM_TRASNMIT_UNLOCKED if it needs it.. > > For this bug fix it makes sense to implement it the way I did because it > needs to be applied to multiple releases (I think I've underlined this > in my changelog). You shouldn't compromise the mainline kernel to ease backporting, I'm not sure why adding a flags to tpm2_load would be a problem for the -stable kernels? It is generally better to make the backports move the older kernels closer to mainline than to have them be something else, it makes it easier to apply future backport fixes. > If you think this is high priority, I can make the next revision into > patch set of two patches. The second patch would implement the change > you suggested. Yes, I think it is important the locking requirement be very clear from the code. Jason From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from quartz.orcorp.ca ([184.70.90.242]:43039 "EHLO quartz.orcorp.ca" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752148AbcHYVNV (ORCPT ); Thu, 25 Aug 2016 17:13:21 -0400 Date: Thu, 25 Aug 2016 15:09:42 -0600 From: Jason Gunthorpe To: Jarkko Sakkinen Cc: Peter Huewe , linux-security-module@vger.kernel.org, stable@vger.kernel.org, Marcel Selhorst , "moderated list:TPM DEVICE DRIVER" , open list Subject: Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted() Message-ID: <20160825210942.GA8502@obsidianresearch.com> References: <1472000243-7088-1-git-send-email-jarkko.sakkinen@linux.intel.com> <20160825183059.GB1142@obsidianresearch.com> <20160825210437.GA8658@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160825210437.GA8658@intel.com> Sender: stable-owner@vger.kernel.org List-ID: On Thu, Aug 25, 2016 at 05:06:10PM -0400, Jarkko Sakkinen wrote: > On Thu, Aug 25, 2016 at 12:30:59PM -0600, Jason Gunthorpe wrote: > > On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote: > > > > > + if (flags & TPM_TRANSMIT_LOCK) > > > + mutex_lock(&chip->tpm_mutex); > > > > I think I would invert this. UNLOCKED is the exceptional case, so I'd > > make the 0 flags lock. If we see UNLOCKED in the caller then we know > > to audit for locking, 0 is much less obvious. > > I'm fine with either way. > > > > @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip, > > > goto out; > > > } > > > > > > - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob"); > > > + rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0); > > > > All these points should accept a flags too and the caller should pass > > in the TPM_TRASNMIT_UNLOCKED if it needs it.. > > For this bug fix it makes sense to implement it the way I did because it > needs to be applied to multiple releases (I think I've underlined this > in my changelog). You shouldn't compromise the mainline kernel to ease backporting, I'm not sure why adding a flags to tpm2_load would be a problem for the -stable kernels? It is generally better to make the backports move the older kernels closer to mainline than to have them be something else, it makes it easier to apply future backport fixes. > If you think this is high priority, I can make the next revision into > patch set of two patches. The second patch would implement the change > you suggested. Yes, I think it is important the locking requirement be very clear from the code. Jason From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Gunthorpe Subject: Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted() Date: Thu, 25 Aug 2016 15:09:42 -0600 Message-ID: <20160825210942.GA8502@obsidianresearch.com> References: <1472000243-7088-1-git-send-email-jarkko.sakkinen@linux.intel.com> <20160825183059.GB1142@obsidianresearch.com> <20160825210437.GA8658@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20160825210437.GA8658-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: tpmdd-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: Jarkko Sakkinen Cc: open list , stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-security-module-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, "moderated list:TPM DEVICE DRIVER" List-Id: tpmdd-devel@lists.sourceforge.net On Thu, Aug 25, 2016 at 05:06:10PM -0400, Jarkko Sakkinen wrote: > On Thu, Aug 25, 2016 at 12:30:59PM -0600, Jason Gunthorpe wrote: > > On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote: > > > > > + if (flags & TPM_TRANSMIT_LOCK) > > > + mutex_lock(&chip->tpm_mutex); > > > > I think I would invert this. UNLOCKED is the exceptional case, so I'd > > make the 0 flags lock. If we see UNLOCKED in the caller then we know > > to audit for locking, 0 is much less obvious. > > I'm fine with either way. > > > > @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip, > > > goto out; > > > } > > > > > > - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob"); > > > + rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0); > > > > All these points should accept a flags too and the caller should pass > > in the TPM_TRASNMIT_UNLOCKED if it needs it.. > > For this bug fix it makes sense to implement it the way I did because it > needs to be applied to multiple releases (I think I've underlined this > in my changelog). You shouldn't compromise the mainline kernel to ease backporting, I'm not sure why adding a flags to tpm2_load would be a problem for the -stable kernels? It is generally better to make the backports move the older kernels closer to mainline than to have them be something else, it makes it easier to apply future backport fixes. > If you think this is high priority, I can make the next revision into > patch set of two patches. The second patch would implement the change > you suggested. Yes, I think it is important the locking requirement be very clear from the code. Jason ------------------------------------------------------------------------------