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 <[email protected]>
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;
}
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 2:33 AM He Fengqing <[email protected]> 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 <[email protected]>
Acked-by: Song Liu <[email protected]>
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 <[email protected]> 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.