Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220711083220.2175036-1-asavkov@redhat.com> <20220711083220.2175036-4-asavkov@redhat.com>
In-Reply-To: <20220711083220.2175036-4-asavkov@redhat.com>
From:   Song Liu <song@kernel.org>
Date:   Tue, 12 Jul 2022 10:53:25 -0700
Message-ID: <CAPhsuW7xTRpLf1kyj5ejH0fV_aHCMQjUwn-uhWeNytXedh4+TQ@mail.gmail.com>
Subject: Re: [RFC PATCH bpf-next 3/4] bpf: add bpf_panic() helper
To:     Artem Savkov <asavkov@redhat.com>
Cc:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>, bpf <bpf@vger.kernel.org>,
        Networking <netdev@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        Andrea Arcangeli <aarcange@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Jul 11, 2022 at 1:32 AM Artem Savkov <asavkov@redhat.com> wrote:
>
> Add a helper that will make the kernel panic immediately with specified
> message. Using this helper requires kernel.destructive_bpf_enabled sysctl
> to be enabled, BPF_F_DESTRUCTIVE flag to be supplied on program load as
> well as CAP_SYS_BOOT capabilities.
>
> Signed-off-by: Artem Savkov <asavkov@redhat.com>
> ---
>  include/linux/bpf.h            |  1 +
>  include/uapi/linux/bpf.h       |  7 +++++++
>  kernel/bpf/core.c              |  1 +
>  kernel/bpf/helpers.c           | 13 +++++++++++++
>  kernel/bpf/verifier.c          |  7 +++++++
>  kernel/trace/bpf_trace.c       |  2 ++
>  tools/include/uapi/linux/bpf.h |  7 +++++++
>  7 files changed, 38 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 43c008e3587a..77c20ba9ca8e 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2339,6 +2339,7 @@ extern const struct bpf_func_proto bpf_strtol_proto;
>  extern const struct bpf_func_proto bpf_strtoul_proto;
>  extern const struct bpf_func_proto bpf_tcp_sock_proto;
>  extern const struct bpf_func_proto bpf_jiffies64_proto;
> +extern const struct bpf_func_proto bpf_panic_proto;
>  extern const struct bpf_func_proto bpf_get_ns_current_pid_tgid_proto;
>  extern const struct bpf_func_proto bpf_event_output_data_proto;
>  extern const struct bpf_func_proto bpf_ringbuf_output_proto;
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 4423874b5da4..e2e2c4de44ee 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3927,6 +3927,12 @@ union bpf_attr {
>   *     Return
>   *             The 64 bit jiffies
>   *
> + * void bpf_panic(const char *msg)
> + *     Description
> + *             Make the kernel panic immediately
> + *     Return
> + *             void
> + *
>   * long bpf_read_branch_records(struct bpf_perf_event_data *ctx, void *buf, u32 size, u64 flags)
>   *     Description
>   *             For an eBPF program attached to a perf event, retrieve the
> @@ -5452,6 +5458,7 @@ union bpf_attr {
>         FN(tcp_send_ack),               \
>         FN(send_signal_thread),         \
>         FN(jiffies64),                  \
> +       FN(panic),                      \
>         FN(read_branch_records),        \
>         FN(get_ns_current_pid_tgid),    \
>         FN(xdp_output),                 \
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index b5ffebcce6cc..0f333a0e85a5 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -2649,6 +2649,7 @@ const struct bpf_func_proto bpf_map_lookup_percpu_elem_proto __weak;
>  const struct bpf_func_proto bpf_spin_lock_proto __weak;
>  const struct bpf_func_proto bpf_spin_unlock_proto __weak;
>  const struct bpf_func_proto bpf_jiffies64_proto __weak;
> +const struct bpf_func_proto bpf_panic_proto __weak;
>
>  const struct bpf_func_proto bpf_get_prandom_u32_proto __weak;
>  const struct bpf_func_proto bpf_get_smp_processor_id_proto __weak;
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index a1c84d256f83..5cb90208a264 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -374,6 +374,19 @@ const struct bpf_func_proto bpf_jiffies64_proto = {
>         .ret_type       = RET_INTEGER,
>  };
>
> +BPF_CALL_1(bpf_panic, const char *, msg)
> +{
> +       panic(msg);

I think we should also check

   capable(CAP_SYS_BOOT) && destructive_ebpf_enabled()

here. Or at least, destructive_ebpf_enabled(). Otherwise, we
may trigger panic after the sysctl is disabled.

In general, I don't think sysctl is a good API, as it is global, and
the user can easily forget to turn it back off. If possible, I would
rather avoid adding new BPF related sysctls.

Thanks,
Song


> +       return 0;
> +}
> +
> +const struct bpf_func_proto bpf_panic_proto = {
> +       .func           = bpf_panic,
> +       .gpl_only       = false,
> +       .ret_type       = RET_VOID,
> +       .arg1_type      = ARG_PTR_TO_CONST_STR,
> +};
> +
>  #ifdef CONFIG_CGROUPS
>  BPF_CALL_0(bpf_get_current_cgroup_id)
>  {
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2859901ffbe3..f49c026917c5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -7285,6 +7285,13 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>                                 reg_type_str(env, regs[BPF_REG_1].type));
>                         return -EACCES;
>                 }
> +               break;
> +       case BPF_FUNC_panic:
> +               struct bpf_prog_aux *aux = env->prog->aux;
> +               if (!aux->destructive) {
> +                       verbose(env, "bpf_panic() calls require BPF_F_DESTRUCTIVE flag\n");
> +                       return -EACCES;
> +               }
>         }
>
>         if (err)
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 4be976cf7d63..3ee888507795 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1304,6 +1304,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>                 return &bpf_find_vma_proto;
>         case BPF_FUNC_trace_vprintk:
>                 return bpf_get_trace_vprintk_proto();
> +       case BPF_FUNC_panic:
> +               return capable(CAP_SYS_BOOT) && destructive_ebpf_enabled() ? &bpf_panic_proto : NULL;
>         default:
>                 return bpf_base_func_proto(func_id);
>         }
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 4423874b5da4..e2e2c4de44ee 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -3927,6 +3927,12 @@ union bpf_attr {
>   *     Return
>   *             The 64 bit jiffies
>   *
> + * void bpf_panic(const char *msg)
> + *     Description
> + *             Make the kernel panic immediately
> + *     Return
> + *             void
> + *
>   * long bpf_read_branch_records(struct bpf_perf_event_data *ctx, void *buf, u32 size, u64 flags)
>   *     Description
>   *             For an eBPF program attached to a perf event, retrieve the
> @@ -5452,6 +5458,7 @@ union bpf_attr {
>         FN(tcp_send_ack),               \
>         FN(send_signal_thread),         \
>         FN(jiffies64),                  \
> +       FN(panic),                      \
>         FN(read_branch_records),        \
>         FN(get_ns_current_pid_tgid),    \
>         FN(xdp_output),                 \
> --
> 2.35.3
>