2009-09-14 18:36:33

by Jonathan M. McCune

[permalink] [raw]
Subject: Re: [tpmdd-devel] [PATCH] TPM: Fixup pubek sysfs file

Hello everybody,

I have decided to chime in as a frequent (and I'm going to claim
"experienced") user of the linux kernel TPM driver.

To the best of my knowledge there is very little code in the wild that
depends on the current structure of these sysfs entries. Probably the
most used set of libraries and programs -- "TrouSerS" and "TPM Tools"
(trousers.sourceforge.net) -- interact directly with the TPM via
/dev/tpm*. I'm fairly confident that "tboot" (tboot.sourceforge.net)
and "jTPM Tools" (trustedjava.sourceforge.net) don't use the sysfs
entries either.

Thus, fixing the one-item-per-file issues seems reasonable to me. For
example, changing /sys/devices/platform/tpm_tis/pcrs from a single
multi-entry file to a directory containing files named 0-15 or 0-23 that
each then contain only the relevant 20-byte value seems quite
reasonable. (Today's TPMs contain either 16 or 24 PCRs but future ones
could contain many more.)

I believe the pubek entry has gone broken for so long because once a TPM
has been "owned" (and "taking ownership" is almost certainly the first
thing one does when using the TPM for something), accessing the pubek
needs to be authorized and so the sysfs entry shouldn't return anything
anyways ("Authorization required."?). But it should definitely be fixed
or removed.

Likewise "caps" could be made into a directory containing files for the
relevant data (Manufacturer, TCG version, Firmware version, ...).

What I _disagree_ with is that these values should be moved debugfs.
They are characteristics of a particular hardware device's current state
and I believe fit the definition of what belongs in sysfs reasonably
well. Their current implementation simply needs help.

The changes I've mentioned are larger and don't seem appropriate for
2.6.31, but perhaps a plan to have them implemented for an upcoming
version is reasonable? Is anybody willing to help put together such a
patch? Discuss whether it's reasonable / acceptable to do so?

Thanks,
-Jon


