[RFC net-next] bpf: taint loading !is_gpl programs

[RFC net-next] bpf: taint loading !is_gpl programs

[Aaron Conole]

The eBPF framework is used for more than just socket level filtering.  It
can also provide tracing, and even change the way packets coming into the
system look.  Most of the eBPF callable symbols are available to non-gpl
programs, and this includes helper functions which modify packets.  This
allows proprietary eBPF code to link to the kernel and make decisions
which can negatively impact network performance.

Since the sources for these programs are only available under a proprietary
license, it seems better to treat them the same as other proprietary
modules: set the system taint flag.  An exemption is made for socket-level
filters, since they do not really impact networking for the whole kernel.

Signed-off-by: Aaron Conole <aconole@bytheb.org>
---
 kernel/bpf/syscall.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index ab0cf4c4..1255b51 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -860,6 +860,11 @@ static int bpf_prog_load(union bpf_attr *attr)
 
 	bpf_prog_kallsyms_add(prog);
 	trace_bpf_prog_load(prog, err);
+	if (type != BPF_PROG_TYPE_SOCKET_FILTER && !is_gpl && !(err < 0)) {
+		if (!test_taint(TAINT_PROPRIETARY_MODULE))
+			pr_warn("bpf license '%s' taints kernel.\n", license);
+		add_taint(TAINT_PROPRIETARY_MODULE, LOCKDEP_STILL_OK);
+	}
 	return err;
 
 free_used_maps:
-- 
2.9.3

Re: [RFC net-next] bpf: taint loading !is_gpl programs

[Daniel Borkmann]

On 04/04/2017 08:33 PM, Aaron Conole wrote:
> The eBPF framework is used for more than just socket level filtering.  It
> can also provide tracing, and even change the way packets coming into the
> system look.  Most of the eBPF callable symbols are available to non-gpl
> programs, and this includes helper functions which modify packets.  This
> allows proprietary eBPF code to link to the kernel and make decisions
> which can negatively impact network performance.
>
> Since the sources for these programs are only available under a proprietary
> license, it seems better to treat them the same as other proprietary
> modules: set the system taint flag.  An exemption is made for socket-level
> filters, since they do not really impact networking for the whole kernel.
>
> Signed-off-by: Aaron Conole <aconole@bytheb.org>
> ---
>   kernel/bpf/syscall.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index ab0cf4c4..1255b51 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -860,6 +860,11 @@ static int bpf_prog_load(union bpf_attr *attr)
>
>   	bpf_prog_kallsyms_add(prog);
>   	trace_bpf_prog_load(prog, err);
> +	if (type != BPF_PROG_TYPE_SOCKET_FILTER && !is_gpl && !(err < 0)) {
> +		if (!test_taint(TAINT_PROPRIETARY_MODULE))
> +			pr_warn("bpf license '%s' taints kernel.\n", license);
> +		add_taint(TAINT_PROPRIETARY_MODULE, LOCKDEP_STILL_OK);
> +	}
>   	return err;
>
>   free_used_maps:
>

Nacked-by: Daniel Borkmann <daniel@iogearbox.net>

This is proposal completely unreasonable; what the purpose of .gpl_only
flags is agreed upon since the beginning is that some of the helpers
are only available if the program is loaded as gpl, f.e. bpf_ktime_get_ns(),
bpf_probe_read(), bpf_probe_write_user(), bpf_trace_printk(),
bpf_skb_event_output(), etc. Now, suddenly switching from one kernel
version to another, existing programs would out of a sudden taint the
kernel, which by itself is unacceptable. There are also many other
subsystems that can modify packets, or affect system performance
negatively if configured wrongly and which in addition *don't require* a
hard capable(CAP_SYS_ADMIN) restriction like such eBPF programs already
do, perhaps should we taint them as well? Plus tracing programs are
attached to passively monitor systems performance, not even modifying
data structures ... The current purpose of .gpl_only is fine as-is, and
there's work in progress for a generic dump mechanism that works with
all program types to improve introspection aspect if that's what you're
after, starting to taint is, in a way, breaking existing applications
and this is not acceptable.

Re: [RFC net-next] bpf: taint loading !is_gpl programs

[Aaron Conole]

Hi Daniel,

Daniel Borkmann <daniel@iogearbox.net> writes:

> On 04/04/2017 08:33 PM, Aaron Conole wrote:
>> The eBPF framework is used for more than just socket level filtering.  It
>> can also provide tracing, and even change the way packets coming into the
>> system look.  Most of the eBPF callable symbols are available to non-gpl
>> programs, and this includes helper functions which modify packets.  This
>> allows proprietary eBPF code to link to the kernel and make decisions
>> which can negatively impact network performance.
>>
>> Since the sources for these programs are only available under a proprietary
>> license, it seems better to treat them the same as other proprietary
>> modules: set the system taint flag.  An exemption is made for socket-level
>> filters, since they do not really impact networking for the whole kernel.
>>
>> Signed-off-by: Aaron Conole <aconole@bytheb.org>
>> ---
>
> Nacked-by: Daniel Borkmann <daniel@iogearbox.net>

