netdev - Re: [PATCH bpf-next 2/3] bpf: Add verifier checks for bpf

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: <CAEf4BzYgxR38405LoyVrwPAzdiax-Fbu+2hNrqPa_6xSnoGWiA@mail.gmail.com>
Date:   Thu, 3 Jun 2021 10:35:00 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next 2/3] bpf: Add verifier checks for bpf_timer.

On Wed, Jun 2, 2021 at 7:04 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Wed, Jun 02, 2021 at 03:34:29PM -0700, Andrii Nakryiko wrote:
> >
> > >  /* copy everything but bpf_spin_lock */
> > >  static inline void copy_map_value(struct bpf_map *map, void *dst, void *src)
> > >  {
> > > +       u32 off = 0, size = 0;
> > > +
> > >         if (unlikely(map_value_has_spin_lock(map))) {
> > > -               u32 off = map->spin_lock_off;
> > > +               off = map->spin_lock_off;
> > > +               size = sizeof(struct bpf_spin_lock);
> > > +       } else if (unlikely(map_value_has_timer(map))) {
> > > +               off = map->timer_off;
> > > +               size = sizeof(struct bpf_timer);
> > > +       }
> >
> > so the need to handle 0, 1, or 2 gaps seems to be the only reason to
> > disallow both bpf_spinlock and bpf_timer in one map element, right?
>
> exactly.
>
> > Isn't it worth addressing it from the very beginning to lift the
> > artificial restriction? E.g., for speed, you'd do:
> >
> > if (likely(neither spinlock nor timer)) {
> >  /* fastest pass */
> > } else if (only one of spinlock or timer) {
> >   /* do what you do here */
> > } else {
> >   int off1, off2, sz1, sz2;
> >
> >   if (spinlock_off < timer_off) {
> >     off1 = spinlock_off;
> >     sz1 = spinlock_sz;
> >     off2 = timer_off;
> >     sz2 = timer_sz;
> >   } else {
> >     ... you get the idea
>
> Not really :)

hm, really? I meant that else will be:

     off1 = timer_off;
     sz1 = timer_sz;
     off2 = spinlock_off;
     sz2 = spinlock_sz;

Just making sure that off1 < off2 always and sz1 and sz2 are matching

> Are you suggesting to support one bpf_spin_lock and one
> bpf_timer inside single map element, but not two spin_locks
> and/or not two bpf_timers?

Yes, exactly. I see bpf_spinlock and bpf_timer as two independent
orthogonal features and I don't understand why we restrict using just
one of them in a given map element. I think those 20 lines of code
that decouples them and removes artificial restriction that users need
to remember (or discover with surprise) is totally worth it.

> Two me it's either one or support any.

I think it's fine to start with supporting one. But one of each. They
are independent of each other.

> Anything in-between doesn't seem worth extra code.

Up to you, but I disagree, obviously. It's possible to work-around
that limitation with extra maps/complexity, so if I ever need to both
lock an element and schedule the timer with it, it's not going to stop
me. :)

>
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index f386f85aee5c..0a828dc4968e 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -3241,6 +3241,15 @@ static int check_map_access(struct bpf_verifier_env *env, u32 regno,
> > >                         return -EACCES;
> > >                 }
> > >         }
> > > +       if (map_value_has_timer(map)) {
> > > +               u32 t = map->timer_off;
> > > +
> > > +               if (reg->smin_value + off < t + sizeof(struct bpf_timer) &&
> >
> > <= ? Otherwise we allow accessing the first byte, unless I'm mistaken.
>
> I don't think so. See the comment above in if (map_value_has_spin_lock(map))
> I didn't copy-paste it, because it's the same logic.

Oh, I didn't realize that this is the interval intersection check I
suggested a long time ago :) yeah, that still looks correct

>
> > > -       if (val) {
> > > -               /* todo: relax this requirement */
> > > -               verbose(env, "bpf_timer field can only be first in the map value element\n");
> >
> > ok, this was confusing, but now I see why you did that...
>
> I'll clarify the comment to say that the next patch fixes it.

ok, thanks