Greg KH wrote:
> On Fri, Sep 04, 2009 at 03:23:16PM -0600, Jason Gunthorpe wrote:
>> On Fri, Sep 04, 2009 at 02:03:25PM -0700, Andrew Morton wrote:
>>> On Thu, 03 Sep 2009 22:02:24 -0700
>>> Roland Dreier <[email protected]> wrote:
>>>
>>>> > Algorithm: 00 00 00 01
>>>> > Encscheme: 00 03
>>>> > Sigscheme: 00 01
>>>> > Parameters: 00 00 08 00 00 00 00 02 00 00 00 00
>>>> > Modulus length: 256
>>>>
>>>> No argument that this fix is an improvement, but what happened to the
>>>> "one value per file" rule for sysfs??
>>> We goofed. This one snuck through.
>>>
>>> It's one of the reasons why I recommend that people cite examples of
>>> the sysfs/procfs output in changelogs. These things are part of the
>>> kernel ABI and once they go in we cannot change them so we have to get
>>> them right first time.
>> Several of the sysfs files in the TPM driver are goofed:
>>
>> /sys/devices/platform/tpm_tis.0 # cat caps
>> Manufacturer: 0x57454300
>> TCG version: 1.2
>> Firmware version: 2.16
>>
>> /sys/devices/platform/tpm_tis.0 # cat pcrs
>> PCR-00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> PCR-01: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> PCR-02: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> PCR-03: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> PCR-04: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> PCR-05: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> PCR-06: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> PCR-07: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> PCR-08: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> PCR-09: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> PCR-10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> PCR-11: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> PCR-12: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> PCR-13: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> PCR-14: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> PCR-15: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> PCR-16: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> PCR-17: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
>> PCR-18: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
>> PCR-19: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
>> PCR-20: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
>> PCR-21: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
>> PCR-22: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
>> PCR-23: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>
>> /sys/devices/platform/tpm_tis.0 # cat pubek
>> Algorithm: 00 00 00 01
>> Encscheme: 00 03
>> Sigscheme: 00 01
>> Parameters: 00 00 08 00 00 00 00 02 00 00 00 00
>> Modulus length: 256
>> Modulus:
>> AF 6B 30 9B 0D B3 B4 7D 74 35 68 45 8D CA A5 EF
>> A8 8A DD 0F D7 84 8E 8D 1F 40 22 92 09 CF 12 C8
>> 9F 6E 55 57 6A 2C A8 0B 5E 45 C7 5E 3D 56 DA 64
>> E6 E1 F7 8C DD 41 92 28 2E 06 41 02 3E 11 7D B2
>> C5 46 38 E5 8C 60 D2 96 EE 0C D6 3D F8 99 E3 02
>> 3A 32 7A 02 C8 31 29 98 28 B9 1B EF 30 A1 A0 45
>> A0 C0 05 0E C5 96 95 FD 91 47 0A 35 E0 69 B0 8B
>> 49 BD B9 F6 5D 25 21 25 79 1B 20 0D C3 C7 1F 87
>> 5E 5F 41 4B DE 32 DF 55 F3 BD 7F CA D8 7D 3A B4
>> D5 0A EF CF 8E 72 20 52 15 FA FB C6 C0 2E C2 AB
>> C6 07 D0 9D 96 6B 2E 30 F7 54 C4 A5 CD 9B 13 54
>> A0 D1 71 66 91 97 06 12 B5 2D B2 33 62 FB 56 62
>> 64 A8 AA E9 F2 F4 03 C3 F4 49 2A 09 0D 7D 75 99
>> 6C F0 47 1E 7D D5 A5 CA CE EF 45 B2 DA 88 93 B4
>> EE EB FB B0 A6 A4 19 C4 B8 0D 04 46 AE BD C5 2E
>> 30 84 49 57 25 34 78 E6 ED C4 50 AF 3B F6 86 43
>>
>> At least this has been like this since before the git history, so it
>> isn't recent..
>>
>> Can these be scheduled for removal or some such process? I haven't
>> seen anything actually using the files, the only one that is really
>> useful is caps...
>>
>> They probably should have been in debugfs, had it existed at the time.
>
> Feel free to move them to debugfs, it looks like that is what they are
> for.
>
> thanks,
>
> greg k-h
>
> ------------------------------------------------------------------------------
> Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
> trial. Simplify your report design, integration and deployment - and focus on
> what you do best, core application coding. Discover what's new with
> Crystal Reports now. http://p.sf.net/sfu/bobj-july
> _______________________________________________
> tpmdd-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
>


2009-09-14 18:35:01

by Rajiv Andrade

[permalink] [raw]
Subject: Re: [tpmdd-devel] [PATCH] TPM: Fixup pubek sysfs file

Ok, when reading the other queued patches I saw this discussion and
meant to write about this too.

On Mon, 2009-09-14 at 14:01 -0400, Jonathan M. McCune wrote:
> Hello everybody,
>
> I have decided to chime in as a frequent (and I'm going to claim
> "experienced") user of the linux kernel TPM driver.
>
> To the best of my knowledge there is very little code in the wild that
> depends on the current structure of these sysfs entries. Probably the
> most used set of libraries and programs -- "TrouSerS" and "TPM Tools"
> (trousers.sourceforge.net) -- interact directly with the TPM via
> /dev/tpm*. I'm fairly confident that "tboot" (tboot.sourceforge.net)
> and "jTPM Tools" (trustedjava.sourceforge.net) don't use the sysfs
> entries either.

At least for TrouSerS I can say it doesn't use sysfs at all. tpm-tools
runs on top of TrouSerS, so the same apply. And for jTPM, it can use the
jTSS wrapper, that wraps TrouSerS function calls, so no sysfs here too,
and IAIK jTSS, that's is intented to be cross-platform, so no sysfs here
too I assume. If I'm not wrong tboot doesn't run in user space. So, good
points for us.

