| Message ID | 20190531231020.628-6-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | LSM: Module stacking for AppArmor | expand |
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. Instead, various helper functions are scattered through these patches and I'm left struggling to figure out where things are actually changing. 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 > +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? 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.
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.
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.
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 3fe39abccc8f..09573c55e535 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -425,7 +425,7 @@ * @inode_getsecid: * Get the secid associated with the node. * @inode contains a pointer to the inode. - * @secid contains a pointer to the location where result will be saved. + * @data contains a pointer to the location where result will be saved. * In case of failure, @secid will be set to zero. * @inode_copy_up: * A file is about to be copied up from lower layer to upper layer of @@ -1566,7 +1566,7 @@ union security_list_options { int flags); int (*inode_listsecurity)(struct inode *inode, char *buffer, size_t buffer_size); - void (*inode_getsecid)(struct inode *inode, u32 *secid); + void (*inode_getsecid)(struct inode *inode, struct lsm_export *data); int (*inode_copy_up)(struct dentry *src, struct cred **new); int (*inode_copy_up_xattr)(const char *name); diff --git a/include/linux/security.h b/include/linux/security.h index 81f9f79f9a1e..fb19f41d630b 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -88,6 +88,11 @@ struct lsm_export { #define LSM_EXPORT_SMACK 0x02 #define LSM_EXPORT_APPARMOR 0x04 +static inline void lsm_export_init(struct lsm_export *l) +{ + memset(l, 0, sizeof(*l)); +} + /* These functions are in security/commoncap.c */ extern int cap_capable(const struct cred *cred, struct user_namespace *ns, int cap, unsigned int opts); diff --git a/security/security.c b/security/security.c index d05f00a40e82..a1f28a5e582b 100644 --- a/security/security.c +++ b/security/security.c @@ -712,6 +712,36 @@ int lsm_superblock_alloc(struct super_block *sb) RC; \ }) +/** + * lsm_export_secid - pull the useful secid out of a lsm_export + * @data: the containing data structure + * @secid: where to put the one that matters. + * + * Shim that will disappear when all lsm_export conversions are done. + */ +static inline void lsm_export_secid(struct lsm_export *data, u32 *secid) +{ + switch (data->flags) { + case LSM_EXPORT_NONE: + *secid = 0; + break; + case LSM_EXPORT_SELINUX: + *secid = data->selinux; + break; + case LSM_EXPORT_SMACK: + *secid = data->smack; + break; + case LSM_EXPORT_APPARMOR: + *secid = data->apparmor; + break; + default: + pr_warn("%s flags=0x%u - not a valid set\n", __func__, + data->flags); + *secid = 0; + break; + } +} + /* Security operations */ int security_binder_set_context_mgr(struct task_struct *mgr) @@ -1389,7 +1419,10 @@ EXPORT_SYMBOL(security_inode_listsecurity); void security_inode_getsecid(struct inode *inode, u32 *secid) { - call_void_hook(inode_getsecid, inode, secid); + struct lsm_export data = { .flags = LSM_EXPORT_NONE }; + + call_void_hook(inode_getsecid, inode, &data); + lsm_export_secid(&data, secid); } int security_inode_copy_up(struct dentry *src, struct cred **new) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index ee840fecfebb..0e31be22d9bb 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -213,6 +213,15 @@ static void cred_init_security(void) tsec->osid = tsec->sid = SECINITSID_KERNEL; } +/* + * Set the SELinux secid in an lsm_export structure + */ +static inline void selinux_export_secid(struct lsm_export *l, u32 secid) +{ + l->selinux = secid; + l->flags |= LSM_EXPORT_SELINUX; +} + /* * get the security ID of a set of credentials */ @@ -3316,15 +3325,16 @@ static int selinux_inode_listsecurity(struct inode *inode, char *buffer, size_t return len; } -static void selinux_inode_getsecid(struct inode *inode, u32 *secid) +static void selinux_inode_getsecid(struct inode *inode, struct lsm_export *l) { struct inode_security_struct *isec = inode_security_novalidate(inode); - *secid = isec->sid; + + selinux_export_secid(l, isec->sid); } static int selinux_inode_copy_up(struct dentry *src, struct cred **new) { - u32 sid; + struct lsm_export l; struct task_security_struct *tsec; struct cred *new_creds = *new; @@ -3336,8 +3346,9 @@ static int selinux_inode_copy_up(struct dentry *src, struct cred **new) tsec = selinux_cred(new_creds); /* Get label from overlay inode and set it in create_sid */ - selinux_inode_getsecid(d_inode(src), &sid); - tsec->create_sid = sid; + lsm_export_init(&l); + selinux_inode_getsecid(d_inode(src), &l); + tsec->create_sid = l.selinux; *new = new_creds; return 0; } diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index e9560b078efe..5e345122ccb1 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -466,6 +466,15 @@ static int smk_ptrace_rule_check(struct task_struct *tracer, return rc; } +/* + * Set the Smack secid in an lsm_export structure + */ +static inline void smack_export_secid(struct lsm_export *l, u32 secid) +{ + l->smack = secid; + l->flags |= LSM_EXPORT_SMACK; +} + /* * LSM hooks. * We he, that is fun! @@ -1481,11 +1490,11 @@ static int smack_inode_listsecurity(struct inode *inode, char *buffer, * @inode: inode to extract the info from * @secid: where result will be saved */ -static void smack_inode_getsecid(struct inode *inode, u32 *secid) +static void smack_inode_getsecid(struct inode *inode, struct lsm_export *l) { struct smack_known *skp = smk_of_inode(inode); - *secid = skp->smk_secid; + smack_export_secid(l, skp->smk_secid); } /*
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. Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> --- include/linux/lsm_hooks.h | 4 ++-- include/linux/security.h | 5 +++++ security/security.c | 35 ++++++++++++++++++++++++++++++++++- security/selinux/hooks.c | 21 ++++++++++++++++----- security/smack/smack_lsm.c | 13 +++++++++++-- 5 files changed, 68 insertions(+), 10 deletions(-)