Change in coreboot[master]: vboot: Add permission check for kernel space

[Daisuke Nojiri (Code Review)]

Daisuke Nojiri has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40579 )

Change subject: vboot: Add permission check for kernel space
......................................................................

vboot: Add permission check for kernel space

This patch restores the permission check for the kernel space which
was dropped when read_space_kernel was moved from Depthcharge by
CL:2155429.

Signed-off-by: dnojiri

BUG=none
BRANCH=none
TEST=none

Change-Id: If6d487940f39865cadc0ca9d5de6e055ad3e017d
---
M src/security/vboot/secdata_tpm.c
1 file changed, 17 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/40579/1

diff --git a/src/security/vboot/secdata_tpm.c b/src/security/vboot/secdata_tpm.c
index d666ae8..5d3d1f7 100644
--- a/src/security/vboot/secdata_tpm.c
+++ b/src/security/vboot/secdata_tpm.c
@@ -38,6 +38,7 @@
 #include <security/tpm/tspi.h>
 #include <vb2_api.h>
 #include <console/console.h>
+#include <tlcl.h>

#ifdef FOR_TEST
 #include <stdio.h>
@@ -68,6 +69,22 @@

uint32_t antirollback_read_space_kernel(struct vb2_context *ctx)
 {
+#if !CONFIG(TPM2_MODE)
+	/*
+	 * Before reading the kernel space, verify its permissions.  If the
+	 * kernel space has the wrong permission, we give up.  This will need
+	 * to be fixed by the recovery kernel.  We will have to worry about
+	 * this because at any time (even with PP turned off) the TPM owner can
+	 * remove and redefine a PP-protected space (but not write to it).
+	 */
+	uint32_t perms;
+
+	RETURN_ON_FAILURE(TlclGetPermissions(KERNEL_NV_INDEX, &perms));
+	if (perms != TPM_NV_PER_PPWRITE) {
+		printk(BIOS_ERR, "TPM: invalid secdata_kernel permissions\n");
+		return TPM_E_CORRUPTED_STATE;
+	}
+#endif
    uint8_t size = VB2_SECDATA_KERNEL_MIN_SIZE;

RETURN_ON_FAILURE(tlcl_read(KERNEL_NV_INDEX, ctx->secdata_kernel,

[Daisuke Nojiri (Code Review)]

Hello build bot (Jenkins), Aaron Durbin,

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

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

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

Change subject: vboot: Add permission check for kernel space
......................................................................

vboot: Add permission check for kernel space

This patch restores the permission check for the kernel space which
was dropped when read_space_kernel was moved from Depthcharge by
CL:2155429.

Signed-off-by: dnojiri

BUG=chromium:1045217, chromium:1020578
BRANCH=none
TEST=none

Change-Id: If6d487940f39865cadc0ca9d5de6e055ad3e017d
---
M src/security/vboot/secdata_tpm.c
1 file changed, 17 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/40579/2

[Daisuke Nojiri (Code Review)]

Hello build bot (Jenkins), Aaron Durbin,

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

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

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

Change subject: vboot: Add permission check for kernel space
......................................................................

vboot: Add permission check for kernel space

This patch restores the permission check for the kernel space which
was dropped when read_space_kernel was moved from Depthcharge by
CL:2155429.

Signed-off-by: dnojiri

BUG=chromium:1045217, chromium:1020578
BRANCH=none
TEST=none

Change-Id: If6d487940f39865cadc0ca9d5de6e055ad3e017d
---
M src/security/vboot/secdata_tpm.c
1 file changed, 17 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/40579/3

[Julius Werner (Code Review)]

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

Change subject: vboot: Add permission check for kernel space
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/40579/3/src/security/vboot/secdata_... 
File src/security/vboot/secdata_tpm.c:

https://review.coreboot.org/c/coreboot/+/40579/3/src/security/vboot/secdata_... 
PS3, Line 72: #if !CONFIG(TPM2)
Please write this in C instead (i.e. 'if (CONFIG(TPM2)) {...}').

https://review.coreboot.org/c/coreboot/+/40579/3/src/security/vboot/secdata_... 
PS3, Line 82: TlclGetPermissions
Hmm, crap, it looks like TlclGetPermissions() was never ported to coreboot, so you'll have to do that first. It's probably better to copy the older pre-2016 version (https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_referen...) rather than the current, more complicated one in vboot today using TlclGetSpaceInfo(). This should go in src/security/tpm/tss/tcg-1.2/tss.c (following coreboot code style there). Looks like we already have tpm_getpermissions_cmd in tss_commands.h.

[Daisuke Nojiri (Code Review)]

Hello build bot (Jenkins), Aaron Durbin,

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

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

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

Change subject: vboot: Add permission check for kernel space
......................................................................

vboot: Add permission check for kernel space

This patch restores the permission check for the kernel space which
was dropped when read_space_kernel was moved from Depthcharge by
CL:2155429.

Signed-off-by: dnojiri

BUG=chromium:1045217, chromium:1020578
BRANCH=none
TEST=none

Change-Id: If6d487940f39865cadc0ca9d5de6e055ad3e017d
---
M src/security/tpm/tss.h
M src/security/tpm/tss/tcg-1.2/tss.c
M src/security/vboot/secdata_tpm.c
3 files changed, 45 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/40579/4

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40579 )

Change subject: vboot: Add permission check for kernel space
......................................................................

Patch Set 4:

(3 comments)

https://review.coreboot.org/c/coreboot/+/40579/4/src/security/tpm/tss.h 
File src/security/tpm/tss.h:

https://review.coreboot.org/c/coreboot/+/40579/4/src/security/tpm/tss.h@203 
PS4, Line 203: uint32_t tlcl_get_permissions(uint32_t index, uint32_t* permissions);
"foo* bar" should be "foo *bar"

https://review.coreboot.org/c/coreboot/+/40579/4/src/security/tpm/tss/tcg-1.... 
File src/security/tpm/tss/tcg-1.2/tss.c:

https://review.coreboot.org/c/coreboot/+/40579/4/src/security/tpm/tss/tcg-1.... 
PS4, Line 363: uint32_t tlcl_get_permissions(uint32_t index, uint32_t* permissions)
"foo* bar" should be "foo *bar"

https://review.coreboot.org/c/coreboot/+/40579/4/src/security/tpm/tss/tcg-1.... 
PS4, Line 367: 	uint8_t* nvdata;
"foo* bar" should be "foo *bar"

[Daisuke Nojiri (Code Review)]

Hello build bot (Jenkins), Aaron Durbin,

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

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

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

Change subject: vboot: Add permission check for kernel space
......................................................................

vboot: Add permission check for kernel space

This patch restores the permission check for the kernel space which
was dropped when read_space_kernel was moved from Depthcharge by
CL:2155429.

Signed-off-by: dnojiri

BUG=chromium:1045217, chromium:1020578
BRANCH=none
TEST=none

Change-Id: If6d487940f39865cadc0ca9d5de6e055ad3e017d
---
M src/security/tpm/tss.h
M src/security/tpm/tss/tcg-1.2/tss.c
M src/security/vboot/secdata_tpm.c
3 files changed, 44 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/40579/5

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40579 )

Change subject: vboot: Add permission check for kernel space
......................................................................

Patch Set 5:

(3 comments)

https://review.coreboot.org/c/coreboot/+/40579/5/src/security/tpm/tss.h 
File src/security/tpm/tss.h:

https://review.coreboot.org/c/coreboot/+/40579/5/src/security/tpm/tss.h@203 
PS5, Line 203: uint32_t tlcl_get_permissions(uint32_t index, uint32_t* permissions);
"foo* bar" should be "foo *bar"

https://review.coreboot.org/c/coreboot/+/40579/5/src/security/tpm/tss/tcg-1.... 
File src/security/tpm/tss/tcg-1.2/tss.c:

https://review.coreboot.org/c/coreboot/+/40579/5/src/security/tpm/tss/tcg-1.... 
PS5, Line 363: uint32_t tlcl_get_permissions(uint32_t index, uint32_t* permissions)
"foo* bar" should be "foo *bar"

https://review.coreboot.org/c/coreboot/+/40579/5/src/security/tpm/tss/tcg-1.... 
PS5, Line 367: 	uint8_t* nvdata;
"foo* bar" should be "foo *bar"

[Daisuke Nojiri (Code Review)]

Hello build bot (Jenkins), Aaron Durbin,

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

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

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

Change subject: vboot: Add permission check for kernel space
......................................................................

vboot: Add permission check for kernel space

This patch restores the permission check for the kernel space which
was dropped when read_space_kernel was moved from Depthcharge by
CL:2155429.

Signed-off-by: dnojiri

BUG=chromium:1045217, chromium:1020578
BRANCH=none
TEST=none

Change-Id: If6d487940f39865cadc0ca9d5de6e055ad3e017d
---
M src/security/tpm/tss.h
M src/security/tpm/tss/tcg-1.2/tss.c
M src/security/vboot/secdata_tpm.c
3 files changed, 44 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/40579/6

[Daisuke Nojiri (Code Review)]

Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40579 )

