Patchwork [01/23] TPM: Add new TPMs to the tail of the list to prevent inadvertent change of dev

login
register
mail settings
Submitter David Howells
Date Aug. 21, 2018, 3:56 p.m.
Message ID <153486701644.13066.13372706238885253812.stgit@warthog.procyon.org.uk>
Download mbox | patch
Permalink /patch/597737/
State New
Headers show

Comments

David Howells - Aug. 21, 2018, 3:56 p.m.
Add newly registered TPMs to the tail of the list, not the beginning, so that
things that are specifying TPM_ANY_NUM don't find that the device they're
using has inadvertently changed.  Adding a second device would break IMA, for
instance.

Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
cc: stable@vger.kernel.org
---

 drivers/char/tpm/tpm-interface.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jason Gunthorpe - Aug. 21, 2018, 6:30 p.m.
On Tue, Aug 21, 2018 at 04:56:56PM +0100, David Howells wrote:
> Add newly registered TPMs to the tail of the list, not the beginning, so that
> things that are specifying TPM_ANY_NUM don't find that the device they're
> using has inadvertently changed.  Adding a second device would break IMA, for
> instance.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
> cc: stable@vger.kernel.org
> ---

We really should apply this patch...

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jarkko Sakkinen - Aug. 24, 2018, 6:19 a.m.
Use "tpm" instead of "TPM" in the short summary.

On Tue, Aug 21, 2018 at 04:56:56PM +0100, David Howells wrote:
> Add newly registered TPMs to the tail of the list, not the beginning, so that
> things that are specifying TPM_ANY_NUM don't find that the device they're
> using has inadvertently changed.  Adding a second device would break IMA, for
> instance.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
> cc: stable@vger.kernel.org

Usually I add Cc-tag before signed-off-by's (and have the first c in
upper case).

Peter's singed-off-by should be accompanied with a co-developed-by tag
if he has participated to the development of this commit.

As far as I see signed-off-by without co-developed-by makes sense in two
occasions:

* You own the subsystem tree i.e. you have to sign the changes that you
  include part of your pull request.
* You are the initial authoer of the change.  

> ---
> 
>  drivers/char/tpm/tpm-interface.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 6af17002a115..cfb9089887bd 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -1122,7 +1122,7 @@ struct tpm_chip *tpm_register_hardware(struct device *dev,
>  
>  	/* Make chip available */
>  	spin_lock(&driver_lock);
> -	list_add_rcu(&chip->list, &tpm_chip_list);

I would add here a comment just as a remainder.
> +	list_add_tail_rcu(&chip->list, &tpm_chip_list);
>  	spin_unlock(&driver_lock);
>  
>  	return chip;

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jarkko Sakkinen - Aug. 24, 2018, 6:24 a.m.
On Tue, Aug 21, 2018 at 12:30:04PM -0600, Jason Gunthorpe wrote:
> On Tue, Aug 21, 2018 at 04:56:56PM +0100, David Howells wrote:
> > Add newly registered TPMs to the tail of the list, not the beginning, so that
> > things that are specifying TPM_ANY_NUM don't find that the device they're
> > using has inadvertently changed.  Adding a second device would break IMA, for
> > instance.
> > 
> > Signed-off-by: David Howells <dhowells@redhat.com>
> > Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
> > cc: stable@vger.kernel.org
> > ---
> 
> We really should apply this patch...
> 
> Jason

This is the first time I remember seeing it.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jarkko Sakkinen - Aug. 24, 2018, 6:25 a.m.
On Fri, Aug 24, 2018 at 09:24:34AM +0300, Jarkko Sakkinen wrote:
> On Tue, Aug 21, 2018 at 12:30:04PM -0600, Jason Gunthorpe wrote:
> > On Tue, Aug 21, 2018 at 04:56:56PM +0100, David Howells wrote:
> > > Add newly registered TPMs to the tail of the list, not the beginning, so that
> > > things that are specifying TPM_ANY_NUM don't find that the device they're
> > > using has inadvertently changed.  Adding a second device would break IMA, for
> > > instance.
> > > 
> > > Signed-off-by: David Howells <dhowells@redhat.com>
> > > Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
> > > cc: stable@vger.kernel.org
> > > ---
> > 
> > We really should apply this patch...
> > 
> > Jason
> 
> This is the first time I remember seeing it.

At least in the sense that I should review it.

/Jarkkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Mimi Zohar - Aug. 24, 2018, 11:22 a.m.
On Fri, 2018-08-24 at 09:25 +0300, Jarkko Sakkinen wrote:
> On Fri, Aug 24, 2018 at 09:24:34AM +0300, Jarkko Sakkinen wrote:
> > On Tue, Aug 21, 2018 at 12:30:04PM -0600, Jason Gunthorpe wrote:
> > > On Tue, Aug 21, 2018 at 04:56:56PM +0100, David Howells wrote:
> > > > Add newly registered TPMs to the tail of the list, not the beginning, so that
> > > > things that are specifying TPM_ANY_NUM don't find that the device they're
> > > > using has inadvertently changed.  Adding a second device would break IMA, for
> > > > instance.
> > > > 
> > > > Signed-off-by: David Howells <dhowells@redhat.com>
> > > > Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > > Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
> > > > cc: stable@vger.kernel.org
> > > > ---
> > > 
> > > We really should apply this patch...
> > > 
> > > Jason
> > 
> > This is the first time I remember seeing it.
> 
> At least in the sense that I should review it.

I remember this patch, because it affected IMA.  It has already been
upstreamed as 398a1e71dc82.

Mimi


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Patch

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 6af17002a115..cfb9089887bd 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1122,7 +1122,7 @@  struct tpm_chip *tpm_register_hardware(struct device *dev,
 
 	/* Make chip available */
 	spin_lock(&driver_lock);
-	list_add_rcu(&chip->list, &tpm_chip_list);
+	list_add_tail_rcu(&chip->list, &tpm_chip_list);
 	spin_unlock(&driver_lock);
 
 	return chip;