Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36208 )
Change subject: security/vboot: Add vboot callbacks to support EC software sync ......................................................................
Patch Set 9:
(9 comments)
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/Kconfig@... PS9, Line 230: depends on CHROMEOS Why? (If anything, I'd say it depends on EC_GOOGLE_CHROMEEC.)
https://review.coreboot.org/c/coreboot/+/36208/8/src/security/vboot/ec_sync.... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/36208/8/src/security/vboot/ec_sync.... PS8, Line 119: (recalc_requested)
Excellent point, this was ported straight from depthcharge w/ as few changes as possible. […]
Isn't resp.status re-read from the EC on every loop iteration? We just set it to BUSY so we don't exit the loop, but then it might already be something else again by the time we get back here. This looks to me like it's intended to avoid us spamming the EC with new START_HASH requests too quickly before it has a chance to process one of those requests far enough to switch its internal hash status to BUSY. I'm not sure if that's really a concern in practice with the current EC code, but from a purely AP point of view it's not just dead code.
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/ec_sync.... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/ec_sync.... PS9, Line 113: case EC_VBOOT_HASH_STATUS_NONE: Like mentioned in the other patch set I think we should keep the recalc_requested unless someone can say for certain from the EC sources that it's not needed.
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/ec_sync.... PS9, Line 235: if (google_chromeec_get_protocol_info(&resp_proto)) nit: might be worth an error message
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/ec_sync.... PS9, Line 245: return 0; This should be an error (right?) and should probably also print something.
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/ec_sync.... PS9, Line 258: burst -= sizeof(struct ec_host_request); No, this... isn't this all wrong? 'burst' *must* be an exact multiple of write_block_size, and the total size of the packet (n * burst + headers) must be smaller or equal to both pdata_max_size and 1024.
I think what you want is
/* Hard limit packet buffer stack allocation size to 1K. */ pdata_max_size = MIN(1024, resp_proto.max_request_packet_size);
burst = pdata_max_size - sizeof(*p); burst = (burst / resp_flash.write_block_size) * resp_flash.write_block_size;
p = alloca(burst + sizeof(*p));
(I don't think you need to count ec_host_request? That's generated separately by the EC command handler, it's not on the alloca buffer.)
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/ec_sync.... PS9, Line 261: bufsize = MIN(1024, burst); ...uhhh... and here you're potentially shrinking the buffer to a smaller size but still using the larger size in the code that fills it below? Bad idea. See above.
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/ec_sync.... PS9, Line 427: "flag.\n"); I think you misunderstood my comment here, I meant there seems to be no need for a line break in this string
https://review.coreboot.org/c/coreboot/+/36208/9/src/security/vboot/ec_sync.... PS9, Line 428: rv = VB2_ERROR_UNKNOWN; (I think the immediate return here was fine btw)