LSM security/integrity/iint.c had the case of kmem_cache_create() w/o a proper
kmem_cache_destroy() destructor.
Introducing the release() hook would enable LSMs to release allocated resources
on exit, and in proper order, rather than dying all together with kernel shutdown
in an undefined order.
Thanks,
Mirsad
---
include/linux/lsm_hooks.h | 1 +
security/integrity/iint.c | 7 +++++++
2 files changed, 8 insertions(+)
--
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia
The European Union
On Fri, Mar 10, 2023 at 08:26:14PM +0100, Mirsad Goran Todorovac wrote:
>
> LSM security/integrity/iint.c had the case of kmem_cache_create() w/o a proper
> kmem_cache_destroy() destructor.
>
> Introducing the release() hook would enable LSMs to release allocated resources
> on exit, and in proper order, rather than dying all together with kernel shutdown
> in an undefined order.
Your patches are sent as separate emails. Have you forgotten to add --thread
to `git format-patch`?
--
With Best Regards,
Andy Shevchenko
On 3/10/2023 11:26 AM, Mirsad Goran Todorovac wrote:
> LSM security/integrity/iint.c had the case of kmem_cache_create() w/o a proper
> kmem_cache_destroy() destructor.
LSMs should be using the security blobs associated with system objects
rather than doing their own memory management.
>
> Introducing the release() hook would enable LSMs to release allocated resources
> on exit, and in proper order, rather than dying all together with kernel shutdown
> in an undefined order.
>
> Thanks,
> Mirsad
>
> ---
> include/linux/lsm_hooks.h | 1 +
> security/integrity/iint.c | 7 +++++++
> 2 files changed, 8 insertions(+)
>
> --
> Mirsad Goran Todorovac
> Sistem inženjer
> Grafički fakultet | Akademija likovnih umjetnosti
> Sveučilište u Zagrebu
>
> System engineer
> Faculty of Graphic Arts | Academy of Fine Arts
> University of Zagreb, Republic of Croatia
> The European Union
On Fri, Mar 10, 2023 at 2:26 PM Mirsad Goran Todorovac
<[email protected]> wrote:
>
> LSM security/integrity/iint.c had the case of kmem_cache_create() w/o a proper
> kmem_cache_destroy() destructor.
>
> Introducing the release() hook would enable LSMs to release allocated resources
> on exit, and in proper order, rather than dying all together with kernel shutdown
> in an undefined order.
>
> Thanks,
> Mirsad
>
> ---
> include/linux/lsm_hooks.h | 1 +
> security/integrity/iint.c | 7 +++++++
> 2 files changed, 8 insertions(+)
I only see the 1/2 patch, did you send the 2/2 patch to the LSM list?
If not, you need to do that.
--
paul-moore.com
On 10. 03. 2023. 23:33, Paul Moore wrote:
> On Fri, Mar 10, 2023 at 2:26 PM Mirsad Goran Todorovac
> <[email protected]> wrote:
>>
>> LSM security/integrity/iint.c had the case of kmem_cache_create() w/o a proper
>> kmem_cache_destroy() destructor.
>>
>> Introducing the release() hook would enable LSMs to release allocated resources
>> on exit, and in proper order, rather than dying all together with kernel shutdown
>> in an undefined order.
>>
>> Thanks,
>> Mirsad
>>
>> ---
>> include/linux/lsm_hooks.h | 1 +
>> security/integrity/iint.c | 7 +++++++
>> 2 files changed, 8 insertions(+)
>
> I only see the 1/2 patch, did you send the 2/2 patch to the LSM list?
> If not, you need to do that
I will resend everything, because this first attempt was buggy and
incorrect regarding the credits. Will try this, Andy said that I wait
for the comments, but I did not expect them before the weekend.
Thank you guys for patience, I am just finishing the test of devres
patch and then I will proceed with integrity LSM patch submission.
Best regards,
Mirsad
--
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia
The European Union
On Fri, Mar 10, 2023 at 5:53 PM Mirsad Goran Todorovac
<[email protected]> wrote:
> On 10. 03. 2023. 23:33, Paul Moore wrote:
> > On Fri, Mar 10, 2023 at 2:26 PM Mirsad Goran Todorovac
> > <[email protected]> wrote:
> >>
> >> LSM security/integrity/iint.c had the case of kmem_cache_create() w/o a proper
> >> kmem_cache_destroy() destructor.
> >>
> >> Introducing the release() hook would enable LSMs to release allocated resources
> >> on exit, and in proper order, rather than dying all together with kernel shutdown
> >> in an undefined order.
> >>
> >> Thanks,
> >> Mirsad
> >>
> >> ---
> >> include/linux/lsm_hooks.h | 1 +
> >> security/integrity/iint.c | 7 +++++++
> >> 2 files changed, 8 insertions(+)
> >
> > I only see the 1/2 patch, did you send the 2/2 patch to the LSM list?
> > If not, you need to do that
>
> I will resend everything, because this first attempt was buggy and
> incorrect regarding the credits. Will try this, Andy said that I wait
> for the comments, but I did not expect them before the weekend.
>
> Thank you guys for patience, I am just finishing the test of devres
> patch and then I will proceed with integrity LSM patch submission.
Thank you for resending the patchset, although a couple of
administrative things to consider for your next posting:
* Each time you post a patchset it is generally considered a best
practice to increment the version number, e.g. "[PATCH vX 0/2]" with
'X' being the version number. This makes it easier to identify
specific revisions and ensure that everyone is reviewing the latest
patchset.
* It is a good idea to use the "./scripts/get_maintainer.pl" script to
ensure you have included the right people and mailing lists on your
submissions as your latest resend did not include the LSM list when it
should have.
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.
--
paul-moore.com
On 11. 03. 2023. 15:59, Paul Moore wrote:
> On Fri, Mar 10, 2023 at 5:53 PM Mirsad Goran Todorovac
> <[email protected]> wrote:
>> On 10. 03. 2023. 23:33, Paul Moore wrote:
>>> On Fri, Mar 10, 2023 at 2:26 PM Mirsad Goran Todorovac
>>> <[email protected]> wrote:
>>>>
>>>> LSM security/integrity/iint.c had the case of kmem_cache_create() w/o a proper
>>>> kmem_cache_destroy() destructor.
>>>>
>>>> Introducing the release() hook would enable LSMs to release allocated resources
>>>> on exit, and in proper order, rather than dying all together with kernel shutdown
>>>> in an undefined order.
>>>>
>>>> Thanks,
>>>> Mirsad
>>>>
>>>> ---
>>>> include/linux/lsm_hooks.h | 1 +
>>>> security/integrity/iint.c | 7 +++++++
>>>> 2 files changed, 8 insertions(+)
>>>
>>> I only see the 1/2 patch, did you send the 2/2 patch to the LSM list?
>>> If not, you need to do that
>>
>> I will resend everything, because this first attempt was buggy and
>> incorrect regarding the credits. Will try this, Andy said that I wait
>> for the comments, but I did not expect them before the weekend.
>>
>> Thank you guys for patience, I am just finishing the test of devres
>> patch and then I will proceed with integrity LSM patch submission.
>
> Thank you for resending the patchset, although a couple of
> administrative things to consider for your next posting:
>
> * Each time you post a patchset it is generally considered a best
> practice to increment the version number, e.g. "[PATCH vX 0/2]" with
> 'X' being the version number. This makes it easier to identify
> specific revisions and ensure that everyone is reviewing the latest
> patchset.
>
> * It is a good idea to use the "./scripts/get_maintainer.pl" script to
> ensure you have included the right people and mailing lists on your
> submissions as your latest resend did not include the LSM list when it
> should have.
Thank you for the advice, Mr. Moore.
It is really my second attempt at patch submission. I've seen
later that other contributors add RESEND or increase the patch
set version.
For the maintainer list, I have trusted the output of a script.
But yes, it's probably best to check thoroughly twice.
> 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 have already realised that simply calling kmem_cache_destroy()
would not even deallocate all possibly allocated subnodes which
I saw from the source.
It is based on the idea of the userspace programs in which I
always relinquish memory on the heap instead of relying in just
exit(). (This also allows greater flexibility with signals,
but that is OT now.)
If this is bound not to cause any problems with TPMs or vTMPs
on the virtual machines, maybe it's a non-issue.
> 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.
Yes, I was worried about the leaks that would be destroyed
all at once or in an undefined order.
I have learned about the devm_kalloc() family of functions and
how it is not used here.
Thank you for considering the patchset at such short notice
and of course I see more experienced developer's ideas dismissed,
so I do not take it with disappointment.
I was already aware that there is currently no mechanism to call
this .release() callback or hook, how should we call it.
Thank you very much for your review.
As I said, I was worried that it could cause problems, in particular
with vTMPs I've read about, randomness for security, nonces that
may not repeat even on virtual server rollbacks or restores to
bare metal and other RFC 4086 considerations which are not even
completely clear to me, as I do not consider myself an expert
in either field.
I am a believer of proper cleanups, and this will not change, but
you have greater experience and knowledge and might find such
mechanism impractical, which I respect with Vulcan aspired logic
and respect :-)
This patch attempt gave me a great brainstorming on how the Linux
drivers work, and I am content with that result. Code of Conduct
warned that newbie kernel developers get ideas on how things should
be done that are not accepted.
Thank you once again, and really have a nice and blessed weekend!
Best regards,
Mirsad
> --
> paul-moore.com
--
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia
The European Union
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
> <[email protected]> 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?
--
With Best Regards,
Andy Shevchenko
On Mon, Mar 13, 2023 at 5:33 AM Andy Shevchenko
<[email protected]> 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
> > <[email protected]> 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
On Mon, Mar 13, 2023 at 04:27:42PM -0400, Paul Moore wrote:
> On Mon, Mar 13, 2023 at 5:33 AM Andy Shevchenko
> <[email protected]> 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
> > > <[email protected]> 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
On Tue, Mar 14, 2023 at 01:05:25PM +0200, Andy Shevchenko 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
> > <[email protected]> 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
> > > > <[email protected]> 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;
Please do not do anything just to "shut up" the kobject code, that
warning is in there on purpose, fix the problem properly and do not try
to work around it as that is totally wrong.
greg k-h
On Tue, Mar 14, 2023 at 7:05 AM Andy Shevchenko
<[email protected]> 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
> > <[email protected]> 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
> > > > <[email protected]> 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