Thanks so much for looking at the patch!

> This is proposal completely unreasonable; what the purpose of .gpl_only
> flags is agreed upon since the beginning is that some of the helpers
> are only available if the program is loaded as gpl, f.e. bpf_ktime_get_ns(),
> bpf_probe_read(), bpf_probe_write_user(), bpf_trace_printk(),
> bpf_skb_event_output(), etc.

This behavior isn't changing with this patch.

> Now, suddenly switching from one kernel
> version to another, existing programs would out of a sudden taint the
> kernel, which by itself is unacceptable.

I'm not sure what you mean here.  The kernel should still be usable.  This
basically says that if someone runs non-GPL eBPF code, they are tainting
the kernel.  More below.

> There are also many other
> subsystems that can modify packets, or affect system performance
> negatively if configured wrongly and which in addition *don't require* a
> hard capable(CAP_SYS_ADMIN) restriction like such eBPF programs already
> do, perhaps should we taint them as well?

This is a good point that there are other methods of doing damage to the
network.  I think it means my commit message wasn't clear enough to
describe why I wanted the change.  The reason I propose this isn't
because someone can theoretically damage things.  It's because eBPF
really is a way of writing specialized kernel modules.

I am really making the distinction here that eBPF code (except for the
case of user-space socket filter) is a kernel module.  I realize that
may not be something folks have considered.  Never-the-less, it is code
which runs in the context of the kernel, out lives the lifetime of user
space, and modifies kernel behavior.  These are the main reasons I
believe this is a kernel module.  And since it is a kernel module, it
shouldn't bypass the existing taint flag that says 'someone is running
non-gpl code in kernel space.'  Do you disagree?  This is also why I
exempt socket filter code.  That really is something which I would
consider running as part of a user-space application.

> Plus tracing programs are
> attached to passively monitor systems performance, not even modifying
> data structures

Tracing code, afaict, must be gpl_only = true to be useful.  So I don't see
how it enters into the equation.  Did I misread something?  Most, if not
all of the networking functions, are gpl_only = false.  This means the
community will have a difficult time supporting reports from this
system.  After all, there's no way to know exactly how this eBPF program
has changed packets in the network without a license to the code.

> ... The current purpose of .gpl_only is fine as-is, and
> there's work in progress for a generic dump mechanism that works with
> all program types to improve introspection aspect if that's what you're
> after, starting to taint is, in a way, breaking existing applications
> and this is not acceptable.

I don't see how this breaks applications.  They continue to run fine.
This patch does not restrict functionality.  Again, did I misunderstand
something?

I'm not sure how a dumping mechanism changes anything either.  I agree
such a utility is very useful.  However, if the poor user who is running
a non-gpl eBPF program is asked to provide a dump of that eBPF program,
they may be barred from doing so by licensing.  How can those cases be
supported?

Re: [RFC net-next] bpf: taint loading !is_gpl programs

[Alexei Starovoitov]

On Wed, Apr 05, 2017 at 10:59:49PM -0400, Aaron Conole wrote:
> Hi Daniel,
> 
> Daniel Borkmann <daniel@iogearbox.net> writes:
> 
> > On 04/04/2017 08:33 PM, Aaron Conole wrote:
> >> The eBPF framework is used for more than just socket level filtering.  It
> >> can also provide tracing, and even change the way packets coming into the
> >> system look.  Most of the eBPF callable symbols are available to non-gpl
> >> programs, and this includes helper functions which modify packets.  This
> >> allows proprietary eBPF code to link to the kernel and make decisions
> >> which can negatively impact network performance.
> >>
> >> Since the sources for these programs are only available under a proprietary
> >> license, it seems better to treat them the same as other proprietary
> >> modules: set the system taint flag.  An exemption is made for socket-level
> >> filters, since they do not really impact networking for the whole kernel.
> >>
> >> Signed-off-by: Aaron Conole <aconole@bytheb.org>
> >> ---
> >
> > Nacked-by: Daniel Borkmann <daniel@iogearbox.net>
> 
> Thanks so much for looking at the patch!

Passive aggressive way of saying 'I want to waste your time'. Not cool.

> > This is proposal completely unreasonable; what the purpose of .gpl_only
> > flags is agreed upon since the beginning is that some of the helpers
> > are only available if the program is loaded as gpl, f.e. bpf_ktime_get_ns(),
> > bpf_probe_read(), bpf_probe_write_user(), bpf_trace_printk(),
> > bpf_skb_event_output(), etc.
> 
> This behavior isn't changing with this patch.

