[PATCH 1/2] [net-next] bpf: fix link error without CONFIG_NET

[PATCH 1/2] [net-next] bpf: fix link error without CONFIG_NET

[Arnd Bergmann]

I ran into this link error with the latest net-next plus linux-next
trees when networking is disabled:

kernel/bpf/verifier.o:(.rodata+0x2958): undefined reference to `tc_cls_act_analyzer_ops'
kernel/bpf/verifier.o:(.rodata+0x2970): undefined reference to `xdp_analyzer_ops'

It seems that the code was written to deal with varying contents of
the arrray, but the actual #ifdef was missing. Both tc_cls_act_analyzer_ops
and xdp_analyzer_ops are defined in the core networking code, so adding
a check for CONFIG_NET seems appropriate here, and I've verified this with
many randconfig builds

Fixes: 4f9218aaf8a4 ("bpf: move knowledge about post-translation offsets out of verifier")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
Since there hasn't been a linux-next release in two weeks, I'm not
entirely sure this is still needed, but from looking of the net-next
contents it seems it is. I did not check any other trees that might
have a fix already.
---
 kernel/bpf/verifier.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2bb6d6aa7085..750aff880ecb 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4561,8 +4561,10 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr)
 }
 
 static const struct bpf_verifier_ops * const bpf_analyzer_ops[] = {
+#ifdef CONFIG_NET
 	[BPF_PROG_TYPE_XDP]		= &xdp_analyzer_ops,
 	[BPF_PROG_TYPE_SCHED_CLS]	= &tc_cls_act_analyzer_ops,
+#endif
 };
 
 int bpf_analyzer(struct bpf_prog *prog, const struct bpf_ext_analyzer_ops *ops,
-- 
2.9.0

[PATCH 2/2] [net-next] bpf: fix out-of-bounds access warning in bpf_check

[Arnd Bergmann]

The bpf_verifer_ops array is generated dynamically and may be
empty depending on configuration, which then causes an out
of bounds access:

kernel/bpf/verifier.c: In function 'bpf_check':
kernel/bpf/verifier.c:4320:29: error: array subscript is above array bounds [-Werror=array-bounds]

This adds a check to the start of the function as a workaround.
I would assume that the function is never called in that configuration,
so the warning is probably harmless.

Fixes: 00176a34d9e2 ("bpf: remove the verifier ops from program structure")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
Since there hasn't been a linux-next release in two weeks, I'm not
entirely sure this is still needed, but from looking of the net-next
contents it seems it is. I did not check any other trees that might
have a fix already.
---
 kernel/bpf/verifier.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 750aff880ecb..debb60ad08ee 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4447,6 +4447,10 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr)
 	struct bpf_verifer_log *log;
 	int ret = -EINVAL;
 
+	/* no program is valid */
+	if (ARRAY_SIZE(bpf_verifier_ops) == 0)
+		return -EINVAL;
+
 	/* 'struct bpf_verifier_env' can be global, but since it's not small,
 	 * allocate/free it every time bpf_check() is called
 	 */
-- 
2.9.0

Re: [PATCH 2/2] [net-next] bpf: fix out-of-bounds access warning in bpf_check

[Alexei Starovoitov]

On Thu, Nov 02, 2017 at 12:05:52PM +0100, Arnd Bergmann wrote:
> The bpf_verifer_ops array is generated dynamically and may be
> empty depending on configuration, which then causes an out
> of bounds access:
> 
> kernel/bpf/verifier.c: In function 'bpf_check':
> kernel/bpf/verifier.c:4320:29: error: array subscript is above array bounds [-Werror=array-bounds]
> 
> This adds a check to the start of the function as a workaround.
> I would assume that the function is never called in that configuration,
> so the warning is probably harmless.
> 
> Fixes: 00176a34d9e2 ("bpf: remove the verifier ops from program structure")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> Since there hasn't been a linux-next release in two weeks, I'm not
> entirely sure this is still needed, but from looking of the net-next
> contents it seems it is. I did not check any other trees that might
> have a fix already.
> ---
>  kernel/bpf/verifier.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 750aff880ecb..debb60ad08ee 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4447,6 +4447,10 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr)
>  	struct bpf_verifer_log *log;
>  	int ret = -EINVAL;
>  
> +	/* no program is valid */
> +	if (ARRAY_SIZE(bpf_verifier_ops) == 0)
> +		return -EINVAL;

sorry I don't see how bpf_verifier_ops can be empty.
Did you mix it up with your previous patch when you made bpf_analyzer_ops empty?

Re: [PATCH 2/2] [net-next] bpf: fix out-of-bounds access warning in bpf_check

[Arnd Bergmann]

On Thu, Nov 2, 2017 at 4:59 PM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Thu, Nov 02, 2017 at 12:05:52PM +0100, Arnd Bergmann wrote:
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 750aff880ecb..debb60ad08ee 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -4447,6 +4447,10 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr)
>>       struct bpf_verifer_log *log;
>>       int ret = -EINVAL;
>>
>> +     /* no program is valid */
>> +     if (ARRAY_SIZE(bpf_verifier_ops) == 0)
>> +             return -EINVAL;
>
> sorry I don't see how bpf_verifier_ops can be empty.
> Did you mix it up with your previous patch when you made bpf_analyzer_ops empty?

I confused the two a couple of times while creating the patches, but
I'm still fairly
sure I got it right in the end:

bpf_verifier_ops is an array that gets generated by including linux/bpf_types.h.
That file has two kinds of entries:

- BPF_MAP_TYPE() entries are left out, as that macro is defined to an
empty string
  here.

- BPF_PROG_TYPE() entries are conditional depending on CONFIG_NET and
  CONFIG_BPF_EVENTS. In the configuration that produces the warning,
  both are disabled.

       Arnd

Re: [PATCH 2/2] [net-next] bpf: fix out-of-bounds access warning in bpf_check

[Jakub Kicinski]

On Thu, 2 Nov 2017 17:14:00 +0100, Arnd Bergmann wrote:
> On Thu, Nov 2, 2017 at 4:59 PM, Alexei Starovoitov wrote:
> > On Thu, Nov 02, 2017 at 12:05:52PM +0100, Arnd Bergmann wrote:  
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index 750aff880ecb..debb60ad08ee 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -4447,6 +4447,10 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr)
> >>       struct bpf_verifer_log *log;
> >>       int ret = -EINVAL;
> >>
> >> +     /* no program is valid */
> >> +     if (ARRAY_SIZE(bpf_verifier_ops) == 0)
> >> +             return -EINVAL;  
> >
> > sorry I don't see how bpf_verifier_ops can be empty.
> > Did you mix it up with your previous patch when you made bpf_analyzer_ops empty?  
> 
> I confused the two a couple of times while creating the patches, but
> I'm still fairly
> sure I got it right in the end:
> 
> bpf_verifier_ops is an array that gets generated by including linux/bpf_types.h.
> That file has two kinds of entries:
> 
> - BPF_MAP_TYPE() entries are left out, as that macro is defined to an
> empty string
>   here.
> 
> - BPF_PROG_TYPE() entries are conditional depending on CONFIG_NET and
>   CONFIG_BPF_EVENTS. In the configuration that produces the warning,
>   both are disabled.

Right.  My preferred fix was to add a NULL entry to the table so it's
never empty, but this is OK too.  Thanks!

Re: [PATCH 2/2] [net-next] bpf: fix out-of-bounds access warning in bpf_check

[Alexei Starovoitov]

On Thu, Nov 02, 2017 at 05:14:00PM +0100, Arnd Bergmann wrote:
> On Thu, Nov 2, 2017 at 4:59 PM, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > On Thu, Nov 02, 2017 at 12:05:52PM +0100, Arnd Bergmann wrote:
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index 750aff880ecb..debb60ad08ee 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -4447,6 +4447,10 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr)
> >>       struct bpf_verifer_log *log;
> >>       int ret = -EINVAL;
> >>
> >> +     /* no program is valid */
> >> +     if (ARRAY_SIZE(bpf_verifier_ops) == 0)
> >> +             return -EINVAL;
> >
> > sorry I don't see how bpf_verifier_ops can be empty.
> > Did you mix it up with your previous patch when you made bpf_analyzer_ops empty?
> 
> I confused the two a couple of times while creating the patches, but
> I'm still fairly
> sure I got it right in the end:
> 
> bpf_verifier_ops is an array that gets generated by including linux/bpf_types.h.
> That file has two kinds of entries:
> 
> - BPF_MAP_TYPE() entries are left out, as that macro is defined to an
> empty string
>   here.
> 
> - BPF_PROG_TYPE() entries are conditional depending on CONFIG_NET and
>   CONFIG_BPF_EVENTS. In the configuration that produces the warning,
>   both are disabled.

I see. Didn't realize that it's possible to enable bpf syscall
without networking and tracing support.
I'm thinking whether it's better to disallow such uselss mode in kconfig,
but it's probably going to be convoluted.
Above if (ARRAY_SIZE(bpf_verifier_ops) == 0) will be optimized away
by gcc in 99.9% of configs, so I guess that's fine, so:
Acked-by: Alexei Starovoitov <ast@kernel.org>

Re: [PATCH 2/2] [net-next] bpf: fix out-of-bounds access warning in bpf_check

[Daniel Borkmann]

On 11/02/2017 12:05 PM, Arnd Bergmann wrote:
> The bpf_verifer_ops array is generated dynamically and may be
> empty depending on configuration, which then causes an out
> of bounds access:
>
> kernel/bpf/verifier.c: In function 'bpf_check':
> kernel/bpf/verifier.c:4320:29: error: array subscript is above array bounds [-Werror=array-bounds]
>
> This adds a check to the start of the function as a workaround.
> I would assume that the function is never called in that configuration,
> so the warning is probably harmless.
>
> Fixes: 00176a34d9e2 ("bpf: remove the verifier ops from program structure")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

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

LGTM, and bpf_analyzer() already has proper logic to bail out for
such cases (although only used by nfp right now, which is there
when NET is configured anyway).

Re: [PATCH 2/2] [net-next] bpf: fix out-of-bounds access warning in bpf_check

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Thu,  2 Nov 2017 12:05:52 +0100

> The bpf_verifer_ops array is generated dynamically and may be
> empty depending on configuration, which then causes an out
> of bounds access:
> 
> kernel/bpf/verifier.c: In function 'bpf_check':
> kernel/bpf/verifier.c:4320:29: error: array subscript is above array bounds [-Werror=array-bounds]
> 
> This adds a check to the start of the function as a workaround.
> I would assume that the function is never called in that configuration,
> so the warning is probably harmless.
> 
> Fixes: 00176a34d9e2 ("bpf: remove the verifier ops from program structure")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Applied.

Re: [PATCH 1/2] [net-next] bpf: fix link error without CONFIG_NET

[Jakub Kicinski]

On Thu,  2 Nov 2017 12:05:51 +0100, Arnd Bergmann wrote:
> I ran into this link error with the latest net-next plus linux-next
> trees when networking is disabled:
> 
> kernel/bpf/verifier.o:(.rodata+0x2958): undefined reference to `tc_cls_act_analyzer_ops'
> kernel/bpf/verifier.o:(.rodata+0x2970): undefined reference to `xdp_analyzer_ops'
> 
> It seems that the code was written to deal with varying contents of
> the arrray, but the actual #ifdef was missing. Both tc_cls_act_analyzer_ops
> and xdp_analyzer_ops are defined in the core networking code, so adding
> a check for CONFIG_NET seems appropriate here, and I've verified this with
> many randconfig builds
> 
> Fixes: 4f9218aaf8a4 ("bpf: move knowledge about post-translation offsets out of verifier")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Thanks Arnd!  I was hoping to nuke this code before build bots catch up
to me, didn't work out :)

Re: [PATCH 1/2] [net-next] bpf: fix link error without CONFIG_NET

[Alexei Starovoitov]

On Thu, Nov 02, 2017 at 10:55:30AM -0700, Jakub Kicinski wrote:
> On Thu,  2 Nov 2017 12:05:51 +0100, Arnd Bergmann wrote:
> > I ran into this link error with the latest net-next plus linux-next
> > trees when networking is disabled:
> > 
> > kernel/bpf/verifier.o:(.rodata+0x2958): undefined reference to `tc_cls_act_analyzer_ops'
> > kernel/bpf/verifier.o:(.rodata+0x2970): undefined reference to `xdp_analyzer_ops'
> > 
> > It seems that the code was written to deal with varying contents of
> > the arrray, but the actual #ifdef was missing. Both tc_cls_act_analyzer_ops
> > and xdp_analyzer_ops are defined in the core networking code, so adding
> > a check for CONFIG_NET seems appropriate here, and I've verified this with
> > many randconfig builds
> > 
> > Fixes: 4f9218aaf8a4 ("bpf: move knowledge about post-translation offsets out of verifier")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> Thanks Arnd!  I was hoping to nuke this code before build bots catch up
> to me, didn't work out :)

yeah. Jakub's patches may not make it in time for net-next closing.
so let's use this fix for now.

Acked-by: Alexei Starovoitov <ast@kernel.org>

Re: [PATCH 1/2] [net-next] bpf: fix link error without CONFIG_NET

[Daniel Borkmann]

On 11/02/2017 12:05 PM, Arnd Bergmann wrote:
> I ran into this link error with the latest net-next plus linux-next
> trees when networking is disabled:
>
> kernel/bpf/verifier.o:(.rodata+0x2958): undefined reference to `tc_cls_act_analyzer_ops'
> kernel/bpf/verifier.o:(.rodata+0x2970): undefined reference to `xdp_analyzer_ops'
>
> It seems that the code was written to deal with varying contents of
> the arrray, but the actual #ifdef was missing. Both tc_cls_act_analyzer_ops
> and xdp_analyzer_ops are defined in the core networking code, so adding
> a check for CONFIG_NET seems appropriate here, and I've verified this with
> many randconfig builds
>
> Fixes: 4f9218aaf8a4 ("bpf: move knowledge about post-translation offsets out of verifier")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

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

Re: [PATCH 1/2] [net-next] bpf: fix link error without CONFIG_NET

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Thu,  2 Nov 2017 12:05:51 +0100

> I ran into this link error with the latest net-next plus linux-next
> trees when networking is disabled:
> 
> kernel/bpf/verifier.o:(.rodata+0x2958): undefined reference to `tc_cls_act_analyzer_ops'
> kernel/bpf/verifier.o:(.rodata+0x2970): undefined reference to `xdp_analyzer_ops'
> 
> It seems that the code was written to deal with varying contents of
> the arrray, but the actual #ifdef was missing. Both tc_cls_act_analyzer_ops
> and xdp_analyzer_ops are defined in the core networking code, so adding
> a check for CONFIG_NET seems appropriate here, and I've verified this with
> many randconfig builds
> 
> Fixes: 4f9218aaf8a4 ("bpf: move knowledge about post-translation offsets out of verifier")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Applied.