[bpf,v2,0/8] bpf: track changes_pkt_data property for global functions

Message

Eduard Zingerman Dec. 10, 2024, 4:10 a.m. UTC

Nick Zavaritsky reported [0] a bug in verifier, where the following
unsafe program is not rejected:

    __attribute__((__noinline__))
    long skb_pull_data(struct __sk_buff *sk, __u32 len)
    {
        return bpf_skb_pull_data(sk, len);
    }

    SEC("tc")
    int test_invalidate_checks(struct __sk_buff *sk)
    {
        int *p = (void *)(long)sk->data;
        if ((void *)(p + 1) > (void *)(long)sk->data_end) return TCX_DROP;
        skb_pull_data(sk, 0);
        /* not safe, p is invalid after bpf_skb_pull_data call */
        *p = 42;
        return TCX_PASS;
    }

This happens because verifier does not track package invalidation
effect of global sub-programs.

This patch-set fixes the issue by modifying check_cfg() to compute
whether or not each sub-program calls (directly or indirectly)
helper invalidating packet pointers.

As global functions could be replaced with extension programs,
a new field 'changes_pkt_data' is added to struct bpf_prog_aux.
Verifier only allows replacing functions that do not change packet
data with functions that do not change packet data.

In case if there is a need to a have a global function that does not
change packet data, but allow replacing it with function that does,
the recommendation is to add a noop call to a helper, e.g.:
- for skb do 'bpf_skb_change_proto(skb, 0, 0)';
- for xdp do 'bpf_xdp_adjust_meta(xdp, 0)'.

Functions also can do tail calls. Effects of the tail call cannot be
analyzed before-hand, thus verifier assumes that tail calls always
change packet data.

Changes v1 [1] -> v2:
- added handling of extension programs and tail calls
  (thanks, Alexei, for all the input).

[0] https://lore.kernel.org/bpf/[e-mail]/
[1] https://lore.kernel.org/bpf/[e-mail]/

Eduard Zingerman (8):
  bpf: add find_containing_subprog() utility function
  bpf: refactor bpf_helper_changes_pkt_data to use helper number
  bpf: track changes_pkt_data property for global functions
  selftests/bpf: test for changing packet data from global functions
  bpf: check changes_pkt_data property for extension programs
  selftests/bpf: freplace tests for tracking of changes_packet_data
  bpf: consider that tail calls invalidate packet pointers
  selftests/bpf: validate that tail call invalidates packet pointers

 include/linux/bpf.h                           |  1 +
 include/linux/bpf_verifier.h                  |  1 +
 include/linux/filter.h                        |  2 +-
 kernel/bpf/core.c                             |  2 +-
 kernel/bpf/verifier.c                         | 78 ++++++++++++++++---
 net/core/filter.c                             | 65 +++++++---------
 .../bpf/prog_tests/changes_pkt_data.c         | 76 ++++++++++++++++++
 .../selftests/bpf/progs/changes_pkt_data.c    | 26 +++++++
 .../bpf/progs/changes_pkt_data_freplace.c     | 18 +++++
 .../testing/selftests/bpf/progs/tc_bpf2bpf.c  |  2 +
 .../selftests/bpf/progs/verifier_sock.c       | 56 +++++++++++++
 11 files changed, 280 insertions(+), 47 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/changes_pkt_data.c
 create mode 100644 tools/testing/selftests/bpf/progs/changes_pkt_data.c
 create mode 100644 tools/testing/selftests/bpf/progs/changes_pkt_data_freplace.c

Comments

[e-mail] Dec. 10, 2024, 6:40 p.m. UTC | #1

Hello:

This series was applied to bpf/bpf.git (master)
by Alexei Starovoitov <[e-mail]>:

On Mon,  9 Dec 2024 20:10:52 -0800 you wrote:
> Nick Zavaritsky reported [0] a bug in verifier, where the following
> unsafe program is not rejected:
> 
>     __attribute__((__noinline__))
>     long skb_pull_data(struct __sk_buff *sk, __u32 len)
>     {
>         return bpf_skb_pull_data(sk, len);
>     }
> 
> [...]

Here is the summary with links:
  - [bpf,v2,1/8] bpf: add find_containing_subprog() utility function
    https://git.kernel.org/bpf/bpf/c/27e88bc4df1d
  - [bpf,v2,2/8] bpf: refactor bpf_helper_changes_pkt_data to use helper number
    https://git.kernel.org/bpf/bpf/c/b238e187b4a2
  - [bpf,v2,3/8] bpf: track changes_pkt_data property for global functions
    https://git.kernel.org/bpf/bpf/c/51081a3f25c7
  - [bpf,v2,4/8] selftests/bpf: test for changing packet data from global functions
    https://git.kernel.org/bpf/bpf/c/3f23ee5590d9
  - [bpf,v2,5/8] bpf: check changes_pkt_data property for extension programs
    https://git.kernel.org/bpf/bpf/c/81f6d0530ba0
  - [bpf,v2,6/8] selftests/bpf: freplace tests for tracking of changes_packet_data
    https://git.kernel.org/bpf/bpf/c/89ff40890d8f
  - [bpf,v2,7/8] bpf: consider that tail calls invalidate packet pointers
    https://git.kernel.org/bpf/bpf/c/1a4607ffba35
  - [bpf,v2,8/8] selftests/bpf: validate that tail call invalidates packet pointers
    https://git.kernel.org/bpf/bpf/c/d9706b56e13b

You are awesome, thank you!