[PATCH 05/58] LSM: Use lsm_export in the inode_getsecid hooks

[Kees Cook]

On Mon, Jun 03, 2019 at 05:29:45PM -0700, Casey Schaufler wrote:
> On 6/1/2019 6:57 PM, Kees Cook wrote:
> > On Fri, May 31, 2019 at 04:09:27PM -0700, Casey Schaufler wrote:
> >> Convert the inode_getsecid hooks to use the lsm_export
> >> structure instead of a u32 secid. There is some scaffolding
> >> involved that will be removed when security_inode_getsecid()
> >> is updated.
> > So, there are like 20 patches that all have basically identical subject
> > and changelog, but some evolve the API in subtle ways. For example,
> > in this patch, there is no mention of adding lsm_export_init(). I would
> > expect all the lsm_export infrastructure and support functions to be
> > introduced in patch 4 where struct lsm_export is initially introduced.
> 
> Fair enough. I didn't introduce helpers until they were used.
> I hoped to avoid the "what is this for?" question that can
> come up when you add functions that aren't used.

True, but since we know a giant set of changes is coming, I think it's
okay. As long there's kerndoc on the helpers, it should be clear what
they're for. And the commit log can include the context for why the
helpers exist. "In later patches, we'll replace secids with lsm_context,
so we need to use foo to do bar etc"

> > Instead, various helper functions are scattered through these patches
> > and I'm left struggling to figure out where things are actually
> > changing.
> 
> I think it's possible that the patches may be too small
> to contain enough context for them to be sensible. It may
> make things more obvious if I combined
> 
> [PATCH 05/58] LSM: Use lsm_export in the inode_getsecid hooks
> [PATCH 20/58] LSM: Use lsm_export in security_inode_getsecid
> 
> into a single patch. That would reduce the amount of scaffolding
> that has to get set up and torn down.

Yeah, that's fine. If you have to do a lot of work to split up a pair of
patches, I think that's fine to combine them. What I usually want to see
is a split of separable changes. Like, adding all the helpers: I can
look at those individually as I read the patch. Then the next patch
might swap a whole logical set of things like putting lsm_context into
the LSMs, but leaving all the interfaces alone. Then fixing the high
level things that use secids, etc.

But, really, the cover letter should cover the evolutionary steps the
series takes: that should serve as a guide for everything trying to
follow your thinking.

> The inconsistency is comes from my use of "lsm_export" for
> the name of the LSM data structure. This is something you've
> commented on elsewhere. The underscore makes the function name
> look like it has an lsm_ prefix, rather than just being the
> name of the structure. If I changed "struct lsm_export" to
> "struct lsmdata" the names:
> 
> lsm_lsmdata_to_secid() and smack_secid_to_lsmdata()
> would be more consistent.

Right. Having a distinct verb in the helper name should solve all my
confusion. :)

lsm_context_to_secid() secid_to_lsm_context() smack_secid_to_lsm_export()
etc

> > Which brings me to another thing I find awkward here: I feel like an LSM
> > shouldn't need to do anything with this object: it should be opaque to
> > the LSM. The LSM infrastructure knows which LSM it has called into. Why
> > isn't this just like the other blobs?
> 
> There's a lot more rework required if the lsm_export has to be
> life-cycle managed. The audit code, for example, passes them about,
> copying, storing and dropping them without a care. I'm not completely
> opposed to taking that on, but it's essentially a rewrite of the
> audit LSM handling. The SO_PEERSEC handling probably has issues as
> well. I think netlabel would be OK, but there's stuff going on elsewhere
> in the networking stack that isn't going to like anything it has to
> worry about allocating and/or freeing.

I didn't mean life-cycle managed, but rather "opaque" to LSM. I just now
tried to construct an example, and have decided it's too crazy. :) The
benefits of your current system are that they are trivially able to be
put on the stack since they're a fixed size. The down side is that each
LSM must manage its own flags, etc. I will ponder alternatives after I
see the next version of your series.

-- 
Kees Cook