[PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Jarkko Sakkinen]

On Thu, Aug 24, 2017 at 10:37:14AM +0200, Alexander Steffen wrote:
> When one of the commands during the auto_startup sequences does not return
> TPM_RC_SUCCESS, tpm_chip_register misleadingly returns ENODEV, even though
> a TPM device is definitely present.
> 
> An error response during those sequences is indeed unexpected, so to
> prevent subsequent errors, the kernel should not make use of the TPM
> device. But user space applications still might be able to communicate with
> the TPM, so they can be used to further diagnose and/or fix the problem. To
> allow this, with this patch the device is still exported to user space,
> even if a TPM error code has been received, but the kernel itself will not
> be allowed to use the device for anything.
> 
> This is not a hypothetical scenario, but there are devices in the wild that
> show this behavior. With this fix, those devices can be recovered from
> their failed state.
> 
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>

I can understand the benefits but you could make the same argument for
any class devices that kernel handles. I don't think it is that common
to let user space access malfunctioning devices.

Adding linux-security-module.

PS. You should have in *all* patches that you don't tag as RFC have
linux-kernel at vger.kernel.org. Now you have it in *none* of your patches.
When you don't have RFC you are saying out loud that this is production
ready code that should be included to the mainline kernel.

PPS. This patch set should be obviously RFC. It's  avery questionable and
intrusive change. That's why I didn't include linux-kernel.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Alexander.Steffen at infineon.com]

> On Thu, Aug 24, 2017 at 10:37:14AM +0200, Alexander Steffen wrote:
> > When one of the commands during the auto_startup sequences does not
> > return TPM_RC_SUCCESS, tpm_chip_register misleadingly returns
> ENODEV,
> > even though a TPM device is definitely present.
> >
> > An error response during those sequences is indeed unexpected, so to
> > prevent subsequent errors, the kernel should not make use of the TPM
> > device. But user space applications still might be able to communicate
> > with the TPM, so they can be used to further diagnose and/or fix the
> > problem. To allow this, with this patch the device is still exported
> > to user space, even if a TPM error code has been received, but the
> > kernel itself will not be allowed to use the device for anything.
> >
> > This is not a hypothetical scenario, but there are devices in the wild
> > that show this behavior. With this fix, those devices can be recovered
> > from their failed state.
> >
> > Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> 
> I can understand the benefits but you could make the same argument for
> any class devices that kernel handles.

In a similar situation I probably would make that argument for other devices as well :-)

> I don't think it is that common to let user space access malfunctioning
> devices.

But is that just because nobody bothered to implement the necessary logic or for some other reason?

In my opinion, it strongly depends on the effects of the malfunction. If the device does not respond at all or returns only garbage, it is of course not useful to make it available. But the case that I want to handle here has a TPM that communicates correctly, i.e. the TIS layer works fine, it just does not return the application-level responses that the kernel expects. I'd like to think of it more as a transparent communication channel that the kernel provides to user space applications, without interfering with the data that is transmitted, similar to how a TCP socket is a transparent channel, that does not care about HTTP 500 codes being transmitted. So it is okay for the kernel to refuse to use such a device internally (the kernel knows there is nothing it can do with a broken device), but why assume that no user space application can use it either?

I'd rather argue that user space applications already need to be prepared to handle TPM errors/malfunctions at any time, so removing this one check does not put them into a worse position. The TPM driver just checks *at startup* that the TPM seems to be okay (i.e. executes all self tests correctly). But if you keep your machine running for two years the TPM may fail at any point and the driver never checks it again, so an application may never assume that simply because /dev/tpm0 exists it can send arbitrary commands there that will always work (in fact, such a look-before-you-leap approach can never be guaranteed to succeed).

Also, what would be the alternative? Your TPM is broken in a way that the kernel does not export it, how do you debug/fix the problem? You cannot even execute a TPM2_GetTestResult to get more detailed error information. So at the moment, we have user space applications bypassing the kernel driver completely, by accessing /dev/mem and reimplementing all the TIS logic. But this is just duplicated code (with its own set of problems, for example, if the kernel driver is active at the same time, there is nothing that prevents concurrent accesses), so nobody wants to implement a similar solution for SPI, and I2C, and all the other interfaces that might be necessary in the future.

> PS. You should have in *all* patches that you don't tag as RFC have linux-
> kernel at vger.kernel.org. Now you have it in *none* of your patches.
> When you don't have RFC you are saying out loud that this is production
> ready code that should be included to the mainline kernel.
> 
> PPS. This patch set should be obviously RFC. It's  avery questionable and
> intrusive change. That's why I didn't include linux-kernel.

Sorry, thanks for explaining, will do that next time.

