diff mbox

[tpmdd-devel,RFC,0/4] RFC: in-kernel resource manager

Message ID 1483421218.19261.4.camel@linux.vnet.ibm.com
State New
Headers show

Commit Message

James Bottomley Jan. 3, 2017, 5:26 a.m. UTC
On Mon, 2017-01-02 at 13:40 -0800, James Bottomley wrote:
> On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley wrote:
> > > On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > > > This patch set adds support for TPM spaces that provide a 
> > > > context for isolating and swapping transient objects. This 
> > > > patch set does not yet include support for isolating policy and 
> > > > HMAC sessions but it is trivial to add once the basic approach 
> > > > is settled (and that's why I created an RFC patch set).
> > > 
> > > The approach looks fine to me.  The only basic query I have is 
> > > about the default: shouldn't it be with resource manager on 
> > > rather than off?  I can't really think of a use case that wants 
> > > the RM off (even if you're running your own, having another 
> > > doesn't hurt anything, and it's still required to share with in
> > > -kernel uses).
> > 
> > This is a valid question and here's a longish explanation.
> > 
> > In TPM2_GetCapability and maybe couple of other commands you can 
> > get handles in the response body. I do not want to have special 
> > cases in the kernel for response bodies because there is no a 
> > generic way to do the substitution. What's worse, new commands in 
> > the standard future revisions could have such commands requiring 
> > special cases. In addition, vendor specific commans could have 
> > handles in the response bodies.
> 
> OK, in general I buy this ... what you're effectively saying is that 
> we need a non-RM interface for certain management type commands.
> 
> However, let me expand a bit on why I'm fretting about the non-RM use
> case.  Right at the moment, we have a single TPM device which you use
> for access to the kernel TPM.  The current tss2 just makes direct use
> of this, meaning it has to have 0666 permissions.  This means that 
> any local user can simply DoS the TPM by running us out of transient
> resources if they don't activate the RM.  If they get a connection
> always via the RM, this isn't a worry.  Perhaps the best way of 
> fixing this is to expose two separate device nodes: one raw to the 
> TPM which we could keep at 0600 and one with an always RM connection 
> which we can set to 0666.  That would mean that access to the non-RM 
> connection is either root only or governed by a system set ACL.

OK, so I put a patch together that does this (see below). It all works
nicely (with a udev script that sets the resource manager device to
0666):

jejb@jarvis:~> ls -l /dev/tpm*
crw------- 1 root root  10,   224 Jan  2 20:54 /dev/tpm0
crw-rw-rw- 1 root root 246, 65536 Jan  2 20:54 /dev/tpm0rm

I've modified the tss to connect to /dev/tpm0rm by default and it all
seems to work.

The patch applies on top of your tabrm branch, by the way.

James

---



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

Comments

Jarkko Sakkinen Jan. 3, 2017, 1:41 p.m. UTC | #1
On Mon, Jan 02, 2017 at 09:26:58PM -0800, James Bottomley wrote:
> On Mon, 2017-01-02 at 13:40 -0800, James Bottomley wrote:
> > On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
> > > On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley wrote:
> > > > On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > > > > This patch set adds support for TPM spaces that provide a 
> > > > > context for isolating and swapping transient objects. This 
> > > > > patch set does not yet include support for isolating policy and 
> > > > > HMAC sessions but it is trivial to add once the basic approach 
> > > > > is settled (and that's why I created an RFC patch set).
> > > > 
> > > > The approach looks fine to me.  The only basic query I have is 
> > > > about the default: shouldn't it be with resource manager on 
> > > > rather than off?  I can't really think of a use case that wants 
> > > > the RM off (even if you're running your own, having another 
> > > > doesn't hurt anything, and it's still required to share with in
> > > > -kernel uses).
> > > 
> > > This is a valid question and here's a longish explanation.
> > > 
> > > In TPM2_GetCapability and maybe couple of other commands you can 
> > > get handles in the response body. I do not want to have special 
> > > cases in the kernel for response bodies because there is no a 
> > > generic way to do the substitution. What's worse, new commands in 
> > > the standard future revisions could have such commands requiring 
> > > special cases. In addition, vendor specific commans could have 
> > > handles in the response bodies.
> > 
> > OK, in general I buy this ... what you're effectively saying is that 
> > we need a non-RM interface for certain management type commands.
> > 
> > However, let me expand a bit on why I'm fretting about the non-RM use
> > case.  Right at the moment, we have a single TPM device which you use
> > for access to the kernel TPM.  The current tss2 just makes direct use
> > of this, meaning it has to have 0666 permissions.  This means that 
> > any local user can simply DoS the TPM by running us out of transient
> > resources if they don't activate the RM.  If they get a connection
> > always via the RM, this isn't a worry.  Perhaps the best way of 
> > fixing this is to expose two separate device nodes: one raw to the 
> > TPM which we could keep at 0600 and one with an always RM connection 
> > which we can set to 0666.  That would mean that access to the non-RM 
> > connection is either root only or governed by a system set ACL.
> 
> OK, so I put a patch together that does this (see below). It all works
> nicely (with a udev script that sets the resource manager device to
> 0666):

This is not yet a comment about this suggestion but I guess one thing
is clear: the stuff in tpm2-space.c and tpm-interface.c changes are the
thing that we can mostly agree on and the area of argumentation is the
user space interface to it?

Just thinking how to split up the non-RFC patch set. This was also what
Jason suggested if I understood his remark correctly.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
James Bottomley Jan. 3, 2017, 4:14 p.m. UTC | #2
On Tue, 2017-01-03 at 15:41 +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 02, 2017 at 09:26:58PM -0800, James Bottomley wrote:
> > On Mon, 2017-01-02 at 13:40 -0800, James Bottomley wrote:
> > > On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
> > > > On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley
> > > > wrote:
> > > > > On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > > > > > This patch set adds support for TPM spaces that provide a 
> > > > > > context for isolating and swapping transient objects. This 
> > > > > > patch set does not yet include support for isolating policy 
> > > > > > and HMAC sessions but it is trivial to add once the basic
> > > > > > approach is settled (and that's why I created an RFC patch
> > > > > > set).
> > > > > 
> > > > > The approach looks fine to me.  The only basic query I have 
> > > > > is about the default: shouldn't it be with resource manager 
> > > > > on rather than off?  I can't really think of a use case that
> > > > > wants the RM off (even if you're running your own, having 
> > > > > another doesn't hurt anything, and it's still required to 
> > > > > share with in-kernel uses).
> > > > 
> > > > This is a valid question and here's a longish explanation.
> > > > 
> > > > In TPM2_GetCapability and maybe couple of other commands you 
> > > > can get handles in the response body. I do not want to have 
> > > > special cases in the kernel for response bodies because there 
> > > > is no a generic way to do the substitution. What's worse, new 
> > > > commands in the standard future revisions could have such 
> > > > commands requiring special cases. In addition, vendor specific 
> > > > commans could have handles in the response bodies.
> > > 
> > > OK, in general I buy this ... what you're effectively saying is 
> > > that we need a non-RM interface for certain management type
> > > commands.
> > > 
> > > However, let me expand a bit on why I'm fretting about the non-RM 
> > > use case.  Right at the moment, we have a single TPM device which 
> > > you use for access to the kernel TPM.  The current tss2 just 
> > > makes direct use of this, meaning it has to have 0666 
> > > permissions.  This means that any local user can simply DoS the 
> > > TPM by running us out of transient resources if they don't 
> > > activate the RM.  If they get a connection always via the RM, 
> > > this isn't a worry.  Perhaps the best way of fixing this is to 
> > > expose two separate device nodes: one raw to the TPM which we 
> > > could keep at 0600 and one with an always RM connection
> > > which we can set to 0666.  That would mean that access to the non
> > > -RM connection is either root only or governed by a system set
> > > ACL.
> > 
> > OK, so I put a patch together that does this (see below). It all 
> > works nicely (with a udev script that sets the resource manager 
> > device to 0666):
> 
> This is not yet a comment about this suggestion but I guess one thing
> is clear: the stuff in tpm2-space.c and tpm-interface.c changes are 
> the thing that we can mostly agree on and the area of argumentation 
> is the user space interface to it?

Agreed.  As I've already said, the space and interface code is working
well for me in production on my laptop.

> Just thinking how to split up the non-RFC patch set. This was also 
> what Jason suggested if I understood his remark correctly.

SUre ... let's get agreement on how we move forward first.  How the
patch is activated by the user has to be sorted out as well before it
can go in, but it doesn't have to be the first thing we do.  I'm happy
to continue playing with the interfaces to see what works and what
doesn't.  My main current feedback is that I think separate devices
works way better than an ioctl becuase the separate devices approach
allows differing system policies for who accesses the RM backed TPM vs
who accesses the raw one.

James


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jarkko Sakkinen Jan. 3, 2017, 6:36 p.m. UTC | #3
On Tue, Jan 03, 2017 at 08:14:55AM -0800, James Bottomley wrote:
> On Tue, 2017-01-03 at 15:41 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 02, 2017 at 09:26:58PM -0800, James Bottomley wrote:
> > > On Mon, 2017-01-02 at 13:40 -0800, James Bottomley wrote:
> > > > On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
> > > > > On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley
> > > > > wrote:
> > > > > > On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > > > > > > This patch set adds support for TPM spaces that provide a 
> > > > > > > context for isolating and swapping transient objects. This 
> > > > > > > patch set does not yet include support for isolating policy 
> > > > > > > and HMAC sessions but it is trivial to add once the basic
> > > > > > > approach is settled (and that's why I created an RFC patch
> > > > > > > set).
> > > > > > 
> > > > > > The approach looks fine to me.  The only basic query I have 
> > > > > > is about the default: shouldn't it be with resource manager 
> > > > > > on rather than off?  I can't really think of a use case that
> > > > > > wants the RM off (even if you're running your own, having 
> > > > > > another doesn't hurt anything, and it's still required to 
> > > > > > share with in-kernel uses).
> > > > > 
> > > > > This is a valid question and here's a longish explanation.
> > > > > 
> > > > > In TPM2_GetCapability and maybe couple of other commands you 
> > > > > can get handles in the response body. I do not want to have 
> > > > > special cases in the kernel for response bodies because there 
> > > > > is no a generic way to do the substitution. What's worse, new 
> > > > > commands in the standard future revisions could have such 
> > > > > commands requiring special cases. In addition, vendor specific 
> > > > > commans could have handles in the response bodies.
> > > > 
> > > > OK, in general I buy this ... what you're effectively saying is 
> > > > that we need a non-RM interface for certain management type
> > > > commands.
> > > > 
> > > > However, let me expand a bit on why I'm fretting about the non-RM 
> > > > use case.  Right at the moment, we have a single TPM device which 
> > > > you use for access to the kernel TPM.  The current tss2 just 
> > > > makes direct use of this, meaning it has to have 0666 
> > > > permissions.  This means that any local user can simply DoS the 
> > > > TPM by running us out of transient resources if they don't 
> > > > activate the RM.  If they get a connection always via the RM, 
> > > > this isn't a worry.  Perhaps the best way of fixing this is to 
> > > > expose two separate device nodes: one raw to the TPM which we 
> > > > could keep at 0600 and one with an always RM connection
> > > > which we can set to 0666.  That would mean that access to the non
> > > > -RM connection is either root only or governed by a system set
> > > > ACL.
> > > 
> > > OK, so I put a patch together that does this (see below). It all 
> > > works nicely (with a udev script that sets the resource manager 
> > > device to 0666):
> > 
> > This is not yet a comment about this suggestion but I guess one thing
> > is clear: the stuff in tpm2-space.c and tpm-interface.c changes are 
> > the thing that we can mostly agree on and the area of argumentation 
> > is the user space interface to it?
> 
> Agreed.  As I've already said, the space and interface code is working
> well for me in production on my laptop.
> 
> > Just thinking how to split up the non-RFC patch set. This was also 
> > what Jason suggested if I understood his remark correctly.
> 
> SUre ... let's get agreement on how we move forward first.  How the
> patch is activated by the user has to be sorted out as well before it
> can go in, but it doesn't have to be the first thing we do.  I'm happy
> to continue playing with the interfaces to see what works and what
> doesn't.  My main current feedback is that I think separate devices
> works way better than an ioctl becuase the separate devices approach
> allows differing system policies for who accesses the RM backed TPM vs
> who accesses the raw one.

I think I see your point. I would rather name the device as tpms0 but
otherwise I think we could do it in the way you suggest...

> James

/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 Jan. 3, 2017, 7:14 p.m. UTC | #4
On Tue, Jan 03, 2017 at 08:36:02PM +0200, Jarkko Sakkinen wrote:
> On Tue, Jan 03, 2017 at 08:14:55AM -0800, James Bottomley wrote:
> > On Tue, 2017-01-03 at 15:41 +0200, Jarkko Sakkinen wrote:
> > > On Mon, Jan 02, 2017 at 09:26:58PM -0800, James Bottomley wrote:
> > > > On Mon, 2017-01-02 at 13:40 -0800, James Bottomley wrote:
> > > > > On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
> > > > > > On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley
> > > > > > wrote:
> > > > > > > On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > > > > > > > This patch set adds support for TPM spaces that provide a 
> > > > > > > > context for isolating and swapping transient objects. This 
> > > > > > > > patch set does not yet include support for isolating policy 
> > > > > > > > and HMAC sessions but it is trivial to add once the basic
> > > > > > > > approach is settled (and that's why I created an RFC patch
> > > > > > > > set).
> > > > > > > 
> > > > > > > The approach looks fine to me.  The only basic query I have 
> > > > > > > is about the default: shouldn't it be with resource manager 
> > > > > > > on rather than off?  I can't really think of a use case that
> > > > > > > wants the RM off (even if you're running your own, having 
> > > > > > > another doesn't hurt anything, and it's still required to 
> > > > > > > share with in-kernel uses).
> > > > > > 
> > > > > > This is a valid question and here's a longish explanation.
> > > > > > 
> > > > > > In TPM2_GetCapability and maybe couple of other commands you 
> > > > > > can get handles in the response body. I do not want to have 
> > > > > > special cases in the kernel for response bodies because there 
> > > > > > is no a generic way to do the substitution. What's worse, new 
> > > > > > commands in the standard future revisions could have such 
> > > > > > commands requiring special cases. In addition, vendor specific 
> > > > > > commans could have handles in the response bodies.
> > > > > 
> > > > > OK, in general I buy this ... what you're effectively saying is 
> > > > > that we need a non-RM interface for certain management type
> > > > > commands.
> > > > > 
> > > > > However, let me expand a bit on why I'm fretting about the non-RM 
> > > > > use case.  Right at the moment, we have a single TPM device which 
> > > > > you use for access to the kernel TPM.  The current tss2 just 
> > > > > makes direct use of this, meaning it has to have 0666 
> > > > > permissions.  This means that any local user can simply DoS the 
> > > > > TPM by running us out of transient resources if they don't 
> > > > > activate the RM.  If they get a connection always via the RM, 
> > > > > this isn't a worry.  Perhaps the best way of fixing this is to 
> > > > > expose two separate device nodes: one raw to the TPM which we 
> > > > > could keep at 0600 and one with an always RM connection
> > > > > which we can set to 0666.  That would mean that access to the non
> > > > > -RM connection is either root only or governed by a system set
> > > > > ACL.
> > > > 
> > > > OK, so I put a patch together that does this (see below). It all 
> > > > works nicely (with a udev script that sets the resource manager 
> > > > device to 0666):
> > > 
> > > This is not yet a comment about this suggestion but I guess one thing
> > > is clear: the stuff in tpm2-space.c and tpm-interface.c changes are 
> > > the thing that we can mostly agree on and the area of argumentation 
> > > is the user space interface to it?
> > 
> > Agreed.  As I've already said, the space and interface code is working
> > well for me in production on my laptop.
> > 
> > > Just thinking how to split up the non-RFC patch set. This was also 
> > > what Jason suggested if I understood his remark correctly.
> > 
> > SUre ... let's get agreement on how we move forward first.  How the
> > patch is activated by the user has to be sorted out as well before it
> > can go in, but it doesn't have to be the first thing we do.  I'm happy
> > to continue playing with the interfaces to see what works and what
> > doesn't.  My main current feedback is that I think separate devices
> > works way better than an ioctl becuase the separate devices approach
> > allows differing system policies for who accesses the RM backed TPM vs
> > who accesses the raw one.
> 
> I think I see your point. I would rather name the device as tpms0 but
> otherwise I think we could do it in the way you suggest...

I think one more stronger argument for tpms0 is that it keeps tpm0
intact. Those who don't care about tpms0 don't have to worry about it
causing regressions. Also it makes it cleaner to put the whole feature
under a compilation flag, which would make to me because that gives
distributions a choice to not enable in-kernel RM when it first hits the
mainline.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
James Bottomley Jan. 3, 2017, 7:34 p.m. UTC | #5
On Tue, 2017-01-03 at 21:14 +0200, Jarkko Sakkinen wrote:
> On Tue, Jan 03, 2017 at 08:36:02PM +0200, Jarkko Sakkinen wrote:
> > On Tue, Jan 03, 2017 at 08:14:55AM -0800, James Bottomley wrote:
> > > On Tue, 2017-01-03 at 15:41 +0200, Jarkko Sakkinen wrote:
[...]
> > > > Just thinking how to split up the non-RFC patch set. This was 
> > > > also what Jason suggested if I understood his remark correctly.
> > > 
> > > SUre ... let's get agreement on how we move forward first.  How 
> > > the patch is activated by the user has to be sorted out as well
> > > before it can go in, but it doesn't have to be the first thing we 
> > > do.  I'm happy to continue playing with the interfaces to see 
> > > what works and what doesn't.  My main current feedback is that I 
> > > think separate devices works way better than an ioctl becuase the 
> > > separate devices approach allows differing system policies for 
> > > who accesses the RM backed TPM vs who accesses the raw one.
> > 
> > I think I see your point. I would rather name the device as tpms0 
> > but otherwise I think we could do it in the way you suggest...

I'm not at all wedded to the name tpm0rm, so tpms0 is fine by me.

> I think one more stronger argument for tpms0 is that it keeps tpm0
> intact. Those who don't care about tpms0 don't have to worry about it
> causing regressions. Also it makes it cleaner to put the whole 
> feature under a compilation flag, which would make to me because that 
> gives distributions a choice to not enable in-kernel RM when it first 
> hits the mainline.

I wouldn't go that far: one of the evils we cause for distros is too
many compile options.  In this case, I can't think of a good reason to
have an option to disable this now the feature is segregated on to a
separate device.  If we get a regression, only users of the new device
will notice.  If it's a compile option, this is the same if the distro
enables it and if it's disabled, no user can test out the feature (and
distros eventually get complaints about it not working).

James



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jason Gunthorpe Jan. 3, 2017, 9:54 p.m. UTC | #6
On Mon, Jan 02, 2017 at 09:26:58PM -0800, James Bottomley wrote:

> OK, so I put a patch together that does this (see below). It all works
> nicely (with a udev script that sets the resource manager device to
> 0666):
> 
> jejb@jarvis:~> ls -l /dev/tpm*
> crw------- 1 root root  10,   224 Jan  2 20:54 /dev/tpm0
> crw-rw-rw- 1 root root 246, 65536 Jan  2 20:54 /dev/tpm0rm
> 
> I've modified the tss to connect to /dev/tpm0rm by default and it all
> seems to work.
> 
> The patch applies on top of your tabrm branch, by the way.

If we are making a new /dev/ node we should think more carefully about
the design.

- Do we need a cdev node for every chip? What about just '/dev/tpm' and
  we encode the chip number in the message. Since the exclusive
  locking is gone this is very doable.
- Should we get rid of the read/write protocol and use ioctl instead?
  As I understand it ioctl is more usable with seccomp and related
  schemes? I could see passing a TPM FD into a sandbox and wanting the
  sandbox only able to do do decrypt/encrypt operations, for instance.
- Something to identify tpm chips and help match key data with the
  proper chip.

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Andy Lutomirski Jan. 4, 2017, 5:47 a.m. UTC | #7
On 01/02/2017 09:26 PM, James Bottomley wrote:
> On Mon, 2017-01-02 at 13:40 -0800, James Bottomley wrote:
>> On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
>>> On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley wrote:
>>>> On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
>>>>> This patch set adds support for TPM spaces that provide a
>>>>> context for isolating and swapping transient objects. This
>>>>> patch set does not yet include support for isolating policy and
>>>>> HMAC sessions but it is trivial to add once the basic approach
>>>>> is settled (and that's why I created an RFC patch set).
>>>>
>>>> The approach looks fine to me.  The only basic query I have is
>>>> about the default: shouldn't it be with resource manager on
>>>> rather than off?  I can't really think of a use case that wants
>>>> the RM off (even if you're running your own, having another
>>>> doesn't hurt anything, and it's still required to share with in
>>>> -kernel uses).
>>>
>>> This is a valid question and here's a longish explanation.
>>>
>>> In TPM2_GetCapability and maybe couple of other commands you can
>>> get handles in the response body. I do not want to have special
>>> cases in the kernel for response bodies because there is no a
>>> generic way to do the substitution. What's worse, new commands in
>>> the standard future revisions could have such commands requiring
>>> special cases. In addition, vendor specific commans could have
>>> handles in the response bodies.
>>
>> OK, in general I buy this ... what you're effectively saying is that
>> we need a non-RM interface for certain management type commands.
>>
>> However, let me expand a bit on why I'm fretting about the non-RM use
>> case.  Right at the moment, we have a single TPM device which you use
>> for access to the kernel TPM.  The current tss2 just makes direct use
>> of this, meaning it has to have 0666 permissions.  This means that
>> any local user can simply DoS the TPM by running us out of transient
>> resources if they don't activate the RM.  If they get a connection
>> always via the RM, this isn't a worry.  Perhaps the best way of
>> fixing this is to expose two separate device nodes: one raw to the
>> TPM which we could keep at 0600 and one with an always RM connection
>> which we can set to 0666.  That would mean that access to the non-RM
>> connection is either root only or governed by a system set ACL.
>
> OK, so I put a patch together that does this (see below). It all works
> nicely (with a udev script that sets the resource manager device to
> 0666):
>
> jejb@jarvis:~> ls -l /dev/tpm*
> crw------- 1 root root  10,   224 Jan  2 20:54 /dev/tpm0
> crw-rw-rw- 1 root root 246, 65536 Jan  2 20:54 /dev/tpm0rm
>
> I've modified the tss to connect to /dev/tpm0rm by default and it all
> seems to work.
>
> The patch applies on top of your tabrm branch, by the way.

Conceptually I like this a *lot* better.  I believe that this 
effectively solves my major gripe with the TPM 1.2 ecosystem.

However, can this be taken just a little farther?  IMO the tpm0rm (or 
tpms0 or whatever) node should also restrict commands that can be sent 
(perhaps by in-kernel whitelist?) to those that shouldn't be restricted 
to the owner (by which I probably mean the Owner, the Platform, etc)? 
For example, someone with tpm0rm open should not be able to change key 
hierarchy passwords, write to NV memory, clear hierarchies, etc.

Hmm.  Maybe there should be a way to allocate NV slots to users. 
/dev/tpm/nv0?  I don't really like that idea, though.


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jarkko Sakkinen Jan. 4, 2017, 12:58 p.m. UTC | #8
On Tue, Jan 03, 2017 at 02:54:45PM -0700, Jason Gunthorpe wrote:
> On Mon, Jan 02, 2017 at 09:26:58PM -0800, James Bottomley wrote:
> 
> > OK, so I put a patch together that does this (see below). It all works
> > nicely (with a udev script that sets the resource manager device to
> > 0666):
> > 
> > jejb@jarvis:~> ls -l /dev/tpm*
> > crw------- 1 root root  10,   224 Jan  2 20:54 /dev/tpm0
> > crw-rw-rw- 1 root root 246, 65536 Jan  2 20:54 /dev/tpm0rm
> > 
> > I've modified the tss to connect to /dev/tpm0rm by default and it all
> > seems to work.
> > 
> > The patch applies on top of your tabrm branch, by the way.
> 
> If we are making a new /dev/ node we should think more carefully about
> the design.
> 
> - Do we need a cdev node for every chip? What about just '/dev/tpm' and
>   we encode the chip number in the message. Since the exclusive
>   locking is gone this is very doable.

What about backwards compatiblity? Or would this be just for /dev/tpms?
We can consider this.

> - Should we get rid of the read/write protocol and use ioctl instead?
>   As I understand it ioctl is more usable with seccomp and related
>   schemes? I could see passing a TPM FD into a sandbox and wanting the
>   sandbox only able to do do decrypt/encrypt operations, for instance.

Are you suggesting that command/response transaction would be handled
by ioctl instead of read/write pair. Would make sense looking at how
read/write is managed now (it is more or less of a hack because it
actually is a transction).

> - Something to identify tpm chips and help match key data with the
>   proper chip.

Hey, here's what I propose. I take some of the ideas (not all there
have been so many) and bake a v2 of the RFC. Lets see where we are
at then. I won't add any reviewed/tested-by's before we are in the
same line with "big ideas" nor do I create a non-RFC patch set.

> Jason

/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 Jan. 4, 2017, 1 p.m. UTC | #9
On Tue, Jan 03, 2017 at 09:47:21PM -0800, Andy Lutomirski wrote:
> On 01/02/2017 09:26 PM, James Bottomley wrote:
> > On Mon, 2017-01-02 at 13:40 -0800, James Bottomley wrote:
> > > On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
> > > > On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley wrote:
> > > > > On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > > > > > This patch set adds support for TPM spaces that provide a
> > > > > > context for isolating and swapping transient objects. This
> > > > > > patch set does not yet include support for isolating policy and
> > > > > > HMAC sessions but it is trivial to add once the basic approach
> > > > > > is settled (and that's why I created an RFC patch set).
> > > > > 
> > > > > The approach looks fine to me.  The only basic query I have is
> > > > > about the default: shouldn't it be with resource manager on
> > > > > rather than off?  I can't really think of a use case that wants
> > > > > the RM off (even if you're running your own, having another
> > > > > doesn't hurt anything, and it's still required to share with in
> > > > > -kernel uses).
> > > > 
> > > > This is a valid question and here's a longish explanation.
> > > > 
> > > > In TPM2_GetCapability and maybe couple of other commands you can
> > > > get handles in the response body. I do not want to have special
> > > > cases in the kernel for response bodies because there is no a
> > > > generic way to do the substitution. What's worse, new commands in
> > > > the standard future revisions could have such commands requiring
> > > > special cases. In addition, vendor specific commans could have
> > > > handles in the response bodies.
> > > 
> > > OK, in general I buy this ... what you're effectively saying is that
> > > we need a non-RM interface for certain management type commands.
> > > 
> > > However, let me expand a bit on why I'm fretting about the non-RM use
> > > case.  Right at the moment, we have a single TPM device which you use
> > > for access to the kernel TPM.  The current tss2 just makes direct use
> > > of this, meaning it has to have 0666 permissions.  This means that
> > > any local user can simply DoS the TPM by running us out of transient
> > > resources if they don't activate the RM.  If they get a connection
> > > always via the RM, this isn't a worry.  Perhaps the best way of
> > > fixing this is to expose two separate device nodes: one raw to the
> > > TPM which we could keep at 0600 and one with an always RM connection
> > > which we can set to 0666.  That would mean that access to the non-RM
> > > connection is either root only or governed by a system set ACL.
> > 
> > OK, so I put a patch together that does this (see below). It all works
> > nicely (with a udev script that sets the resource manager device to
> > 0666):
> > 
> > jejb@jarvis:~> ls -l /dev/tpm*
> > crw------- 1 root root  10,   224 Jan  2 20:54 /dev/tpm0
> > crw-rw-rw- 1 root root 246, 65536 Jan  2 20:54 /dev/tpm0rm
> > 
> > I've modified the tss to connect to /dev/tpm0rm by default and it all
> > seems to work.
> > 
> > The patch applies on top of your tabrm branch, by the way.
> 
> Conceptually I like this a *lot* better.  I believe that this effectively
> solves my major gripe with the TPM 1.2 ecosystem.
> 
> However, can this be taken just a little farther?  IMO the tpm0rm (or tpms0
> or whatever) node should also restrict commands that can be sent (perhaps by
> in-kernel whitelist?) to those that shouldn't be restricted to the owner (by
> which I probably mean the Owner, the Platform, etc)? For example, someone
> with tpm0rm open should not be able to change key hierarchy passwords, write
> to NV memory, clear hierarchies, etc.

Yes. This was already discussed in Linux Plumbers. It is trivial to have
that. I just left it out from this RFC patch set to get something not
too complicated out quickly. Whitelist is coming to the non-RFC version.

> Hmm.  Maybe there should be a way to allocate NV slots to users.
> /dev/tpm/nv0?  I don't really like that idea, though.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jason Gunthorpe Jan. 4, 2017, 4:55 p.m. UTC | #10
On Wed, Jan 04, 2017 at 02:58:10PM +0200, Jarkko Sakkinen wrote:
> On Tue, Jan 03, 2017 at 02:54:45PM -0700, Jason Gunthorpe wrote:
> > On Mon, Jan 02, 2017 at 09:26:58PM -0800, James Bottomley wrote:
> > 
> > > OK, so I put a patch together that does this (see below). It all works
> > > nicely (with a udev script that sets the resource manager device to
> > > 0666):
> > > 
> > > jejb@jarvis:~> ls -l /dev/tpm*
> > > crw------- 1 root root  10,   224 Jan  2 20:54 /dev/tpm0
> > > crw-rw-rw- 1 root root 246, 65536 Jan  2 20:54 /dev/tpm0rm
> > > 
> > > I've modified the tss to connect to /dev/tpm0rm by default and it all
> > > seems to work.
> > > 
> > > The patch applies on top of your tabrm branch, by the way.
> > 
> > If we are making a new /dev/ node we should think more carefully about
> > the design.
> > 
> > - Do we need a cdev node for every chip? What about just '/dev/tpm' and
> >   we encode the chip number in the message. Since the exclusive
> >   locking is gone this is very doable.
> 
> What about backwards compatiblity? Or would this be just for /dev/tpms?
> We can consider this.

Yes, just for the new char dev.

> > - Should we get rid of the read/write protocol and use ioctl instead?
> >   As I understand it ioctl is more usable with seccomp and related
> >   schemes? I could see passing a TPM FD into a sandbox and wanting the
> >   sandbox only able to do do decrypt/encrypt operations, for instance.
> 
> Are you suggesting that command/response transaction would be handled
> by ioctl instead of read/write pair. Would make sense looking at how
> read/write is managed now (it is more or less of a hack because it
> actually is a transction).

Yes.

> > - Something to identify tpm chips and help match key data with the
> >   proper chip.
> 
> Hey, here's what I propose. I take some of the ideas (not all there
> have been so many) and bake a v2 of the RFC. Lets see where we are
> at then. I won't add any reviewed/tested-by's before we are in the
> same line with "big ideas" nor do I create a non-RFC patch set.

