os/board/bk7239n/: Change memory access permission in MPU#7290
os/board/bk7239n/: Change memory access permission in MPU#7290namanjain7 wants to merge 1 commit intoSamsung:masterfrom
Conversation
ewoodev
left a comment
ewoodev
left a comment
There was a problem hiding this comment.
firstly, BSP MPU setting is only PRIV, this loading logic is allow UNPRIV
There was a problem hiding this comment.
Hello @namanjain7 @ewoodev
During internal testing, we found that the Bluetooth app directly uses kernel-space memory. The call path is as follows:
When the BLE server disconnects, ble_server_connected_cb is invoked; with the current Bluetooth architecture, this callback is called directly from the kernel layer.
static ble_server_init_config server_config = {
ble_server_connected_cb,
ble_server_disconnected_cb,
ble_server_mtu_update_cb,
ble_server_passkey_display_cb,
ble_server_pair_bond_cb,
true,
gatt_profile, sizeof(gatt_profile) / sizeof(ble_server_gatt_t)
};
The callback invokes the ble_server_set_adv_data interface. Inside that interface, a local variable is defined: blemgr_msg_s msg = {BLE_CMD_SET_ADV_DATA, BLE_MANAGER_FAIL, (void *)(data), NULL};. That local variable lives in kernel space, yet it is consumed in the BLE message handler task (which belongs to app space). The usage site is:
ble_result_e blemgr_handle_request(blemgr_msg_s *msg) {
trble_result_e ret = TRBLE_FAIL;
blemgr_msg_params queue_msg = {
.evt = msg->event,
.count = 3
};
So we configured the system to allow the app to access kernel space.
There was a problem hiding this comment.
Currently, the structure is not such that BLE requests are directly passed from within the callback called from the Kernel thread.
In the file named ble_response_handler.c, server and client callbacks are wrapped once, so the callback of the application layer is designed to be called by the user thread. (ecode only)
The structure is as follows:
Kernel thread calls response handler callback -> mq enqueue -> user thread mq dequeue -> calls application layer callback
Given this structure, it seems there would be no issues if kernel access is blocked on the user side.
There was a problem hiding this comment.
Hello @giwon-nam The issue we tested is based on the code on GitHub, and tests related to ble_rmc will have this problem. Can you help me double-check it?
There was a problem hiding this comment.
It seems that the issue occurred in the part where the connected callback is called during disconnection on the TP1x Plus.
I will contact Beken and request that during disconnection, only the disconnected callback should be called, and not the connected callback.
There was a problem hiding this comment.
Hi @giwon-nam ,
In the ble_perfs and ble_tester examples, the attribute write callback ble_peri_cb_charact_rmc_sync invokes ble_server_attr_get_data. So this appears to cause the same server disconnection issue, What is the recommended way to avoid this?
There was a problem hiding this comment.
This state is not safetly..
we didn't receive any report issue about it.
you have to open issue this.
we will apply this PR. Please reslove the ble issue ASAP.
There was a problem hiding this comment.
Hello, @lingzhou2018
For now, the ble_server_attr_get_data function you mentioned appears to be functioning without issues.
Currently, on the ecode side, ble_server_attr_get_data is called within the attribute callback invoked by the kernel thread to check what data was written.
It has been confirmed that there are no issues on the ecode side after reflecting the content of the current PR.
If the schedule is urgent, it would be better to merge this PR first and then further analyze why crashes occur in ble_perfs and ble_tester.
PR #7290 — os/board/bk7239n/: Change memory access permission in MPU
Repository: Base → Head: HEAD Commit: Scope: Review the single MPU policy change in Review Summary
Final Verdict: 💬 Comment Must-Fix IssuesNo blocking issues found in static review. Nice-to-Have Improvements1. Add protected-build boot coverage for the tightened MPU windows
Problem Impact
Recommended Action
Example Validation Matrix
Notable Improvements
✔ The patch updates the whole coarse policy set, not just one alias
✔ The PR touches the active bk7239n MPU source file
Final AssessmentMust-Fix Summary
Nice-to-Have Summary
Residual Risk / Test Gap
Final Verdict
The patch is directionally reasonable and I did not find a code-level blocker in the surrounding MPU setup. I am stopping at |
Memory region is accessible only in priviledge mode. Unprivileged code cannot access it at all. This commit protects critical system memory (kernel region) from app memory.
namanjain7
force-pushed
the
mpu_memory_changes
branch
from
bd6ecbb to
cec81a8
Compare
amandeep-samsung
left a comment
amandeep-samsung
left a comment
There was a problem hiding this comment.
MPU PRIV is applied to all defined 7 MPU regions.
7 participants
Memory region is accessible only in priviledge mode. Unprivileged code cannot access it at all. This commit protects critical system memory (kernel region) from app memory.