Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] proc: unguarded zero-length user copies in pde_ioctl_buf()
- [Low] proc: split declaration and initialization of __free() variable in pde_ioctl_buf()
--
commit 468d1faef733a0559be5a05e125e1ce20ec69e4e
Author: Christian Brauner [off-list ref]
proc: let procfs copy ioctl arguments in and out
This commit introduces ->proc_ioctl_buf() and ->proc_ioctl_args to allow
procfs to copy ioctl arguments from and to userspace outside of pinning the
proc_dir_entry, preventing page faults during user copies from blocking
remove_proc_entry().
quoted hunk ↗ jump to hunk
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 3f29a1663d266..7e4ca5751abb5 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -571,11 +572,66 @@ static long pde_ioctl(struct proc_dir_entry *pde, struct file *file, unsigned in
[ ... ]
quoted hunk ↗ jump to hunk
+static long pde_ioctl_buf(struct proc_dir_entry *pde, struct file *file,
+ unsigned int cmd, void __user *uarg, unsigned long arg)
+{
+ void *buf __free(kvfree) = NULL;
+ struct proc_ioctl_arg a;
+ size_t size;
+ long rv;
+
+ if (!pde_ioctl_arg(pde, cmd, &a))
+ return -ENOTTY;
+ size = max(a.in, a.out);
+ if (size) {
+ buf = kvzalloc(size, GFP_KERNEL);
[Severity: Low]
This isn't a bug, but in pde_ioctl_buf() in fs/proc/inode.c, buf is declared
with __free(kvfree) = NULL at the top of the function and assigned later when
size is non-zero. Per the cleanup.h guidelines, could buf be defined and
initialized in a single statement once size is known (for example,
void *buf __free(kvfree) = size ? kvzalloc(size, GFP_KERNEL) : NULL;)?
quoted hunk ↗ jump to hunk
+ if (!buf)
+ return -ENOMEM;
+ if (copy_from_user(buf, uarg, a.in))
+ return -EFAULT;
+ }
+
+ if (!pde_pin(pde))
+ return -ENOTTY;
+ rv = pde->proc_ops->proc_ioctl_buf(file, cmd, buf ?: (void *)arg);
+ pde_unpin(pde);
+ if (rv >= 0 && copy_to_user(uarg, buf, a.out))
+ return -EFAULT;
[Severity: Medium]
Should copy_from_user() and copy_to_user() in pde_ioctl_buf() in
fs/proc/inode.c be guarded by checking a.in and a.out respectively?
Because a.in and a.out are runtime values loaded from
proc_ops->proc_ioctl_args, check_copy_size() and _inline_copy_from_user() /
_inline_copy_to_user() do not short-circuit when the length is 0 and still
invoke raw_copy_from_user() / raw_copy_to_user().
When a command has a.out == 0 (such as an _IOW or _IO command) and userspace
passes arg == 0, calling copy_to_user(NULL, buf, 0) fails with -EFAULT on
C-SKY after ->proc_ioctl_buf() has already succeeded, because C-SKY checks the
destination pointer for NULL before checking whether the length is 0:
arch/csky/lib/usercopy.c:raw_copy_to_user() {
...
"0: cmpnei %1, 0 \n"
" bf 8f \n"
...
}
On UML, calling copy_from_user() with a.in == 0 or copy_to_user() with
a.out == 0 still enters buffer_op() -> do_op_one_page() -> maybe_map() ->
handle_page_fault() on uarg before checking if remain == 0:
arch/um/kernel/skas/uaccess.c:buffer_op() {
...
size = min(PAGE_ALIGN(addr) - addr, (unsigned long) len);
remain = len;
n = do_op_one_page(addr, size, is_write, op, arg);
if (n != 0)
return (n < 0 ? remain : 0);
...
}
Additionally, when both a.in == 0 and a.out == 0, size is 0 and buf remains
NULL, so copy_to_user(uarg, NULL, 0) passes a NULL source pointer into
raw_copy_to_user() (and memcpy() on CONFIG_UACCESS_MEMCPY or UML builds).
Would it make sense to check if (a.in && copy_from_user(buf, uarg, a.in)) and
if (rv >= 0 && a.out && copy_to_user(uarg, buf, a.out))?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-work-procfs-core-v1-0-75df35e4f8b8@kernel.org?part=15