On Thu, Dec 22, 2022 at 4:28 PM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
> On Thu, Dec 22, 2022 at 12:07 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Thu, Dec 22, 2022 at 2:59 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > On Thu, Dec 22, 2022 at 2:40 PM <sdf@xxxxxxxxxx> wrote:
> > > > On 12/22, Paul Moore wrote:
> > > > > On Thu, Dec 22, 2022 at 12:19 PM <sdf@xxxxxxxxxx> 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
