Re: [PATCH v1 0/2] Add destructor hook to LSM modules

[Andy Shevchenko]

On Mon, Mar 13, 2023 at 04:27:42PM -0400, Paul Moore wrote:
> On Mon, Mar 13, 2023 at 5:33 AM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > On Sat, Mar 11, 2023 at 09:59:17AM -0500, Paul Moore wrote:
> > > On Fri, Mar 10, 2023 at 5:53 PM Mirsad Goran Todorovac
> > > <mirsad.todorovac@xxxxxxxxxxxx> wrote:

...

> > > With that out of the way, I wanted to make a quick comment on the
> > > patch itself.  Currently LSMs do not support unloading, or dynamic
> > > loading for that matter.  There are several reasons for this, but
> > > perhaps the most important is that in order to help meet the security
> > > goals for several of the LSMs they need to be present in the kernel
> > > from the very beginning and remain until the very end.  Adding a
> > > proper "release" method to a LSM is going to be far more complicated
> > > than what you've done with this patchset, involving a lot of
> > > discussion both for the LSM layer itself and all of the currently
> > > supported LSMs, and ultimately I don't believe it is something we will
> > > want to support.
> > >
> > > I appreciate your desire to help, and I want to thank you for your
> > > patch and the effort behind it, but I don't believe the kobject memory
> > > leak you saw at kernel shutdown was a real issue (it was only "leaked"
> > > because the system was shutting down) and I'm not sure the current
> > > behavior is something we want to change in the near future.
> >
> > Currently the security module so secure that even adds an unneeded noise to
> > the debugging output.
> >
> > At very least it would be nice to add a stub and put a big comment
> > (on your taste) explaining why we do not do anything there.
> >
> > Agree?
> 
> No.

Are you sure? I'm proposing to add a stub which is no-op, but with a comment
inside explaining why. In such case we:

1) shut the kobject infra up;
2) keep the status quo in LSM;
3) put in the code a good explanation for others on what's going on.

> At least not without a lot of additional work beyond what was
> presented in this patchset.  What about all of the other kobject
> caches created by other LSMs, this is more than just the IMA
> iint_cache.  I'm also skeptical that this patchset was ever tested and
> verified as the newly added release() method was never actually called
> from anywhere that I could see.

I'm not talking about this patchset, but you are right that it wasn't
tested.

> I think we would need to see a proper, verified fix before I could say
> for certain.

And continuing to spread the noise in the logs just because LSM is stubborn?

> If you want to discuss potential designs, we can do that
> too, but please remember the constraints that were already mentioned
> about intentionally not allowing the LSMs to be unloaded (prior to
> system shutdown).
> 
> I don't know the answer to this, but I'm guessing the LSMs aren't the
> only kernel subsystems to "leak" memory on system shutdown; working on
> the assumption that this is the case, how are those "leaked"
> allocations handled?

Note, I'm full for the proper fix, but the current issue is logs flooding done
by LSM that needs to be addressed.

-- 
With Best Regards,
Andy Shevchenko