Change subject: vboot: Add permission check for kernel space
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/c/coreboot/+/40579/3/src/security/vboot/secdata_... 
File src/security/vboot/secdata_tpm.c:

https://review.coreboot.org/c/coreboot/+/40579/3/src/security/vboot/secdata_... 
PS3, Line 72: #if !CONFIG(TPM2)
> Please write this in C instead (i.e. 'if (CONFIG(TPM2)) {...}').

That requires TPM_NV_PER_PPWRITE to be defined for TPM2 as well. Do you still want it?

https://review.coreboot.org/c/coreboot/+/40579/3/src/security/vboot/secdata_... 
PS3, Line 82: TlclGetPermissions
> Hmm, crap, it looks like TlclGetPermissions() was never ported to coreboot, so you'll have to do tha […]

Done

[Paul Menzel (Code Review)]

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

Change subject: vboot: Add permission check for kernel space
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/c/coreboot/+/40579/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40579/6//COMMIT_MSG@13 
PS6, Line 13: Signed-off-by: dnojiri 
Please use you full name and move the SoB line right below the Change-Id line.

https://review.coreboot.org/c/coreboot/+/40579/6/src/security/vboot/secdata_... 
File src/security/vboot/secdata_tpm.c:

https://review.coreboot.org/c/coreboot/+/40579/6/src/security/vboot/secdata_... 
PS6, Line 86: 			       "TPM: invalid secdata_kernel permissions\n");
This should fit in one line.

