| Message ID | 20241009173222.12219-5-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | LSM: Move away from secids | expand |
On Oct 9, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote: > > Replace the secid value stored in struct audit_context with a struct > lsm_prop. Change the code that uses this value to accommodate the > change. security_audit_rule_match() expects a lsm_prop, so existing > scaffolding can be removed. A call to security_secid_to_secctx() > is changed to security_lsmprop_to_secctx(). The call to > security_ipc_getsecid() is scaffolded. > > A new function lsmprop_is_set() is introduced to identify whether > an lsm_prop contains a non-zero value. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > include/linux/security.h | 24 ++++++++++++++++++++++++ > kernel/audit.h | 3 ++- > kernel/auditsc.c | 19 ++++++++----------- > 3 files changed, 34 insertions(+), 12 deletions(-) > > diff --git a/include/linux/security.h b/include/linux/security.h > index f1c68e38b15d..5652baa4ca3c 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -291,6 +291,19 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id) > > #ifdef CONFIG_SECURITY > > +/** > + * lsmprop_is_set - report if there is a value in the lsm_prop > + * @prop: Pointer to the exported LSM data > + * > + * Returns true if there is a value set, false otherwise > + */ > +static inline bool lsm_prop_is_set(struct lsm_prop *prop) > +{ > + const struct lsm_prop empty = {}; > + > + return !!memcmp(prop, &empty, sizeof(*prop)); > +} > + > int call_blocking_lsm_notifier(enum lsm_event event, void *data); > int register_blocking_lsm_notifier(struct notifier_block *nb); > int unregister_blocking_lsm_notifier(struct notifier_block *nb); > @@ -552,6 +565,17 @@ int security_bdev_setintegrity(struct block_device *bdev, > size_t size); > #else /* CONFIG_SECURITY */ > > +/** > + * lsmprop_is_set - report if there is a value in the lsm_prop > + * @prop: Pointer to the exported LSM data > + * > + * Returns true if there is a value set, false otherwise > + */ > +static inline bool lsm_prop_is_set(struct lsm_prop *prop) > +{ > + return false; > +} If we're going to call this lsmprop_is_set() (see 5/13), we really should name it that way to start in this patch. Considering everything else in this patchset looks okay, if you want me to fix this up during the merge let me know. -- paul-moore.com
On 10/10/2024 8:08 PM, Paul Moore wrote: > On Oct 9, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote: >> Replace the secid value stored in struct audit_context with a struct >> lsm_prop. Change the code that uses this value to accommodate the >> change. security_audit_rule_match() expects a lsm_prop, so existing >> scaffolding can be removed. A call to security_secid_to_secctx() >> is changed to security_lsmprop_to_secctx(). The call to >> security_ipc_getsecid() is scaffolded. >> >> A new function lsmprop_is_set() is introduced to identify whether >> an lsm_prop contains a non-zero value. >> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> --- >> include/linux/security.h | 24 ++++++++++++++++++++++++ >> kernel/audit.h | 3 ++- >> kernel/auditsc.c | 19 ++++++++----------- >> 3 files changed, 34 insertions(+), 12 deletions(-) >> >> diff --git a/include/linux/security.h b/include/linux/security.h >> index f1c68e38b15d..5652baa4ca3c 100644 >> --- a/include/linux/security.h >> +++ b/include/linux/security.h >> @@ -291,6 +291,19 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id) >> >> #ifdef CONFIG_SECURITY >> >> +/** >> + * lsmprop_is_set - report if there is a value in the lsm_prop >> + * @prop: Pointer to the exported LSM data >> + * >> + * Returns true if there is a value set, false otherwise >> + */ >> +static inline bool lsm_prop_is_set(struct lsm_prop *prop) >> +{ >> + const struct lsm_prop empty = {}; >> + >> + return !!memcmp(prop, &empty, sizeof(*prop)); >> +} >> + >> int call_blocking_lsm_notifier(enum lsm_event event, void *data); >> int register_blocking_lsm_notifier(struct notifier_block *nb); >> int unregister_blocking_lsm_notifier(struct notifier_block *nb); >> @@ -552,6 +565,17 @@ int security_bdev_setintegrity(struct block_device *bdev, >> size_t size); >> #else /* CONFIG_SECURITY */ >> >> +/** >> + * lsmprop_is_set - report if there is a value in the lsm_prop >> + * @prop: Pointer to the exported LSM data >> + * >> + * Returns true if there is a value set, false otherwise >> + */ >> +static inline bool lsm_prop_is_set(struct lsm_prop *prop) >> +{ >> + return false; >> +} > If we're going to call this lsmprop_is_set() (see 5/13), we really should > name it that way to start in this patch. Agreed. That's an unfortunate artifact of the lsmblob to lsm_prop name change. > Considering everything else in this patchset looks okay, if you want me > to fix this up during the merge let me know. I can do a v5 if that makes life easier, but if you're OK with fixing it during the merge I'm completely fine with that. Thank you. > -- > paul-moore.com
On Fri, Oct 11, 2024 at 11:52 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 10/10/2024 8:08 PM, Paul Moore wrote: > > On Oct 9, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote: > >> Replace the secid value stored in struct audit_context with a struct > >> lsm_prop. Change the code that uses this value to accommodate the > >> change. security_audit_rule_match() expects a lsm_prop, so existing > >> scaffolding can be removed. A call to security_secid_to_secctx() > >> is changed to security_lsmprop_to_secctx(). The call to > >> security_ipc_getsecid() is scaffolded. > >> > >> A new function lsmprop_is_set() is introduced to identify whether > >> an lsm_prop contains a non-zero value. > >> > >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > >> --- > >> include/linux/security.h | 24 ++++++++++++++++++++++++ > >> kernel/audit.h | 3 ++- > >> kernel/auditsc.c | 19 ++++++++----------- > >> 3 files changed, 34 insertions(+), 12 deletions(-) ... > >> +/** > >> + * lsmprop_is_set - report if there is a value in the lsm_prop > >> + * @prop: Pointer to the exported LSM data > >> + * > >> + * Returns true if there is a value set, false otherwise > >> + */ > >> +static inline bool lsm_prop_is_set(struct lsm_prop *prop) > >> +{ > >> + return false; > >> +} > > > > If we're going to call this lsmprop_is_set() (see 5/13), we really should > > name it that way to start in this patch. > > Agreed. That's an unfortunate artifact of the lsmblob to lsm_prop name change. > > > Considering everything else in this patchset looks okay, if you want me > > to fix this up during the merge let me know. > > I can do a v5 if that makes life easier, but if you're OK with fixing it > during the merge I'm completely fine with that. Thank you. For trivial things like this where I've already reviewed the full patchset it's easier/quicker if I just make the change as I can do it and not have to re-review everything. Otherwise it's another revision for you to post, me to review, etc.; granted in that case I'm really just diffing between v4 and v5, not really doing a full review unless something odd pops up in the diff, but I think you get the idea.
On 10/11/2024 9:11 AM, Paul Moore wrote: > On Fri, Oct 11, 2024 at 11:52 AM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 10/10/2024 8:08 PM, Paul Moore wrote: >>> On Oct 9, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote: >>>> Replace the secid value stored in struct audit_context with a struct >>>> lsm_prop. Change the code that uses this value to accommodate the >>>> change. security_audit_rule_match() expects a lsm_prop, so existing >>>> scaffolding can be removed. A call to security_secid_to_secctx() >>>> is changed to security_lsmprop_to_secctx(). The call to >>>> security_ipc_getsecid() is scaffolded. >>>> >>>> A new function lsmprop_is_set() is introduced to identify whether >>>> an lsm_prop contains a non-zero value. >>>> >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>>> --- >>>> include/linux/security.h | 24 ++++++++++++++++++++++++ >>>> kernel/audit.h | 3 ++- >>>> kernel/auditsc.c | 19 ++++++++----------- >>>> 3 files changed, 34 insertions(+), 12 deletions(-) > .. > >>>> +/** >>>> + * lsmprop_is_set - report if there is a value in the lsm_prop >>>> + * @prop: Pointer to the exported LSM data >>>> + * >>>> + * Returns true if there is a value set, false otherwise >>>> + */ >>>> +static inline bool lsm_prop_is_set(struct lsm_prop *prop) >>>> +{ >>>> + return false; >>>> +} >>> If we're going to call this lsmprop_is_set() (see 5/13), we really should >>> name it that way to start in this patch. >> Agreed. That's an unfortunate artifact of the lsmblob to lsm_prop name change. >> >>> Considering everything else in this patchset looks okay, if you want me >>> to fix this up during the merge let me know. >> I can do a v5 if that makes life easier, but if you're OK with fixing it >> during the merge I'm completely fine with that. Thank you. > For trivial things like this where I've already reviewed the full > patchset it's easier/quicker if I just make the change as I can do it > and not have to re-review everything. Otherwise it's another revision > for you to post, me to review, etc.; granted in that case I'm really > just diffing between v4 and v5, not really doing a full review unless > something odd pops up in the diff, but I think you get the idea. Indeed. Go forth and merge. Thanks again.
On Fri, Oct 11, 2024 at 12:34 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 10/11/2024 9:11 AM, Paul Moore wrote: > > On Fri, Oct 11, 2024 at 11:52 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > >> On 10/10/2024 8:08 PM, Paul Moore wrote: > >>> On Oct 9, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote: > >>>> Replace the secid value stored in struct audit_context with a struct > >>>> lsm_prop. Change the code that uses this value to accommodate the > >>>> change. security_audit_rule_match() expects a lsm_prop, so existing > >>>> scaffolding can be removed. A call to security_secid_to_secctx() > >>>> is changed to security_lsmprop_to_secctx(). The call to > >>>> security_ipc_getsecid() is scaffolded. > >>>> > >>>> A new function lsmprop_is_set() is introduced to identify whether > >>>> an lsm_prop contains a non-zero value. > >>>> > >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > >>>> --- > >>>> include/linux/security.h | 24 ++++++++++++++++++++++++ > >>>> kernel/audit.h | 3 ++- > >>>> kernel/auditsc.c | 19 ++++++++----------- > >>>> 3 files changed, 34 insertions(+), 12 deletions(-) > > .. > > > >>>> +/** > >>>> + * lsmprop_is_set - report if there is a value in the lsm_prop > >>>> + * @prop: Pointer to the exported LSM data > >>>> + * > >>>> + * Returns true if there is a value set, false otherwise > >>>> + */ > >>>> +static inline bool lsm_prop_is_set(struct lsm_prop *prop) > >>>> +{ > >>>> + return false; > >>>> +} > >>> If we're going to call this lsmprop_is_set() (see 5/13), we really should > >>> name it that way to start in this patch. > >> Agreed. That's an unfortunate artifact of the lsmblob to lsm_prop name change. > >> > >>> Considering everything else in this patchset looks okay, if you want me > >>> to fix this up during the merge let me know. > >> I can do a v5 if that makes life easier, but if you're OK with fixing it > >> during the merge I'm completely fine with that. Thank you. > > For trivial things like this where I've already reviewed the full > > patchset it's easier/quicker if I just make the change as I can do it > > and not have to re-review everything. Otherwise it's another revision > > for you to post, me to review, etc.; granted in that case I'm really > > just diffing between v4 and v5, not really doing a full review unless > > something odd pops up in the diff, but I think you get the idea. > > Indeed. Go forth and merge. Thanks again. ... and now everything is merged into lsm/dev, thanks everyone!
diff --git a/include/linux/security.h b/include/linux/security.h index f1c68e38b15d..5652baa4ca3c 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -291,6 +291,19 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id) #ifdef CONFIG_SECURITY +/** + * lsmprop_is_set - report if there is a value in the lsm_prop + * @prop: Pointer to the exported LSM data + * + * Returns true if there is a value set, false otherwise + */ +static inline bool lsm_prop_is_set(struct lsm_prop *prop) +{ + const struct lsm_prop empty = {}; + + return !!memcmp(prop, &empty, sizeof(*prop)); +} + int call_blocking_lsm_notifier(enum lsm_event event, void *data); int register_blocking_lsm_notifier(struct notifier_block *nb); int unregister_blocking_lsm_notifier(struct notifier_block *nb); @@ -552,6 +565,17 @@ int security_bdev_setintegrity(struct block_device *bdev, size_t size); #else /* CONFIG_SECURITY */ +/** + * lsmprop_is_set - report if there is a value in the lsm_prop + * @prop: Pointer to the exported LSM data + * + * Returns true if there is a value set, false otherwise + */ +static inline bool lsm_prop_is_set(struct lsm_prop *prop) +{ + return false; +} + static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data) { return 0; diff --git a/kernel/audit.h b/kernel/audit.h index a60d2840559e..d14924a887c9 100644 --- a/kernel/audit.h +++ b/kernel/audit.h @@ -11,6 +11,7 @@ #include <linux/fs.h> #include <linux/audit.h> +#include <linux/security.h> #include <linux/skbuff.h> #include <uapi/linux/mqueue.h> #include <linux/tty.h> @@ -160,7 +161,7 @@ struct audit_context { kuid_t uid; kgid_t gid; umode_t mode; - u32 osid; + struct lsm_prop oprop; int has_perm; uid_t perm_uid; gid_t perm_gid; diff --git a/kernel/auditsc.c b/kernel/auditsc.c index aaf672a962d6..e89499819817 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -724,9 +724,7 @@ static int audit_filter_rules(struct task_struct *tsk, /* Find ipc objects that match */ if (!ctx || ctx->type != AUDIT_IPC) break; - /* scaffolding */ - prop.scaffold.secid = ctx->ipc.osid; - if (security_audit_rule_match(&prop, + if (security_audit_rule_match(&ctx->ipc.oprop, f->type, f->op, f->lsm_rule)) ++result; @@ -1394,19 +1392,17 @@ static void show_special(struct audit_context *context, int *call_panic) audit_log_format(ab, " a%d=%lx", i, context->socketcall.args[i]); break; } - case AUDIT_IPC: { - u32 osid = context->ipc.osid; - + case AUDIT_IPC: audit_log_format(ab, "ouid=%u ogid=%u mode=%#ho", from_kuid(&init_user_ns, context->ipc.uid), from_kgid(&init_user_ns, context->ipc.gid), context->ipc.mode); - if (osid) { + if (lsm_prop_is_set(&context->ipc.oprop)) { char *ctx = NULL; u32 len; - if (security_secid_to_secctx(osid, &ctx, &len)) { - audit_log_format(ab, " osid=%u", osid); + if (security_lsmprop_to_secctx(&context->ipc.oprop, + &ctx, &len)) { *call_panic = 1; } else { audit_log_format(ab, " obj=%s", ctx); @@ -1426,7 +1422,7 @@ static void show_special(struct audit_context *context, int *call_panic) context->ipc.perm_gid, context->ipc.perm_mode); } - break; } + break; case AUDIT_MQ_OPEN: audit_log_format(ab, "oflag=0x%x mode=%#ho mq_flags=0x%lx mq_maxmsg=%ld " @@ -2642,7 +2638,8 @@ void __audit_ipc_obj(struct kern_ipc_perm *ipcp) context->ipc.gid = ipcp->gid; context->ipc.mode = ipcp->mode; context->ipc.has_perm = 0; - security_ipc_getsecid(ipcp, &context->ipc.osid); + /* scaffolding */ + security_ipc_getsecid(ipcp, &context->ipc.oprop.scaffold.secid); context->type = AUDIT_IPC; }
Replace the secid value stored in struct audit_context with a struct lsm_prop. Change the code that uses this value to accommodate the change. security_audit_rule_match() expects a lsm_prop, so existing scaffolding can be removed. A call to security_secid_to_secctx() is changed to security_lsmprop_to_secctx(). The call to security_ipc_getsecid() is scaffolded. A new function lsmprop_is_set() is introduced to identify whether an lsm_prop contains a non-zero value. Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> --- include/linux/security.h | 24 ++++++++++++++++++++++++ kernel/audit.h | 3 ++- kernel/auditsc.c | 19 ++++++++----------- 3 files changed, 34 insertions(+), 12 deletions(-)