Alexander
????{.n?+???????+%???????\x17??w??{.n?+????{??????????v?^?)????w*\x1fjg???\x1e???????j??\a??G??????\f???j:+v???w?j?m?????\x1e??\x1e?w?????f???h?????????

[PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Jarkko Sakkinen]

On Mon, Aug 28, 2017 at 05:15:58PM +0000, Alexander.Steffen at infineon.com wrote:
> But is that just because nobody bothered to implement the necessary
> logic or for some other reason?

We do not want user space to access broken hardware. It's a huge risk
for system stability and potentially could be used for evil purposes.

This is not going to mainline as it is not suitable for general
consumption. You must use a patched kernel if you want this.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Michal Suchánek]

Hello,

On Tue, 29 Aug 2017 15:55:09 +0300
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:

> On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> Alexander.Steffen at infineon.com wrote:
> > But is that just because nobody bothered to implement the necessary
> > logic or for some other reason?  
> 
> We do not want user space to access broken hardware. It's a huge risk
> for system stability and potentially could be used for evil purposes.
> 
> This is not going to mainline as it is not suitable for general
> consumption. You must use a patched kernel if you want this.
> 
> /Jarkko
> 

It has been pointed out that userspace applications that use direct IO
access exist for the purpose. So using a kernel driver is an
improvement over that if the interface is otherwise sane.

What do you expect is the potential for instability or evil use?

With a kernel driver arbitrating the bus access as it would in any
other case I do not see much potential for instability. If there are
cases when the arbitration fails they can surely be more likely
triggered in cases other than userspace sending arbitrary requests to a
device which is in a state the kernel does not support but otherwise
responsive.

If you really think that accessing a device that is in unsupported
state at boot (as opposed to getting unto unsupported state during
device operation after boot) is a real problem it can be selectable as
compile time option so people who do not want the code do not get it.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Peter Huewe]

Thabks Michal!

Am 29. August 2017 15:17:39 MESZ schrieb "Michal Such?nek" <msuchanek@suse.de>:
>Hello,
>
>On Tue, 29 Aug 2017 15:55:09 +0300
>Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
>
>> On Mon, Aug 28, 2017 at 05:15:58PM +0000,
>> Alexander.Steffen at infineon.com wrote:
>> > But is that just because nobody bothered to implement the necessary
>> > logic or for some other reason?  
>> 
>> We do not want user space to access broken hardware. It's a huge risk
>> for system stability and potentially could be used for evil purposes.
>> 
>> This is not going to mainline as it is not suitable for general
>> consumption. You must use a patched kernel if you want this.
>> 
>> /Jarkko
>> 
>
>It has been pointed out that userspace applications that use direct IO
>access exist for the purpose. So using a kernel driver is an
>improvement over that if the interface is otherwise sane.
+1

>
>What do you expect is the potential for instability or evil use?
>
>With a kernel driver arbitrating the bus access as it would in any
>other case I do not see much potential for instability. 

Exactly.
 

>If there are
>cases when the arbitration fails they can surely be more likely
>triggered in cases other than userspace sending arbitrary requests to a
>device which is in a state the kernel does not support but otherwise
>responsive.

Its the same as with every other device - as long as it communicates like a tpm, talks like a tpm, behaves like a tpm - it's a tpm. 
Even if the kernel does not recognize the tpm's internal state.


>If you really think that accessing a device that is in unsupported
>state at boot (as opposed to getting unto unsupported state during
>device operation after boot) is a real problem it can be selectable as
>compile time option so people who do not want the code do not get it.

I would more favor a module option, 
but honestly I don't see a reason to not pull this code in, as it is.
Maybe mark the kernel as tainted if you think it is an issue.

Adding this as a out of tree patch is far from userfriendly.
If there is a chance to debug and fix a "unknown" state using the kernel as the communication layer, I would like to use this way.


Peter

>
>Thanks
>
>Michal
>
>------------------------------------------------------------------------------
>Check out the vibrant tech community on one of the world's most
>engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>_______________________________________________
>tpmdd-devel mailing list
>tpmdd-devel at lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

-- 
Sent from my mobile
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Jarkko Sakkinen]

On Tue, Aug 29, 2017 at 03:53:17PM +0200, Peter Huewe wrote:
> Thabks Michal!
> 
> Am 29. August 2017 15:17:39 MESZ schrieb "Michal Such?nek" <msuchanek@suse.de>:
> >Hello,
> >
> >On Tue, 29 Aug 2017 15:55:09 +0300
> >Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> >
> >> On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> >> Alexander.Steffen at infineon.com wrote:
> >> > But is that just because nobody bothered to implement the necessary
> >> > logic or for some other reason?  
> >> 
> >> We do not want user space to access broken hardware. It's a huge risk
> >> for system stability and potentially could be used for evil purposes.
> >> 
> >> This is not going to mainline as it is not suitable for general
> >> consumption. You must use a patched kernel if you want this.
> >> 
> >> /Jarkko
> >> 
> >
> >It has been pointed out that userspace applications that use direct IO
> >access exist for the purpose. So using a kernel driver is an
> >improvement over that if the interface is otherwise sane.
> +1
> 
> >
> >What do you expect is the potential for instability or evil use?
> >
> >With a kernel driver arbitrating the bus access as it would in any
> >other case I do not see much potential for instability. 
> 
> Exactly.
>  
> 
> >If there are
> >cases when the arbitration fails they can surely be more likely
> >triggered in cases other than userspace sending arbitrary requests to a
> >device which is in a state the kernel does not support but otherwise
> >responsive.
> 
> Its the same as with every other device - as long as it communicates like a tpm, talks like a tpm, behaves like a tpm - it's a tpm. 
> Even if the kernel does not recognize the tpm's internal state.
> 
> 
> >If you really think that accessing a device that is in unsupported
> >state at boot (as opposed to getting unto unsupported state during
> >device operation after boot) is a real problem it can be selectable as
> >compile time option so people who do not want the code do not get it.
> 
> I would more favor a module option, 
> but honestly I don't see a reason to not pull this code in, as it is.
> Maybe mark the kernel as tainted if you think it is an issue.
> 
> Adding this as a out of tree patch is far from userfriendly.
> If there is a chance to debug and fix a "unknown" state using the kernel as the communication layer, I would like to use this way.
> 
> 
> Peter

