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

[Casey Schaufler]

On 10/23/2022 12:27 AM, Tetsuo Handa wrote:
> On 2022/10/21 8:42, Casey Schaufler wrote:
>> 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'm not expecting for unloadable LSM modules.
> I'm expecting for loadable LSM modules.
>
> I'm not expecting to make all LSM modules to be implemented as loadable
> LSM modules, for some want to associate "security label" to everything
> (including processes which might start before the global init process starts)
> but others do not need to associate "security label" to everything.
>
>> 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.
> There can be LSM modules whose lifetime of hooks match the lifetime of
> a process which registered hooks for that process. In that case, being
> automatically unregistered upon process termination would be preferable.
>
> But there are LSM modules whose lifetime of hooks is irrelevant to a process
> which registered a hook for that process. In that case, we need a method for
> allowing registered hooks to remain even after that process terminated.
>
> Please don't think loadable LSM modules as something that require special
> handling. TOMOYO is an LSM module whose lifetime of hooks is irrelevant to
> a process which registered a hook for that process, but does not need to
> associate "security label" to everything. It has to be trivial to convert
> TOMOYO as a loadable LSM module.

I don't see that having a built-in version of TOMOYO and a loadable version
needs to be difficult. That's something that whoever creates the loadable
security module scheme is going to have to address. It will depend on the
details of the loadable module mechanism. I can't comment on how that will
work because I don't see loadable modules hitting the top of my queue.

>
>> 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.
> Defining constants for each LSM module (i.e. "LSM: Add an LSM identifier
> for external use") is the show-stopper for implementing loadable modules.

One possible way for loadable modules to work would be to have a built-in
module LSM_ID_MODLOADER which maintains its own list of module hooks.
The values returned from lsm_self_attr() would identify the this LSM
and the data value would have to identify the loaded module it refers to,
perhaps as "TOMOYO=XYZ" or "datastate=foobar". A flag LSM_ATTR_LOADED would
indicate that the attribute needed to be processed according to loadable
module attribute rules, whatever they might be.

So no, it's not a show stopper. Not any more than u32 secids are a showstopper
for process attributes it audit records. LSM IDs are inconvenient, and not my
first choice, but I'm not going to let that get in the way of getting this
code upstream.  

> We won't be able to accept whatever LSM modules to upstream, and we won't
> be able to enable whatever LSM modules in distributor kernels.

A built in module loader security module would address this issue.
Getting such a module accepted upstream is not going to be trivial,
but the BPF people seem to have managed it.

> LSM modules which cannot define a constant due to either "not accepted
> to upstream" or "not enabled by distributor kernels" will be forbidden.
> I expect that we assign a constant upon module registration (instead of
> API visible constants) if we require all LSM modules to have a constant.

Maybe the thing to do is rewrite TOMOYO in eBPF. If I wanted to have a
loadable security module I could either take ten years or so to get a
loadable module scheme upstream in addition to my module, or I could
write it in eBPF and use it the next day. I don't know enough about eBPF
programming to say if it has everything TOMOYO needs, but it sure looks
like an easier path if it does.

>> 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.
> This code is exactly for locking out loadable modules.

I hope that I have suggested viable (if not convenient) alternatives.
I suppose it is possible that locking out loadable modules is one
motivation behind the LSM ID scheme, but I really doubt it. And more
importantly, as I've outlined above, I can't be successful in locking
out loadable security modules. I don't even see it as an additional
complication.