Re: [RFC PATCH 08/29] lsm: get rid of the lsm_names list and do some cleanup

[John Johansen <john.johansen@canonical.com>]

On 4/10/25 15:47, Paul Moore wrote:
> On Wed, Apr 9, 2025 at 7:13 PM Kees Cook <kees@kernel.org> wrote:
>>
>> On Wed, Apr 09, 2025 at 02:49:53PM -0400, Paul Moore wrote:
>>> The LSM currently has a lot of code to maintain a list of the
>>> currently active LSMs in a human readable string, with the only
>>> user being the "/sys/kernel/security/lsm" code.  Let's drop all
>>> of that code and generate the string on an as-needed basis when
>>> userspace reads "/sys/kernel/security/lsm".
>>>
>>> Signed-off-by: Paul Moore <paul@paul-moore.com>
>>> ---
>>>   include/linux/lsm_hooks.h |  1 -
>>>   security/inode.c          | 27 +++++++++++++++++++--
>>>   security/lsm_init.c       | 49 ---------------------------------------
>>>   3 files changed, 25 insertions(+), 52 deletions(-)
> 
> ...
> 
>>> @@ -343,8 +345,29 @@ static struct dentry *lsm_dentry;
>>>   static ssize_t lsm_read(struct file *filp, char __user *buf, size_t count,
>>>                        loff_t *ppos)
>>>   {
>>> -     return simple_read_from_buffer(buf, count, ppos, lsm_names,
>>> -             strlen(lsm_names));
>>> +     int i;
>>> +     char *str;
>>> +     ssize_t rc, len = 0;
>>> +
>>> +     for (i = 0; i < lsm_count; i++)
>>> +             /* the '+ 1' accounts for either a comma or a NUL terminator */
>>> +             len += strlen(lsm_order[i]->id->name) + 1;
>>> +
>>> +     str = kmalloc(len, GFP_KERNEL);
>>> +     if (!str)
>>> +             return -ENOMEM;
>>> +     str[0] = '\0';
>>> +
>>> +     i = 0;
>>> +     while (i < lsm_count) {
>>> +             strcat(str, lsm_order[i]->id->name);
>>> +             if (++i < lsm_count)
>>> +                     strcat(str, ",");
>>> +     }
>>> +
>>> +     rc = simple_read_from_buffer(buf, count, ppos, str, len);
>>> +     kfree(str);
>>> +     return rc;
>>
>> Hrm, at least cache it?
> 
> Are you aware of a performance critical use of this?
> 
no I can't see anything performance critical, I think it just is cleaner
to only generate once if after init the list doesn't change.

>> Better yet, do this whole thing in a initcall after LSMs are loaded, and
>> both can gain __ro_after_init...
> 
> I *really* disliked all the stuff we were having to do during boot,
> and all the redundant global state we were keeping around.  I'll go
> ahead and cache the lsm_read() result local to the function but that's
> probably all I'm going to accept at this point in time.
> 
fair, I don't even think this needs to be changed, I think kees's suggestion
is more of a nice to have