Hey everyone,
In v5.4-rc2 we added two new helpers check_zeroed_user() and
copy_struct_from_user() including selftests (cf. [1]). It is a generic
interface designed to copy a struct from userspace. The helpers will be
especially useful for structs versioned by size of which we have quite a
few.
The most obvious benefit is that this helper lets us get rid of
duplicate code. We've already switched over sched_setattr(), perf_event_open(),
and clone3(). More importantly it will also help to ensure that users
implementing versioning-by-size end up with the same core semantics.
This point is especially crucial since we have at least one case where
versioning-by-size is used but with slighly different semantics:
sched_setattr(), perf_event_open(), and clone3() all do do similar
checks to copy_struct_from_user() while rt_sigprocmask(2) always rejects
differently-sized struct arguments.
This little series switches over bpf codepaths that have hand-rolled
implementations of these helpers.
Thanks!
Christian
/* Reference */
[1]: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper")
Christian Brauner (3):
bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero()
bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd()
bpf: use copy_struct_from_user() in bpf() syscall
kernel/bpf/syscall.c | 38 ++++++++++++--------------------------
1 file changed, 12 insertions(+), 26 deletions(-)
--
2.23.0
In v5.4-rc2 we added a new helper (cf. [1]) check_zeroed_user() which
does what bpf_check_uarg_tail_zero() is doing generically. We're slowly
switching such codepaths over to use check_zeroed_user() instead of
using their own hand-rolled version.
[1]: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper")
Signed-off-by: Christian Brauner <[email protected]>
---
kernel/bpf/syscall.c | 22 +++++++---------------
1 file changed, 7 insertions(+), 15 deletions(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 82eabd4e38ad..78790778f101 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -63,30 +63,22 @@ int bpf_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;
+ size_t size = min(expected_size, actual_size);
+ size_t rest = max(expected_size, actual_size) - size;
int err;
if (unlikely(actual_size > PAGE_SIZE)) /* silly large */
return -E2BIG;
- if (unlikely(!access_ok(uaddr, actual_size)))
- return -EFAULT;
-
if (actual_size <= expected_size)
return 0;
- addr = uaddr + expected_size;
- end = uaddr + actual_size;
+ err = check_zeroed_user(uaddr + expected_size, rest);
+ if (err < 0)
+ return err;
- for (; addr < end; addr++) {
- err = get_user(val, addr);
- if (err)
- return err;
- if (val)
- return -E2BIG;
- }
+ if (err)
+ return -E2BIG;
return 0;
}
--
2.23.0
In v5.4-rc2 we added a new helper (cf. [1]) copy_struct_from_user().
This helper is intended for all codepaths that copy structs from
userspace that are versioned by size. bpf_prog_get_info_by_fd() does
exactly what copy_struct_from_user() is doing.
Note that copy_struct_from_user() is calling min() already. So
technically, the min_t() call could go. But the info_len is used further
below so leave it.
[1]: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper")
Signed-off-by: Christian Brauner <[email protected]>
---
kernel/bpf/syscall.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 78790778f101..6f4f9097b1fe 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2312,13 +2312,10 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
u32 ulen;
int err;
- err = bpf_check_uarg_tail_zero(uinfo, sizeof(info), info_len);
+ info_len = min_t(u32, sizeof(info), info_len);
+ err = copy_struct_from_user(&info, sizeof(info), uinfo, info_len);
if (err)
return err;
- info_len = min_t(u32, sizeof(info), info_len);
-
- if (copy_from_user(&info, uinfo, info_len))
- return -EFAULT;
info.type = prog->type;
info.id = prog->aux->id;
--
2.23.0
In v5.4-rc2 we added a new helper (cf. [1]) copy_struct_from_user().
This helper is intended for all codepaths that copy structs from
userspace that are versioned by size. The bpf() syscall does exactly
what copy_struct_from_user() is doing.
Note that copy_struct_from_user() is calling min() already. So
technically, the min_t() call could go. But the size is used further
below so leave it.
[1]: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper")
Signed-off-by: Christian Brauner <[email protected]>
---
kernel/bpf/syscall.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 6f4f9097b1fe..6fdcbdb27501 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2819,14 +2819,11 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
if (sysctl_unprivileged_bpf_disabled && !capable(CAP_SYS_ADMIN))
return -EPERM;
- err = bpf_check_uarg_tail_zero(uattr, sizeof(attr), size);
- if (err)
- return err;
size = min_t(u32, size, sizeof(attr));
-
/* copy attributes from user space, may be less than sizeof(bpf_attr) */
- if (copy_from_user(&attr, uattr, size) != 0)
- return -EFAULT;
+ err = copy_struct_from_user(&attr, sizeof(attr), uattr, size);
+ if (err)
+ return err;
err = security_bpf(cmd, &attr, size);
if (err < 0)
--
2.23.0
On Wed, Oct 9, 2019 at 9:09 AM Christian Brauner
<[email protected]> wrote:
>
> Hey everyone,
>
> In v5.4-rc2 we added two new helpers check_zeroed_user() and
> copy_struct_from_user() including selftests (cf. [1]). It is a generic
> interface designed to copy a struct from userspace. The helpers will be
> especially useful for structs versioned by size of which we have quite a
> few.
>
> The most obvious benefit is that this helper lets us get rid of
> duplicate code. We've already switched over sched_setattr(), perf_event_open(),
> and clone3(). More importantly it will also help to ensure that users
> implementing versioning-by-size end up with the same core semantics.
>
> This point is especially crucial since we have at least one case where
> versioning-by-size is used but with slighly different semantics:
> sched_setattr(), perf_event_open(), and clone3() all do do similar
> checks to copy_struct_from_user() while rt_sigprocmask(2) always rejects
> differently-sized struct arguments.
>
> This little series switches over bpf codepaths that have hand-rolled
> implementations of these helpers.
check_zeroed_user() is not in bpf-next.
we will let this set sit in patchworks for some time until bpf-next
is merged back into net-next and we fast forward it.
Then we can apply it (assuming no conflicts).
On Wed, Oct 09, 2019 at 04:06:18PM -0700, Alexei Starovoitov wrote:
> On Wed, Oct 9, 2019 at 9:09 AM Christian Brauner
> <[email protected]> wrote:
> >
> > Hey everyone,
> >
> > In v5.4-rc2 we added two new helpers check_zeroed_user() and
> > copy_struct_from_user() including selftests (cf. [1]). It is a generic
> > interface designed to copy a struct from userspace. The helpers will be
> > especially useful for structs versioned by size of which we have quite a
> > few.
> >
> > The most obvious benefit is that this helper lets us get rid of
> > duplicate code. We've already switched over sched_setattr(), perf_event_open(),
> > and clone3(). More importantly it will also help to ensure that users
> > implementing versioning-by-size end up with the same core semantics.
> >
> > This point is especially crucial since we have at least one case where
> > versioning-by-size is used but with slighly different semantics:
> > sched_setattr(), perf_event_open(), and clone3() all do do similar
> > checks to copy_struct_from_user() while rt_sigprocmask(2) always rejects
> > differently-sized struct arguments.
> >
> > This little series switches over bpf codepaths that have hand-rolled
> > implementations of these helpers.
>
> check_zeroed_user() is not in bpf-next.
> we will let this set sit in patchworks for some time until bpf-next
> is merged back into net-next and we fast forward it.
> Then we can apply it (assuming no conflicts).
Sounds good to me. Just ping me when you need me to resend rebase onto
bpf-next.
Christian
On 2019-10-09, Christian Brauner <[email protected]> wrote:
> In v5.4-rc2 we added a new helper (cf. [1]) copy_struct_from_user().
> This helper is intended for all codepaths that copy structs from
> userspace that are versioned by size. bpf_prog_get_info_by_fd() does
> exactly what copy_struct_from_user() is doing.
> Note that copy_struct_from_user() is calling min() already. So
> technically, the min_t() call could go. But the info_len is used further
> below so leave it.
>
> [1]: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper")
> Signed-off-by: Christian Brauner <[email protected]>
Acked-by: Aleksa Sarai <[email protected]>
> ---
> kernel/bpf/syscall.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 78790778f101..6f4f9097b1fe 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2312,13 +2312,10 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
> u32 ulen;
> int err;
>
> - err = bpf_check_uarg_tail_zero(uinfo, sizeof(info), info_len);
> + info_len = min_t(u32, sizeof(info), info_len);
> + err = copy_struct_from_user(&info, sizeof(info), uinfo, info_len);
> if (err)
> return err;
> - info_len = min_t(u32, sizeof(info), info_len);
> -
> - if (copy_from_user(&info, uinfo, info_len))
> - return -EFAULT;
>
> info.type = prog->type;
> info.id = prog->aux->id;
> --
> 2.23.0
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
On 2019-10-09, Christian Brauner <[email protected]> wrote:
> In v5.4-rc2 we added a new helper (cf. [1]) copy_struct_from_user().
> This helper is intended for all codepaths that copy structs from
> userspace that are versioned by size. The bpf() syscall does exactly
> what copy_struct_from_user() is doing.
> Note that copy_struct_from_user() is calling min() already. So
> technically, the min_t() call could go. But the size is used further
> below so leave it.
>
> [1]: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper")
> Signed-off-by: Christian Brauner <[email protected]>
Acked-by: Aleksa Sarai <[email protected]>
> ---
> kernel/bpf/syscall.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 6f4f9097b1fe..6fdcbdb27501 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2819,14 +2819,11 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
> if (sysctl_unprivileged_bpf_disabled && !capable(CAP_SYS_ADMIN))
> return -EPERM;
>
> - err = bpf_check_uarg_tail_zero(uattr, sizeof(attr), size);
> - if (err)
> - return err;
> size = min_t(u32, size, sizeof(attr));
> -
> /* copy attributes from user space, may be less than sizeof(bpf_attr) */
> - if (copy_from_user(&attr, uattr, size) != 0)
> - return -EFAULT;
> + err = copy_struct_from_user(&attr, sizeof(attr), uattr, size);
> + if (err)
> + return err;
>
> err = security_bpf(cmd, &attr, size);
> if (err < 0)
> --
> 2.23.0
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
On 2019-10-09, Christian Brauner <[email protected]> wrote:
> In v5.4-rc2 we added a new helper (cf. [1]) check_zeroed_user() which
> does what bpf_check_uarg_tail_zero() is doing generically. We're slowly
> switching such codepaths over to use check_zeroed_user() instead of
> using their own hand-rolled version.
>
> [1]: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper")
> Signed-off-by: Christian Brauner <[email protected]>
Acked-by: Aleksa Sarai <[email protected]>
> ---
> kernel/bpf/syscall.c | 22 +++++++---------------
> 1 file changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 82eabd4e38ad..78790778f101 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -63,30 +63,22 @@ int bpf_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;
> + size_t size = min(expected_size, actual_size);
> + size_t rest = max(expected_size, actual_size) - size;
> int err;
>
> if (unlikely(actual_size > PAGE_SIZE)) /* silly large */
> return -E2BIG;
>
> - if (unlikely(!access_ok(uaddr, actual_size)))
> - return -EFAULT;
> -
> if (actual_size <= expected_size)
> return 0;
>
> - addr = uaddr + expected_size;
> - end = uaddr + actual_size;
> + err = check_zeroed_user(uaddr + expected_size, rest);
> + if (err < 0)
> + return err;
>
> - for (; addr < end; addr++) {
> - err = get_user(val, addr);
> - if (err)
> - return err;
> - if (val)
> - return -E2BIG;
> - }
> + if (err)
> + return -E2BIG;
>
> return 0;
> }
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
On Thu, Oct 10, 2019 at 2:26 AM Christian Brauner
<[email protected]> wrote:
>
> On Wed, Oct 09, 2019 at 04:06:18PM -0700, Alexei Starovoitov wrote:
> > On Wed, Oct 9, 2019 at 9:09 AM Christian Brauner
> > <[email protected]> wrote:
> > >
> > > Hey everyone,
> > >
> > > In v5.4-rc2 we added two new helpers check_zeroed_user() and
> > > copy_struct_from_user() including selftests (cf. [1]). It is a generic
> > > interface designed to copy a struct from userspace. The helpers will be
> > > especially useful for structs versioned by size of which we have quite a
> > > few.
> > >
> > > The most obvious benefit is that this helper lets us get rid of
> > > duplicate code. We've already switched over sched_setattr(), perf_event_open(),
> > > and clone3(). More importantly it will also help to ensure that users
> > > implementing versioning-by-size end up with the same core semantics.
> > >
> > > This point is especially crucial since we have at least one case where
> > > versioning-by-size is used but with slighly different semantics:
> > > sched_setattr(), perf_event_open(), and clone3() all do do similar
> > > checks to copy_struct_from_user() while rt_sigprocmask(2) always rejects
> > > differently-sized struct arguments.
> > >
> > > This little series switches over bpf codepaths that have hand-rolled
> > > implementations of these helpers.
> >
> > check_zeroed_user() is not in bpf-next.
> > we will let this set sit in patchworks for some time until bpf-next
> > is merged back into net-next and we fast forward it.
> > Then we can apply it (assuming no conflicts).
>
> Sounds good to me. Just ping me when you need me to resend rebase onto
> bpf-next.
-rc1 is now in bpf-next.
I took a look at patches and they look good overall.
In patches 2 and 3 the zero init via "= {};"
should be unnecessary anymore due to
copy_struct_from_user() logic, right?
Could you also convert all other case in kernel/bpf/,
so bpf_check_uarg_tail_zero() can be removed ?
Otherwise the half-way conversion will look odd.
On Tue, Oct 15, 2019 at 3:55 PM Christian Brauner
<[email protected]> wrote:
>
> On Tue, Oct 15, 2019 at 03:45:54PM -0700, Alexei Starovoitov wrote:
> > On Thu, Oct 10, 2019 at 2:26 AM Christian Brauner
> > <[email protected]> wrote:
> > >
> > > On Wed, Oct 09, 2019 at 04:06:18PM -0700, Alexei Starovoitov wrote:
> > > > On Wed, Oct 9, 2019 at 9:09 AM Christian Brauner
> > > > <[email protected]> wrote:
> > > > >
> > > > > Hey everyone,
> > > > >
> > > > > In v5.4-rc2 we added two new helpers check_zeroed_user() and
> > > > > copy_struct_from_user() including selftests (cf. [1]). It is a generic
> > > > > interface designed to copy a struct from userspace. The helpers will be
> > > > > especially useful for structs versioned by size of which we have quite a
> > > > > few.
> > > > >
> > > > > The most obvious benefit is that this helper lets us get rid of
> > > > > duplicate code. We've already switched over sched_setattr(), perf_event_open(),
> > > > > and clone3(). More importantly it will also help to ensure that users
> > > > > implementing versioning-by-size end up with the same core semantics.
> > > > >
> > > > > This point is especially crucial since we have at least one case where
> > > > > versioning-by-size is used but with slighly different semantics:
> > > > > sched_setattr(), perf_event_open(), and clone3() all do do similar
> > > > > checks to copy_struct_from_user() while rt_sigprocmask(2) always rejects
> > > > > differently-sized struct arguments.
> > > > >
> > > > > This little series switches over bpf codepaths that have hand-rolled
> > > > > implementations of these helpers.
> > > >
> > > > check_zeroed_user() is not in bpf-next.
> > > > we will let this set sit in patchworks for some time until bpf-next
> > > > is merged back into net-next and we fast forward it.
> > > > Then we can apply it (assuming no conflicts).
> > >
> > > Sounds good to me. Just ping me when you need me to resend rebase onto
> > > bpf-next.
> >
> > -rc1 is now in bpf-next.
> > I took a look at patches and they look good overall.
> >
> > In patches 2 and 3 the zero init via "= {};"
> > should be unnecessary anymore due to
> > copy_struct_from_user() logic, right?
>
> Right, I can remove them.
>
> >
> > Could you also convert all other case in kernel/bpf/,
> > so bpf_check_uarg_tail_zero() can be removed ?
> > Otherwise the half-way conversion will look odd.
>
> Hm, I thought I did that and concluded that bpf_check_uarg_tail_zero()
> can't be removed because sometimes it is called to verify whether a
> given struct is zeroed but nothing is actually copied from userspace but
> rather to userspace. See for example
> v5.4-rc3:kernel/bpf/syscall.c:bpf_map_get_info_by_fd()
> All call sites where something is actually copied from userspace I've
> switched to copy_struct_from_user().
I see. You're right.
Could you update the comment in bpf_check_uarg_tail_zero()
to clarify that copy_struct_from_user() should be used whenever
possible instead ?
On Tue Oct 15, 2019 at 4:02 PM Alexei Starovoitov wrote:
> On Tue, Oct 15, 2019 at 3:55 PM Christian Brauner
> <[email protected]> wrote:
> >
> > On Tue, Oct 15, 2019 at 03:45:54PM -0700, Alexei Starovoitov wrote:
> > > On Thu, Oct 10, 2019 at 2:26 AM Christian Brauner
> > > <[email protected]> wrote:
> > > >
> > > > On Wed, Oct 09, 2019 at 04:06:18PM -0700, Alexei Starovoitov wrote:
> > > > > On Wed, Oct 9, 2019 at 9:09 AM Christian Brauner
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > Hey everyone,
> > > > > >
> > > > > > In v5.4-rc2 we added two new helpers check_zeroed_user() and
> > > > > > copy_struct_from_user() including selftests (cf. [1]). It is a generic
> > > > > > interface designed to copy a struct from userspace. The helpers will be
> > > > > > especially useful for structs versioned by size of which we have quite a
> > > > > > few.
> > > > > >
> > > > > > The most obvious benefit is that this helper lets us get rid of
> > > > > > duplicate code. We've already switched over sched_setattr(), perf_event_open(),
> > > > > > and clone3(). More importantly it will also help to ensure that users
> > > > > > implementing versioning-by-size end up with the same core semantics.
> > > > > >
> > > > > > This point is especially crucial since we have at least one case where
> > > > > > versioning-by-size is used but with slighly different semantics:
> > > > > > sched_setattr(), perf_event_open(), and clone3() all do do similar
> > > > > > checks to copy_struct_from_user() while rt_sigprocmask(2) always rejects
> > > > > > differently-sized struct arguments.
> > > > > >
> > > > > > This little series switches over bpf codepaths that have hand-rolled
> > > > > > implementations of these helpers.
> > > > >
> > > > > check_zeroed_user() is not in bpf-next.
> > > > > we will let this set sit in patchworks for some time until bpf-next
> > > > > is merged back into net-next and we fast forward it.
> > > > > Then we can apply it (assuming no conflicts).
> > > >
> > > > Sounds good to me. Just ping me when you need me to resend rebase onto
> > > > bpf-next.
> > >
> > > -rc1 is now in bpf-next.
> > > I took a look at patches and they look good overall.
> > >
> > > In patches 2 and 3 the zero init via "= {};"
> > > should be unnecessary anymore due to
> > > copy_struct_from_user() logic, right?
> >
> > Right, I can remove them.
> >
> > >
> > > Could you also convert all other case in kernel/bpf/,
> > > so bpf_check_uarg_tail_zero() can be removed ?
> > > Otherwise the half-way conversion will look odd.
> >
> > Hm, I thought I did that and concluded that bpf_check_uarg_tail_zero()
> > can't be removed because sometimes it is called to verify whether a
> > given struct is zeroed but nothing is actually copied from userspace but
> > rather to userspace. See for example
> > v5.4-rc3:kernel/bpf/syscall.c:bpf_map_get_info_by_fd()
> > All call sites where something is actually copied from userspace I've
> > switched to copy_struct_from_user().
>
> I see. You're right.
> Could you update the comment in bpf_check_uarg_tail_zero()
> to clarify that copy_struct_from_user() should be used whenever
> possible instead ?
Yup, can do.
Christian
On Tue, Oct 15, 2019 at 03:45:54PM -0700, Alexei Starovoitov wrote:
> On Thu, Oct 10, 2019 at 2:26 AM Christian Brauner
> <[email protected]> wrote:
> >
> > On Wed, Oct 09, 2019 at 04:06:18PM -0700, Alexei Starovoitov wrote:
> > > On Wed, Oct 9, 2019 at 9:09 AM Christian Brauner
> > > <[email protected]> wrote:
> > > >
> > > > Hey everyone,
> > > >
> > > > In v5.4-rc2 we added two new helpers check_zeroed_user() and
> > > > copy_struct_from_user() including selftests (cf. [1]). It is a generic
> > > > interface designed to copy a struct from userspace. The helpers will be
> > > > especially useful for structs versioned by size of which we have quite a
> > > > few.
> > > >
> > > > The most obvious benefit is that this helper lets us get rid of
> > > > duplicate code. We've already switched over sched_setattr(), perf_event_open(),
> > > > and clone3(). More importantly it will also help to ensure that users
> > > > implementing versioning-by-size end up with the same core semantics.
> > > >
> > > > This point is especially crucial since we have at least one case where
> > > > versioning-by-size is used but with slighly different semantics:
> > > > sched_setattr(), perf_event_open(), and clone3() all do do similar
> > > > checks to copy_struct_from_user() while rt_sigprocmask(2) always rejects
> > > > differently-sized struct arguments.
> > > >
> > > > This little series switches over bpf codepaths that have hand-rolled
> > > > implementations of these helpers.
> > >
> > > check_zeroed_user() is not in bpf-next.
> > > we will let this set sit in patchworks for some time until bpf-next
> > > is merged back into net-next and we fast forward it.
> > > Then we can apply it (assuming no conflicts).
> >
> > Sounds good to me. Just ping me when you need me to resend rebase onto
> > bpf-next.
>
> -rc1 is now in bpf-next.
> I took a look at patches and they look good overall.
>
> In patches 2 and 3 the zero init via "= {};"
> should be unnecessary anymore due to
> copy_struct_from_user() logic, right?
Right, I can remove them.
>
> Could you also convert all other case in kernel/bpf/,
> so bpf_check_uarg_tail_zero() can be removed ?
> Otherwise the half-way conversion will look odd.
Hm, I thought I did that and concluded that bpf_check_uarg_tail_zero()
can't be removed because sometimes it is called to verify whether a
given struct is zeroed but nothing is actually copied from userspace but
rather to userspace. See for example
v5.4-rc3:kernel/bpf/syscall.c:bpf_map_get_info_by_fd()
All call sites where something is actually copied from userspace I've
switched to copy_struct_from_user().
Christian
Hey everyone,
In v5.4-rc2 we added two new helpers check_zeroed_user() and
copy_struct_from_user() including selftests (cf. [1]). It is a generic
interface designed to copy a struct from userspace. The helpers will be
especially useful for structs versioned by size of which we have quite a
few.
The most obvious benefit is that this helper lets us get rid of
duplicate code. We've already switched over sched_setattr(), perf_event_open(),
and clone3(). More importantly it will also help to ensure that users
implementing versioning-by-size end up with the same core semantics.
This point is especially crucial since we have at least one case where
versioning-by-size is used but with slighly different semantics:
sched_setattr(), perf_event_open(), and clone3() all do do similar
checks to copy_struct_from_user() while rt_sigprocmask(2) always rejects
differently-sized struct arguments.
This little series switches over bpf codepaths that have hand-rolled
implementations of these helpers.
Thanks!
Christian
/* v1 */
Link: https://lore.kernel.org/r/[email protected]
/* v2 */
- rebase onto bpf-next
/* Reference */
[1]: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper")
Christian Brauner (3):
bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero()
bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd()
bpf: use copy_struct_from_user() in bpf() syscall
kernel/bpf/syscall.c | 46 +++++++++++++++++---------------------------
1 file changed, 18 insertions(+), 28 deletions(-)
--
2.23.0
In v5.4-rc2 we added a new helper (cf. [1]) copy_struct_from_user().
This helper is intended for all codepaths that copy structs from
userspace that are versioned by size. bpf_prog_get_info_by_fd() does
exactly what copy_struct_from_user() is doing.
Note that copy_struct_from_user() is calling min() already. So
technically, the min_t() call could go. But the info_len is used further
below so leave it.
[1]: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper")
Cc: Alexei Starovoitov <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Cc: [email protected]
Acked-by: Aleksa Sarai <[email protected]>
Signed-off-by: Christian Brauner <[email protected]>
---
/* v1 */
Link: https://lore.kernel.org/r/[email protected]
/* v2 */
- Alexei Starovoitov <[email protected]>:
- remove unneeded initialization
---
kernel/bpf/syscall.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index b289ae747cae..45524089e15d 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2309,20 +2309,17 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
union bpf_attr __user *uattr)
{
struct bpf_prog_info __user *uinfo = u64_to_user_ptr(attr->info.info);
- struct bpf_prog_info info = {};
+ struct bpf_prog_info info;
u32 info_len = attr->info.info_len;
struct bpf_prog_stats stats;
char __user *uinsns;
u32 ulen;
int err;
- err = bpf_check_uarg_tail_zero(uinfo, sizeof(info), info_len);
+ info_len = min_t(u32, sizeof(info), info_len);
+ err = copy_struct_from_user(&info, sizeof(info), uinfo, info_len);
if (err)
return err;
- info_len = min_t(u32, sizeof(info), info_len);
-
- if (copy_from_user(&info, uinfo, info_len))
- return -EFAULT;
info.type = prog->type;
info.id = prog->aux->id;
--
2.23.0
In v5.4-rc2 we added a new helper (cf. [1]) check_zeroed_user() which
does what bpf_check_uarg_tail_zero() is doing generically. We're slowly
switching such codepaths over to use check_zeroed_user() instead of
using their own hand-rolled version.
[1]: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper")
Cc: Alexei Starovoitov <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Cc: [email protected]
Acked-by: Aleksa Sarai <[email protected]>
Signed-off-by: Christian Brauner <[email protected]>
---
/* v1 */
Link: https://lore.kernel.org/r/[email protected]
/* v2 */
- Alexei Starovoitov <[email protected]>:
- Add a comment in bpf_check_uarg_tail_zero() to clarify that
copy_struct_from_user() should be used whenever possible instead.
---
kernel/bpf/syscall.c | 26 +++++++++++---------------
1 file changed, 11 insertions(+), 15 deletions(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 82eabd4e38ad..b289ae747cae 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -58,35 +58,31 @@ static const struct bpf_map_ops * const bpf_map_types[] = {
* There is a ToCToU between this function call and the following
* copy_from_user() call. However, this is not a concern since this function is
* meant to be a future-proofing of bits.
+ *
+ * Note, instead of using bpf_check_uarg_tail_zero() followed by
+ * copy_from_user() use the dedicated copy_struct_from_user() helper which
+ * performs both tasks whenever possible.
*/
int bpf_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;
+ size_t size = min(expected_size, actual_size);
+ size_t rest = max(expected_size, actual_size) - size;
int err;
if (unlikely(actual_size > PAGE_SIZE)) /* silly large */
return -E2BIG;
- if (unlikely(!access_ok(uaddr, actual_size)))
- return -EFAULT;
-
if (actual_size <= expected_size)
return 0;
- addr = uaddr + expected_size;
- end = uaddr + actual_size;
+ err = check_zeroed_user(uaddr + expected_size, rest);
+ if (err < 0)
+ return err;
- for (; addr < end; addr++) {
- err = get_user(val, addr);
- if (err)
- return err;
- if (val)
- return -E2BIG;
- }
+ if (err)
+ return -E2BIG;
return 0;
}
--
2.23.0
In v5.4-rc2 we added a new helper (cf. [1]) copy_struct_from_user().
This helper is intended for all codepaths that copy structs from
userspace that are versioned by size. The bpf() syscall does exactly
what copy_struct_from_user() is doing.
Note that copy_struct_from_user() is calling min() already. So
technically, the min_t() call could go. But the size is used further
below so leave it.
[1]: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper")
Cc: Alexei Starovoitov <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Cc: [email protected]
Acked-by: Aleksa Sarai <[email protected]>
Signed-off-by: Christian Brauner <[email protected]>
---
/* v1 */
Link: https://lore.kernel.org/r/[email protected]
/* v2 */
- Alexei Starovoitov <[email protected]>:
- remove unneeded initialization
---
kernel/bpf/syscall.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 45524089e15d..5db9887a8f4c 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2817,20 +2817,17 @@ static int bpf_task_fd_query(const union bpf_attr *attr,
SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, size)
{
- union bpf_attr attr = {};
+ union bpf_attr attr;
int err;
if (sysctl_unprivileged_bpf_disabled && !capable(CAP_SYS_ADMIN))
return -EPERM;
- err = bpf_check_uarg_tail_zero(uattr, sizeof(attr), size);
- if (err)
- return err;
size = min_t(u32, size, sizeof(attr));
-
/* copy attributes from user space, may be less than sizeof(bpf_attr) */
- if (copy_from_user(&attr, uattr, size) != 0)
- return -EFAULT;
+ err = copy_struct_from_user(&attr, sizeof(attr), uattr, size);
+ if (err)
+ return err;
err = security_bpf(cmd, &attr, size);
if (err < 0)
--
2.23.0
On Wed, Oct 16, 2019 at 02:41:35AM +0200, Christian Brauner wrote:
> Hey everyone,
>
> In v5.4-rc2 we added two new helpers check_zeroed_user() and
> copy_struct_from_user() including selftests (cf. [1]). It is a generic
> interface designed to copy a struct from userspace. The helpers will be
> especially useful for structs versioned by size of which we have quite a
> few.
>
> The most obvious benefit is that this helper lets us get rid of
> duplicate code. We've already switched over sched_setattr(), perf_event_open(),
> and clone3(). More importantly it will also help to ensure that users
> implementing versioning-by-size end up with the same core semantics.
>
> This point is especially crucial since we have at least one case where
> versioning-by-size is used but with slighly different semantics:
> sched_setattr(), perf_event_open(), and clone3() all do do similar
> checks to copy_struct_from_user() while rt_sigprocmask(2) always rejects
> differently-sized struct arguments.
>
> This little series switches over bpf codepaths that have hand-rolled
> implementations of these helpers.
>
> Thanks!
> Christian
>
> /* v1 */
> Link: https://lore.kernel.org/r/[email protected]
>
> /* v2 */
> - rebase onto bpf-next
>
> /* Reference */
> [1]: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper")
Alexei, instead of applying the series you can also just pull from me:
The following changes since commit 5bc60de50dfea235634fdf38cbc992fb968d113b:
selftests: bpf: Don't try to read files without read permission (2019-10-15 16:27:25 -0700)
are available in the Git repository at:
[email protected]:pub/scm/linux/kernel/git/brauner/linux tags/bpf-copy-struct-from-user
for you to fetch changes up to da1699b959d182bb161be3ffc17eab063b2aedd2:
bpf: use copy_struct_from_user() in bpf() syscall (2019-10-16 02:35:11 +0200)
----------------------------------------------------------------
bpf-copy-struct-from-user
----------------------------------------------------------------
Christian Brauner (3):
bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero()
bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd()
bpf: use copy_struct_from_user() in bpf() syscall
kernel/bpf/syscall.c | 46 ++++++++++++++++++----------------------------
1 file changed, 18 insertions(+), 28 deletions(-)
On Tue, Oct 15, 2019 at 5:41 PM Christian Brauner
<[email protected]> wrote:
>
> Hey everyone,
>
> In v5.4-rc2 we added two new helpers check_zeroed_user() and
> copy_struct_from_user() including selftests (cf. [1]). It is a generic
> interface designed to copy a struct from userspace. The helpers will be
> especially useful for structs versioned by size of which we have quite a
> few.
Was it tested?
Either your conversion is incorrect or that generic helper is broken.
./test_progs -n 2
and
./test_btf
are catching the bug:
BTF prog info raw test[8] (line_info (No subprog. zero tailing
line_info): do_test_info_raw:6205:FAIL prog_fd:-1
expected_prog_load_failure:0 errno:7
nonzero tailing record in line_infoprocessed 0 insns (limit 1000000)
max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
On Tue, Oct 15, 2019 at 07:14:42PM -0700, Alexei Starovoitov wrote:
> On Tue, Oct 15, 2019 at 5:41 PM Christian Brauner
> <[email protected]> wrote:
> >
> > Hey everyone,
> >
> > In v5.4-rc2 we added two new helpers check_zeroed_user() and
> > copy_struct_from_user() including selftests (cf. [1]). It is a generic
> > interface designed to copy a struct from userspace. The helpers will be
> > especially useful for structs versioned by size of which we have quite a
> > few.
>
> Was it tested?
> Either your conversion is incorrect or that generic helper is broken.
> ./test_progs -n 2
> and
> ./test_btf
> are catching the bug:
> BTF prog info raw test[8] (line_info (No subprog. zero tailing
> line_info): do_test_info_raw:6205:FAIL prog_fd:-1
> expected_prog_load_failure:0 errno:7
> nonzero tailing record in line_infoprocessed 0 insns (limit 1000000)
> max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
Ugh, I misrememberd what the helper I helped design returns. The fix is:
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 5db9887a8f4c..0920593eacd0 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -78,11 +78,8 @@ int bpf_check_uarg_tail_zero(void __user *uaddr,
return 0;
err = check_zeroed_user(uaddr + expected_size, rest);
- if (err < 0)
- return err;
-
- if (err)
- return -E2BIG;
+ if (err <= 0)
+ return err ?: -E2BIG;
return 0;
}
aka check_zeroed_user() returns 0 if non-zero bytes are present, 1 if no
non-zero bytes were present, and -errno on error.
I'll send a fixed version. The tests pass for me with this.
Christian
Hey everyone,
In v5.4-rc2 we added two new helpers check_zeroed_user() and
copy_struct_from_user() including selftests (cf. [1]). It is a generic
interface designed to copy a struct from userspace. The helpers will be
especially useful for structs versioned by size of which we have quite a
few.
The most obvious benefit is that this helper lets us get rid of
duplicate code. We've already switched over sched_setattr(), perf_event_open(),
and clone3(). More importantly it will also help to ensure that users
implementing versioning-by-size end up with the same core semantics.
This point is especially crucial since we have at least one case where
versioning-by-size is used but with slighly different semantics:
sched_setattr(), perf_event_open(), and clone3() all do do similar
checks to copy_struct_from_user() while rt_sigprocmask(2) always rejects
differently-sized struct arguments.
This little series switches over bpf codepaths that have hand-rolled
implementations of these helpers.
This series can also be pulled:
The following changes since commit 5bc60de50dfea235634fdf38cbc992fb968d113b:
selftests: bpf: Don't try to read files without read permission (2019-10-15 16:27:25 -0700)
are available in the Git repository at:
[email protected]:pub/scm/linux/kernel/git/brauner/linux tags/bpf-copy-struct-from-user
for you to fetch changes up to 31a197d406cc01451e98312665d116c2dbb08202:
bpf: use copy_struct_from_user() in bpf() syscall (2019-10-16 05:32:38 +0200)
----------------------------------------------------------------
bpf-copy-struct-from-user
----------------------------------------------------------------
Christian Brauner (3):
bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero()
bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd()
bpf: use copy_struct_from_user() in bpf() syscall
kernel/bpf/syscall.c | 45 ++++++++++++++++-----------------------------
1 file changed, 16 insertions(+), 29 deletions(-)
Thanks!
Christian
/* v1 */
Link: https://lore.kernel.org/r/[email protected]
/* v2 */
Link: https://lore.kernel.org/r/[email protected]
- rebase onto bpf-next
/* Reference */
[1]: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper")
Christian Brauner (3):
bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero()
bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd()
bpf: use copy_struct_from_user() in bpf() syscall
kernel/bpf/syscall.c | 45 ++++++++++++++++----------------------------
1 file changed, 16 insertions(+), 29 deletions(-)
--
2.23.0
Hey everyone,
This is v4. If you still feel that I should leave this code alone then
simply ignore it. I won't send another version. Relevant tests pass and
I've verified that other failures were already present without this
patch series applied.
The misplaced min check has been moved after copy_struct_from_user() so
no non-zero bytes can be missed by copy_struct_from_user().
In v5.4-rc2 we added two new helpers check_zeroed_user() and
copy_struct_from_user() including selftests (cf. [1]). It is a generic
interface designed to copy a struct from userspace. The helpers will be
especially useful for structs versioned by size of which we have quite a
few.
The most obvious benefit is that this helper lets us get rid of
duplicate code. We've already switched over sched_setattr(), perf_event_open(),
and clone3(). More importantly it will also help to ensure that users
implementing versioning-by-size end up with the same core semantics.
This point is especially crucial since we have at least one case where
versioning-by-size is used but with slighly different semantics:
sched_setattr(), perf_event_open(), and clone3() all do do similar
checks to copy_struct_from_user() while rt_sigprocmask(2) always rejects
differently-sized struct arguments.
This little series switches over bpf codepaths that have hand-rolled
implementations of these helpers.
/* v1 */
Link: https://lore.kernel.org/r/[email protected]
/* v2 */
Link: https://lore.kernel.org/r/[email protected]
- rebase onto bpf-next
/* v3 */
Link: https://lore.kernel.org/r/[email protected]
/* Reference */
[1]: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper")
Christian Brauner (3):
bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero()
bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd()
bpf: use copy_struct_from_user() in bpf() syscall
kernel/bpf/syscall.c | 39 ++++++++++++---------------------------
1 file changed, 12 insertions(+), 27 deletions(-)
--
2.23.0
In v5.4-rc2 we added a new helper (cf. [1]) check_zeroed_user() which
does what bpf_check_uarg_tail_zero() is doing generically. We're slowly
switching such codepaths over to use check_zeroed_user() instead of
using their own hand-rolled version.
[1]: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper")
Cc: Alexei Starovoitov <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Cc: Aleksa Sarai <[email protected]>
Cc: [email protected]
Signed-off-by: Christian Brauner <[email protected]>
---
/* v1 */
Link: https://lore.kernel.org/r/[email protected]
/* v2 */
Link: https://lore.kernel.org/r/[email protected]
- Alexei Starovoitov <[email protected]>:
- Add a comment in bpf_check_uarg_tail_zero() to clarify that
copy_struct_from_user() should be used whenever possible instead.
/* v3 */
Link: https://lore.kernel.org/r/[email protected]
- Christian Brauner <[email protected]>:
- use correct checks for check_zeroed_user()
/* v4 */
- Alexei Starovoitov <[email protected]>:
- remove unnecessary min() and max() calls
---
kernel/bpf/syscall.c | 23 +++++++----------------
1 file changed, 7 insertions(+), 16 deletions(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 82eabd4e38ad..8d112bc069c0 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -58,35 +58,26 @@ static const struct bpf_map_ops * const bpf_map_types[] = {
* There is a ToCToU between this function call and the following
* copy_from_user() call. However, this is not a concern since this function is
* meant to be a future-proofing of bits.
+ *
+ * Note, instead of using bpf_check_uarg_tail_zero() followed by
+ * copy_from_user() use the dedicated copy_struct_from_user() helper which
+ * performs both tasks whenever possible.
*/
int bpf_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 (unlikely(actual_size > PAGE_SIZE)) /* silly large */
return -E2BIG;
- if (unlikely(!access_ok(uaddr, actual_size)))
- return -EFAULT;
-
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;
- }
+ err = check_zeroed_user(uaddr + expected_size, actual_size - expected_size);
+ if (err <= 0)
+ return err ?: -E2BIG;
return 0;
}
--
2.23.0
On Wed, Oct 16, 2019 at 01:18:07PM +0200, Christian Brauner wrote:
> Hey everyone,
>
> This is v4. If you still feel that I should leave this code alone then
> simply ignore it. I won't send another version. Relevant tests pass and
> I've verified that other failures were already present without this
> patch series applied.
I'm looking at it the following way:
- v1 was posted with zero testing. Obviously broken patches.
- v[23] was claimed to be tested yet there were serious bugs.
Means you folks ran only the tests that I pointed out in v1.
- in v4 patch 3 now has imbalanced copy_to_user. Previously there was:
bpf_check_tail_zero+copy_from+copy_to. Now it's copy_struct_from_user+copy_to.
It's puzzling to read that code.
More so the patch removes actual_size > PAGE_SIZE check.
It's a change in behavior that commit log doesn't talk about.
- so even v4 is not ready to be merged.
- the copy_struct_from_user api was implemented by the same people who
sent buggy patches. When you guys came up with this 'generic' api
you didn't consider bpf usage and bpf_check_uarg_tail_zero() is still necessary.
- few places that were converted to copy_struct_from_user() still have
size > PAGE_SIZE. Why wasn't it part of generic?
It means that the api likely will be refactored again, but looking at the way
the patches were crafted I have no confidence that it will be thoroughly tested.
- and if I accept this set the future refactoring may break bpf side silently.
- what check_zeroed_user() is actually doing? imo it's a premature
optimization with complex implementation. Most of the time the user space passes
the size that is the same as kernel expects or smaller. Rarely user space
libs are newer than the kernel. In such case they should probe the kernel
once for new features (like libbpf does) and should not be calling kernel api
again and again to receive the same E2BIG again and again. So the fancy long read
optimization is used once in real life. Yet it's much more complex than
simple byte loop we do in bpf_check_uarg_tail_zero.
- so no, I'm not applying this. Instead I'm taking bets when this shiny new thing
will cause issues to other subsystems.