It's breaking userspace, hence obvious Nack.

> all of the networking functions, are gpl_only = false.  This means the
> community will have a difficult time supporting reports from this
> system.  After all, there's no way to know exactly how this eBPF program
> has changed packets in the network without a license to the code.

bpf is the user space program. Whatever you mean by 'community'
is obviously cannot 'support' all possible user space applications
written using given kernel abi.
The reason tracing progs are gpl because they look into gpl-ed kernel,
networking progs don't. They deal with bytes on the wire.

Re: [RFC net-next] bpf: taint loading !is_gpl programs

[Aaron Conole]

Hi Alexei, and Daniel,

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Wed, Apr 05, 2017 at 10:59:49PM -0400, Aaron Conole wrote:
>> Hi Daniel,
>> 
>> Daniel Borkmann <daniel@iogearbox.net> writes:
>> 
>> > On 04/04/2017 08:33 PM, Aaron Conole wrote:
>> >> The eBPF framework is used for more than just socket level filtering.  It
>> >> can also provide tracing, and even change the way packets coming into the
>> >> system look.  Most of the eBPF callable symbols are available to non-gpl
>> >> programs, and this includes helper functions which modify packets.  This
>> >> allows proprietary eBPF code to link to the kernel and make decisions
>> >> which can negatively impact network performance.
>> >>
>> >> Since the sources for these programs are only available under a proprietary
>> >> license, it seems better to treat them the same as other proprietary
>> >> modules: set the system taint flag.  An exemption is made for socket-level
>> >> filters, since they do not really impact networking for the whole kernel.
>> >>
>> >> Signed-off-by: Aaron Conole <aconole@bytheb.org>
>> >> ---
>> >
>> > Nacked-by: Daniel Borkmann <daniel@iogearbox.net>

Given we have different views about this, I think I am okay with some
middle ground.

Here's the next-steps plan.  Please tell if you dislike it or want to
change it:

1. Add a ref counter for tracking load and unload, which can be queried
from a procfs or bpf fs interface

2. Add a new print during panic when the refcount is non-zero.

This lets us know that there could be some kind of ebpf program loaded,
and we would ask for sources before trying to disassemble.

Does this sound reasonable?

Re: [RFC net-next] bpf: taint loading !is_gpl programs

[Alexei Starovoitov]

On Fri, Apr 07, 2017 at 01:46:28PM -0400, Aaron Conole wrote:
> Hi Alexei, and Daniel,
> 
> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> 
> > On Wed, Apr 05, 2017 at 10:59:49PM -0400, Aaron Conole wrote:
> >> Hi Daniel,
> >> 
> >> Daniel Borkmann <daniel@iogearbox.net> writes:
> >> 
> >> > On 04/04/2017 08:33 PM, Aaron Conole wrote:
> >> >> The eBPF framework is used for more than just socket level filtering.  It
> >> >> can also provide tracing, and even change the way packets coming into the
> >> >> system look.  Most of the eBPF callable symbols are available to non-gpl
> >> >> programs, and this includes helper functions which modify packets.  This
> >> >> allows proprietary eBPF code to link to the kernel and make decisions
> >> >> which can negatively impact network performance.
> >> >>
> >> >> Since the sources for these programs are only available under a proprietary
> >> >> license, it seems better to treat them the same as other proprietary
> >> >> modules: set the system taint flag.  An exemption is made for socket-level
> >> >> filters, since they do not really impact networking for the whole kernel.
> >> >>
> >> >> Signed-off-by: Aaron Conole <aconole@bytheb.org>
> >> >> ---
> >> >
> >> > Nacked-by: Daniel Borkmann <daniel@iogearbox.net>
> 
> Given we have different views about this, I think I am okay with some
> middle ground.
> 
> Here's the next-steps plan.  Please tell if you dislike it or want to
> change it:
> 
> 1. Add a ref counter for tracking load and unload, which can be queried
> from a procfs or bpf fs interface
> 
> 2. Add a new print during panic when the refcount is non-zero.
> 
> This lets us know that there could be some kind of ebpf program loaded,
> and we would ask for sources before trying to disassemble.
> 
> Does this sound reasonable?

yeah. for the purpose of identifying whether any classic or extended bpf
programs loaded that makes sense. The only question how far we should
take it, since nft and acpi bytecode falls into the same category.
Also my understanding this is just one of out of many items on
redhat todo list to make bpf supported in rhel, so I think it makes
sense to discuss the whole list all at once. If we add patches here and
there without having full picture we may end up with obsolete api
or api superseded by other patches/features.
So let's discuss all feature requests first.