As I said in previous response it's not a good idea to give all-in
access at this point. You cannot turn back from that once it is in
the mainline.

I would rather suggest scoping lowest common denominator subset of
TPM commands that you need and allow only those commands to pass
through. If the subset turns out to be too limited, you can expand
it later on.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Jarkko Sakkinen]

On Tue, Aug 29, 2017 at 03:17:39PM +0200, Michal Such?nek wrote:
> Hello,
> 
> On Tue, 29 Aug 2017 15:55:09 +0300
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> 
> > On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> > Alexander.Steffen at infineon.com wrote:
> > > But is that just because nobody bothered to implement the necessary
> > > logic or for some other reason?  
> > 
> > We do not want user space to access broken hardware. It's a huge risk
> > for system stability and potentially could be used for evil purposes.
> > 
> > This is not going to mainline as it is not suitable for general
> > consumption. You must use a patched kernel if you want this.
> > 
> > /Jarkko
> > 
> 
> It has been pointed out that userspace applications that use direct IO
> access exist for the purpose. So using a kernel driver is an
> improvement over that if the interface is otherwise sane.
> 
> What do you expect is the potential for instability or evil use?

By definition the use of broken hardware can have unpredictable effects.
Use a patched kernel if you want to do it.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Jarkko Sakkinen]

On Wed, Aug 30, 2017 at 01:15:10PM +0300, Jarkko Sakkinen wrote:
> On Tue, Aug 29, 2017 at 03:17:39PM +0200, Michal Such?nek wrote:
> > Hello,
> > 
> > On Tue, 29 Aug 2017 15:55:09 +0300
> > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > 
> > > On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> > > Alexander.Steffen at infineon.com wrote:
> > > > But is that just because nobody bothered to implement the necessary
> > > > logic or for some other reason?  
> > > 
> > > We do not want user space to access broken hardware. It's a huge risk
> > > for system stability and potentially could be used for evil purposes.
> > > 
> > > This is not going to mainline as it is not suitable for general
> > > consumption. You must use a patched kernel if you want this.
> > > 
> > > /Jarkko
> > > 
> > 
> > It has been pointed out that userspace applications that use direct IO
> > access exist for the purpose. So using a kernel driver is an
> > improvement over that if the interface is otherwise sane.
> > 
> > What do you expect is the potential for instability or evil use?
> 
> By definition the use of broken hardware can have unpredictable effects.
> Use a patched kernel if you want to do it.
> 
> /Jarkko

I.e. too many unknown unknowns for mainline.

I could consider a solution for the TPM error case on self-test that
allows only restricted subset of commands.

The patch description did not go to *any* detail on how it is used so
practically it's unreviewable at this point. There's a big burder of
proof and now there's only hand waving.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Michal Suchánek]

On Wed, 30 Aug 2017 13:20:02 +0300
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:

> On Wed, Aug 30, 2017 at 01:15:10PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Aug 29, 2017 at 03:17:39PM +0200, Michal Such?nek wrote:  
> > > Hello,
> > > 
> > > On Tue, 29 Aug 2017 15:55:09 +0300
> > > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > >   
> > > > On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> > > > Alexander.Steffen at infineon.com wrote:  
> > > > > But is that just because nobody bothered to implement the
> > > > > necessary logic or for some other reason?    
> > > > 
> > > > We do not want user space to access broken hardware. It's a
> > > > huge risk for system stability and potentially could be used
> > > > for evil purposes.
> > > > 
> > > > This is not going to mainline as it is not suitable for general
> > > > consumption. You must use a patched kernel if you want this.
> > > > 
> > > > /Jarkko
> > > >   
> > > 
> > > It has been pointed out that userspace applications that use
> > > direct IO access exist for the purpose. So using a kernel driver
> > > is an improvement over that if the interface is otherwise sane.
> > > 
> > > What do you expect is the potential for instability or evil use?  
> > 
> > By definition the use of broken hardware can have unpredictable
> > effects. Use a patched kernel if you want to do it.
> > 
> > /Jarkko  
> 
> I.e. too many unknown unknowns for mainline.
> 
> I could consider a solution for the TPM error case on self-test that
> allows only restricted subset of commands.
> 
> The patch description did not go to *any* detail on how it is used so
> practically it's unreviewable at this point. There's a big burder of
> proof and now there's only hand waving.
> 
Hello,

