Skip to content

onlp/sysi: Make the ONIE IDPROM buffer pass strict_string_checks - #216

Open
richardkuo1999 wants to merge 1 commit into
accton:support_linux_4.19from
richardkuo1999:4.19-ONSBUSWI-3713
Open

onlp/sysi: Make the ONIE IDPROM buffer pass strict_string_checks#216
richardkuo1999 wants to merge 1 commit into
accton:support_linux_4.19from
richardkuo1999:4.19-ONSBUSWI-3713

Conversation

@richardkuo1999

Copy link
Copy Markdown

onlp_sysi_onie_data_get() allocated exactly 256 bytes and filled every one of them with raw IDPROM content, so the buffer handed to is_valid_tlvinfo_header__() is not guaranteed to be NUL-terminated. strcmp() requires a terminated string, so AddressSanitizer with strict_string_checks=1 reports a heap-buffer-overflow on that call whenever the device carries no zero byte at all - an erased or corrupted IDPROM. Allocating one extra zero byte makes the same run come back clean.

Real hardware is not affected: on a programmed IDPROM totallen sits at offsets 9-10, so data[9] is 0x00 for any payload below 256 bytes, and the comparison resolves at index 7 or earlier. The change is about satisfying the strcmp contract under strict checking, not about a failure observed in the field.

The read length is deliberately left at 256, so the decoded content and the *size == len guard behave exactly as before.

  • introduce a local const int len instead of repeating the literal 256
  • derive the i2c word-read loop bound from len (len / 2) on as7716-24xc, as7716-32x and as7816-64x, and the byte-read bound on as9516-32d
  • covers 34 platforms; no behavioural change on any of them

onlp_sysi_onie_data_get() allocated exactly 256 bytes and filled every
one of them with raw IDPROM content, so the buffer handed to
is_valid_tlvinfo_header__() is not guaranteed to be NUL-terminated.
strcmp() requires a terminated string, so AddressSanitizer with
strict_string_checks=1 reports a heap-buffer-overflow on that call
whenever the device carries no zero byte at all - an erased or corrupted
IDPROM. Allocating one extra zero byte makes the same run come back
clean.

Real hardware is not affected: on a programmed IDPROM totallen sits at
offsets 9-10, so data[9] is 0x00 for any payload below 256 bytes, and
the comparison resolves at index 7 or earlier. The change is about
satisfying the strcmp contract under strict checking, not about a
failure observed in the field.

The read length is deliberately left at 256, so the decoded content and
the *size == len guard behave exactly as before.

- introduce a local const int len instead of repeating the literal 256
- derive the i2c word-read loop bound from len (len / 2) on as7716-24xc,
  as7716-32x and as7816-64x, and the byte-read bound on as9516-32d
- covers 34 platforms; no behavioural change on any of them
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant