[BUG][Cyrus BOARD] out of bound access in board/varisys/common/sys_eeprom.c
Hello Andy,
in the code of your patch "mpc85xx: Add support for the Varisys Cyrus board" merged in 2015 as 87e29878cab an out of bound access occurs. See below.
On 11/4/15 10:48 PM, Andy Fleming wrote:
This board runs a P5020 or P5040 chip, and utilizes an EEPROM with similar formatting to the Freescale P5020DS.
Large amounts of this code were developed by Adrian Cox <adrian at humboldt dot co dot uk>
Signed-off-by: Andy Fleming afleming@gmail.com Reviewed-by: York Sun yorksun@freescale.com
<snip />
+++ b/board/varisys/common/sys_eeprom.c
<snip />
+static struct __attribute__ ((__packed__)) eeprom {
- u8 id[4]; /* 0x00 - 0x03 EEPROM Tag 'NXID' */
- u8 sn[12]; /* 0x04 - 0x0F Serial Number */
- u8 errata[5]; /* 0x10 - 0x14 Errata Level */
- u8 date[6]; /* 0x15 - 0x1a Build Date */
- u8 res_0; /* 0x1b Reserved */
- u32 version; /* 0x1c - 0x1f NXID Version */
- u8 tempcal[8]; /* 0x20 - 0x27 Temperature Calibration Factors */
- u8 tempcalsys[2]; /* 0x28 - 0x29 System Temperature Calibration
Factors */
- u8 tempcalflags; /* 0x2a Temperature Calibration Flags */
- u8 res_1[21]; /* 0x2b - 0x3f Reserved */
- u8 mac_count; /* 0x40 Number of MAC addresses */
- u8 mac_flag; /* 0x41 MAC table flags */
- u8 mac[MAX_NUM_PORTS][6]; /* 0x42 - x MAC addresses */
- u32 crc; /* x+1 CRC32 checksum */
+} e;
<snip /> // MAX_NUM_PORTS = 8
+int mac_read_from_eeprom_common(void) +{
- unsigned int i;
- u32 crc, crc_offset = offsetof(struct eeprom, crc);
- u32 *crcp; /* Pointer to the CRC in the data read from the EEPROM */
- puts("EEPROM: ");
- if (read_eeprom()) {
printf("Read failed.\n");return 0;- }
- if (!is_valid) {
printf("Invalid ID (%02x %02x %02x %02x)\n",e.id[0], e.id[1], e.id[2], e.id[3]);return 0;- }
- crc = crc32(0, (void *)&e, crc_offset);
- crcp = (void *)&e + crc_offset;
- if (crc != be32_to_cpu(*crcp)) {
printf("CRC mismatch (%08x != %08x)\n", crc,be32_to_cpu(e.crc));return 0;- }
- /*
* MAC address #9 in v1 occupies the same position as the CRC in v0.* Erase it so that it's not mistaken for a MAC address. We'll* update the CRC later.*/- if (e.version == 0)
memset(e.mac[8], 0xff, 6);
Here you are writing 2 bytes beyond the size of e.
Best regards
Heinrich
On Mon, Apr 12, 2021 at 12:25:32AM +0200, Heinrich Schuchardt wrote:
Hello Andy,
in the code of your patch "mpc85xx: Add support for the Varisys Cyrus board" merged in 2015 as 87e29878cab an out of bound access occurs. See below.
On 11/4/15 10:48 PM, Andy Fleming wrote:
This board runs a P5020 or P5040 chip, and utilizes an EEPROM with similar formatting to the Freescale P5020DS.
Large amounts of this code were developed by Adrian Cox <adrian at humboldt dot co dot uk>
Signed-off-by: Andy Fleming afleming@gmail.com Reviewed-by: York Sun yorksun@freescale.com
<snip />
+++ b/board/varisys/common/sys_eeprom.c
<snip />
+static struct __attribute__ ((__packed__)) eeprom {
- u8 id[4]; /* 0x00 - 0x03 EEPROM Tag 'NXID' */
- u8 sn[12]; /* 0x04 - 0x0F Serial Number */
- u8 errata[5]; /* 0x10 - 0x14 Errata Level */
- u8 date[6]; /* 0x15 - 0x1a Build Date */
- u8 res_0; /* 0x1b Reserved */
- u32 version; /* 0x1c - 0x1f NXID Version */
- u8 tempcal[8]; /* 0x20 - 0x27 Temperature Calibration Factors */
- u8 tempcalsys[2]; /* 0x28 - 0x29 System Temperature Calibration
Factors */
- u8 tempcalflags; /* 0x2a Temperature Calibration Flags */
- u8 res_1[21]; /* 0x2b - 0x3f Reserved */
- u8 mac_count; /* 0x40 Number of MAC addresses */
- u8 mac_flag; /* 0x41 MAC table flags */
- u8 mac[MAX_NUM_PORTS][6]; /* 0x42 - x MAC addresses */
- u32 crc; /* x+1 CRC32 checksum */
+} e;
<snip /> // MAX_NUM_PORTS = 8
+int mac_read_from_eeprom_common(void) +{
- unsigned int i;
- u32 crc, crc_offset = offsetof(struct eeprom, crc);
- u32 *crcp; /* Pointer to the CRC in the data read from the EEPROM */
- puts("EEPROM: ");
- if (read_eeprom()) {
printf("Read failed.\n");return 0;- }
- if (!is_valid) {
printf("Invalid ID (%02x %02x %02x %02x)\n",e.id[0], e.id[1], e.id[2], e.id[3]);return 0;- }
- crc = crc32(0, (void *)&e, crc_offset);
- crcp = (void *)&e + crc_offset;
- if (crc != be32_to_cpu(*crcp)) {
printf("CRC mismatch (%08x != %08x)\n", crc,be32_to_cpu(e.crc));return 0;- }
- /*
* MAC address #9 in v1 occupies the same position as the CRC in v0.* Erase it so that it's not mistaken for a MAC address. We'll* update the CRC later.*/- if (e.version == 0)
memset(e.mac[8], 0xff, 6);Here you are writing 2 bytes beyond the size of e.
A useful analysis in case anyone brings these boards back. I'm about to push the series that delete this due to lack of migration.
On Sun, Apr 11, 2021 at 17:40 Tom Rini trini@konsulko.com wrote:
On Mon, Apr 12, 2021 at 12:25:32AM +0200, Heinrich Schuchardt wrote:
Hello Andy,
in the code of your patch "mpc85xx: Add support for the Varisys Cyrus board" merged in 2015 as 87e29878cab an out of bound access occurs. See below.
On 11/4/15 10:48 PM, Andy Fleming wrote:
This board runs a P5020 or P5040 chip, and utilizes an EEPROM with similar formatting to the Freescale P5020DS.
Large amounts of this code were developed by Adrian Cox <adrian at humboldt dot co dot uk>
Signed-off-by: Andy Fleming afleming@gmail.com Reviewed-by: York Sun yorksun@freescale.com
<snip />
+++ b/board/varisys/common/sys_eeprom.c
<snip />
+static struct __attribute__ ((__packed__)) eeprom {
- u8 id[4]; /* 0x00 - 0x03 EEPROM Tag 'NXID' */
- u8 sn[12]; /* 0x04 - 0x0F Serial Number */
- u8 errata[5]; /* 0x10 - 0x14 Errata Level */
- u8 date[6]; /* 0x15 - 0x1a Build Date */
- u8 res_0; /* 0x1b Reserved */
- u32 version; /* 0x1c - 0x1f NXID Version */
- u8 tempcal[8]; /* 0x20 - 0x27 Temperature Calibration Factors */
- u8 tempcalsys[2]; /* 0x28 - 0x29 System Temperature Calibration
Factors */
- u8 tempcalflags; /* 0x2a Temperature Calibration Flags */
- u8 res_1[21]; /* 0x2b - 0x3f Reserved */
- u8 mac_count; /* 0x40 Number of MAC addresses */
- u8 mac_flag; /* 0x41 MAC table flags */
- u8 mac[MAX_NUM_PORTS][6]; /* 0x42 - x MAC addresses */
- u32 crc; /* x+1 CRC32 checksum */
+} e;
<snip /> // MAX_NUM_PORTS = 8
+int mac_read_from_eeprom_common(void) +{
- unsigned int i;
- u32 crc, crc_offset = offsetof(struct eeprom, crc);
- u32 *crcp; /* Pointer to the CRC in the data read from the EEPROM
*/
- puts("EEPROM: ");
- if (read_eeprom()) {
printf("Read failed.\n");return 0;- }
- if (!is_valid) {
printf("Invalid ID (%02x %02x %02x %02x)\n",e.id[0], e.id[1], e.id[2], e.id[3]);return 0;- }
- crc = crc32(0, (void *)&e, crc_offset);
- crcp = (void *)&e + crc_offset;
- if (crc != be32_to_cpu(*crcp)) {
printf("CRC mismatch (%08x != %08x)\n", crc,be32_to_cpu(e.crc));return 0;- }
- /*
- MAC address #9 in v1 occupies the same position as the CRC in
v0.
- Erase it so that it's not mistaken for a MAC address. We'll
- update the CRC later.
- */
- if (e.version == 0)
memset(e.mac[8], 0xff, 6);Here you are writing 2 bytes beyond the size of e.
A useful analysis in case anyone brings these boards back. I'm about to push the series that delete this due to lack of migration.
Interesting. It’s clear there’s ambiguity there, as some versions have more data there, but that definitely looks wrong. Unfortunately, I don’t have one of these boards anymore (And I doubt nxp does), so no resurrection anytime soon.
Andy
participants (3)
-
Andy Fleming -
Heinrich Schuchardt -
Tom Rini