New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 782244 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

ec: Fix CONFIG_MPU for non-power-2 data RAM size and un-memmapped flash

Project Member Reported by sha...@chromium.org, Nov 7 2017

Issue description

mpu_protect_ram() currently doesn't work unless data RAM size is a power of 2, so let's fix that.

Also mpu_lock_*_flash() should probably be skipped when our flash storage is not memory mapped (eg. on mec1322).
 
Cc: -sha...@chromium.org
Owner: sha...@chromium.org
> mpu_lock_*_flash() should probably be skipped when our flash storage is not memory mapped

This needs to be fixed certainly, skipped maybe not, don't we want the code RAM to be RO for the MPU ? is this already the case ?
> don't we want the code RAM to be RO for the MPU ?

I don't understand, can you explain more? Currently mpu_lock_*_flash() prevents execution from code memory in the non-active region. But for non-memmapped parts, there is no separate code memory for RO vs RW, there is only a single region of code memory that is loaded with either RO or RW. So that's why I suggested that this protection is not relevant for non-memmapped design.
> for non-memmapped parts, there is no separate code memory for RO vs RW, 
> there is only a single region of code memory that is loaded with either RO
>  or RW. So that's why I suggested that this protection is not relevant 
> for non-memmapped design.

Yes, my confusing remark was: while this protection is irrelevant for chips which are not doing eXecute-In-Place, those chips might need another protection for the code section (the one currently in-use) : can we ensure the code in RAM cannot be modified (by hostile exploit) ie set this code RAM to read-only (which is not needed on XiP chips since flash is kind-of read-only by default as it has another WP mechanism) ?
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/b6991dd96d8bf6cb86a39b3da590ccd8b4e1e036

commit b6991dd96d8bf6cb86a39b3da590ccd8b4e1e036
Author: Shawn Nematbakhsh <shawnn@chromium.org>
Date: Tue Nov 14 18:11:18 2017

cortex-m: mpu: Support unaligned regions and protect code RAM

Support protection of regions that aren't aligned to a power of 2 by
using two MPU entries, and taking advantage of the sub-region feature.
Also protect code RAM from being overwritten, on parts that use external
storage.

BUG= chromium:782244 
BRANCH=None
TEST=On kevin, call:
mpu_protect_data_ram();
mpu_protect_code_ram();
mpu_enable();
Verify that first call results in the following update_region params:
addr: 0x200c2000 size: 0xc01d
Decoded: Protect 24K region
Verify that second call results in the following params:
addr: 0x100a8000 size: 0xc021
Decoded: Protect 96K region
addr: 0x100c0000 size: 0xf01b
Decoded: Protect remaining 8K region
Also verify that writes to beginning and end of code ram region trigger
data access violation after enabling protection.
Also verify that sysjump fails.

Change-Id: Ieb7a4ec3a089e8a2d29f231e1e3acf2e78e560a1
Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/757721
Commit-Ready: Shawn N <shawnn@chromium.org>
Tested-by: Shawn N <shawnn@chromium.org>
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/b6991dd96d8bf6cb86a39b3da590ccd8b4e1e036/core/cortex-m/include/mpu.h
[modify] https://crrev.com/b6991dd96d8bf6cb86a39b3da590ccd8b4e1e036/board/kevin/board.h
[modify] https://crrev.com/b6991dd96d8bf6cb86a39b3da590ccd8b4e1e036/common/system.c
[modify] https://crrev.com/b6991dd96d8bf6cb86a39b3da590ccd8b4e1e036/core/cortex-m/mpu.c

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/68bd2d4fb278781d1ff7c40f42f43e8e445d89ac

commit 68bd2d4fb278781d1ff7c40f42f43e8e445d89ac
Author: Shawn Nematbakhsh <shawnn@chromium.org>
Date: Tue Nov 14 18:11:19 2017

npcx: Use compatible MPU config

MPU is already configured for access restriction in cortex-m core code
so take care not to conflict.

BUG= chromium:782244 
BRANCH=None
TEST=Build + boot on kevin, verify hibernate doesn't panic.

Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org>
Change-Id: I9903cbc69002529ebbfa3fc1be3de4f74264e4aa
Reviewed-on: https://chromium-review.googlesource.com/759157
Commit-Ready: Shawn N <shawnn@chromium.org>
Tested-by: Shawn N <shawnn@chromium.org>
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/68bd2d4fb278781d1ff7c40f42f43e8e445d89ac/chip/npcx/system-npcx5.c
[modify] https://crrev.com/68bd2d4fb278781d1ff7c40f42f43e8e445d89ac/chip/npcx/system-npcx7.c