[Daisuke Nojiri (Code Review)]

Hello build bot (Jenkins), Aaron Durbin,

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

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

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

Change subject: vboot: Add permission check for kernel space
......................................................................

vboot: Add permission check for kernel space

This patch restores the permission check for the kernel space which
was dropped when read_space_kernel was moved from Depthcharge by
CL:2155429.

BUG=chromium:1045217, chromium:1020578
BRANCH=none
TEST=none

Signed-off-by: dnojiri 
Change-Id: If6d487940f39865cadc0ca9d5de6e055ad3e017d
---
M src/security/tpm/tss.h
M src/security/tpm/tss/tcg-1.2/tss.c
M src/security/vboot/secdata_tpm.c
3 files changed, 45 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/40579/7

[Julius Werner (Code Review)]

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

Change subject: vboot: Add permission check for kernel space
......................................................................

Patch Set 7: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/40579/6/src/security/vboot/secdata_... 
File src/security/vboot/secdata_tpm.c:

https://review.coreboot.org/c/coreboot/+/40579/6/src/security/vboot/secdata_... 
PS6, Line 86: 			       "TPM: invalid secdata_kernel permissions\n");
> This should fit in one line.

I don't think it does? As long as files aren't globally adjusted to 96 characters, we should stick to 80 when the rest of the file does too.

