[PATCH net 0/2] Two BPF fixes

[PATCH net 0/2] Two BPF fixes

[Daniel Borkmann]

The set fixes cb_access and xdp_adjust_head bits in struct bpf_prog,
that are used for requirement checks on the program rather than f.e.
heuristics. Thus, for tail calls, we cannot make any assumptions and
are forced to set them.

Thanks!

Daniel Borkmann (2):
  bpf: fix cb access in socket filter programs on tail calls
  bpf: fix checking xdp_adjust_head on tail calls

 kernel/bpf/syscall.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
1.9.3

[PATCH net 1/2] bpf: fix cb access in socket filter programs on tail calls

[Daniel Borkmann]

Commit ff936a04e5f2 ("bpf: fix cb access in socket filter programs")
added a fix for socket filter programs such that in i) AF_PACKET the
20 bytes of skb->cb[] area gets zeroed before use in order to not leak
data, and ii) socket filter programs attached to TCP/UDP sockets need
to save/restore these 20 bytes since they are also used by protocol
layers at that time.

The problem is that bpf_prog_run_save_cb() and bpf_prog_run_clear_cb()
only look at the actual attached program to determine whether to zero
or save/restore the skb->cb[] parts. There can be cases where the
actual attached program does not access the skb->cb[], but the program
tail calls into another program which does access this area. In such
a case, the zero or save/restore is currently not performed.

Since the programs we tail call into are unknown at verification time
and can dynamically change, we need to assume that whenever the attached
program performs a tail call, that later programs could access the
skb->cb[], and therefore we need to always set cb_access to 1.

Fixes: ff936a04e5f2 ("bpf: fix cb access in socket filter programs")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/syscall.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 7af0dcc..ee5c969 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -617,6 +617,13 @@ static void fixup_bpf_calls(struct bpf_prog *prog)
 			if (insn->imm == BPF_FUNC_xdp_adjust_head)
 				prog->xdp_adjust_head = 1;
 			if (insn->imm == BPF_FUNC_tail_call) {
+				/* If we tail call into other programs, we
+				 * cannot make any assumptions since they
+				 * can be replaced dynamically during runtime
+				 * in the program array.
+				 */
+				prog->cb_access = 1;
+
 				/* mark bpf_tail_call as different opcode
 				 * to avoid conditional branch in
 				 * interpeter for every normal call
-- 
1.9.3

[PATCH net 2/2] bpf: fix checking xdp_adjust_head on tail calls

[Daniel Borkmann]

Commit 17bedab27231 ("bpf: xdp: Allow head adjustment in XDP prog")
added the xdp_adjust_head bit to the BPF prog in order to tell drivers
that the program that is to be attached requires support for the XDP
bpf_xdp_adjust_head() helper such that drivers not supporting this
helper can reject the program. There are also drivers that do support
the helper, but need to check for xdp_adjust_head bit in order to move
packet metadata prepended by the firmware away for making headroom.

For these cases, the current check for xdp_adjust_head bit is insufficient
since there can be cases where the program itself does not use the
bpf_xdp_adjust_head() helper, but tail calls into another program that
uses bpf_xdp_adjust_head(). As such, the xdp_adjust_head bit is still
set to 0. Since the first program has no control over which program it
calls into, we need to assume that bpf_xdp_adjust_head() helper is used
upon tail calls. Thus, for the very same reasons in cb_access, set the
xdp_adjust_head bit to 1 when the main program uses tail calls.

Fixes: 17bedab27231 ("bpf: xdp: Allow head adjustment in XDP prog")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
Cc: Martin KaFai Lau <kafai@fb.com>
---
 kernel/bpf/syscall.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index ee5c969..821f9e8 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -623,6 +623,7 @@ static void fixup_bpf_calls(struct bpf_prog *prog)
 				 * in the program array.
 				 */
 				prog->cb_access = 1;
+				prog->xdp_adjust_head = 1;
 
 				/* mark bpf_tail_call as different opcode
 				 * to avoid conditional branch in
-- 
1.9.3

Re: [PATCH net 0/2] Two BPF fixes

[David Miller]

From: Daniel Borkmann <daniel@iogearbox.net>
Date: Mon, 17 Apr 2017 03:12:05 +0200

> The set fixes cb_access and xdp_adjust_head bits in struct bpf_prog,
> that are used for requirement checks on the program rather than f.e.
> heuristics. Thus, for tail calls, we cannot make any assumptions and
> are forced to set them.

Series applied, thanks.

Tail calls bring up all kinds of caching and assumption issues, see my
question in another thread about how register cached SKB parameters
are handled in JITs across tail calls.

[PATCH net 0/2] Two BPF fixes

[Daniel Borkmann]

This set contains two BPF fixes for net, one that addresses the
complaint from Geert wrt static allocations, and the other is a
fix wrt mem accounting that I found recently during testing.

Thanks!

Daniel Borkmann (2):
  bpf: dynamically allocate digest scratch buffer
  bpf: fix overflow in prog accounting

 include/linux/bpf.h    |  2 +-
 include/linux/filter.h | 17 ++++++++--
 kernel/bpf/core.c      | 88 ++++++++++++++++++++++++++++++++++++++++++--------
 kernel/bpf/syscall.c   | 27 +---------------
 kernel/bpf/verifier.c  |  6 ++--
 5 files changed, 94 insertions(+), 46 deletions(-)

-- 
1.9.3