* [bpf-next 0/3] potential memleak and use after free in bpf verifier
@ 2021-07-07 4:38 He Fengqing
2021-07-07 4:38 ` [bpf-next 1/3] bpf: Move bpf_prog_clone_free into filter.h file He Fengqing
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: He Fengqing @ 2021-07-07 4:38 UTC (permalink / raw)
To: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend, kpsingh
Cc: davem, kuba, netdev, bpf, linux-kernel
While reading the code of bpf verifier, I found these two issues.
Patch 1 move the bpf_prog_clone_free function into filter.h, so
we can use it in other file. Patch 2 fix memleak in an error
handling path in bpf_patch_insn_data function.
Patch 3 fix a use after free in bpf_check function.
He Fengqing (3):
bpf: Move bpf_prog_clone_free into filter.h file
bpf: Fix a memory leak in an error handling path in
'bpf_patch_insn_data()'
bpf: Fix a use after free in bpf_check()
include/linux/filter.h | 17 ++++++++++++-
kernel/bpf/core.c | 27 +++++---------------
kernel/bpf/verifier.c | 58 ++++++++++++++++++++++++++++++++----------
net/core/filter.c | 2 +-
4 files changed, 68 insertions(+), 36 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [bpf-next 1/3] bpf: Move bpf_prog_clone_free into filter.h file 2021-07-07 4:38 [bpf-next 0/3] potential memleak and use after free in bpf verifier He Fengqing @ 2021-07-07 4:38 ` He Fengqing 2021-07-07 7:02 ` Song Liu 2021-07-07 4:38 ` [bpf-next 2/3] bpf: Fix a memory leak in an error handling path in 'bpf_patch_insn_data()' He Fengqing 2021-07-07 4:38 ` [bpf-next 3/3] bpf: Fix a use after free in bpf_check() He Fengqing 2 siblings, 1 reply; 13+ messages in thread From: He Fengqing @ 2021-07-07 4:38 UTC (permalink / raw) To: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend, kpsingh Cc: davem, kuba, netdev, bpf, linux-kernel Move bpf_prog_clone_free function into filter.h, so we can use it in other file. Signed-off-by: He Fengqing <hefengqing@huawei.com> --- include/linux/filter.h | 15 +++++++++++++++ kernel/bpf/core.c | 20 +------------------- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/include/linux/filter.h b/include/linux/filter.h index 472f97074da0..f39e008a377d 100644 --- a/include/linux/filter.h +++ b/include/linux/filter.h @@ -884,6 +884,21 @@ struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, gfp_t gfp_extra_flags); void __bpf_prog_free(struct bpf_prog *fp); +static inline void bpf_prog_clone_free(struct bpf_prog *fp) +{ + /* aux was stolen by the other clone, so we cannot free + * it from this path! It will be freed eventually by the + * other program on release. + * + * At this point, we don't need a deferred release since + * clone is guaranteed to not be locked. + */ + fp->aux = NULL; + fp->stats = NULL; + fp->active = NULL; + __bpf_prog_free(fp); +} + static inline void bpf_prog_unlock_free(struct bpf_prog *fp) { __bpf_prog_free(fp); diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index 034ad93a1ad7..49b0311f48c1 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -238,10 +238,7 @@ 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. */ - fp_old->aux = NULL; - fp_old->stats = NULL; - fp_old->active = NULL; - __bpf_prog_free(fp_old); + bpf_prog_clone_free(fp_old); } return fp; @@ -1102,21 +1099,6 @@ static struct bpf_prog *bpf_prog_clone_create(struct bpf_prog *fp_other, return fp; } -static void bpf_prog_clone_free(struct bpf_prog *fp) -{ - /* aux was stolen by the other clone, so we cannot free - * it from this path! It will be freed eventually by the - * other program on release. - * - * At this point, we don't need a deferred release since - * clone is guaranteed to not be locked. - */ - fp->aux = NULL; - fp->stats = NULL; - fp->active = NULL; - __bpf_prog_free(fp); -} - void bpf_jit_prog_release_other(struct bpf_prog *fp, struct bpf_prog *fp_other) { /* We have to repoint aux->prog to self, as we don't -- 2.25.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [bpf-next 1/3] bpf: Move bpf_prog_clone_free into filter.h file 2021-07-07 4:38 ` [bpf-next 1/3] bpf: Move bpf_prog_clone_free into filter.h file He Fengqing @ 2021-07-07 7:02 ` Song Liu 0 siblings, 0 replies; 13+ messages in thread From: Song Liu @ 2021-07-07 7:02 UTC (permalink / raw) To: He Fengqing Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, David S . Miller, Jakub Kicinski, Networking, bpf, open list On Tue, Jul 6, 2021 at 8:53 PM He Fengqing <hefengqing@huawei.com> wrote: > > Move bpf_prog_clone_free function into filter.h, so we can use > it in other file. > > Signed-off-by: He Fengqing <hefengqing@huawei.com> > --- > include/linux/filter.h | 15 +++++++++++++++ > kernel/bpf/core.c | 20 +------------------- > 2 files changed, 16 insertions(+), 19 deletions(-) > > diff --git a/include/linux/filter.h b/include/linux/filter.h > index 472f97074da0..f39e008a377d 100644 > --- a/include/linux/filter.h > +++ b/include/linux/filter.h > @@ -884,6 +884,21 @@ struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, > gfp_t gfp_extra_flags); > void __bpf_prog_free(struct bpf_prog *fp); > > +static inline void bpf_prog_clone_free(struct bpf_prog *fp) > +{ > + /* aux was stolen by the other clone, so we cannot free > + * it from this path! It will be freed eventually by the > + * other program on release. > + * > + * At this point, we don't need a deferred release since > + * clone is guaranteed to not be locked. > + */ > + fp->aux = NULL; > + fp->stats = NULL; > + fp->active = NULL; > + __bpf_prog_free(fp); > +} > + > static inline void bpf_prog_unlock_free(struct bpf_prog *fp) > { > __bpf_prog_free(fp); > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > index 034ad93a1ad7..49b0311f48c1 100644 > --- a/kernel/bpf/core.c > +++ b/kernel/bpf/core.c > @@ -238,10 +238,7 @@ 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. > */ After the change, we can remove the comment above. > - fp_old->aux = NULL; > - fp_old->stats = NULL; > - fp_old->active = NULL; > - __bpf_prog_free(fp_old); > + bpf_prog_clone_free(fp_old); Please add a couple sentences in the commit log about this chanage. Thanks, Song ^ permalink raw reply [flat|nested] 13+ messages in thread
* [bpf-next 2/3] bpf: Fix a memory leak in an error handling path in 'bpf_patch_insn_data()' 2021-07-07 4:38 [bpf-next 0/3] potential memleak and use after free in bpf verifier He Fengqing 2021-07-07 4:38 ` [bpf-next 1/3] bpf: Move bpf_prog_clone_free into filter.h file He Fengqing @ 2021-07-07 4:38 ` He Fengqing 2021-07-07 4:38 ` [bpf-next 3/3] bpf: Fix a use after free in bpf_check() He Fengqing 2 siblings, 0 replies; 13+ messages in thread From: He Fengqing @ 2021-07-07 4:38 UTC (permalink / raw) To: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend, kpsingh Cc: davem, kuba, netdev, bpf, linux-kernel In bpf_patch_insn_data function, if adjust_insn_aux_data() return error, we need to free new_prog. Signed-off-by: He Fengqing <hefengqing@huawei.com> --- kernel/bpf/verifier.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index be38bb930bf1..41109f49b724 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -11501,8 +11501,11 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of env->insn_aux_data[off].orig_idx); return NULL; } - if (adjust_insn_aux_data(env, new_prog, off, len)) + if (adjust_insn_aux_data(env, new_prog, off, len)) { + if (new_prog != env->prog) + bpf_prog_clone_free(new_prog); return NULL; + } adjust_subprog_starts(env, off, len); adjust_poke_descs(new_prog, off, len); return new_prog; -- 2.25.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [bpf-next 3/3] bpf: Fix a use after free in bpf_check() 2021-07-07 4:38 [bpf-next 0/3] potential memleak and use after free in bpf verifier He Fengqing 2021-07-07 4:38 ` [bpf-next 1/3] bpf: Move bpf_prog_clone_free into filter.h file He Fengqing 2021-07-07 4:38 ` [bpf-next 2/3] bpf: Fix a memory leak in an error handling path in 'bpf_patch_insn_data()' He Fengqing @ 2021-07-07 4:38 ` He Fengqing 2021-07-07 7:25 ` Song Liu 2 siblings, 1 reply; 13+ messages in thread From: He Fengqing @ 2021-07-07 4:38 UTC (permalink / raw) To: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend, kpsingh Cc: davem, kuba, netdev, bpf, linux-kernel 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(-) 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. -- 2.25.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [bpf-next 3/3] bpf: Fix a use after free in bpf_check() 2021-07-07 4:38 ` [bpf-next 3/3] bpf: Fix a use after free in bpf_check() He Fengqing @ 2021-07-07 7:25 ` Song Liu 2021-07-08 3:00 ` He Fengqing 0 siblings, 1 reply; 13+ messages in thread From: Song Liu @ 2021-07-07 7:25 UTC (permalink / raw) To: He Fengqing Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, David S . Miller, Jakub Kicinski, Networking, bpf, open list 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [bpf-next 3/3] bpf: Fix a use after free in bpf_check() 2021-07-07 7:25 ` Song Liu @ 2021-07-08 3:00 ` He Fengqing 2021-07-08 3:09 ` Alexei Starovoitov 0 siblings, 1 reply; 13+ messages in thread From: He Fengqing @ 2021-07-08 3:00 UTC (permalink / raw) To: Song Liu Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, David S . Miller, Jakub Kicinski, Networking, bpf, open list 在 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 > . > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [bpf-next 3/3] bpf: Fix a use after free in bpf_check() 2021-07-08 3:00 ` He Fengqing @ 2021-07-08 3:09 ` Alexei Starovoitov 2021-07-09 11:11 ` He Fengqing 0 siblings, 1 reply; 13+ messages in thread From: Alexei Starovoitov @ 2021-07-08 3:09 UTC (permalink / raw) To: He Fengqing Cc: Song Liu, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, David S . Miller, Jakub Kicinski, Networking, bpf, open list 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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [bpf-next 3/3] bpf: Fix a use after free in bpf_check() 2021-07-08 3:09 ` Alexei Starovoitov @ 2021-07-09 11:11 ` He Fengqing 2021-07-09 15:12 ` Alexei Starovoitov 0 siblings, 1 reply; 13+ messages in thread From: He Fengqing @ 2021-07-09 11:11 UTC (permalink / raw) To: Alexei Starovoitov Cc: Song Liu, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, David S . Miller, Jakub Kicinski, Networking, bpf, open list 在 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [bpf-next 3/3] bpf: Fix a use after free in bpf_check() 2021-07-09 11:11 ` He Fengqing @ 2021-07-09 15:12 ` Alexei Starovoitov 2021-07-12 2:17 ` He Fengqing 0 siblings, 1 reply; 13+ messages in thread From: Alexei Starovoitov @ 2021-07-09 15:12 UTC (permalink / raw) To: He Fengqing Cc: Song Liu, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, David S . Miller, Jakub Kicinski, Networking, bpf, open list 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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [bpf-next 3/3] bpf: Fix a use after free in bpf_check() 2021-07-09 15:12 ` Alexei Starovoitov @ 2021-07-12 2:17 ` He Fengqing 2021-07-13 23:17 ` Alexei Starovoitov 0 siblings, 1 reply; 13+ messages in thread From: He Fengqing @ 2021-07-12 2:17 UTC (permalink / raw) To: Alexei Starovoitov Cc: Song Liu, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, David S . Miller, Jakub Kicinski, Networking, bpf, open list 在 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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [bpf-next 3/3] bpf: Fix a use after free in bpf_check() 2021-07-12 2:17 ` He Fengqing @ 2021-07-13 23:17 ` Alexei Starovoitov 2021-07-14 1:53 ` He Fengqing 0 siblings, 1 reply; 13+ messages in thread From: Alexei Starovoitov @ 2021-07-13 23:17 UTC (permalink / raw) To: He Fengqing Cc: Song Liu, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, David S . Miller, Jakub Kicinski, Networking, bpf, open list 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [bpf-next 3/3] bpf: Fix a use after free in bpf_check() 2021-07-13 23:17 ` Alexei Starovoitov @ 2021-07-14 1:53 ` He Fengqing 0 siblings, 0 replies; 13+ messages in thread From: He Fengqing @ 2021-07-14 1:53 UTC (permalink / raw) To: Alexei Starovoitov Cc: Song Liu, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, David S . Miller, Jakub Kicinski, Networking, bpf, open list 在 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. > . > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-07-14 1:53 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-07 4:38 [bpf-next 0/3] potential memleak and use after free in bpf verifier He Fengqing 2021-07-07 4:38 ` [bpf-next 1/3] bpf: Move bpf_prog_clone_free into filter.h file He Fengqing 2021-07-07 7:02 ` Song Liu 2021-07-07 4:38 ` [bpf-next 2/3] bpf: Fix a memory leak in an error handling path in 'bpf_patch_insn_data()' He Fengqing 2021-07-07 4:38 ` [bpf-next 3/3] bpf: Fix a use after free in bpf_check() He Fengqing 2021-07-07 7:25 ` Song Liu 2021-07-08 3:00 ` He Fengqing 2021-07-08 3:09 ` Alexei Starovoitov 2021-07-09 11:11 ` He Fengqing 2021-07-09 15:12 ` Alexei Starovoitov 2021-07-12 2:17 ` He Fengqing 2021-07-13 23:17 ` Alexei Starovoitov 2021-07-14 1:53 ` He Fengqing
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).