Hi,
Please find the latest report on new defect(s) introduced to coreboot found with Coverity Scan.
1 new defect(s) introduced to coreboot found with Coverity Scan.
New defect(s) Reported-by: Coverity Scan Showing 1 of 1 defect(s)
** CID 1498959: (STRING_OVERFLOW) /src/mainboard/prodrive/hermes/mainboard.c: 183 in format_pn() /src/mainboard/prodrive/hermes/mainboard.c: 182 in format_pn()
________________________________________________________________________________________________________ *** CID 1498959: (STRING_OVERFLOW) /src/mainboard/prodrive/hermes/mainboard.c: 183 in format_pn() 177 static char buffer[32 + HERMES_SN_PN_LENGTH] = { 0 }; 178 179 const char *part_num = eeprom_read_serial(offset, "N/A"); 180 181 memset(buffer, 0, sizeof(buffer)); 182 strcpy(buffer, prefix);
CID 1498959: (STRING_OVERFLOW) You might overrun the 64-character fixed-size string "buffer + strlen(prefix)" by copying "part_num" without checking the length.
183 strcpy(buffer + strlen(prefix), part_num); 184 185 return buffer; 186 } 187 188 static void mainboard_smbios_strings(struct device *dev, struct smbios_type11 *t) /src/mainboard/prodrive/hermes/mainboard.c: 182 in format_pn() 176 { 177 static char buffer[32 + HERMES_SN_PN_LENGTH] = { 0 }; 178 179 const char *part_num = eeprom_read_serial(offset, "N/A"); 180 181 memset(buffer, 0, sizeof(buffer));
CID 1498959: (STRING_OVERFLOW) You might overrun the 64-character fixed-size string "buffer" by copying "prefix" without checking the length.
182 strcpy(buffer, prefix); 183 strcpy(buffer + strlen(prefix), part_num); 184 185 return buffer; 186 } 187
________________________________________________________________________________________________________ To view the defects in Coverity Scan visit, https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0...
Hi list,
We made the patches that made Coverity angry about this `format_pn()` function. However, this is not an actual bug: the `eeprom_read_serial()` function returns a buffer that is at most 32 (`HERMES_SN_PN_LENGTH`) characters long, and the length of the `prefix` string is known at build-time (it's a string literal in both call sites) to be less than 32 characters long.
Does anyone have any ideas to make Coverity shut up without making the code unnecessarily ugly?
Thanks in advance, Angel
"Angel Pons" th3fanbus@gmail.com schrieb:
We made the patches that made Coverity angry about this `format_pn()` function. However, this is not an actual bug: the `eeprom_read_serial()` function returns a buffer that is at most 32 (`HERMES_SN_PN_LENGTH`) characters long, and the length of the `prefix` string is known at build-time (it's a string literal in both call sites) to be less than 32 characters long.
There's no guarantee that the string returned by eeprom_read_serial() is 0-terminated (not even in its implementation) and strcpy proceeds until the first 0 it sees, even if that's only 2GB later. Use strncpy instead to prevent out of bound copies.
Patrick
Hi Patrick,
On Tue, Oct 11, 2022 at 6:43 PM Patrick Georgi patrick@coreboot.org wrote:
"Angel Pons" th3fanbus@gmail.com schrieb:
We made the patches that made Coverity angry about this `format_pn()` function. However, this is not an actual bug: the `eeprom_read_serial()` function returns a buffer that is at most 32 (`HERMES_SN_PN_LENGTH`) characters long, and the length of the `prefix` string is known at build-time (it's a string literal in both call sites) to be less than 32 characters long.
There's no guarantee that the string returned by eeprom_read_serial() is 0-terminated (not even in its implementation) and strcpy proceeds until the first 0 it sees, even if that's only 2GB later. Use strncpy instead to prevent out of bound copies.
Oh, we wanted to use `strncat()` but it doesn't exist in coreboot. Then we considered `strcat()` and it didn't exist either, and we ended up using `strcpy()`. Will use `strncpy()`, thank you!
Patrick
Best regards, Angel
Hi all,
On Tue, Oct 11, 2022 at 7:03 PM Angel Pons th3fanbus@gmail.com wrote:
Hi Patrick,
On Tue, Oct 11, 2022 at 6:43 PM Patrick Georgi patrick@coreboot.org wrote:
"Angel Pons" th3fanbus@gmail.com schrieb:
We made the patches that made Coverity angry about this `format_pn()` function. However, this is not an actual bug: the `eeprom_read_serial()` function returns a buffer that is at most 32 (`HERMES_SN_PN_LENGTH`) characters long, and the length of the `prefix` string is known at build-time (it's a string literal in both call sites) to be less than 32 characters long.
There's no guarantee that the string returned by eeprom_read_serial() is 0-terminated (not even in its implementation) and strcpy proceeds until the first 0 it sees, even if that's only 2GB later. Use strncpy instead to prevent out of bound copies.
Oh, we wanted to use `strncat()` but it doesn't exist in coreboot. Then we considered `strcat()` and it didn't exist either, and we ended up using `strcpy()`. Will use `strncpy()`, thank you!
After some discussion on IRC, we ended up making https://review.coreboot.org/68322 and https://review.coreboot.org/68323 to address the issues. Thank you all.
Patrick
Best regards, Angel
Best regards, Angel
-
Angel Pons -
Patrick Georgi -
scan-admin＠coverity.com