Re: [PATCH v38 04/39] LSM: Maintain a table of LSM attribute data

[Casey Schaufler]

On 10/13/2022 3:04 AM, Tetsuo Handa wrote:
> On 2022/09/28 4:53, Casey Schaufler wrote:
>> @@ -483,6 +491,16 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>>  {
>>  	int i;
>>  
>> +	/*
>> +	 * A security module may call security_add_hooks() more
>> +	 * than once. Landlock is one such case.
>> +	 */
>> +	if (lsm_id == 0 || lsm_idlist[lsm_id - 1] != lsmid)
>> +		lsm_idlist[lsm_id++] = lsmid;
>> +
>> +	if (lsm_id > LSMID_ENTRIES)
>> +		panic("%s Too many LSMs registered.\n", __func__);
> I'm not happy with LSMID_ENTRIES. This is a way towards forever forbidding LKM-based LSMs.

I don't see any way given the locking issues that we're ever going to
mix built in security modules and loaded security modules on the same
hook lists. The SELinux module deletion code is sufficiently scary that
it is being removed. That does not mean that I think loadable modules
are impossible, I think it means that their management is going to have
to be separate, the same way the BPF programs are handled. The only way
that I see a unified hook list is for all the LSMs to be implemented as
loadable modules, and I can't see that happening in my lifetime.

I can see an LSM like BPF, as I mentioned before, that manages loaded
modules. Over the years I've seen several designs that might work. I'm
encouraged (and not a little bit frightened) by the success of the BPF
work.

Converting the array[LSMID_ENTRIES] implementation to a hlist like the
hooks have used would not be that big a project and I don't see that
making such a change would be a show-stopper for implementing loadable
modules. I think that a lot of other issues would be more significant.

I will, on the other hand, listen to compelling arguments. It is not the
intention of this code to lock out loadable modules. If I thought it would
I would not have proposed it.

> I'm fine with using UAPI-visible constants for switching /proc/ files.
> But TOMOYO does not need such constant because TOMOYO does not use /proc/ files.
>
> Also, lsm_self_attr() will be limited for LSM modules which use /proc/ files, and
> therefore I think prctl() will be already there.

While the proposed set of attributes map to those in /proc/.../attr there is
no reason to assume they will be limited to those. I can see providing several
of the Smack attributes currently manipulated in smackfs, such as relabel-self.
If we are providing SELinux specific values like keycreate there's no reason
we can't provide Smack or TOMOYO specific values as well.

>
>> +
>>  	for (i = 0; i < count; i++) {
>>  		hooks[i].lsmid = lsmid;
>>  		hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);