diff mbox series

[bpf,7/7] bpf: make direct packet write unclone more robust

Message ID 20181024200549.8516-8-daniel@iogearbox.net
State Accepted, archived
Delegated to: BPF Maintainers
Headers show
Series Batch of direct packet access fixes for BPF | expand

Commit Message

Daniel Borkmann Oct. 24, 2018, 8:05 p.m. UTC 
Given this seems to be quite fragile and can easily slip through the
cracks, lets make direct packet write more robust by requiring that
future program types which allow for such write must provide a prologue
callback. In case of XDP and sk_msg it's noop, thus add a generic noop
handler there. The latter starts out with NULL data/data_end unconditionally
when sg pages are shared.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c |  6 +++++-
 net/core/filter.c     | 11 +++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

Comments

Song Liu Oct. 24, 2018, 9:42 p.m. UTC | #1 
On Wed, Oct 24, 2018 at 1:06 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Given this seems to be quite fragile and can easily slip through the
> cracks, lets make direct packet write more robust by requiring that
> future program types which allow for such write must provide a prologue
> callback. In case of XDP and sk_msg it's noop, thus add a generic noop
> handler there. The latter starts out with NULL data/data_end unconditionally
> when sg pages are shared.
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Acked-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  kernel/bpf/verifier.c |  6 +++++-
>  net/core/filter.c     | 11 +++++++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5fc9a65..171a2c8 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5709,7 +5709,11 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
>         bool is_narrower_load;
>         u32 target_size;
>
> -       if (ops->gen_prologue) {
> +       if (ops->gen_prologue || env->seen_direct_write) {
> +               if (!ops->gen_prologue) {
> +                       verbose(env, "bpf verifier is misconfigured\n");
> +                       return -EINVAL;
> +               }

nit: how about this?

diff --git i/kernel/bpf/verifier.c w/kernel/bpf/verifier.c
index 6fbe7a8afed7..d35078024e35 100644
--- i/kernel/bpf/verifier.c
+++ w/kernel/bpf/verifier.c
@@ -5286,6 +5286,11 @@ static int convert_ctx_accesses(struct
bpf_verifier_env *env)
        bool is_narrower_load;
        u32 target_size;

+       if (!ops->gen_prologue && env->seen_direct_write) {
+               verbose(env, "bpf verifier is misconfigured\n");
+               return -EINVAL;
+       }
+
        if (ops->gen_prologue) {
                cnt = ops->gen_prologue(insn_buf, env->seen_direct_write,
                                        env->prog);
Daniel Borkmann Oct. 24, 2018, 10:08 p.m. UTC | #2 
On 10/24/2018 11:42 PM, Song Liu wrote:
> On Wed, Oct 24, 2018 at 1:06 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> Given this seems to be quite fragile and can easily slip through the
>> cracks, lets make direct packet write more robust by requiring that
>> future program types which allow for such write must provide a prologue
>> callback. In case of XDP and sk_msg it's noop, thus add a generic noop
>> handler there. The latter starts out with NULL data/data_end unconditionally
>> when sg pages are shared.
>>
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> Acked-by: Alexei Starovoitov <ast@kernel.org>
>> ---
>>  kernel/bpf/verifier.c |  6 +++++-
>>  net/core/filter.c     | 11 +++++++++++
>>  2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 5fc9a65..171a2c8 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -5709,7 +5709,11 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
>>         bool is_narrower_load;
>>         u32 target_size;
>>
>> -       if (ops->gen_prologue) {
>> +       if (ops->gen_prologue || env->seen_direct_write) {
>> +               if (!ops->gen_prologue) {
>> +                       verbose(env, "bpf verifier is misconfigured\n");
>> +                       return -EINVAL;
>> +               }
> 
> nit: how about this?
> 
> diff --git i/kernel/bpf/verifier.c w/kernel/bpf/verifier.c
> index 6fbe7a8afed7..d35078024e35 100644
> --- i/kernel/bpf/verifier.c
> +++ w/kernel/bpf/verifier.c
> @@ -5286,6 +5286,11 @@ static int convert_ctx_accesses(struct
> bpf_verifier_env *env)
>         bool is_narrower_load;
>         u32 target_size;
> 
> +       if (!ops->gen_prologue && env->seen_direct_write) {
> +               verbose(env, "bpf verifier is misconfigured\n");
> +               return -EINVAL;
> +       }
> +
>         if (ops->gen_prologue) {
>                 cnt = ops->gen_prologue(insn_buf, env->seen_direct_write,
>                                         env->prog);
> 

Hm, probably matter of different style preference, personally I'd prefer
the one as is though.

Thanks,
Daniel
Song Liu Oct. 24, 2018, 11:36 p.m. UTC | #3 
On Wed, Oct 24, 2018 at 3:08 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 10/24/2018 11:42 PM, Song Liu wrote:
> > On Wed, Oct 24, 2018 at 1:06 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>
> >> Given this seems to be quite fragile and can easily slip through the
> >> cracks, lets make direct packet write more robust by requiring that
> >> future program types which allow for such write must provide a prologue
> >> callback. In case of XDP and sk_msg it's noop, thus add a generic noop
> >> handler there. The latter starts out with NULL data/data_end unconditionally
> >> when sg pages are shared.
> >>
> >> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> >> Acked-by: Alexei Starovoitov <ast@kernel.org>
> >> ---
> >>  kernel/bpf/verifier.c |  6 +++++-
> >>  net/core/filter.c     | 11 +++++++++++
> >>  2 files changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index 5fc9a65..171a2c8 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -5709,7 +5709,11 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
> >>         bool is_narrower_load;
> >>         u32 target_size;
> >>
> >> -       if (ops->gen_prologue) {
> >> +       if (ops->gen_prologue || env->seen_direct_write) {
> >> +               if (!ops->gen_prologue) {
> >> +                       verbose(env, "bpf verifier is misconfigured\n");
> >> +                       return -EINVAL;
> >> +               }
> >
> > nit: how about this?
> >
> > diff --git i/kernel/bpf/verifier.c w/kernel/bpf/verifier.c
> > index 6fbe7a8afed7..d35078024e35 100644
> > --- i/kernel/bpf/verifier.c
> > +++ w/kernel/bpf/verifier.c
> > @@ -5286,6 +5286,11 @@ static int convert_ctx_accesses(struct
> > bpf_verifier_env *env)
> >         bool is_narrower_load;
> >         u32 target_size;
> >
> > +       if (!ops->gen_prologue && env->seen_direct_write) {
> > +               verbose(env, "bpf verifier is misconfigured\n");
> > +               return -EINVAL;
> > +       }
> > +
> >         if (ops->gen_prologue) {
> >                 cnt = ops->gen_prologue(insn_buf, env->seen_direct_write,
> >                                         env->prog);
> >
>
> Hm, probably matter of different style preference, personally I'd prefer
> the one as is though.

Yeah, it is just a nitpick.

Thanks!

Acked-by: Song Liu <songliubraving@fb.com>
diff mbox series

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5fc9a65..171a2c8 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5709,7 +5709,11 @@  static int convert_ctx_accesses(struct bpf_verifier_env *env)
 	bool is_narrower_load;
 	u32 target_size;
 
-	if (ops->gen_prologue) {
+	if (ops->gen_prologue || env->seen_direct_write) {
+		if (!ops->gen_prologue) {
+			verbose(env, "bpf verifier is misconfigured\n");
+			return -EINVAL;
+		}
 		cnt = ops->gen_prologue(insn_buf, env->seen_direct_write,
 					env->prog);
 		if (cnt >= ARRAY_SIZE(insn_buf)) {
diff --git a/net/core/filter.c b/net/core/filter.c
index 3fdddfa..cd648d0 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5644,6 +5644,15 @@  static bool sock_filter_is_valid_access(int off, int size,
 					       prog->expected_attach_type);
 }
 
+static int bpf_noop_prologue(struct bpf_insn *insn_buf, bool direct_write,
+			     const struct bpf_prog *prog)
+{
+	/* Neither direct read nor direct write requires any preliminary
+	 * action.
+	 */
+	return 0;
+}
+
 static int bpf_unclone_prologue(struct bpf_insn *insn_buf, bool direct_write,
 				const struct bpf_prog *prog, int drop_verdict)
 {
@@ -7210,6 +7219,7 @@  const struct bpf_verifier_ops xdp_verifier_ops = {
 	.get_func_proto		= xdp_func_proto,
 	.is_valid_access	= xdp_is_valid_access,
 	.convert_ctx_access	= xdp_convert_ctx_access,
+	.gen_prologue		= bpf_noop_prologue,
 };
 
 const struct bpf_prog_ops xdp_prog_ops = {
@@ -7308,6 +7318,7 @@  const struct bpf_verifier_ops sk_msg_verifier_ops = {
 	.get_func_proto		= sk_msg_func_proto,
 	.is_valid_access	= sk_msg_is_valid_access,
 	.convert_ctx_access	= sk_msg_convert_ctx_access,
+	.gen_prologue		= bpf_noop_prologue,
 };
 
 const struct bpf_prog_ops sk_msg_prog_ops = {