Usually with something like this there will be lots of dicussion
around the uapi portion - that is the portion we have to reatin
backwards compatability with forever - so there is a natural need to
make sure it is sane.

This is why I'm so cautious to limit what is possible because it is
easier to add new stuff then take stuff away.

So design your patch set to keep the uapi stuff as distinct and
well-described as possible - the other parts are much easier to review
and agree on.

Jason

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

Patch

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index ac4c05f..25b8d30 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -33,6 +33,7 @@  DEFINE_IDR(dev_nums_idr);
 static DEFINE_MUTEX(idr_lock);
 
 struct class *tpm_class;
+struct class *tpm_rm_class;
 dev_t tpm_devt;
 
 /**
@@ -169,27 +170,39 @@  struct tpm_chip *tpm_chip_alloc(struct device *pdev,
 	chip->dev_num = rc;
 
 	device_initialize(&chip->dev);
+	device_initialize(&chip->devrm);
 
 	chip->dev.class = tpm_class;
 	chip->dev.release = tpm_dev_release;
 	chip->dev.parent = pdev;
 	chip->dev.groups = chip->groups;
 
+	chip->devrm.parent = pdev;
+	chip->devrm.class = tpm_rm_class;
+
 	if (chip->dev_num == 0)
 		chip->dev.devt = MKDEV(MISC_MAJOR, TPM_MINOR);
 	else
 		chip->dev.devt = MKDEV(MAJOR(tpm_devt), chip->dev_num);
 
+	chip->devrm.devt = MKDEV(MAJOR(tpm_devt), chip->dev_num + TPM_NUM_DEVICES);
+
 	rc = dev_set_name(&chip->dev, "tpm%d", chip->dev_num);
 	if (rc)
 		goto out;
+	rc = dev_set_name(&chip->devrm, "tpm%drm", chip->dev_num);
+	if (rc)
+		goto out;
 
 	if (!pdev)
 		chip->flags |= TPM_CHIP_FLAG_VIRTUAL;
 
 	cdev_init(&chip->cdev, &tpm_fops);
+	cdev_init(&chip->cdevrm, &tpm_rm_fops);
 	chip->cdev.owner = THIS_MODULE;
+	chip->cdevrm.owner = THIS_MODULE;
 	chip->cdev.kobj.parent = &chip->dev.kobj;
+	chip->cdevrm.kobj.parent = &chip->devrm.kobj;
 
 	chip->tr_buf.data = kzalloc(TPM_BUFSIZE, GFP_KERNEL);
 	if (!chip->tr_buf.data) {
@@ -208,6 +221,7 @@  struct tpm_chip *tpm_chip_alloc(struct device *pdev,
 
 out:
 	put_device(&chip->dev);
+	put_device(&chip->devrm);
 	return ERR_PTR(rc);
 }
 EXPORT_SYMBOL_GPL(tpm_chip_alloc);
@@ -252,7 +266,7 @@  static int tpm_add_char_device(struct tpm_chip *chip)
 			dev_name(&chip->dev), MAJOR(chip->dev.devt),
 			MINOR(chip->dev.devt), rc);
 
-		return rc;
+		goto err_1;
 	}
 
 	rc = device_add(&chip->dev);
@@ -262,16 +276,44 @@  static int tpm_add_char_device(struct tpm_chip *chip)
 			dev_name(&chip->dev), MAJOR(chip->dev.devt),
 			MINOR(chip->dev.devt), rc);
 
-		cdev_del(&chip->cdev);
-		return rc;
+		goto err_2;
+	}
+
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		rc = cdev_add(&chip->cdevrm, chip->devrm.devt, 1);
+	if (rc) {
+		dev_err(&chip->dev,
+			"unable to cdev_add() %s, major %d, minor %d, err=%d\n",
+			dev_name(&chip->devrm), MAJOR(chip->devrm.devt),
+			MINOR(chip->devrm.devt), rc);
+
+		goto err_3;
 	}
 
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		rc = device_add(&chip->devrm);
+	if (rc) {
+		dev_err(&chip->dev,
+			"unable to device_register() %s, major %d, minor %d, err=%d\n",
+			dev_name(&chip->devrm), MAJOR(chip->devrm.devt),
+			MINOR(chip->devrm.devt), rc);
+
+		goto err_4;
+	}
 	/* Make the chip available. */
 	mutex_lock(&idr_lock);
 	idr_replace(&dev_nums_idr, chip, chip->dev_num);
 	mutex_unlock(&idr_lock);
 
 	return rc;
