lib: os: fdtable: Add syscalls for POSIX read/write/close operations #26117
Conversation
pfalcon
commented
Jun 11, 2020
pfalcon
commented
pfalcon
requested review from
andrewboie,
andyross,
jukkar and
tbursztyka
as code owners
zephyrbot
added
area: Networking
area: API
| #ifdef CONFIG_USERSPACE | ||
| ssize_t z_vrfy_sys_write(int fd, const void *buf, size_t sz) | ||
| { | ||
| Z_OOPS(Z_SYSCALL_MEMORY_READ(buf, sz)); |
There was a problem hiding this comment.
Don't we need to make a copy of the buf data here?
There was a problem hiding this comment.
Just looking the existing calls in sockets.c, but indeed we do not copy the data buffer there either.
There was a problem hiding this comment.
We need to copy only stuff which may lead to security issues. E.g., if some memory block contains pointers, we need to copy those pointers (not necessarily the entire memory block!), then validate the copies, then let the kernel use copies for dereferencing.
(Yeah, yeah, someone looking for side-channel (e.g., timing) attacks hardening may copy more stuff, e.g. control-dependent data. Good luck with that to whoever will do that.)
There was a problem hiding this comment.
The contents of buf are really application dependent, and generally are not used by the kernel, unless it is a kernel type of socket / block / char device.
Validating the application has access to the memory should be sufficient.
Fdtable is a kernel-level structure, so functions accessing it, like read(), write(), close(), should be syscalls. This conversion however emphasizes a difference between "syscall" and "C standard library function". C stdlib functions should be represented by just a standard linkage symbol, and can be used even without prototype. Syscall is however a special code sequence, which may require special attributes and thus a prototype for usage. To deal with these distinction, we define a convention that "sys_name" is a syscall, while "name" is a normal C function (implemented as a wrapper around syscall). (And we also need to define "_name" to interface with Newlib (actually, just "_write".)) Fixes: zephyrproject-rtos#25407 Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
With POSIX subsystem enabled (CONFIG_POSIX_API=y), it's possible to use generic POSIX read()/write() calls on a socket file descriptor. This test verifies their operaration on this case. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
pfalcon
force-pushed
the
read-write-syscalls-only
branch
from
6063242 to
e089e26
Compare
|
This is split off from #25443, for read/write/close syscalls which didn't lead to controversy and need for further work. Happy to see it pass CI. |
Yes, this is addition to the POSIX subsystem, which follows POSIX semantics. This also follows your suggestion to split off parts not related to ioctl into a separate PR. |
| @@ -0,0 +1,9 @@ | |||
| # SPDX-License-Identifier: Apache-2.0 | |||
There was a problem hiding this comment.
Awww... why didn't you use socketpair(2)? ... jk. ;-)
From what I understand, it is unfortunately a pre-existing condition. I agree though, it's an issue that should be addressed, but maybe in a separate ticket. |
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
