| Message ID | 20210707043811.5349-4-hefengqing@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | potential memleak and use after free in bpf verifier | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 12 of 12 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 3290 this patch: 3290 |
| netdev/kdoc | success | Errors and warnings before: 1 this patch: 1 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | CHECK: multiple assignments should be avoided WARNING: line length of 86 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 3387 this patch: 3387 |
| netdev/header_inline | success | Link |
On Tue, Jul 6, 2021 at 8:53 PM He Fengqing <hefengqing@huawei.com> wrote: > > In bpf_patch_insn_data, env->prog was input parameter of > bpf_patch_insn_single function. bpf_patch_insn_single call > bpf_prog_realloc to realloc ebpf prog. When we need to malloc new prog, > bpf_prog_realloc will free the old prog, in this scenery is the > env->prog. > Then bpf_patch_insn_data function call adjust_insn_aux_data function, if > adjust_insn_aux_data function return error, bpf_patch_insn_data will > return NULL. > In bpf_check->convert_ctx_accesses->bpf_patch_insn_data call chain, if > bpf_patch_insn_data return NULL, env->prog has been freed in > bpf_prog_realloc, then bpf_check will use the freed env->prog. Besides "what is the bug", please also describe "how to fix it". For example, add "Fix it by adding a free_old argument to bpf_prog_realloc(), and ...". Also, for the subject of 0/3, it is better to say "fix potential memory leak and ...". > > Signed-off-by: He Fengqing <hefengqing@huawei.com> > --- > include/linux/filter.h | 2 +- > kernel/bpf/core.c | 9 ++++--- > kernel/bpf/verifier.c | 53 ++++++++++++++++++++++++++++++++---------- > net/core/filter.c | 2 +- > 4 files changed, 49 insertions(+), 17 deletions(-) > > diff --git a/include/linux/filter.h b/include/linux/filter.h > index f39e008a377d..ec11a5ae92c2 100644 > --- a/include/linux/filter.h > +++ b/include/linux/filter.h > @@ -881,7 +881,7 @@ void bpf_prog_jit_attempt_done(struct bpf_prog *prog); > struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags); > struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flags); > struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, > - gfp_t gfp_extra_flags); > + gfp_t gfp_extra_flags, bool free_old); > void __bpf_prog_free(struct bpf_prog *fp); > > static inline void bpf_prog_clone_free(struct bpf_prog *fp) > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > index 49b0311f48c1..e5616bb1665b 100644 > --- a/kernel/bpf/core.c > +++ b/kernel/bpf/core.c > @@ -218,7 +218,7 @@ void bpf_prog_fill_jited_linfo(struct bpf_prog *prog, > } > > struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, > - gfp_t gfp_extra_flags) > + gfp_t gfp_extra_flags, bool free_old) > { > gfp_t gfp_flags = GFP_KERNEL_ACCOUNT | __GFP_ZERO | gfp_extra_flags; > struct bpf_prog *fp; > @@ -238,7 +238,8 @@ struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, > /* We keep fp->aux from fp_old around in the new > * reallocated structure. > */ > - bpf_prog_clone_free(fp_old); > + if (free_old) > + bpf_prog_clone_free(fp_old); > } > > return fp; > @@ -456,7 +457,7 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off, > * last page could have large enough tailroom. > */ > prog_adj = bpf_prog_realloc(prog, bpf_prog_size(insn_adj_cnt), > - GFP_USER); > + GFP_USER, false); > if (!prog_adj) > return ERR_PTR(-ENOMEM); > > @@ -1150,6 +1151,8 @@ struct bpf_prog *bpf_jit_blind_constants(struct bpf_prog *prog) > return tmp; > } > > + if (tmp != clone) > + bpf_prog_clone_free(clone); > clone = tmp; > insn_delta = rewritten - 1; > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 41109f49b724..e75b933f69e4 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -11855,7 +11855,10 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, > new_prog = bpf_patch_insn_data(env, adj_idx, patch, patch_len); > if (!new_prog) > return -ENOMEM; > - env->prog = new_prog; > + if (new_prog != env->prog) { > + bpf_prog_clone_free(env->prog); > + env->prog = new_prog; > + } Can we move this check into bpf_patch_insn_data()? > insns = new_prog->insnsi; > aux = env->insn_aux_data; > delta += patch_len - 1; [...] > diff --git a/net/core/filter.c b/net/core/filter.c > index d70187ce851b..8a8d1a3ba5c2 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -1268,7 +1268,7 @@ static struct bpf_prog *bpf_migrate_filter(struct bpf_prog *fp) > > /* Expand fp for appending the new filter representation. */ > old_fp = fp; > - fp = bpf_prog_realloc(old_fp, bpf_prog_size(new_len), 0); > + fp = bpf_prog_realloc(old_fp, bpf_prog_size(new_len), 0, true); Can we add some logic here and not add free_old to bpf_prog_realloc()? Thanks, Song
在 2021/7/7 15:25, Song Liu 写道: > On Tue, Jul 6, 2021 at 8:53 PM He Fengqing <hefengqing@huawei.com> wrote: >> >> In bpf_patch_insn_data, env->prog was input parameter of >> bpf_patch_insn_single function. bpf_patch_insn_single call >> bpf_prog_realloc to realloc ebpf prog. When we need to malloc new prog, >> bpf_prog_realloc will free the old prog, in this scenery is the >> env->prog. >> Then bpf_patch_insn_data function call adjust_insn_aux_data function, if >> adjust_insn_aux_data function return error, bpf_patch_insn_data will >> return NULL. >> In bpf_check->convert_ctx_accesses->bpf_patch_insn_data call chain, if >> bpf_patch_insn_data return NULL, env->prog has been freed in >> bpf_prog_realloc, then bpf_check will use the freed env->prog. > > Besides "what is the bug", please also describe "how to fix it". For example, > add "Fix it by adding a free_old argument to bpf_prog_realloc(), and ...". > Also, for the subject of 0/3, it is better to say "fix potential > memory leak and ...". Thanks for your suggestion. > >> >> Signed-off-by: He Fengqing <hefengqing@huawei.com> >> --- >> include/linux/filter.h | 2 +- >> kernel/bpf/core.c | 9 ++++--- >> kernel/bpf/verifier.c | 53 ++++++++++++++++++++++++++++++++---------- >> net/core/filter.c | 2 +- >> 4 files changed, 49 insertions(+), 17 deletions(-) >> >> diff --git a/include/linux/filter.h b/include/linux/filter.h >> index f39e008a377d..ec11a5ae92c2 100644 >> --- a/include/linux/filter.h >> +++ b/include/linux/filter.h >> @@ -881,7 +881,7 @@ void bpf_prog_jit_attempt_done(struct bpf_prog *prog); >> struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags); >> struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flags); >> struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, >> - gfp_t gfp_extra_flags); >> + gfp_t gfp_extra_flags, bool free_old); >> void __bpf_prog_free(struct bpf_prog *fp); >> >> static inline void bpf_prog_clone_free(struct bpf_prog *fp) >> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c >> index 49b0311f48c1..e5616bb1665b 100644 >> --- a/kernel/bpf/core.c >> +++ b/kernel/bpf/core.c >> @@ -218,7 +218,7 @@ void bpf_prog_fill_jited_linfo(struct bpf_prog *prog, >> } >> >> struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, >> - gfp_t gfp_extra_flags) >> + gfp_t gfp_extra_flags, bool free_old) >> { >> gfp_t gfp_flags = GFP_KERNEL_ACCOUNT | __GFP_ZERO | gfp_extra_flags; >> struct bpf_prog *fp; >> @@ -238,7 +238,8 @@ struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, >> /* We keep fp->aux from fp_old around in the new >> * reallocated structure. >> */ >> - bpf_prog_clone_free(fp_old); >> + if (free_old) >> + bpf_prog_clone_free(fp_old); >> } >> >> return fp; >> @@ -456,7 +457,7 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off, >> * last page could have large enough tailroom. >> */ >> prog_adj = bpf_prog_realloc(prog, bpf_prog_size(insn_adj_cnt), >> - GFP_USER); >> + GFP_USER, false); >> if (!prog_adj) >> return ERR_PTR(-ENOMEM); >> >> @@ -1150,6 +1151,8 @@ struct bpf_prog *bpf_jit_blind_constants(struct bpf_prog *prog) >> return tmp; >> } >> >> + if (tmp != clone) >> + bpf_prog_clone_free(clone); >> clone = tmp; >> insn_delta = rewritten - 1; >> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index 41109f49b724..e75b933f69e4 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -11855,7 +11855,10 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, >> new_prog = bpf_patch_insn_data(env, adj_idx, patch, patch_len); >> if (!new_prog) >> return -ENOMEM; >> - env->prog = new_prog; >> + if (new_prog != env->prog) { >> + bpf_prog_clone_free(env->prog); >> + env->prog = new_prog; >> + } > > Can we move this check into bpf_patch_insn_data()? Ok, I will change this in next version. > >> insns = new_prog->insnsi; >> aux = env->insn_aux_data; >> delta += patch_len - 1; > [...] > >> diff --git a/net/core/filter.c b/net/core/filter.c >> index d70187ce851b..8a8d1a3ba5c2 100644 >> --- a/net/core/filter.c >> +++ b/net/core/filter.c >> @@ -1268,7 +1268,7 @@ static struct bpf_prog *bpf_migrate_filter(struct bpf_prog *fp) >> >> /* Expand fp for appending the new filter representation. */ >> old_fp = fp; >> - fp = bpf_prog_realloc(old_fp, bpf_prog_size(new_len), 0); >> + fp = bpf_prog_realloc(old_fp, bpf_prog_size(new_len), 0, true); > > Can we add some logic here and not add free_old to bpf_prog_realloc()? Ok, maybe we can free old_fp here, never in bpf_prog_realloc. > > Thanks, > Song > . >
On Wed, Jul 7, 2021 at 8:00 PM He Fengqing <hefengqing@huawei.com> wrote: > > Ok, I will change this in next version. before you spam the list with the next version please explain why any of these changes are needed? I don't see an explanation in the patches and I don't see a bug in the code. Did you check what is the prog clone ? When is it constructed? Why verifier has anything to do with it?
在 2021/7/8 11:09, Alexei Starovoitov 写道: > On Wed, Jul 7, 2021 at 8:00 PM He Fengqing <hefengqing@huawei.com> wrote: >> >> Ok, I will change this in next version. > > before you spam the list with the next version > please explain why any of these changes are needed? > I don't see an explanation in the patches and I don't see a bug in the code. > Did you check what is the prog clone ? > When is it constructed? Why verifier has anything to do with it? > . > I'm sorry, I didn't describe these errors clearly. bpf_check(bpf_verifier_env) | |->do_misc_fixups(env) | | | |->bpf_patch_insn_data(env) | | | | | |->bpf_patch_insn_single(env->prog) | | | | | | | |->bpf_prog_realloc(env->prog) | | | | | | | | | |->construct new_prog | | | | | free old_prog(env->prog) | | | | | | | | | |->return new_prog; | | | | | | | |->return new_prog; | | | | | |->adjust_insn_aux_data | | | | | | | |->return ENOMEM; | | | | | |->return NULL; | | | |->return ENOMEM; bpf_verifier_env->prog had been freed in bpf_prog_realloc function. There are two errors here, the first is memleak in the bpf_patch_insn_data function, and the second is use after free in the bpf_check function. memleak in bpf_patch_insn_data: Look at the call chain above, if adjust_insn_aux_data function return ENOMEM, bpf_patch_insn_data will return NULL, but we do not free the new_prog. So in the patch 2, before bpf_patch_insn_data return NULL, we free the new_prog. use after free in bpf_check: If bpf_patch_insn_data function return NULL, we will not assign new_prog to the bpf_verifier_env->prog, but bpf_verifier_env->prog has been freed in the bpf_prog_realloc function. Then in bpf_check function, we will use bpf_verifier_env->prog after do_misc_fixups function. In the patch 3, I added a free_old parameter to bpf_prog_realloc, in this scenario we don't free old_prog. Instead, we free it in the do_misc_fixups function when bpf_patch_insn_data return a valid new_prog. Thanks for your reviews.
On Fri, Jul 9, 2021 at 4:11 AM He Fengqing <hefengqing@huawei.com> wrote: > > > > 在 2021/7/8 11:09, Alexei Starovoitov 写道: > > On Wed, Jul 7, 2021 at 8:00 PM He Fengqing <hefengqing@huawei.com> wrote: > >> > >> Ok, I will change this in next version. > > > > before you spam the list with the next version > > please explain why any of these changes are needed? > > I don't see an explanation in the patches and I don't see a bug in the code. > > Did you check what is the prog clone ? > > When is it constructed? Why verifier has anything to do with it? > > . > > > > > I'm sorry, I didn't describe these errors clearly. > > bpf_check(bpf_verifier_env) > | > |->do_misc_fixups(env) > | | > | |->bpf_patch_insn_data(env) > | | | > | | |->bpf_patch_insn_single(env->prog) > | | | | > | | | |->bpf_prog_realloc(env->prog) > | | | | | > | | | | |->construct new_prog > | | | | | free old_prog(env->prog) > | | | | | > | | | | |->return new_prog; > | | | | > | | | |->return new_prog; > | | | > | | |->adjust_insn_aux_data > | | | | > | | | |->return ENOMEM; > | | | > | | |->return NULL; > | | > | |->return ENOMEM; > > bpf_verifier_env->prog had been freed in bpf_prog_realloc function. > > > There are two errors here, the first is memleak in the > bpf_patch_insn_data function, and the second is use after free in the > bpf_check function. > > memleak in bpf_patch_insn_data: > > Look at the call chain above, if adjust_insn_aux_data function return > ENOMEM, bpf_patch_insn_data will return NULL, but we do not free the > new_prog. > > So in the patch 2, before bpf_patch_insn_data return NULL, we free the > new_prog. > > use after free in bpf_check: > > If bpf_patch_insn_data function return NULL, we will not assign new_prog > to the bpf_verifier_env->prog, but bpf_verifier_env->prog has been freed > in the bpf_prog_realloc function. Then in bpf_check function, we will > use bpf_verifier_env->prog after do_misc_fixups function. > > In the patch 3, I added a free_old parameter to bpf_prog_realloc, in > this scenario we don't free old_prog. Instead, we free it in the > do_misc_fixups function when bpf_patch_insn_data return a valid new_prog. Thanks for explaining. Why not to make adjust_insn_aux_data() in bpf_patch_insn_data() first then? Just changing the order will resolve both issues, no?
在 2021/7/9 23:12, Alexei Starovoitov 写道: > On Fri, Jul 9, 2021 at 4:11 AM He Fengqing <hefengqing@huawei.com> wrote: >> >> >> >> 在 2021/7/8 11:09, Alexei Starovoitov 写道: >>> On Wed, Jul 7, 2021 at 8:00 PM He Fengqing <hefengqing@huawei.com> wrote: >>>> >>>> Ok, I will change this in next version. >>> >>> before you spam the list with the next version >>> please explain why any of these changes are needed? >>> I don't see an explanation in the patches and I don't see a bug in the code. >>> Did you check what is the prog clone ? >>> When is it constructed? Why verifier has anything to do with it? >>> . >>> >> >> >> I'm sorry, I didn't describe these errors clearly. >> >> bpf_check(bpf_verifier_env) >> | >> |->do_misc_fixups(env) >> | | >> | |->bpf_patch_insn_data(env) >> | | | >> | | |->bpf_patch_insn_single(env->prog) >> | | | | >> | | | |->bpf_prog_realloc(env->prog) >> | | | | | >> | | | | |->construct new_prog >> | | | | | free old_prog(env->prog) >> | | | | | >> | | | | |->return new_prog; >> | | | | >> | | | |->return new_prog; >> | | | >> | | |->adjust_insn_aux_data >> | | | | >> | | | |->return ENOMEM; >> | | | >> | | |->return NULL; >> | | >> | |->return ENOMEM; >> >> bpf_verifier_env->prog had been freed in bpf_prog_realloc function. >> >> >> There are two errors here, the first is memleak in the >> bpf_patch_insn_data function, and the second is use after free in the >> bpf_check function. >> >> memleak in bpf_patch_insn_data: >> >> Look at the call chain above, if adjust_insn_aux_data function return >> ENOMEM, bpf_patch_insn_data will return NULL, but we do not free the >> new_prog. >> >> So in the patch 2, before bpf_patch_insn_data return NULL, we free the >> new_prog. >> >> use after free in bpf_check: >> >> If bpf_patch_insn_data function return NULL, we will not assign new_prog >> to the bpf_verifier_env->prog, but bpf_verifier_env->prog has been freed >> in the bpf_prog_realloc function. Then in bpf_check function, we will >> use bpf_verifier_env->prog after do_misc_fixups function. >> >> In the patch 3, I added a free_old parameter to bpf_prog_realloc, in >> this scenario we don't free old_prog. Instead, we free it in the >> do_misc_fixups function when bpf_patch_insn_data return a valid new_prog. > > Thanks for explaining. > Why not to make adjust_insn_aux_data() in bpf_patch_insn_data() first then? > Just changing the order will resolve both issues, no? > . > adjust_insn_aux_data() need the new constructed new_prog as an input parameter, so we must call bpf_patch_insn_single() before adjust_insn_aux_data(). But we can make adjust_insn_aux_data() never return ENOMEM. In bpf_patch_insn_data(), first we pre-malloc memory for new aux_data, then call bpf_patch_insn_single() to constructed the new_prog, at last call adjust_insn_aux_data() functin. In this way, adjust_insn_aux_data() never fails. bpf_patch_insn_data(env) { struct bpf_insn_aux_data *new_data = vzalloc(); struct bpf_prog *new_prog; if (new_data == NULL) return NULL; new_prog = bpf_patch_insn_single(env->prog); if (new_prog == NULL) { vfree(new_data); return NULL; } adjust_insn_aux_data(new_prog, new_data); return new_prog; } What do you think about it?
On Sun, Jul 11, 2021 at 7:17 PM He Fengqing <hefengqing@huawei.com> wrote: > > > > 在 2021/7/9 23:12, Alexei Starovoitov 写道: > > On Fri, Jul 9, 2021 at 4:11 AM He Fengqing <hefengqing@huawei.com> wrote: > >> > >> > >> > >> 在 2021/7/8 11:09, Alexei Starovoitov 写道: > >>> On Wed, Jul 7, 2021 at 8:00 PM He Fengqing <hefengqing@huawei.com> wrote: > >>>> > >>>> Ok, I will change this in next version. > >>> > >>> before you spam the list with the next version > >>> please explain why any of these changes are needed? > >>> I don't see an explanation in the patches and I don't see a bug in the code. > >>> Did you check what is the prog clone ? > >>> When is it constructed? Why verifier has anything to do with it? > >>> . > >>> > >> > >> > >> I'm sorry, I didn't describe these errors clearly. > >> > >> bpf_check(bpf_verifier_env) > >> | > >> |->do_misc_fixups(env) > >> | | > >> | |->bpf_patch_insn_data(env) > >> | | | > >> | | |->bpf_patch_insn_single(env->prog) > >> | | | | > >> | | | |->bpf_prog_realloc(env->prog) > >> | | | | | > >> | | | | |->construct new_prog > >> | | | | | free old_prog(env->prog) > >> | | | | | > >> | | | | |->return new_prog; > >> | | | | > >> | | | |->return new_prog; > >> | | | > >> | | |->adjust_insn_aux_data > >> | | | | > >> | | | |->return ENOMEM; > >> | | | > >> | | |->return NULL; > >> | | > >> | |->return ENOMEM; > >> > >> bpf_verifier_env->prog had been freed in bpf_prog_realloc function. > >> > >> > >> There are two errors here, the first is memleak in the > >> bpf_patch_insn_data function, and the second is use after free in the > >> bpf_check function. > >> > >> memleak in bpf_patch_insn_data: > >> > >> Look at the call chain above, if adjust_insn_aux_data function return > >> ENOMEM, bpf_patch_insn_data will return NULL, but we do not free the > >> new_prog. > >> > >> So in the patch 2, before bpf_patch_insn_data return NULL, we free the > >> new_prog. > >> > >> use after free in bpf_check: > >> > >> If bpf_patch_insn_data function return NULL, we will not assign new_prog > >> to the bpf_verifier_env->prog, but bpf_verifier_env->prog has been freed > >> in the bpf_prog_realloc function. Then in bpf_check function, we will > >> use bpf_verifier_env->prog after do_misc_fixups function. > >> > >> In the patch 3, I added a free_old parameter to bpf_prog_realloc, in > >> this scenario we don't free old_prog. Instead, we free it in the > >> do_misc_fixups function when bpf_patch_insn_data return a valid new_prog. > > > > Thanks for explaining. > > Why not to make adjust_insn_aux_data() in bpf_patch_insn_data() first then? > > Just changing the order will resolve both issues, no? > > . > > > adjust_insn_aux_data() need the new constructed new_prog as an input > parameter, so we must call bpf_patch_insn_single() before > adjust_insn_aux_data(). Right. I forgot about insn_has_def32() logic and commit b325fbca4b13 ("bpf: verifier: mark patched-insn with sub-register zext flag") that added that extra parameter. > But we can make adjust_insn_aux_data() never return ENOMEM. In > bpf_patch_insn_data(), first we pre-malloc memory for new aux_data, then > call bpf_patch_insn_single() to constructed the new_prog, at last call > adjust_insn_aux_data() functin. In this way, adjust_insn_aux_data() > never fails. > > bpf_patch_insn_data(env) { > struct bpf_insn_aux_data *new_data = vzalloc(); > struct bpf_prog *new_prog; > if (new_data == NULL) > return NULL; > > new_prog = bpf_patch_insn_single(env->prog); > if (new_prog == NULL) { > vfree(new_data); > return NULL; > } > > adjust_insn_aux_data(new_prog, new_data); > return new_prog; > } > What do you think about it? That's a good idea. Let's do that. The new size for vzalloc is easy to compute. What should be the commit in the Fixes tag? commit 8041902dae52 ("bpf: adjust insn_aux_data when patching insns") right? 4 year old bug then. I wonder why syzbot with malloc error injection didn't catch it sooner.
在 2021/7/14 7:17, Alexei Starovoitov 写道: > On Sun, Jul 11, 2021 at 7:17 PM He Fengqing <hefengqing@huawei.com> wrote: >> >> >> >> 在 2021/7/9 23:12, Alexei Starovoitov 写道: >>> On Fri, Jul 9, 2021 at 4:11 AM He Fengqing <hefengqing@huawei.com> wrote: >>>> >>>> >>>> >>>> 在 2021/7/8 11:09, Alexei Starovoitov 写道: >>>>> On Wed, Jul 7, 2021 at 8:00 PM He Fengqing <hefengqing@huawei.com> wrote: >>>>>> >>>>>> Ok, I will change this in next version. >>>>> >>>>> before you spam the list with the next version >>>>> please explain why any of these changes are needed? >>>>> I don't see an explanation in the patches and I don't see a bug in the code. >>>>> Did you check what is the prog clone ? >>>>> When is it constructed? Why verifier has anything to do with it? >>>>> . >>>>> >>>> >>>> >>>> I'm sorry, I didn't describe these errors clearly. >>>> >>>> bpf_check(bpf_verifier_env) >>>> | >>>> |->do_misc_fixups(env) >>>> | | >>>> | |->bpf_patch_insn_data(env) >>>> | | | >>>> | | |->bpf_patch_insn_single(env->prog) >>>> | | | | >>>> | | | |->bpf_prog_realloc(env->prog) >>>> | | | | | >>>> | | | | |->construct new_prog >>>> | | | | | free old_prog(env->prog) >>>> | | | | | >>>> | | | | |->return new_prog; >>>> | | | | >>>> | | | |->return new_prog; >>>> | | | >>>> | | |->adjust_insn_aux_data >>>> | | | | >>>> | | | |->return ENOMEM; >>>> | | | >>>> | | |->return NULL; >>>> | | >>>> | |->return ENOMEM; >>>> >>>> bpf_verifier_env->prog had been freed in bpf_prog_realloc function. >>>> >>>> >>>> There are two errors here, the first is memleak in the >>>> bpf_patch_insn_data function, and the second is use after free in the >>>> bpf_check function. >>>> >>>> memleak in bpf_patch_insn_data: >>>> >>>> Look at the call chain above, if adjust_insn_aux_data function return >>>> ENOMEM, bpf_patch_insn_data will return NULL, but we do not free the >>>> new_prog. >>>> >>>> So in the patch 2, before bpf_patch_insn_data return NULL, we free the >>>> new_prog. >>>> >>>> use after free in bpf_check: >>>> >>>> If bpf_patch_insn_data function return NULL, we will not assign new_prog >>>> to the bpf_verifier_env->prog, but bpf_verifier_env->prog has been freed >>>> in the bpf_prog_realloc function. Then in bpf_check function, we will >>>> use bpf_verifier_env->prog after do_misc_fixups function. >>>> >>>> In the patch 3, I added a free_old parameter to bpf_prog_realloc, in >>>> this scenario we don't free old_prog. Instead, we free it in the >>>> do_misc_fixups function when bpf_patch_insn_data return a valid new_prog. >>> >>> Thanks for explaining. >>> Why not to make adjust_insn_aux_data() in bpf_patch_insn_data() first then? >>> Just changing the order will resolve both issues, no? >>> . >>> >> adjust_insn_aux_data() need the new constructed new_prog as an input >> parameter, so we must call bpf_patch_insn_single() before >> adjust_insn_aux_data(). > > Right. I forgot about insn_has_def32() logic and > commit b325fbca4b13 ("bpf: verifier: mark patched-insn with > sub-register zext flag") > that added that extra parameter. > >> But we can make adjust_insn_aux_data() never return ENOMEM. In >> bpf_patch_insn_data(), first we pre-malloc memory for new aux_data, then >> call bpf_patch_insn_single() to constructed the new_prog, at last call >> adjust_insn_aux_data() functin. In this way, adjust_insn_aux_data() >> never fails. >> >> bpf_patch_insn_data(env) { >> struct bpf_insn_aux_data *new_data = vzalloc(); >> struct bpf_prog *new_prog; >> if (new_data == NULL) >> return NULL; >> >> new_prog = bpf_patch_insn_single(env->prog); >> if (new_prog == NULL) { >> vfree(new_data); >> return NULL; >> } >> >> adjust_insn_aux_data(new_prog, new_data); >> return new_prog; >> } >> What do you think about it? > > That's a good idea. Let's do that. The new size for vzalloc is easy to compute. > What should be the commit in the Fixes tag? > commit 8041902dae52 ("bpf: adjust insn_aux_data when patching insns") > right? Ok, I will add this in the commit message. > 4 year old bug then. > I wonder why syzbot with malloc error injection didn't catch it sooner. > . >
diff --git a/include/linux/filter.h b/include/linux/filter.h index f39e008a377d..ec11a5ae92c2 100644 --- a/include/linux/filter.h +++ b/include/linux/filter.h @@ -881,7 +881,7 @@ void bpf_prog_jit_attempt_done(struct bpf_prog *prog); struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags); struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flags); struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, - gfp_t gfp_extra_flags); + gfp_t gfp_extra_flags, bool free_old); void __bpf_prog_free(struct bpf_prog *fp); static inline void bpf_prog_clone_free(struct bpf_prog *fp) diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index 49b0311f48c1..e5616bb1665b 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -218,7 +218,7 @@ void bpf_prog_fill_jited_linfo(struct bpf_prog *prog, } struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, - gfp_t gfp_extra_flags) + gfp_t gfp_extra_flags, bool free_old) { gfp_t gfp_flags = GFP_KERNEL_ACCOUNT | __GFP_ZERO | gfp_extra_flags; struct bpf_prog *fp; @@ -238,7 +238,8 @@ struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, /* We keep fp->aux from fp_old around in the new * reallocated structure. */ - bpf_prog_clone_free(fp_old); + if (free_old) + bpf_prog_clone_free(fp_old); } return fp; @@ -456,7 +457,7 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off, * last page could have large enough tailroom. */ prog_adj = bpf_prog_realloc(prog, bpf_prog_size(insn_adj_cnt), - GFP_USER); + GFP_USER, false); if (!prog_adj) return ERR_PTR(-ENOMEM); @@ -1150,6 +1151,8 @@ struct bpf_prog *bpf_jit_blind_constants(struct bpf_prog *prog) return tmp; } + if (tmp != clone) + bpf_prog_clone_free(clone); clone = tmp; insn_delta = rewritten - 1; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 41109f49b724..e75b933f69e4 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -11855,7 +11855,10 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env, new_prog = bpf_patch_insn_data(env, adj_idx, patch, patch_len); if (!new_prog) return -ENOMEM; - env->prog = new_prog; + if (new_prog != env->prog) { + bpf_prog_clone_free(env->prog); + env->prog = new_prog; + } insns = new_prog->insnsi; aux = env->insn_aux_data; delta += patch_len - 1; @@ -11895,7 +11898,10 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env) if (!new_prog) return -ENOMEM; - env->prog = new_prog; + if (new_prog != env->prog) { + bpf_prog_clone_free(env->prog); + env->prog = new_prog; + } delta += cnt - 1; } } @@ -11944,7 +11950,10 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env) return -ENOMEM; delta += cnt - 1; - env->prog = new_prog; + if (new_prog != env->prog) { + bpf_prog_clone_free(env->prog); + env->prog = new_prog; + } insn = new_prog->insnsi + i + delta; continue; } @@ -12042,9 +12051,11 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env) return -ENOMEM; delta += cnt - 1; - - /* keep walking new program and skip insns we just inserted */ - env->prog = new_prog; + if (new_prog != env->prog) { + bpf_prog_clone_free(env->prog); + /* keep walking new program and skip insns we just inserted */ + env->prog = new_prog; + } insn = new_prog->insnsi + i + delta; } @@ -12419,7 +12430,10 @@ static int do_misc_fixups(struct bpf_verifier_env *env) return -ENOMEM; delta += cnt - 1; - env->prog = prog = new_prog; + if (new_prog != env->prog) { + bpf_prog_clone_free(env->prog); + env->prog = prog = new_prog; + } insn = new_prog->insnsi + i + delta; continue; } @@ -12439,7 +12453,10 @@ static int do_misc_fixups(struct bpf_verifier_env *env) return -ENOMEM; delta += cnt - 1; - env->prog = prog = new_prog; + if (new_prog != env->prog) { + bpf_prog_clone_free(env->prog); + env->prog = prog = new_prog; + } insn = new_prog->insnsi + i + delta; continue; } @@ -12492,7 +12509,10 @@ static int do_misc_fixups(struct bpf_verifier_env *env) return -ENOMEM; delta += cnt - 1; - env->prog = prog = new_prog; + if (new_prog != env->prog) { + bpf_prog_clone_free(env->prog); + env->prog = prog = new_prog; + } insn = new_prog->insnsi + i + delta; continue; } @@ -12584,7 +12604,10 @@ static int do_misc_fixups(struct bpf_verifier_env *env) return -ENOMEM; delta += cnt - 1; - env->prog = prog = new_prog; + if (new_prog != env->prog) { + bpf_prog_clone_free(env->prog); + env->prog = prog = new_prog; + } insn = new_prog->insnsi + i + delta; continue; } @@ -12623,7 +12646,10 @@ static int do_misc_fixups(struct bpf_verifier_env *env) return -ENOMEM; delta += cnt - 1; - env->prog = prog = new_prog; + if (new_prog != env->prog) { + bpf_prog_clone_free(env->prog); + env->prog = prog = new_prog; + } insn = new_prog->insnsi + i + delta; continue; } @@ -12700,7 +12726,10 @@ static int do_misc_fixups(struct bpf_verifier_env *env) return -ENOMEM; delta += cnt - 1; - env->prog = prog = new_prog; + if (new_prog != env->prog) { + bpf_prog_clone_free(env->prog); + env->prog = prog = new_prog; + } insn = new_prog->insnsi + i + delta; continue; } diff --git a/net/core/filter.c b/net/core/filter.c index d70187ce851b..8a8d1a3ba5c2 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -1268,7 +1268,7 @@ static struct bpf_prog *bpf_migrate_filter(struct bpf_prog *fp) /* Expand fp for appending the new filter representation. */ old_fp = fp; - fp = bpf_prog_realloc(old_fp, bpf_prog_size(new_len), 0); + fp = bpf_prog_realloc(old_fp, bpf_prog_size(new_len), 0, true); if (!fp) { /* The old_fp is still around in case we couldn't * allocate new memory, so uncharge on that one.
In bpf_patch_insn_data, env->prog was input parameter of bpf_patch_insn_single function. bpf_patch_insn_single call bpf_prog_realloc to realloc ebpf prog. When we need to malloc new prog, bpf_prog_realloc will free the old prog, in this scenery is the env->prog. Then bpf_patch_insn_data function call adjust_insn_aux_data function, if adjust_insn_aux_data function return error, bpf_patch_insn_data will return NULL. In bpf_check->convert_ctx_accesses->bpf_patch_insn_data call chain, if bpf_patch_insn_data return NULL, env->prog has been freed in bpf_prog_realloc, then bpf_check will use the freed env->prog. Signed-off-by: He Fengqing <hefengqing@huawei.com> --- include/linux/filter.h | 2 +- kernel/bpf/core.c | 9 ++++--- kernel/bpf/verifier.c | 53 ++++++++++++++++++++++++++++++++---------- net/core/filter.c | 2 +- 4 files changed, 49 insertions(+), 17 deletions(-)