>
> Thus, fixing the one-item-per-file issues seems reasonable to me. For
> example, changing /sys/devices/platform/tpm_tis/pcrs from a single
> multi-entry file to a directory containing files named 0-15 or 0-23 that
> each then contain only the relevant 20-byte value seems quite
> reasonable. (Today's TPMs contain either 16 or 24 PCRs but future ones
> could contain many more.)
>

The number of PCRs of a particular TPM can be retrieved sending it the
TPM_GetCapability command, so it's feasible to build this directory
structure dynamically (chip independent).


> The changes I've mentioned are larger and don't seem appropriate for
> 2.6.31, but perhaps a plan to have them implemented for an upcoming
> version is reasonable? Is anybody willing to help put together such a
> patch? Discuss whether it's reasonable / acceptable to do so?
>

Count on me. ;-)

Do you all agree? I'll start writing something/patches and will post
here for further discussions.

Rajiv

> Thanks,
> -Jon
>
>
> Greg KH wrote:
> > On Fri, Sep 04, 2009 at 03:23:16PM -0600, Jason Gunthorpe wrote:
> >> On Fri, Sep 04, 2009 at 02:03:25PM -0700, Andrew Morton wrote:
> >>> On Thu, 03 Sep 2009 22:02:24 -0700
> >>> Roland Dreier <[email protected]> wrote:
> >>>
> >>>> > Algorithm: 00 00 00 01
> >>>> > Encscheme: 00 03
> >>>> > Sigscheme: 00 01
> >>>> > Parameters: 00 00 08 00 00 00 00 02 00 00 00 00
> >>>> > Modulus length: 256
> >>>>
> >>>> No argument that this fix is an improvement, but what happened to the
> >>>> "one value per file" rule for sysfs??
> >>> We goofed. This one snuck through.
> >>>
> >>> It's one of the reasons why I recommend that people cite examples of
> >>> the sysfs/procfs output in changelogs. These things are part of the
> >>> kernel ABI and once they go in we cannot change them so we have to get
> >>> them right first time.
> >> Several of the sysfs files in the TPM driver are goofed:
> >>
> >> /sys/devices/platform/tpm_tis.0 # cat caps
> >> Manufacturer: 0x57454300
> >> TCG version: 1.2
> >> Firmware version: 2.16
> >>
> >> /sys/devices/platform/tpm_tis.0 # cat pcrs
> >> PCR-00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> PCR-01: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> PCR-02: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> PCR-03: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> PCR-04: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> PCR-05: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> PCR-06: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> PCR-07: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> PCR-08: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> PCR-09: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> PCR-10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> PCR-11: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> PCR-12: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> PCR-13: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> PCR-14: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> PCR-15: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> PCR-16: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> PCR-17: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> >> PCR-18: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> >> PCR-19: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> >> PCR-20: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> >> PCR-21: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> >> PCR-22: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
> >> PCR-23: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>
> >> /sys/devices/platform/tpm_tis.0 # cat pubek
> >> Algorithm: 00 00 00 01
> >> Encscheme: 00 03
> >> Sigscheme: 00 01
> >> Parameters: 00 00 08 00 00 00 00 02 00 00 00 00
> >> Modulus length: 256
> >> Modulus:
> >> AF 6B 30 9B 0D B3 B4 7D 74 35 68 45 8D CA A5 EF
> >> A8 8A DD 0F D7 84 8E 8D 1F 40 22 92 09 CF 12 C8
> >> 9F 6E 55 57 6A 2C A8 0B 5E 45 C7 5E 3D 56 DA 64
> >> E6 E1 F7 8C DD 41 92 28 2E 06 41 02 3E 11 7D B2
> >> C5 46 38 E5 8C 60 D2 96 EE 0C D6 3D F8 99 E3 02
> >> 3A 32 7A 02 C8 31 29 98 28 B9 1B EF 30 A1 A0 45
> >> A0 C0 05 0E C5 96 95 FD 91 47 0A 35 E0 69 B0 8B
> >> 49 BD B9 F6 5D 25 21 25 79 1B 20 0D C3 C7 1F 87
> >> 5E 5F 41 4B DE 32 DF 55 F3 BD 7F CA D8 7D 3A B4
> >> D5 0A EF CF 8E 72 20 52 15 FA FB C6 C0 2E C2 AB
> >> C6 07 D0 9D 96 6B 2E 30 F7 54 C4 A5 CD 9B 13 54
> >> A0 D1 71 66 91 97 06 12 B5 2D B2 33 62 FB 56 62
> >> 64 A8 AA E9 F2 F4 03 C3 F4 49 2A 09 0D 7D 75 99
> >> 6C F0 47 1E 7D D5 A5 CA CE EF 45 B2 DA 88 93 B4
> >> EE EB FB B0 A6 A4 19 C4 B8 0D 04 46 AE BD C5 2E
> >> 30 84 49 57 25 34 78 E6 ED C4 50 AF 3B F6 86 43
> >>
> >> At least this has been like this since before the git history, so it
> >> isn't recent..
> >>
> >> Can these be scheduled for removal or some such process? I haven't
> >> seen anything actually using the files, the only one that is really
> >> useful is caps...
> >>
> >> They probably should have been in debugfs, had it existed at the time.
> >
> > Feel free to move them to debugfs, it looks like that is what they are
> > for.
> >
> > thanks,
> >
> > greg k-h
> >
> > ------------------------------------------------------------------------------
> > Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
> > trial. Simplify your report design, integration and deployment - and focus on
> > what you do best, core application coding. Discover what's new with
> > Crystal Reports now. http://p.sf.net/sfu/bobj-july
> > _______________________________________________
> > tpmdd-devel mailing list
> > [email protected]
> > https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
> >
>

