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 <[email protected]>
Cc: Adrian Ratiu <[email protected]>
Cc: Mohammad Husain <[email protected]>
Cc: Qais Yousef <[email protected]>
Cc: Srinivas Ramana <[email protected]>
Cc: duyuchao <[email protected]>
Cc: Manjo Raja Rao <[email protected]>
Cc: Karim Yaghmour <[email protected]>
Cc: Tamir Carmeli <[email protected]>
Cc: Yonghong Song <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Brendan Gregg <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Peter Ziljstra <[email protected]>
Cc: Andrii Nakryiko <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: [email protected]
Signed-off-by: Joel Fernandes (Google) <[email protected]>
---
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(-)
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:
--
2.21.0.1020.gf2820cf01a-goog
The uapi in tools/ needs an update after support for new bpf function
calls were added. This commit does the same.
Signed-off-by: Joel Fernandes (Google) <[email protected]>
---
tools/include/uapi/linux/bpf.h | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 929c8e537a14..05af4e1151d3 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -2431,6 +2431,18 @@ 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
+ *
+ * int bpf_probe_read_kernel(void *dst, int size, void *src)
+ * Description
+ * Read a kernel pointer safely.
+ * Return
+ * 0 on success or negative error
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -2531,7 +2543,9 @@ 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), \
+ FN(probe_read_kernel),
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
* function eBPF program intends to call
--
2.21.0.1020.gf2820cf01a-goog
bpf_probe_read is deprecated and ambiguous. Add a warning if programs
still use it, so that they may be moved to not use it. After sufficient
time, the warning can be removed.
Signed-off-by: Joel Fernandes (Google) <[email protected]>
---
kernel/bpf/verifier.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 09d5d972c9ff..f8cc77e85b48 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7656,6 +7656,10 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
if (insn->src_reg == BPF_PSEUDO_CALL)
continue;
+ if (insn->imm == BPF_FUNC_probe_read)
+ pr_warn_once("bpf_probe_read is deprecated, please use "
+ "bpf_probe_read_{kernel,user} in eBPF programs.\n");
+
if (insn->imm == BPF_FUNC_get_route_realm)
prog->dst_needed = 1;
if (insn->imm == BPF_FUNC_get_prandom_u32)
--
2.21.0.1020.gf2820cf01a-goog
The bpf_probe_read function is ambiguous in whether the pointer being
read is a kernel or user one. Add a specific function for kernel pointer
in this patch. Previous patches add it for userspace pointers.
Signed-off-by: Joel Fernandes (Google) <[email protected]>
---
include/uapi/linux/bpf.h | 9 ++++++++-
kernel/trace/bpf_trace.c | 22 ++++++++++++++++++++++
2 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 8146784b9fe3..05af4e1151d3 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2437,6 +2437,12 @@ union bpf_attr {
* Read a userspace pointer safely.
* Return
* 0 on success or negative error
+ *
+ * int bpf_probe_read_kernel(void *dst, int size, void *src)
+ * Description
+ * Read a kernel pointer safely.
+ * Return
+ * 0 on success or negative error
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -2538,7 +2544,8 @@ union bpf_attr {
FN(tcp_sock), \
FN(skb_ecn_set_ce), \
FN(get_listener_sock), \
- FN(probe_read_user),
+ FN(probe_read_user), \
+ FN(probe_read_kernel),
/* 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 7485deb0777f..99dc354fd62b 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_kernel, void *, dst, u32, size, const void *, unsafe_ptr)
+{
+ int ret;
+
+ ret = probe_kernel_read(dst, unsafe_ptr, size);
+ if (unlikely(ret < 0))
+ memset(dst, 0, size);
+
+ return ret;
+}
+
+static const struct bpf_func_proto bpf_probe_read_kernel_proto = {
+ .func = bpf_probe_read_kernel,
+ .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_read_user, void *, dst, u32, size, const void *, unsafe_ptr)
{
int ret;
@@ -593,6 +613,8 @@ tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_probe_read_proto;
case BPF_FUNC_probe_read_user:
return &bpf_probe_read_user_proto;
+ case BPF_FUNC_probe_read_kernel:
+ return &bpf_probe_read_kernel_proto;
case BPF_FUNC_ktime_get_ns:
return &bpf_ktime_get_ns_proto;
case BPF_FUNC_tail_call:
--
2.21.0.1020.gf2820cf01a-goog
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 <[email protected]>
> Cc: Adrian Ratiu <[email protected]>
> Cc: Mohammad Husain <[email protected]>
> Cc: Qais Yousef <[email protected]>
> Cc: Srinivas Ramana <[email protected]>
> Cc: duyuchao <[email protected]>
> Cc: Manjo Raja Rao <[email protected]>
> Cc: Karim Yaghmour <[email protected]>
> Cc: Tamir Carmeli <[email protected]>
> Cc: Yonghong Song <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Brendan Gregg <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>
> Cc: Peter Ziljstra <[email protected]>
> Cc: Andrii Nakryiko <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: [email protected]
> Signed-off-by: Joel Fernandes (Google) <[email protected]>
> ---
> 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 <[email protected]>
> > Cc: Adrian Ratiu <[email protected]>
> > Cc: Mohammad Husain <[email protected]>
> > Cc: Qais Yousef <[email protected]>
> > Cc: Srinivas Ramana <[email protected]>
> > Cc: duyuchao <[email protected]>
> > Cc: Manjo Raja Rao <[email protected]>
> > Cc: Karim Yaghmour <[email protected]>
> > Cc: Tamir Carmeli <[email protected]>
> > Cc: Yonghong Song <[email protected]>
> > Cc: Alexei Starovoitov <[email protected]>
> > Cc: Brendan Gregg <[email protected]>
> > Cc: Masami Hiramatsu <[email protected]>
> > Cc: Peter Ziljstra <[email protected]>
> > Cc: Andrii Nakryiko <[email protected]>
> > Cc: Steven Rostedt <[email protected]>
> > Cc: Kees Cook <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Joel Fernandes (Google) <[email protected]>
> > ---
> > 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? [email protected] 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 <[email protected]>
>>> Cc: Adrian Ratiu <[email protected]>
>>> Cc: Mohammad Husain <[email protected]>
>>> Cc: Qais Yousef <[email protected]>
>>> Cc: Srinivas Ramana <[email protected]>
>>> Cc: duyuchao <[email protected]>
>>> Cc: Manjo Raja Rao <[email protected]>
>>> Cc: Karim Yaghmour <[email protected]>
>>> Cc: Tamir Carmeli <[email protected]>
>>> Cc: Yonghong Song <[email protected]>
>>> Cc: Alexei Starovoitov <[email protected]>
>>> Cc: Brendan Gregg <[email protected]>
>>> Cc: Masami Hiramatsu <[email protected]>
>>> Cc: Peter Ziljstra <[email protected]>
>>> Cc: Andrii Nakryiko <[email protected]>
>>> Cc: Steven Rostedt <[email protected]>
>>> Cc: Kees Cook <[email protected]>
>>> Cc: [email protected]
>>> Signed-off-by: Joel Fernandes (Google) <[email protected]>
>>> ---
>>> 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? [email protected] is CC'd.
Yeah, send the BPF series to [email protected] 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 <[email protected]>
> >>> Cc: Adrian Ratiu <[email protected]>
> >>> Cc: Mohammad Husain <[email protected]>
> >>> Cc: Qais Yousef <[email protected]>
> >>> Cc: Srinivas Ramana <[email protected]>
> >>> Cc: duyuchao <[email protected]>
> >>> Cc: Manjo Raja Rao <[email protected]>
> >>> Cc: Karim Yaghmour <[email protected]>
> >>> Cc: Tamir Carmeli <[email protected]>
> >>> Cc: Yonghong Song <[email protected]>
> >>> Cc: Alexei Starovoitov <[email protected]>
> >>> Cc: Brendan Gregg <[email protected]>
> >>> Cc: Masami Hiramatsu <[email protected]>
> >>> Cc: Peter Ziljstra <[email protected]>
> >>> Cc: Andrii Nakryiko <[email protected]>
> >>> Cc: Steven Rostedt <[email protected]>
> >>> Cc: Kees Cook <[email protected]>
> >>> Cc: [email protected]
> >>> Signed-off-by: Joel Fernandes (Google) <[email protected]>
> >>> ---
> >>> 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? [email protected] is CC'd.
>
> Yeah, send the BPF series to [email protected] 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 <[email protected]> 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 <[email protected]>
> >>> Cc: Adrian Ratiu <[email protected]>
> >>> Cc: Mohammad Husain <[email protected]>
> >>> Cc: Qais Yousef <[email protected]>
> >>> Cc: Srinivas Ramana <[email protected]>
> >>> Cc: duyuchao <[email protected]>
> >>> Cc: Manjo Raja Rao <[email protected]>
> >>> Cc: Karim Yaghmour <[email protected]>
> >>> Cc: Tamir Carmeli <[email protected]>
> >>> Cc: Yonghong Song <[email protected]>
> >>> Cc: Alexei Starovoitov <[email protected]>
> >>> Cc: Brendan Gregg <[email protected]>
> >>> Cc: Masami Hiramatsu <[email protected]>
> >>> Cc: Peter Ziljstra <[email protected]>
> >>> Cc: Andrii Nakryiko <[email protected]>
> >>> Cc: Steven Rostedt <[email protected]>
> >>> Cc: Kees Cook <[email protected]>
> >>> Cc: [email protected]
> >>> Signed-off-by: Joel Fernandes (Google) <[email protected]>
> >>> ---
> >>> 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? [email protected] is CC'd.
>
> Yeah, send the BPF series to [email protected] 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
--
Masami Hiramatsu <[email protected]>