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).