| Message ID | 20160723000526.GA11650@ircssh.c.rugged-nimbus-611.internal |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Jul 22, 2016 at 05:05:27PM -0700, Sargun Dhillon wrote: > It was tested with the tracex7 program on x86-64. it's my fault to start tracexN tradition that turned out to be cumbersome, let's not continue it. Instead could you rename it to something meaningful? Like test_probe_write_user ? Right now it just prints client's peer address and human needs to visually verify that probe_write_user actually happened, if you can convert it into a test it will help a lot. We were planning to convert all of the samples/bpf/ into tests, so we can run them continuously. btw, single patch re-submit will not be picked up. Please always re-submit the whole patch set together. > +static const struct bpf_func_proto *bpf_get_probe_write_proto(void) { > + pr_warn_once("*****************************************************\n"); > + pr_warn_once("* bpf_probe_write_user: Experimental Feature in use *\n"); > + pr_warn_once("* bpf_probe_write_user: Feature may corrupt memory *\n"); > + pr_warn_once("*****************************************************\n"); > + pr_notice_ratelimited("bpf_probe_write_user: %s[%d] installing program with helper: it may corrupt user memory!", > + current->comm, task_pid_nr(current)); I thought we were argeeing on single pr_warn_ratelimited without banner ? The rest looks good. Thanks!
On Sat, Jul 23, 2016 at 12:35:12PM -0700, Alexei Starovoitov wrote: > On Fri, Jul 22, 2016 at 05:05:27PM -0700, Sargun Dhillon wrote: > > It was tested with the tracex7 program on x86-64. > > it's my fault to start tracexN tradition that turned out to be > cumbersome, let's not continue it. Instead could you rename it > to something meaningful? Like test_probe_write_user ? > Right now it just prints client's peer address and human needs to > visually verify that probe_write_user actually happened, if you can > convert it into a test it will help a lot. > We were planning to convert all of the samples/bpf/ into tests, > so we can run them continuously. The example has been modified to act like a test in the follow up set. It tests for the positive case (Did the helper work or not) as opposed to the negative case (is the helper able to violate the safety constraints we set forth)? I could do that as well, in another patch by mprotecting those pages, or some such. Should I add an additional negative test? > > btw, single patch re-submit will not be picked up. Please always > re-submit the whole patch set together. > I'll resubmit the set on another thread. It's just easier doing the reviews inline. > > +static const struct bpf_func_proto *bpf_get_probe_write_proto(void) { > > + pr_warn_once("*****************************************************\n"); > > + pr_warn_once("* bpf_probe_write_user: Experimental Feature in use *\n"); > > + pr_warn_once("* bpf_probe_write_user: Feature may corrupt memory *\n"); > > + pr_warn_once("*****************************************************\n"); > > + pr_notice_ratelimited("bpf_probe_write_user: %s[%d] installing program with helper: it may corrupt user memory!", > > + current->comm, task_pid_nr(current)); > > I thought we were argeeing on single pr_warn_ratelimited without banner ? I'll switch to that. > > The rest looks good. > Thanks! >
On Sat, Jul 23, 2016 at 05:39:42PM -0700, Sargun Dhillon wrote: > The example has been modified to act like a test in the follow up set. It tests > for the positive case (Did the helper work or not) as opposed to the negative > case (is the helper able to violate the safety constraints we set forth)? I > could do that as well, in another patch by mprotecting those pages, or some > such. Should I add an additional negative test? That would be awesome, but doesn't have to be in this patch set. It can be done as a follow up. Thanks!
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 2b7076f..da218fe 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -365,6 +365,16 @@ enum bpf_func_id { */ BPF_FUNC_get_current_task, + /** + * bpf_probe_write_user(void *dst, void *src, int len) + * safely attempt to write to a location + * @dst: destination address in userspace + * @src: source address on stack + * @len: number of bytes to copy + * Return: 0 on success or negative error + */ + BPF_FUNC_probe_write_user, + __BPF_FUNC_MAX_ID, }; diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index a12bbd3..f9c13e0 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -81,6 +81,56 @@ static const struct bpf_func_proto bpf_probe_read_proto = { .arg3_type = ARG_ANYTHING, }; +static u64 bpf_probe_write_user(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5) +{ + void *unsafe_ptr = (void *) (long) r1; + void *src = (void *) (long) r2; + int size = (int) r3; + + /* + * Ensure we're in a user context which it is safe for the helper + * to run. This helper has no business in a kthread. + * + * access_ok should prevent writing to non-user memory, but in some + * situations (nommu, temporary switch, etc...) access_ok does not + * provide enough validation. + * + * In order to avoid this we check the current segment to verify that + * it is USER_DS. This avoid odd architectures and user threads that + * temporarily switch to KERNEL_DS. + */ + + if (unlikely(in_interrupt() || + current->flags & (PF_KTHREAD | PF_EXITING))) + return -EPERM; + if (unlikely(segment_eq(get_fs(), KERNEL_DS))) + return -EPERM; + if (!access_ok(VERIFY_WRITE, unsafe_ptr, size)) + return -EPERM; + + return probe_kernel_write(unsafe_ptr, src, size); +} + +static const struct bpf_func_proto bpf_probe_write_user_proto = { + .func = bpf_probe_write_user, + .gpl_only = true, + .ret_type = RET_INTEGER, + .arg1_type = ARG_ANYTHING, + .arg2_type = ARG_PTR_TO_STACK, + .arg3_type = ARG_CONST_STACK_SIZE, +}; + +static const struct bpf_func_proto *bpf_get_probe_write_proto(void) { + pr_warn_once("*****************************************************\n"); + pr_warn_once("* bpf_probe_write_user: Experimental Feature in use *\n"); + pr_warn_once("* bpf_probe_write_user: Feature may corrupt memory *\n"); + pr_warn_once("*****************************************************\n"); + pr_notice_ratelimited("bpf_probe_write_user: %s[%d] installing program with helper: it may corrupt user memory!", + current->comm, task_pid_nr(current)); + + return &bpf_probe_write_user_proto; +} + /* * limited trace_printk() * only %d %u %x %ld %lu %lx %lld %llu %llx %p %s conversion specifiers allowed @@ -362,6 +412,8 @@ static const struct bpf_func_proto *tracing_func_proto(enum bpf_func_id func_id) return &bpf_get_smp_processor_id_proto; case BPF_FUNC_perf_event_read: return &bpf_perf_event_read_proto; + case BPF_FUNC_probe_write_user: + return bpf_get_probe_write_proto(); default: return NULL; } diff --git a/samples/bpf/bpf_helpers.h b/samples/bpf/bpf_helpers.h index 84e3fd9..217c8d5 100644 --- a/samples/bpf/bpf_helpers.h +++ b/samples/bpf/bpf_helpers.h @@ -41,6 +41,8 @@ static int (*bpf_perf_event_output)(void *ctx, void *map, int index, void *data, (void *) BPF_FUNC_perf_event_output; static int (*bpf_get_stackid)(void *ctx, void *map, int flags) = (void *) BPF_FUNC_get_stackid; +static int (*bpf_probe_write_user)(void *dst, void *src, int size) = + (void *) BPF_FUNC_probe_write_user; /* llvm builtin functions that eBPF C program may use to * emit BPF_LD_ABS and BPF_LD_IND instructions