there was a bug patched recently in which Linux was not sending the
shutdown command on system shutdown. Presumably with this bug some TPMs
consider being under attack and stop performing most functions.
However, you should be able to read the log if this is implemented
sanely. For that the TPM needs to be accessible.

There are probably other cases when the TPM might be useless for system
use but it might be useful to access it. For example, does Linux handle
uninitialized TPMs?

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Jarkko Sakkinen]

On Wed, Aug 30, 2017 at 12:34:16PM +0200, Michal Such?nek wrote:
> On Wed, 30 Aug 2017 13:20:02 +0300
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> 
> > On Wed, Aug 30, 2017 at 01:15:10PM +0300, Jarkko Sakkinen wrote:
> > > On Tue, Aug 29, 2017 at 03:17:39PM +0200, Michal Such?nek wrote:  
> > > > Hello,
> > > > 
> > > > On Tue, 29 Aug 2017 15:55:09 +0300
> > > > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > > >   
> > > > > On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> > > > > Alexander.Steffen at infineon.com wrote:  
> > > > > > But is that just because nobody bothered to implement the
> > > > > > necessary logic or for some other reason?    
> > > > > 
> > > > > We do not want user space to access broken hardware. It's a
> > > > > huge risk for system stability and potentially could be used
> > > > > for evil purposes.
> > > > > 
> > > > > This is not going to mainline as it is not suitable for general
> > > > > consumption. You must use a patched kernel if you want this.
> > > > > 
> > > > > /Jarkko
> > > > >   
> > > > 
> > > > It has been pointed out that userspace applications that use
> > > > direct IO access exist for the purpose. So using a kernel driver
> > > > is an improvement over that if the interface is otherwise sane.
> > > > 
> > > > What do you expect is the potential for instability or evil use?  
> > > 
> > > By definition the use of broken hardware can have unpredictable
> > > effects. Use a patched kernel if you want to do it.
> > > 
> > > /Jarkko  
> > 
> > I.e. too many unknown unknowns for mainline.
> > 
> > I could consider a solution for the TPM error case on self-test that
> > allows only restricted subset of commands.
> > 
> > The patch description did not go to *any* detail on how it is used so
> > practically it's unreviewable at this point. There's a big burder of
> > proof and now there's only hand waving.
> > 
> Hello,
> 
> there was a bug patched recently in which Linux was not sending the
> shutdown command on system shutdown. Presumably with this bug some TPMs
> consider being under attack and stop performing most functions.
> However, you should be able to read the log if this is implemented
> sanely. For that the TPM needs to be accessible.
> 
> There are probably other cases when the TPM might be useless for system
> use but it might be useful to access it. For example, does Linux handle
> uninitialized TPMs?
> 
> Thanks
> 
> Michal

Agreed. I still think it would make sense to start with a limited subset
of TPM commands, not with all-command-allowed.

I guess Alexander should be able to propose such subset.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Alexander.Steffen at infineon.com]

> On Wed, Aug 30, 2017 at 12:34:16PM +0200, Michal Such??nek wrote:
> > On Wed, 30 Aug 2017 13:20:02 +0300
> > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > > On Wed, Aug 30, 2017 at 01:15:10PM +0300, Jarkko Sakkinen wrote:
> > > > On Tue, Aug 29, 2017 at 03:17:39PM +0200, Michal Such??nek wrote:
> > > > > Hello,
> > > > >
> > > > > On Tue, 29 Aug 2017 15:55:09 +0300 Jarkko Sakkinen
> > > > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > > >
> > > > > > On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> > > > > > Alexander.Steffen at infineon.com wrote:
> > > > > > > But is that just because nobody bothered to implement the
> > > > > > > necessary logic or for some other reason?
> > > > > >
> > > > > > We do not want user space to access broken hardware. It's a
> > > > > > huge risk for system stability and potentially could be used
> > > > > > for evil purposes.
> > > > > >
> > > > > > This is not going to mainline as it is not suitable for
> > > > > > general consumption. You must use a patched kernel if you want
> this.
> > > > > >
> > > > > > /Jarkko
> > > > > >
> > > > >
> > > > > It has been pointed out that userspace applications that use
> > > > > direct IO access exist for the purpose. So using a kernel driver
> > > > > is an improvement over that if the interface is otherwise sane.
> > > > >
> > > > > What do you expect is the potential for instability or evil use?
> > > >
> > > > By definition the use of broken hardware can have unpredictable
> > > > effects. Use a patched kernel if you want to do it.
> > > >
> > > > /Jarkko
> > >
> > > I.e. too many unknown unknowns for mainline.
> > >
> > > I could consider a solution for the TPM error case on self-test that
> > > allows only restricted subset of commands.
> > >
> > > The patch description did not go to *any* detail on how it is used
> > > so practically it's unreviewable at this point. There's a big burder
> > > of proof and now there's only hand waving.
> > >
> > Hello,
> >
> > there was a bug patched recently in which Linux was not sending the
> > shutdown command on system shutdown. Presumably with this bug some
> > TPMs consider being under attack and stop performing most functions.
> > However, you should be able to read the log if this is implemented
> > sanely. For that the TPM needs to be accessible.
> >
> > There are probably other cases when the TPM might be useless for
> > system use but it might be useful to access it. For example, does
> > Linux handle uninitialized TPMs?
> >
> > Thanks
> >
> > Michal

