On 2024-09-02, Arnd Bergmann arnd@arndb.de wrote:
On Mon, Sep 2, 2024, at 07:06, Aleksa Sarai wrote:
This is based on copy_struct_from_user(), but there is one additional case to consider when creating a syscall that returns an extensible-struct to userspace -- how should data in the struct that cannot fit into the userspace struct be handled (ksize > usize)?
There are three possibilies:
The interface is like sched_getattr(2), where new information will be silently not provided to userspace. This is probably what most interfaces will want to do, as it provides the most possible backwards-compatibility.
The interface is like lsm_list_modules(2), where you want to return an error like -EMSGSIZE if not providing information could result in the userspace program making a serious mistake (such as one that could lead to a security problem) or if you want to provide some flag to userspace so they know that they are missing some information.
I'm not sure if EMSGSIZE is the best choice here, my feeling is that the kernel should instead try to behave the same way as an older kernel that did not know about the extra fields:
I agree this API is not ideal for syscalls because it can lead to backward-compatibility issues, but that is how lsm_list_modules(2) works. I suspect most syscalls will go with designs (1) or (3).
if the structure has always been longer than the provided buffer, -EMSGSIZE should likely have been returned all along. If older kernels just provided a short buffer, changing it now is an ABI change that would only affect intentionally broken callers, and I think keeping the behavior unchanged is more consistent.
if an extra flag was added along with the larger buffer size, the old kernel would likely have rejected the new flag with -EINVAL, so I think returning the same thing for userspace built against the old kernel headers is more consistent.
+static __always_inline __must_check int +copy_struct_to_user(void __user *dst, size_t usize, const void *src,
size_t ksize, bool *ignored_trailing)This feels like the kind of function that doesn't need to be inline at all and could be moved to lib/usercopy.c instead. It should clearly stay in the same place as copy_struct_from_user(), but we could move that as well.
IIRC Kees suggested copy_struct_from_user() be inline when I first included it, though I would have to dig through the old threads to find the reasoning. __builtin_object_size() was added some time after it was merged so that wasn't the original reason.
+{
- size_t size = min(ksize, usize);
- size_t rest = max(ksize, usize) - size;
- /* Double check if ksize is larger than a known object size. */
- if (WARN_ON_ONCE(ksize > __builtin_object_size(src, 1)))
return -E2BIG;I guess the __builtin_object_size() check is the reason for making it __always_inline, but that could be done in a trivial inline wrapper around the extern function. If ksize is always expected to be a constant for all callers, the check could even become a compile-time check instead of a WARN_ON_ONCE() that feels wrong here: if there is a code path where this can get triggered, there is clearly a kernel bug, but the only way to find out is to have a userspace caller that triggers the code path.
Again, the same code is already in copy_struct_from_user(), so this is not something you are adding but rather something we may want to change for both.
Arnd