[PATCH 4/4] Improve handling of TPM2 event logs

[PATCH 4/4] Improve handling of TPM2 event logs

[Petr Vandrovec]

From: Petr Vandrovec <petr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>

When TPM2 log has entries with more than 3 digests, or with digests
not listed in the log header, log gets misparsed, eventually
leading to kernel complaint that code tried to vmalloc 512MB of
memory (I have no idea what would happen on bigger system).

So code should not parse only first 3 digests: both event header
and event itself are already in memory, so we can parse any number
of digests, as long as we do not try to parse whole memory when
given count of 0xFFFFFFFF.

So this change:

* Rejects event entry with more digests than log header describes.
  Digest types should be unique, and all should be described in
  log header, so there cannot be more digests in the event than in
  the header.

* Reject event entry with digest that is not described in the
  log header.  In theory code could hardcode information about
  digest IDs already assigned by TCG, but if firmware authors
  cannot get event log format right, why should anyone believe
  that they got event log content right.

Signed-off-by: Petr Vandrovec <petr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
---
 drivers/char/tpm/tpm2_eventlog.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tpm/tpm2_eventlog.c b/drivers/char/tpm/tpm2_eventlog.c
index 513897cf9c4b..6fe59b75152b 100644
--- a/drivers/char/tpm/tpm2_eventlog.c
+++ b/drivers/char/tpm/tpm2_eventlog.c
@@ -56,18 +56,23 @@ static int calc_tpm2_event_size(struct tcg_pcr_event2 *event,
 
 	efispecid = (struct tcg_efi_specid_event *)event_header->event;
 
-	for (i = 0; (i < event->count) && (i < TPM2_ACTIVE_PCR_BANKS);
-	     i++) {
+	/* Check if event is malformed. */
+	if (event->count > efispecid->num_algs)
+		return 0;
+
+	for (i = 0; i < event->count; i++) {
 		halg_size = sizeof(event->digests[i].alg_id);
 		memcpy(&halg, marker, halg_size);
 		marker = marker + halg_size;
-		for (j = 0; (j < efispecid->num_algs); j++) {
+		for (j = 0; j < efispecid->num_algs; j++) {
 			if (halg == efispecid->digest_sizes[j].alg_id) {
-				marker = marker +
-					efispecid->digest_sizes[j].digest_size;
+				marker += efispecid->digest_sizes[j].digest_size;
 				break;
 			}
 		}
+		/* Algorithm without known length. Such event is unparseable. */
+		if (j == efispecid->num_algs)
+			return 0;
 	}
 
 	event_field = (struct tcg_event_field *)marker;
-- 
2.11.0


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

Re: [PATCH 4/4] Improve handling of TPM2 event logs

[Jarkko Sakkinen]

On Wed, Mar 29, 2017 at 12:43:30AM -0700, Petr Vandrovec wrote:
> From: Petr Vandrovec <petr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> 
> When TPM2 log has entries with more than 3 digests, or with digests
> not listed in the log header, log gets misparsed, eventually
> leading to kernel complaint that code tried to vmalloc 512MB of
> memory (I have no idea what would happen on bigger system).

It would be easier to follow this if you would give a concrete
example of such situation. Since you are contributing this, you
must must have bumped into this, haven't you?

> So code should not parse only first 3 digests: both event header
> and event itself are already in memory, so we can parse any number
> of digests, as long as we do not try to parse whole memory when
> given count of 0xFFFFFFFF.
> 
> So this change:
> 
> * Rejects event entry with more digests than log header describes.
>   Digest types should be unique, and all should be described in
>   log header, so there cannot be more digests in the event than in
>   the header.
> 
> * Reject event entry with digest that is not described in the
>   log header.  In theory code could hardcode information about
>   digest IDs already assigned by TCG, but if firmware authors
>   cannot get event log format right, why should anyone believe
>   that they got event log content right.


I have to say that I really didn't what this change is doing after
reading these. I only undestood what this commit is doing by reading
the code change.

So we have a constant for banks and you are reading the number of banks
from the event to make it more dynamic? Why it requires such a complicated 
explanation? And what is your short summary trying to say?

/Jarkko

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

Re: [PATCH 4/4] Improve handling of TPM2 event logs

[Petr Vandrovec]

Jarkko Sakkinen wrote:
> On Wed, Mar 29, 2017 at 12:43:30AM -0700, Petr Vandrovec wrote:
>> From: Petr Vandrovec<petr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
>>
>> When TPM2 log has entries with more than 3 digests, or with digests
>> not listed in the log header, log gets misparsed, eventually
>> leading to kernel complaint that code tried to vmalloc 512MB of
>> memory (I have no idea what would happen on bigger system).
>
> It would be easier to follow this if you would give a concrete
> example of such situation. Since you are contributing this, you
> must must have bumped into this, haven't you?

Yes.  Our firmware reports TPM2 log with 4 digests (SHA1, SHA256, 
SHA384, SHA512).  Current code in kernel skips over first 3 digests 
only, trying to parse 4th digest as event length, coming to conclusion 
that entry is hundred of megabytes or few gigabytes long, and then 
failing to vmalloc() that amount of memory.

You can find parsed textual log at 
https://drive.google.com/open?id=0B7WL11GSMhrQRDBLWjc3bEZQTEk (192KB), 
and binary log at 
https://drive.google.com/open?id=0B7WL11GSMhrQQ1RCMmRmM2paR3c (48KB).

I hit situation with malformed event log on my Dell laptop - after TPM 
firmware got updated from 1.2 to 2.0, chip & everything is reported as 
TPM2, except that log stays in TPM1.2 format.


>> So code should not parse only first 3 digests: both event header
>> and event itself are already in memory, so we can parse any number
>> of digests, as long as we do not try to parse whole memory when
>> given count of 0xFFFFFFFF.
>>
>> So this change:
>>
>> * Rejects event entry with more digests than log header describes.
>>    Digest types should be unique, and all should be described in
>>    log header, so there cannot be more digests in the event than in
>>    the header.
>>
>> * Reject event entry with digest that is not described in the
>>    log header.  In theory code could hardcode information about
>>    digest IDs already assigned by TCG, but if firmware authors
>>    cannot get event log format right, why should anyone believe
>>    that they got event log content right.
>
>
> I have to say that I really didn't what this change is doing after
> reading these. I only undestood what this commit is doing by reading
> the code change.
>
> So we have a constant for banks and you are reading the number of banks
> from the event to make it more dynamic? Why it requires such a complicated
> explanation? And what is your short summary trying to say?

My short summary is trying to say that there is no reason to restrict 
parsing to only 3 digests (static number of banks), as long as we 
validate that log is logically consistent.
								Petr


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

Re: [PATCH 4/4] Improve handling of TPM2 event logs

[Jarkko Sakkinen]

On Wed, Apr 05, 2017 at 01:29:29PM -0700, Petr Vandrovec wrote:
> Jarkko Sakkinen wrote:
> > On Wed, Mar 29, 2017 at 12:43:30AM -0700, Petr Vandrovec wrote:
> > > From: Petr Vandrovec<petr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > 
> > > When TPM2 log has entries with more than 3 digests, or with digests
> > > not listed in the log header, log gets misparsed, eventually
> > > leading to kernel complaint that code tried to vmalloc 512MB of
> > > memory (I have no idea what would happen on bigger system).
> > 
> > It would be easier to follow this if you would give a concrete
> > example of such situation. Since you are contributing this, you
> > must must have bumped into this, haven't you?
> 
> Yes.  Our firmware reports TPM2 log with 4 digests (SHA1, SHA256, SHA384,
> SHA512).  Current code in kernel skips over first 3 digests only, trying to
> parse 4th digest as event length, coming to conclusion that entry is hundred
> of megabytes or few gigabytes long, and then failing to vmalloc() that
> amount of memory.
> 
> You can find parsed textual log at
> https://drive.google.com/open?id=0B7WL11GSMhrQRDBLWjc3bEZQTEk (192KB), and
> binary log at https://drive.google.com/open?id=0B7WL11GSMhrQQ1RCMmRmM2paR3c
> (48KB).
> 
> I hit situation with malformed event log on my Dell laptop - after TPM
> firmware got updated from 1.2 to 2.0, chip & everything is reported as TPM2,
> except that log stays in TPM1.2 format.

Great thank you. I think I'll merge this already to my master branch
(not yet the whole series). Thanks a lot for spotting and fixing this
regression.

/Jarkko 

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

Re: [PATCH 4/4] Improve handling of TPM2 event logs

[Jarkko Sakkinen]

On Thu, Apr 06, 2017 at 09:35:34AM +0300, Jarkko Sakkinen wrote:
> On Wed, Apr 05, 2017 at 01:29:29PM -0700, Petr Vandrovec wrote:
> > Jarkko Sakkinen wrote:
> > > On Wed, Mar 29, 2017 at 12:43:30AM -0700, Petr Vandrovec wrote:
> > > > From: Petr Vandrovec<petr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > 
> > > > When TPM2 log has entries with more than 3 digests, or with digests
> > > > not listed in the log header, log gets misparsed, eventually
> > > > leading to kernel complaint that code tried to vmalloc 512MB of
> > > > memory (I have no idea what would happen on bigger system).
> > > 
> > > It would be easier to follow this if you would give a concrete
> > > example of such situation. Since you are contributing this, you
> > > must must have bumped into this, haven't you?
> > 
> > Yes.  Our firmware reports TPM2 log with 4 digests (SHA1, SHA256, SHA384,
> > SHA512).  Current code in kernel skips over first 3 digests only, trying to
> > parse 4th digest as event length, coming to conclusion that entry is hundred
> > of megabytes or few gigabytes long, and then failing to vmalloc() that
> > amount of memory.
> > 
> > You can find parsed textual log at
> > https://drive.google.com/open?id=0B7WL11GSMhrQRDBLWjc3bEZQTEk (192KB), and
> > binary log at https://drive.google.com/open?id=0B7WL11GSMhrQQ1RCMmRmM2paR3c
> > (48KB).
> > 
> > I hit situation with malformed event log on my Dell laptop - after TPM
> > firmware got updated from 1.2 to 2.0, chip & everything is reported as TPM2,
> > except that log stays in TPM1.2 format.
> 
> Great thank you. I think I'll merge this already to my master branch
> (not yet the whole series). Thanks a lot for spotting and fixing this
> regression.
> 
> /Jarkko 

http://git.infradead.org/users/jjs/linux-tpmdd.git/commit/21229f46ce976bb8652d1da55f7823f4818b06d7

Please consider following in the future:

- Short summary was misleading. It should fix loud and clear as
  it fixes a real regression. It should not be called an improvement.
- Add the fixes line.
- I would recommend to submit fixes separately from rest of the series
  so that they get more quickly noticed.

This will make both your and my life easier...

/Jarkko

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

Re: [PATCH 4/4] Improve handling of TPM2 event logs

[Jarkko Sakkinen]

On Thu, Apr 06, 2017 at 09:44:54AM +0300, Jarkko Sakkinen wrote:
> On Thu, Apr 06, 2017 at 09:35:34AM +0300, Jarkko Sakkinen wrote:
> > On Wed, Apr 05, 2017 at 01:29:29PM -0700, Petr Vandrovec wrote:
> > > Jarkko Sakkinen wrote:
> > > > On Wed, Mar 29, 2017 at 12:43:30AM -0700, Petr Vandrovec wrote:
> > > > > From: Petr Vandrovec<petr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > > 
> > > > > When TPM2 log has entries with more than 3 digests, or with digests
> > > > > not listed in the log header, log gets misparsed, eventually
> > > > > leading to kernel complaint that code tried to vmalloc 512MB of
> > > > > memory (I have no idea what would happen on bigger system).
> > > > 
> > > > It would be easier to follow this if you would give a concrete
> > > > example of such situation. Since you are contributing this, you
> > > > must must have bumped into this, haven't you?
> > > 
> > > Yes.  Our firmware reports TPM2 log with 4 digests (SHA1, SHA256, SHA384,
> > > SHA512).  Current code in kernel skips over first 3 digests only, trying to
> > > parse 4th digest as event length, coming to conclusion that entry is hundred
> > > of megabytes or few gigabytes long, and then failing to vmalloc() that
> > > amount of memory.
> > > 
> > > You can find parsed textual log at
> > > https://drive.google.com/open?id=0B7WL11GSMhrQRDBLWjc3bEZQTEk (192KB), and
> > > binary log at https://drive.google.com/open?id=0B7WL11GSMhrQQ1RCMmRmM2paR3c
> > > (48KB).
> > > 
> > > I hit situation with malformed event log on my Dell laptop - after TPM
> > > firmware got updated from 1.2 to 2.0, chip & everything is reported as TPM2,
> > > except that log stays in TPM1.2 format.
> > 
> > Great thank you. I think I'll merge this already to my master branch
> > (not yet the whole series). Thanks a lot for spotting and fixing this
> > regression.
> > 
> > /Jarkko 
> 
> http://git.infradead.org/users/jjs/linux-tpmdd.git/commit/21229f46ce976bb8652d1da55f7823f4818b06d7
> 
> Please consider following in the future:
> 
> - Short summary was misleading. It should fix loud and clear as
>   it fixes a real regression. It should not be called an improvement.
> - Add the fixes line.
> - I would recommend to submit fixes separately from rest of the series
>   so that they get more quickly noticed.
> 
> This will make both your and my life easier...
> 
> /Jarkko

And please run checkpatch:

WARNING: line over 80 characters
#61: FILE: drivers/char/tpm/tpm2_eventlog.c:69:
+				marker +=
efispecid->digest_sizes[j].digest_size;

/Jarkko

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