The two scenarios that I had in mind:

1. A TPM in failure mode. This signals a broken device, yes, but it is part of the specification, so we should support it. If it was unreasonable to talk to such a device, why specify failure mode in the first place?

2. A TPM in some vendor-specific mode. This is what happens during field upgrade with some Infineon TPMs. During the field upgrade process, the communication looks TPM-like (i.e. the usual 10-byte header is present), but it is not part of any (public) specification, and standard TPM commands are obviously unsupported. This works fine with the current driver, as long as you do not interrupt the upgrade process, as the driver never checks the device again during usage. But if the upgrade process is interrupted (e.g. by power loss) and the TPM has to start again in this vendor-specific mode, then the device simply disappears from the system and you have no chance to fix the problem.

> Agreed. I still think it would make sense to start with a limited subset of TPM
> commands, not with all-command-allowed.
> 
> I guess Alexander should be able to propose such subset.

For scenario #1 you could probably come up with a list of commands that are generally useful. But once you are restricted to those five commands, you block iterative debugging of the "I see where the problem might be, could you try to execute ..." fashion by requiring the other person to patch and rebuild their kernel.

For scenario #2 I see no chance to do that in a generic way. I could maybe tell you what the commands in this mode currently look like for Infineon TPMs, so that they can be whitelisted, but they might look different in the future and they are certainly different for other vendor's implementations.

So with both scenarios you end up with a lot of infrastructure for this whitelist approach in general and add a growing list of allowed commands, that are initially never present in the kernel version where you need them most, and where it then takes several months or years until they finally reach your users through official channels. And with all that in place, you gain what exactly? Why is it even useful to whitelist functionality based on command codes? If the device is broken, it might be that just some TIS registers do not work correctly. Or maybe just some parameter combinations for specific commands do not work anymore (e.g. your RSA hardware is broken, so RSA operations fail, but ECC is still good).

My point is: You already need to guard against misbehaving devices, but you cannot do so by trying to classify devices as either "good" or "bad". A device that initially looks good can turn bad at any time. So for example, you cannot run an endless loop until the good device returns the expected answer, but you always have to use proper timeouts. If such safeguards are missing somewhere, then it is already a bug that needs to be fixed. But if such safeguards are present everywhere where needed, there should be nothing that a good or a bad device can do, that harms the kernel or any (well-written) application.

Alexander
????{.n?+???????+%???????\x17??w??{.n?+????{??????????v?^?)????w*\x1fjg???\x1e???????j??\a??G??????\f???j:+v???w?j?m?????\x1e??\x1e?w?????f???h?????????

[tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Jarkko Sakkinen]

On Thu, Aug 31, 2017 at 04:18:42PM +0000, Alexander.Steffen at infineon.com wrote:
> > I guess Alexander should be able to propose such subset.
> 
> For scenario #1 you could probably come up with a list of commands
> that are generally useful. But once you are restricted to those five
> commands, you block iterative debugging of the "I see where the
> problem might be, could you try to execute ..." fashion by requiring
> the other person to patch and rebuild their kernel.

If the subset turns out to be wrong, it can be revisited.

> For scenario #2 I see no chance to do that in a generic way. I could
> maybe tell you what the commands in this mode currently look like for
> Infineon TPMs, so that they can be whitelisted, but they might look
> different in the future and they are certainly different for other
> vendor's implementations.

It's easy to check whether a command is vendor specific and allow to
pass those through.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Peter Huewe]

Am 30. August 2017 12:15:10 MESZ schrieb Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>:
>On Tue, Aug 29, 2017 at 03:17:39PM +0200, Michal Such?nek wrote:
>> Hello,
>> 
>> On Tue, 29 Aug 2017 15:55:09 +0300
>> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
>> 
>> > On Mon, Aug 28, 2017 at 05:15:58PM +0000,
>> > Alexander.Steffen at infineon.com wrote:
>> > > But is that just because nobody bothered to implement the
>necessary
>> > > logic or for some other reason?  
>> > 
>> > We do not want user space to access broken hardware. It's a huge
>risk
>> > for system stability and potentially could be used for evil
>purposes.
>> > 
>> > This is not going to mainline as it is not suitable for general
>> > consumption. You must use a patched kernel if you want this.
>> > 
>> > /Jarkko
>> > 
>> 
>> It has been pointed out that userspace applications that use direct
>IO
>> access exist for the purpose. So using a kernel driver is an
>> improvement over that if the interface is otherwise sane.
>> 
>> What do you expect is the potential for instability or evil use?
>
>By definition the use of broken hardware can have unpredictable
>effects.
>Use a patched kernel if you want to do it.

If the s.m.a.r.t selftest of your hard disk fails, you can still access it, even though the hw selftest says it is broken.
Same situation.



>
>/Jarkko
>
>------------------------------------------------------------------------------
>Check out the vibrant tech community on one of the world's most
>engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>_______________________________________________
>tpmdd-devel mailing list
>tpmdd-devel at lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

-- 
Sent from my mobile
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Jarkko Sakkinen]

