events-arch-arm: check the payload length in the ampere, hisilicon and jaguarmicro decoders - #265
Open
agenticode wants to merge 3 commits into
Open
agenticode wants to merge 3 commits into
agenticode wants to merge 3 commits into
Conversation
decode_amp_oem_type_error() picks a payload type from event->error[0]
and casts the buffer to the matching struct without looking at
event->length. decode_amp_arm_vendor_data() is worse: it ignores its
length argument entirely and casts the buffer to a 48-byte
amp_payload0_type_sec.
That one is registered with .midr = -1, so it is the fallback decoder
for every ARM CPU with no exact match, not only Ampere parts.
Feeding ras:arm_event records whose vendor data is shorter than the
struct kills the daemon:
AddressSanitizer: heap-buffer-overflow
READ of size 8
#0 decode_amp_payload0_err_regs non-standard-ampere.c:657
mchehab#1 decode_amp_arm_vendor_data non-standard-ampere.c:1016
mchehab#2 ras_arm_vendor_data_decode ras-arm-vendor-data.c:75
mchehab#3 ras_arm_event_handler ras-arm-handler.c:579
0x7d1fe90ae104 is located 4 bytes after 4096-byte region
The object it runs off is the trace ring buffer page itself, so without
ASan this silently decodes whatever follows it and writes that to the
database.
non-standard-ampereone.c already checks this the same way.
Signed-off-by: Jongwon Lee <ai@linux.com>
decode_hisi_common_section() casts event->error to
hisi_common_error_section without checking event->length, then dumps
err->reg_array up to err->reg_array_size, which comes straight from the
firmware payload. Both read past the end of a short record.
A ras:non_standard_event with a 1-byte payload on the common section
GUID gives:
AddressSanitizer: heap-buffer-overflow
READ of size 4
#0 decode_hisi_common_section non-standard-hisilicon.c:331
Check the length before the cast and clamp the register dump to the
bytes that were actually delivered.
Signed-off-by: Jongwon Lee <ai@linux.com>
decode_jm_oem_type_error() casts event->error to one of five payload
structs without checking event->length, and decode_jm_common_sec_tail()
loops over err->reg_array up to the firmware-supplied
err->reg_array_size.
A ras:non_standard_event with a 1-byte payload on three of the
JaguarMicro section GUIDs gives:
AddressSanitizer: heap-buffer-overflow
READ of size 4
#0 decode_jm_common_sec_tail non-standard-jaguarmicro.c:574
Check the length before each cast and pass the remaining byte count down
to the tail decoder so the register dump stops at the end of the record.
Signed-off-by: Jongwon Lee <ai@linux.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three ARM vendor decoders cast
event->errorto a fixed-size struct without looking atevent->length, and two of them walk a register array whose length also comes from the firmware payload. A short CPER record makes them read past the end of the trace ring buffer page.154bdc1("rasdaemon: fix several security issues") fixed this class innon-standard-yitian.cand maderas_arm_event_handler()rejectoem_len > len, so the length handed to the decoders is correct now - these three just never look at it.non-standard-ampereone.chas had the check since it was added.The worst one is
decode_amp_arm_vendor_data(): it is registered with.midr = -1, so it is the fallback for every ARM CPU that has no exact match, not only Ampere parts. An Ampere Altra reports MIDR 0x413fd0c1 and has no exact handler, so that is the path it takes.Reproducer
x86_64, Fedora 44, kernel 6.19.10, ASan build, events generated from a small out-of-tree module firing the real tracepoints.
ras:arm_event, vendor data shorter thanstruct amp_payload0_type_sec- dies after 149 of 4000 events, 3 runs out of 3:The region is the ring buffer page. Without ASan the decode usually stays inside the page, so instead of crashing it decodes the next event's bytes and inserts them into the database.
Same on aarch64 (Fedora 44, gcc 16.2, ASan), this time deterministic: the registered decoders are called directly through the
HAVE_UNITTESThooks with a 1-byte payload, one fork per decoder, so every one of the 14 gets a verdict.With the patches applied every one of those returns -1 with a "truncated payload" line instead, and the x86_64 soak keeps logging.
Testing
aarch64, Fedora 44:
meson test: 9/9 with both compilerspython3 tests/run.py: 140 tests, rc=0scripts/checkpatch.pl --no-tree --strict: 0 errors, 0 warnings, 0 checks on all three patchesx86_64, Fedora 44, 8 vCPU: same,
meson test9/9 with gcc and clang,tests/run.py140 tests.non-standard-hisi_hip08.chas the same bug - the aarch64 sweep kills it indecode_hip08_oem_type1_error(:684),decode_hip08_oem_type2_error(:850) anddecode_hip08_pcie_local_error(:995). It is the same fix, but it is not in this series; I can send it as a follow-up.