casaroli opened a new pull request, #21473: URL: https://github.com/apache/nuttx/pull/21473
## Summary `mkfifo()`, the target of `mount()`, the new path of `rename()`, and the device name of the loop ioctls `LOOPIOC_SETUP` and `MTD_LOOPIOC_SETUP` read a path for the first time with the inode tree write lock held. In a kernel build this path is user memory. If that read ends the caller, the lock stays held, and every later user of the inode tree blocks. The lock is an rwsem, and nothing gives it to the next task. This PR reads each path before the lock: - `mount()` and the pseudo file system path of `rename()` call `inode_search_setup()` before `inode_lock()`. It works on the string only and does not look at the inode tree, so resolve, permission checks and change in `rename()` all stay under the lock, and symbolic links still cannot be swapped between check and use. - `nx_mkfifo()`, `losetup()` and `mtd_loop_setup()` call `strnlen(path, PATH_MAX)`, before they allocate anything. This reads every byte that `inode_search_setup()` reads later. ## Impact **On master alone this PR shows no change that a program can see.** On master a bad user pointer in a system call panics with or without this PR. The change takes effect only with a change that ends the caller on such a fault, such as #20822. With #20822 and without this PR, one bad path to `mkfifo()` blocks the file system for all processes. Behaviour with good paths does not change. One error can change: `LOOPIOC_SETUP` and `MTD_LOOPIOC_SETUP` with an empty device name now return `EINVAL` before they look at the file, so a missing file no longer gives another error first. Generic code: flat, protected and kernel builds, every architecture. No ABI change. Not a breaking change. Flash cost: 0 bytes of image text on `qemu-armv8a:knsh`, `mps2-an500:nsh` and `rv-virt:nsh`. With `CONFIG_PIPES` and `CONFIG_DEV_LOOP` on `nsh`: +40 bytes on arm64, +40 on Cortex-M, +28 on RISC-V. Limits: - `mkdir()`, `rmdir()`, `unlink()`, `link()` and `symlink()` read the path once with no lock, and again under the lock. So do `mkfifo()`, `mount()` and `rename()` after this PR. A second thread that unmaps the page between the two reads is not covered. A copy of the path would cover it, but that is a larger change. - With `CONFIG_DISABLE_PSEUDOFS_OPERATIONS`, `mount()` does not call `inode_search_setup()`, so its target is still first read under the lock. ## Testing Test program `uaccess-locks2.c` on qemu-armv8a:knsh (QEMU, HVF), master 6e1126a2362, with the arm64 commit of #20822, one boot per case. A child gives the call a bad path. A second process then takes the inode tree lock for reading and for writing. "Cross" is a path that runs off the end of a mapped page into an unmapped page. | Call | without this PR | with this PR | |---|---|---| | `mkfifo()`, bad and cross | the second process blocks | runs | | `mount()` target, bad and cross | the second process blocks | runs | | `rename()` new path, cross | the second process blocks | runs | | `LOOPIOC_SETUP` device name, bad | the second process blocks | runs | | `MTD_LOOPIOC_SETUP` device name, bad | the second process blocks | runs | | `mkdir()`, `rmdir()`, `unlink()`, `link()`, `symlink()`, `shm_open()`, `shm_unlink()`, `sem_open()`, `sem_unlink()`, `mq_open()`, `mq_unlink()`, `rename()` old path, bad | runs | runs | In each row the child ends with exit status 11. Test program `pathops.c` with good paths: `mkfifo()`, `mount()` and `rename()` with new names, existing names, relative paths and paths of about 200 bytes; `rename()` across directories, over an existing target, into a directory, onto itself and below itself, in the pseudo file system and on tmpfs; and the expected errors. The output is the same on master and with this PR, on qemu-armv8a:knsh and qemu-armv8a:nsh: 36 checks PASS. ostest exits with status 0 on qemu-armv8a:knsh and qemu-armv8a:nsh. Builds with `EXTRAFLAGS="-Wno-cpp -Werror"`, each OK: qemu-armv8a:knsh, qemu-armv8a:nsh, fvp-armv8r:pnsh (arm64 kernel, flat, protected), rv-virt:knsh64, rv-virt:pnsh64 (rv64 kernel, protected), mps2-an500:knsh, lm3s6965-ek:qemu-protected (Cortex-M protected), sim:nsh; qemu-armv8a:nsh with `CONFIG_DEV_LOOP`, with `CONFIG_FILEMTD` and `CONFIG_MTD_LOOP`, and with `CONFIG_DISABLE_PSEUDOFS_OPERATIONS`. Documentation: `10_filesystem.rst` describes these functions but not their locks. No change needed. `tools/checkpatch.sh -c -u -m -g` passes. -- 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]