On Wed, Aug 30, 2017 at 12:41:51PM +0200, Peter Huewe wrote:
> 
> 
> Am 30. August 2017 12:15:10 MESZ schrieb Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>:
> >On Tue, Aug 29, 2017 at 03:17:39PM +0200, Michal Such?nek wrote:
> >> Hello,
> >> 
> >> On Tue, 29 Aug 2017 15:55:09 +0300
> >> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> >> 
> >> > On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> >> > Alexander.Steffen at infineon.com wrote:
> >> > > But is that just because nobody bothered to implement the
> >necessary
> >> > > logic or for some other reason?  
> >> > 
> >> > We do not want user space to access broken hardware. It's a huge
> >risk
> >> > for system stability and potentially could be used for evil
> >purposes.
> >> > 
> >> > This is not going to mainline as it is not suitable for general
> >> > consumption. You must use a patched kernel if you want this.
> >> > 
> >> > /Jarkko
> >> > 
> >> 
> >> It has been pointed out that userspace applications that use direct
> >IO
> >> access exist for the purpose. So using a kernel driver is an
> >> improvement over that if the interface is otherwise sane.
> >> 
> >> What do you expect is the potential for instability or evil use?
> >
> >By definition the use of broken hardware can have unpredictable
> >effects.
> >Use a patched kernel if you want to do it.
> 
> If the s.m.a.r.t selftest of your hard disk fails, you can still
> access it, even though the hw selftest says it is broken.
> Same situation.

Not sure if you can compare these directly although I get your point.

Waiting for more comments on this. At the moment I'm still dilated to
restricted access because it gives more variables for the future.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Alexander.Steffen at infineon.com]

> On Wed, Aug 30, 2017 at 12:41:51PM +0200, Peter Huewe wrote:
> >
> >
> > Am 30. August 2017 12:15:10 MESZ schrieb Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com>:
> > >On Tue, Aug 29, 2017 at 03:17:39PM +0200, Michal Such??nek wrote:
> > >> Hello,
> > >>
> > >> On Tue, 29 Aug 2017 15:55:09 +0300
> > >> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > >>
> > >> > On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> > >> > Alexander.Steffen at infineon.com wrote:
> > >> > > But is that just because nobody bothered to implement the
> > >necessary
> > >> > > logic or for some other reason?
> > >> >
> > >> > We do not want user space to access broken hardware. It's a huge
> > >risk
> > >> > for system stability and potentially could be used for evil
> > >purposes.
> > >> >
> > >> > This is not going to mainline as it is not suitable for general
> > >> > consumption. You must use a patched kernel if you want this.
> > >> >
> > >> > /Jarkko
> > >> >
> > >>
> > >> It has been pointed out that userspace applications that use direct
> > >IO
> > >> access exist for the purpose. So using a kernel driver is an
> > >> improvement over that if the interface is otherwise sane.
> > >>
> > >> What do you expect is the potential for instability or evil use?
> > >
> > >By definition the use of broken hardware can have unpredictable
> > >effects.
> > >Use a patched kernel if you want to do it.
> >
> > If the s.m.a.r.t selftest of your hard disk fails, you can still
> > access it, even though the hw selftest says it is broken.
> > Same situation.
> 
> Not sure if you can compare these directly although I get your point.
> 
> Waiting for more comments on this. At the moment I'm still dilated to
> restricted access because it gives more variables for the future.
> 
> /Jarkko

I think, this is a perfect example. In both cases we have intelligent devices, that can detect that they are broken and report it correctly (instead of just malfunction randomly). This also shows that those devices are not so broken that they completely misbehave (i.e. significantly violate their spec), they just do not provide the full functionality anymore.

Could you give me one example how you'd imagine such a device to impact stability? My patch already prevents the kernel from using broken TPMs for anything, it only provides the communication facilities for user space applications.

With regard to security/attacks, it is precisely the TPM's job to protect itself (and your secrets). You get all your security guarantees from the TPM, not the driver code, so it does not matter what the driver does, even with a broken TPM.

I understand your point of not wanting to enable functionality that you cannot disable anymore, but I fail to see why you'd ever want to disable (part of) this functionality again. Knowing more about the potential problems would allow me to prevent them from the beginning or to come up with a better safeguard than the command-based whitelisting.