2009-09-14 18:43:40

by Jason Gunthorpe

[permalink] [raw]
Subject: Re: [tpmdd-devel] [PATCH] TPM: Fixup pubek sysfs file

On Mon, Sep 14, 2009 at 03:34:39PM -0300, Rajiv Andrade wrote:

> > Thus, fixing the one-item-per-file issues seems reasonable to me. For
> > example, changing /sys/devices/platform/tpm_tis/pcrs from a single
> > multi-entry file to a directory containing files named 0-15 or 0-23 that
> > each then contain only the relevant 20-byte value seems quite
> > reasonable. (Today's TPMs contain either 16 or 24 PCRs but future ones
> > could contain many more.)

> The number of PCRs of a particular TPM can be retrieved sending it the
> TPM_GetCapability command, so it's feasible to build this directory
> structure dynamically (chip independent).

I think as we've seen, keeping the sysfs stuff working has a real
cost. So that should be balanced with an actual need. Further, talking
to the TPM and handling all the parsing properly is a huge complex
pain, it would be better to have this code gone completely.

Can anyone think of a reason why the PCRs should be accessed from
sysfs? I can't. They are only really useful in connection with other
complex TPM operations. Userspace can fully access them using the
relevant TPM calls.

The pubek should definately go, it is useless in 99% of cases.

The caps file should be split up into 'manufacturer' 'tcg_version'
(please include the spec rev too!!) and maybe 'firmware_version'. This
would at least be useful to udev.

Regards,
Jason

2009-09-14 19:25:21

by Jonathan M. McCune

[permalink] [raw]
Subject: Re: [tpmdd-devel] [PATCH] TPM: Fixup pubek sysfs file

Inline...

Jason Gunthorpe wrote:
> On Mon, Sep 14, 2009 at 03:34:39PM -0300, Rajiv Andrade wrote:
>
>>> Thus, fixing the one-item-per-file issues seems reasonable to me. For
>>> example, changing /sys/devices/platform/tpm_tis/pcrs from a single
>>> multi-entry file to a directory containing files named 0-15 or 0-23 that
>>> each then contain only the relevant 20-byte value seems quite
>>> reasonable. (Today's TPMs contain either 16 or 24 PCRs but future ones
>>> could contain many more.)
>
>> The number of PCRs of a particular TPM can be retrieved sending it the
>> TPM_GetCapability command, so it's feasible to build this directory
>> structure dynamically (chip independent).
>
> I think as we've seen, keeping the sysfs stuff working has a real
> cost. So that should be balanced with an actual need. Further, talking
> to the TPM and handling all the parsing properly is a huge complex
> pain, it would be better to have this code gone completely.

