xtensa: fix OOB TLB entry access - #42
Open
hinaultd wants to merge 1 commit into
Open
Conversation
The r[id]tlb[01], [iw][id]tlb opcodes use a TLB way index supplied by the
guest in a register. split_tlb_entry_spec() masks that index with 0x7 for
the ITLB and 0xf for the DTLB, yielding 0..7 and 0..15 respectively, but
the backing host arrays are declared as
xtensa_tlb_entry itlb[7][MAX_TLB_WAY_SIZE];
xtensa_tlb_entry dtlb[10][MAX_TLB_WAY_SIZE];
in arch/xtensa/cpu.h. Way index 7 is therefore out of bounds for the ITLB,
and 10..15 are out of bounds for the DTLB. Guest code can reach this
through rdtlb0/rdtlb1/wdtlb/witlb/idtlb/iitlb, so a malicious or simply
buggy guest can read and write host memory past those arrays.
This is a backport of QEMU commit 604927e357c2 ("target/xtensa: fix OOB
TLB entry access", 2023-12-15), which tlib's xtensa port predates:
- split_tlb_entry_spec() now returns whether the requested way is valid,
- get_tlb_entry() returns NULL for an invalid way,
- the rtlb0/rtlb1/itlb helpers check for NULL and return 0 or do nothing,
- wtlb skips the write for an invalid way,
- xtensa_tlb_get_entry() asserts that the way and entry indices are in
range.
Only cores with XCHAL_HAVE_PTP_MMU are affected, i.e. those that enable
XTENSA_OPTION_MMU: dc233c, de233_fpu and test_mmuhifi_c3. Cores using
region protection instead, such as esp32 and esp32s3, take the else branch
of split_tlb_entry_spec() where the way index is hardcoded to 0.
One deliberate deviation from upstream: the new bounds check in
xtensa_tlb_get_entry() uses tlib_assert() rather than assert(). assert()
is compiled out in the Release build, which both defeats the check where
it matters and leaves the tlb local variable unused, producing a
-Wunused-variable warning. tlib_assert() is always active and reports
through tlib_abortf(), consistent with its use elsewhere in tlib.
Verified by rebuilding translate-xtensa-le.so and booting a real ESP-IDF
image on an esp32s3 core: behaviour is unchanged and the new assertion
does not fire. That path goes through get_physical_addr_region(), which
calls xtensa_tlb_get_entry() directly with wi = 0 and ei masked to 0..7,
against a region-protection TLB configured as nways = 1, way_size = {8},
so the indices are in range by construction.
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
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.
Backport of QEMU commit
604927e357c2("target/xtensa: fix OOB TLB entry access", 2023-12-15), which tlib's xtensa port predates.The problem
r[id]tlb[01]and[iw][id]tlbtake the TLB way index from a guest register.split_tlb_entry_spec()masks it with0x7for the ITLB and0xffor the DTLB:but the backing host arrays in
arch/xtensa/cpu.hareso way index 7 is out of bounds for the ITLB and 10..15 are out of bounds for the DTLB.
xtensa_tlb_get_entry()indexes them without any check, andget_tlb_entry()cannot fail, sordtlb0/rdtlb1/wdtlb/witlb/idtlb/iitlblet guest code read and write host memory past those arrays.The fix
Same shape as upstream:
split_tlb_entry_spec()returns whether the requested way is valid;get_tlb_entry()returnsNULLfor an invalid way;rtlb0/rtlb1/itlbhelpers check forNULLand return 0 or do nothing;wtlbskips the write for an invalid way;xtensa_tlb_get_entry()asserts that the way and entry indices are in range.Affected cores
Only cores with
XCHAL_HAVE_PTP_MMU, i.e. those enablingXTENSA_OPTION_MMU:dc233c,de233_fpu,test_mmuhifi_c3. Cores using region protection instead — includingesp32andesp32s3— take theelsebranch ofsplit_tlb_entry_spec(), where the way index is hardcoded to 0.One deliberate deviation from upstream
The new bounds check in
xtensa_tlb_get_entry()usestlib_assert()rather thanassert().assert()is compiled out in the Release build, which both defeats the check where it matters and leaves thetlblocal unused — the first build of this patch emitted-Wunused-variable.tlib_assert()is always active and reports throughtlib_abortf(), consistent with its use elsewhere in tlib. Happy to switch it back if you'd rather stay byte-for-byte with upstream.Testing
translate-xtensa-le.soforTARGET_ARCH=xtensa,CMAKE_BUILD_TYPE=Release: compiles without warnings.esp32s3core before and after: behaviour unchanged, and the new assertion does not fire. That path reachesxtensa_tlb_get_entry()throughget_physical_addr_region()withwi = 0andeimasked to 0..7, against a region-protection TLB configured asnways = 1, way_size = {8}— in range by construction.