Alexander
????{.n?+???????+%???????\x17??w??{.n?+????{??????????v?^?)????w*\x1fjg???\x1e???????j??\a??G??????\f???j:+v???w?j?m?????\x1e??\x1e?w?????f???h?????????

[tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Jarkko Sakkinen]

On Thu, Aug 31, 2017 at 04:26:10PM +0000, Alexander.Steffen at infineon.com wrote:
> > On Wed, Aug 30, 2017 at 12:41:51PM +0200, Peter Huewe wrote:
> > >
> > >
> > > Am 30. August 2017 12:15:10 MESZ schrieb Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com>:
> > > >On Tue, Aug 29, 2017 at 03:17:39PM +0200, Michal Such?nek wrote:
> > > >> Hello,
> > > >>
> > > >> On Tue, 29 Aug 2017 15:55:09 +0300
> > > >> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > > >>
> > > >> > On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> > > >> > Alexander.Steffen at infineon.com wrote:
> > > >> > > But is that just because nobody bothered to implement the
> > > >necessary
> > > >> > > logic or for some other reason?
> > > >> >
> > > >> > We do not want user space to access broken hardware. It's a huge
> > > >risk
> > > >> > for system stability and potentially could be used for evil
> > > >purposes.
> > > >> >
> > > >> > This is not going to mainline as it is not suitable for general
> > > >> > consumption. You must use a patched kernel if you want this.
> > > >> >
> > > >> > /Jarkko
> > > >> >
> > > >>
> > > >> It has been pointed out that userspace applications that use direct
> > > >IO
> > > >> access exist for the purpose. So using a kernel driver is an
> > > >> improvement over that if the interface is otherwise sane.
> > > >>
> > > >> What do you expect is the potential for instability or evil use?
> > > >
> > > >By definition the use of broken hardware can have unpredictable
> > > >effects.
> > > >Use a patched kernel if you want to do it.
> > >
> > > If the s.m.a.r.t selftest of your hard disk fails, you can still
> > > access it, even though the hw selftest says it is broken.
> > > Same situation.
> > 
> > Not sure if you can compare these directly although I get your point.
> > 
> > Waiting for more comments on this. At the moment I'm still dilated to
> > restricted access because it gives more variables for the future.
> > 
> > /Jarkko
> 
> I think, this is a perfect example. In both cases we have intelligent devices, that can detect that they are broken and report it correctly (instead of just malfunction randomly). This also shows that those devices are not so broken that they completely misbehave (i.e. significantly violate their spec), they just do not provide the full functionality anymore.
> 
> Could you give me one example how you'd imagine such a device to impact stability? My patch already prevents the kernel from using broken TPMs for anything, it only provides the communication facilities for user space applications.
> 
> With regard to security/attacks, it is precisely the TPM's job to protect itself (and your secrets). You get all your security guarantees from the TPM, not the driver code, so it does not matter what the driver does, even with a broken TPM.
> 
> I understand your point of not wanting to enable functionality that you cannot disable anymore, but I fail to see why you'd ever want to disable (part of) this functionality again. Knowing more about the potential problems would allow me to prevent them from the beginning or to come up with a better safeguard than the command-based whitelisting.
> 
> Alexander

I think we should hold with this patch set up until we get a mailing
list for tpmdd, which we don't have right now. I've submitted a request
to postmaster at vger.kernel.org but haven't received any feedback. I'll
inform in the linux-security-module list when the new list is created
so you know.

Some of the comments make sense like the one Peter mentioned earlier
and your comments about TPM security but you failed to address these
in the cover letter. For next iteration put this to the cover letter.

Thanks.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RESEND 1/3] tpm-chip: Move idr_replace calls to appropriate places

[Jarkko Sakkinen]

On Thu, Aug 24, 2017 at 10:37:12AM +0200, Alexander Steffen wrote:
> According to the comments, adding/removing the chip from the list should be
> the first/last action in (un)register. But currently it is done in a
> subfunction in the middle of the process. Moving the code from the
> subfunctions to the appropriate places within (un)register ensures that the
> code matches the comments.
> 
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> ---
>  drivers/char/tpm/tpm-chip.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 67ec9d3..a353b7a 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -327,11 +327,6 @@ static int tpm_add_char_device(struct tpm_chip *chip)
>  		}
>  	}
>  
> -	/* Make the chip available. */
> -	mutex_lock(&idr_lock);
> -	idr_replace(&dev_nums_idr, chip, chip->dev_num);
> -	mutex_unlock(&idr_lock);
> -
>  	return rc;
>  }
>  
> @@ -339,11 +334,6 @@ static void tpm_del_char_device(struct tpm_chip *chip)
>  {
>  	cdev_device_del(&chip->cdev, &chip->dev);
>  
> -	/* Make the chip unavailable. */
> -	mutex_lock(&idr_lock);
> -	idr_replace(&dev_nums_idr, NULL, chip->dev_num);
> -	mutex_unlock(&idr_lock);
> -
>  	/* Make the driver uncallable. */
>  	down_write(&chip->ops_sem);
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> @@ -438,6 +428,11 @@ int tpm_chip_register(struct tpm_chip *chip)
>  		return rc;
>  	}
>  
> +	/* Make the chip available. */
> +	mutex_lock(&idr_lock);
> +	idr_replace(&dev_nums_idr, chip, chip->dev_num);
> +	mutex_unlock(&idr_lock);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(tpm_chip_register);
> @@ -457,6 +452,11 @@ EXPORT_SYMBOL_GPL(tpm_chip_register);
>   */
>  void tpm_chip_unregister(struct tpm_chip *chip)
>  {
> +	/* Make the chip unavailable. */
> +	mutex_lock(&idr_lock);
> +	idr_replace(&dev_nums_idr, NULL, chip->dev_num);
> +	mutex_unlock(&idr_lock);
> +
>  	tpm_del_legacy_sysfs(chip);
>  	tpm_bios_log_teardown(chip);
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> -- 

This is unnecessary and questionable code shuffling in a very critical
places of the driver code where race conditions are easily introduced.

If you don't have a better reason to do this, I'm not going to take
this. I also fail to see the connection to the patch set as whole.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RESEND 1/3] tpm-chip: Move idr_replace calls to appropriate places

