| Message ID | 20171102110558.2746221-2-arnd@arndb.de |
|---|---|
| State | Accepted |
| Commit | eba0c929d1d0f16c4b03628b7bf8ce363b9e5c9a |
| Headers | show |
| Series | [1/2,net-next] bpf: fix link error without CONFIG_NET | expand |
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?
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
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!
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>
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).
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.
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 */
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(+) -- 2.9.0