| Message ID | 20191016004138.24845-1-christian.brauner@ubuntu.com |
|---|---|
| Headers | show |
| Series | bpf: switch to new usercopy helpers | expand |
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(-)
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
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
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(-)