| Message ID | 20190531231020.628-23-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:44PM -0700, Casey Schaufler wrote: > Remove lsm_export scaffolding around audit_sig_sid by > changing the u32 secid into an lsm_export structure named > audit_sig_lsm. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > include/linux/security.h | 7 +++++++ > kernel/audit.c | 18 ++++++++---------- > kernel/audit.h | 2 +- > kernel/auditsc.c | 3 +-- > 4 files changed, 17 insertions(+), 13 deletions(-) > > diff --git a/include/linux/security.h b/include/linux/security.h > index 40aa7b9f3c83..e76d7a9dbe50 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -93,6 +93,13 @@ static inline void lsm_export_init(struct lsm_export *l) > memset(l, 0, sizeof(*l)); > } > > +static inline bool lsm_export_any(struct lsm_export *l) > +{ > + return (((l->flags & LSM_EXPORT_SELINUX) && l->selinux) || > + ((l->flags & LSM_EXPORT_SMACK) && l->smack) || > + ((l->flags & LSM_EXPORT_APPARMOR) && l->apparmor)); > +} All of these helpers need kerndoc. Bikeshed on naming: - struct lsm_export renamed to lsm_secid - lsm_export_any renamed to lsm_secid_defined() or ..._is_set() or ..._non_zero() ?
On 6/1/2019 7:03 PM, Kees Cook wrote: > On Fri, May 31, 2019 at 04:09:44PM -0700, Casey Schaufler wrote: >> Remove lsm_export scaffolding around audit_sig_sid by >> changing the u32 secid into an lsm_export structure named >> audit_sig_lsm. >> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> --- >> include/linux/security.h | 7 +++++++ >> kernel/audit.c | 18 ++++++++---------- >> kernel/audit.h | 2 +- >> kernel/auditsc.c | 3 +-- >> 4 files changed, 17 insertions(+), 13 deletions(-) >> >> diff --git a/include/linux/security.h b/include/linux/security.h >> index 40aa7b9f3c83..e76d7a9dbe50 100644 >> --- a/include/linux/security.h >> +++ b/include/linux/security.h >> @@ -93,6 +93,13 @@ static inline void lsm_export_init(struct lsm_export *l) >> memset(l, 0, sizeof(*l)); >> } >> >> +static inline bool lsm_export_any(struct lsm_export *l) >> +{ >> + return (((l->flags & LSM_EXPORT_SELINUX) && l->selinux) || >> + ((l->flags & LSM_EXPORT_SMACK) && l->smack) || >> + ((l->flags & LSM_EXPORT_APPARMOR) && l->apparmor)); >> +} > All of these helpers need kerndoc. Point. > Bikeshed on naming: > - struct lsm_export renamed to lsm_secid I want to get away from the expectation that what an LSM exports has to be a u32 secid. It's not in any patchset yet, but I plan to replace the Smack u32 with a struct smack_known * at some point in the future. That will require a little work in the secmark code, but will have significant performance improvement in audit and UDS. > - lsm_export_any renamed to lsm_secid_defined() or ..._is_set() or > ..._non_zero() ? I'll admit lsm_export_any() isn't a great name. The state it has to convey is "some LSM has set a value, and it isn't an error value." Like "secid != 0", except that it matters whether the 0 came from secid having never been set, as opposed to it was set because something went wrong. At the same time, I don't want it to imply that the value is set for all LSMs, because it may not be. That's why I used "any". Some LSM *has* set a value. That value may not be the one you're hoping for, but you may need to call the subsystem (e.g.audit) that's going to look. Maybe lsm_export_is_interesting()? I'd love to discover there's a convention I could adhere to.
On Mon, Jun 03, 2019 at 03:23:07PM -0700, Casey Schaufler wrote: > Maybe lsm_export_is_interesting()? > I'd love to discover there's a convention I could adhere to. I'd agree "lsm_data" seems meaningless. lsm_export does seem a better name, though it has the "export is also a verb" issue. Would "lsm_context" or "lsm_ctx"? be better? then we get lsm_ctx_is_interesting() and lsm_ctx_to_secid() ?
On 6/6/2019 11:41 AM, Kees Cook wrote: > On Mon, Jun 03, 2019 at 03:23:07PM -0700, Casey Schaufler wrote: >> Maybe lsm_export_is_interesting()? >> I'd love to discover there's a convention I could adhere to. > I'd agree "lsm_data" seems meaningless. lsm_export does seem a better > name, though it has the "export is also a verb" issue. Would "lsm_context" > or "lsm_ctx"? > be better? > > then we get lsm_ctx_is_interesting() and lsm_ctx_to_secid() ? Fiddling around with this led me to think "struct lsmdata" would work, although maybe "struct lsmblob", in keeping with the notion it is opaque. Leaving out the "_" helps with the verb issue, I think. I think ctx or context is right out, as secctx is the string representation, and it would really confuse things.
On Thu, Jun 06, 2019 at 12:17:42PM -0700, Casey Schaufler wrote: > On 6/6/2019 11:41 AM, Kees Cook wrote: > > On Mon, Jun 03, 2019 at 03:23:07PM -0700, Casey Schaufler wrote: > >> Maybe lsm_export_is_interesting()? > >> I'd love to discover there's a convention I could adhere to. > > I'd agree "lsm_data" seems meaningless. lsm_export does seem a better > > name, though it has the "export is also a verb" issue. Would "lsm_context" > > or "lsm_ctx"? > > be better? > > > > then we get lsm_ctx_is_interesting() and lsm_ctx_to_secid() ? > > Fiddling around with this led me to think "struct lsmdata" > would work, although maybe "struct lsmblob", in keeping with > the notion it is opaque. Leaving out the "_" helps with the > verb issue, I think. I think ctx or context is right out, as > secctx is the string representation, and it would really confuse > things. Ah yeah, good point on "context". Does "blob" conflict with the existing "blob" stuff? If it's always going to be u32 data, do we want it to be lsm_u32 ? Or, since it's a multiplexor, lsmmux ?
On 6/6/2019 1:53 PM, Kees Cook wrote: > On Thu, Jun 06, 2019 at 12:17:42PM -0700, Casey Schaufler wrote: >> On 6/6/2019 11:41 AM, Kees Cook wrote: >>> On Mon, Jun 03, 2019 at 03:23:07PM -0700, Casey Schaufler wrote: >>>> Maybe lsm_export_is_interesting()? >>>> I'd love to discover there's a convention I could adhere to. >>> I'd agree "lsm_data" seems meaningless. lsm_export does seem a better >>> name, though it has the "export is also a verb" issue. Would "lsm_context" >>> or "lsm_ctx"? >>> be better? >>> >>> then we get lsm_ctx_is_interesting() and lsm_ctx_to_secid() ? >> Fiddling around with this led me to think "struct lsmdata" >> would work, although maybe "struct lsmblob", in keeping with >> the notion it is opaque. Leaving out the "_" helps with the >> verb issue, I think. I think ctx or context is right out, as >> secctx is the string representation, and it would really confuse >> things. > Ah yeah, good point on "context". Does "blob" conflict with the existing > "blob" stuff? I don't think so. Some people might think it a bit too cute, but I kind of like it. > If it's always going to be u32 data, do we want it to be > lsm_u32 ? At some point I would love to have the Smack data be a struct smack_known pointer, but that's a future thing. > Or, since it's a multiplexor, lsmmux ? I'd rather describe what's in it than how it's used.
On Thu, Jun 06, 2019 at 02:06:44PM -0700, Casey Schaufler wrote:
> I'd rather describe what's in it than how it's used.
Yeah, good point. :)
diff --git a/include/linux/security.h b/include/linux/security.h index 40aa7b9f3c83..e76d7a9dbe50 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -93,6 +93,13 @@ static inline void lsm_export_init(struct lsm_export *l) memset(l, 0, sizeof(*l)); } +static inline bool lsm_export_any(struct lsm_export *l) +{ + return (((l->flags & LSM_EXPORT_SELINUX) && l->selinux) || + ((l->flags & LSM_EXPORT_SMACK) && l->smack) || + ((l->flags & LSM_EXPORT_APPARMOR) && l->apparmor)); +} + /** * lsm_export_secid - pull the useful secid out of a lsm_export * @data: the containing data structure diff --git a/kernel/audit.c b/kernel/audit.c index fa4c5544eb37..5226e2af9498 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -135,9 +135,9 @@ static u32 audit_backlog_limit = 64; static u32 audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME; /* The identity of the user shutting down the audit system. */ -kuid_t audit_sig_uid = INVALID_UID; -pid_t audit_sig_pid = -1; -u32 audit_sig_sid = 0; +kuid_t audit_sig_uid = INVALID_UID; +pid_t audit_sig_pid = -1; +struct lsm_export audit_sig_lsm; /* Records can be lost in several ways: 0) [suppressed in audit_alloc] @@ -1429,23 +1429,21 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) } case AUDIT_SIGNAL_INFO: len = 0; - if (audit_sig_sid) { - struct lsm_export le; - - lsm_export_to_all(&le, audit_sig_sid); - err = security_secid_to_secctx(&le, &ctx, &len); + if (lsm_export_any(&audit_sig_lsm)) { + err = security_secid_to_secctx(&audit_sig_lsm, &ctx, + &len); if (err) return err; } sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL); if (!sig_data) { - if (audit_sig_sid) + if (lsm_export_any(&audit_sig_lsm)) security_release_secctx(ctx, len); return -ENOMEM; } sig_data->uid = from_kuid(&init_user_ns, audit_sig_uid); sig_data->pid = audit_sig_pid; - if (audit_sig_sid) { + if (lsm_export_any(&audit_sig_lsm)) { memcpy(sig_data->ctx, ctx, len); security_release_secctx(ctx, len); } diff --git a/kernel/audit.h b/kernel/audit.h index 958d5b8fc1b3..64498850c52b 100644 --- a/kernel/audit.h +++ b/kernel/audit.h @@ -338,7 +338,7 @@ extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len); extern pid_t audit_sig_pid; extern kuid_t audit_sig_uid; -extern u32 audit_sig_sid; +extern struct lsm_export audit_sig_lsm; extern int audit_filter(int msgtype, unsigned int listtype); diff --git a/kernel/auditsc.c b/kernel/auditsc.c index 71daead619e5..41f540037a93 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -2398,8 +2398,7 @@ int audit_signal_info(int sig, struct task_struct *t) audit_sig_uid = auid; else audit_sig_uid = uid; - security_task_getsecid(current, &le); - lsm_export_secid(&le, &audit_sig_sid); + security_task_getsecid(current, &audit_sig_lsm); } if (!audit_signals || audit_dummy_context())
Remove lsm_export scaffolding around audit_sig_sid by changing the u32 secid into an lsm_export structure named audit_sig_lsm. Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> --- include/linux/security.h | 7 +++++++ kernel/audit.c | 18 ++++++++---------- kernel/audit.h | 2 +- kernel/auditsc.c | 3 +-- 4 files changed, 17 insertions(+), 13 deletions(-)