netdev - Re: [PATCHv4 bpf-next 07/13] bpf: Add reference tracking to verifier

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] [day] [month] [year] [list]

Message-ID: <CAOftzPggHHWz3_v5bdRZMpqr1k8tucFCbzC8vHbgV0bMyUEu1A@mail.gmail.com>
Date:   Thu, 10 Jan 2019 11:26:44 -0800
From:   Joe Stringer <joe@...d.net.nz>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Joe Stringer <joe@...d.net.nz>,
        Daniel Borkmann <daniel@...earbox.net>,
        netdev <netdev@...r.kernel.org>, ast@...nel.org,
        john fastabend <john.fastabend@...il.com>
Subject: Re: [PATCHv4 bpf-next 07/13] bpf: Add reference tracking to verifier

[resend as apparently I untoggled the "plain text" option in gmail...]

On Tue, 8 Jan 2019 at 23:42, Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Tue, Oct 02, 2018 at 01:35:35PM -0700, Joe Stringer wrote:
> > Allow helper functions to acquire a reference and return it into a
> > register. Specific pointer types such as the PTR_TO_SOCKET will
> > implicitly represent such a reference. The verifier must ensure that
> > these references are released exactly once in each path through the
> > program.
> >
> > To achieve this, this commit assigns an id to the pointer and tracks it
> > in the 'bpf_func_state', then when the function or program exits,
> > verifies that all of the acquired references have been freed. When the
> > pointer is passed to a function that frees the reference, it is removed
> > from the 'bpf_func_state` and all existing copies of the pointer in
> > registers are marked invalid.
> >
> > Signed-off-by: Joe Stringer <joe@...d.net.nz>
> > Acked-by: Alexei Starovoitov <ast@...nel.org>
> ...
> > +static void release_reg_references(struct bpf_verifier_env *env,
> > +                                struct bpf_func_state *state, int id)
> > +{
> > +     struct bpf_reg_state *regs = state->regs, *reg;
> > +     int i;
> > +
> > +     for (i = 0; i < MAX_BPF_REG; i++)
> > +             if (regs[i].id == id)
> > +                     mark_reg_unknown(env, regs, i);
> > +
> > +     bpf_for_each_spilled_reg(i, state, reg) {
> > +             if (!reg)
> > +                     continue;
> > +             if (reg_is_refcounted(reg) && reg->id == id)
> > +                     __mark_reg_unknown(reg);
> > +     }
> > +}
>
> Hi Joe,
>
> I've been looking at this function again and wondering why second reg->id == id
> check needs additional reg_is_refcounted() check?
> No tests have failed when I removed it.
> If reg->id is equal to id being released we need to clear the reg regardless
> whethere it's in the registers (the first loop) or
> whether it's in the stack (second loop).
> I think when reg->id == id that reg is guaranteed to be refcounted.
> Would you agree?

Yes,  the id should only ever be allocated once so if we end up
attempting to release register references to it, that should already
imply that the id identifies a reference and not some other pointer
type.

>
> I propose to simply remove that unnecessary reg_is_refcounted(reg) check.
> We can replace it with warn_on, but imo that's overkill.
> I'm thinking to repurpose release_reg_references() function for my bpf_spin_lock work
> and that check is in the way.

SGTM, looks entirely unnecessary to me.

Cheers,
Joe