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 11:
(5 comments)
https://review.coreboot.org/c/coreboot/+/36208/11/src/security/vboot/ec_sync... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/36208/11/src/security/vboot/ec_sync... PS11, Line 269: burst = pdata_max_size - sizeof(*params); This could go negative for weird edge cases, so lets make burst an int or ssize_t to be safe.
https://review.coreboot.org/c/coreboot/+/36208/11/src/security/vboot/ec_sync... PS11, Line 273: if (burst <= sizeof(struct ec_params_flash_write)) { This check doesn't make much sense now because 'burst' doesn't include actually the space for the params. Really all you want to check at this point is that burst is larger than zero.
https://review.coreboot.org/c/coreboot/+/36208/11/src/security/vboot/ec_sync... PS11, Line 280: read_buffer = alloca(bufsize); nit: not sure why you need that extra variable rather than doing
params = alloca(bufsize);
directly
https://review.coreboot.org/c/coreboot/+/36208/11/src/security/vboot/ec_sync... PS11, Line 300: - noop for a memory region (Not sure what this means, I think you're thinking of memory-mapped SPI which is explicitly not really "a memory region". CBFS cache *is* a memory region but there this is not a no-op. I'm not sure you really need a comment at all here, the rdev API is pretty ubiquitous and well-understood across coreboot.)
https://review.coreboot.org/c/coreboot/+/36208/11/src/security/vboot/ec_sync... PS11, Line 307: printk(BIOS_ERR, "EC failed flash write command!\n"); nit: maybe print offset?