Re: [PATCH 1/2] bpf: add a bpf_override_function helper

[Josef Bacik]

On Sun, Nov 12, 2017 at 11:38:24AM +0100, Ingo Molnar wrote:
> 
> * Alexei Starovoitov <ast@xxxxxx> wrote:
> 
> > > One of the major advantages of having an in-kernel BPF sandbox is to never 
> > > crash the kernel - and allowing BPF programs to just randomly modify the 
> > > return value of kernel functions sounds immensely broken to me.
> > > 
> > > (And yes, I realize that kprobes are used here as a vehicle, but the point 
> > > remains.)
> > 
> > yeah. modifying arbitrary function return pushes bpf outside of
> > its safety guarantees and in that sense doing the same
> > override_return could be done from a kernel module if kernel
> > provides the x64 side of the facility introduced by this patch.
> > On the other side adding parts of this feature to the kernel only
> > to be used by external kernel module is quite ugly too and not
> > something that was ever done before.
> > How about we restrict this bpf_override_return() only to the functions
> > which callers expect to handle errors ?
> > We can add something similar to NOKPROBE_SYMBOL(). Like
> > ALLOW_RETURN_OVERRIDE() and on btrfs side mark the functions
> > we're going to test with this feature.
> >
> > 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,

Josef