Re: [PATCH] bpf: restore the ebpf audit UNLOAD id field
On Thu, Dec 22, 2022 at 4:28 PM Stanislav Fomichev <sdf(a)google.com> wrote:
On Thu, Dec 22, 2022 at 12:07 PM Paul Moore
<paul(a)paul-moore.com> wrote:
> On Thu, Dec 22, 2022 at 2:59 PM Paul Moore <paul(a)paul-moore.com> wrote:
> > On Thu, Dec 22, 2022 at 2:40 PM <sdf(a)google.com> wrote:
> > > On 12/22, Paul Moore wrote:
> > > > On Thu, Dec 22, 2022 at 12:19 PM <sdf(a)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