+ err_4:
+	cdev_del(&chip->cdevrm);
+ err_3:
+	device_del(&chip->dev);
+ err_2:
+	cdev_del(&chip->cdev);
+ err_1:
+	return rc;
 }
 
 static void tpm_del_char_device(struct tpm_chip *chip)
@@ -279,6 +321,11 @@  static void tpm_del_char_device(struct tpm_chip *chip)
 	cdev_del(&chip->cdev);
 	device_del(&chip->dev);
 
+	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+		cdev_del(&chip->cdevrm);
+		device_del(&chip->devrm);
+	}
+
 	/* Make the chip unavailable. */
 	mutex_lock(&idr_lock);
 	idr_replace(&dev_nums_idr, NULL, chip->dev_num);
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 139638b..bed29f9 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -54,23 +54,28 @@  static void timeout_work(struct work_struct *work)
 	mutex_unlock(&priv->buffer_mutex);
 }
 
-static int tpm_open(struct inode *inode, struct file *file)
+static int tpm_open_internal(struct inode *inode, struct file *file, bool is_rm)
 {
-	struct tpm_chip *chip =
-		container_of(inode->i_cdev, struct tpm_chip, cdev);
+	struct tpm_chip *chip;
 	struct file_priv *priv;
 
+	if (is_rm)
+		chip = container_of(inode->i_cdev, struct tpm_chip, cdevrm);
+	else
+		chip = container_of(inode->i_cdev, struct tpm_chip, cdev);
+
 	/* It's assured that the chip will be opened just once,
 	 * by the check of is_open variable, which is protected
 	 * by driver_lock. */
-	if (test_and_set_bit(0, &chip->is_open)) {
+	if (!is_rm && test_and_set_bit(0, &chip->is_open)) {
 		dev_dbg(&chip->dev, "Another process owns this TPM\n");
 		return -EBUSY;
 	}
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (priv == NULL) {
-		clear_bit(0, &chip->is_open);
+		if (!is_rm)
+			clear_bit(0, &chip->is_open);
 		return -ENOMEM;
 	}
 
@@ -82,9 +87,27 @@  static int tpm_open(struct inode *inode, struct file *file)
 	INIT_WORK(&priv->work, timeout_work);
 
 	file->private_data = priv;
+
+	if (is_rm) {
+		priv->space.context_buf =  kzalloc(PAGE_SIZE, GFP_KERNEL);
+		if (!priv->space.context_buf)
+			return -ENOMEM;
+		priv->has_space = true;
+	}
+
 	return 0;
 }
 
+static int tpm_open(struct inode *inode, struct file *file)
+{
+	return tpm_open_internal(inode, file, false);
+}
+
+static int tpm_rm_open(struct inode *inode, struct file *file)
+{
+	return tpm_open_internal(inode, file, true);
+}
+
 static ssize_t tpm_read(struct file *file, char __user *buf,
 			size_t size, loff_t *off)
 {
@@ -169,65 +192,6 @@  static ssize_t tpm_write(struct file *file, const char __user *buf,
 	return in_size;
 }
 
-/**
- * tpm_ioc_new_space - handler for %SGX_IOC_NEW_SPACE ioctl
- *
- * Creates a new TPM space that can hold a set of transient objects. The space
- * is isolated with virtual handles that are mapped into physical handles by the
- * driver.
- */
-static long tpm_ioc_new_space(struct file *file, unsigned int ioctl,
-			      unsigned long arg)
-{
-	struct file_priv *priv = file->private_data;
-	struct tpm_chip *chip = priv->chip;
-	int rc = 0;
-
-	if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
-		return -EOPNOTSUPP;
-
-	mutex_lock(&priv->buffer_mutex);
-
-	if (priv->has_space) {
-		rc = -EBUSY;
-		goto out;
-	}
-
-	priv->space.context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!priv->space.context_buf) {
-		rc = -ENOMEM;
-		goto out;
-	}
-
-	/* The TPM device can be opened again as this file has been moved to a
-	 * TPM handle space.
-	 */
-	priv->has_space = true;
-	clear_bit(0, &chip->is_open);
-out:
-	mutex_unlock(&priv->buffer_mutex);
-	return rc;
-}
-
-static long tpm_ioctl(struct file *file, unsigned int ioctl,
-		      unsigned long arg)
-{
-	switch (ioctl) {
-	case TPM_IOC_NEW_SPACE:
-		return tpm_ioc_new_space(file, ioctl, arg);
-	default:
-		return -ENOIOCTLCMD;
-	}
-}
-
-#ifdef CONFIG_COMPAT
-static long tpm_compat_ioctl(struct file *file, unsigned int ioctl,
-			     unsigned long arg)
-{
-	return tpm_ioctl(file, ioctl, arg);
-}
-#endif
-
 /*
  * Called on file close
  */
@@ -247,7 +211,8 @@  static int tpm_release(struct inode *inode, struct file *file)
 	flush_work(&priv->work);
 	file->private_data = NULL;
 	atomic_set(&priv->data_pending, 0);
-	clear_bit(0, &priv->chip->is_open);
+	if (!priv->has_space)
+		clear_bit(0, &priv->chip->is_open);
 	kfree(priv);
 	return 0;
 }
