On 6/1/2019 6:57 PM, Kees Cook wrote:

On Fri, May 31, 2019 at 04:09:27PM -0700, Casey Schaufler wrote:

quoted

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.

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.

Also, I find the helper naming to be not easy to follow but this is
mainly due to me repeatedly trying to parse the helpers as
noun-verb-noun, so lsm_export_secid() kind of makes sense ("write to
secid, based on the flags") but then I see smack_export_secid() and this
is "write to the internal lsm_export storage, the value of this secid".
The direction here is what ends up confusing me. Which direction is the
data moving?

lsm_export_to_secid() <- from lsm_export to secid
smack_secid_to_lsm_export() <- from secid to smack's lsm_export record

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.

quoted

+static inline void selinux_export_secid(struct lsm_export *l, u32 secid)
+{
+	l->selinux = secid;
+	l->flags |= LSM_EXPORT_SELINUX;
+}

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.

Anyway, I'll keep reading maybe I just haven't gotten far enough, but
I'd love some help in the 0/NN cover letter describing what the
evolution actually does through the series so I can follow along and you
can set my expectations about what I'll be looking for in each patch.

OK. Good feedback. The next batch will be better.