| Message ID | 20220309123321.2400262-3-houtao1@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | fixes for bpf_jit_harden race | expand |
On 3/9/22 1:33 PM, Hou Tao wrote: > It will be used to do cleanup for subprog which has been jited in first > pass but extra pass has not been done. The scenario is possible when > extra pass for subprog in the middle fails. The failure may lead to > oops due to inconsistent status for pack allocator (e.g. ro_hdr->size > and use_bpf_prog_pack) and memory leak in aux->jit_data. > > For x86-64, bpf_int_jit_abort() will free allocated memories saved in > aux->jit_data and fall back to interpreter mode to bypass the calling > of bpf_jit_binary_pack_free() in bpf_jit_free(). > > Signed-off-by: Hou Tao <houtao1@huawei.com> > --- > arch/x86/net/bpf_jit_comp.c | 24 ++++++++++++++++++++++++ > include/linux/filter.h | 1 + > kernel/bpf/core.c | 9 +++++++++ > kernel/bpf/verifier.c | 3 +++ > 4 files changed, 37 insertions(+) > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > index ec3f00be2ac5..49bc0ddd55ae 100644 > --- a/arch/x86/net/bpf_jit_comp.c > +++ b/arch/x86/net/bpf_jit_comp.c > @@ -2244,6 +2244,30 @@ struct x64_jit_data { > struct jit_context ctx; > }; > > +void bpf_int_jit_abort(struct bpf_prog *prog) > +{ > + struct x64_jit_data *jit_data = prog->aux->jit_data; > + struct bpf_binary_header *header, *rw_header; > + > + if (!jit_data) > + return; > + > + prog->bpf_func = NULL; > + prog->jited = 0; > + prog->jited_len = 0; > + > + header = jit_data->header; > + rw_header = jit_data->rw_header; > + bpf_arch_text_copy(&header->size, &rw_header->size, > + sizeof(rw_header->size)); > + bpf_jit_binary_pack_free(header, rw_header); > + > + kvfree(jit_data->addrs); > + kfree(jit_data); > + > + prog->aux->jit_data = NULL; > +} > + > #define MAX_PASSES 20 > #define PADDING_PASSES (MAX_PASSES - 5) > > diff --git a/include/linux/filter.h b/include/linux/filter.h > index 9bf26307247f..f3a913229edd 100644 > --- a/include/linux/filter.h > +++ b/include/linux/filter.h > @@ -945,6 +945,7 @@ u64 __bpf_call_base(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5); > (void *)__bpf_call_base) > > struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog); > +void bpf_int_jit_abort(struct bpf_prog *prog); > void bpf_jit_compile(struct bpf_prog *prog); > bool bpf_jit_needs_zext(void); > bool bpf_jit_supports_kfunc_call(void); > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > index ab630f773ec1..a1841e11524c 100644 > --- a/kernel/bpf/core.c > +++ b/kernel/bpf/core.c > @@ -2636,6 +2636,15 @@ struct bpf_prog * __weak bpf_int_jit_compile(struct bpf_prog *prog) > return prog; > } > > +/* > + * If arch JIT uses aux->jit_data to save temporary allocated status and > + * supports subprog, it needs to override the function to free allocated > + * memories and fall back to interpreter mode for passed prog. > + */ > +void __weak bpf_int_jit_abort(struct bpf_prog *prog) > +{ > +} > + > /* Stub for JITs that support eBPF. All cBPF code gets transformed into > * eBPF by the kernel and is later compiled by bpf_int_jit_compile(). > */ > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index e34264200e09..885e515cf83f 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -13086,6 +13086,9 @@ static int jit_subprogs(struct bpf_verifier_env *env) > if (tmp != func[i] || func[i]->bpf_func != old_bpf_func) { > verbose(env, "JIT doesn't support bpf-to-bpf calls\n"); > err = -ENOTSUPP; > + /* Abort extra pass for the remaining subprogs */ > + while (++i < env->subprog_cnt) > + bpf_int_jit_abort(func[i]); Don't quite follow this one. For example, if we'd fail in the second pass, the goto out_addrs from jit would free and clear the prog->aux->jit_data. If we'd succeed but different prog is returned, prog->aux->jit_data is released and later the goto out_free in here would clear the jited prog via bpf_jit_free(). Which code path leaves prog->aux->jit_data as non-NULL such that extra bpf_int_jit_abort() is needed? > goto out_free; > } > cond_resched(); >
On 3/12/22 12:54 AM, Daniel Borkmann wrote: [...] > Don't quite follow this one. For example, if we'd fail in the second pass, the > goto out_addrs from jit would free and clear the prog->aux->jit_data. If we'd succeed > but different prog is returned, prog->aux->jit_data is released and later the goto > out_free in here would clear the jited prog via bpf_jit_free(). Which code path leaves > prog->aux->jit_data as non-NULL such that extra bpf_int_jit_abort() is needed? Nevermind, it's for those that haven't been jited second time yet..
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c index ec3f00be2ac5..49bc0ddd55ae 100644 --- a/arch/x86/net/bpf_jit_comp.c +++ b/arch/x86/net/bpf_jit_comp.c @@ -2244,6 +2244,30 @@ struct x64_jit_data { struct jit_context ctx; }; +void bpf_int_jit_abort(struct bpf_prog *prog) +{ + struct x64_jit_data *jit_data = prog->aux->jit_data; + struct bpf_binary_header *header, *rw_header; + + if (!jit_data) + return; + + prog->bpf_func = NULL; + prog->jited = 0; + prog->jited_len = 0; + + header = jit_data->header; + rw_header = jit_data->rw_header; + bpf_arch_text_copy(&header->size, &rw_header->size, + sizeof(rw_header->size)); + bpf_jit_binary_pack_free(header, rw_header); + + kvfree(jit_data->addrs); + kfree(jit_data); + + prog->aux->jit_data = NULL; +} + #define MAX_PASSES 20 #define PADDING_PASSES (MAX_PASSES - 5) diff --git a/include/linux/filter.h b/include/linux/filter.h index 9bf26307247f..f3a913229edd 100644 --- a/include/linux/filter.h +++ b/include/linux/filter.h @@ -945,6 +945,7 @@ u64 __bpf_call_base(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5); (void *)__bpf_call_base) struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog); +void bpf_int_jit_abort(struct bpf_prog *prog); void bpf_jit_compile(struct bpf_prog *prog); bool bpf_jit_needs_zext(void); bool bpf_jit_supports_kfunc_call(void); diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index ab630f773ec1..a1841e11524c 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -2636,6 +2636,15 @@ struct bpf_prog * __weak bpf_int_jit_compile(struct bpf_prog *prog) return prog; } +/* + * If arch JIT uses aux->jit_data to save temporary allocated status and + * supports subprog, it needs to override the function to free allocated + * memories and fall back to interpreter mode for passed prog. + */ +void __weak bpf_int_jit_abort(struct bpf_prog *prog) +{ +} + /* Stub for JITs that support eBPF. All cBPF code gets transformed into * eBPF by the kernel and is later compiled by bpf_int_jit_compile(). */ diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index e34264200e09..885e515cf83f 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -13086,6 +13086,9 @@ static int jit_subprogs(struct bpf_verifier_env *env) if (tmp != func[i] || func[i]->bpf_func != old_bpf_func) { verbose(env, "JIT doesn't support bpf-to-bpf calls\n"); err = -ENOTSUPP; + /* Abort extra pass for the remaining subprogs */ + while (++i < env->subprog_cnt) + bpf_int_jit_abort(func[i]); goto out_free; } cond_resched();
It will be used to do cleanup for subprog which has been jited in first pass but extra pass has not been done. The scenario is possible when extra pass for subprog in the middle fails. The failure may lead to oops due to inconsistent status for pack allocator (e.g. ro_hdr->size and use_bpf_prog_pack) and memory leak in aux->jit_data. For x86-64, bpf_int_jit_abort() will free allocated memories saved in aux->jit_data and fall back to interpreter mode to bypass the calling of bpf_jit_binary_pack_free() in bpf_jit_free(). Signed-off-by: Hou Tao <houtao1@huawei.com> --- arch/x86/net/bpf_jit_comp.c | 24 ++++++++++++++++++++++++ include/linux/filter.h | 1 + kernel/bpf/core.c | 9 +++++++++ kernel/bpf/verifier.c | 3 +++ 4 files changed, 37 insertions(+)