Re: [PATCH] bpf: restore the ebpf audit UNLOAD id field

From: Paul Moore <paul@paul-moore.com>
To: Stanislav Fomichev <sdf@google.com>
Cc: linux-audit@redhat.com, bpf@vger.kernel.org,
	Alexei Starovoitov <ast@kernel.org>,
	Burn Alting <burn.alting@iinet.net.au>
Subject: Re: [PATCH] bpf: restore the ebpf audit UNLOAD id field
Date: Fri, 23 Dec 2022 10:30:08 -0500	[thread overview]
Message-ID: <CAHC9VhTdRC8VqrnnHaM=jBtrgdb2KrqM7-4Z==qcQTosbXfJMg@mail.gmail.com> (raw)
In-Reply-To: <CAKH8qBszD=PYO_nVjYUTnj7UXVcBvA95meULQGs53eyo9xfD+A@mail.gmail.com>

On Thu, Dec 22, 2022 at 4:28 PM Stanislav Fomichev <sdf@google.com> wrote:
> On Thu, Dec 22, 2022 at 12:07 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Thu, Dec 22, 2022 at 2:59 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Thu, Dec 22, 2022 at 2:40 PM <sdf@google.com> wrote:
> > > > On 12/22, Paul Moore wrote:
> > > > > On Thu, Dec 22, 2022 at 12:19 PM <sdf@google.com> wrote:
> > > > > > On 12/21, Paul Moore wrote:

...

> > > FWIW, the currently-work-in-progress v2 patch adds a getter for the ID
> > > with a WARN() check to flag callers who are trying to access a
> > > bad/free'd bpf_prog.  Unfortunately it touches a decent chunk of code,
> > > but I think it might be a nice additional check at runtime.
> > >
> > > +u32 bpf_prog_get_id(const struct bpf_prog *prog)
> > > +{
> > > +       if (WARN(!prog->valid_id, "Attempting to use invalid eBPF program"))
> > > +               return 0;
> > > +       return prog->aux->__id;
> > > +}
> >
> > I should add that the getter is currently a static inline in bpf.h.
>
> I don't see why we need to WARN on !valid_id, but I might be missing something.
> There are no places currently where we report 'id == 0' to the
> userspace, so we only need to take care of the offloaded case that
> resets id to zero early (instead of resetting it during regular
> __bpf_prog_put path).

I put the WARN there, in place of a normal 'if (!prog->valid_id)', as
an extra runtime check/debug-tool for those who have CONFIG_BUG
enabled.  I'm sure everything works properly now with respect to not
using a bpf_prog reference after it has been free'd/released, but
mistakes do happen - look at the regression/bug that started this
thread :)

If you really don't want the WARN() there, I can replace it with the
simple '!prog->valid_id' check, let me know.  It's your code, you
should maintain it how you want; I just want to make sure we are
generating audit records correctly.

> > > > > I'm not seeing any other comments, so I'll go ahead with putting
> > > > > together a v2 that sets an invalid flag/bit and I'll post that for
> > > > > further discussion/review.

-- 
paul-moore.com