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

On Thu, Dec 22, 2022 at 4:28 PM Stanislav Fomichev <[email protected]> wrote:
> On Thu, Dec 22, 2022 at 12:07 PM Paul Moore <[email protected]> wrote:
> > On Thu, Dec 22, 2022 at 2:59 PM Paul Moore <[email protected]> wrote:
> > > On Thu, Dec 22, 2022 at 2:40 PM <[email protected]> wrote:
> > > > On 12/22, Paul Moore wrote:
> > > > > On Thu, Dec 22, 2022 at 12:19 PM <[email protected]> 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

--
Linux-audit mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/linux-audit