bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] bpf: switch to new usercopy helpers
@ 2019-10-09 16:09 Christian Brauner
  2019-10-09 16:09 ` [PATCH 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
                   ` (4 more replies)
  0 siblings, 5 replies; 33+ messages in thread
From: Christian Brauner @ 2019-10-09 16:09 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, bpf
  Cc: Martin KaFai Lau, Song Liu, Yonghong Song, netdev, linux-kernel,
	Christian Brauner

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero()
  2019-10-09 16:09 [PATCH 0/3] bpf: switch to new usercopy helpers Christian Brauner
@ 2019-10-09 16:09 ` Christian Brauner
  2019-10-10 10:50   ` Aleksa Sarai
  2019-10-09 16:09 ` [PATCH 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 33+ messages in thread
From: Christian Brauner @ 2019-10-09 16:09 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, bpf
  Cc: Martin KaFai Lau, Song Liu, Yonghong Song, netdev, linux-kernel,
	Christian Brauner

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 <christian.brauner@ubuntu.com>
---
 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


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd()
  2019-10-09 16:09 [PATCH 0/3] bpf: switch to new usercopy helpers Christian Brauner
  2019-10-09 16:09 ` [PATCH 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
@ 2019-10-09 16:09 ` Christian Brauner
  2019-10-10 10:51   ` Aleksa Sarai
  2019-10-09 16:09 ` [PATCH 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 33+ messages in thread
From: Christian Brauner @ 2019-10-09 16:09 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, bpf
  Cc: Martin KaFai Lau, Song Liu, Yonghong Song, netdev, linux-kernel,
	Christian Brauner

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 <christian.brauner@ubuntu.com>
---
 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


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH 3/3] bpf: use copy_struct_from_user() in bpf() syscall
  2019-10-09 16:09 [PATCH 0/3] bpf: switch to new usercopy helpers Christian Brauner
  2019-10-09 16:09 ` [PATCH 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
  2019-10-09 16:09 ` [PATCH 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
@ 2019-10-09 16:09 ` Christian Brauner
  2019-10-10 10:51   ` Aleksa Sarai
  2019-10-09 23:06 ` [PATCH 0/3] bpf: switch to new usercopy helpers Alexei Starovoitov
  2019-10-16  0:41 ` [PATCH v2 " Christian Brauner
  4 siblings, 1 reply; 33+ messages in thread
From: Christian Brauner @ 2019-10-09 16:09 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, bpf
  Cc: Martin KaFai Lau, Song Liu, Yonghong Song, netdev, linux-kernel,
	Christian Brauner

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 <christian.brauner@ubuntu.com>
---
 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


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* Re: [PATCH 0/3] bpf: switch to new usercopy helpers
  2019-10-09 16:09 [PATCH 0/3] bpf: switch to new usercopy helpers Christian Brauner
                   ` (2 preceding siblings ...)
  2019-10-09 16:09 ` [PATCH 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner
@ 2019-10-09 23:06 ` Alexei Starovoitov
  2019-10-10  9:26   ` Christian Brauner
  2019-10-16  0:41 ` [PATCH v2 " Christian Brauner
  4 siblings, 1 reply; 33+ messages in thread
From: Alexei Starovoitov @ 2019-10-09 23:06 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Martin KaFai Lau,
	Song Liu, Yonghong Song, Network Development, LKML

On Wed, Oct 9, 2019 at 9:09 AM Christian Brauner
<christian.brauner@ubuntu.com> 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).

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 0/3] bpf: switch to new usercopy helpers
  2019-10-09 23:06 ` [PATCH 0/3] bpf: switch to new usercopy helpers Alexei Starovoitov
@ 2019-10-10  9:26   ` Christian Brauner
  2019-10-15 22:45     ` Alexei Starovoitov
  0 siblings, 1 reply; 33+ messages in thread
From: Christian Brauner @ 2019-10-10  9:26 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Martin KaFai Lau,
	Song Liu, Yonghong Song, Network Development, LKML

On Wed, Oct 09, 2019 at 04:06:18PM -0700, Alexei Starovoitov wrote:
> On Wed, Oct 9, 2019 at 9:09 AM Christian Brauner
> <christian.brauner@ubuntu.com> 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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero()
  2019-10-09 16:09 ` [PATCH 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
@ 2019-10-10 10:50   ` Aleksa Sarai
  0 siblings, 0 replies; 33+ messages in thread
From: Aleksa Sarai @ 2019-10-10 10:50 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Martin KaFai Lau,
	Song Liu, Yonghong Song, netdev, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1868 bytes --]

On 2019-10-09, Christian Brauner <christian.brauner@ubuntu.com> 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 <christian.brauner@ubuntu.com>

Acked-by: Aleksa Sarai <cyphar@cyphar.com>

> ---
>  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/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd()
  2019-10-09 16:09 ` [PATCH 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
@ 2019-10-10 10:51   ` Aleksa Sarai
  0 siblings, 0 replies; 33+ messages in thread
From: Aleksa Sarai @ 2019-10-10 10:51 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Martin KaFai Lau,
	Song Liu, Yonghong Song, netdev, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1607 bytes --]

On 2019-10-09, Christian Brauner <christian.brauner@ubuntu.com> 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 <christian.brauner@ubuntu.com>

Acked-by: Aleksa Sarai <cyphar@cyphar.com>

> ---
>  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/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 3/3] bpf: use copy_struct_from_user() in bpf() syscall
  2019-10-09 16:09 ` [PATCH 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner
@ 2019-10-10 10:51   ` Aleksa Sarai
  0 siblings, 0 replies; 33+ messages in thread
From: Aleksa Sarai @ 2019-10-10 10:51 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Martin KaFai Lau,
	Song Liu, Yonghong Song, netdev, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1727 bytes --]

On 2019-10-09, Christian Brauner <christian.brauner@ubuntu.com> 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 <christian.brauner@ubuntu.com>

Acked-by: Aleksa Sarai <cyphar@cyphar.com>

> ---
>  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/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 0/3] bpf: switch to new usercopy helpers
  2019-10-10  9:26   ` Christian Brauner
@ 2019-10-15 22:45     ` Alexei Starovoitov
  2019-10-15 22:55       ` Christian Brauner
  0 siblings, 1 reply; 33+ messages in thread
From: Alexei Starovoitov @ 2019-10-15 22:45 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Martin KaFai Lau,
	Song Liu, Yonghong Song, Network Development, LKML

On Thu, Oct 10, 2019 at 2:26 AM Christian Brauner
<christian.brauner@ubuntu.com> 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
> > <christian.brauner@ubuntu.com> 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.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 0/3] bpf: switch to new usercopy helpers
  2019-10-15 22:45     ` Alexei Starovoitov
@ 2019-10-15 22:55       ` Christian Brauner
  2019-10-15 23:02         ` Alexei Starovoitov
  0 siblings, 1 reply; 33+ messages in thread
From: Christian Brauner @ 2019-10-15 22:55 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Martin KaFai Lau,
	Song Liu, Yonghong Song, Network Development, LKML

On Tue, Oct 15, 2019 at 03:45:54PM -0700, Alexei Starovoitov wrote:
> On Thu, Oct 10, 2019 at 2:26 AM Christian Brauner
> <christian.brauner@ubuntu.com> 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
> > > <christian.brauner@ubuntu.com> 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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 0/3] bpf: switch to new usercopy helpers
  2019-10-15 22:55       ` Christian Brauner
@ 2019-10-15 23:02         ` Alexei Starovoitov
  2019-10-15 23:08           ` Christian Brauner
  0 siblings, 1 reply; 33+ messages in thread
From: Alexei Starovoitov @ 2019-10-15 23:02 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Martin KaFai Lau,
	Song Liu, Yonghong Song, Network Development, LKML

On Tue, Oct 15, 2019 at 3:55 PM Christian Brauner
<christian.brauner@ubuntu.com> 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
> > <christian.brauner@ubuntu.com> 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
> > > > <christian.brauner@ubuntu.com> 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 ?

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 0/3] bpf: switch to new usercopy helpers
  2019-10-15 23:02         ` Alexei Starovoitov
@ 2019-10-15 23:08           ` Christian Brauner
  0 siblings, 0 replies; 33+ messages in thread
From: Christian Brauner @ 2019-10-15 23:08 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Martin KaFai Lau,
	Song Liu, Yonghong Song, Network Development, LKML

On Tue Oct 15, 2019 at 4:02 PM Alexei Starovoitov wrote:
> On Tue, Oct 15, 2019 at 3:55 PM Christian Brauner
> <christian.brauner@ubuntu.com> 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
> > > <christian.brauner@ubuntu.com> 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
> > > > > <christian.brauner@ubuntu.com> 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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH v2 0/3] bpf: switch to new usercopy helpers
  2019-10-09 16:09 [PATCH 0/3] bpf: switch to new usercopy helpers Christian Brauner
                   ` (3 preceding siblings ...)
  2019-10-09 23:06 ` [PATCH 0/3] bpf: switch to new usercopy helpers Alexei Starovoitov
@ 2019-10-16  0:41 ` Christian Brauner
  2019-10-16  0:41   ` [PATCH v2 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
                     ` (5 more replies)
  4 siblings, 6 replies; 33+ messages in thread
From: Christian Brauner @ 2019-10-16  0:41 UTC (permalink / raw)
  To: christian.brauner
  Cc: ast, bpf, daniel, kafai, linux-kernel, netdev, songliubraving, yhs

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/20191009160907.10981-1-christian.brauner@ubuntu.com

/* 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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH v2 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero()
  2019-10-16  0:41 ` [PATCH v2 " Christian Brauner
@ 2019-10-16  0:41   ` Christian Brauner
  2019-10-16  0:41   ` [PATCH v2 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 33+ messages in thread
From: Christian Brauner @ 2019-10-16  0:41 UTC (permalink / raw)
  To: christian.brauner
  Cc: ast, bpf, daniel, kafai, linux-kernel, netdev, songliubraving,
	yhs, Aleksa Sarai

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 <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: bpf@vger.kernel.org
Acked-by: Aleksa Sarai <cyphar@cyphar.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
Link: https://lore.kernel.org/r/20191009160907.10981-2-christian.brauner@ubuntu.com

/* v2 */
- Alexei Starovoitov <ast@kernel.org>:
  - 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


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v2 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd()
  2019-10-16  0:41 ` [PATCH v2 " Christian Brauner
  2019-10-16  0:41   ` [PATCH v2 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
@ 2019-10-16  0:41   ` Christian Brauner
  2019-10-16  0:41   ` [PATCH v2 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 33+ messages in thread
From: Christian Brauner @ 2019-10-16  0:41 UTC (permalink / raw)
  To: christian.brauner
  Cc: ast, bpf, daniel, kafai, linux-kernel, netdev, songliubraving,
	yhs, Aleksa Sarai

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 <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: bpf@vger.kernel.org
Acked-by: Aleksa Sarai <cyphar@cyphar.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
Link: https://lore.kernel.org/r/20191009160907.10981-3-christian.brauner@ubuntu.com

/* v2 */
- Alexei Starovoitov <ast@kernel.org>:
  - 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


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v2 3/3] bpf: use copy_struct_from_user() in bpf() syscall
  2019-10-16  0:41 ` [PATCH v2 " Christian Brauner
  2019-10-16  0:41   ` [PATCH v2 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
  2019-10-16  0:41   ` [PATCH v2 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
@ 2019-10-16  0:41   ` Christian Brauner
  2019-10-16  0:48   ` [PATCH v2 0/3] bpf: switch to new usercopy helpers Christian Brauner
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 33+ messages in thread
From: Christian Brauner @ 2019-10-16  0:41 UTC (permalink / raw)
  To: christian.brauner
  Cc: ast, bpf, daniel, kafai, linux-kernel, netdev, songliubraving,
	yhs, Aleksa Sarai

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 <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: bpf@vger.kernel.org
Acked-by: Aleksa Sarai <cyphar@cyphar.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
Link: https://lore.kernel.org/r/20191009160907.10981-4-christian.brauner@ubuntu.com

/* v2 */
- Alexei Starovoitov <ast@kernel.org>:
  - 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


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 0/3] bpf: switch to new usercopy helpers
  2019-10-16  0:41 ` [PATCH v2 " Christian Brauner
                     ` (2 preceding siblings ...)
  2019-10-16  0:41   ` [PATCH v2 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner
@ 2019-10-16  0:48   ` Christian Brauner
  2019-10-16  2:14   ` Alexei Starovoitov
  2019-10-16  3:44   ` [PATCH v3 " Christian Brauner
  5 siblings, 0 replies; 33+ messages in thread
From: Christian Brauner @ 2019-10-16  0:48 UTC (permalink / raw)
  To: ast; +Cc: bpf, daniel, kafai, linux-kernel, netdev, songliubraving, yhs

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/20191009160907.10981-1-christian.brauner@ubuntu.com
> 
> /* 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:

  git@gitolite.kernel.org: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(-)

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 0/3] bpf: switch to new usercopy helpers
  2019-10-16  0:41 ` [PATCH v2 " Christian Brauner
                     ` (3 preceding siblings ...)
  2019-10-16  0:48   ` [PATCH v2 0/3] bpf: switch to new usercopy helpers Christian Brauner
@ 2019-10-16  2:14   ` Alexei Starovoitov
  2019-10-16  3:31     ` Christian Brauner
  2019-10-16  3:44   ` [PATCH v3 " Christian Brauner
  5 siblings, 1 reply; 33+ messages in thread
From: Alexei Starovoitov @ 2019-10-16  2:14 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Alexei Starovoitov, bpf, Daniel Borkmann, Martin KaFai Lau, LKML,
	Network Development, Song Liu, Yonghong Song

On Tue, Oct 15, 2019 at 5:41 PM Christian Brauner
<christian.brauner@ubuntu.com> 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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v2 0/3] bpf: switch to new usercopy helpers
  2019-10-16  2:14   ` Alexei Starovoitov
@ 2019-10-16  3:31     ` Christian Brauner
  0 siblings, 0 replies; 33+ messages in thread
From: Christian Brauner @ 2019-10-16  3:31 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Alexei Starovoitov, bpf, Daniel Borkmann, Martin KaFai Lau, LKML,
	Network Development, Song Liu, Yonghong Song

On Tue, Oct 15, 2019 at 07:14:42PM -0700, Alexei Starovoitov wrote:
> On Tue, Oct 15, 2019 at 5:41 PM Christian Brauner
> <christian.brauner@ubuntu.com> 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

^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 0/3] bpf: switch to new usercopy helpers
  2019-10-16  0:41 ` [PATCH v2 " Christian Brauner
                     ` (4 preceding siblings ...)
  2019-10-16  2:14   ` Alexei Starovoitov
@ 2019-10-16  3:44   ` Christian Brauner
  2019-10-16  3:44     ` [PATCH v3 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
                       ` (3 more replies)
  5 siblings, 4 replies; 33+ messages in thread
From: Christian Brauner @ 2019-10-16  3:44 UTC (permalink / raw)
  To: christian.brauner
  Cc: ast, bpf, daniel, kafai, linux-kernel, netdev, songliubraving, yhs

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:

  git@gitolite.kernel.org: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/20191009160907.10981-1-christian.brauner@ubuntu.com

/* v2 */
Link: https://lore.kernel.org/r/20191016004138.24845-1-christian.brauner@ubuntu.com
- 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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH v3 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero()
  2019-10-16  3:44   ` [PATCH v3 " Christian Brauner
@ 2019-10-16  3:44     ` Christian Brauner
  2019-10-16  5:23       ` Alexei Starovoitov
  2019-10-16  3:44     ` [PATCH v3 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
                       ` (2 subsequent siblings)
  3 siblings, 1 reply; 33+ messages in thread
From: Christian Brauner @ 2019-10-16  3:44 UTC (permalink / raw)
  To: christian.brauner
  Cc: ast, bpf, daniel, kafai, linux-kernel, netdev, songliubraving,
	yhs, Aleksa Sarai

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 <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: bpf@vger.kernel.org
Acked-by: Aleksa Sarai <cyphar@cyphar.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
Link: https://lore.kernel.org/r/20191009160907.10981-2-christian.brauner@ubuntu.com

/* v2 */
Link: https://lore.kernel.org/r/20191016004138.24845-2-christian.brauner@ubuntu.com
- Alexei Starovoitov <ast@kernel.org>:
  - Add a comment in bpf_check_uarg_tail_zero() to clarify that
    copy_struct_from_user() should be used whenever possible instead.

/* v3 */
- Christian Brauner <christian.brauner@ubuntu.com>:
  - use correct checks for check_zeroed_user()
---
 kernel/bpf/syscall.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 82eabd4e38ad..40edcaeccd71 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -58,35 +58,28 @@ 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;
-
-	for (; addr < end; addr++) {
-		err = get_user(val, addr);
-		if (err)
-			return err;
-		if (val)
-			return -E2BIG;
-	}
+	err = check_zeroed_user(uaddr + expected_size, rest);
+	if (err <= 0)
+		return err ?: -E2BIG;
 
 	return 0;
 }
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd()
  2019-10-16  3:44   ` [PATCH v3 " Christian Brauner
  2019-10-16  3:44     ` [PATCH v3 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
@ 2019-10-16  3:44     ` Christian Brauner
  2019-10-16  5:25       ` Alexei Starovoitov
  2019-10-16  3:44     ` [PATCH v3 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner
  2019-10-16 11:18     ` [PATCH bpf-next v4 0/3] bpf: switch to new usercopy helpers Christian Brauner
  3 siblings, 1 reply; 33+ messages in thread
From: Christian Brauner @ 2019-10-16  3:44 UTC (permalink / raw)
  To: christian.brauner
  Cc: ast, bpf, daniel, kafai, linux-kernel, netdev, songliubraving,
	yhs, Aleksa Sarai

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 <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: bpf@vger.kernel.org
Acked-by: Aleksa Sarai <cyphar@cyphar.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
Link: https://lore.kernel.org/r/20191009160907.10981-3-christian.brauner@ubuntu.com

/* v2 */
Link: https://lore.kernel.org/r/20191016004138.24845-3-christian.brauner@ubuntu.com
- Alexei Starovoitov <ast@kernel.org>:
  - remove unneeded initialization

/* v3 */
unchanged
---
 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 40edcaeccd71..151447f314ca 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2306,20 +2306,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


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v3 3/3] bpf: use copy_struct_from_user() in bpf() syscall
  2019-10-16  3:44   ` [PATCH v3 " Christian Brauner
  2019-10-16  3:44     ` [PATCH v3 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
  2019-10-16  3:44     ` [PATCH v3 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
@ 2019-10-16  3:44     ` Christian Brauner
  2019-10-16 11:18     ` [PATCH bpf-next v4 0/3] bpf: switch to new usercopy helpers Christian Brauner
  3 siblings, 0 replies; 33+ messages in thread
From: Christian Brauner @ 2019-10-16  3:44 UTC (permalink / raw)
  To: christian.brauner
  Cc: ast, bpf, daniel, kafai, linux-kernel, netdev, songliubraving,
	yhs, Aleksa Sarai

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 <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: bpf@vger.kernel.org
Acked-by: Aleksa Sarai <cyphar@cyphar.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
Link: https://lore.kernel.org/r/20191009160907.10981-4-christian.brauner@ubuntu.com

/* v2 */
Link: https://lore.kernel.org/r/20191016004138.24845-4-christian.brauner@ubuntu.com
- Alexei Starovoitov <ast@kernel.org>:
  - remove unneeded initialization

/* v3 */
unchanged
---
 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 151447f314ca..0920593eacd0 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2814,20 +2814,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


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero()
  2019-10-16  3:44     ` [PATCH v3 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
@ 2019-10-16  5:23       ` Alexei Starovoitov
  0 siblings, 0 replies; 33+ messages in thread
From: Alexei Starovoitov @ 2019-10-16  5:23 UTC (permalink / raw)
  To: Christian Brauner
  Cc: ast, bpf, daniel, kafai, linux-kernel, netdev, songliubraving,
	yhs, Aleksa Sarai

On Wed, Oct 16, 2019 at 05:44:30AM +0200, Christian Brauner 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")
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: bpf@vger.kernel.org
> Acked-by: Aleksa Sarai <cyphar@cyphar.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
> /* v1 */
> Link: https://lore.kernel.org/r/20191009160907.10981-2-christian.brauner@ubuntu.com
> 
> /* v2 */
> Link: https://lore.kernel.org/r/20191016004138.24845-2-christian.brauner@ubuntu.com
> - Alexei Starovoitov <ast@kernel.org>:
>   - Add a comment in bpf_check_uarg_tail_zero() to clarify that
>     copy_struct_from_user() should be used whenever possible instead.
> 
> /* v3 */
> - Christian Brauner <christian.brauner@ubuntu.com>:
>   - use correct checks for check_zeroed_user()
> ---
>  kernel/bpf/syscall.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 82eabd4e38ad..40edcaeccd71 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -58,35 +58,28 @@ 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;
> -
> -	for (; addr < end; addr++) {
> -		err = get_user(val, addr);
> -		if (err)
> -			return err;
> -		if (val)
> -			return -E2BIG;
> -	}
> +	err = check_zeroed_user(uaddr + expected_size, rest);

Just noticed this 'rest' math.
I bet compiler can optimize unnecessary min+max, but
let's save it from that job.
Just do actual_size - expected_size here instead.


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd()
  2019-10-16  3:44     ` [PATCH v3 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
@ 2019-10-16  5:25       ` Alexei Starovoitov
  2019-10-16  7:30         ` [PATCH v3 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd()y Christian Brauner
  2019-10-16  7:43         ` [PATCH v3 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
  0 siblings, 2 replies; 33+ messages in thread
From: Alexei Starovoitov @ 2019-10-16  5:25 UTC (permalink / raw)
  To: Christian Brauner
  Cc: ast, bpf, daniel, kafai, linux-kernel, netdev, songliubraving,
	yhs, Aleksa Sarai

On Wed, Oct 16, 2019 at 05:44:31AM +0200, Christian Brauner 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")
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: bpf@vger.kernel.org
> Acked-by: Aleksa Sarai <cyphar@cyphar.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
> /* v1 */
> Link: https://lore.kernel.org/r/20191009160907.10981-3-christian.brauner@ubuntu.com
> 
> /* v2 */
> Link: https://lore.kernel.org/r/20191016004138.24845-3-christian.brauner@ubuntu.com
> - Alexei Starovoitov <ast@kernel.org>:
>   - remove unneeded initialization
> 
> /* v3 */
> unchanged
> ---
>  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 40edcaeccd71..151447f314ca 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2306,20 +2306,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);

really?! min?!
Frankly I'm disappointed in quality of these patches.
Especially considering it's v3.

Just live the code alone.


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd()y
  2019-10-16  5:25       ` Alexei Starovoitov
@ 2019-10-16  7:30         ` Christian Brauner
  2019-10-16  7:43         ` [PATCH v3 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
  1 sibling, 0 replies; 33+ messages in thread
From: Christian Brauner @ 2019-10-16  7:30 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: ast, bpf, daniel, kafai, linux-kernel, netdev, songliubraving,
	yhs, Aleksa Sarai

On Tue, Oct 15, 2019 at 10:25:49PM -0700, Alexei Starovoitov wrote:
> On Wed, Oct 16, 2019 at 05:44:31AM +0200, Christian Brauner 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")
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: bpf@vger.kernel.org
> > Acked-by: Aleksa Sarai <cyphar@cyphar.com>
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> > /* v1 */
> > Link: https://lore.kernel.org/r/20191009160907.10981-3-christian.brauner@ubuntu.com
> > 
> > /* v2 */
> > Link: https://lore.kernel.org/r/20191016004138.24845-3-christian.brauner@ubuntu.com
> > - Alexei Starovoitov <ast@kernel.org>:
> >   - remove unneeded initialization
> > 
> > /* v3 */
> > unchanged
> > ---
> >  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 40edcaeccd71..151447f314ca 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -2306,20 +2306,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);
> 
> really?! min?!
> Frankly I'm disappointed in quality of these patches.
> Especially considering it's v3.

Ok, then I'm sorry.

Christian

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v3 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd()
  2019-10-16  5:25       ` Alexei Starovoitov
  2019-10-16  7:30         ` [PATCH v3 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd()y Christian Brauner
@ 2019-10-16  7:43         ` Christian Brauner
  1 sibling, 0 replies; 33+ messages in thread
From: Christian Brauner @ 2019-10-16  7:43 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: ast, bpf, daniel, kafai, linux-kernel, netdev, songliubraving,
	yhs, Aleksa Sarai

On Tue, Oct 15, 2019 at 10:25:49PM -0700, Alexei Starovoitov wrote:
> On Wed, Oct 16, 2019 at 05:44:31AM +0200, Christian Brauner 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")
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: bpf@vger.kernel.org
> > Acked-by: Aleksa Sarai <cyphar@cyphar.com>
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> > /* v1 */
> > Link: https://lore.kernel.org/r/20191009160907.10981-3-christian.brauner@ubuntu.com
> > 
> > /* v2 */
> > Link: https://lore.kernel.org/r/20191016004138.24845-3-christian.brauner@ubuntu.com
> > - Alexei Starovoitov <ast@kernel.org>:
> >   - remove unneeded initialization
> > 
> > /* v3 */
> > unchanged
> > ---
> >  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 40edcaeccd71..151447f314ca 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -2306,20 +2306,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);
> 
> really?! min?!
> Frankly I'm disappointed in quality of these patches.
> Especially considering it's v3.
> 
> Just live the code alone.

Oh, I didn't see that part. I didn't know that this would upset
you that much. Sure, I can leave the code alone. Or I try to fix this
up. If you're not happy with you can just ignore this.

Christian

^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH bpf-next v4 0/3] bpf: switch to new usercopy helpers
  2019-10-16  3:44   ` [PATCH v3 " Christian Brauner
                       ` (2 preceding siblings ...)
  2019-10-16  3:44     ` [PATCH v3 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner
@ 2019-10-16 11:18     ` Christian Brauner
  2019-10-16 11:18       ` [PATCH bpf-next v4 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
                         ` (3 more replies)
  3 siblings, 4 replies; 33+ messages in thread
From: Christian Brauner @ 2019-10-16 11:18 UTC (permalink / raw)
  To: christian.brauner
  Cc: ast, bpf, daniel, kafai, linux-kernel, netdev, songliubraving, yhs

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/20191009160907.10981-1-christian.brauner@ubuntu.com

/* v2 */
Link: https://lore.kernel.org/r/20191016004138.24845-1-christian.brauner@ubuntu.com
- rebase onto bpf-next

/* v3 */
Link: https://lore.kernel.org/r/20191016034432.4418-1-christian.brauner@ubuntu.com

/* 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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH bpf-next v4 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero()
  2019-10-16 11:18     ` [PATCH bpf-next v4 0/3] bpf: switch to new usercopy helpers Christian Brauner
@ 2019-10-16 11:18       ` Christian Brauner
  2019-10-16 11:18       ` [PATCH bpf-next v4 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
                         ` (2 subsequent siblings)
  3 siblings, 0 replies; 33+ messages in thread
From: Christian Brauner @ 2019-10-16 11:18 UTC (permalink / raw)
  To: christian.brauner
  Cc: ast, bpf, daniel, kafai, linux-kernel, netdev, songliubraving,
	yhs, Aleksa Sarai

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 <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: bpf@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
Link: https://lore.kernel.org/r/20191009160907.10981-2-christian.brauner@ubuntu.com

/* v2 */
Link: https://lore.kernel.org/r/20191016004138.24845-2-christian.brauner@ubuntu.com
- Alexei Starovoitov <ast@kernel.org>:
  - 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/20191016034432.4418-2-christian.brauner@ubuntu.com
- Christian Brauner <christian.brauner@ubuntu.com>:
  - use correct checks for check_zeroed_user()

/* v4 */
- Alexei Starovoitov <ast@kernel.org>:
  - 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


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH bpf-next v4 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd()
  2019-10-16 11:18     ` [PATCH bpf-next v4 0/3] bpf: switch to new usercopy helpers Christian Brauner
  2019-10-16 11:18       ` [PATCH bpf-next v4 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
@ 2019-10-16 11:18       ` Christian Brauner
  2019-10-16 11:18       ` [PATCH bpf-next v4 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner
  2019-10-16 18:31       ` [PATCH bpf-next v4 0/3] bpf: switch to new usercopy helpers Alexei Starovoitov
  3 siblings, 0 replies; 33+ messages in thread
From: Christian Brauner @ 2019-10-16 11:18 UTC (permalink / raw)
  To: christian.brauner
  Cc: ast, bpf, daniel, kafai, linux-kernel, netdev, songliubraving,
	yhs, Aleksa Sarai

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 <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: bpf@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
Link: https://lore.kernel.org/r/20191009160907.10981-3-christian.brauner@ubuntu.com

/* v2 */
Link: https://lore.kernel.org/r/20191016004138.24845-3-christian.brauner@ubuntu.com
- Alexei Starovoitov <ast@kernel.org>:
  - remove unneeded initialization

/* v3 */
Link: https://lore.kernel.org/r/20191016034432.4418-3-christian.brauner@ubuntu.com
unchanged

/* v4 */
- Alexei Starovoitov <ast@kernel.org>:
  - move misplaced min after copy_struct_from_user()
---
 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 8d112bc069c0..d554ca7671b6 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2304,21 +2304,18 @@ 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);
+	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;
 	info.load_time = prog->aux->load_time;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH bpf-next v4 3/3] bpf: use copy_struct_from_user() in bpf() syscall
  2019-10-16 11:18     ` [PATCH bpf-next v4 0/3] bpf: switch to new usercopy helpers Christian Brauner
  2019-10-16 11:18       ` [PATCH bpf-next v4 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
  2019-10-16 11:18       ` [PATCH bpf-next v4 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
@ 2019-10-16 11:18       ` Christian Brauner
  2019-10-16 18:31       ` [PATCH bpf-next v4 0/3] bpf: switch to new usercopy helpers Alexei Starovoitov
  3 siblings, 0 replies; 33+ messages in thread
From: Christian Brauner @ 2019-10-16 11:18 UTC (permalink / raw)
  To: christian.brauner
  Cc: ast, bpf, daniel, kafai, linux-kernel, netdev, songliubraving,
	yhs, Aleksa Sarai

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 <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: bpf@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
Link: https://lore.kernel.org/r/20191009160907.10981-4-christian.brauner@ubuntu.com

/* v2 */
Link: https://lore.kernel.org/r/20191016004138.24845-4-christian.brauner@ubuntu.com
- Alexei Starovoitov <ast@kernel.org>:
  - remove unneeded initialization

/* v3 */
Link: https://lore.kernel.org/r/20191016034432.4418-4-christian.brauner@ubuntu.com
unchanged

/* v4 */
- Alexei Starovoitov <ast@kernel.org>:
  - move misplaced min after copy_struct_from_user()
---
 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 d554ca7671b6..47bf4a81002d 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2812,21 +2812,18 @@ 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);
+	/* copy attributes from user space, may be less than sizeof(bpf_attr) */
+	err = copy_struct_from_user(&attr, sizeof(attr), uattr, 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 = security_bpf(cmd, &attr, size);
 	if (err < 0)
 		return err;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* Re: [PATCH bpf-next v4 0/3] bpf: switch to new usercopy helpers
  2019-10-16 11:18     ` [PATCH bpf-next v4 0/3] bpf: switch to new usercopy helpers Christian Brauner
                         ` (2 preceding siblings ...)
  2019-10-16 11:18       ` [PATCH bpf-next v4 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner
@ 2019-10-16 18:31       ` Alexei Starovoitov
  3 siblings, 0 replies; 33+ messages in thread
From: Alexei Starovoitov @ 2019-10-16 18:31 UTC (permalink / raw)
  To: Christian Brauner
  Cc: ast, bpf, daniel, kafai, linux-kernel, netdev, songliubraving, yhs

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.


^ permalink raw reply	[flat|nested] 33+ messages in thread

end of thread, other threads:[~2019-10-16 18:31 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-09 16:09 [PATCH 0/3] bpf: switch to new usercopy helpers Christian Brauner
2019-10-09 16:09 ` [PATCH 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
2019-10-10 10:50   ` Aleksa Sarai
2019-10-09 16:09 ` [PATCH 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
2019-10-10 10:51   ` Aleksa Sarai
2019-10-09 16:09 ` [PATCH 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner
2019-10-10 10:51   ` Aleksa Sarai
2019-10-09 23:06 ` [PATCH 0/3] bpf: switch to new usercopy helpers Alexei Starovoitov
2019-10-10  9:26   ` Christian Brauner
2019-10-15 22:45     ` Alexei Starovoitov
2019-10-15 22:55       ` Christian Brauner
2019-10-15 23:02         ` Alexei Starovoitov
2019-10-15 23:08           ` Christian Brauner
2019-10-16  0:41 ` [PATCH v2 " Christian Brauner
2019-10-16  0:41   ` [PATCH v2 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
2019-10-16  0:41   ` [PATCH v2 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
2019-10-16  0:41   ` [PATCH v2 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner
2019-10-16  0:48   ` [PATCH v2 0/3] bpf: switch to new usercopy helpers Christian Brauner
2019-10-16  2:14   ` Alexei Starovoitov
2019-10-16  3:31     ` Christian Brauner
2019-10-16  3:44   ` [PATCH v3 " Christian Brauner
2019-10-16  3:44     ` [PATCH v3 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
2019-10-16  5:23       ` Alexei Starovoitov
2019-10-16  3:44     ` [PATCH v3 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
2019-10-16  5:25       ` Alexei Starovoitov
2019-10-16  7:30         ` [PATCH v3 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd()y Christian Brauner
2019-10-16  7:43         ` [PATCH v3 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
2019-10-16  3:44     ` [PATCH v3 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner
2019-10-16 11:18     ` [PATCH bpf-next v4 0/3] bpf: switch to new usercopy helpers Christian Brauner
2019-10-16 11:18       ` [PATCH bpf-next v4 1/3] bpf: use check_zeroed_user() in bpf_check_uarg_tail_zero() Christian Brauner
2019-10-16 11:18       ` [PATCH bpf-next v4 2/3] bpf: use copy_struct_from_user() in bpf_prog_get_info_by_fd() Christian Brauner
2019-10-16 11:18       ` [PATCH bpf-next v4 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner
2019-10-16 18:31       ` [PATCH bpf-next v4 0/3] bpf: switch to new usercopy helpers Alexei Starovoitov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).