[Alexander.Steffen at infineon.com]

> On Thu, Aug 24, 2017 at 10:37:12AM +0200, Alexander Steffen wrote:
> > According to the comments, adding/removing the chip from the list
> > should be the first/last action in (un)register. But currently it is
> > done in a subfunction in the middle of the process. Moving the code
> > from the subfunctions to the appropriate places within (un)register
> > ensures that the code matches the comments.
> >
> > Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> > ---
> >  drivers/char/tpm/tpm-chip.c | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > index 67ec9d3..a353b7a 100644
> > --- a/drivers/char/tpm/tpm-chip.c
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -327,11 +327,6 @@ static int tpm_add_char_device(struct tpm_chip
> *chip)
> >  		}
> >  	}
> >
> > -	/* Make the chip available. */
> > -	mutex_lock(&idr_lock);
> > -	idr_replace(&dev_nums_idr, chip, chip->dev_num);
> > -	mutex_unlock(&idr_lock);
> > -
> >  	return rc;
> >  }
> >
> > @@ -339,11 +334,6 @@ static void tpm_del_char_device(struct tpm_chip
> > *chip)  {
> >  	cdev_device_del(&chip->cdev, &chip->dev);
> >
> > -	/* Make the chip unavailable. */
> > -	mutex_lock(&idr_lock);
> > -	idr_replace(&dev_nums_idr, NULL, chip->dev_num);
> > -	mutex_unlock(&idr_lock);
> > -
> >  	/* Make the driver uncallable. */
> >  	down_write(&chip->ops_sem);
> >  	if (chip->flags & TPM_CHIP_FLAG_TPM2) @@ -438,6 +428,11 @@ int
> > tpm_chip_register(struct tpm_chip *chip)
> >  		return rc;
> >  	}
> >
> > +	/* Make the chip available. */
> > +	mutex_lock(&idr_lock);
> > +	idr_replace(&dev_nums_idr, chip, chip->dev_num);
> > +	mutex_unlock(&idr_lock);
> > +
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(tpm_chip_register);
> > @@ -457,6 +452,11 @@ EXPORT_SYMBOL_GPL(tpm_chip_register);
> >   */
> >  void tpm_chip_unregister(struct tpm_chip *chip)  {
> > +	/* Make the chip unavailable. */
> > +	mutex_lock(&idr_lock);
> > +	idr_replace(&dev_nums_idr, NULL, chip->dev_num);
> > +	mutex_unlock(&idr_lock);
> > +
> >  	tpm_del_legacy_sysfs(chip);
> >  	tpm_bios_log_teardown(chip);
> >  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> > --
> 
> This is unnecessary and questionable code shuffling in a very critical places of
> the driver code where race conditions are easily introduced.

Can you explain what race conditions you fear here?

My understanding of the code so far is this: There are two separate paths to the TPM (from kernel and user space), that share the common driver code (tpm_transmit and everything below), but that can (in theory) exist without the other, i.e. the kernel can use the TPM without ever exporting it to user space and user space applications can send commands to the TPM without the kernel using the TPM for anything.

If the kernel wants to use the TPM, it needs to go through tpm_chip_find_get at some point. Every request from user space passes through tpm_common_write (except for everything from tpm-sysfs.c, that also somehow lacks the serialization imposed by tpm_try_get_ops, but that is a different problem). By not placing the chip into dev_nums_idr, I prevent the kernel from using the TPM while leaving the user space path intact.

So, based on those assumptions, that the kernel and user space paths are independent, until they meet at tpm_transmit, which is serialized by tpm_try_get_ops, it should not matter in what order I make the device available for kernel or user space usage, or whether I do not make it available for one of them at all. What race conditions could there be?

> If you don't have a better reason to do this, I'm not going to take this.

The comments currently state that the idr_replace calls should be the first/last step in the process, so either the code or the comments are wrong and need to be changed. I opted for changing the code, since, as explained above, I cannot see how the kernel and user space paths in this place interact (i.e. as far as I understand the code, you can place the idr_replace call anywhere after the call to tpm*_auto_startup, without being able to detect a difference). Also, "somewhere in the middle, add the chip to the list" does not make for a very useful comment ;-)

> I also fail to see the connection to the patch set as whole.

PATCH 3/3 needs a way to skip the idr_replace call under some circumstances, and this seemed like a cleaner solution than passing around additional flags, that also fixed the comment/code mismatch.

Alexander
????{.n?+???????+%???????\x17??w??{.n?+????{??????????v?^?)????w*\x1fjg???\x1e???????j??\a??G??????\f???j:+v???w?j?m?????\x1e??\x1e?w?????f???h?????????