Status: Verified (was: Untriaged)
Project Member

Comment 8 by bugdroid1@chromium.org, Feb 12 2018

Labels: merge-merged-firmware-eve-9584.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/d4fb16ed8f49f54fbd62084d7b02e0c57a4eb5b1

commit d4fb16ed8f49f54fbd62084d7b02e0c57a4eb5b1
Author: Shawn Nematbakhsh <shawnn@chromium.org>
Date: Mon Feb 12 19:49:05 2018

cortex-m: mpu: Support unaligned regions and protect code RAM

Support protection of regions that aren't aligned to a power of 2 by
using two MPU entries, and taking advantage of the sub-region feature.
Also protect code RAM from being overwritten, on parts that use external
storage.

BUG= chromium:782244 
BRANCH=None
TEST=On kevin, call:
mpu_protect_data_ram();
mpu_protect_code_ram();
mpu_enable();
Verify that first call results in the following update_region params:
addr: 0x200c2000 size: 0xc01d
Decoded: Protect 24K region
Verify that second call results in the following params:
addr: 0x100a8000 size: 0xc021
Decoded: Protect 96K region
addr: 0x100c0000 size: 0xf01b
Decoded: Protect remaining 8K region
Also verify that writes to beginning and end of code ram region trigger
data access violation after enabling protection.
Also verify that sysjump fails.

Change-Id: I57d9b05b7f34eabb4e7126b2a0d45f0881905621
Signed-off-by: Duncan Laurie <dlaurie@google.com>
Original-Commit-Id: b6991dd96d8bf6cb86a39b3da590ccd8b4e1e036
Original-Change-Id: Ieb7a4ec3a089e8a2d29f231e1e3acf2e78e560a1
Original-Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org>
Original-Reviewed-on: https://chromium-review.googlesource.com/757721
Original-Commit-Ready: Shawn N <shawnn@chromium.org>
Original-Tested-by: Shawn N <shawnn@chromium.org>
Original-Reviewed-by: Vincent Palatin <vpalatin@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/914631

[modify] https://crrev.com/d4fb16ed8f49f54fbd62084d7b02e0c57a4eb5b1/core/cortex-m/include/mpu.h
[modify] https://crrev.com/d4fb16ed8f49f54fbd62084d7b02e0c57a4eb5b1/board/kevin/board.h
[modify] https://crrev.com/d4fb16ed8f49f54fbd62084d7b02e0c57a4eb5b1/common/system.c
[modify] https://crrev.com/d4fb16ed8f49f54fbd62084d7b02e0c57a4eb5b1/core/cortex-m/mpu.c

Project Member

Comment 9 by bugdroid1@chromium.org, Feb 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/172e1e6339bb28334d475c710b47606dfba5a521

commit 172e1e6339bb28334d475c710b47606dfba5a521
Author: Shawn Nematbakhsh <shawnn@chromium.org>
Date: Mon Feb 12 19:49:06 2018

npcx: Use compatible MPU config

MPU is already configured for access restriction in cortex-m core code
so take care not to conflict.

BUG= chromium:782244 
BRANCH=None
TEST=Build + boot on kevin, verify hibernate doesn't panic.

Change-Id: I42448338dc44afa453dc7990b0fc25828d4a0f85
Signed-off-by: Duncan Laurie <dlaurie@google.com>
Original-Commit-Id: 68bd2d4fb278781d1ff7c40f42f43e8e445d89ac
Original-Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org>
Original-Change-Id: I9903cbc69002529ebbfa3fc1be3de4f74264e4aa
Original-Reviewed-on: https://chromium-review.googlesource.com/759157
Original-Commit-Ready: Shawn N <shawnn@chromium.org>
Original-Tested-by: Shawn N <shawnn@chromium.org>
Original-Reviewed-by: Vincent Palatin <vpalatin@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/914632

[modify] https://crrev.com/172e1e6339bb28334d475c710b47606dfba5a521/chip/npcx/system-npcx5.c
[modify] https://crrev.com/172e1e6339bb28334d475c710b47606dfba5a521/chip/npcx/system-npcx7.c

Sign in to add a comment