netdev - Re: [PATCH bpf 7/7] bpf: make direct packet write unclone more robust

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPhsuW4jvG9KyvcdRF_KNShsa=28T1Q8qSJ=gB-hWe-DoggCCg@mail.gmail.com>
Date:   Wed, 24 Oct 2018 16:36:22 -0700
From:   Song Liu <liu.song.a23@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf 7/7] bpf: make direct packet write unclone more robust

On Wed, Oct 24, 2018 at 3:08 PM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 10/24/2018 11:42 PM, Song Liu wrote:
> > On Wed, Oct 24, 2018 at 1:06 PM Daniel Borkmann <daniel@...earbox.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@...earbox.net>
> >> Acked-by: Alexei Starovoitov <ast@...nel.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@...com>