On 11. 03. 2023. 15:59, Paul Moore wrote: > On Fri, Mar 10, 2023 at 5:53 PM Mirsad Goran Todorovac > <mirsad.todorovac@xxxxxxxxxxxx> wrote: >> On 10. 03. 2023. 23:33, Paul Moore wrote: >>> On Fri, Mar 10, 2023 at 2:26 PM Mirsad Goran Todorovac >>> <mirsad.goran.todorovac@xxxxxx> 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
