MIME-Version: 1.0
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>
In-Reply-To: <ZA7tyrscjwavzY3a@smile.fi.intel.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Mon, 13 Mar 2023 16:27:42 -0400
Message-ID: <CAHC9VhTMoCAFhaa36Bq7_jiKGiaeMbYTuWv7tTQP1OHpY0EUsg@mail.gmail.com>
Subject: Re: [PATCH v1 0/2] Add destructor hook to LSM modules
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.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>,
        =?UTF-8?Q?Thomas_Wei=C3=9Fschuh?= <linux@weissschuh.net>,
        Mimi Zohar <zohar@linux.ibm.com>,
        Casey Schaufler <casey@schaufler-ca.com>,
        =?UTF-8?Q?Christian_G=C3=B6ttsche?= <cgzones@googlemail.com>,
        =?UTF-8?B?TWlja2HDq2wgU2FsYcO8bg==?= <mic@digikod.net>,
        Frederick Lawler <fred@cloudflare.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Mar 13, 2023 at 5:33=E2=80=AFAM 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=E2=80=AFPM 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.  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 think we would need to see a proper, verified fix before I could say
for certain.  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?

--
paul-moore.com