casaroli opened a new pull request, #20822:
URL: https://github.com/apache/nuttx/pull/20822
## Summary
In a kernel build a system call uses the user pointers that it gets as they
are. A pointer can be in the user address window but have no page behind it,
for example `read(fd, (void *)0xf0000000, 1)` on qemu-armv8a:knsh. The kernel
then takes a data abort at EL1 when it touches the pointer, and
`arm64_fatal_handler()` panics, because only a fault at EL0 ends a task. So one
bad pointer from any process stops the whole system.
### arch/arm64: End the caller on a bad user address in a system call.
The handler treats such an abort as a fault of the caller when all of these
are true:
- the abort is a data abort at EL1 with a valid FAR, and the fault is a
translation, access flag or permission fault;
- the CPU is not in interrupt context;
- the task is a user task in a system call (`TCB_FLAG_SYSCALL`), and it is
not already exiting;
- the address environment of the caller is active (no `addrenv_select()` of
another process);
- FAR is a user address (`up_addrenv_user_vaddr()`).
Then the caller ends with `_exit(SIGSEGV)`, the same as for a fault at EL0,
and the handler prints the usual "Segmentation fault in ..." report with the
bad address. There are two ways:
- The fault is in the arm64 `memcpy()` or `memset()`. Both are leaf
functions, so the handler returns from the function to its caller, and the
system call finishes and releases its locks, file references and memory. If
`memcpy()` faulted on a read of the user source, the handler clears the kernel
destination first, so that old kernel data does not go on in place of the user
data. When the system call returns, `dispatch_syscall()` ends the caller. Most
drivers and file systems copy the data of `read()` and `write()` with these two
functions.
- The fault is in other code. The handler ends the caller at once. It moves
the exception frame to the top of the kernel stack, so that `_exit()` does not
run below the frames of the system call, and it clears `TCB_FLAG_SYSCALL`, so
that a second fault in the exit path still panics.
In all other cases the handler panics as before. A fault on a kernel address
still panics: that is a kernel bug or a missing pointer check.
### fs/vfs: Fill a local stat buffer and copy it out at the end.
`stat()`, `lstat()`, `fstat()`, `statfs()` and `fstatfs()` wrote the
caller's buffer field by field while they held an inode reference, the
temporary path buffer of `inode_search_setup()` or a file reference. If the
system call ends at one of these writes, these are not released: each `stat()`
with a bad buffer lost a path buffer (256 bytes of kernel heap once the two
static buffers are gone) and an inode reference. Now these functions fill a
buffer on the kernel stack and copy it to the caller's buffer with one
`memcpy()` after the release. The caller's buffer changes only on success.
Cost: 80 bytes of stack in `nx_stat()` and `nx_fstat()`, 48 bytes in `statfs()`
and `fstatfs()` (arm64, `-fstack-usage`).
### fs/vfs/fs_ioctl.c: Add blank lines after declarations.
Whitespace only. checkpatch checks the whole file, and it had three older
nxstyle errors.
### fs/vfs: Read the FIONBIO argument before the file reference is taken.
`file_vioctl()` read the `int` of `ioctl(fd, FIONBIO, &value)` after the
driver ioctl, while `nx_vioctl()` held a reference to the file. If the system
call ends at that read, the reference is not released, and the `struct file`
and its inode reference are lost when the last descriptor closes. Now
`nx_vioctl()` reads the `int` before `file_get2()` and gives the driver and
`file_vioctl()` a pointer to the kernel copy. Kernel callers of `file_ioctl()`
do not change.
This is separate from #20418 ("copy in ioctls that carry user pointers"):
#20418 copies in nested ioctl structures in the system call wrapper and refuses
kernel addresses, but it passes an unmapped user address on to `ioctl()` and
does not read FIONBIO. This commit changes only `nx_vioctl()`, which #20418
does not touch.
## Impact
Only arm64 with `CONFIG_BUILD_KERNEL` for the fault handling. Flat and
protected builds, other architectures and faults on kernel addresses do not
change: they panic as before. The two VFS commits are generic: the stat family
and FIONBIO behave the same, except that the stat buffer is no longer changed
when the call fails.
A process that gives a system call a user address with no page (or a
read-only page for a write) now ends with exit status 11, and the system
continues. Before, the system stopped.
Limits:
- In `memcpy()`/`memset()` the system call finishes with partial data: a
copy from a bad user buffer gives zeros (a faulting pipe write puts zero bytes
into the pipe). The caller ends only when the system call returns.
- In other code the system call does not finish, so its cleanup code does
not run. A plain mutex recovers, because `nxsem_wait_slow()` gives a mutex
whose holder has exited to the next caller. A recursive mutex keeps the count
of the dead holder, a semaphore used as a lock stays taken, and a file
reference or a kernel allocation of the system call is not released. Paths that
still touch user memory in C with something held include `FIOGCLEX`,
`FIOC_GETLK`/`FIOC_SETLK`, driver `ioctl()` methods and `readdir()`.
- In other code `_exit()` runs on the kernel stack of the task, so the
frames of the system call are overwritten. An object on that stack that the
system call linked into a kernel list is lost.
- `arm64_syscall()` copies `siginfo_t` to the user stack for
`SYS_signal_handler` with the interrupt flag set. A fault there still panics.
- The caller gets no `EFAULT` and no real signal. Like a fault at EL0 today,
the task ends with `_exit(SIGSEGV)`, so the parent sees exit status 11.
A return of `EFAULT` on every path needs user-copy helpers with a fault
fixup for each access to user memory. NuttX drivers and file systems use user
pointers directly, so that is a larger change for later. Until then this change
is the fallback for all paths.
## Testing
qemu-armv8a:knsh with `CONFIG_PIPES=y` and `CONFIG_SCHED_CHILD_STATUS=y`,
under HVF, on master 05d97546995. A test program forks a child for each case.
Each child gives one system call a bad user pointer: `read()` into an unmapped
data address and into an unmapped shared memory address, `write()` to a pipe,
the `stat()` path and buffer, the `ioctl(FIONBIO)` argument, `nanosleep()`,
`clock_gettime()`, `sigprocmask()`, `read()` into the program text (a
permission fault), and a `read()` that starts in a mapped shared memory page
and runs into the next, unmapped page.
- master: kernel panic on the first case (`Assertion failed panic: at file:
common/arm64_fatal.c:696`). The system stops.
- this branch: each child ends with exit status 11, and NSH continues. 11 of
11 cases in one pass, then 220 of 220 in 20 passes.
- this branch: a child writes from a bad pointer to a pipe (the fault comes
in `memcpy()` with the pipe mutex held). A second child then writes 1 byte to
the same pipe: the write returns 1, and the second child ends at once.
Kernel heap growth (`free`, Kmem used) over 20 faulting children of one
case, after one warm-up child:
| Case | arm64 commit only | all four commits |
| --- | --- | --- |
| `read()` cases (4), pipe `write()`, `stat()` path, `nanosleep()`,
`clock_gettime()`, `sigprocmask()` | 0 | 0 |
| `stat()` buffer | +4896 bytes, 18 allocations | 0 |
| `ioctl(FIONBIO)` argument | +48 bytes, 1 allocation (one per open file) |
0 |
| `fstat()`, `statfs()`, `fstatfs()` buffer | not run | 0 |
| child calls `_exit(11)` (control) | 0 | 0 |
| child writes the bad address at EL0 (control) | 0 | 0 |
The two controls also give 0 on master. Over the whole test program (231
faulting children) Kmem grows by 392 bytes once; a run with 338 children grows
by the same 392 bytes.
ostest: exit status 0 on qemu-armv8a:knsh and qemu-armv8a:nsh.
rv-virt:knsh64 builds.
--
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]
