The function check_uarg_tail_zero() may be useful for other part of the
code in the syscall.c file. Move this function at the beginning of the
file.
Signed-off-by: Mickaël Salaün <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Martin KaFai Lau <[email protected]>
---
This is needed for the Landlock patch series. :)
---
kernel/bpf/syscall.c | 52 ++++++++++++++++++++++++++--------------------------
1 file changed, 26 insertions(+), 26 deletions(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 6c772adabad2..c653ee0bd162 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -48,6 +48,32 @@ static const struct bpf_map_ops * const bpf_map_types[] = {
#undef BPF_MAP_TYPE
};
+static int check_uarg_tail_zero(void __user *uaddr,
+ size_t expected_size,
+ size_t actual_size)
+{
+ unsigned char __user *addr;
+ unsigned char __user *end;
+ unsigned char val;
+ int err;
+
+ if (actual_size <= expected_size)
+ return 0;
+
+ addr = uaddr + expected_size;
+ end = uaddr + actual_size;
+
+ for (; addr < end; addr++) {
+ err = get_user(val, addr);
+ if (err)
+ return err;
+ if (val)
+ return -E2BIG;
+ }
+
+ return 0;
+}
+
static struct bpf_map *find_and_alloc_map(union bpf_attr *attr)
{
struct bpf_map *map;
@@ -1246,32 +1272,6 @@ static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
return fd;
}
-static int check_uarg_tail_zero(void __user *uaddr,
- size_t expected_size,
- size_t actual_size)
-{
- unsigned char __user *addr;
- unsigned char __user *end;
- unsigned char val;
- int err;
-
- if (actual_size <= expected_size)
- return 0;
-
- addr = uaddr + expected_size;
- end = uaddr + actual_size;
-
- for (; addr < end; addr++) {
- err = get_user(val, addr);
- if (err)
- return err;
- if (val)
- return -E2BIG;
- }
-
- return 0;
-}
-
static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
const union bpf_attr *attr,
union bpf_attr __user *uattr)
--
2.13.3
The function check_uarg_tail_zero() was created from bpf(2) for
BPF_OBJ_GET_INFO_BY_FD without taking the access_ok() nor the PAGE_SIZE
checks. Make this checks more generally available while unlikely to be
triggered, extend the memory range check and add an explanation
including why the ToCToU should not be a security concern.
Signed-off-by: Mickaël Salaün <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Martin KaFai Lau <[email protected]>
Link: https://lkml.kernel.org/r/CAGXu5j+vRGFvJZmjtAcT8Hi8B+Wz0e1b6VKYZHfQP_=DXzC4CQ@mail.gmail.com
---
kernel/bpf/syscall.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c653ee0bd162..b884fdc371e0 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -48,6 +48,15 @@ static const struct bpf_map_ops * const bpf_map_types[] = {
#undef BPF_MAP_TYPE
};
+/*
+ * 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.
+ *
+ * There is a ToCToU between this function call and the following
+ * copy_from_user() call. However, this should not be a concern since this
+ * function is meant to be a future-proofing of bits.
+ */
static int check_uarg_tail_zero(void __user *uaddr,
size_t expected_size,
size_t actual_size)
@@ -57,6 +66,12 @@ static int check_uarg_tail_zero(void __user *uaddr,
unsigned char val;
int err;
+ if (unlikely(!access_ok(VERIFY_READ, uaddr, actual_size)))
+ return -EFAULT;
+
+ if (unlikely(actual_size > PAGE_SIZE)) /* silly large */
+ return -E2BIG;
+
if (actual_size <= expected_size)
return 0;
@@ -1393,17 +1408,6 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
if (!capable(CAP_SYS_ADMIN) && sysctl_unprivileged_bpf_disabled)
return -EPERM;
- if (!access_ok(VERIFY_READ, uattr, 1))
- return -EFAULT;
-
- if (size > PAGE_SIZE) /* silly large */
- return -E2BIG;
-
- /* 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.
- */
err = check_uarg_tail_zero(uattr, sizeof(attr), size);
if (err)
return err;
--
2.13.3
On 08/07/2017 06:36 PM, Mickaël Salaün wrote:
> The function check_uarg_tail_zero() may be useful for other part of the
> code in the syscall.c file. Move this function at the beginning of the
> file.
>
> Signed-off-by: Mickaël Salaün <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Daniel Borkmann <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Martin KaFai Lau <[email protected]>
Acked-by: Daniel Borkmann <[email protected]>
On 08/07/2017 06:36 PM, Mickaël Salaün wrote:
> The function check_uarg_tail_zero() was created from bpf(2) for
> BPF_OBJ_GET_INFO_BY_FD without taking the access_ok() nor the PAGE_SIZE
> checks. Make this checks more generally available while unlikely to be
> triggered, extend the memory range check and add an explanation
> including why the ToCToU should not be a security concern.
>
> Signed-off-by: Mickaël Salaün <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Daniel Borkmann <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Martin KaFai Lau <[email protected]>
> Link: https://lkml.kernel.org/r/CAGXu5j+vRGFvJZmjtAcT8Hi8B+Wz0e1b6VKYZHfQP_=DXzC4CQ@mail.gmail.com
> ---
> kernel/bpf/syscall.c | 26 +++++++++++++++-----------
> 1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index c653ee0bd162..b884fdc371e0 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -48,6 +48,15 @@ static const struct bpf_map_ops * const bpf_map_types[] = {
> #undef BPF_MAP_TYPE
> };
>
> +/*
> + * 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.
Nit: don't
> + *
> + * There is a ToCToU between this function call and the following
> + * copy_from_user() call. However, this should not be a concern since this
Lets make it a bit more clear to the reader: s/should not/is not/
> + * function is meant to be a future-proofing of bits.
> + */
> static int check_uarg_tail_zero(void __user *uaddr,
> size_t expected_size,
> size_t actual_size)
> @@ -57,6 +66,12 @@ static int check_uarg_tail_zero(void __user *uaddr,
> unsigned char val;
> int err;
>
> + if (unlikely(!access_ok(VERIFY_READ, uaddr, actual_size)))
> + return -EFAULT;
> +
> + if (unlikely(actual_size > PAGE_SIZE)) /* silly large */
> + return -E2BIG;
> +
Yeah, moving the checks into check_uarg_tail_zero() is
fine by me. Can we make the 'silly large' test first, so
we don't generate unnecessary work if we bail out later
anyway?
Other than that:
Acked-by: Daniel Borkmann <[email protected]>
Thanks,
Daniel
> if (actual_size <= expected_size)
> return 0;
>
> @@ -1393,17 +1408,6 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
> if (!capable(CAP_SYS_ADMIN) && sysctl_unprivileged_bpf_disabled)
> return -EPERM;
>
> - if (!access_ok(VERIFY_READ, uattr, 1))
> - return -EFAULT;
> -
> - if (size > PAGE_SIZE) /* silly large */
> - return -E2BIG;
> -
> - /* 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.
> - */
> err = check_uarg_tail_zero(uattr, sizeof(attr), size);
> if (err)
> return err;
>