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

[David Miller]

From: Arnd Bergmann 
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 

Applied.

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 


Acked-by: Daniel Borkmann 

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

[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
>  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 

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

[Arnd Bergmann]

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.

   Arnd

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 
> ---
> 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?