Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 12 Nov 2017 11:38:24 +0100
From:   Ingo Molnar <mingo@kernel.org>
To:     Alexei Starovoitov <ast@fb.com>
Cc:     Josef Bacik <josef@toxicpanda.com>, rostedt@goodmis.org,
        mingo@redhat.com, davem@davemloft.net, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, ast@kernel.org, kernel-team@fb.com,
        daniel@iogearbox.net, Josef Bacik <jbacik@fb.com>
Subject: Re: [PATCH 1/2] bpf: add a bpf_override_function helper
Message-ID: <20171112103824.433mm7caxsuhoj2g@gmail.com>
References: <1510086523-8859-1-git-send-email-josef@toxicpanda.com>
 <1510086523-8859-2-git-send-email-josef@toxicpanda.com>
 <20171110093459.w2pvo3ntkwbmgnha@gmail.com>
 <20171110171428.hrw5cpxy4sgzf7mn@destiny>
 <20171111081455.qx4rodxldofbzypb@gmail.com>
 <23fd1b7a-5c7d-8b11-adc5-7e6679b6e61e@fb.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <23fd1b7a-5c7d-8b11-adc5-7e6679b6e61e@fb.com>
User-Agent: NeoMutt/20170609 (1.8.3)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


* Alexei Starovoitov <ast@fb.com> 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.

Thanks,

	Ingo

From 1583842032885975307@xxx Sun Nov 12 06:51:08 +0000 2017
X-GM-THRID: 1583463755336960738
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread