linux-kernel - Re: [PATCH 1/2] bpf: add a bpf_override

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] [day] [month] [year] [list]

Message-ID: <20171115073456.2dx4l2onbxn3ekzu@gmail.com>
Date:   Wed, 15 Nov 2017 08:34:56 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Josef Bacik <josef@...icpanda.com>
Cc:     Alexei Starovoitov <ast@...com>, rostedt@...dmis.org,
        mingo@...hat.com, davem@...emloft.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, ast@...nel.org, kernel-team@...com,
        daniel@...earbox.net, Josef Bacik <jbacik@...com>
Subject: Re: [PATCH 1/2] bpf: add a bpf_override_function helper


* Josef Bacik <josef@...icpanda.com> wrote:

> > > Then 'not crashing kernel' requirement will be preserved.
> > > btrfs or whatever else we will be testing with override_return
> > > will be functioning in 'stress test' mode and if bpf program
> > > is not careful and returns error all the time then one particular
> > > subsystem (like btrfs) will not be functional, but the kernel
> > > will not be crashing.
> > > Thoughts?
> > 
> > Yeah, that approach sounds much better to me: it should be fundamentally be 
> > opt-in, and should be documented that it should not be possible to crash the 
> > kernel via changing the return value.
> > 
> > I'd make it a bit clearer in the naming what the purpose of the annotation is: for 
> > example would BPF_ALLOW_ERROR_INJECTION() work for you guys? I.e. I think it 
> > should generally be used to change actual integer error values - or at most user 
> > pointers, but not kernel pointers. Not enforced in a type safe manner, but the 
> > naming should give enough hints?
> > 
> > Such return-injection BFR programs can still totally confuse user-space obviously: 
> > for example returning an IO error could corrupt application data - but that's the 
> > nature of such facilities and similar results could already be achieved via ptrace 
> > as well. But the result of a BPF program should never be _worse_ than ptrace, in 
> > terms of kernel integrity.
> > 
> > Note that with such a safety mechanism in place no kernel message has to be 
> > generated either I suspect.
> > 
> > In any case, my NAK would be lifted with such an approach.
> 
> I'm going to want to annotate kmalloc, so it's still going to be possible to
> make things go horribly wrong, is this still going to be ok with you?  Obviously
> I want to use this for btrfs, but really what I used this for originally was an
> NBD problem where I had to do special handling for getting EINTR back from
> kernel_sendmsg, which was a pain to trigger properly without this patch.  Opt-in
> is going to make it so we're just flagging important function calls anwyay
> because those are the ones that fail rarely and that we want to test, which puts
> us back in the same situation you are worried about, so it doesn't make much
> sense to me to do it this way.  Thanks,

I suppose - let's see how it goes? The important factor is the opt-in aspect I 
believe.

Technically the kernel should never crash on a kmalloc() failure either, although 
obviously things can go horribly wrong from user-space's perspective.

Thanks,

	Ingo