[Daisuke Nojiri (Code Review)]

Hello build bot (Jenkins), Julius Werner, Aaron Durbin,

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

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

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

Change subject: vboot: Add permission check for kernel space
......................................................................

vboot: Add permission check for kernel space

This patch restores the permission check for the kernel space which
was dropped when read_space_kernel was moved from Depthcharge by
CL:2155429.

BUG=chromium:1045217, chromium:1020578
BRANCH=none
TEST=none

Signed-off-by: dnojiri 
Change-Id: If6d487940f39865cadc0ca9d5de6e055ad3e017d
---
M src/security/tpm/tss.h
M src/security/tpm/tss/tcg-1.2/tss.c
M src/security/vboot/secdata_tpm.c
3 files changed, 46 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/40579/8

[Julius Werner (Code Review)]

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

Change subject: vboot: Add permission check for kernel space
......................................................................

Patch Set 8: Code-Review+2

[Daisuke Nojiri (Code Review)]

Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40579 )

Change subject: vboot: Add permission check for kernel space
......................................................................

Patch Set 8:

(6 comments)

https://review.coreboot.org/c/coreboot/+/40579/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40579/6//COMMIT_MSG@13 
PS6, Line 13: Signed-off-by: dnojiri
> Please use you full name and move the SoB line right below the Change-Id line.

Done

https://review.coreboot.org/c/coreboot/+/40579/5/src/security/tpm/tss.h 
File src/security/tpm/tss.h:

https://review.coreboot.org/c/coreboot/+/40579/5/src/security/tpm/tss.h@203 
PS5, Line 203: uint32_t tlcl_get_permissions(uint32_t index, uint32_t* permissions);
> "foo* bar" should be "foo *bar"

Done

https://review.coreboot.org/c/coreboot/+/40579/5/src/security/tpm/tss/tcg-1.... 
File src/security/tpm/tss/tcg-1.2/tss.c:

https://review.coreboot.org/c/coreboot/+/40579/5/src/security/tpm/tss/tcg-1.... 
PS5, Line 363: uint32_t tlcl_get_permissions(uint32_t index, uint32_t* permissions)
> "foo* bar" should be "foo *bar"

Done

https://review.coreboot.org/c/coreboot/+/40579/5/src/security/tpm/tss/tcg-1.... 
PS5, Line 367: 	uint8_t* nvdata;
> "foo* bar" should be "foo *bar"

Done

https://review.coreboot.org/c/coreboot/+/40579/3/src/security/vboot/secdata_... 
File src/security/vboot/secdata_tpm.c:

https://review.coreboot.org/c/coreboot/+/40579/3/src/security/vboot/secdata_... 
PS3, Line 72: #if !CONFIG(TPM2)
> That requires TPM_NV_PER_PPWRITE to be defined for TPM2 as well. […]

Done

https://review.coreboot.org/c/coreboot/+/40579/6/src/security/vboot/secdata_... 
File src/security/vboot/secdata_tpm.c:

https://review.coreboot.org/c/coreboot/+/40579/6/src/security/vboot/secdata_... 
PS6, Line 86: 			       "TPM: invalid secdata_kernel permissions\n");
> I don't think it does? As long as files aren't globally adjusted to 96 characters, we should stick t […]

Ack

[Julius Werner (Code Review)]

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40579 )

Change subject: vboot: Add permission check for kernel space
......................................................................

vboot: Add permission check for kernel space

This patch restores the permission check for the kernel space which
was dropped when read_space_kernel was moved from Depthcharge by
CL:2155429.

BUG=chromium:1045217, chromium:1020578
BRANCH=none
TEST=none

