Page MenuHomeFreeBSD

arm64 pmap: Fix a buffer overrun initializing per-superpage locks.
ClosedPublic

Authored by jhb on Feb 3 2023, 7:04 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Oct 9, 12:04 PM
Unknown Object (File)
Mon, Sep 30, 1:39 PM
Unknown Object (File)
Sep 20 2024, 6:17 PM
Unknown Object (File)
Sep 20 2024, 5:35 AM
Unknown Object (File)
Sep 18 2024, 12:26 PM
Unknown Object (File)
Sep 17 2024, 2:40 AM
Unknown Object (File)
Sep 15 2024, 10:03 PM
Unknown Object (File)
Sep 15 2024, 2:11 AM
Subscribers

Details

Summary

pmap_init_pv_table makes a first pass over the memory segments to
compute the amount of address space needed to allocate per-superpage
locks. It then makes a second pass over each segment allocating
domain-local memory to back the pages for the locks belonging to each
segment. This second pass rounds each segment's allocation up to a
page size since the domain-local allocation has to be a multiple of
pages. However, the first pass was only doing a single round of the
total page counts up at the end not accounting for the padding present
in each segment. To fix, apply the rounding in each segment in the
first pass instead of just at the end.

While here, tidy the second pass a bit by trimming some
not-quite-right logic copied from amd64. In particular, compute pages
directly at the start of the loop iteration to more closely match the
first loop. Then, drop an always-false condition as 'end' was
computed as 'start + pages' where 'start == highest + 1'. Thus, the
actual condition being tested was 'if (highest >= highest + 1 +
pages)'.

Reported by: CHERI (overflow)
Sponsored by: DARPA

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jhb requested review of this revision.Feb 3 2023, 7:04 PM
sys/arm64/arm64/pmap.c
1377

This was already dead and remains dead...

1383–1384

Why do we even need highest? Just initialise start and do start += s / sizeof(*pvd) at the end of the loop body (maybe even with the i = 0 and i++)? Makes it look more like a normal for loop...

sys/arm64/arm64/pmap.c
1383–1384

Actually, I think I can just assign pvd to pv_table before the loop and remove start and highest entirely. amd64 still needs much of this as it deals with an edge condition for a superpage spanning a segment boundary that arm64 doesn't try to handle.

sys/arm64/arm64/pmap.c
1411

I worry, btw, that by not doing the 'roundup' by pages in this loop here, we can end up using backing store from the wrong domain. One naive fix would be to merge these loops and set seg->md_first to the current pvd in the loop above, but you'd still need to think about how to handle the weird edge case below. Presumably the edge case below "can't" happen across a NUMA boundary in practice?

Further simplify the main loop.

markj added inline comments.
sys/arm64/arm64/pmap.c
1411

I think you're right.

Alternately, we could check for the case where the previous segment belongs to a different domain and the current segment is 2MB aligned (or at least doesn't overlap with the previous segment), and align pvd to PAGE_SIZE if so.

This revision is now accepted and ready to land.Feb 10 2023, 5:13 PM