[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Casey Schaufler]

On 4/29/2018 4:49 AM, Tetsuo Handa wrote:
> Casey Schaufler wrote:
>> I think that we're on the verge of having a major merge collision.
> I think that the reason of major merge collision is that your patchset is
> too big to review.

My patch set is large because it has to address all of the
aspects of multiple modules running on the system before some
important members of the community will look at it seriously.

>
>   > That's a lot of overhead. The smaller the patches, the more work
>   > required to make the patch set. You're right that the current patches
>   > are too big. Moving forward I will be providing smaller, easier to
>   > deal with patches. The sheer overhead of rebaseing a large patch set
>   > has slowed down the development considerably. Minions. What I need are
>   > minions.
>
> You are holding the global lock for patchset which you did not get response.
> If you agree with breaking into smaller patches, we will be able to solve
> the major merge collision.

I will be presenting the set of smaller patches once I finish merging
them into 4.18. It's a bigger jobs than usual because of the change from
lists to hlists, the significant changes to AppArmor and the early stages
of SELinux namespacing. I hope to have it done mid week, but the changes
need to get tested, and there are lots of configurations involved.

I am perfectly amenable to the patches going in in logical stages.
In fact, that is what I would recommend.

>> I hope to have the multiple major module code seriously reviewed as of
>> 4.18 and start putting real pressure on getting it in for 4.19/4.20.
> Before you repost your patchset, please answer questions below.
>
> Q1: Apart from whether memory for security blobs should be managed by
>     infrastructure or by individual module, how do you guarantee that
>     only release hook where corresponding alloc hook succeeded when
>     one of alloc hooks returned an error?

The infrastructure allocates a zeroed blob. Module "alloc" hooks
are called. The module specific hooks set their data as desired.
If a module alloc hook fails the complete set of free hooks can
be called because the module data will either have been set or
will be still be zeroed. This means that module free hooks have
to accommodate the possibility the the module specific data was
never set.

The reality is that many of the free hooks go away with the
infrastructure blob management. Those that remain can easily
be confirmed to be safe in the case where all the modules data
is NULL.
 

> My patch 1/2 provides that mechanism. Please show your patch which
> implements only that functionality instead of whole patchset.

It's not an explicit change. It is an artifact of the infrastructure
allocating and freeing the data. You can't tease the removal of the
module free hooks apart from the infrastructure freeing the data.

>   > It is my firm hope that the multiple major module changes are
>   > going to start being seriously considered in the next release or
>   > two. That would reduce the complexity of what you're trying to
>   > accomplish because at the point all modules will be equal. I have
>   > always committed to making design choices that aren't going to
>   > prevent loadable/unloadable modules. I have also expressed no
>   > interest in doing it myself. From my selfish perspective I would
>   > like to see module loading follow my work, as having yet another
>   > major merge effort will delay the clean-up I know I'll have to do
>   > after 4.18.
>
> Depending on your patch, it is possible that managing memory for security
> blobs by infrastructure can become harmful for LKM-based LSMs, for the
> amount of memory which will be needed by LKM-based LSMs is not known
> until LKM-based LSMs are loaded.

It's certainly not more "harmful" than the single blob mechanism we
have now. The patches I'm proposing don't make the situation any worse
than it is today. As I've maintained all along, I'm not trying to
address LKM-based modules.

There is no reason I can see that blob sizes couldn't change while
a system is running (yes, I have looked into it) provided that the
blob size and content list is recorded in the blob and that the dynamic
modules check this information before trying to use it.

	smack_inode(struct inode *inode)
	{
		return inode->i_security + smack_blob_sizes.lbs_inode;
	}

	lkm_based_module_inode(struct inode *inode)
	{
		struct lsm_inode_blob_header *ibh = inode->security;
		if (some_appropriate_check(ibh, LKM_BASED_MODULE_ID))
			return inode->i_security + lkm_based_module_blob_sizes.lbs_inode;
		/* No blob for this object */
		return NULL;
	}

A refinement to this might keep the offset for dynamic module data
in the blob header. That would be more flexible, but would consume
more memory for each blob.

> LKM-based LSMs might be forced to manage their security blobs without
> using infrastructure-managed ->security field. Then, that will not make
> "all modules equal".

Regardless of how the data is managed the information about how much
is being used and where to find it has to be kept somewhere. With
static modules it is easier because you only need to figure that out
once, at the beginning. If you're going to have blob data changing
you have to look it up or figure it out at every reference. It's
not impossible. It's not even really that hard, but it won't be
cheap.

> So, please show your approach which allows LKM-based LSMs to handle
> infrastructure-managed ->security field (if you want to use
> infrastructure-managed ->security field).

I'll repeat, I don't have code to do it because I have explicitly
excluded lkm-based modules from my todo list. If I were doing it,
I would propose an infrastructure managed header in the security
blobs containing sufficient information for the modules to determine
if the data they are looking for is present. lkm-based modules would
have to be coded to accept that they might get blobs that were created
before the module was loaded, and that do not contain data for that
module.

The blob header mechanism would only be included if lkm-based security
module support was configured.

>> The advent of the Age of Containers is driving this.
> I've been asking for runtime loading of LSMs before the Age of
> Containers comes. ;-)

Yes, you have!

> Q2: What functionality are Containers people want LSM?

It varies. Container people are a diverse lot.

>     Use different LSM module for different container?

That is definitely one use case. You might run Smack on the base
system and label each container based on who payed for it, and
let the security module set within the container be whatever the
job demands.

>     Runtime load/unload of LSM modules?

I would see that as something you might do on container
startup, but managing the module set could get ugly.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html