| Message ID | 20210714101815.164322-1-hefengqing@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v2] bpf: verifier: Fix potential memleak and UAF 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 11 of 11 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: 30 this patch: 30 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | fail | Link |
| netdev/checkpatch | warning | WARNING: line length of 81 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 30 this patch: 30 |
| netdev/header_inline | success | Link |
On Wed, Jul 14, 2021 at 2:33 AM He Fengqing <hefengqing@huawei.com> wrote: > > In bpf_patch_insn_data(), we first use the bpf_patch_insn_single() to > insert new instructions, then use adjust_insn_aux_data() to adjust > insn_aux_data. If the old env->prog have no enough room for new inserted > instructions, we use bpf_prog_realloc to construct new_prog and free the > old env->prog. > > There have two errors here. First, if adjust_insn_aux_data() return > ENOMEM, we should free the new_prog. Second, if adjust_insn_aux_data() > return ENOMEM, bpf_patch_insn_data() will return NULL, and env->prog has > been freed in bpf_prog_realloc, but we will use it in bpf_check(). > > So in this patch, we make the adjust_insn_aux_data() never fails. In > bpf_patch_insn_data(), we first pre-malloc memory for the new > insn_aux_data, then call bpf_patch_insn_single() to insert new > instructions, at last call adjust_insn_aux_data() to adjust > insn_aux_data. > > Fixes: 8041902dae52 ("bpf: adjust insn_aux_data when patching insns") > > Signed-off-by: He Fengqing <hefengqing@huawei.com> Acked-by: Song Liu <songliubraving@fb.com> with one nitpick below. > > v1->v2: > pre-malloc memory for new insn_aux_data first, then > adjust_insn_aux_data() will never fails. > --- > kernel/bpf/verifier.c | 30 +++++++++++++++++++----------- > 1 file changed, 19 insertions(+), 11 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index be38bb930bf1..07cf791510f1 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -11425,10 +11425,11 @@ static void convert_pseudo_ld_imm64(struct bpf_verifier_env *env) > * insni[off, off + cnt). Adjust corresponding insn_aux_data by copying > * [0, off) and [off, end) to new locations, so the patched range stays zero > */ > -static int adjust_insn_aux_data(struct bpf_verifier_env *env, > - struct bpf_prog *new_prog, u32 off, u32 cnt) > +static void adjust_insn_aux_data(struct bpf_verifier_env *env, > + struct bpf_insn_aux_data *new_data, > + struct bpf_prog *new_prog, u32 off, u32 cnt) > { > - struct bpf_insn_aux_data *new_data, *old_data = env->insn_aux_data; > + struct bpf_insn_aux_data *old_data = env->insn_aux_data; > struct bpf_insn *insn = new_prog->insnsi; > u32 old_seen = old_data[off].seen; > u32 prog_len; > @@ -11441,12 +11442,9 @@ static int adjust_insn_aux_data(struct bpf_verifier_env *env, > old_data[off].zext_dst = insn_has_def32(env, insn + off + cnt - 1); > > if (cnt == 1) > - return 0; > + return; > prog_len = new_prog->len; > - new_data = vzalloc(array_size(prog_len, > - sizeof(struct bpf_insn_aux_data))); > - if (!new_data) > - return -ENOMEM; > + > memcpy(new_data, old_data, sizeof(struct bpf_insn_aux_data) * off); > memcpy(new_data + off + cnt - 1, old_data + off, > sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1)); > @@ -11457,7 +11455,7 @@ static int adjust_insn_aux_data(struct bpf_verifier_env *env, > } > env->insn_aux_data = new_data; > vfree(old_data); > - return 0; > + return; No need to say return here. > } > > static void adjust_subprog_starts(struct bpf_verifier_env *env, u32 off, u32 len) > @@ -11492,6 +11490,14 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of > const struct bpf_insn *patch, u32 len) > { > struct bpf_prog *new_prog; > + struct bpf_insn_aux_data *new_data = NULL; > + > + if (len > 1) { > + new_data = vzalloc(array_size(env->prog->len + len - 1, > + sizeof(struct bpf_insn_aux_data))); > + if (!new_data) > + return NULL; > + } > > new_prog = bpf_patch_insn_single(env->prog, off, patch, len); > if (IS_ERR(new_prog)) { > @@ -11499,10 +11505,12 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of > verbose(env, > "insn %d cannot be patched due to 16-bit range\n", > env->insn_aux_data[off].orig_idx); > + if (new_data) > + vfree(new_data); > + > return NULL; > } > - if (adjust_insn_aux_data(env, new_prog, off, len)) > - return NULL; > + adjust_insn_aux_data(env, new_data, new_prog, off, len); > adjust_subprog_starts(env, off, len); > adjust_poke_descs(new_prog, off, len); > return new_prog; > -- > 2.25.1 >
On Wed, Jul 14, 2021 at 5:54 PM Song Liu <song@kernel.org> wrote: > > - return 0; > > + return; > No need to say return here. > > > } > > > > static void adjust_subprog_starts(struct bpf_verifier_env *env, u32 off, u32 len) > > @@ -11492,6 +11490,14 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of > > const struct bpf_insn *patch, u32 len) > > { > > struct bpf_prog *new_prog; > > + struct bpf_insn_aux_data *new_data = NULL; > > + > > + if (len > 1) { > > + new_data = vzalloc(array_size(env->prog->len + len - 1, > > + sizeof(struct bpf_insn_aux_data))); > > + if (!new_data) > > + return NULL; I removed the redundant 'return' that Song pointed out and the redundant 'if' above. And applied to bpf-next. Though it's a fix, I think it's ok to go via bpf-next, since even syzbot didn't find it.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index be38bb930bf1..07cf791510f1 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -11425,10 +11425,11 @@ static void convert_pseudo_ld_imm64(struct bpf_verifier_env *env) * insni[off, off + cnt). Adjust corresponding insn_aux_data by copying * [0, off) and [off, end) to new locations, so the patched range stays zero */ -static int adjust_insn_aux_data(struct bpf_verifier_env *env, - struct bpf_prog *new_prog, u32 off, u32 cnt) +static void adjust_insn_aux_data(struct bpf_verifier_env *env, + struct bpf_insn_aux_data *new_data, + struct bpf_prog *new_prog, u32 off, u32 cnt) { - struct bpf_insn_aux_data *new_data, *old_data = env->insn_aux_data; + struct bpf_insn_aux_data *old_data = env->insn_aux_data; struct bpf_insn *insn = new_prog->insnsi; u32 old_seen = old_data[off].seen; u32 prog_len; @@ -11441,12 +11442,9 @@ static int adjust_insn_aux_data(struct bpf_verifier_env *env, old_data[off].zext_dst = insn_has_def32(env, insn + off + cnt - 1); if (cnt == 1) - return 0; + return; prog_len = new_prog->len; - new_data = vzalloc(array_size(prog_len, - sizeof(struct bpf_insn_aux_data))); - if (!new_data) - return -ENOMEM; + memcpy(new_data, old_data, sizeof(struct bpf_insn_aux_data) * off); memcpy(new_data + off + cnt - 1, old_data + off, sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1)); @@ -11457,7 +11455,7 @@ static int adjust_insn_aux_data(struct bpf_verifier_env *env, } env->insn_aux_data = new_data; vfree(old_data); - return 0; + return; } static void adjust_subprog_starts(struct bpf_verifier_env *env, u32 off, u32 len) @@ -11492,6 +11490,14 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of const struct bpf_insn *patch, u32 len) { struct bpf_prog *new_prog; + struct bpf_insn_aux_data *new_data = NULL; + + if (len > 1) { + new_data = vzalloc(array_size(env->prog->len + len - 1, + sizeof(struct bpf_insn_aux_data))); + if (!new_data) + return NULL; + } new_prog = bpf_patch_insn_single(env->prog, off, patch, len); if (IS_ERR(new_prog)) { @@ -11499,10 +11505,12 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of verbose(env, "insn %d cannot be patched due to 16-bit range\n", env->insn_aux_data[off].orig_idx); + if (new_data) + vfree(new_data); + return NULL; } - if (adjust_insn_aux_data(env, new_prog, off, len)) - return NULL; + adjust_insn_aux_data(env, new_data, new_prog, off, len); adjust_subprog_starts(env, off, len); adjust_poke_descs(new_prog, off, len); return new_prog;
In bpf_patch_insn_data(), we first use the bpf_patch_insn_single() to insert new instructions, then use adjust_insn_aux_data() to adjust insn_aux_data. If the old env->prog have no enough room for new inserted instructions, we use bpf_prog_realloc to construct new_prog and free the old env->prog. There have two errors here. First, if adjust_insn_aux_data() return ENOMEM, we should free the new_prog. Second, if adjust_insn_aux_data() return ENOMEM, bpf_patch_insn_data() will return NULL, and env->prog has been freed in bpf_prog_realloc, but we will use it in bpf_check(). So in this patch, we make the adjust_insn_aux_data() never fails. In bpf_patch_insn_data(), we first pre-malloc memory for the new insn_aux_data, then call bpf_patch_insn_single() to insert new instructions, at last call adjust_insn_aux_data() to adjust insn_aux_data. Fixes: 8041902dae52 ("bpf: adjust insn_aux_data when patching insns") Signed-off-by: He Fengqing <hefengqing@huawei.com> v1->v2: pre-malloc memory for new insn_aux_data first, then adjust_insn_aux_data() will never fails. --- kernel/bpf/verifier.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)