| Message ID | 20220418145945.38797-23-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v35,01/29] integrity: disassociate ima_filter_rule from security_audit_rule | expand |
On 4/18/22 07:59, Casey Schaufler wrote: > Replace the osid field in the audit_names structure > with a lsmblob structure. This accomodates the use > of an lsmblob in security_audit_rule_match() and > security_inode_getsecid(). > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > Acked-by: Paul Moore <paul@paul-moore.com> > --- > kernel/audit.h | 2 +- > kernel/auditsc.c | 22 ++++++++-------------- > 2 files changed, 9 insertions(+), 15 deletions(-) > > diff --git a/kernel/audit.h b/kernel/audit.h > index 316fac62d5f7..4af63e7dde17 100644 > --- a/kernel/audit.h > +++ b/kernel/audit.h > @@ -82,7 +82,7 @@ struct audit_names { > kuid_t uid; > kgid_t gid; > dev_t rdev; > - u32 osid; > + struct lsmblob lsmblob; > struct audit_cap_data fcap; > unsigned int fcap_ver; > unsigned char type; /* record type */ > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index 231631f61550..6fe9f2525fc1 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -700,17 +700,16 @@ static int audit_filter_rules(struct task_struct *tsk, > * lsmblob, which happens later in > * this patch set. > */ > - lsmblob_init(&blob, name->osid); > result = security_audit_rule_match( > - &blob, > + &name->lsmblob, > f->type, > f->op, > &f->lsm_rules); > } else if (ctx) { > list_for_each_entry(n, &ctx->names_list, list) { > - lsmblob_init(&blob, n->osid); > if (security_audit_rule_match( > - &blob, f->type, f->op, > + &n->lsmblob, > + f->type, f->op, > &f->lsm_rules)) { > ++result; > break; > @@ -1589,13 +1588,12 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n, > from_kgid(&init_user_ns, n->gid), > MAJOR(n->rdev), > MINOR(n->rdev)); > - if (n->osid != 0) { > - struct lsmblob blob; > + if (lsmblob_is_set(&n->lsmblob)) { > struct lsmcontext lsmctx; > > - lsmblob_init(&blob, n->osid); > - if (security_secid_to_secctx(&blob, &lsmctx, LSMBLOB_FIRST)) { > - audit_log_format(ab, " osid=%u", n->osid); > + if (security_secid_to_secctx(&n->lsmblob, &lsmctx, > + LSMBLOB_FIRST)) { > + audit_log_format(ab, " osid=?"); is there something better we can do here? This feels like a regression > if (call_panic) > *call_panic = 2; > } else { > @@ -2297,17 +2295,13 @@ static void audit_copy_inode(struct audit_names *name, > const struct dentry *dentry, > struct inode *inode, unsigned int flags) > { > - struct lsmblob blob; > - > name->ino = inode->i_ino; > name->dev = inode->i_sb->s_dev; > name->mode = inode->i_mode; > name->uid = inode->i_uid; > name->gid = inode->i_gid; > name->rdev = inode->i_rdev; > - security_inode_getsecid(inode, &blob); > - /* scaffolding until osid is updated */ > - name->osid = lsmblob_first(&blob); > + security_inode_getsecid(inode, &name->lsmblob); > if (flags & AUDIT_INODE_NOEVAL) { > name->fcap_ver = -1; > return;
On Mon, Apr 25, 2022 at 7:32 PM John Johansen <john.johansen@canonical.com> wrote: > On 4/18/22 07:59, Casey Schaufler wrote: > > Replace the osid field in the audit_names structure > > with a lsmblob structure. This accomodates the use > > of an lsmblob in security_audit_rule_match() and > > security_inode_getsecid(). > > > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > > Acked-by: Paul Moore <paul@paul-moore.com> > > --- > > kernel/audit.h | 2 +- > > kernel/auditsc.c | 22 ++++++++-------------- > > 2 files changed, 9 insertions(+), 15 deletions(-) ... > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index 231631f61550..6fe9f2525fc1 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -700,17 +700,16 @@ static int audit_filter_rules(struct task_struct *tsk, > > * lsmblob, which happens later in > > * this patch set. > > */ > > - lsmblob_init(&blob, name->osid); > > result = security_audit_rule_match( > > - &blob, > > + &name->lsmblob, > > f->type, > > f->op, > > &f->lsm_rules); > > } else if (ctx) { > > list_for_each_entry(n, &ctx->names_list, list) { > > - lsmblob_init(&blob, n->osid); > > if (security_audit_rule_match( > > - &blob, f->type, f->op, > > + &n->lsmblob, > > + f->type, f->op, > > &f->lsm_rules)) { > > ++result; > > break; > > @@ -1589,13 +1588,12 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n, > > from_kgid(&init_user_ns, n->gid), > > MAJOR(n->rdev), > > MINOR(n->rdev)); > > - if (n->osid != 0) { > > - struct lsmblob blob; > > + if (lsmblob_is_set(&n->lsmblob)) { > > struct lsmcontext lsmctx; > > > > - lsmblob_init(&blob, n->osid); > > - if (security_secid_to_secctx(&blob, &lsmctx, LSMBLOB_FIRST)) { > > - audit_log_format(ab, " osid=%u", n->osid); > > + if (security_secid_to_secctx(&n->lsmblob, &lsmctx, > > + LSMBLOB_FIRST)) { > > + audit_log_format(ab, " osid=?"); > > is there something better we can do here? This feels like a regression Unfortunately no, or at least nothing has been suggested that is an improvement on this approach. We could overload the existing field, but that runs the risk of confusing userspace tooling and potentially bumping into the buffer limit in some more complex configurations. The "?" value was chosen as it is a commonly accepted way for the audit subsystem to indicate that a value is "missing" and in the case of new/updated userspace tooling this would be an indication to look for the new record type which provides all of the necessary LSM labels. In the case of old/unaware userspace tooling it would serve as a graceful indicator that something is awry, i.e. you are using new kernel functionality without updating your userspace.
diff --git a/kernel/audit.h b/kernel/audit.h index 316fac62d5f7..4af63e7dde17 100644 --- a/kernel/audit.h +++ b/kernel/audit.h @@ -82,7 +82,7 @@ struct audit_names { kuid_t uid; kgid_t gid; dev_t rdev; - u32 osid; + struct lsmblob lsmblob; struct audit_cap_data fcap; unsigned int fcap_ver; unsigned char type; /* record type */ diff --git a/kernel/auditsc.c b/kernel/auditsc.c index 231631f61550..6fe9f2525fc1 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -700,17 +700,16 @@ static int audit_filter_rules(struct task_struct *tsk, * lsmblob, which happens later in * this patch set. */ - lsmblob_init(&blob, name->osid); result = security_audit_rule_match( - &blob, + &name->lsmblob, f->type, f->op, &f->lsm_rules); } else if (ctx) { list_for_each_entry(n, &ctx->names_list, list) { - lsmblob_init(&blob, n->osid); if (security_audit_rule_match( - &blob, f->type, f->op, + &n->lsmblob, + f->type, f->op, &f->lsm_rules)) { ++result; break; @@ -1589,13 +1588,12 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n, from_kgid(&init_user_ns, n->gid), MAJOR(n->rdev), MINOR(n->rdev)); - if (n->osid != 0) { - struct lsmblob blob; + if (lsmblob_is_set(&n->lsmblob)) { struct lsmcontext lsmctx; - lsmblob_init(&blob, n->osid); - if (security_secid_to_secctx(&blob, &lsmctx, LSMBLOB_FIRST)) { - audit_log_format(ab, " osid=%u", n->osid); + if (security_secid_to_secctx(&n->lsmblob, &lsmctx, + LSMBLOB_FIRST)) { + audit_log_format(ab, " osid=?"); if (call_panic) *call_panic = 2; } else { @@ -2297,17 +2295,13 @@ static void audit_copy_inode(struct audit_names *name, const struct dentry *dentry, struct inode *inode, unsigned int flags) { - struct lsmblob blob; - name->ino = inode->i_ino; name->dev = inode->i_sb->s_dev; name->mode = inode->i_mode; name->uid = inode->i_uid; name->gid = inode->i_gid; name->rdev = inode->i_rdev; - security_inode_getsecid(inode, &blob); - /* scaffolding until osid is updated */ - name->osid = lsmblob_first(&blob); + security_inode_getsecid(inode, &name->lsmblob); if (flags & AUDIT_INODE_NOEVAL) { name->fcap_ver = -1; return;