| Message ID | 20190506183116.33014-1-joel@joelfernandes.org |
|---|---|
| State | RFC |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [v2,1/4] bpf: Add support for reading user pointers | expand |
On 05/06/2019 08:31 PM, Joel Fernandes (Google) wrote: > The eBPF based opensnoop tool fails to read the file path string passed > to the do_sys_open function. This is because it is a pointer to > userspace address and causes an -EFAULT when read with > probe_kernel_read. This is not an issue when running the tool on x86 but > is an issue on arm64. This patch adds a new bpf function call based > which calls the recently proposed probe_user_read function [1]. > Using this function call from opensnoop fixes the issue on arm64. > > [1] https://lore.kernel.org/patchwork/patch/1051588/ > > Cc: Michal Gregorczyk <michalgr@live.com> > Cc: Adrian Ratiu <adrian.ratiu@collabora.com> > Cc: Mohammad Husain <russoue@gmail.com> > Cc: Qais Yousef <qais.yousef@arm.com> > Cc: Srinivas Ramana <sramana@codeaurora.org> > Cc: duyuchao <yuchao.du@unisoc.com> > Cc: Manjo Raja Rao <linux@manojrajarao.com> > Cc: Karim Yaghmour <karim.yaghmour@opersys.com> > Cc: Tamir Carmeli <carmeli.tamir@gmail.com> > Cc: Yonghong Song <yhs@fb.com> > Cc: Alexei Starovoitov <ast@kernel.org> > Cc: Brendan Gregg <brendan.d.gregg@gmail.com> > Cc: Masami Hiramatsu <mhiramat@kernel.org> > Cc: Peter Ziljstra <peterz@infradead.org> > Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Kees Cook <keescook@chromium.org> > Cc: kernel-team@android.com > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > --- > Masami, could you carry these patches in the series where are you add > probe_user_read function? > > Previous submissions is here: > https://lore.kernel.org/patchwork/patch/1069552/ > v1->v2: split tools uapi sync into separate commit, added deprecation > warning for old bpf_probe_read function. Please properly submit this series to bpf tree once the base infrastructure from Masami is upstream. This series here should also fix up all current probe read usage under samples/bpf/ and tools/testing/selftests/bpf/. Thanks, Daniel
On Mon, May 06, 2019 at 09:11:19PM +0200, Daniel Borkmann wrote: > On 05/06/2019 08:31 PM, Joel Fernandes (Google) wrote: > > The eBPF based opensnoop tool fails to read the file path string passed > > to the do_sys_open function. This is because it is a pointer to > > userspace address and causes an -EFAULT when read with > > probe_kernel_read. This is not an issue when running the tool on x86 but > > is an issue on arm64. This patch adds a new bpf function call based > > which calls the recently proposed probe_user_read function [1]. > > Using this function call from opensnoop fixes the issue on arm64. > > > > [1] https://lore.kernel.org/patchwork/patch/1051588/ > > > > Cc: Michal Gregorczyk <michalgr@live.com> > > Cc: Adrian Ratiu <adrian.ratiu@collabora.com> > > Cc: Mohammad Husain <russoue@gmail.com> > > Cc: Qais Yousef <qais.yousef@arm.com> > > Cc: Srinivas Ramana <sramana@codeaurora.org> > > Cc: duyuchao <yuchao.du@unisoc.com> > > Cc: Manjo Raja Rao <linux@manojrajarao.com> > > Cc: Karim Yaghmour <karim.yaghmour@opersys.com> > > Cc: Tamir Carmeli <carmeli.tamir@gmail.com> > > Cc: Yonghong Song <yhs@fb.com> > > Cc: Alexei Starovoitov <ast@kernel.org> > > Cc: Brendan Gregg <brendan.d.gregg@gmail.com> > > Cc: Masami Hiramatsu <mhiramat@kernel.org> > > Cc: Peter Ziljstra <peterz@infradead.org> > > Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: kernel-team@android.com > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > --- > > Masami, could you carry these patches in the series where are you add > > probe_user_read function? > > > > Previous submissions is here: > > https://lore.kernel.org/patchwork/patch/1069552/ > > v1->v2: split tools uapi sync into separate commit, added deprecation > > warning for old bpf_probe_read function. > > Please properly submit this series to bpf tree once the base > infrastructure from Masami is upstream. Could you clarify what do you mean by "properly submit this series to bpf tree" mean? bpf@vger.kernel.org is CC'd. > This series here should > also fix up all current probe read usage under samples/bpf/ and > tools/testing/selftests/bpf/. Ok. Agreed, will do that. thanks, - Joel
On 05/06/19 14:31, Joel Fernandes (Google) wrote: > The eBPF based opensnoop tool fails to read the file path string passed > to the do_sys_open function. This is because it is a pointer to > userspace address and causes an -EFAULT when read with > probe_kernel_read. This is not an issue when running the tool on x86 but > is an issue on arm64. This patch adds a new bpf function call based > which calls the recently proposed probe_user_read function [1]. > Using this function call from opensnoop fixes the issue on arm64. You haven't updated the commit message as agreed. Please add more explanation on how arm64 fails or drop the reference. Anyone reads this as-is would think it always fails on arm64 but it does under some circumstances which should be explained properly. I tried opensnoop on 5.1-rc7 and 4.9.173 stable on juno-r2 using the in-tree defconfig and opensnoop returned the correct results on both cases. Thanks -- Qais Yousef
On 05/06/2019 09:57 PM, Joel Fernandes wrote: > On Mon, May 06, 2019 at 09:11:19PM +0200, Daniel Borkmann wrote: >> On 05/06/2019 08:31 PM, Joel Fernandes (Google) wrote: >>> The eBPF based opensnoop tool fails to read the file path string passed >>> to the do_sys_open function. This is because it is a pointer to >>> userspace address and causes an -EFAULT when read with >>> probe_kernel_read. This is not an issue when running the tool on x86 but >>> is an issue on arm64. This patch adds a new bpf function call based >>> which calls the recently proposed probe_user_read function [1]. >>> Using this function call from opensnoop fixes the issue on arm64. >>> >>> [1] https://lore.kernel.org/patchwork/patch/1051588/ >>> >>> Cc: Michal Gregorczyk <michalgr@live.com> >>> Cc: Adrian Ratiu <adrian.ratiu@collabora.com> >>> Cc: Mohammad Husain <russoue@gmail.com> >>> Cc: Qais Yousef <qais.yousef@arm.com> >>> Cc: Srinivas Ramana <sramana@codeaurora.org> >>> Cc: duyuchao <yuchao.du@unisoc.com> >>> Cc: Manjo Raja Rao <linux@manojrajarao.com> >>> Cc: Karim Yaghmour <karim.yaghmour@opersys.com> >>> Cc: Tamir Carmeli <carmeli.tamir@gmail.com> >>> Cc: Yonghong Song <yhs@fb.com> >>> Cc: Alexei Starovoitov <ast@kernel.org> >>> Cc: Brendan Gregg <brendan.d.gregg@gmail.com> >>> Cc: Masami Hiramatsu <mhiramat@kernel.org> >>> Cc: Peter Ziljstra <peterz@infradead.org> >>> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> >>> Cc: Steven Rostedt <rostedt@goodmis.org> >>> Cc: Kees Cook <keescook@chromium.org> >>> Cc: kernel-team@android.com >>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> >>> --- >>> Masami, could you carry these patches in the series where are you add >>> probe_user_read function? >>> >>> Previous submissions is here: >>> https://lore.kernel.org/patchwork/patch/1069552/ >>> v1->v2: split tools uapi sync into separate commit, added deprecation >>> warning for old bpf_probe_read function. >> >> Please properly submit this series to bpf tree once the base >> infrastructure from Masami is upstream. > > Could you clarify what do you mean by "properly submit this series to bpf > tree" mean? bpf@vger.kernel.org is CC'd. Yeah, send the BPF series to bpf@vger.kernel.org once Masami's patches have hit mainline, and we'll then route yours as fixes the usual path through bpf tree. >> This series here should >> also fix up all current probe read usage under samples/bpf/ and >> tools/testing/selftests/bpf/. > > Ok. Agreed, will do that. Great, thanks! Daniel
On Tue, May 07, 2019 at 01:10:45AM +0200, Daniel Borkmann wrote: > On 05/06/2019 09:57 PM, Joel Fernandes wrote: > > On Mon, May 06, 2019 at 09:11:19PM +0200, Daniel Borkmann wrote: > >> On 05/06/2019 08:31 PM, Joel Fernandes (Google) wrote: > >>> The eBPF based opensnoop tool fails to read the file path string passed > >>> to the do_sys_open function. This is because it is a pointer to > >>> userspace address and causes an -EFAULT when read with > >>> probe_kernel_read. This is not an issue when running the tool on x86 but > >>> is an issue on arm64. This patch adds a new bpf function call based > >>> which calls the recently proposed probe_user_read function [1]. > >>> Using this function call from opensnoop fixes the issue on arm64. > >>> > >>> [1] https://lore.kernel.org/patchwork/patch/1051588/ > >>> > >>> Cc: Michal Gregorczyk <michalgr@live.com> > >>> Cc: Adrian Ratiu <adrian.ratiu@collabora.com> > >>> Cc: Mohammad Husain <russoue@gmail.com> > >>> Cc: Qais Yousef <qais.yousef@arm.com> > >>> Cc: Srinivas Ramana <sramana@codeaurora.org> > >>> Cc: duyuchao <yuchao.du@unisoc.com> > >>> Cc: Manjo Raja Rao <linux@manojrajarao.com> > >>> Cc: Karim Yaghmour <karim.yaghmour@opersys.com> > >>> Cc: Tamir Carmeli <carmeli.tamir@gmail.com> > >>> Cc: Yonghong Song <yhs@fb.com> > >>> Cc: Alexei Starovoitov <ast@kernel.org> > >>> Cc: Brendan Gregg <brendan.d.gregg@gmail.com> > >>> Cc: Masami Hiramatsu <mhiramat@kernel.org> > >>> Cc: Peter Ziljstra <peterz@infradead.org> > >>> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> > >>> Cc: Steven Rostedt <rostedt@goodmis.org> > >>> Cc: Kees Cook <keescook@chromium.org> > >>> Cc: kernel-team@android.com > >>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > >>> --- > >>> Masami, could you carry these patches in the series where are you add > >>> probe_user_read function? > >>> > >>> Previous submissions is here: > >>> https://lore.kernel.org/patchwork/patch/1069552/ > >>> v1->v2: split tools uapi sync into separate commit, added deprecation > >>> warning for old bpf_probe_read function. > >> > >> Please properly submit this series to bpf tree once the base > >> infrastructure from Masami is upstream. > > > > Could you clarify what do you mean by "properly submit this series to bpf > > tree" mean? bpf@vger.kernel.org is CC'd. > > Yeah, send the BPF series to bpf@vger.kernel.org once Masami's patches have > hit mainline, and we'll then route yours as fixes the usual path through > bpf tree. Sounds great to me, thanks! - Joel
On Tue, 7 May 2019 01:10:45 +0200 Daniel Borkmann <daniel@iogearbox.net> wrote: > On 05/06/2019 09:57 PM, Joel Fernandes wrote: > > On Mon, May 06, 2019 at 09:11:19PM +0200, Daniel Borkmann wrote: > >> On 05/06/2019 08:31 PM, Joel Fernandes (Google) wrote: > >>> The eBPF based opensnoop tool fails to read the file path string passed > >>> to the do_sys_open function. This is because it is a pointer to > >>> userspace address and causes an -EFAULT when read with > >>> probe_kernel_read. This is not an issue when running the tool on x86 but > >>> is an issue on arm64. This patch adds a new bpf function call based > >>> which calls the recently proposed probe_user_read function [1]. > >>> Using this function call from opensnoop fixes the issue on arm64. > >>> > >>> [1] https://lore.kernel.org/patchwork/patch/1051588/ > >>> > >>> Cc: Michal Gregorczyk <michalgr@live.com> > >>> Cc: Adrian Ratiu <adrian.ratiu@collabora.com> > >>> Cc: Mohammad Husain <russoue@gmail.com> > >>> Cc: Qais Yousef <qais.yousef@arm.com> > >>> Cc: Srinivas Ramana <sramana@codeaurora.org> > >>> Cc: duyuchao <yuchao.du@unisoc.com> > >>> Cc: Manjo Raja Rao <linux@manojrajarao.com> > >>> Cc: Karim Yaghmour <karim.yaghmour@opersys.com> > >>> Cc: Tamir Carmeli <carmeli.tamir@gmail.com> > >>> Cc: Yonghong Song <yhs@fb.com> > >>> Cc: Alexei Starovoitov <ast@kernel.org> > >>> Cc: Brendan Gregg <brendan.d.gregg@gmail.com> > >>> Cc: Masami Hiramatsu <mhiramat@kernel.org> > >>> Cc: Peter Ziljstra <peterz@infradead.org> > >>> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> > >>> Cc: Steven Rostedt <rostedt@goodmis.org> > >>> Cc: Kees Cook <keescook@chromium.org> > >>> Cc: kernel-team@android.com > >>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > >>> --- > >>> Masami, could you carry these patches in the series where are you add > >>> probe_user_read function? > >>> > >>> Previous submissions is here: > >>> https://lore.kernel.org/patchwork/patch/1069552/ > >>> v1->v2: split tools uapi sync into separate commit, added deprecation > >>> warning for old bpf_probe_read function. > >> > >> Please properly submit this series to bpf tree once the base > >> infrastructure from Masami is upstream. > > > > Could you clarify what do you mean by "properly submit this series to bpf > > tree" mean? bpf@vger.kernel.org is CC'd. > > Yeah, send the BPF series to bpf@vger.kernel.org once Masami's patches have > hit mainline, and we'll then route yours as fixes the usual path through > bpf tree. OK, then I focus on my series. Keep this series separated. Thank you! > > >> This series here should > >> also fix up all current probe read usage under samples/bpf/ and > >> tools/testing/selftests/bpf/. > > > > Ok. Agreed, will do that. > > Great, thanks! > Daniel
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 929c8e537a14..8146784b9fe3 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -2431,6 +2431,12 @@ union bpf_attr { * Return * A **struct bpf_sock** pointer on success, or **NULL** in * case of failure. + * + * int bpf_probe_read_user(void *dst, int size, void *src) + * Description + * Read a userspace pointer safely. + * Return + * 0 on success or negative error */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -2531,7 +2537,8 @@ union bpf_attr { FN(sk_fullsock), \ FN(tcp_sock), \ FN(skb_ecn_set_ce), \ - FN(get_listener_sock), + FN(get_listener_sock), \ + FN(probe_read_user), /* integer value in 'imm' field of BPF_CALL instruction selects which helper * function eBPF program intends to call diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index d64c00afceb5..7485deb0777f 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -153,6 +153,26 @@ static const struct bpf_func_proto bpf_probe_read_proto = { .arg3_type = ARG_ANYTHING, }; +BPF_CALL_3(bpf_probe_read_user, void *, dst, u32, size, const void *, unsafe_ptr) +{ + int ret; + + ret = probe_user_read(dst, unsafe_ptr, size); + if (unlikely(ret < 0)) + memset(dst, 0, size); + + return ret; +} + +static const struct bpf_func_proto bpf_probe_read_user_proto = { + .func = bpf_probe_read_user, + .gpl_only = true, + .ret_type = RET_INTEGER, + .arg1_type = ARG_PTR_TO_UNINIT_MEM, + .arg2_type = ARG_CONST_SIZE_OR_ZERO, + .arg3_type = ARG_ANYTHING, +}; + BPF_CALL_3(bpf_probe_write_user, void *, unsafe_ptr, const void *, src, u32, size) { @@ -571,6 +591,8 @@ tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_map_delete_elem_proto; case BPF_FUNC_probe_read: return &bpf_probe_read_proto; + case BPF_FUNC_probe_read_user: + return &bpf_probe_read_user_proto; case BPF_FUNC_ktime_get_ns: return &bpf_ktime_get_ns_proto; case BPF_FUNC_tail_call:
The eBPF based opensnoop tool fails to read the file path string passed to the do_sys_open function. This is because it is a pointer to userspace address and causes an -EFAULT when read with probe_kernel_read. This is not an issue when running the tool on x86 but is an issue on arm64. This patch adds a new bpf function call based which calls the recently proposed probe_user_read function [1]. Using this function call from opensnoop fixes the issue on arm64. [1] https://lore.kernel.org/patchwork/patch/1051588/ Cc: Michal Gregorczyk <michalgr@live.com> Cc: Adrian Ratiu <adrian.ratiu@collabora.com> Cc: Mohammad Husain <russoue@gmail.com> Cc: Qais Yousef <qais.yousef@arm.com> Cc: Srinivas Ramana <sramana@codeaurora.org> Cc: duyuchao <yuchao.du@unisoc.com> Cc: Manjo Raja Rao <linux@manojrajarao.com> Cc: Karim Yaghmour <karim.yaghmour@opersys.com> Cc: Tamir Carmeli <carmeli.tamir@gmail.com> Cc: Yonghong Song <yhs@fb.com> Cc: Alexei Starovoitov <ast@kernel.org> Cc: Brendan Gregg <brendan.d.gregg@gmail.com> Cc: Masami Hiramatsu <mhiramat@kernel.org> Cc: Peter Ziljstra <peterz@infradead.org> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Kees Cook <keescook@chromium.org> Cc: kernel-team@android.com Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> --- Masami, could you carry these patches in the series where are you add probe_user_read function? Previous submissions is here: https://lore.kernel.org/patchwork/patch/1069552/ v1->v2: split tools uapi sync into separate commit, added deprecation warning for old bpf_probe_read function. include/uapi/linux/bpf.h | 9 ++++++++- kernel/trace/bpf_trace.c | 22 ++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-)