Attention is currently required from: Felix Singer, Nico Huber, Paul Menzel, Angel Pons.
Utkarsh Verma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74128 )
Change subject: arch/x86/smbios: Avoid buffer overflows ......................................................................
Patch Set 4:
(3 comments)
File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/74128/comment/2dab843c_a5f0faf0 PS4, Line 148: char string_buffer[15];
In the case below (i.e. 8 byte buffer storing %d) it might make a difference […]
Thanks for pointing out the buffer size, will fix it in the next patch.
I would be interested in working towards improving the current handling of `t->eos` and I've already explored the smbios codebase a bit. Will try working on a different patch and wait for feedback.
https://review.coreboot.org/c/coreboot/+/74128/comment/cc341069_abb129e8 PS4, Line 552: static unsigned short cnt = 0;
Changing the variable type makes no difference because of the "integer […]
Oh, this is a nice practical concept I didn't know. Thanks for the reference link.
Regarding changing the data type, I did it to hint that it will be printed with "%hx" below as a 4-digit hex number. So it is already understood by developers that it is not expected to be more than 255. So, even if the compiler promotes it, we'll simply ensure at other places that its value doesn't exceed 255.
https://review.coreboot.org/c/coreboot/+/74128/comment/72b4832d_cdf79e69 PS4, Line 565: hh
IIRC, we have or are close to having support for platforms with >256 cores. […]
Got it. I will add this in the next set of changes.
-
Utkarsh Verma (Code Review)