Change in coreboot[master]: security/tpm/tspi/crtm.c: Fix handling of white space delimited list

[Harshit Sharma (Code Review)]

Harshit Sharma has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

security/tpm/tspi/crtm.c: Fix handling of white space delimited list

The current implementation uses strcmp() without splitting the list
and therefore returns false even when the string pointed to by
'name' is a part of 'whitelist'. The patch fixes this problem.
Also, update help text of Kconfig to a list delimited by white
space to align with other lists we use.

Change-Id: Ifd285162ea6e562a5bb18325a1b767ac2e4276f3
Signed-off-by: Harshit Sharma 
---
M src/security/tpm/Kconfig
M src/security/tpm/tspi/crtm.c
2 files changed, 11 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/41280/1

diff --git a/src/security/tpm/Kconfig b/src/security/tpm/Kconfig
index 6741614..187685c 100644
--- a/src/security/tpm/Kconfig
+++ b/src/security/tpm/Kconfig
@@ -112,6 +112,6 @@
    depends on TPM_MEASURED_BOOT
    help
      Runtime data whitelist of cbfs filenames. Needs to be a
-	  comma separated list
+	  space delimited list.

endmenu # Trusted Platform Module (tpm)
diff --git a/src/security/tpm/tspi/crtm.c b/src/security/tpm/tspi/crtm.c
index 8bcc01b..d096828 100644
--- a/src/security/tpm/tspi/crtm.c
+++ b/src/security/tpm/tspi/crtm.c
@@ -1,4 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
+/* This file is part of the coreboot project. */

#include <console/console.h>
 #include <fmap.h>
@@ -88,14 +89,20 @@
    const char *whitelist = CONFIG_TPM_MEASURED_BOOT_RUNTIME_DATA;
    size_t whitelist_len = sizeof(CONFIG_TPM_MEASURED_BOOT_RUNTIME_DATA) - 1;
    size_t name_len = strlen(name);
-	int i;
+	char delim[] = " ";
+	char tmp_whitelist[whitelist_len + 1];
+	char *token;

if (!whitelist_len || !name_len)
    	return false;

