[BUG][Cyrus BOARD] out of bound access in board/varisys/common/sys_eeprom.c

[Andy Fleming]

On Sun, Apr 11, 2021 at 17:40 Tom Rini <trini at 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 at gmail.com>
> > > Reviewed-by: York Sun <yorksun at 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