Date:   Tue, 14 Mar 2023 13:05:25 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Paul Moore <paul@paul-moore.com>
Cc:     Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>,
        Mirsad Goran Todorovac <mirsad.goran.todorovac@alu.hr>,
        linux-kernel@vger.kernel.org, linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Thomas =?iso-8859-1?Q?Wei=DFschuh?= <linux@weissschuh.net>,
        Mimi Zohar <zohar@linux.ibm.com>,
        Casey Schaufler <casey@schaufler-ca.com>,
        Christian =?iso-8859-1?Q?G=F6ttsche?= <cgzones@googlemail.com>,
        =?iso-8859-1?Q?Micka=EBl_Sala=FCn?= <mic@digikod.net>,
        Frederick Lawler <fred@cloudflare.com>
Subject: Re: [PATCH v1 0/2] Add destructor hook to LSM modules
Message-ID: <ZBBU9diKqetWQztO@smile.fi.intel.com>
References: <20230310192614.GA528@domac.alu.hr>
 <CAHC9VhSzppHevG_Td+hKU4KRSDgV_NYf2RSeKA06PR-rD+dJLw@mail.gmail.com>
 <c1c1cbcc-10b6-de3f-81e8-78e6b173d46f@alu.unizg.hr>
 <CAHC9VhRFQtqTZku==BkW0uz1oZgG63j15GoQD1iexW4aPoAPcA@mail.gmail.com>
 <ZA7tyrscjwavzY3a@smile.fi.intel.com>
 <CAHC9VhTMoCAFhaa36Bq7_jiKGiaeMbYTuWv7tTQP1OHpY0EUsg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAHC9VhTMoCAFhaa36Bq7_jiKGiaeMbYTuWv7tTQP1OHpY0EUsg@mail.gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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@linux.intel.com> 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@alu.unizg.hr> 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