Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* [bpf-next, v2] bpf: verifier: Fix potential memleak and UAF in bpf verifier
@ 2021-07-14 10:18 He Fengqing
  2021-07-15  0:54 ` Song Liu
  0 siblings, 1 reply; 3+ messages in thread
From: He Fengqing @ 2021-07-14 10:18 UTC (permalink / raw)
  To: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend, kpsingh
  Cc: davem, netdev, bpf, linux-kernel

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(-)

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [bpf-next, v2] bpf: verifier: Fix potential memleak and UAF in bpf verifier
  2021-07-14 10:18 [bpf-next, v2] bpf: verifier: Fix potential memleak and UAF in bpf verifier He Fengqing
@ 2021-07-15  0:54 ` Song Liu
  2021-07-15  1:34   ` Alexei Starovoitov
  0 siblings, 1 reply; 3+ messages in thread
From: Song Liu @ 2021-07-15  0:54 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, Networking, bpf, open list

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
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [bpf-next, v2] bpf: verifier: Fix potential memleak and UAF in bpf verifier
  2021-07-15  0:54 ` Song Liu
@ 2021-07-15  1:34   ` Alexei Starovoitov
  0 siblings, 0 replies; 3+ messages in thread
From: Alexei Starovoitov @ 2021-07-15  1:34 UTC (permalink / raw)
  To: Song Liu
  Cc: He Fengqing, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, David S . Miller, Networking, bpf,
	open list

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.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-07-15  1:34 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-14 10:18 [bpf-next, v2] bpf: verifier: Fix potential memleak and UAF in bpf verifier He Fengqing
2021-07-15  0:54 ` Song Liu
2021-07-15  1:34   ` Alexei Starovoitov

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).