[PoC] lib: os: fdtable: Add syscalls for POSIX read/write/close/ioctl operations

[pfalcon]

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: #25407

Signed-off-by: Paul Sokolovsky paul.sokolovsky@linaro.org

[pfalcon]

@andrewboie, long-standing technical debt.

Plan of work: conversion for read/write/close is pushed. Will see if CI passes. There're existing socket tests which could be easily (albeit in Do-Repeat-Yourself manner) converted to test read/write. But this test also calls more stuff, like fcntl. That in turn is based on ioctl. And ioctl is used for some kernel-level stuff. So, there's a bit of scope creep, and changes would be exactly "leaf in dependency graph" and "isolated", as there can be potential changes to socket family implementations (or maybe not, didn't look into that yet).

@cfriedt: FYI (and please review once finished, currently it's not, per above).

[cfriedt]

Getting "undefined reference to _write" and "undefined reference to _read" in libc_nano.a . It doesn't jump out at me immediately why that might be the case.
https://app.shippable.com/github/zephyrproject-rtos/zephyr/runs/73259/5/console

[pfalcon]

... But it should explain why this spent so much time in the backlog - dealing with all these naming (ab)uses across different targets is a bit boring (and I'm not particularly interested in kernel/userspace split ;-) ). Anyway, fixing it.

[pfalcon]

Ok, read/write/close syscall are now clean re: build. Will proceed with ioctl tomorrow, then test is the next step after that.

[pfalcon]

Getting "undefined reference to _write" and "undefined reference to _read" in libc_nano.a . It doesn't jump out at me immediately why that might be the case.

For the record, to elaborate what happened there. Prebuilt Newlib, as shipped with Zephyr SDK is call into write to dump some error on stdout/stderr. The issue is that the actual function called is inconsistent among the builds for different architectures. For example, x86 calls write(), while ARM - _write(). My original idea was to make _write() a syscall, while write() a normal userspace func, that due to the above, I fell back to Linux-like convention of sys_write() being a syscall, and write()/_write() userspace funcs.

Thinking about it, ARM newlib build is right here, while x86 is wrong. The whole idea of underscore-prefixed functions is that they don't provide full POSIX semantics. So, if x86 called _write, we wouldn't need to define write() for !CONFIG_POSIX_API case, and @cfriedt would never get confused by calling write() on a socket in this case, and getting output dumped to console. Actually, that would be the right way to resolve the issue, so let me even try to submit zephyrproject-rtos/sdk-ng#221 (though I'm not sure who would be looking into that).

[pfalcon]

In the meantime, pushed a commit with ioctl() conversion to syscall.

[pfalcon]

In the meantime, pushed a commit with ioctl() conversion to syscall.

And it's the usual suspect, @andrewboie, I'm sure there's nothing new in this #if 0, it was expected all along (and again, why it spent so much time in backlog).

[zephyrbot]

All checks passed.

checkpatch (informational only, not a failure)

-:11: WARNING:LONG_LINE: line over 80 characters
#11: FILE: include/posix/sys/ioctl.h:11:
+__syscall int sys_ioctl(int fd, unsigned long request, long n_args, uintptr_t *args);

-:211: WARNING:LONG_LINE: line over 80 characters
#211: FILE: lib/os/fdtable.c:250:
+	res = z_fdtable_call_ioctl(fdtable[fd].vtable, fdtable[fd].obj, ZFD_IOCTL_CLOSE, 0);

-:255: WARNING:LONG_LINE: line over 80 characters
#255: FILE: lib/os/fdtable.c:292:
+int z_impl_sys_ioctl(int fd, unsigned long request, long n_args, uintptr_t *args)

-:264: WARNING:LONG_LINE: line over 80 characters
#264: FILE: lib/os/fdtable.c:298:
+	return fdtable[fd].vtable->ioctl(fdtable[fd].obj, request, n_args, args);

-:268: WARNING:LONG_LINE: line over 80 characters
#268: FILE: lib/os/fdtable.c:302:
+ssize_t z_vrfy_sys_ioctl(int fd, unsigned long request, long n_args, uintptr_t *args)

-:539: WARNING:LONG_LINE: line over 80 characters
#539: FILE: subsys/net/lib/sockets/sockets.c:1679:
+static int sock_ioctl_vmeth(void *obj, unsigned long request, long n_args, uintptr_t *args)

-:652: WARNING:LONG_LINE: line over 80 characters
#652: FILE: subsys/net/lib/sockets/sockets_tls.c:1200:
+	ret = z_fdtable_call_ioctl(&sock_fd_op_vtable.fd_vtable, ctx, ZFD_IOCTL_CLOSE, 0);

-:661: WARNING:LONG_LINE: line over 80 characters
#661: FILE: subsys/net/lib/sockets/sockets_tls.c:1334:
+	err = z_fdtable_call_ioctl(&sock_fd_op_vtable.fd_vtable, child, ZFD_IOCTL_CLOSE, 0);

- total: 0 errors, 8 warnings, 764 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PREFER_KERNEL_TYPES PRINTK_WITHOUT_KERN_LEVEL SPDX_LICENSE_TAG SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

[pfalcon]

Otherwise, 3rd commit in series patches an existing socket test to add read()/write() testcases.

[andrewboie]

I'm not sure though what additional validation is needed on the individual arguments in the array though?

@dleach02 That's the problem: the individual arguments needs to be validated. They could contain pointer values, and if so every single one of them needs to be bounded and checked for appropriate access. We can't just unmarshal them on the kernel side and call it a day handing them off to the implementation; if the implementation treats any of the args as a pointer and not an integral value we have a potential exploit because none of these pointers have been validated.

What we have here is a variadic interface to send up to N arbitrary uintptr_t arguments to the kernel, with no knowledge on the kernel side what these arguments actually represent, since the verification is being done in a generic verifier for any ioctl.

This generic interface will not work, the particular semantics of the arguments entirely depend on what the particular function is. Which is why I asked to just demux ioctl calls in userspace and send them each as different syscalls.

Now that I think of it, there's also the problem that there's no accounting for what threads can use what file descriptors, unless I am missing something a malicious thread could manipulate any file descriptor active in the system in the current design. That's something that should probably be looked at separately.

[cfriedt]

Now that I think of it, there's also the problem that there's no accounting for what threads can use what file descriptors, unless I am missing something a malicious thread could manipulate any file descriptor active in the system in the current design. That's something that should probably be looked at separately.

Yep. That's a biggie.

[pfalcon]

Yep. That's a biggie.

Indeed, that's a "biggie":

File descriptors are per process, not per thread. So whoever will implement support for multiple processes in Zephyr, will also make sure that fdtable is per-process.

[dleach02]

@andrewboie, I understand what you are saying. We should clarify (if it isn't already done), what the rules are for validating the parameters in a system call. It seems like the rule you are articulating is that all parameters must be properly typed and known at the time of the call and they must be able to be properly verified. Hiding the type of the parameter is not allowed.

I'm assuming the baseline problem is that the IOCTL is directed at different underlying drivers/interfaces and as such, the interpretation of the data is different depending on the driver needs. The generic interface of the IOCTL will not be able to be supported via a syscall. If the "syscall" happens lower where the user space code takes in the IOCTL and then breaks out the parameters of the veriadic that could potentially be a syscall point of entry that could validate the parameters?

[cfriedt]

Yep. That's a biggie.

File descriptors are per process, not per thread. So whoever will implement support for multiple processes in Zephyr, will also make sure that fdtable is per-process.

Of course. I didn't want to go on a diatribe, but obviously processes, permissions, pre-opened file descriptors (stdin, stdout, stderr), virtual devfs, console on serial port, etc. However, currently Zephyr does attempt to separate thread permissions based on (physical / virtual) address space and metadata. So even though the entire "process" has a flat set of file descriptors, certain threads really shouldn't be able to write to some file descriptors and until processes are implemented, fd's may need to have permissions associated with a thread or thread group.

[pfalcon]

@cfriedt: Show me a reference to the general purpose POSIX standard which says that, and we'll discuss it (later).

[andrewboie]

Yep. That's a biggie.

Permission accounting is a must-have in order to introduce these syscalls, you wouldn't want Minecraft to be able read Firefox's file descriptors at-will. I have a solution I'm testing to fix this for sockets (we used to have it but it broke, it uses kernel object permissions) that should be generalize-able

About ioctl(): consider for a moment that although having syscalls with arbitrary variadic argument types is a nightmare, nobody I know of against having an extensible vectored driver interface mechanism...there's no requirement to use ioctl()'s interface specifically, ioctls aren't portable anyway and you can always write wrappers

have a look at how Windows does it: https://docs.microsoft.com/en-us/windows/win32/api/ioapiset/nf-ioapiset-deviceiocontrol?redirectedfrom=MSDN their definition with a device, handle, and in/out buffers would be trivial to validate in a syscall

[d3zd3z]

As far as ioctl calls go, maybe we can glean something from how this is implemented in various Unixy platforms. The 'request' value is not a general number, but various bits within the number have meaning. Two of the bits indicate the direction that the data parameter moves, and then a range of the value indicates the size of the data pointer. It effectively maps the command number directly to the size and direction of the payload. The only calls that have to do additional validation are those that have additional pointers in the block being checked.

[pfalcon]

Ok, so I split off changes for just read()/write() and added a dedicated test for them: #26117

[github-actions]

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.

[github-actions]

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.