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.
> 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.
>> +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.
