diff mbox series

[v35,22/29] Audit: Keep multiple LSM data in audit_names

Message ID 20220418145945.38797-23-casey@schaufler-ca.com (mailing list archive)
State New
Headers show
Series [v35,01/29] integrity: disassociate ima_filter_rule from security_audit_rule | expand

Commit Message

Casey Schaufler April 18, 2022, 2:59 p.m. UTC
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(-)

Comments

John Johansen April 25, 2022, 11:32 p.m. UTC | #1
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;
Paul Moore April 26, 2022, 5:57 p.m. UTC | #2
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 mbox series

Patch

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;