Change in coreboot[master]: security/vboot: Remove buffer_size from struct vboot_working_data

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36844 )

Change subject: security/vboot: Remove buffer_size from struct vboot_working_data
......................................................................

security/vboot: Remove buffer_size from struct vboot_working_data

Since buffer_size is no longer used, remove it from struct
vboot_working_data.

BRANCH=none
BUG=chromium:1021452
TEST=emerge-kukui coreboot

Change-Id: Ie770e89b4a45e0ec703d5bbb8fb6a298ce915056
Signed-off-by: Yu-Ping Wu 
---
M src/lib/coreboot_table.c
M src/security/vboot/common.c
M src/security/vboot/misc.h
3 files changed, 8 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/36844/1

diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c
index d3576e6..709bb0d 100644
--- a/src/lib/coreboot_table.c
+++ b/src/lib/coreboot_table.c
@@ -227,7 +227,12 @@
    vbwb->tag = LB_TAG_VBOOT_WORKBUF;
    vbwb->size = sizeof(*vbwb);
    vbwb->range_start = (uintptr_t)wd + wd->buffer_offset;
-	vbwb->range_size = wd->buffer_size;
+	/*
+	 * TODO(chromium:1021452)
+	 * This value being zero doesn't cause any problems, since it is never
+	 * read downstream. Fix or deprecate vboot_working_data.
+	 */
+	vbwb->range_size = 0;
 }

__weak uint32_t board_id(void) { return UNDEFINED_STRAPPING_ID; }
diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c
index 043748c..ea22bdf 100644
--- a/src/security/vboot/common.c
+++ b/src/security/vboot/common.c
@@ -73,11 +73,11 @@
     */
    memset(wd, 0, sizeof(*wd));
    wd->buffer_offset = ALIGN_UP(sizeof(*wd), 16);
-	wd->buffer_size = VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE
+	const uint16_t size = VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE
    		  - wd->buffer_offset;

