derek/gem5
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

arch-arm: avoid using an uninitialized variable use in MMU walks (#1198)

Browse Source 
While running a simple Arm32 binary, I noticed that all memory
transactions were being marked as NS instead of S once I turn on the MMU
(even though the page tables have the NS bit set to zero). The result of
this was that semihosting calls were failing since they were using
functional accesses with the SECURE flag set, but the caches only
contained NS tagged entries so these accesses always read stale values
from DRAM.

Digging through the Arm MMU code it appears that the NS bit lookup was
being keyed of the `secureLookup` flag which is only used for long
descriptors. I believe 0c28712f51 should
have used isSecure instead of secureLookup. To avoid using these
uninitialized values in the future I wrapped the LPAE state in a
std::optional to ensure that it is only accessed once initialized.

Change-Id: Ibc406ed3f4cfa768f470e34a5eca3c1a2bf45cd8
This commit is contained in:
Alexander Richardson
2024-06-07 00:59:28 -07:00
committed by GitHub
parent 8e5fbcbbbb
commit ec5881ec4e
2 changed files with 49 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

64
src/arch/arm/table_walker.cc
View File

@@ -132,9 +132,8 @@ TableWalker::WalkerState::WalkerState() :
sctlr(0), scr(0), cpsr(0), tcr(0),
htcr(0), hcr(0), vtcr(0),
isWrite(false), isFetch(false), isSecure(false),
isUncacheable(false),
secureLookup(false), rwTable(false), userTable(false), xnTable(false),
pxnTable(false), hpd(false), sh(0), irgn(0), orgn(0), stage2Req(false),
isUncacheable(false), longDescData(std::nullopt),
hpd(false), sh(0), irgn(0), orgn(0), stage2Req(false),
stage2Tran(nullptr), timing(false), functional(false),
mode(BaseMMU::Read), tranType(MMU::NormalTran), l2Desc(l1Desc),
delayed(false), tableWalker(nullptr)
@@ -366,6 +365,7 @@ TableWalker::walk(const RequestPtr &_req, ThreadContext *_tc, uint16_t _asid,
currState->mode = _mode;
currState->tranType = tranType;
currState->isSecure = secure;
currState->secureLookup = secure;
currState->physAddrRange = _physAddrRange;
/** @todo These should be cached or grabbed from cached copies in
@@ -433,14 +433,14 @@ TableWalker::walk(const RequestPtr &_req, ThreadContext *_tc, uint16_t _asid,
if (long_desc_format) {
// Helper variables used for hierarchical permissions
currState->secureLookup = currState->isSecure;
currState->rwTable = true;
currState->userTable = true;
currState->xnTable = false;
currState->pxnTable = false;
currState->longDescData = WalkerState::LongDescData();
currState->longDescData->rwTable = true;
currState->longDescData->userTable = true;
currState->longDescData->xnTable = false;
currState->longDescData->pxnTable = false;
++stats.walksLongDescriptor;
} else {
currState->longDescData = std::nullopt;
++stats.walksShortDescriptor;
}
@@ -677,7 +677,7 @@ TableWalker::processWalk()
flag.set(Request::UNCACHEABLE);
}
if (currState->isSecure) {
if (currState->secureLookup) {
flag.set(Request::SECURE);
}
@@ -703,7 +703,7 @@ TableWalker::processWalkLPAE()
stats.walkWaitTime.sample(curTick() - currState->startTime);
Request::Flags flag = Request::PT_WALK;
if (currState->isSecure)
if (currState->secureLookup)
flag.set(Request::SECURE);
// work out which base address register to use, if in hyp mode we always
@@ -1079,7 +1079,7 @@ TableWalker::processWalkAArch64()
flag.set(Request::UNCACHEABLE);
}
if (currState->isSecure) {
if (currState->secureLookup) {
flag.set(Request::SECURE);
}
@@ -1112,10 +1112,12 @@ TableWalker::walkAddresses(Addr ttbr, GrainSize tg, int tsz, int pa_range)
"Walk Cache hit: va=%#x, level=%d, table address=%#x\n",
currState->vaddr, entry->lookupLevel, entry->pfn);
currState->xnTable = entry->xn;
currState->pxnTable = entry->pxn;
currState->rwTable = bits(entry->ap, 1);
currState->userTable = bits(entry->ap, 0);
if (currState->longDescData.has_value()) {
currState->longDescData->xnTable = entry->xn;
currState->longDescData->pxnTable = entry->pxn;
currState->longDescData->rwTable = bits(entry->ap, 1);
currState->longDescData->userTable = bits(entry->ap, 0);
}
table_addr = entry->pfn;
first_level = (LookupLevel)(entry->lookupLevel + 1);
@@ -1668,7 +1670,7 @@ TableWalker::doL1Descriptor()
flag.set(Request::UNCACHEABLE);
}
if (currState->isSecure)
if (currState->secureLookup)
flag.set(Request::SECURE);
fetchDescriptor(
@@ -1786,13 +1788,17 @@ TableWalker::doLongDescriptor()
// Set hierarchical permission flags
currState->secureLookup = currState->secureLookup &&
currState->longDesc.secureTable();
currState->rwTable = currState->rwTable &&
currState->longDescData->rwTable =
currState->longDescData->rwTable &&
(currState->longDesc.rwTable() || currState->hpd);
currState->userTable = currState->userTable &&
currState->longDescData->userTable =
currState->longDescData->userTable &&
(currState->longDesc.userTable() || currState->hpd);
currState->xnTable = currState->xnTable ||
currState->longDescData->xnTable =
currState->longDescData->xnTable ||
(currState->longDesc.xnTable() && !currState->hpd);
currState->pxnTable = currState->pxnTable ||
currState->longDescData->pxnTable =
currState->longDescData->pxnTable ||
(currState->longDesc.pxnTable() && !currState->hpd);
// Set up next level lookup
@@ -2221,9 +2227,10 @@ TableWalker::insertPartialTableEntry(LongDescriptor &descriptor)
te.regime = currState->regime;
te.xn = currState->xnTable;
te.pxn = currState->pxnTable;
te.ap = (currState->rwTable << 1) | (currState->userTable);
te.xn = currState->longDescData->xnTable;
te.pxn = currState->longDescData->pxnTable;
te.ap = (currState->longDescData->rwTable << 1) |
(currState->longDescData->userTable);
memAttrsWalkAArch64(te);
@@ -2279,15 +2286,16 @@ TableWalker::insertTableEntry(DescriptorBase &descriptor, bool long_descriptor)
dynamic_cast<LongDescriptor &>(descriptor);
te.tg = l_descriptor.grainSize;
te.xn |= currState->xnTable;
te.pxn = currState->pxnTable || l_descriptor.pxn();
te.xn |= currState->longDescData->xnTable;
te.pxn = currState->longDescData->pxnTable || l_descriptor.pxn();
if (isStage2) {
// this is actually the HAP field, but its stored in the same bit
// possitions as the AP field in a stage 1 translation.
te.hap = l_descriptor.ap();
} else {
te.ap = ((!currState->rwTable || descriptor.ap() >> 1) << 1) |
(currState->userTable && (descriptor.ap() & 0x1));
te.ap = ((!currState->longDescData->rwTable ||
descriptor.ap() >> 1) << 1) |
(currState->longDescData->userTable && (descriptor.ap() & 0x1));
}
if (currState->aarch64)
memAttrsAArch64(currState->tc, te, l_descriptor);

19
src/arch/arm/table_walker.hh
View File

@@ -890,17 +890,24 @@ class TableWalker : public ClockedObject
/** If the access comes from the secure state. */
bool isSecure;
/** Whether lookups should be treated as using the secure state.
* This is usually the same as isSecure, but can be set to false by the
* long descriptor table attributes. */
bool secureLookup = false;
/** True if table walks are uncacheable (for table descriptors) */
bool isUncacheable;
/** Helper variables used to implement hierarchical access permissions
* when the long-desc. format is used (LPAE only) */
bool secureLookup;
bool rwTable;
bool userTable;
bool xnTable;
bool pxnTable;
* when the long-desc. format is used. */
struct LongDescData
{
bool rwTable = false;
bool userTable = false;
bool xnTable = false;
bool pxnTable = false;
};
std::optional<LongDescData> longDescData;
/** Hierarchical access permission disable */
bool hpd;