| Message ID | 20190618230551.7475-5-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | LSM: Module stacking for AppArmor | expand |
On Tue, Jun 18, 2019 at 04:05:30PM -0700, Casey Schaufler wrote: > When more than one security module is exporting data to > audit and networking sub-systems a single 32 bit integer > is no longer sufficient to represent the data. Add a > structure to be used instead. > > The lsmblob structure is currently an array of > u32 "secids". There is an entry for each of the > security modules built into the system that would > use secids if active. The system assigns the module > a "slot" when it registers hooks. If modules are > compiled in but not registered there will be unused > slots. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > include/linux/lsm_hooks.h | 1 + > include/linux/security.h | 62 +++++++++++++++++++++++++++++++++++++++ > security/security.c | 31 ++++++++++++++++++++ > 3 files changed, 94 insertions(+) > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 3fe39abccc8f..4d1ddf1a2aa6 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -2038,6 +2038,7 @@ struct security_hook_list { > struct hlist_head *head; > union security_list_options hook; > char *lsm; > + int slot; > } __randomize_layout; Hm, this feels redundant (as does the existing "char *lsm") now that we have lsm_info. The place for assigned-at-init value is blob_sizes, which hangs off of lsm_info (as does the LSM char *)... > > /* > diff --git a/include/linux/security.h b/include/linux/security.h > index 49f2685324b0..28d074866895 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -76,6 +76,68 @@ enum lsm_event { > LSM_POLICY_CHANGE, > }; > > +/* > + * Data exported by the security modules > + */ > +#define LSMDATA_ENTRIES ( \ LSMBLOB_ENTRIES? > + (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \ > + (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \ > + (IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0) ) > + > +struct lsmblob { > + u32 secid[LSMDATA_ENTRIES]; > +}; Cool; I like this auto-sizing. > + > +#define LSMDATA_INVALID -1 > + > +/** > + * lsmblob_init - initialize an lsmblob structure. > + * @l: Pointer to the data to initialize > + * @secid: The initial secid value > + * > + * Set all secid for all modules to the specified value. > + */ > +static inline void lsmblob_init(struct lsmblob *l, u32 secid) > +{ > + int i; > + > + for (i = 0; i < LSMDATA_ENTRIES; i++) > + l->secid[i] = secid; For all these LSMDATA_ENTRIES, I prefer ARRAY_SIZE(l->secid), but *shrug* > +} > + > +/** > + * lsmblob_is_set - report if there is an value in the lsmblob > + * @l: Pointer to the exported LSM data > + * > + * Returns true if there is a secid set, false otherwise > + */ > +static inline bool lsmblob_is_set(struct lsmblob *l) > +{ > + int i; > + > + for (i = 0; i < LSMDATA_ENTRIES; i++) > + if (l->secid[i] != 0) > + return true; > + return false; > +} > + > +/** > + * lsmblob_equal - report if the two lsmblob's are equal > + * @l: Pointer to one LSM data > + * @m: Pointer to the other LSM data > + * > + * Returns true if all entries in the two are equal, false otherwise > + */ > +static inline bool lsmblob_equal(struct lsmblob *l, struct lsmblob *m) > +{ > + int i; > + > + for (i = 0; i < LSMDATA_ENTRIES; i++) > + if (l->secid[i] != m->secid[i]) > + return false; > + return true; > +} > + > /* 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..5aa3c052d702 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -317,6 +317,7 @@ static void __init ordered_lsm_init(void) > init_debug("sock blob size = %d\n", blob_sizes.lbs_sock); > init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock); > init_debug("task blob size = %d\n", blob_sizes.lbs_task); > + init_debug("lsmblob size = %lu\n", sizeof(struct lsmblob)); > > /* > * Create any kmem_caches needed for blobs > @@ -420,6 +421,11 @@ static int lsm_append(char *new, char **result) > return 0; > } > > +/* > + * Current index to use while initializing the lsmblob secid list. > + */ > +static int lsm_slot __initdata; > + > /** > * security_add_hooks - Add a modules hooks to the hook lists. > * @hooks: the hooks to add > @@ -427,15 +433,40 @@ static int lsm_append(char *new, char **result) > * @lsm: the name of the security module > * > * Each LSM has to register its hooks with the infrastructure. > + * If the LSM is using hooks that export secids allocate a slot > + * for it in the lsmblob. > */ > void __init security_add_hooks(struct security_hook_list *hooks, int count, > char *lsm) > { > + int slot = LSMDATA_INVALID; > int i; > > for (i = 0; i < count; i++) { > hooks[i].lsm = lsm; > hlist_add_tail_rcu(&hooks[i].list, hooks[i].head); > + /* > + * If this is one of the hooks that uses a secid > + * note it so that a slot can in allocated for the > + * secid in the lsmblob structure. > + */ > + if (hooks[i].head == &security_hook_heads.audit_rule_match || > + hooks[i].head == &security_hook_heads.kernel_act_as || > + hooks[i].head == > + &security_hook_heads.socket_getpeersec_dgram || > + hooks[i].head == &security_hook_heads.secctx_to_secid || > + hooks[i].head == &security_hook_heads.secid_to_secctx || > + hooks[i].head == &security_hook_heads.ipc_getsecid || > + hooks[i].head == &security_hook_heads.task_getsecid || > + hooks[i].head == &security_hook_heads.inode_getsecid || > + hooks[i].head == &security_hook_heads.cred_getsecid) { > + if (slot == LSMDATA_INVALID) { > + slot = lsm_slot++; This needs to sanity check lsm_slot against lsmblob secids array size, just we we catch cases cleanly if an LSM adds a hook but doesn't add itself to the LSMDATA_ENTRIES macro. > + init_debug("%s assigned lsmblob slot %d\n", > + hooks[i].lsm, slot); > + } > + } > + hooks[i].slot = slot; > } > if (lsm_append(lsm, &lsm_names) < 0) > panic("%s - Cannot get early memory.\n", __func__); > -- > 2.20.1 >
On Tue, Jun 18, 2019 at 09:52:44PM -0700, Kees Cook wrote: > On Tue, Jun 18, 2019 at 04:05:30PM -0700, Casey Schaufler wrote: > > When more than one security module is exporting data to > > audit and networking sub-systems a single 32 bit integer > > is no longer sufficient to represent the data. Add a > > structure to be used instead. > > > > The lsmblob structure is currently an array of > > u32 "secids". There is an entry for each of the > > security modules built into the system that would > > use secids if active. The system assigns the module > > a "slot" when it registers hooks. If modules are > > compiled in but not registered there will be unused > > slots. > > > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > > --- > > include/linux/lsm_hooks.h | 1 + > > include/linux/security.h | 62 +++++++++++++++++++++++++++++++++++++++ > > security/security.c | 31 ++++++++++++++++++++ > > 3 files changed, 94 insertions(+) > > > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > > index 3fe39abccc8f..4d1ddf1a2aa6 100644 > > --- a/include/linux/lsm_hooks.h > > +++ b/include/linux/lsm_hooks.h > > @@ -2038,6 +2038,7 @@ struct security_hook_list { > > struct hlist_head *head; > > union security_list_options hook; > > char *lsm; > > + int slot; > > } __randomize_layout; > > Hm, this feels redundant (as does the existing "char *lsm") now that we > have lsm_info. The place for assigned-at-init value is blob_sizes, which > hangs off of lsm_info (as does the LSM char *)... Hm, nevermind. lsm_info is __initdata. I will ponder a way to refactor this in the future. For now, just leave slot in here with char *lsm.
On 6/18/2019 9:52 PM, Kees Cook wrote: > On Tue, Jun 18, 2019 at 04:05:30PM -0700, Casey Schaufler wrote: >> When more than one security module is exporting data to >> audit and networking sub-systems a single 32 bit integer >> is no longer sufficient to represent the data. Add a >> structure to be used instead. >> >> The lsmblob structure is currently an array of >> u32 "secids". There is an entry for each of the >> security modules built into the system that would >> use secids if active. The system assigns the module >> a "slot" when it registers hooks. If modules are >> compiled in but not registered there will be unused >> slots. >> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> --- >> include/linux/lsm_hooks.h | 1 + >> include/linux/security.h | 62 +++++++++++++++++++++++++++++++++++++++ >> security/security.c | 31 ++++++++++++++++++++ >> 3 files changed, 94 insertions(+) >> >> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >> index 3fe39abccc8f..4d1ddf1a2aa6 100644 >> --- a/include/linux/lsm_hooks.h >> +++ b/include/linux/lsm_hooks.h >> @@ -2038,6 +2038,7 @@ struct security_hook_list { >> struct hlist_head *head; >> union security_list_options hook; >> char *lsm; >> + int slot; >> } __randomize_layout; > Hm, this feels redundant (as does the existing "char *lsm") now that we > have lsm_info. The place for assigned-at-init value is blob_sizes, which > hangs off of lsm_info (as does the LSM char *)... Hm, is right. If the "char *lsm" pointer were replaced with a "struct lsm_info *lsm_info" pointer, and "int slot" added to lsm_info it would be a touch cleaner. A little bit of gimmickry might be required for initialization, but maybe not. It's worth looking into. >> >> /* >> diff --git a/include/linux/security.h b/include/linux/security.h >> index 49f2685324b0..28d074866895 100644 >> --- a/include/linux/security.h >> +++ b/include/linux/security.h >> @@ -76,6 +76,68 @@ enum lsm_event { >> LSM_POLICY_CHANGE, >> }; >> >> +/* >> + * Data exported by the security modules >> + */ >> +#define LSMDATA_ENTRIES ( \ > LSMBLOB_ENTRIES? That would be about 7% better. >> + (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \ >> + (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \ >> + (IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0) ) >> + >> +struct lsmblob { >> + u32 secid[LSMDATA_ENTRIES]; >> +}; > Cool; I like this auto-sizing. I figured this was either clever or hideous, but hadn't fully decided which. >> + >> +#define LSMDATA_INVALID -1 >> + >> +/** >> + * lsmblob_init - initialize an lsmblob structure. >> + * @l: Pointer to the data to initialize >> + * @secid: The initial secid value >> + * >> + * Set all secid for all modules to the specified value. >> + */ >> +static inline void lsmblob_init(struct lsmblob *l, u32 secid) >> +{ >> + int i; >> + >> + for (i = 0; i < LSMDATA_ENTRIES; i++) >> + l->secid[i] = secid; > For all these LSMDATA_ENTRIES, I prefer ARRAY_SIZE(l->secid), but > *shrug* I suppose, although you're adding compile time, and the definition of LSMDATA_ENTRIES is just above. >> +} >> + >> +/** >> + * lsmblob_is_set - report if there is an value in the lsmblob >> + * @l: Pointer to the exported LSM data >> + * >> + * Returns true if there is a secid set, false otherwise >> + */ >> +static inline bool lsmblob_is_set(struct lsmblob *l) >> +{ >> + int i; >> + >> + for (i = 0; i < LSMDATA_ENTRIES; i++) >> + if (l->secid[i] != 0) >> + return true; >> + return false; >> +} >> + >> +/** >> + * lsmblob_equal - report if the two lsmblob's are equal >> + * @l: Pointer to one LSM data >> + * @m: Pointer to the other LSM data >> + * >> + * Returns true if all entries in the two are equal, false otherwise >> + */ >> +static inline bool lsmblob_equal(struct lsmblob *l, struct lsmblob *m) >> +{ >> + int i; >> + >> + for (i = 0; i < LSMDATA_ENTRIES; i++) >> + if (l->secid[i] != m->secid[i]) >> + return false; >> + return true; >> +} >> + >> /* 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..5aa3c052d702 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -317,6 +317,7 @@ static void __init ordered_lsm_init(void) >> init_debug("sock blob size = %d\n", blob_sizes.lbs_sock); >> init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock); >> init_debug("task blob size = %d\n", blob_sizes.lbs_task); >> + init_debug("lsmblob size = %lu\n", sizeof(struct lsmblob)); >> >> /* >> * Create any kmem_caches needed for blobs >> @@ -420,6 +421,11 @@ static int lsm_append(char *new, char **result) >> return 0; >> } >> >> +/* >> + * Current index to use while initializing the lsmblob secid list. >> + */ >> +static int lsm_slot __initdata; >> + >> /** >> * security_add_hooks - Add a modules hooks to the hook lists. >> * @hooks: the hooks to add >> @@ -427,15 +433,40 @@ static int lsm_append(char *new, char **result) >> * @lsm: the name of the security module >> * >> * Each LSM has to register its hooks with the infrastructure. >> + * If the LSM is using hooks that export secids allocate a slot >> + * for it in the lsmblob. >> */ >> void __init security_add_hooks(struct security_hook_list *hooks, int count, >> char *lsm) >> { >> + int slot = LSMDATA_INVALID; >> int i; >> >> for (i = 0; i < count; i++) { >> hooks[i].lsm = lsm; >> hlist_add_tail_rcu(&hooks[i].list, hooks[i].head); >> + /* >> + * If this is one of the hooks that uses a secid >> + * note it so that a slot can in allocated for the >> + * secid in the lsmblob structure. >> + */ >> + if (hooks[i].head == &security_hook_heads.audit_rule_match || >> + hooks[i].head == &security_hook_heads.kernel_act_as || >> + hooks[i].head == >> + &security_hook_heads.socket_getpeersec_dgram || >> + hooks[i].head == &security_hook_heads.secctx_to_secid || >> + hooks[i].head == &security_hook_heads.secid_to_secctx || >> + hooks[i].head == &security_hook_heads.ipc_getsecid || >> + hooks[i].head == &security_hook_heads.task_getsecid || >> + hooks[i].head == &security_hook_heads.inode_getsecid || >> + hooks[i].head == &security_hook_heads.cred_getsecid) { >> + if (slot == LSMDATA_INVALID) { >> + slot = lsm_slot++; > This needs to sanity check lsm_slot against lsmblob secids array size, > just we we catch cases cleanly if an LSM adds a hook but doesn't add > itself to the LSMDATA_ENTRIES macro. Point, and easy enough. >> + init_debug("%s assigned lsmblob slot %d\n", >> + hooks[i].lsm, slot); >> + } >> + } >> + hooks[i].slot = slot; >> } >> if (lsm_append(lsm, &lsm_names) < 0) >> panic("%s - Cannot get early memory.\n", __func__); >> -- >> 2.20.1 >>
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 3fe39abccc8f..4d1ddf1a2aa6 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -2038,6 +2038,7 @@ struct security_hook_list { struct hlist_head *head; union security_list_options hook; char *lsm; + int slot; } __randomize_layout; /* diff --git a/include/linux/security.h b/include/linux/security.h index 49f2685324b0..28d074866895 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -76,6 +76,68 @@ enum lsm_event { LSM_POLICY_CHANGE, }; +/* + * Data exported by the security modules + */ +#define LSMDATA_ENTRIES ( \ + (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \ + (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \ + (IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0) ) + +struct lsmblob { + u32 secid[LSMDATA_ENTRIES]; +}; + +#define LSMDATA_INVALID -1 + +/** + * lsmblob_init - initialize an lsmblob structure. + * @l: Pointer to the data to initialize + * @secid: The initial secid value + * + * Set all secid for all modules to the specified value. + */ +static inline void lsmblob_init(struct lsmblob *l, u32 secid) +{ + int i; + + for (i = 0; i < LSMDATA_ENTRIES; i++) + l->secid[i] = secid; +} + +/** + * lsmblob_is_set - report if there is an value in the lsmblob + * @l: Pointer to the exported LSM data + * + * Returns true if there is a secid set, false otherwise + */ +static inline bool lsmblob_is_set(struct lsmblob *l) +{ + int i; + + for (i = 0; i < LSMDATA_ENTRIES; i++) + if (l->secid[i] != 0) + return true; + return false; +} + +/** + * lsmblob_equal - report if the two lsmblob's are equal + * @l: Pointer to one LSM data + * @m: Pointer to the other LSM data + * + * Returns true if all entries in the two are equal, false otherwise + */ +static inline bool lsmblob_equal(struct lsmblob *l, struct lsmblob *m) +{ + int i; + + for (i = 0; i < LSMDATA_ENTRIES; i++) + if (l->secid[i] != m->secid[i]) + return false; + return true; +} + /* 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..5aa3c052d702 100644 --- a/security/security.c +++ b/security/security.c @@ -317,6 +317,7 @@ static void __init ordered_lsm_init(void) init_debug("sock blob size = %d\n", blob_sizes.lbs_sock); init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock); init_debug("task blob size = %d\n", blob_sizes.lbs_task); + init_debug("lsmblob size = %lu\n", sizeof(struct lsmblob)); /* * Create any kmem_caches needed for blobs @@ -420,6 +421,11 @@ static int lsm_append(char *new, char **result) return 0; } +/* + * Current index to use while initializing the lsmblob secid list. + */ +static int lsm_slot __initdata; + /** * security_add_hooks - Add a modules hooks to the hook lists. * @hooks: the hooks to add @@ -427,15 +433,40 @@ static int lsm_append(char *new, char **result) * @lsm: the name of the security module * * Each LSM has to register its hooks with the infrastructure. + * If the LSM is using hooks that export secids allocate a slot + * for it in the lsmblob. */ void __init security_add_hooks(struct security_hook_list *hooks, int count, char *lsm) { + int slot = LSMDATA_INVALID; int i; for (i = 0; i < count; i++) { hooks[i].lsm = lsm; hlist_add_tail_rcu(&hooks[i].list, hooks[i].head); + /* + * If this is one of the hooks that uses a secid + * note it so that a slot can in allocated for the + * secid in the lsmblob structure. + */ + if (hooks[i].head == &security_hook_heads.audit_rule_match || + hooks[i].head == &security_hook_heads.kernel_act_as || + hooks[i].head == + &security_hook_heads.socket_getpeersec_dgram || + hooks[i].head == &security_hook_heads.secctx_to_secid || + hooks[i].head == &security_hook_heads.secid_to_secctx || + hooks[i].head == &security_hook_heads.ipc_getsecid || + hooks[i].head == &security_hook_heads.task_getsecid || + hooks[i].head == &security_hook_heads.inode_getsecid || + hooks[i].head == &security_hook_heads.cred_getsecid) { + if (slot == LSMDATA_INVALID) { + slot = lsm_slot++; + init_debug("%s assigned lsmblob slot %d\n", + hooks[i].lsm, slot); + } + } + hooks[i].slot = slot; } if (lsm_append(lsm, &lsm_names) < 0) panic("%s - Cannot get early memory.\n", __func__);
When more than one security module is exporting data to audit and networking sub-systems a single 32 bit integer is no longer sufficient to represent the data. Add a structure to be used instead. The lsmblob structure is currently an array of u32 "secids". There is an entry for each of the security modules built into the system that would use secids if active. The system assigns the module a "slot" when it registers hooks. If modules are compiled in but not registered there will be unused slots. Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> --- include/linux/lsm_hooks.h | 1 + include/linux/security.h | 62 +++++++++++++++++++++++++++++++++++++++ security/security.c | 31 ++++++++++++++++++++ 3 files changed, 94 insertions(+)