linux-kernel - Re: [RFC PATCH bpf-next 3/4] bpf: add bpf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YtFjCSR8YiK8E13J@samus.usersys.redhat.com>
Date:   Fri, 15 Jul 2022 14:52:25 +0200
From:   Artem Savkov <asavkov@...hat.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Song Liu <song@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        Andrea Arcangeli <aarcange@...hat.com>, dvacek@...hat.com
Subject: Re: [RFC PATCH bpf-next 3/4] bpf: add bpf_panic() helper

On Wed, Jul 13, 2022 at 03:20:22PM -0700, Alexei Starovoitov wrote:
> On Wed, Jul 13, 2022 at 6:31 AM Artem Savkov <asavkov@...hat.com> wrote:
> >
> > On Tue, Jul 12, 2022 at 11:08:54AM -0700, Alexei Starovoitov wrote:
> > > On Tue, Jul 12, 2022 at 10:53 AM Song Liu <song@...nel.org> wrote:
> > > >
> > > > >
> > > > > +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.
> > >
> > > +1. New syscal isn't warranted here.
> > > Just CAP_SYS_BOOT would be enough here.
> >
> > Point taken, I'll remove sysctl knob in any further versions.
> >
> > > Also full blown panic() seems unnecessary.
> > > If the motivation is to get a memory dump then crash_kexec() helper
> > > would be more suitable.
> > > If the goal is to reboot the system then the wrapper of sys_reboot()
> > > is better.
> > > Unfortunately the cover letter lacks these details.
> >
> > The main goal is to get the memory dump, so crash_kexec() should be enough.
> > However panic() is a bit more versatile and it's consequences are configurable
> > to some extent. Are there any downsides to using it?
> 
> versatile? In what sense? That it does a lot more than kexec?
> That's a disadvantage.
> We should provide bpf with minimal building blocks and let
> bpf program decide what to do.
> If dmesg (that is part of panic) is useful it should be its
> own kfunc.
> If halt is necessary -> separate kfunc as well.
> reboot -> another kfunc.
> 
> Also panic() is not guaranteed to do kexec and just
> panic is not what you stated is the goal of the helper.

Alright, if the aim is to provide the smallest building blocks then
crash_kexec() is a better choice.

> >
> > > Why this destructive action cannot be delegated to user space?
> >
> > Going through userspace adds delays and makes it impossible to hit "exactly
> > the right moment" thus making it unusable in most cases.
> 
> What would be an example of that?
> kexec is not instant either.

With kexec at least the thread it got called in is in a proper state. I
guess it is possible to achieve this by signalling userspace to do
kexec/panic and then block the thread somehow but that won't work in a
single-cpu case. Or am I missing something?

-- 
 Artem