[PATCH bpf] bpftool: Don't crash on missing jited insns or ksyms

[PATCH bpf] bpftool: Don't crash on missing jited insns or ksyms

[Toke Høiland-Jørgensen]

When JIT hardening is turned on, the kernel can fail to return jited_ksyms
or jited_prog_insns, but still have positive values in nr_jited_ksyms and
jited_prog_len. This causes bpftool to crash when trying to dump the
program because it only checks the len fields not the actual pointers to
the instructions and ksyms.

Fix this by adding the missing checks.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/bpf/bpftool/prog.c          | 2 +-
 tools/bpf/bpftool/xlated_dumper.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 4535c863d2cd..2ce9c5ba1934 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -493,7 +493,7 @@ static int do_dump(int argc, char **argv)
 
 	info = &info_linear->info;
 	if (mode == DUMP_JITED) {
-		if (info->jited_prog_len == 0) {
+		if (info->jited_prog_len == 0 || !info->jited_prog_insns) {
 			p_info("no instructions returned");
 			goto err_free;
 		}
diff --git a/tools/bpf/bpftool/xlated_dumper.c b/tools/bpf/bpftool/xlated_dumper.c
index 494d7ae3614d..5b91ee65a080 100644
--- a/tools/bpf/bpftool/xlated_dumper.c
+++ b/tools/bpf/bpftool/xlated_dumper.c
@@ -174,7 +174,7 @@ static const char *print_call(void *private_data,
 	struct kernel_sym *sym;
 
 	if (insn->src_reg == BPF_PSEUDO_CALL &&
-	    (__u32) insn->imm < dd->nr_jited_ksyms)
+	    (__u32) insn->imm < dd->nr_jited_ksyms && dd->jited_ksyms)
 		address = dd->jited_ksyms[insn->imm];
 
 	sym = kernel_syms_search(dd, address);
-- 
2.24.0

Re: [PATCH bpf] bpftool: Don't crash on missing jited insns or ksyms

[Martin Lau]

On Tue, Dec 10, 2019 at 03:30:47PM +0100, Toke Høiland-Jørgensen wrote:
> When JIT hardening is turned on, the kernel can fail to return jited_ksyms
JIT hardening means net.core.bpf_jit_harden?
From the code, it happens on the bpf_dump_raw_ok() check which is
actually "kernel.kptr_restrict" instead?

> or jited_prog_insns, but still have positive values in nr_jited_ksyms and
> jited_prog_len. This causes bpftool to crash when trying to dump the
> program because it only checks the len fields not the actual pointers to
> the instructions and ksyms.
> 
> Fix this by adding the missing checks.
Changes look good.

> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  tools/bpf/bpftool/prog.c          | 2 +-
>  tools/bpf/bpftool/xlated_dumper.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 4535c863d2cd..2ce9c5ba1934 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -493,7 +493,7 @@ static int do_dump(int argc, char **argv)
>  
>  	info = &info_linear->info;
>  	if (mode == DUMP_JITED) {
> -		if (info->jited_prog_len == 0) {
> +		if (info->jited_prog_len == 0 || !info->jited_prog_insns) {
>  			p_info("no instructions returned");
>  			goto err_free;
>  		}
> diff --git a/tools/bpf/bpftool/xlated_dumper.c b/tools/bpf/bpftool/xlated_dumper.c
> index 494d7ae3614d..5b91ee65a080 100644
> --- a/tools/bpf/bpftool/xlated_dumper.c
> +++ b/tools/bpf/bpftool/xlated_dumper.c
> @@ -174,7 +174,7 @@ static const char *print_call(void *private_data,
>  	struct kernel_sym *sym;
>  
>  	if (insn->src_reg == BPF_PSEUDO_CALL &&
> -	    (__u32) insn->imm < dd->nr_jited_ksyms)
> +	    (__u32) insn->imm < dd->nr_jited_ksyms && dd->jited_ksyms)
>  		address = dd->jited_ksyms[insn->imm];
>  
>  	sym = kernel_syms_search(dd, address);
> -- 
> 2.24.0
> 

Re: [PATCH bpf] bpftool: Don't crash on missing jited insns or ksyms

[Toke Høiland-Jørgensen]

Martin Lau <kafai@fb.com> writes:

> On Tue, Dec 10, 2019 at 03:30:47PM +0100, Toke Høiland-Jørgensen wrote:
>> When JIT hardening is turned on, the kernel can fail to return jited_ksyms
> JIT hardening means net.core.bpf_jit_harden?
> From the code, it happens on the bpf_dump_raw_ok() check which is
> actually "kernel.kptr_restrict" instead?

Ah, yeah, you're right. I was looking through the hardening patchset and
the bpf_jit_harden setting was the first thing I hit upon; must admit I
didn't check this too closely :)

I'll send a v2 with an updated commit message...

-Toke