From: Aleksa Sarai <[email protected]>
Patch changelog:
v3: [<https://lore.kernel.org/lkml/[email protected]/>]
* Rename is_zeroed_user() to check_zeroed_user(). [Christian Brauner]
* Various minor cleanups. [Christian Brauner]
* Add copy_struct_from_user() tests.
v2: <https://lore.kernel.org/lkml/[email protected]/>
v1: <https://lore.kernel.org/lkml/[email protected]/>
This series was split off from the openat2(2) syscall discussion[1].
However, the copy_struct_to_user() helper has been dropped, because
after some discussion it appears that there is no really obvious
semantics for how copy_struct_to_user() should work on mixed-vintages
(for instance, whether [2] is the correct semantics for all syscalls).
A common pattern for syscall extensions is increasing the size of a
struct passed from userspace, such that the zero-value of the new fields
result in the old kernel behaviour (allowing for a mix of userspace and
kernel vintages to operate on one another in most cases).
Previously there was no common lib/ function that implemented
the necessary extension-checking semantics (and different syscalls
implemented them slightly differently or incompletely[3]). This series
implements the helper and ports several syscalls to use it.
Some in-kernel selftests are included in this patch. More complete
self-tests for copy_struct_from_user() are included in the openat2()
patchset.
[1]: https://lore.kernel.org/lkml/[email protected]/
[2]: commit 1251201c0d34 ("sched/core: Fix uclamp ABI bug, clean up and
robustify sched_read_attr() ABI logic and code")
[3]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
similar checks to copy_struct_from_user() while rt_sigprocmask(2)
always rejects differently-sized struct arguments.
Aleksa Sarai (4):
lib: introduce copy_struct_from_user() helper
clone3: switch to copy_struct_from_user()
sched_setattr: switch to copy_struct_from_user()
perf_event_open: switch to copy_struct_from_user()
include/linux/bitops.h | 7 ++
include/linux/uaccess.h | 4 ++
include/uapi/linux/sched.h | 2 +
kernel/events/core.c | 47 +++----------
kernel/fork.c | 34 ++--------
kernel/sched/core.c | 43 ++----------
lib/strnlen_user.c | 8 +--
lib/test_user_copy.c | 133 +++++++++++++++++++++++++++++++++++--
lib/usercopy.c | 123 ++++++++++++++++++++++++++++++++++
9 files changed, 287 insertions(+), 114 deletions(-)
--
2.23.0
From: Aleksa Sarai <[email protected]>
The change is very straightforward, and helps unify the syscall
interface for struct-from-userspace syscalls.
Signed-off-by: Aleksa Sarai <[email protected]>
---
kernel/events/core.c | 47 +++++++++-----------------------------------
1 file changed, 9 insertions(+), 38 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 4655adbbae10..3f0cb82e4fbc 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -10586,55 +10586,26 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
u32 size;
int ret;
- if (!access_ok(uattr, PERF_ATTR_SIZE_VER0))
- return -EFAULT;
-
- /*
- * zero the full structure, so that a short copy will be nice.
- */
+ /* Zero the full structure, so that a short copy will be nice. */
memset(attr, 0, sizeof(*attr));
ret = get_user(size, &uattr->size);
if (ret)
return ret;
- if (size > PAGE_SIZE) /* silly large */
- goto err_size;
-
- if (!size) /* abi compat */
+ /* ABI compatibility quirk: */
+ if (!size)
size = PERF_ATTR_SIZE_VER0;
-
- if (size < PERF_ATTR_SIZE_VER0)
+ if (size < PERF_ATTR_SIZE_VER0 || size > PAGE_SIZE)
goto err_size;
- /*
- * If we're handed a bigger struct than we know of,
- * ensure all the unknown bits are 0 - i.e. new
- * user-space does not rely on any kernel feature
- * extensions we dont know about yet.
- */
- if (size > sizeof(*attr)) {
- unsigned char __user *addr;
- unsigned char __user *end;
- unsigned char val;
-
- addr = (void __user *)uattr + sizeof(*attr);
- end = (void __user *)uattr + size;
-
- for (; addr < end; addr++) {
- ret = get_user(val, addr);
- if (ret)
- return ret;
- if (val)
- goto err_size;
- }
- size = sizeof(*attr);
+ ret = copy_struct_from_user(attr, sizeof(*attr), uattr, size);
+ if (ret) {
+ if (ret == -E2BIG)
+ goto err_size;
+ return ret;
}
- ret = copy_from_user(attr, uattr, size);
- if (ret)
- return -EFAULT;
-
attr->size = size;
if (attr->__reserved_1)
--
2.23.0
On Tue, Oct 01, 2019 at 05:15:26AM +1000, Aleksa Sarai wrote:
> From: Aleksa Sarai <[email protected]>
>
> The change is very straightforward, and helps unify the syscall
> interface for struct-from-userspace syscalls.
>
> Signed-off-by: Aleksa Sarai <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
-Kees
> ---
> kernel/events/core.c | 47 +++++++++-----------------------------------
> 1 file changed, 9 insertions(+), 38 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 4655adbbae10..3f0cb82e4fbc 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -10586,55 +10586,26 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
> u32 size;
> int ret;
>
> - if (!access_ok(uattr, PERF_ATTR_SIZE_VER0))
> - return -EFAULT;
> -
> - /*
> - * zero the full structure, so that a short copy will be nice.
> - */
> + /* Zero the full structure, so that a short copy will be nice. */
> memset(attr, 0, sizeof(*attr));
>
> ret = get_user(size, &uattr->size);
> if (ret)
> return ret;
>
> - if (size > PAGE_SIZE) /* silly large */
> - goto err_size;
> -
> - if (!size) /* abi compat */
> + /* ABI compatibility quirk: */
> + if (!size)
> size = PERF_ATTR_SIZE_VER0;
> -
> - if (size < PERF_ATTR_SIZE_VER0)
> + if (size < PERF_ATTR_SIZE_VER0 || size > PAGE_SIZE)
> goto err_size;
>
> - /*
> - * If we're handed a bigger struct than we know of,
> - * ensure all the unknown bits are 0 - i.e. new
> - * user-space does not rely on any kernel feature
> - * extensions we dont know about yet.
> - */
> - if (size > sizeof(*attr)) {
> - unsigned char __user *addr;
> - unsigned char __user *end;
> - unsigned char val;
> -
> - addr = (void __user *)uattr + sizeof(*attr);
> - end = (void __user *)uattr + size;
> -
> - for (; addr < end; addr++) {
> - ret = get_user(val, addr);
> - if (ret)
> - return ret;
> - if (val)
> - goto err_size;
> - }
> - size = sizeof(*attr);
> + ret = copy_struct_from_user(attr, sizeof(*attr), uattr, size);
> + if (ret) {
> + if (ret == -E2BIG)
> + goto err_size;
> + return ret;
> }
>
> - ret = copy_from_user(attr, uattr, size);
> - if (ret)
> - return -EFAULT;
> -
> attr->size = size;
>
> if (attr->__reserved_1)
> --
> 2.23.0
>
--
Kees Cook