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

[Paul Moore]

On Tue, Mar 14, 2023 at 7:05 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> 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.

If you want to introduce a change to add a release method, do it
properly so it does the right thing for all the LSMs and not just the
one you happen to care about at this moment.  If you don't do the
change properly it means I'm going to have to complete the work and
I've got more important things relating to the LSM that need my
attention.

-- 
paul-moore.com