Netdev Archive on lore.kernel.org
 help / color / 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; 26+ 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] 26+ 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; 26+ 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	[flat|nested] 26+ 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; 26+ 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	[flat|nested] 26+ 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; 26+ 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	[flat|nested] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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	[flat|nested] 26+ 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; 26+ 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	[flat|nested] 26+ 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; 26+ 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	[flat|nested] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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	[flat|nested] 26+ 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
                       ` (2 more replies)
  5 siblings, 3 replies; 26+ 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] 26+ 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
  2019-10-16  3:44     ` [PATCH v3 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner
  2 siblings, 1 reply; 26+ 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	[flat|nested] 26+ 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
  2 siblings, 1 reply; 26+ 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	[flat|nested] 26+ 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
  2 siblings, 0 replies; 26+ 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	[flat|nested] 26+ 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; 26+ 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] 26+ 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
  0 siblings, 0 replies; 26+ 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] 26+ messages in thread

end of thread, back to index

Thread overview: 26+ 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  3:44     ` [PATCH v3 3/3] bpf: use copy_struct_from_user() in bpf() syscall Christian Brauner

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org netdev@archiver.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox