[2/2,net-next] bpf: fix out-of-bounds access warning in bpf_check
diff mbox series

Message ID 20171102110558.2746221-2-arnd@arndb.de
State New, archived
Headers show
Series
  • [1/2,net-next] bpf: fix link error without CONFIG_NET
Related show

Commit Message

Arnd Bergmann Nov. 2, 2017, 11:05 a.m. UTC
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(+)

Comments

Alexei Starovoitov Nov. 2, 2017, 3:59 p.m. UTC | #1
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?
Arnd Bergmann Nov. 2, 2017, 4:14 p.m. UTC | #2
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
Jakub Kicinski Nov. 2, 2017, 5:58 p.m. UTC | #3
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!
Alexei Starovoitov Nov. 2, 2017, 6:47 p.m. UTC | #4
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>
Daniel Borkmann Nov. 2, 2017, 10:35 p.m. UTC | #5
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).
David Miller Nov. 3, 2017, 5:20 a.m. UTC | #6
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.

Patch
diff mbox series

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
 	 */