casaroli commented on issue #1329: URL: https://github.com/apache/nuttx/issues/1329#issuecomment-6272780607
I reviewed and tested the four open PRs of @royzah that address this issue, and I want to keep the points that concern all of them in one place. | PR | What it does | State | |---|---|---| | #20412 | Helpers that refuse kernel addresses from user space in a kernel build (`uaccess_ok()`, `uaccess_check()`, `uaccess_fault()`), page-table walkers for riscv and x86_64 | Open; nothing calls the checks yet, the others use them | | #20418 | Copies in the ioctls and calls that carry user pointers, and refuses kernel pointers | Draft, on top of #20412 | | #20415 | Keeps user semaphores in the kernel | Draft, on top of #20412 | | #20425 | Signalling or rescheduling another process needs `PR_CAP_ADMIN` | Draft; independent of #20412 (#20417 is merged) | All four rebase cleanly on master (c7cba680a10), build, and pass ostest on `qemu-armv8a:knsh` and `:nsh`. My reviews: https://github.com/apache/nuttx/pull/20412#pullrequestreview-5575023201, https://github.com/apache/nuttx/pull/20418#pullrequestreview-5575023672, https://github.com/apache/nuttx/pull/20415#pullrequestreview-5575024174, https://github.com/apache/nuttx/pull/20425#pullrequestreview-5575024560. **Merge order.** #20412 first: it is the base and it is closest to merge. Then #20418 and #20415, each as one commit on top of it. #20425 can go at any time. **Open design point.** #20418 (and #20415) still generate per-call checks in `tools/mksyscall.c`, which xiaoxiang asked to keep out of the general stub/proxy creator in the #20412 review. #20425 puts its checks into the functions instead. Can we agree on one place before the next round? **Gap that none of the four closes yet.** The checks accept any address inside the user window, also an address that is not mapped. Then the kernel faults on it at EL1, and on arm64 a kernel fault is a panic. So any program in a kernel build can stop the system: ```c read(fd, (void *)0xF0000000, 1); /* in the user window, not mapped */ ``` On master, and with all four PRs, this panics the kernel (`arm64_fatal.c:696`). A kernel pointer (`0x1000`) is refused with #20418, which is good. To close the gap, the kernel must recover from a fault on a user address during a system call: either a fixup table for the user-access code (as `copy_from_user()` does on Linux), or a rule that a fault at EL1 on a user address in a system call ends the caller instead of the system. **Small, older point.** `ARCH_ADDRENV_VEND` and `ARCH_SHM_VEND` are inclusive, but `arm64_uservaddr()`, `riscv_uservaddr()`, the armv7-a copy and the new x86_64 `up_addrenv_user_vaddr()` test `vaddr < VEND`, so the last byte of each window counts as "not user". It is older than these PRs; a small separate cleanup would fix it. I am happy to test new versions. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
