netdev - Re: [PATCH bpf] bpf: search_bpf_extables should search subprogram extables

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20230607210401.GB2023@templeofstupid.com>
Date: Wed, 7 Jun 2023 14:04:01 -0700
From: Krister Johansen <kjlx@...pleofstupid.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	John Fastabend <john.fastabend@...il.com>,
	Andrii Nakryiko <andrii@...nel.org>,
	Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu <song@...nel.org>,
	Yonghong Song <yhs@...com>, KP Singh <kpsingh@...nel.org>,
	Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>,
	Jiri Olsa <jolsa@...nel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>,
	Jesper Dangaard Brouer <hawk@...nel.org>,
	Nathan Chancellor <nathan@...nel.org>,
	Nick Desaulniers <ndesaulniers@...gle.com>,
	Tom Rix <trix@...hat.com>, LKML <linux-kernel@...r.kernel.org>,
	Network Development <netdev@...r.kernel.org>,
	clang-built-linux <llvm@...ts.linux.dev>,
	stable <stable@...r.kernel.org>
Subject: Re: [PATCH bpf] bpf: search_bpf_extables should search subprogram
 extables

On Mon, Jun 05, 2023 at 06:31:57PM -0700, Alexei Starovoitov wrote:
> On Mon, Jun 5, 2023 at 5:46 PM Krister Johansen <kjlx@...pleofstupid.com> wrote:
> > With your comments in mind, I took
> > another look at the ksym fields in the aux structs.  I have this in the
> > main program:
> >
> >   ksym = {
> >     start = 18446744072638420852,
> >     end = 18446744072638423040,
> >     name = <...>
> >     lnode = {
> >       next = 0xffff88d9c1065168,
> >       prev = 0xffff88da91609168
> >     },
> >     tnode = {
> >       node = {{
> >           __rb_parent_color = 18446613068361611640,
> >           rb_right = 0xffff88da91609178,
> >           rb_left = 0xffff88d9f0c5a578
> >         }, {
> >           __rb_parent_color = 18446613068361611664,
> >           rb_right = 0xffff88da91609190,
> >           rb_left = 0xffff88d9f0c5a590
> >         }}
> >     },
> >     prog = true
> >   },
> >
> > and this in the func[0] subprogram:
> >
> >   ksym = {
> >     start = 18446744072638420852,
> >     end = 18446744072638423040,
> >     name = <...>
> >     lnode = {
> >       next = 0xffff88da91609168,
> >       prev = 0xffffffff981f8990 <bpf_kallsyms>
> >     },
> >     tnode = {
> >       node = {{
> >           __rb_parent_color = 18446613068361606520,
> >           rb_right = 0x0,
> >           rb_left = 0x0
> >         }, {
> >           __rb_parent_color = 18446613068361606544,
> >           rb_right = 0x0,
> >           rb_left = 0x0
> >         }}
> >     },
> >     prog = true
> >   },
> >
> > That sure looks like func[0] is a leaf in the rbtree and the main
> > program is an intermediate node with leaves.  If that's the case, then
> > bpf_prog_ksym_find may have found the main program instead of the
> > subprogram.  In that case, do you think it's better to skip the main
> > program's call to bpf_prog_ksym_set_addr() if it has subprograms instead
> > of searching for subprograms if the main program is found?
> 
> I see.
> Looks like we're doing double bpf_prog_kallsyms_add().
> First in in jit_subprogs():
>         for (i = 0; i < env->subprog_cnt; i++) {
>                 bpf_prog_lock_ro(func[i]);
>                 bpf_prog_kallsyms_add(func[i]);
>         }
> and then again:
> bpf_prog_kallsyms_add(prog);
> in bpf_prog_load().
> 
> because func[0] is the main prog.
> 
> We are also doing double bpf_prog_lock_ro() for main prog,
> but that's not causing harm.
> 
> The fix is probably just this:
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1e38584d497c..89266dac9c12 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -17633,7 +17633,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
>         /* finally lock prog and jit images for all functions and
>          * populate kallsysm
>          */
> -       for (i = 0; i < env->subprog_cnt; i++) {
> +       for (i = 1; i < env->subprog_cnt; i++) {
>                 bpf_prog_lock_ro(func[i]);
>                 bpf_prog_kallsyms_add(func[i]);
>         }

This will cause the oops to always occur, because func[0] has a extable
entry when jit_subporgs() completes, but prog->aux doesn't.
jit_subprogs also sets prog->bpf_func which prevents the other copy of
the main program from getting jit'd, and consequently getting an extable
assigned.

There are probably a few options to fix:

1. skip the bpf_prog_kallsyms_add in bpf_prog_load if the program being
loaded has subprograms

2. check extables when searching to see if they're NULL and if the
subprogram has one instead

3. copy the main program's extable back to prog->aux

I'll send out a v2 here shortly that includes the selftest you
requested.  It takes approach #3, which is also a 1-line change.

-K