arm: nxp: mpu: Fix Coverity issue #441
Conversation
The NXP MPU requires special handling of region descriptor 0 to guarantee that the debugger has access to the entire address space. It does not allow writes from the core to affect the start or end addresses, or the permissions associated with the debugger. The original implementation of this driver attempted to work around region descriptor 0, resulting in an off-by-1 error caught by Coverity. Instead, define region descriptor 0 explicitly in the mpu_regions array, and add some asserts to ensure that one doesn't try to change its start or end addresses. This has an added benefit such that more permissions can be enabled in region 0 if desired, whereas the previous implementation always forced all writable permissions to be cleared. Coverity-CID: 170473 Jira: ZEP-2258 Signed-off-by: Maureen Helm <[email protected]>
Both the ARM and NXP MPU drivers incorrectly calculated the region index by assuming the region type (e.g., THREAD_STACK_GUARD_REGION) was zero-indexed, when in reality it is one-indexed. This had the effect of wasting one region. Signed-off-by: Maureen Helm <[email protected]>
MaureenHelm
requested review from
nashif,
andrewboie,
fvincenzo,
carlescufi,
galak and
agross-oss
|
After 07e0a78, attempting to execute from RAM no longer triggers a MPU-caused bus fault on frdm_k64f, as can be seen from mpu_test run - you'll get a usage fault / invalid use of the EPSR instead of a bus fault. Also my own tests of stack/heap/data execution all successfully execute code from those regions without causing any fault at all after this change; those test actual execution of code written to RAM. Looking at the MPU configuration before and after this change, the only difference seems to be that previously region 0 was disabled via a direct write to RGD0_WORD2, causing the valid mask to also be cleared, whereas now you are configuring it through RGDAAC0 and therefore leaving it marked valid. Otherwise, before and after the change, the region attributes seem to be identical (0x7c0). I would have thought that would be ok since you are clearing the non-debugger permissions, but for some reason it doesn't seem to be working AFAICS as long as the valid mask remains set. |
|
FWIW, my tests are here: |
|
Thanks for catching this. It turns out that clearing certain bits in WORD2/RGDAAC0 actually enables access. Specifically, the MxSM register fields: Bus Master 3 Supervisor Mode Access Control I've got a fix I'm testing now. |
|
@stephensmalley, please have a look at #484. I like your test and think it's worth submitting, perhaps merge the new functionality into the existing mpu_test. |
|
Submitted as #493 |
…ct-rtos#441) Signed-off-by: Geoff Gustafson <[email protected]>
Fixes a Coverity issue in the NXP MPU driver, as well as a second similar issue in the NXP and ARM MPU drivers found by inspection.
Neither of these issues are causing any actual runtime failures, and given we are so close to releasing 1.8, I suggest we do not include this in the release. I don't want any surprises like #261.