/* Initialize vb2_shared_data and friends. */
-	assert(vb2api_init(vboot_get_workbuf(wd), wd->buffer_size,
+	assert(vb2api_init(vboot_get_workbuf(wd), size,
    		   vboot_ctx_ptr) == VB2_SUCCESS);

return *vboot_ctx_ptr;
@@ -137,14 +137,6 @@
    	cbmem_add(CBMEM_ID_VBOOT_WORKBUF, cbmem_size);
    assert(wd_cbmem != NULL);
    memcpy(wd_cbmem, wd_preram, sizeof(struct vboot_working_data));
-	/*
-	 * TODO(chromium:1021452): buffer_size is uint16_t and not large enough
-	 * to hold the kernel verification workbuf size.  The only code which
-	 * reads this value is in lb_vboot_workbuf() for lb_range->range_size.
-	 * This value being zero doesn't cause any problems, since it is never
-	 * read downstream.  Fix or deprecate vboot_working_data.
-	 */
-	wd_cbmem->buffer_size = 0;
    vb2api_relocate(vboot_get_workbuf(wd_cbmem),
    		vboot_get_workbuf(wd_preram),
    		cbmem_size - wd_cbmem->buffer_offset,
diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h
index 455773c..6141674 100644
--- a/src/security/vboot/misc.h
+++ b/src/security/vboot/misc.h
@@ -39,7 +39,6 @@
    struct selected_region selected_region;
    /* offset of the buffer from the start of this struct */
    uint16_t buffer_offset;
-	uint16_t buffer_size;
 };

/*

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36844 )

Change subject: security/vboot: Remove buffer_size from struct vboot_working_data
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36844/1/src/security/vboot/common.c 
File src/security/vboot/common.c:

https://review.coreboot.org/c/coreboot/+/36844/1/src/security/vboot/common.c... 
PS1, Line 80: size
No need for a temporary variable here.  In fact, actually introduces the bug that I was previously encountering, since uint16_t is too small for this number.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36844 )

Change subject: security/vboot: Remove buffer_size from struct vboot_working_data
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36844/1/src/lib/coreboot_table.c 
File src/lib/coreboot_table.c:

https://review.coreboot.org/c/coreboot/+/36844/1/src/lib/coreboot_table.c@23... 
PS1, Line 235: 	vbwb->range_size = 0;
If this was truly never read we could move this so add_cbmem_pointers() below, assuming vboot_working_data is sitting at the beginning of the cbmem region for this (I didn't double check it).

[Julius Werner (Code Review)]

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

Change subject: security/vboot: Remove buffer_size from struct vboot_working_data
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/36844/1/src/lib/coreboot_table.c 
File src/lib/coreboot_table.c:

https://review.coreboot.org/c/coreboot/+/36844/1/src/lib/coreboot_table.c@23... 
PS1, Line 235: 	vbwb->range_size = 0;
> If this was truly never read we could move this so add_cbmem_pointers() below, assuming vboot_workin […]

I think the problem is that we don't pass the whole range here, only the workbuffer part without the working_data header (see how range_start is set above). We'll eventually want to get to the point where we can put this into add_cbmem_pointers(), but we'll have to fully get rid of the working_data header first. This is an intermediate step in that direction.

That said, setting this to 0 here seems really wrong. Since range_size is a uint32_t (unlike wd->buffer_size) and we know the buffer must now always be VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE, can we just hardcode range_size to (VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE - wd->buffer_offset) here for now?

https://review.coreboot.org/c/coreboot/+/36844/1/src/security/vboot/common.c 
File src/security/vboot/common.c:

https://review.coreboot.org/c/coreboot/+/36844/1/src/security/vboot/common.c... 
PS1, Line 80: size
> No need for a temporary variable here. […]

It's not too small for the firmware buffer? But I agree there's no need for the extra variable (and also, for a temporary stack variable that's not serialized or permanently stored anywhere there's really never a reason not to use the most appropriate data type even if it is larger, which would be size_t for sizes).

[Yu-Ping Wu (Code Review)]

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

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

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

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

Change subject: security/vboot: Remove buffer_size from struct vboot_working_data
......................................................................

security/vboot: Remove buffer_size from struct vboot_working_data

Since buffer_size is no longer used, remove it from struct
vboot_working_data.

BRANCH=none
BUG=chromium:1021452
TEST=emerge-kukui coreboot

Change-Id: Ie770e89b4a45e0ec703d5bbb8fb6a298ce915056
Signed-off-by: Yu-Ping Wu 
---
M src/lib/coreboot_table.c
M src/security/vboot/common.c
M src/security/vboot/misc.h
3 files changed, 12 insertions(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/36844/2

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36844 )

Change subject: security/vboot: Remove buffer_size from struct vboot_working_data
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/36844/1/src/lib/coreboot_table.c 
File src/lib/coreboot_table.c:

https://review.coreboot.org/c/coreboot/+/36844/1/src/lib/coreboot_table.c@23... 
PS1, Line 235: 	vbwb->range_size = 0;
> I think the problem is that we don't pass the whole range here, only the workbuffer part without the […]

Done

https://review.coreboot.org/c/coreboot/+/36844/1/src/security/vboot/common.c 
File src/security/vboot/common.c:

https://review.coreboot.org/c/coreboot/+/36844/1/src/security/vboot/common.c... 
PS1, Line 80: size
> It's not too small for the firmware buffer? But I agree there's no need for the extra variable (and  […]

Done

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36844 )

Change subject: security/vboot: Remove buffer_size from struct vboot_working_data
......................................................................

Patch Set 2:

(4 comments)

Looks good after fixing the comment.

https://review.coreboot.org/c/coreboot/+/36844/1/src/lib/coreboot_table.c 
File src/lib/coreboot_table.c:

https://review.coreboot.org/c/coreboot/+/36844/1/src/lib/coreboot_table.c@23... 
PS1, Line 235: 	vbwb->range_size = 0;
> eventually want to get to the point where we can put this into add_cbmem_pointers()

Since the cbmem entry name is sort of a misnomer, but would suit our future use, shall we just keep it the same (VBOOT_WORKBUF)?

https://review.coreboot.org/c/coreboot/+/36844/2/src/lib/coreboot_table.c 
File src/lib/coreboot_table.c:

https://review.coreboot.org/c/coreboot/+/36844/2/src/lib/coreboot_table.c@23... 
PS2, Line 232: VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE
(VB2_... - wd->buffer_offset)
Or just say "a known value"

https://review.coreboot.org/c/coreboot/+/36844/2/src/lib/coreboot_table.c@23... 
PS2, Line 232: could
s/could //

https://review.coreboot.org/c/coreboot/+/36844/1/src/security/vboot/common.c 
File src/security/vboot/common.c:

https://review.coreboot.org/c/coreboot/+/36844/1/src/security/vboot/common.c... 
PS1, Line 80: size
> Done

It used to fit.  12 K < 64 K (uint16_t).

[Julius Werner (Code Review)]

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

Change subject: security/vboot: Remove buffer_size from struct vboot_working_data
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/36844/1/src/lib/coreboot_table.c 
File src/lib/coreboot_table.c:

https://review.coreboot.org/c/coreboot/+/36844/1/src/lib/coreboot_table.c@23... 
PS1, Line 235: 	vbwb->range_size = 0;
> > eventually want to get to the point where we can put this into add_cbmem_pointers() […]

Sure

https://review.coreboot.org/c/coreboot/+/36844/2/src/security/vboot/common.c 
File src/security/vboot/common.c:

https://review.coreboot.org/c/coreboot/+/36844/2/src/security/vboot/common.c... 
PS2, Line 80: 			   wd->buffer_offset,
nit: maybe that's just me but I'd indent this line one more tab.

[Yu-Ping Wu (Code Review)]

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

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

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

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

Change subject: security/vboot: Remove buffer_size from struct vboot_working_data
......................................................................

security/vboot: Remove buffer_size from struct vboot_working_data

Since buffer_size is no longer used, remove it from struct
vboot_working_data.

BRANCH=none
BUG=chromium:1021452
TEST=emerge-kukui coreboot

Change-Id: Ie770e89b4a45e0ec703d5bbb8fb6a298ce915056
Signed-off-by: Yu-Ping Wu 
---
M src/lib/coreboot_table.c
M src/security/vboot/common.c
M src/security/vboot/misc.h
3 files changed, 11 insertions(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/36844/3

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36844 )

Change subject: security/vboot: Remove buffer_size from struct vboot_working_data
......................................................................

Patch Set 3:

(3 comments)

https://review.coreboot.org/c/coreboot/+/36844/2/src/lib/coreboot_table.c 
File src/lib/coreboot_table.c:

https://review.coreboot.org/c/coreboot/+/36844/2/src/lib/coreboot_table.c@23... 
PS2, Line 232: VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE
> (VB2_... - wd->buffer_offset) […]

Done

https://review.coreboot.org/c/coreboot/+/36844/2/src/lib/coreboot_table.c@23... 
PS2, Line 232: could
> s/could //

Done

https://review.coreboot.org/c/coreboot/+/36844/2/src/security/vboot/common.c 
File src/security/vboot/common.c:

https://review.coreboot.org/c/coreboot/+/36844/2/src/security/vboot/common.c... 
PS2, Line 80: 			   wd->buffer_offset,
> nit: maybe that's just me but I'd indent this line one more tab.

Done

[Julius Werner (Code Review)]

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

Change subject: security/vboot: Remove buffer_size from struct vboot_working_data
......................................................................

Patch Set 3: Code-Review+2

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36844 )

Change subject: security/vboot: Remove buffer_size from struct vboot_working_data
......................................................................

Patch Set 3:

I'm going to rebase these along w/ my other patch. I submitted the vboot_named_region_* one.

[Patrick Georgi (Code Review)]

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

Change subject: security/vboot: Remove buffer_size from struct vboot_working_data
......................................................................

security/vboot: Remove buffer_size from struct vboot_working_data

Since buffer_size is no longer used, remove it from struct
vboot_working_data.

BRANCH=none
BUG=chromium:1021452
TEST=emerge-kukui coreboot

Change-Id: Ie770e89b4a45e0ec703d5bbb8fb6a298ce915056
Signed-off-by: Yu-Ping Wu 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/36844
Tested-by: build bot (Jenkins) 
Reviewed-by: Julius Werner 
---
M src/lib/coreboot_table.c
M src/security/vboot/common.c
M src/security/vboot/misc.h
3 files changed, 11 insertions(+), 13 deletions(-)

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

diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c
index d3576e6..241d8e1 100644
--- a/src/lib/coreboot_table.c
+++ b/src/lib/coreboot_table.c
@@ -227,7 +227,14 @@
    vbwb->tag = LB_TAG_VBOOT_WORKBUF;
    vbwb->size = sizeof(*vbwb);
    vbwb->range_start = (uintptr_t)wd + wd->buffer_offset;
-	vbwb->range_size = wd->buffer_size;
+	/*
+	 * TODO(chromium:1021452): Since cbmem size of vboot workbuf is now
+	 * always a known value, we hardcode the value of range_size here.
+	 * Ultimately we'll want to move this to add_cbmem_pointers() below,
+	 * but we'll have to get rid of the vboot_working_data struct first.
+	 */
+	vbwb->range_size = VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE -
+		wd->buffer_offset;
 }

__weak uint32_t board_id(void) { return UNDEFINED_STRAPPING_ID; }
diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c
index 043748c..3f57602 100644
--- a/src/security/vboot/common.c
+++ b/src/security/vboot/common.c
@@ -73,11 +73,11 @@
     */
    memset(wd, 0, sizeof(*wd));
    wd->buffer_offset = ALIGN_UP(sizeof(*wd), 16);
-	wd->buffer_size = VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE
-			  - wd->buffer_offset;

/* Initialize vb2_shared_data and friends. */
-	assert(vb2api_init(vboot_get_workbuf(wd), wd->buffer_size,
+	assert(vb2api_init(vboot_get_workbuf(wd),
+			   VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE -
+				wd->buffer_offset,
    		   vboot_ctx_ptr) == VB2_SUCCESS);

return *vboot_ctx_ptr;
@@ -137,14 +137,6 @@
    	cbmem_add(CBMEM_ID_VBOOT_WORKBUF, cbmem_size);
    assert(wd_cbmem != NULL);
    memcpy(wd_cbmem, wd_preram, sizeof(struct vboot_working_data));
-	/*
-	 * TODO(chromium:1021452): buffer_size is uint16_t and not large enough
-	 * to hold the kernel verification workbuf size.  The only code which
-	 * reads this value is in lb_vboot_workbuf() for lb_range->range_size.
-	 * This value being zero doesn't cause any problems, since it is never
-	 * read downstream.  Fix or deprecate vboot_working_data.
-	 */
-	wd_cbmem->buffer_size = 0;
    vb2api_relocate(vboot_get_workbuf(wd_cbmem),
    		vboot_get_workbuf(wd_preram),
    		cbmem_size - wd_cbmem->buffer_offset,
diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h
index 812bbe7..e438848 100644
--- a/src/security/vboot/misc.h
+++ b/src/security/vboot/misc.h
@@ -39,7 +39,6 @@
    struct selected_region selected_region;
    /* offset of the buffer from the start of this struct */
    uint16_t buffer_offset;
-	uint16_t buffer_size;
 };

/*