@@ -258,10 +223,15 @@  const struct file_operations tpm_fops = {
 	.open = tpm_open,
 	.read = tpm_read,
 	.write = tpm_write,
-	.unlocked_ioctl = tpm_ioctl,
-#ifdef CONFIG_COMPAT
-	.compat_ioctl = tpm_compat_ioctl,
-#endif
+	.release = tpm_release,
+};
+
+const struct file_operations tpm_rm_fops = {
+	.owner = THIS_MODULE,
+	.llseek = no_llseek,
+	.open = tpm_rm_open,
+	.read = tpm_read,
+	.write = tpm_write,
 	.release = tpm_release,
 };
 
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index a1ae57e..c1829de 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1194,9 +1194,17 @@  static int __init tpm_init(void)
 		return PTR_ERR(tpm_class);
 	}
 
-	rc = alloc_chrdev_region(&tpm_devt, 0, TPM_NUM_DEVICES, "tpm");
+	tpm_rm_class = class_create(THIS_MODULE, "tpmrm");
+	if (IS_ERR(tpm_rm_class)) {
+		pr_err("couldn't create tpmrm class\n");
+		class_destroy(tpm_class);
+		return PTR_ERR(tpm_rm_class);
+	}
+
+	rc = alloc_chrdev_region(&tpm_devt, 0, 2*TPM_NUM_DEVICES, "tpm");
 	if (rc < 0) {
 		pr_err("tpm: failed to allocate char dev region\n");
+		class_destroy(tpm_rm_class);
 		class_destroy(tpm_class);
 		return rc;
 	}
@@ -1208,7 +1216,8 @@  static void __exit tpm_exit(void)
 {
 	idr_destroy(&dev_nums_idr);
 	class_destroy(tpm_class);
-	unregister_chrdev_region(tpm_devt, TPM_NUM_DEVICES);
+	class_destroy(tpm_rm_class);
+	unregister_chrdev_region(tpm_devt, 2*TPM_NUM_DEVICES);
 }
 
 subsys_initcall(tpm_init);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index c6171e5..890fb6b 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -186,8 +186,8 @@  struct tpm_buf {
 };
 
 struct tpm_chip {
-	struct device dev;
-	struct cdev cdev;
+	struct device dev, devrm;
+	struct cdev cdev, cdevrm;
 
 	/* A driver callback under ops cannot be run unless ops_sem is held
 	 * (sometimes implicitly, eg for the sysfs code). ops becomes null
@@ -493,8 +493,10 @@  static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
 }
 
 extern struct class *tpm_class;
+extern struct class *tpm_rm_class;
 extern dev_t tpm_devt;
 extern const struct file_operations tpm_fops;
+extern const struct file_operations tpm_rm_fops;
 extern struct idr dev_nums_idr;
 
 enum tpm_transmit_flags {