Signed-off-by: dnojiri 
Change-Id: If6d487940f39865cadc0ca9d5de6e055ad3e017d
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40579
Reviewed-by: Julius Werner 
Tested-by: build bot (Jenkins) 
---
M src/security/tpm/tss.h
M src/security/tpm/tss/tcg-1.2/tss.c
M src/security/vboot/secdata_tpm.c
3 files changed, 46 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Julius Werner: Looks good to me, approved

diff --git a/src/security/tpm/tss.h b/src/security/tpm/tss.h
index 5237387..57f3b24 100644
--- a/src/security/tpm/tss.h
+++ b/src/security/tpm/tss.h
@@ -197,4 +197,9 @@
  */
 uint32_t tlcl_disable_platform_hierarchy(void);

+/**
+ * Get the permission bits for the NVRAM space with |index|.
+ */
+uint32_t tlcl_get_permissions(uint32_t index, uint32_t *permissions);
+
 #endif /* TSS_H_ */
diff --git a/src/security/tpm/tss/tcg-1.2/tss.c b/src/security/tpm/tss/tcg-1.2/tss.c
index 9bc72d2..ea3f94d 100644
--- a/src/security/tpm/tss/tcg-1.2/tss.c
+++ b/src/security/tpm/tss/tcg-1.2/tss.c
@@ -359,3 +359,22 @@
    	       kPcrDigestLength);
    return result;
 }
+
+uint32_t tlcl_get_permissions(uint32_t index, uint32_t *permissions)
+{
+	struct s_tpm_getpermissions_cmd cmd;
+	uint8_t response[TPM_LARGE_ENOUGH_COMMAND_SIZE];
+	uint8_t *nvdata;
+	uint32_t result;
+	uint32_t size;
+
+	memcpy(&cmd, &tpm_getpermissions_cmd, sizeof(cmd));
+	to_tpm_uint32(cmd.buffer + tpm_getpermissions_cmd.index, index);
+	result = tlcl_send_receive(cmd.buffer, response, sizeof(response));
+	if (result != TPM_SUCCESS)
+		return result;
+
+	nvdata = response + kTpmResponseHeaderLength + sizeof(size);
+	from_tpm_uint32(nvdata + kNvDataPublicPermissionsOffset, permissions);
+	return result;
+}
diff --git a/src/security/vboot/secdata_tpm.c b/src/security/vboot/secdata_tpm.c
index d666ae8..37665bc 100644
--- a/src/security/vboot/secdata_tpm.c
+++ b/src/security/vboot/secdata_tpm.c
@@ -36,6 +36,8 @@
 #include <security/vboot/tpm_common.h>
 #include <string.h>
 #include <security/tpm/tspi.h>
+#include <security/tpm/tss.h>
+#include <security/tpm/tss/tcg-1.2/tss_structures.h>
 #include <vb2_api.h>
 #include <console/console.h>

@@ -68,6 +70,26 @@

uint32_t antirollback_read_space_kernel(struct vb2_context *ctx)
 {
+	if (!CONFIG(TPM2)) {
+		/*
+		 * Before reading the kernel space, verify its permissions. If
+		 * the kernel space has the wrong permission, we give up. This
+		 * will need to be fixed by the recovery kernel. We will have
+		 * to worry about this because at any time (even with PP turned
+		 * off) the TPM owner can remove and redefine a PP-protected
+		 * space (but not write to it).
+		 */
+		uint32_t perms;
+
+		RETURN_ON_FAILURE(tlcl_get_permissions(KERNEL_NV_INDEX,
+						       &perms));
+		if (perms != TPM_NV_PER_PPWRITE) {
+			printk(BIOS_ERR,
+			       "TPM: invalid secdata_kernel permissions\n");
+			return TPM_E_CORRUPTED_STATE;
+		}
+	}
+
    uint8_t size = VB2_SECDATA_KERNEL_MIN_SIZE;

RETURN_ON_FAILURE(tlcl_read(KERNEL_NV_INDEX, ctx->secdata_kernel,