-	for (i = 0; (i + name_len) <= whitelist_len; i++) {
-		if (!strcmp(whitelist + i, name))
+	strcpy(tmp_whitelist, whitelist);
+	token = strtok(tmp_whitelist, delim);
+
+	while (token != NULL) {
+		if (!strcmp(token, name))
    		return true;
+		token = strtok(NULL, delim);
    }

return false;

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/41280/1/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/41280/1/src/security/tpm/tspi/crtm.... 
PS1, Line 2: /* This file is part of the coreboot project. */
Please split this into a separate commit.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 1:

```
    CC         bootblock/security/tpm/tspi/crtm.o
src/security/tpm/tspi/crtm.c: In function 'is_runtime_data':
src/security/tpm/tspi/crtm.c:93:2: error: ISO C90 forbids variable length array 'tmp_whitelist' [-Werror=vla]
  char tmp_whitelist[whitelist_len + 1];
  ^~~~
src/security/tpm/tspi/crtm.c:100:10: error: implicit declaration of function 'strtok'; did you mean 'strspn'? [-Werror=implicit-function-declaration]
  token = strtok(tmp_whitelist, delim);
          ^~~~~~
          strspn
src/security/tpm/tspi/crtm.c:100:8: error: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
  token = strtok(tmp_whitelist, delim);
        ^
src/security/tpm/tspi/crtm.c:105:9: error: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
   token = strtok(NULL, delim);
         ^
```

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41280/1/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/41280/1/src/security/tpm/tspi/crtm.... 
PS1, Line 2: /* This file is part of the coreboot project. */
> Please split this into a separate commit.

or rather: leave out the line..

[Harshit Sharma (Code Review)]

Hello build bot (Jenkins), Paul Menzel, Werner Zeh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41280

to look at the new patch set (#2).

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

security/tpm/tspi/crtm.c: Fix handling of white space delimited list

The current implementation uses strcmp() without splitting the list
and therefore returns false even when the string pointed to by
'name' is a part of 'whitelist'. The patch fixes this problem.
Also, update help text of Kconfig to a list delimited by white
space to align with other lists we use.

Change-Id: Ifd285162ea6e562a5bb18325a1b767ac2e4276f3
Signed-off-by: Harshit Sharma 
---
M src/security/tpm/Kconfig
M src/security/tpm/tspi/crtm.c
2 files changed, 10 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/41280/2

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41280/1/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/41280/1/src/security/tpm/tspi/crtm.... 
PS1, Line 100:  strtok
It looks like our string.h does not provide strtok() function (see src/include/string.h)
Do you have a different way of dealing with this situation?

[Harshit Sharma (Code Review)]

Hello build bot (Jenkins), Paul Menzel, Werner Zeh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41280

to look at the new patch set (#3).

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

security/tpm/tspi/crtm.c: Fix handling of white space delimited list

The current implementation uses strcmp() without splitting the list
and therefore returns false even when the string pointed to by
'name' is a part of 'whitelist'. The patch fixes this problem.
Also, update help text of Kconfig to a list delimited by white
space to align with other lists we use.

Change-Id: Ifd285162ea6e562a5bb18325a1b767ac2e4276f3
Signed-off-by: Harshit Sharma 
---
M src/security/tpm/Kconfig
M src/security/tpm/tspi/crtm.c
2 files changed, 14 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/41280/3

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 3:

(1 comment)
> Patch Set 1:
>
> (1 comment)

https://review.coreboot.org/c/coreboot/+/41280/1/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/41280/1/src/security/tpm/tspi/crtm.... 
PS1, Line 2: /* This file is part of the coreboot project. */
> or rather: leave out the line..

Done

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41280/1/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/41280/1/src/security/tpm/tspi/crtm.... 
PS1, Line 100:  strtok
> It looks like our string.h does not provide strtok() function (see src/include/string.h) […]

I just uploaded a patch handling it without strtok().

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41280/3/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/41280/3/src/security/tpm/tspi/crtm.... 
PS3, Line 100: == whitelist_len
looks like it's time to introduce strtok()

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41280/3/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/41280/3/src/security/tpm/tspi/crtm.... 
PS3, Line 100: == whitelist_len
> looks like it's time to introduce strtok()

We can do in a special commit but let's take this one here as it is since it is the first patch for our GSoC student.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 3: Code-Review+1

(1 comment)

Nice. Glad to have you in coreboot. If you could send a follow-up adding strtok() to our library, that’d be awesome.

https://review.coreboot.org/c/coreboot/+/41280/3/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/41280/3/src/security/tpm/tspi/crtm.... 
PS3, Line 92: int
unsigned

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 3:

Thanks for fixing my implementation Harshit!

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 3:
> Patch Set 3: Code-Review+1
>
> (1 comment)
>
> Nice. Glad to have you in coreboot. If you could send a follow-up adding strtok() to our library, that’d be awesome.

Thanks Paul. Sure, I'd love to work on adding strtok().

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 3:
> Patch Set 3:
>
> Thanks for fixing my implementation Harshit!

No Problem. Btw Werner is the one who discovered this issue.

[Harshit Sharma (Code Review)]

Hello Philipp Deppenwiese, build bot (Jenkins), Paul Menzel, Julius Werner, Werner Zeh, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41280

to look at the new patch set (#4).

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

security/tpm/tspi/crtm.c: Fix handling of white space delimited list

The current implementation uses strcmp() without splitting the list
and therefore returns false even when the string pointed to by
'name' is a part of 'whitelist'. The patch fixes this problem.
Also, update help text of Kconfig to a list delimited by white
space to align with other lists we use.

Change-Id: Ifd285162ea6e562a5bb18325a1b767ac2e4276f3
Signed-off-by: Harshit Sharma 
---
M src/security/tpm/Kconfig
M src/security/tpm/tspi/crtm.c
2 files changed, 14 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/41280/4

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41280/3/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/41280/3/src/security/tpm/tspi/crtm.... 
PS3, Line 92: int
> unsigned

Done

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/41280/3/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/41280/3/src/security/tpm/tspi/crtm.... 
PS3, Line 100: == whitelist_len
> We can do in a special commit but let's take this one here as it is since it is the first patch for  […]

If you want to add strtok there's a decent version in libpayload (based on strspn that we already have) you could just copy. But I'm not sure it's the best solution for this.

https://review.coreboot.org/c/coreboot/+/41280/4/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/41280/4/src/security/tpm/tspi/crtm.... 
PS4, Line 97: 	strcpy(tmp, whitelist);
I don't think you really need to copy the string every time this is called... if you already know the end to which you want to compare, you can just use strncmp() instead of setting it to '\0'. And you could use strchr() to find the token end which might make it a bit easier. How about this:

const char *end;
 const char *token = whitelist;
 while ((end = strchr(token, ' '))) {
   if (!strncmp(token, name, end - token)
     return true;
   entry = end + 1;
 }
 if (!strcmp(token, name))
   return true;
 return false;

[Harshit Sharma (Code Review)]

Hello Philipp Deppenwiese, build bot (Jenkins), Paul Menzel, Julius Werner, Werner Zeh, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41280

to look at the new patch set (#5).

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

security/tpm/tspi/crtm.c: Fix handling of white space delimited list

The current implementation uses strcmp() without splitting the list
and therefore returns false even when the string pointed to by
'name' is a part of 'whitelist'. The patch fixes this problem.
Also, update help text of Kconfig to a list delimited by white
space to align with other lists we use.

Change-Id: Ifd285162ea6e562a5bb18325a1b767ac2e4276f3
Signed-off-by: Harshit Sharma 
---
M src/security/tpm/Kconfig
M src/security/tpm/tspi/crtm.c
2 files changed, 8 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/41280/5

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/c/coreboot/+/41280/3/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/41280/3/src/security/tpm/tspi/crtm.... 
PS3, Line 100: == whitelist_len
> If you want to add strtok there's a decent version in libpayload (based on strspn that we already ha […]

Thanks. I'll have a look at it.

https://review.coreboot.org/c/coreboot/+/41280/4/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/41280/4/src/security/tpm/tspi/crtm.... 
PS4, Line 97: 	strcpy(tmp, whitelist);
> I don't think you really need to copy the string every time this is called... […]

Thanks Julius. I wanted to get rid of temp too. I didn't notice strncmp() is already there in our library. And your use of strchr() does make the code cleaner.

It may not be required in case of cbfs filenames but i've added one more condition to check if the length of substring equals name_len so that it doesn't falsely return true for corner cases where token is "abc" and name is "abcd", and vice-versa.

[Harshit Sharma (Code Review)]

Hello Philipp Deppenwiese, build bot (Jenkins), Paul Menzel, Julius Werner, Werner Zeh, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41280

to look at the new patch set (#6).

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

security/tpm/tspi/crtm.c: Fix handling of white space delimited list

The current implementation uses strcmp() without splitting the list
and therefore returns false even when the string pointed to by
'name' is a part of 'whitelist'. The patch fixes this problem.
Also, update help text of Kconfig to a list delimited by white
space to align with other lists we use.

Change-Id: Ifd285162ea6e562a5bb18325a1b767ac2e4276f3
Signed-off-by: Harshit Sharma 
---
M src/security/tpm/Kconfig
M src/security/tpm/tspi/crtm.c
2 files changed, 8 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/41280/6

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 6: Code-Review+1

(1 comment)

The implementation looks good. But it's hard to assess the impact of
changing the string format (from comma to whitespace). IMHO, this
should be a separate patch, people should be notified and maybe
the Kconfig name should also change so it becomes visible in `make
oldconfig`.

https://review.coreboot.org/c/coreboot/+/41280/6/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/41280/6/src/security/tpm/tspi/crtm.... 
PS6, Line 102: 	if (!strcmp(whitelist, name))
             : 		return true;
             : 
             : 	return false;
Same as `return !strcmp(whitelist, name);`.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 6:
> Patch Set 6: Code-Review+1
>
> (1 comment)
>
> The implementation looks good. But it's hard to assess the impact of
> changing the string format (from comma to whitespace). IMHO, this
> should be a separate patch, people should be notified and maybe
> the Kconfig name should also change so it becomes visible in `make
> oldconfig`.

I had the same thought, but then settled on the assumption, that the current behavior is broken and never worked despite for having one word. Or am I missing something?

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 6:
> > The implementation looks good. But it's hard to assess the impact of
> > changing the string format (from comma to whitespace). IMHO, this
> > should be a separate patch, people should be notified and maybe
> > the Kconfig name should also change so it becomes visible in `make
> > oldconfig`.
>
> I had the same thought, but then settled on the assumption, that the current behavior is broken and never worked despite for having one word. Or am I missing something?

That's correct. But without the comma/space change, it would actually
fix existing configs ;)

[Harshit Sharma (Code Review)]

Hello Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Paul Menzel, Julius Werner, Werner Zeh, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41280

to look at the new patch set (#7).

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

security/tpm/tspi/crtm.c: Fix handling of white space delimited list

The current implementation uses strcmp() without splitting the list
and therefore returns false even when the string pointed to by
'name' is a part of 'whitelist'. The patch fixes this problem.
Also, update help text of Kconfig to a list delimited by white
space to align with other lists we use.

Change-Id: Ifd285162ea6e562a5bb18325a1b767ac2e4276f3
Signed-off-by: Harshit Sharma 
---
M src/security/tpm/tspi/crtm.c
1 file changed, 7 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/41280/7

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 8:

This change is ready for review.

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41280/6/src/security/tpm/tspi/crtm.... 
File src/security/tpm/tspi/crtm.c:

https://review.coreboot.org/c/coreboot/+/41280/6/src/security/tpm/tspi/crtm.... 
PS6, Line 102: 	if (!strcmp(whitelist, name))
             : 		return true;
             : 
             : 	return false;
> Same as `return !strcmp(whitelist, name);`.

Done

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 8:
> Patch Set 6: Code-Review+1
>
> (1 comment)
>
> The implementation looks good. But it's hard to assess the impact of
> changing the string format (from comma to whitespace). IMHO, this
> should be a separate patch, people should be notified and maybe
> the Kconfig name should also change so it becomes visible in `make
> oldconfig`.

I just moved Kconfig to a separate patch. https://review.coreboot.org/c/coreboot/+/41463

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 8: Code-Review+2

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 8: Code-Review-1

Sorry, splitting the update of the help text from the actual change
is not what I asked for and makes things worse. What I meant is the
two individual changes, 1. fixing the parsing, 2. changing comma-
separated to space-separated. And it was just an idea, if nobody
cares, just keep it together.

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi/crtm.c: Fix handling of white space delimited list
......................................................................

Patch Set 8:
> Patch Set 8: Code-Review-1
>
> Sorry, splitting the update of the help text from the actual change
> is not what I asked for and makes things worse. What I meant is the
> two individual changes, 1. fixing the parsing, 2. changing comma-
> separated to space-separated. And it was just an idea, if nobody
> cares, just keep it together.

I think I got you now. What you meant is a patch to fix the parsing with a comma-separated list first and then another patch on top of it to change it to space-separated and to update the whitelist implementation accordingly.

Since the current implementation never worked even for comma-separated, for now, I am just merging Kconfig back to this patch to see if everyone is on board.

Change in coreboot[master]: security/tpm/tspi: Fix handling of white space delimited list

[Harshit Sharma (Code Review)]

Hello Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Paul Menzel, Julius Werner, Werner Zeh, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41280

to look at the new patch set (#9).

Change subject: security/tpm/tspi: Fix handling of white space delimited list
......................................................................

security/tpm/tspi: Fix handling of white space delimited list

The current implementation uses strcmp() without splitting the list
and therefore returns false even when the string pointed to by
'name' is a part of 'whitelist'. The patch fixes this problem.
Also, update help text of CONFIG_TPM_MEASURED_BOOT_RUNTIME_DATA to
space delimited list to align it with the other lists we use.

Change-Id: Ifd285162ea6e562a5bb18325a1b767ac2e4276f3
Signed-off-by: Harshit Sharma 
---
M src/security/tpm/Kconfig
M src/security/tpm/tspi/crtm.c
2 files changed, 6 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/41280/9

Change in coreboot[master]: security/tpm/tspi: Fix handling of white space delimited list

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi: Fix handling of white space delimited list
......................................................................

Patch Set 9: Code-Review+1

Change in coreboot[master]: security/tpm/tspi: Fix handling of white space delimited list

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi: Fix handling of white space delimited list
......................................................................

Patch Set 9: Code-Review+2

So now that there is a patch for strtok in the tree should we move forward to strtok()?
Or would you lik eto keep this for now and provide a follow-up patch once strtok() is merged?

Change in coreboot[master]: security/tpm/tspi: Fix handling of white space delimited list

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi: Fix handling of white space delimited list
......................................................................

Patch Set 9: Code-Review+1
> Patch Set 9: Code-Review+2
>
> So now that there is a patch for strtok in the tree should we move forward to strtok()?
> Or would you lik eto keep this for now and provide a follow-up patch once strtok() is merged?

Oh, BTW: now that you have removed the Kconfig change form this patch you should stick with the comma as separator in order to stay in line with the help text.
You can than switch back to a whitespace in the Kconfig patch.

Change in coreboot[master]: security/tpm/tspi: Fix handling of white space delimited list

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi: Fix handling of white space delimited list
......................................................................

Patch Set 9:
> Patch Set 9: Code-Review+2
>
> So now that there is a patch for strtok in the tree should we move forward to strtok()?
> Or would you lik eto keep this for now and provide a follow-up patch once strtok() is merged?

I don't think a follow-up patch with strtok() is required because if we go with strtok(), we'd have to create a local copy of whitelist before tokenizing.

Change in coreboot[master]: security/tpm/tspi: Fix handling of white space delimited list

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi: Fix handling of white space delimited list
......................................................................

Patch Set 9:
> Patch Set 9: Code-Review+1
> > Patch Set 9: Code-Review+2
> >
> > So now that there is a patch for strtok in the tree should we move forward to strtok()?
> > Or would you lik eto keep this for now and provide a follow-up patch once strtok() is merged?
>
> Oh, BTW: now that you have removed the Kconfig change form this patch you should stick with the comma as separator in order to stay in line with the help text.
> You can than switch back to a whitespace in the Kconfig patch.

I added Kconfig back in patch set 9. Since the previous implementation never worked even for comma-separated, do you think it is required to make changes in two separate patches?

Change in coreboot[master]: security/tpm/tspi: Fix handling of white space delimited list

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi: Fix handling of white space delimited list
......................................................................

Patch Set 9: Code-Review+2
> Patch Set 9:
> > Patch Set 9: Code-Review+1
> > > Patch Set 9: Code-Review+2
> > >
> > > So now that there is a patch for strtok in the tree should we move forward to strtok()?
> > > Or would you lik eto keep this for now and provide a follow-up patch once strtok() is merged?
> >
> > Oh, BTW: now that you have removed the Kconfig change form this patch you should stick with the comma as separator in order to stay in line with the help text.
> > You can than switch back to a whitespace in the Kconfig patch.
>
> I added Kconfig back in patch set 9. Since the previous implementation never worked even for comma-separated, do you think it is required to make changes in two separate patches?

Ups, I had my view on Patchset 8, sorry.
All clear than, let's get this patch in.

Change in coreboot[master]: security/tpm/tspi: Fix handling of white space delimited list

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi: Fix handling of white space delimited list
......................................................................

Patch Set 9:
> Patch Set 9: Code-Review+2
> > Patch Set 9:
> > > Patch Set 9: Code-Review+1
> > > > Patch Set 9: Code-Review+2
> > > >
> > > > So now that there is a patch for strtok in the tree should we move forward to strtok()?
> > > > Or would you lik eto keep this for now and provide a follow-up patch once strtok() is merged?
> > >
> > > Oh, BTW: now that you have removed the Kconfig change form this patch you should stick with the comma as separator in order to stay in line with the help text.
> > > You can than switch back to a whitespace in the Kconfig patch.
> >
> > I added Kconfig back in patch set 9. Since the previous implementation never worked even for comma-separated, do you think it is required to make changes in two separate patches?
>
> Ups, I had my view on Patchset 8, sorry.
> All clear than, let's get this patch in.

Thanks!

Change in coreboot[master]: security/tpm/tspi: Fix handling of white space delimited list

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi: Fix handling of white space delimited list
......................................................................

security/tpm/tspi: Fix handling of white space delimited list

The current implementation uses strcmp() without splitting the list
and therefore returns false even when the string pointed to by
'name' is a part of 'whitelist'. The patch fixes this problem.
Also, update help text of CONFIG_TPM_MEASURED_BOOT_RUNTIME_DATA to
space delimited list to align it with the other lists we use.

Change-Id: Ifd285162ea6e562a5bb18325a1b767ac2e4276f3
Signed-off-by: Harshit Sharma 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/41280
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
Reviewed-by: Werner Zeh 
---
M src/security/tpm/Kconfig
M src/security/tpm/tspi/crtm.c
2 files changed, 6 insertions(+), 5 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, but someone else must approve
  Werner Zeh: Looks good to me, approved

diff --git a/src/security/tpm/Kconfig b/src/security/tpm/Kconfig
index 6741614..b6a7781 100644
--- a/src/security/tpm/Kconfig
+++ b/src/security/tpm/Kconfig
@@ -112,6 +112,6 @@
    depends on TPM_MEASURED_BOOT
    help
      Runtime data whitelist of cbfs filenames. Needs to be a
-	  comma separated list
+	  space delimited list

endmenu # Trusted Platform Module (tpm)
diff --git a/src/security/tpm/tspi/crtm.c b/src/security/tpm/tspi/crtm.c
index 8bcc01b..49daeb0 100644
--- a/src/security/tpm/tspi/crtm.c
+++ b/src/security/tpm/tspi/crtm.c
@@ -88,17 +88,18 @@
    const char *whitelist = CONFIG_TPM_MEASURED_BOOT_RUNTIME_DATA;
    size_t whitelist_len = sizeof(CONFIG_TPM_MEASURED_BOOT_RUNTIME_DATA) - 1;
    size_t name_len = strlen(name);
-	int i;
+	const char *end;

if (!whitelist_len || !name_len)
    	return false;

-	for (i = 0; (i + name_len) <= whitelist_len; i++) {
-		if (!strcmp(whitelist + i, name))
+	while ((end = strchr(whitelist, ' '))) {
+		if (end - whitelist == name_len && !strncmp(whitelist, name, name_len))
    		return true;
+		whitelist = end + 1;
    }

-	return false;
+	return !strcmp(whitelist, name);
 }

uint32_t tspi_measure_cbfs_hook(struct cbfsf *fh, const char *name)

Change in coreboot[master]: security/tpm/tspi: Fix handling of white space delimited list

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41280 )

Change subject: security/tpm/tspi: Fix handling of white space delimited list
......................................................................

Patch Set 10:

Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4
Emulation targets:
"QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/3560
"QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3559
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3558
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3557

Please note: This test is under development and might not be accurate at all!