The "talking to the TPM" code is indeed somewhat complex, but see my
comments below regarding IMA (*).

> Can anyone think of a reason why the PCRs should be accessed from
> sysfs? I can't. They are only really useful in connection with other
> complex TPM operations. Userspace can fully access them using the
> relevant TPM calls.

Fair enough, though they are useful in combination with IMA and as a
sanity check that the driver is working correctly.

> The pubek should definately go, it is useless in 99% of cases.

Seconded.

> The caps file should be split up into 'manufacturer' 'tcg_version'
> (please include the spec rev too!!) and maybe 'firmware_version'. This
> would at least be useful to udev.

Seems reasonable.

* IBM's "Integrity Measurement Architecture (IMA)" is part of the Linux
kernel as of v2.6.30 (linux-ima.sourceforge.net). See
security/integrity/ima/*. This mechanism can make use of the TPM. I've
copied Reiner Sailer, one of the developers of IMA.

IMA can leverage the ability to read and extend PCRs, and its security
properties derive from that functionality being a part of the kernel.
PCR reading is currently part of the TPM driver in drivers/char/tpm/tpm.c.

Thus, to the question, "Can we remove the ability of the kernel to send
TPM commands such as GetCapability and PCRRead?" I believe the answer is
no. Or at least, not without causing a lot of work for the IMA system.
Although it may be worth discussing whether this functionality should be
moved from the driver into IMA.

To the question, "Can we remove the sysfs entries relating to PCRs and
pubek," I believe the answer is yes, if the resulting complexity
reduction actually turns out to be significant.

Regards,
-Jon

2009-09-14 19:46:33

by Jason Gunthorpe

[permalink] [raw]
Subject: Re: [tpmdd-devel] [PATCH] TPM: Fixup pubek sysfs file

On Mon, Sep 14, 2009 at 03:23:09PM -0400, Jonathan M. McCune wrote:

>> Can anyone think of a reason why the PCRs should be accessed from
>> sysfs? I can't. They are only really useful in connection with other
>> complex TPM operations. Userspace can fully access them using the
>> relevant TPM calls.
>
> Fair enough, though they are useful in combination with IMA and as a
> sanity check that the driver is working correctly.
[..]
> IMA can leverage the ability to read and extend PCRs, and its security
> properties derive from that functionality being a part of the kernel.
> PCR reading is currently part of the TPM driver in
> drivers/char/tpm/tpm.c.

Sanzzy

> Thus, to the question, "Can we remove the ability of the kernel to send
> TPM commands such as GetCapability and PCRRead?" I believe the answer is
> no. Or at least, not without causing a lot of work for the IMA system.
> Although it may be worth discussing whether this functionality should be
> moved from the driver into IMA.

Fair enough, fortunately TPM_PCRRead and TPM_Extend are two of the
simplest commands. As has been done special casing the fixed size
messages is easy and robust.

The pcrs file really isn't that offsensive to me, the PCR registers
can reasonably thought of as a single memory space. If it wasn't for
the PCR-%u prefix I think it would be fine for sysfs??

Ie:

BD ED 7B B2 EB AE BE 31 4E CC 57 82 81 64 FC 8A 09 FA 19 25
05 37 25 B5 24 59 14 A5 94 1A 42 97 39 E6 07 40 27 3A 7D 48
3A 3F 78 0F 11 A4 B4 99 69 FC AA 80 CD 6E 39 57 C3 3B 22 75

not:

PCR-00: BD ED 7B B2 EB AE BE 31 4E CC 57 82 81 64 FC 8A 09 FA 19 25
PCR-01: 05 37 25 B5 24 59 14 A5 94 1A 42 97 39 E6 07 40 27 3A 7D 48
PCR-02: 3A 3F 78 0F 11 A4 B4 99 69 FC AA 80 CD 6E 39 57 C3 3B 22 75

Greg?

Jason

2009-09-14 19:52:06

by Greg KH

[permalink] [raw]
Subject: Re: [tpmdd-devel] [PATCH] TPM: Fixup pubek sysfs file

On Mon, Sep 14, 2009 at 01:46:26PM -0600, Jason Gunthorpe wrote:
> On Mon, Sep 14, 2009 at 03:23:09PM -0400, Jonathan M. McCune wrote:
>
> >> Can anyone think of a reason why the PCRs should be accessed from
> >> sysfs? I can't. They are only really useful in connection with other
> >> complex TPM operations. Userspace can fully access them using the
> >> relevant TPM calls.
> >
> > Fair enough, though they are useful in combination with IMA and as a
> > sanity check that the driver is working correctly.
> [..]
> > IMA can leverage the ability to read and extend PCRs, and its security
> > properties derive from that functionality being a part of the kernel.
> > PCR reading is currently part of the TPM driver in
> > drivers/char/tpm/tpm.c.
>
> Sanzzy
>
> > Thus, to the question, "Can we remove the ability of the kernel to send
> > TPM commands such as GetCapability and PCRRead?" I believe the answer is
> > no. Or at least, not without causing a lot of work for the IMA system.
> > Although it may be worth discussing whether this functionality should be
> > moved from the driver into IMA.
>
> Fair enough, fortunately TPM_PCRRead and TPM_Extend are two of the
> simplest commands. As has been done special casing the fixed size
> messages is easy and robust.
>
> The pcrs file really isn't that offsensive to me, the PCR registers
> can reasonably thought of as a single memory space. If it wasn't for
> the PCR-%u prefix I think it would be fine for sysfs??
>
> Ie:
>
> BD ED 7B B2 EB AE BE 31 4E CC 57 82 81 64 FC 8A 09 FA 19 25
> 05 37 25 B5 24 59 14 A5 94 1A 42 97 39 E6 07 40 27 3A 7D 48
> 3A 3F 78 0F 11 A4 B4 99 69 FC AA 80 CD 6E 39 57 C3 3B 22 75
>
> not:
>
> PCR-00: BD ED 7B B2 EB AE BE 31 4E CC 57 82 81 64 FC 8A 09 FA 19 25
> PCR-01: 05 37 25 B5 24 59 14 A5 94 1A 42 97 39 E6 07 40 27 3A 7D 48
> PCR-02: 3A 3F 78 0F 11 A4 B4 99 69 FC AA 80 CD 6E 39 57 C3 3B 22 75
>
> Greg?

That should just be a binary sysfs file, as it shows something that is
represented in hardware, not anything that is modified by the kernel,
right?

thanks,

greg k-h

2009-09-15 03:06:31

by Mimi Zohar

[permalink] [raw]
Subject: Re: [tpmdd-devel] [PATCH] TPM: Fixup pubek sysfs file

On Mon, 2009-09-14 at 13:46 -0600, Jason Gunthorpe wrote:
> On Mon, Sep 14, 2009 at 03:23:09PM -0400, Jonathan M. McCune wrote:
>
> >> Can anyone think of a reason why the PCRs should be accessed from
> >> sysfs? I can't. They are only really useful in connection with other
> >> complex TPM operations. Userspace can fully access them using the
> >> relevant TPM calls.

> > Fair enough, though they are useful in combination with IMA and as a
> > sanity check that the driver is working correctly.

The IMA ltp testsuite currently uses the sysfs PCR values 0-7 to
validate the boot aggregate, the first measurement in the the IMA
measurement list.

> [..]
> > IMA can leverage the ability to read and extend PCRs, and its security
> > properties derive from that functionality being a part of the kernel.
> > PCR reading is currently part of the TPM driver in
> > drivers/char/tpm/tpm.c.
>
> Sanzzy
>
> > Thus, to the question, "Can we remove the ability of the kernel to send
> > TPM commands such as GetCapability and PCRRead?" I believe the answer is
> > no. Or at least, not without causing a lot of work for the IMA system.

Definitely! On boot, IMA reads pcr 0-7 to calculate the boot aggregate.
IMA then extends the PCR, normally PCR-10, with the boot aggregate and
stores it as the first measurement in the IMA measurement list. Each
subsequent measurement, based on policy, extends the PCR and is appended
to the IMA measurement list.

> > Although it may be worth discussing whether this functionality should be
> > moved from the driver into IMA.

Are you really suggesting moving the tpm_pcr_read()/extend() to IMA?
Why?

> Fair enough, fortunately TPM_PCRRead and TPM_Extend are two of the
> simplest commands. As has been done special casing the fixed size
> messages is easy and robust.

Mimi

2009-09-15 16:34:29

by Hal Finney

[permalink] [raw]
Subject: Re: [tpmdd-devel] [PATCH] TPM: Fixup pubek sysfs file

One file-based aspect of the TPM driver interface is the securityfs
file system, typically mounted at /sys/kernel/security/tpm0. This
includes the IMA and/or BIOS event measurements. Trousers does read
these files optionally, however it has to be configured in tcsd.conf.
But the file formats are shared between the kernel driver and the
Trousers parsing code. This functionality is necessary for Trousers to
report pre-boot and post-boot measurement events to user code, which
in turn is necessary for meaningful Quote operations in some contexts.

Hal Finney

On Mon, Sep 14, 2009 at 11:01 AM, Jonathan M. McCune <[email protected]> wrote:
> Hello everybody,
>
> I have decided to chime in as a frequent (and I'm going to claim
> "experienced") user of the linux kernel TPM driver.
>
> To the best of my knowledge there is very little code in the wild that
> depends on the current structure of these sysfs entries. ?Probably the
> most used set of libraries and programs -- "TrouSerS" and "TPM Tools"
> (trousers.sourceforge.net) -- interact directly with the TPM via
> /dev/tpm*. ?I'm fairly confident that "tboot" (tboot.sourceforge.net)
> and "jTPM Tools" (trustedjava.sourceforge.net) don't use the sysfs
> entries either.
>
> Thus, fixing the one-item-per-file issues seems reasonable to me. For
> example, changing /sys/devices/platform/tpm_tis/pcrs from a single
> multi-entry file to a directory containing files named 0-15 or 0-23 that
> each then contain only the relevant 20-byte value seems quite
> reasonable. (Today's TPMs contain either 16 or 24 PCRs but future ones
> could contain many more.)
>
> I believe the pubek entry has gone broken for so long because once a TPM
> has been "owned" (and "taking ownership" is almost certainly the first
> thing one does when using the TPM for something), accessing the pubek
> needs to be authorized and so the sysfs entry shouldn't return anything
> anyways ("Authorization required."?). But it should definitely be fixed
> or removed.
>
> Likewise "caps" could be made into a directory containing files for the
> relevant data (Manufacturer, TCG version, Firmware version, ...).
>
> What I _disagree_ with is that these values should be moved debugfs.
> They are characteristics of a particular hardware device's current state
> and I believe fit the definition of what belongs in sysfs reasonably
> well. ?Their current implementation simply needs help.
>
> The changes I've mentioned are larger and don't seem appropriate for
> 2.6.31, but perhaps a plan to have them implemented for an upcoming
> version is reasonable? ?Is anybody willing to help put together such a
> patch? ?Discuss whether it's reasonable / acceptable to do so?
>
> Thanks,
> -Jon

2009-09-15 18:54:30

by Jonathan M. McCune

[permalink] [raw]
Subject: Re: [tpmdd-devel] [PATCH] TPM: Fixup pubek sysfs file

>
>>> Although it may be worth discussing whether this functionality should be
>>> moved from the driver into IMA.
>
> Are you really suggesting moving the tpm_pcr_read()/extend() to IMA?
> Why?
>

I mentioned this since I'm not aware of anything else in the kernel that
uses this functionality. But this may also be a case of "it ain't
broke, so don't fix it."

Cheers,
-Jon