audit: group task params

RFC PATCH Superseded Richard Guy Briggs 3 participants 18 messages May 4, 2018 – May 11, 2018

Subsystems (15)

Networking Network drivers netfilter MM core +11 more subsystems

lore

Share & tools

lore

Richard Guy Briggs

author

2018-05-04 20:54 UTC #0 lore raw

cover audit: group task params

Group the audit parameters for each task into one structure. In particular, remove the loginuid and sessionid values and the audit context pointer from the task structure, replacing them with an audit task information structure to contain them. Use access functions to access audit values.

Note: Use static allocation of the audit task information structure initially. Dynamic allocation was considered and attempted, but isn't ready yet. Static allocation has the limitation that future audit task information structure changes would cause a visible change to the rest of the kernel, whereas dynamic allocation would mostly hide any future changes.

The first four access normalization patches could stand alone.

Passes audit-testsuite.

Richard Guy Briggs (5):
audit: normalize loginuid read access
audit: convert sessionid unset to a macro
audit: use inline function to get audit context
audit: use inline function to set audit context
audit: collect audit task parameters

 MAINTAINERS                          |  2 +-
 include/linux/audit.h                | 30 ++++++++++---
 include/linux/audit_task.h           | 31 ++++++++++++++
 include/linux/sched.h                |  6 +--
 include/net/xfrm.h                   |  4 +-
 include/uapi/linux/audit.h           |  1 +
 init/init_task.c                     |  8 +++-
 kernel/audit.c                       |  4 +-
 kernel/audit_watch.c                 |  2 +-
 kernel/auditsc.c                     | 82 ++++++++++++++++++------------------
 kernel/fork.c                        |  2 +-
 net/bridge/netfilter/ebtables.c      |  2 +-
 net/core/dev.c                       |  2 +-
 net/netfilter/x_tables.c             |  2 +-
 net/netlabel/netlabel_user.c         |  2 +-
 security/integrity/ima/ima_api.c     |  2 +-
 security/integrity/integrity_audit.c |  2 +-
 security/lsm_audit.c                 |  2 +-
 security/selinux/hooks.c             |  4 +-
 security/selinux/selinuxfs.c         |  6 +--
 security/selinux/ss/services.c       | 12 +++---
 21 files changed, 129 insertions(+), 79 deletions(-)
 create mode 100644 include/linux/audit_task.h

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2018-05-04 20:54 UTC #1 lore raw

RFC PATCH 1/5 audit: normalize loginuid read access

Recognizing that the loginuid is an internal audit value, use an access function to retrieve the audit loginuid value for the task rather than reaching directly into the task struct to get it.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/auditsc.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

kernel/auditsc.c +8 -8

index 479c031..f3817d0 100644
@@ -374,7 +374,7 @@ static int audit_field_compare(struct task_struct *tsk,
 	case AUDIT_COMPARE_EGID_TO_OBJ_GID:
 		return audit_compare_gid(cred->egid, name, f, ctx);
 	case AUDIT_COMPARE_AUID_TO_OBJ_UID:
-		return audit_compare_uid(tsk->loginuid, name, f, ctx);
+		return audit_compare_uid(audit_get_loginuid(tsk), name, f, ctx);
 	case AUDIT_COMPARE_SUID_TO_OBJ_UID:
 		return audit_compare_uid(cred->suid, name, f, ctx);
 	case AUDIT_COMPARE_SGID_TO_OBJ_GID:

@@ -385,7 +385,7 @@ static int audit_field_compare(struct task_struct *tsk,
 		return audit_compare_gid(cred->fsgid, name, f, ctx);
 	/* uid comparisons */
 	case AUDIT_COMPARE_UID_TO_AUID:
-		return audit_uid_comparator(cred->uid, f->op, tsk->loginuid);
+		return audit_uid_comparator(cred->uid, f->op, audit_get_loginuid(tsk));
 	case AUDIT_COMPARE_UID_TO_EUID:
 		return audit_uid_comparator(cred->uid, f->op, cred->euid);
 	case AUDIT_COMPARE_UID_TO_SUID:

@@ -394,11 +394,11 @@ static int audit_field_compare(struct task_struct *tsk,
 		return audit_uid_comparator(cred->uid, f->op, cred->fsuid);
 	/* auid comparisons */
 	case AUDIT_COMPARE_AUID_TO_EUID:
-		return audit_uid_comparator(tsk->loginuid, f->op, cred->euid);
+		return audit_uid_comparator(audit_get_loginuid(tsk), f->op, cred->euid);
 	case AUDIT_COMPARE_AUID_TO_SUID:
-		return audit_uid_comparator(tsk->loginuid, f->op, cred->suid);
+		return audit_uid_comparator(audit_get_loginuid(tsk), f->op, cred->suid);
 	case AUDIT_COMPARE_AUID_TO_FSUID:
-		return audit_uid_comparator(tsk->loginuid, f->op, cred->fsuid);
+		return audit_uid_comparator(audit_get_loginuid(tsk), f->op, cred->fsuid);
 	/* euid comparisons */
 	case AUDIT_COMPARE_EUID_TO_SUID:
 		return audit_uid_comparator(cred->euid, f->op, cred->suid);

@@ -611,7 +611,7 @@ static int audit_filter_rules(struct task_struct *tsk,
 				result = match_tree_refs(ctx, rule->tree);
 			break;
 		case AUDIT_LOGINUID:
-			result = audit_uid_comparator(tsk->loginuid, f->op, f->uid);
+			result = audit_uid_comparator(audit_get_loginuid(tsk), f->op, f->uid);
 			break;
 		case AUDIT_LOGINUID_SET:
 			result = audit_comparator(audit_loginuid_set(tsk), f->op, f->val);

@@ -2287,8 +2287,8 @@ int audit_signal_info(int sig, struct task_struct *t)
 	    (sig == SIGTERM || sig == SIGHUP ||
 	     sig == SIGUSR1 || sig == SIGUSR2)) {
 		audit_sig_pid = task_tgid_nr(tsk);
-		if (uid_valid(tsk->loginuid))
-			audit_sig_uid = tsk->loginuid;
+		if (uid_valid(audit_get_loginuid(tsk)))
+			audit_sig_uid = audit_get_loginuid(tsk);
 		else
 			audit_sig_uid = uid;
 		security_task_getsecid(tsk, &audit_sig_sid);

Signature · 1 line

1.8.3.1

Paul Moore

maintainer

2018-05-09 15:13 UTC #2 lore raw

Re: [PATCH ghak81 RFC V1 1/5] audit: normalize loginuid read access

Quoted 56 lines · Richard Guy Briggs

Recognizing that the loginuid is an internal audit value, use an access function to retrieve the audit loginuid value for the task rather than reaching directly into the task struct to get it.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/auditsc.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

kernel/auditsc.c +6 -6

index 479c031..f3817d0 100644
@@ -374,7 +374,7 @@ static int audit_field_compare(struct task_struct *tsk,
        case AUDIT_COMPARE_EGID_TO_OBJ_GID:
                return audit_compare_gid(cred->egid, name, f, ctx);
        case AUDIT_COMPARE_AUID_TO_OBJ_UID:
-               return audit_compare_uid(tsk->loginuid, name, f, ctx);
+               return audit_compare_uid(audit_get_loginuid(tsk), name, f, ctx);
        case AUDIT_COMPARE_SUID_TO_OBJ_UID:
                return audit_compare_uid(cred->suid, name, f, ctx);
        case AUDIT_COMPARE_SGID_TO_OBJ_GID:

@@ -385,7 +385,7 @@ static int audit_field_compare(struct task_struct *tsk,
                return audit_compare_gid(cred->fsgid, name, f, ctx);
        /* uid comparisons */
        case AUDIT_COMPARE_UID_TO_AUID:
-               return audit_uid_comparator(cred->uid, f->op, tsk->loginuid);
+               return audit_uid_comparator(cred->uid, f->op, audit_get_loginuid(tsk));
        case AUDIT_COMPARE_UID_TO_EUID:
                return audit_uid_comparator(cred->uid, f->op, cred->euid);
        case AUDIT_COMPARE_UID_TO_SUID:

@@ -394,11 +394,11 @@ static int audit_field_compare(struct task_struct *tsk,
                return audit_uid_comparator(cred->uid, f->op, cred->fsuid);
        /* auid comparisons */
        case AUDIT_COMPARE_AUID_TO_EUID:
-               return audit_uid_comparator(tsk->loginuid, f->op, cred->euid);
+               return audit_uid_comparator(audit_get_loginuid(tsk), f->op, cred->euid);
        case AUDIT_COMPARE_AUID_TO_SUID:
-               return audit_uid_comparator(tsk->loginuid, f->op, cred->suid);
+               return audit_uid_comparator(audit_get_loginuid(tsk), f->op, cred->suid);
        case AUDIT_COMPARE_AUID_TO_FSUID:
-               return audit_uid_comparator(tsk->loginuid, f->op, cred->fsuid);
+               return audit_uid_comparator(audit_get_loginuid(tsk), f->op, cred->fsuid);
        /* euid comparisons */
        case AUDIT_COMPARE_EUID_TO_SUID:
                return audit_uid_comparator(cred->euid, f->op, cred->suid);

@@ -611,7 +611,7 @@ static int audit_filter_rules(struct task_struct *tsk,
                                result = match_tree_refs(ctx, rule->tree);
                        break;
                case AUDIT_LOGINUID:
-                       result = audit_uid_comparator(tsk->loginuid, f->op, f->uid);
+                       result = audit_uid_comparator(audit_get_loginuid(tsk), f->op, f->uid);
                        break;
                case AUDIT_LOGINUID_SET:
                        result = audit_comparator(audit_loginuid_set(tsk), f->op, f->val);

kernel/auditsc.c int audit_signal_info(int sig, struct task_struct *t) · line 2291

Quoted 7 lines

kernel/auditsc.c +2 -2

@@ -2287,8 +2287,8 @@ int audit_signal_info(int sig, struct task_struct *t)
            (sig == SIGTERM || sig == SIGHUP ||
             sig == SIGUSR1 || sig == SIGUSR2)) {
                audit_sig_pid = task_tgid_nr(tsk);
-               if (uid_valid(tsk->loginuid))
-                       audit_sig_uid = tsk->loginuid;
+               if (uid_valid(audit_get_loginuid(tsk)))
+                       audit_sig_uid = audit_get_loginuid(tsk);

I realize this comment is a little silly given the nature of loginuid, but if we are going to abstract away loginuid accesses (which I think is good), we should probably access it once, store it in a local variable, perform the validity check on the local variable, then commit the local variable to audit_sig_uid. I realize a TOCTOU problem is unlikely here, but with this new layer of abstraction it seems that some additional safety might be a good thing.

Quoted 5 lines

kernel/auditsc.c +0 -1

                else
                        audit_sig_uid = uid;
                security_task_getsecid(tsk, &audit_sig_sid);
--

1.8.3.1

Signature · 2 lines

paul moore
www.paul-moore.com

Richard Guy Briggs

author

2018-05-10 21:21 UTC #3 lore raw

Re: [PATCH ghak81 RFC V1 1/5] audit: normalize loginuid read access

Quoted 65 lines · Paul Moore

Quoted 56 lines · Richard Guy Briggs

Recognizing that the loginuid is an internal audit value, use an access function to retrieve the audit loginuid value for the task rather than reaching directly into the task struct to get it.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/auditsc.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

kernel/auditsc.c +6 -6

index 479c031..f3817d0 100644
@@ -374,7 +374,7 @@ static int audit_field_compare(struct task_struct *tsk,
        case AUDIT_COMPARE_EGID_TO_OBJ_GID:
                return audit_compare_gid(cred->egid, name, f, ctx);
        case AUDIT_COMPARE_AUID_TO_OBJ_UID:
-               return audit_compare_uid(tsk->loginuid, name, f, ctx);
+               return audit_compare_uid(audit_get_loginuid(tsk), name, f, ctx);
        case AUDIT_COMPARE_SUID_TO_OBJ_UID:
                return audit_compare_uid(cred->suid, name, f, ctx);
        case AUDIT_COMPARE_SGID_TO_OBJ_GID:

@@ -385,7 +385,7 @@ static int audit_field_compare(struct task_struct *tsk,
                return audit_compare_gid(cred->fsgid, name, f, ctx);
        /* uid comparisons */
        case AUDIT_COMPARE_UID_TO_AUID:
-               return audit_uid_comparator(cred->uid, f->op, tsk->loginuid);
+               return audit_uid_comparator(cred->uid, f->op, audit_get_loginuid(tsk));
        case AUDIT_COMPARE_UID_TO_EUID:
                return audit_uid_comparator(cred->uid, f->op, cred->euid);
        case AUDIT_COMPARE_UID_TO_SUID:

@@ -394,11 +394,11 @@ static int audit_field_compare(struct task_struct *tsk,
                return audit_uid_comparator(cred->uid, f->op, cred->fsuid);
        /* auid comparisons */
        case AUDIT_COMPARE_AUID_TO_EUID:
-               return audit_uid_comparator(tsk->loginuid, f->op, cred->euid);
+               return audit_uid_comparator(audit_get_loginuid(tsk), f->op, cred->euid);
        case AUDIT_COMPARE_AUID_TO_SUID:
-               return audit_uid_comparator(tsk->loginuid, f->op, cred->suid);
+               return audit_uid_comparator(audit_get_loginuid(tsk), f->op, cred->suid);
        case AUDIT_COMPARE_AUID_TO_FSUID:
-               return audit_uid_comparator(tsk->loginuid, f->op, cred->fsuid);
+               return audit_uid_comparator(audit_get_loginuid(tsk), f->op, cred->fsuid);
        /* euid comparisons */
        case AUDIT_COMPARE_EUID_TO_SUID:
                return audit_uid_comparator(cred->euid, f->op, cred->suid);

@@ -611,7 +611,7 @@ static int audit_filter_rules(struct task_struct *tsk,
                                result = match_tree_refs(ctx, rule->tree);
                        break;
                case AUDIT_LOGINUID:
-                       result = audit_uid_comparator(tsk->loginuid, f->op, f->uid);
+                       result = audit_uid_comparator(audit_get_loginuid(tsk), f->op, f->uid);
                        break;
                case AUDIT_LOGINUID_SET:
                        result = audit_comparator(audit_loginuid_set(tsk), f->op, f->val);

kernel/auditsc.c int audit_signal_info(int sig, struct task_struct *t) · line 2291

Quoted 7 lines

kernel/auditsc.c +2 -2

@@ -2287,8 +2287,8 @@ int audit_signal_info(int sig, struct task_struct *t)
            (sig == SIGTERM || sig == SIGHUP ||
             sig == SIGUSR1 || sig == SIGUSR2)) {
                audit_sig_pid = task_tgid_nr(tsk);
-               if (uid_valid(tsk->loginuid))
-                       audit_sig_uid = tsk->loginuid;
+               if (uid_valid(audit_get_loginuid(tsk)))
+                       audit_sig_uid = audit_get_loginuid(tsk);

Quoted 7 lines

I realize this comment is a little silly given the nature of loginuid, but if we are going to abstract away loginuid accesses (which I think is good), we should probably access it once, store it in a local variable, perform the validity check on the local variable, then commit the local variable to audit_sig_uid. I realize a TOCTOU problem is unlikely here, but with this new layer of abstraction it seems that some additional safety might be a good thing.

Ok, I'll just assign it to where it is going and check it there, holding the audit_ctl_lock the whole time, since it should have been done anyways for all of audit_sig_{pid,uid,sid} anyways to get a consistent view from the AUDIT_SIGNAL_INFO fetch.

Quoted 5 lines

kernel/auditsc.c int audit_signal_info(int sig, struct task_struct *t) · line 2294
Quoted 3 lines
kernel/auditsc.c +0 -0
                else
                        audit_sig_uid = uid;
                security_task_getsecid(tsk, &audit_sig_sid);
paul moore

- RGB

--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Richard Guy Briggs

author

2018-05-11 22:17 UTC #4 lore raw

Re: [PATCH ghak81 RFC V1 1/5] audit: normalize loginuid read access

Quoted 74 lines · Richard Guy Briggs

Quoted 65 lines · Paul Moore

Quoted 56 lines · Richard Guy Briggs

Recognizing that the loginuid is an internal audit value, use an access function to retrieve the audit loginuid value for the task rather than reaching directly into the task struct to get it.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/auditsc.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

kernel/auditsc.c +6 -6

index 479c031..f3817d0 100644
@@ -374,7 +374,7 @@ static int audit_field_compare(struct task_struct *tsk,
        case AUDIT_COMPARE_EGID_TO_OBJ_GID:
                return audit_compare_gid(cred->egid, name, f, ctx);
        case AUDIT_COMPARE_AUID_TO_OBJ_UID:
-               return audit_compare_uid(tsk->loginuid, name, f, ctx);
+               return audit_compare_uid(audit_get_loginuid(tsk), name, f, ctx);
        case AUDIT_COMPARE_SUID_TO_OBJ_UID:
                return audit_compare_uid(cred->suid, name, f, ctx);
        case AUDIT_COMPARE_SGID_TO_OBJ_GID:

@@ -385,7 +385,7 @@ static int audit_field_compare(struct task_struct *tsk,
                return audit_compare_gid(cred->fsgid, name, f, ctx);
        /* uid comparisons */
        case AUDIT_COMPARE_UID_TO_AUID:
-               return audit_uid_comparator(cred->uid, f->op, tsk->loginuid);
+               return audit_uid_comparator(cred->uid, f->op, audit_get_loginuid(tsk));
        case AUDIT_COMPARE_UID_TO_EUID:
                return audit_uid_comparator(cred->uid, f->op, cred->euid);
        case AUDIT_COMPARE_UID_TO_SUID:

@@ -394,11 +394,11 @@ static int audit_field_compare(struct task_struct *tsk,
                return audit_uid_comparator(cred->uid, f->op, cred->fsuid);
        /* auid comparisons */
        case AUDIT_COMPARE_AUID_TO_EUID:
-               return audit_uid_comparator(tsk->loginuid, f->op, cred->euid);
+               return audit_uid_comparator(audit_get_loginuid(tsk), f->op, cred->euid);
        case AUDIT_COMPARE_AUID_TO_SUID:
-               return audit_uid_comparator(tsk->loginuid, f->op, cred->suid);
+               return audit_uid_comparator(audit_get_loginuid(tsk), f->op, cred->suid);
        case AUDIT_COMPARE_AUID_TO_FSUID:
-               return audit_uid_comparator(tsk->loginuid, f->op, cred->fsuid);
+               return audit_uid_comparator(audit_get_loginuid(tsk), f->op, cred->fsuid);
        /* euid comparisons */
        case AUDIT_COMPARE_EUID_TO_SUID:
                return audit_uid_comparator(cred->euid, f->op, cred->suid);

@@ -611,7 +611,7 @@ static int audit_filter_rules(struct task_struct *tsk,
                                result = match_tree_refs(ctx, rule->tree);
                        break;
                case AUDIT_LOGINUID:
-                       result = audit_uid_comparator(tsk->loginuid, f->op, f->uid);
+                       result = audit_uid_comparator(audit_get_loginuid(tsk), f->op, f->uid);
                        break;
                case AUDIT_LOGINUID_SET:
                        result = audit_comparator(audit_loginuid_set(tsk), f->op, f->val);

kernel/auditsc.c int audit_signal_info(int sig, struct task_struct *t) · line 2291

Quoted 7 lines

kernel/auditsc.c +2 -2

@@ -2287,8 +2287,8 @@ int audit_signal_info(int sig, struct task_struct *t)
            (sig == SIGTERM || sig == SIGHUP ||
             sig == SIGUSR1 || sig == SIGUSR2)) {
                audit_sig_pid = task_tgid_nr(tsk);
-               if (uid_valid(tsk->loginuid))
-                       audit_sig_uid = tsk->loginuid;
+               if (uid_valid(audit_get_loginuid(tsk)))
+                       audit_sig_uid = audit_get_loginuid(tsk);

Quoted 7 lines

I realize this comment is a little silly given the nature of loginuid, but if we are going to abstract away loginuid accesses (which I think is good), we should probably access it once, store it in a local variable, perform the validity check on the local variable, then commit the local variable to audit_sig_uid. I realize a TOCTOU problem is unlikely here, but with this new layer of abstraction it seems that some additional safety might be a good thing.

Quoted 4 lines

Ok, I'll just assign it to where it is going and check it there, holding the audit_ctl_lock the whole time, since it should have been done anyways for all of audit_sig_{pid,uid,sid} anyways to get a consistent view from the AUDIT_SIGNAL_INFO fetch.

Hmmm, holding audit_ctl_lock won't work because it could sleep trying to get the lock and the signal info is set in a context where sleeping isn't permitted. I'll just use a local var...

Quoted 7 lines

Quoted 5 lines
kernel/auditsc.c int audit_signal_info(int sig, struct task_struct *t) · line 2294
Quoted 3 lines
kernel/auditsc.c +0 -0
                else
                        audit_sig_uid = uid;
                security_task_getsecid(tsk, &audit_sig_sid);
paul moore
- RGB

- RGB

--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Richard Guy Briggs

author

2018-05-04 20:54 UTC #5 lore raw

RFC PATCH 2/5 audit: convert sessionid unset to a macro

In reply to Richard Guy Briggs

Use a macro, "AUDIT_SID_UNSET", to replace each instance of initialization and comparison to an audit session ID.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h      | 2 +-
 include/net/xfrm.h         | 2 +-
 include/uapi/linux/audit.h | 1 +
 init/init_task.c           | 2 +-
 kernel/auditsc.c           | 4 ++--
 5 files changed, 6 insertions(+), 5 deletions(-)

include/linux/audit.h +1 -1

index 75d5b03..5f86f7c 100644
@@ -513,7 +513,7 @@ static inline kuid_t audit_get_loginuid(struct task_struct *tsk)
 }
 static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 {
-	return -1;
+	return AUDIT_SID_UNSET;
 }
 static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
 { }

include/net/xfrm.h +1 -1

index a872379..fcce8ee 100644
@@ -751,7 +751,7 @@ static inline void xfrm_audit_helper_usrinfo(bool task_valid,
 					    audit_get_loginuid(current) :
 					    INVALID_UID);
 	const unsigned int ses = task_valid ? audit_get_sessionid(current) :
-		(unsigned int) -1;
+		AUDIT_SID_UNSET;
 
 	audit_log_format(audit_buf, " auid=%u ses=%u", auid, ses);
 	audit_log_task_context(audit_buf);

include/uapi/linux/audit.h +1 -0

index 4e61a9e..04f9bd2 100644
@@ -465,6 +465,7 @@ struct audit_tty_status {
 };
 
 #define AUDIT_UID_UNSET (unsigned int)-1
+#define AUDIT_SID_UNSET ((unsigned int)-1)
 
 /* audit_rule_data supports filter rules with both integer and string
  * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and

init/init_task.c +1 -1

index 3ac6e75..c788f91 100644
@@ -119,7 +119,7 @@ struct task_struct init_task
 	.thread_node	= LIST_HEAD_INIT(init_signals.thread_head),
 #ifdef CONFIG_AUDITSYSCALL
 	.loginuid	= INVALID_UID,
-	.sessionid	= (unsigned int)-1,
+	.sessionid	= AUDIT_SID_UNSET,
 #endif
 #ifdef CONFIG_PERF_EVENTS
 	.perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),

kernel/auditsc.c +2 -2

index f3817d0..6e3ceb9 100644
@@ -2050,7 +2050,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid,
 int audit_set_loginuid(kuid_t loginuid)
 {
 	struct task_struct *task = current;
-	unsigned int oldsessionid, sessionid = (unsigned int)-1;
+	unsigned int oldsessionid, sessionid = AUDIT_SID_UNSET;
 	kuid_t oldloginuid;
 	int rc;

@@ -2064,7 +2064,7 @@ int audit_set_loginuid(kuid_t loginuid)
 	/* are we setting or clearing? */
 	if (uid_valid(loginuid)) {
 		sessionid = (unsigned int)atomic_inc_return(&session_id);
-		if (unlikely(sessionid == (unsigned int)-1))
+		if (unlikely(sessionid == AUDIT_SID_UNSET))
 			sessionid = (unsigned int)atomic_inc_return(&session_id);
 	}

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2018-05-09 01:34 UTC #6 lore raw

Re: [PATCH ghak81 RFC V1 2/5] audit: convert sessionid unset to a macro

Quoted 4 lines · Richard Guy Briggs

Use a macro, "AUDIT_SID_UNSET", to replace each instance of initialization and comparison to an audit session ID.
Signed-off-by Richard Guy Briggs <[email protected]>

There's a minor issue with this patch, adding a header include to init/init_task.c in this patch and removing it from patch 5. That'll be in the next revision.

I have dynamic allocation working, so that has a good chance of appearing too.

Quoted 88 lines

---
 include/linux/audit.h      | 2 +-
 include/net/xfrm.h         | 2 +-
 include/uapi/linux/audit.h | 1 +
 init/init_task.c           | 2 +-
 kernel/auditsc.c           | 4 ++--
 5 files changed, 6 insertions(+), 5 deletions(-)

include/linux/audit.h +1 -1

index 75d5b03..5f86f7c 100644
@@ -513,7 +513,7 @@ static inline kuid_t audit_get_loginuid(struct task_struct *tsk)
 }
 static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 {
-	return -1;
+	return AUDIT_SID_UNSET;
 }
 static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
 { }

include/net/xfrm.h +1 -1

index a872379..fcce8ee 100644
@@ -751,7 +751,7 @@ static inline void xfrm_audit_helper_usrinfo(bool task_valid,
 					    audit_get_loginuid(current) :
 					    INVALID_UID);
 	const unsigned int ses = task_valid ? audit_get_sessionid(current) :
-		(unsigned int) -1;
+		AUDIT_SID_UNSET;
 
 	audit_log_format(audit_buf, " auid=%u ses=%u", auid, ses);
 	audit_log_task_context(audit_buf);

include/uapi/linux/audit.h +1 -0

index 4e61a9e..04f9bd2 100644
@@ -465,6 +465,7 @@ struct audit_tty_status {
 };
 
 #define AUDIT_UID_UNSET (unsigned int)-1
+#define AUDIT_SID_UNSET ((unsigned int)-1)
 
 /* audit_rule_data supports filter rules with both integer and string
  * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and

init/init_task.c +1 -1

index 3ac6e75..c788f91 100644
@@ -119,7 +119,7 @@ struct task_struct init_task
 	.thread_node	= LIST_HEAD_INIT(init_signals.thread_head),
 #ifdef CONFIG_AUDITSYSCALL
 	.loginuid	= INVALID_UID,
-	.sessionid	= (unsigned int)-1,
+	.sessionid	= AUDIT_SID_UNSET,
 #endif
 #ifdef CONFIG_PERF_EVENTS
 	.perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),

kernel/auditsc.c +2 -2

index f3817d0..6e3ceb9 100644
@@ -2050,7 +2050,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid,
 int audit_set_loginuid(kuid_t loginuid)
 {
 	struct task_struct *task = current;
-	unsigned int oldsessionid, sessionid = (unsigned int)-1;
+	unsigned int oldsessionid, sessionid = AUDIT_SID_UNSET;
 	kuid_t oldloginuid;
 	int rc;

@@ -2064,7 +2064,7 @@ int audit_set_loginuid(kuid_t loginuid)
 	/* are we setting or clearing? */
 	if (uid_valid(loginuid)) {
 		sessionid = (unsigned int)atomic_inc_return(&session_id);
-		if (unlikely(sessionid == (unsigned int)-1))
+		if (unlikely(sessionid == AUDIT_SID_UNSET))
 			sessionid = (unsigned int)atomic_inc_return(&session_id);
 	}

Signature · 6 lines

1.8.3.1

--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Paul Moore

maintainer

2018-05-09 15:18 UTC #7 lore raw

Re: [PATCH ghak81 RFC V1 2/5] audit: convert sessionid unset to a macro

Quoted 9 lines · Richard Guy Briggs

Quoted 4 lines · Richard Guy Briggs
Use a macro, "AUDIT_SID_UNSET", to replace each instance of initialization and comparison to an audit session ID.
Signed-off-by Richard Guy Briggs <[email protected]>
There's a minor issue with this patch, adding a header include to init/init_task.c in this patch and removing it from patch 5. That'll be in the next revision.

Okay, thanks for the heads-up. FWIW, this patch looks reasonable in principle; changing magic numbers to macros/constants is almost always a step in the right direction.

Quoted 2 lines

I have dynamic allocation working, so that has a good chance of appearing too.

I'll comment on that in your patch 0, I just want to get through the rest of the patches first.

Quoted 102 lines

Quoted 84 lines

---
 include/linux/audit.h      | 2 +-
 include/net/xfrm.h         | 2 +-
 include/uapi/linux/audit.h | 1 +
 init/init_task.c           | 2 +-
 kernel/auditsc.c           | 4 ++--
 5 files changed, 6 insertions(+), 5 deletions(-)

include/linux/audit.h +1 -1

index 75d5b03..5f86f7c 100644
@@ -513,7 +513,7 @@ static inline kuid_t audit_get_loginuid(struct task_struct *tsk)
 }
 static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 {
-     return -1;
+     return AUDIT_SID_UNSET;
 }
 static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
 { }

include/net/xfrm.h +1 -1

index a872379..fcce8ee 100644
@@ -751,7 +751,7 @@ static inline void xfrm_audit_helper_usrinfo(bool task_valid,
                                          audit_get_loginuid(current) :
                                          INVALID_UID);
      const unsigned int ses = task_valid ? audit_get_sessionid(current) :
-             (unsigned int) -1;
+             AUDIT_SID_UNSET;
 
      audit_log_format(audit_buf, " auid=%u ses=%u", auid, ses);
      audit_log_task_context(audit_buf);

include/uapi/linux/audit.h +1 -0

index 4e61a9e..04f9bd2 100644
@@ -465,6 +465,7 @@ struct audit_tty_status {
 };
 
 #define AUDIT_UID_UNSET (unsigned int)-1
+#define AUDIT_SID_UNSET ((unsigned int)-1)
 
 /* audit_rule_data supports filter rules with both integer and string
  * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and

init/init_task.c +1 -1

index 3ac6e75..c788f91 100644
@@ -119,7 +119,7 @@ struct task_struct init_task
      .thread_node    = LIST_HEAD_INIT(init_signals.thread_head),
 #ifdef CONFIG_AUDITSYSCALL
      .loginuid       = INVALID_UID,
-     .sessionid      = (unsigned int)-1,
+     .sessionid      = AUDIT_SID_UNSET,
 #endif
 #ifdef CONFIG_PERF_EVENTS
      .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),

kernel/auditsc.c +2 -3

index f3817d0..6e3ceb9 100644
@@ -2050,7 +2050,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid,
 int audit_set_loginuid(kuid_t loginuid)
 {
      struct task_struct *task = current;
-     unsigned int oldsessionid, sessionid = (unsigned int)-1;
+     unsigned int oldsessionid, sessionid = AUDIT_SID_UNSET;
      kuid_t oldloginuid;
      int rc;

@@ -2064,7 +2064,7 @@ int audit_set_loginuid(kuid_t loginuid)
      /* are we setting or clearing? */
      if (uid_valid(loginuid)) {
              sessionid = (unsigned int)atomic_inc_return(&session_id);
-             if (unlikely(sessionid == (unsigned int)-1))
+             if (unlikely(sessionid == AUDIT_SID_UNSET))
                      sessionid = (unsigned int)atomic_inc_return(&session_id);
      }
 
--

1.8.3.1

Quoted 4 lines

--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit

Signature · 2 lines

paul moore
www.paul-moore.com

Richard Guy Briggs

author

2018-05-04 20:54 UTC #8 lore raw

RFC PATCH 3/5 audit: use inline function to get audit context

In reply to Richard Guy Briggs

Recognizing that the audit context is an internal audit value, use an access function to retrieve the audit context pointer for the task rather than reaching directly into the task struct to get it.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h                | 16 ++++++++---
 include/net/xfrm.h                   |  2 +-
 kernel/audit.c                       |  4 +--
 kernel/audit_watch.c                 |  2 +-
 kernel/auditsc.c                     | 52 ++++++++++++++++++------------------
 net/bridge/netfilter/ebtables.c      |  2 +-
 net/core/dev.c                       |  2 +-
 net/netfilter/x_tables.c             |  2 +-
 net/netlabel/netlabel_user.c         |  2 +-
 security/integrity/ima/ima_api.c     |  2 +-
 security/integrity/integrity_audit.c |  2 +-
 security/lsm_audit.c                 |  2 +-
 security/selinux/hooks.c             |  4 +--
 security/selinux/selinuxfs.c         |  6 ++---
 security/selinux/ss/services.c       | 12 ++++-----
 15 files changed, 60 insertions(+), 52 deletions(-)

include/linux/audit.h +12 -4

index 5f86f7c..93e4c61 100644
@@ -235,26 +235,30 @@ extern void __audit_inode_child(struct inode *parent,
 extern void __audit_seccomp(unsigned long syscall, long signr, int code);
 extern void __audit_ptrace(struct task_struct *t);
 
+static inline struct audit_context *audit_context(struct task_struct *task)
+{
+	return task->audit_context;
+}
 static inline bool audit_dummy_context(void)
 {
-	void *p = current->audit_context;
+	void *p = audit_context(current);
 	return !p || *(int *)p;
 }
 static inline void audit_free(struct task_struct *task)
 {
-	if (unlikely(task->audit_context))
+	if (unlikely(audit_context(task)))
 		__audit_free(task);
 }
 static inline void audit_syscall_entry(int major, unsigned long a0,
 				       unsigned long a1, unsigned long a2,
 				       unsigned long a3)
 {
-	if (unlikely(current->audit_context))
+	if (unlikely(audit_context(current)))
 		__audit_syscall_entry(major, a0, a1, a2, a3);
 }
 static inline void audit_syscall_exit(void *pt_regs)
 {
-	if (unlikely(current->audit_context)) {
+	if (unlikely(audit_context(current))) {
 		int success = is_syscall_success(pt_regs);
 		long return_code = regs_return_value(pt_regs);

@@ -468,6 +472,10 @@ static inline bool audit_dummy_context(void)
 {
 	return true;
 }
+static inline struct audit_context *audit_context(struct task_struct *task)
+{
+	return NULL;
+}
 static inline struct filename *audit_reusename(const __user char *name)
 {
 	return NULL;

include/net/xfrm.h +1 -1

index fcce8ee..2788332 100644
@@ -736,7 +736,7 @@ static inline struct audit_buffer *xfrm_audit_start(const char *op)
 
 	if (audit_enabled == 0)
 		return NULL;
-	audit_buf = audit_log_start(current->audit_context, GFP_ATOMIC,
+	audit_buf = audit_log_start(audit_context(current), GFP_ATOMIC,
 				    AUDIT_MAC_IPSEC_EVENT);
 	if (audit_buf == NULL)
 		return NULL;

kernel/audit.c +2 -2

index e9f9a90..9a03603 100644
@@ -1099,7 +1099,7 @@ static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature
 
 	if (audit_enabled == AUDIT_OFF)
 		return;
-	ab = audit_log_start(current->audit_context,
+	ab = audit_log_start(audit_context(current),
 			     GFP_KERNEL, AUDIT_FEATURE_CHANGE);
 	if (!ab)
 		return;

@@ -2317,7 +2317,7 @@ void audit_log_link_denied(const char *operation)
 		return;
 
 	/* Generate AUDIT_ANOM_LINK with subject, operation, outcome. */
-	ab = audit_log_start(current->audit_context, GFP_KERNEL,
+	ab = audit_log_start(audit_context(current), GFP_KERNEL,
 			     AUDIT_ANOM_LINK);
 	if (!ab)
 		return;

kernel/audit_watch.c +1 -1

index 9eb8b35..8b596c4 100644
@@ -274,7 +274,7 @@ static void audit_update_watch(struct audit_parent *parent,
 		/* If the update involves invalidating rules, do the inode-based
 		 * filtering now, so we don't omit records. */
 		if (invalidating && !audit_dummy_context())
-			audit_filter_inodes(current, current->audit_context);
+			audit_filter_inodes(current, audit_context(current));
 
 		/* updating ino will likely change which audit_hash_list we
 		 * are on so we need a new watch for the new list */

kernel/auditsc.c +26 ~~-26~~

208 lines not on this page · raw message

net/bridge/netfilter/ebtables.c +1 -1

index 032e0fe..ff8450b 100644
@@ -1062,7 +1062,7 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
 
 #ifdef CONFIG_AUDIT
 	if (audit_enabled) {
-		audit_log(current->audit_context, GFP_KERNEL,
+		audit_log(audit_context(current), GFP_KERNEL,
 			  AUDIT_NETFILTER_CFG,
 			  "table=%s family=%u entries=%u",
 			  repl->name, AF_BRIDGE, repl->nentries);

net/core/dev.c +1 -1

index 969462e..2837086 100644
@@ -6749,7 +6749,7 @@ static int __dev_set_promiscuity(struct net_device *dev, int inc, bool notify)
 			dev->flags & IFF_PROMISC ? "entered" : "left");
 		if (audit_enabled) {
 			current_uid_gid(&uid, &gid);
-			audit_log(current->audit_context, GFP_ATOMIC,
+			audit_log(audit_context(current), GFP_ATOMIC,
 				AUDIT_ANOM_PROMISCUOUS,
 				"dev=%s prom=%d old_prom=%d auid=%u uid=%u gid=%u ses=%u",
 				dev->name, (dev->flags & IFF_PROMISC),

net/netfilter/x_tables.c +1 -1

index 71325fe..f271630 100644
@@ -1414,7 +1414,7 @@ struct xt_table_info *
 
 #ifdef CONFIG_AUDIT
 	if (audit_enabled) {
-		audit_log(current->audit_context, GFP_KERNEL,
+		audit_log(audit_context(current), GFP_KERNEL,
 			  AUDIT_NETFILTER_CFG,
 			  "table=%s family=%u entries=%u",
 			  table->name, table->af, private->number);

net/netlabel/netlabel_user.c +1 -1

index 58495f4..6cd5573 100644
@@ -104,7 +104,7 @@ struct audit_buffer *netlbl_audit_start_common(int type,
 	if (audit_enabled == 0)
 		return NULL;
 
-	audit_buf = audit_log_start(current->audit_context, GFP_ATOMIC, type);
+	audit_buf = audit_log_start(audit_context(current), GFP_ATOMIC, type);
 	if (audit_buf == NULL)
 		return NULL;

security/integrity/ima/ima_api.c +1 -1

index bf88236..a727ae0 100644
@@ -326,7 +326,7 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
 		hex_byte_pack(hash + (i * 2), iint->ima_hash->digest[i]);
 	hash[i * 2] = '\0';
 
-	ab = audit_log_start(current->audit_context, GFP_KERNEL,
+	ab = audit_log_start(audit_context(current), GFP_KERNEL,
 			     AUDIT_INTEGRITY_RULE);
 	if (!ab)
 		goto out;

security/integrity/integrity_audit.c +1 -1

index 90987d1..79adc98a 100644
@@ -38,7 +38,7 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
 	if (!integrity_audit_info && audit_info == 1)	/* Skip info messages */
 		return;
 
-	ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);
+	ab = audit_log_start(audit_context(current), GFP_KERNEL, audit_msgno);
 	audit_log_format(ab, "pid=%d uid=%u auid=%u ses=%u",
 			 task_pid_nr(current),
 			 from_kuid(&init_user_ns, current_cred()->uid),

security/lsm_audit.c +1 -1

index 67703db..ccae258 100644
@@ -447,7 +447,7 @@ void common_lsm_audit(struct common_audit_data *a,
 	if (a == NULL)
 		return;
 	/* we use GFP_ATOMIC so we won't sleep */
-	ab = audit_log_start(current->audit_context, GFP_ATOMIC | __GFP_NOWARN,
+	ab = audit_log_start(audit_context(current), GFP_ATOMIC | __GFP_NOWARN,
 			     AUDIT_AVC);
 
 	if (ab == NULL)

security/selinux/hooks.c +2 -2

index 4cafe6a..f1de97a 100644
@@ -3294,7 +3294,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
 			} else {
 				audit_size = 0;
 			}
-			ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
+			ab = audit_log_start(audit_context(current), GFP_ATOMIC, AUDIT_SELINUX_ERR);
 			audit_log_format(ab, "op=setxattr invalid_context=");
 			audit_log_n_untrustedstring(ab, value, audit_size);
 			audit_log_end(ab);

@@ -6431,7 +6431,7 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
 					audit_size = size - 1;
 				else
 					audit_size = size;
-				ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
+				ab = audit_log_start(audit_context(current), GFP_ATOMIC, AUDIT_SELINUX_ERR);
 				audit_log_format(ab, "op=fscreate invalid_context=");
 				audit_log_n_untrustedstring(ab, value, audit_size);
 				audit_log_end(ab);

security/selinux/selinuxfs.c +3 -3

index efdc633..e5f90da 100644
@@ -167,7 +167,7 @@ static ssize_t sel_write_enforce(struct file *file, const char __user *buf,
 				      NULL);
 		if (length)
 			goto out;
-		audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STATUS,
+		audit_log(audit_context(current), GFP_KERNEL, AUDIT_MAC_STATUS,
 			"enforcing=%d old_enforcing=%d auid=%u ses=%u"
 			" enabled=%d old-enabled=%d lsm=selinux res=1",
 			new_value, old_value,

@@ -303,7 +303,7 @@ static ssize_t sel_write_disable(struct file *file, const char __user *buf,
 		length = selinux_disable(fsi->state);
 		if (length)
 			goto out;
-		audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STATUS,
+		audit_log(audit_context(current), GFP_KERNEL, AUDIT_MAC_STATUS,
 			"enforcing=%d old_enforcing=%d auid=%u ses=%u"
 			" enabled=%d old-enabled=%d lsm=selinux res=1",
 			enforcing, enforcing,

@@ -581,7 +581,7 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
 	length = count;
 
 out1:
-	audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
+	audit_log(audit_context(current), GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
 		"auid=%u ses=%u lsm=selinux res=1",
 		from_kuid(&init_user_ns, audit_get_loginuid(current)),
 		audit_get_sessionid(current));

security/selinux/ss/services.c +6 -6

index 8057e19..83f40e2 100644
@@ -501,7 +501,7 @@ static void security_dump_masked_av(struct policydb *policydb,
 		goto out;
 
 	/* audit a message */
-	ab = audit_log_start(current->audit_context,
+	ab = audit_log_start(audit_context(current),
 			     GFP_ATOMIC, AUDIT_SELINUX_ERR);
 	if (!ab)
 		goto out;

@@ -743,7 +743,7 @@ static int security_validtrans_handle_fail(struct selinux_state *state,
 		goto out;
 	if (context_struct_to_string(p, tcontext, &t, &tlen))
 		goto out;
-	audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR,
+	audit_log(audit_context(current), GFP_ATOMIC, AUDIT_SELINUX_ERR,
 		  "op=security_validate_transition seresult=denied"
 		  " oldcontext=%s newcontext=%s taskcontext=%s tclass=%s",
 		  o, n, t, sym_name(p, SYM_CLASSES, tclass-1));

@@ -929,7 +929,7 @@ int security_bounded_transition(struct selinux_state *state,
 					      &old_name, &length) &&
 		    !context_struct_to_string(policydb, new_context,
 					      &new_name, &length)) {
-			audit_log(current->audit_context,
+			audit_log(audit_context(current),
 				  GFP_ATOMIC, AUDIT_SELINUX_ERR,
 				  "op=security_bounded_transition "
 				  "seresult=denied "

@@ -1586,7 +1586,7 @@ static int compute_sid_handle_invalid_context(
 		goto out;
 	if (context_struct_to_string(policydb, newcontext, &n, &nlen))
 		goto out;
-	audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR,
+	audit_log(audit_context(current), GFP_ATOMIC, AUDIT_SELINUX_ERR,
 		  "op=security_compute_sid invalid_context=%s"
 		  " scontext=%s"
 		  " tcontext=%s"

@@ -2882,7 +2882,7 @@ int security_set_bools(struct selinux_state *state, int len, int *values)
 
 	for (i = 0; i < len; i++) {
 		if (!!values[i] != policydb->bool_val_to_struct[i]->state) {
-			audit_log(current->audit_context, GFP_ATOMIC,
+			audit_log(audit_context(current), GFP_ATOMIC,
 				AUDIT_MAC_CONFIG_CHANGE,
 				"bool=%s val=%d old_val=%d auid=%u ses=%u",
 				sym_name(policydb, SYM_BOOLS, i),

@@ -3025,7 +3025,7 @@ int security_sid_mls_copy(struct selinux_state *state,
 		if (rc) {
 			if (!context_struct_to_string(policydb, &newcon, &s,
 						      &len)) {
-				audit_log(current->audit_context,
+				audit_log(audit_context(current),
 					  GFP_ATOMIC, AUDIT_SELINUX_ERR,
 					  "op=security_sid_mls_copy "
 					  "invalid_context=%s", s);

Signature · 1 line

1.8.3.1

Paul Moore

maintainer

2018-05-09 15:28 UTC #9 lore raw

Re: [PATCH ghak81 RFC V1 3/5] audit: use inline function to get audit context

Quoted 28 lines · Richard Guy Briggs

Recognizing that the audit context is an internal audit value, use an access function to retrieve the audit context pointer for the task rather than reaching directly into the task struct to get it.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h                | 16 ++++++++---
 include/net/xfrm.h                   |  2 +-
 kernel/audit.c                       |  4 +--
 kernel/audit_watch.c                 |  2 +-
 kernel/auditsc.c                     | 52 ++++++++++++++++++------------------
 net/bridge/netfilter/ebtables.c      |  2 +-
 net/core/dev.c                       |  2 +-
 net/netfilter/x_tables.c             |  2 +-
 net/netlabel/netlabel_user.c         |  2 +-
 security/integrity/ima/ima_api.c     |  2 +-
 security/integrity/integrity_audit.c |  2 +-
 security/lsm_audit.c                 |  2 +-
 security/selinux/hooks.c             |  4 +--
 security/selinux/selinuxfs.c         |  6 ++---
 security/selinux/ss/services.c       | 12 ++++-----
 15 files changed, 60 insertions(+), 52 deletions(-)

include/linux/audit.h +0 -0

index 5f86f7c..93e4c61 100644

include/linux/audit.h extern void __audit_inode_child(struct inode *parent, · line 241

Quoted 7 lines

include/linux/audit.h +4 -0

@@ -235,26 +235,30 @@ extern void __audit_inode_child(struct inode *parent,
 extern void __audit_seccomp(unsigned long syscall, long signr, int code);
 extern void __audit_ptrace(struct task_struct *t);
 
+static inline struct audit_context *audit_context(struct task_struct *task)
+{
+       return task->audit_context;
+}

Another case where I think I agree with everything here on principle, especially when one considers it in the larger context of the audit container ID work. However, I think we might be able to somply this a bit by eliminating the parameter to the new audit_context() helper and making it always reference the current task_struct. Based on this patch it would appear that this change would work for all callers except for audit_take_context() and __audit_syscall_entry(), both of which are contained within the core audit code and are enough of a special case that I think it is acceptable for them to access the context directly. I'm trying to think of reasons why a non-audit kernel subsystem would ever need to access the audit context of a process other than current and I can't think of any ... removing the task_struct pointer might help prevent mistakes/abuse in the future.

Quoted 22 lines

kernel/auditsc.c +2 -2

index 6e3ceb9..a4bbdcc 100644
@@ -836,7 +836,7 @@ static inline struct audit_context *audit_take_context(struct task_struct *tsk,
                                                      int return_valid,
                                                      long return_code)
 {
-       struct audit_context *context = tsk->audit_context;
+       struct audit_context *context = audit_context(tsk);
 
        if (!context)
                return NULL;

@@ -1510,7 +1510,7 @@ void __audit_syscall_entry(int major, unsigned long a1, unsigned long a2,
                           unsigned long a3, unsigned long a4)
 {
        struct task_struct *tsk = current;
-       struct audit_context *context = tsk->audit_context;
+       struct audit_context *context = audit_context(tsk);
        enum audit_state     state;
 
        if (!audit_enabled || !context)

Signature · 2 lines

paul moore
www.paul-moore.com

Richard Guy Briggs

author

2018-05-10 21:17 UTC #10 lore raw

Re: [PATCH ghak81 RFC V1 3/5] audit: use inline function to get audit context

Quoted 42 lines · Paul Moore

Quoted 28 lines · Richard Guy Briggs
Recognizing that the audit context is an internal audit value, use an access function to retrieve the audit context pointer for the task rather than reaching directly into the task struct to get it.
Signed-off-by Richard Guy Briggs <[email protected]>
---
 include/linux/audit.h                | 16 ++++++++---
 include/net/xfrm.h                   |  2 +-
 kernel/audit.c                       |  4 +--
 kernel/audit_watch.c                 |  2 +-
 kernel/auditsc.c                     | 52 ++++++++++++++++++------------------
 net/bridge/netfilter/ebtables.c      |  2 +-
 net/core/dev.c                       |  2 +-
 net/netfilter/x_tables.c             |  2 +-
 net/netlabel/netlabel_user.c         |  2 +-
 security/integrity/ima/ima_api.c     |  2 +-
 security/integrity/integrity_audit.c |  2 +-
 security/lsm_audit.c                 |  2 +-
 security/selinux/hooks.c             |  4 +--
 security/selinux/selinuxfs.c         |  6 ++---
 security/selinux/ss/services.c       | 12 ++++-----
 15 files changed, 60 insertions(+), 52 deletions(-)
include/linux/audit.h +0 -0
index 5f86f7c..93e4c61 100644
include/linux/audit.h extern void __audit_inode_child(struct inode *parent, · line 241
Quoted 7 lines
include/linux/audit.h +4 -0
@@ -235,26 +235,30 @@ extern void __audit_inode_child(struct inode *parent,
 extern void __audit_seccomp(unsigned long syscall, long signr, int code);
 extern void __audit_ptrace(struct task_struct *t);
 
+static inline struct audit_context *audit_context(struct task_struct *task)
+{
+       return task->audit_context;
+}
Another case where I think I agree with everything here on principle, especially when one considers it in the larger context of the audit container ID work. However, I think we might be able to somply this a bit by eliminating the parameter to the new audit_context() helper and making it always reference the current task_struct. Based on this

Quoted 8 lines

patch it would appear that this change would work for all callers except for audit_take_context() and __audit_syscall_entry(), both of which are contained within the core audit code and are enough of a special case that I think it is acceptable for them to access the context directly. I'm trying to think of reasons why a non-audit kernel subsystem would ever need to access the audit context of a process other than current and I can't think of any ... removing the task_struct pointer might help prevent mistakes/abuse in the future.

As for __audit_syscall_{entry,exit}() and audit_signal_info(), they are using current. current is assigned to local variable tsk only to be used as the LHS in assignments and for locking.

But, audit_take_context() and audit_log_exit() are both called also from __audit_free() which can have non-current handed to it by copy_process() cleaning up, while do_exit() appears to still be in current.

So, Ok, ditch the parameter to audit_context() and use local access when needed.

Quoted 21 lines

kernel/auditsc.c +2 -2

index 6e3ceb9..a4bbdcc 100644
@@ -836,7 +836,7 @@ static inline struct audit_context *audit_take_context(struct task_struct *tsk,
                                                      int return_valid,
                                                      long return_code)
 {
-       struct audit_context *context = tsk->audit_context;
+       struct audit_context *context = audit_context(tsk);
 
        if (!context)
                return NULL;

@@ -1510,7 +1510,7 @@ void __audit_syscall_entry(int major, unsigned long a1, unsigned long a2,
                           unsigned long a3, unsigned long a4)
 {
        struct task_struct *tsk = current;
-       struct audit_context *context = tsk->audit_context;
+       struct audit_context *context = audit_context(tsk);
        enum audit_state     state;

Quoted 5 lines

Quoted 1 line
kernel/auditsc.c +0 -0
        if (!audit_enabled || !context)
Signature · 2 lines
paul moore
www.paul-moore.com

- RGB

--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Richard Guy Briggs

author

2018-05-04 20:54 UTC #11 lore raw

RFC PATCH 4/5 audit: use inline function to set audit context

In reply to Richard Guy Briggs

Recognizing that the audit context is an internal audit value, use an access function to set the audit context pointer for the task rather than reaching directly into the task struct to set it.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h | 8 ++++++++
 kernel/auditsc.c      | 6 +++---
 kernel/fork.c         | 2 +-
 3 files changed, 12 insertions(+), 4 deletions(-)

include/linux/audit.h +8 -0

index 93e4c61..dba0d45 100644
@@ -235,6 +235,10 @@ extern void __audit_inode_child(struct inode *parent,
 extern void __audit_seccomp(unsigned long syscall, long signr, int code);
 extern void __audit_ptrace(struct task_struct *t);
 
+static inline void audit_set_context(struct task_struct *task, struct audit_context *ctx)
+{
+	task->audit_context = ctx;
+}
 static inline struct audit_context *audit_context(struct task_struct *task)
 {
 	return task->audit_context;

@@ -472,6 +476,10 @@ static inline bool audit_dummy_context(void)
 {
 	return true;
 }
+static inline void audit_set_context(struct task_struct *task, struct audit_context *ctx)
+{
+	task->audit_context = ctx;
+}
 static inline struct audit_context *audit_context(struct task_struct *task)
 {
 	return NULL;

kernel/auditsc.c +3 -3

index a4bbdcc..f294e4a 100644
@@ -865,7 +865,7 @@ static inline struct audit_context *audit_take_context(struct task_struct *tsk,
 		audit_filter_inodes(tsk, context);
 	}
 
-	tsk->audit_context = NULL;
+	audit_set_context(tsk, NULL);
 	return context;
 }

@@ -952,7 +952,7 @@ int audit_alloc(struct task_struct *tsk)
 	}
 	context->filterkey = key;
 
-	tsk->audit_context  = context;
+	audit_set_context(tsk, context);
 	set_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT);
 	return 0;
 }

@@ -1590,7 +1590,7 @@ void __audit_syscall_exit(int success, long return_code)
 		kfree(context->filterkey);
 		context->filterkey = NULL;
 	}
-	tsk->audit_context = context;
+	audit_set_context(tsk, context);
 }
 
 static inline void handle_one(const struct inode *inode)

kernel/fork.c +1 -1

index 242c8c9..cd18448 100644
@@ -1713,7 +1713,7 @@ static __latent_entropy struct task_struct *copy_process(
 	p->start_time = ktime_get_ns();
 	p->real_start_time = ktime_get_boot_ns();
 	p->io_context = NULL;
-	p->audit_context = NULL;
+	audit_set_context(p, NULL);
 	cgroup_fork(p);
 #ifdef CONFIG_NUMA
 	p->mempolicy = mpol_dup(p->mempolicy);

Signature · 1 line

1.8.3.1

Tobin C. Harding

2018-05-09 02:07 UTC #12 lore raw

Re: [PATCH ghak81 RFC V1 4/5] audit: use inline function to set audit context

Quoted 27 lines · Richard Guy Briggs

Recognizing that the audit context is an internal audit value, use an access function to set the audit context pointer for the task rather than reaching directly into the task struct to set it.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h | 8 ++++++++
 kernel/auditsc.c      | 6 +++---
 kernel/fork.c         | 2 +-
 3 files changed, 12 insertions(+), 4 deletions(-)

include/linux/audit.h +4 -0

index 93e4c61..dba0d45 100644
@@ -235,6 +235,10 @@ extern void __audit_inode_child(struct inode *parent,
 extern void __audit_seccomp(unsigned long syscall, long signr, int code);
 extern void __audit_ptrace(struct task_struct *t);
 
+static inline void audit_set_context(struct task_struct *task, struct audit_context *ctx)
+{
+	task->audit_context = ctx;
+}
 static inline struct audit_context *audit_context(struct task_struct *task)
 {
 	return task->audit_context;

include/linux/audit.h static inline bool audit_dummy_context(void) · line 482

Quoted 7 lines

include/linux/audit.h +4 -0

@@ -472,6 +476,10 @@ static inline bool audit_dummy_context(void)
 {
 	return true;
 }
+static inline void audit_set_context(struct task_struct *task, struct audit_context *ctx)
+{
+	task->audit_context = ctx;
+}

If audit_context is an internal audit value why do we set it when CONFIG_AUDITSYSCALL is not set?

thanks,
Tobin.

Richard Guy Briggs

author

2018-05-09 12:09 UTC #13 lore raw

Re: [PATCH ghak81 RFC V1 4/5] audit: use inline function to set audit context

Quoted 30 lines · Tobin C. Harding

Quoted 29 lines · Richard Guy Briggs

Recognizing that the audit context is an internal audit value, use an access function to set the audit context pointer for the task rather than reaching directly into the task struct to set it.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h | 8 ++++++++
 kernel/auditsc.c      | 6 +++---
 kernel/fork.c         | 2 +-
 3 files changed, 12 insertions(+), 4 deletions(-)

include/linux/audit.h +4 -0

index 93e4c61..dba0d45 100644
@@ -235,6 +235,10 @@ extern void __audit_inode_child(struct inode *parent,
 extern void __audit_seccomp(unsigned long syscall, long signr, int code);
 extern void __audit_ptrace(struct task_struct *t);
 
+static inline void audit_set_context(struct task_struct *task, struct audit_context *ctx)
+{
+	task->audit_context = ctx;
+}
 static inline struct audit_context *audit_context(struct task_struct *task)
 {
 	return task->audit_context;

@@ -472,6 +476,10 @@ static inline bool audit_dummy_context(void)
 {
 	return true;

Quoted 8 lines

include/linux/audit.h static inline bool audit_dummy_context(void) · line 482
Quoted 5 lines
include/linux/audit.h +4 -0
 }
+static inline void audit_set_context(struct task_struct *task, struct audit_context *ctx)
+{
+	task->audit_context = ctx;
+}
If audit_context is an internal audit value why do we set it when CONFIG_AUDITSYSCALL is not set?

Agreed, that is unnecessary, but harmless since it won't be called, or will be called with a value of NULL. That has been fixed in my dynamic allocation patchset since not even the audit_task_info struct is available to assign the value. It is now an empty function like the rest.

Quoted 1 line

Tobin.

- RGB

--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Richard Guy Briggs

author

2018-05-04 20:54 UTC #14 lore raw

RFC PATCH 5/5 audit: collect audit task parameters

In reply to Richard Guy Briggs

The audit-related parameters in struct task_struct should ideally be collected together and accessed through a standard audit API.

Collect the existing loginuid, sessionid and audit_context together in a new struct audit_task_info pointer called "audit" in struct task_struct.

Use kmem_cache to manage this pool of memory. Un-inline audit_free() to be able to always recover that memory.

See: https://github.com/linux-audit/audit-kernel/issues/81

Signed-off-by Richard Guy Briggs <[email protected]>

---
 MAINTAINERS                |  2 +-
 include/linux/audit.h      |  8 ++++----
 include/linux/audit_task.h | 31 +++++++++++++++++++++++++++++++
 include/linux/sched.h      |  6 ++----
 init/init_task.c           |  8 ++++++--
 kernel/auditsc.c           |  4 ++--
 6 files changed, 46 insertions(+), 13 deletions(-)
 create mode 100644 include/linux/audit_task.h

MAINTAINERS +1 -1

index 0a1410d..8c7992d 100644
@@ -2510,7 +2510,7 @@ L:	[email protected] (moderated for non-subscribers)
 W:	https://github.com/linux-audit
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git
 S:	Supported
-F:	include/linux/audit.h
+F:	include/linux/audit*.h
 F:	include/uapi/linux/audit.h
 F:	kernel/audit*

include/linux/audit.h +4 -4

index dba0d45..1324969 100644
@@ -237,11 +237,11 @@ extern void __audit_inode_child(struct inode *parent,
 
 static inline void audit_set_context(struct task_struct *task, struct audit_context *ctx)
 {
-	task->audit_context = ctx;
+	task->audit.ctx = ctx;
 }
 static inline struct audit_context *audit_context(struct task_struct *task)
 {
-	return task->audit_context;
+	return task->audit.ctx;
 }
 static inline bool audit_dummy_context(void)
 {

@@ -330,12 +330,12 @@ extern int auditsc_get_stamp(struct audit_context *ctx,
 
 static inline kuid_t audit_get_loginuid(struct task_struct *tsk)
 {
-	return tsk->loginuid;
+	return tsk->audit.loginuid;
 }
 
 static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 {
-	return tsk->sessionid;
+	return tsk->audit.sessionid;
 }
 
 extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);

include/linux/audit_task.h new file +31 -0

new file mode 100644
index 0000000..d4b3a20
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* audit_task.h -- definition of audit_task_info structure
+ *
+ * Copyright 2018 Red Hat Inc., Raleigh, North Carolina.
+ * All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Written by Richard Guy Briggs <[email protected]>
+ *
+ */
+
+#ifndef _LINUX_AUDIT_TASK_H_
+#define _LINUX_AUDIT_TASK_H_
+
+struct audit_context;
+struct audit_task_info {
+	kuid_t			loginuid;
+	unsigned int		sessionid;
+	struct audit_context	*ctx;
+};
+
+#endif

include/linux/sched.h +2 -4

index b3d697f..b58eca0 100644
@@ -27,9 +27,9 @@
 #include <linux/signal_types.h>
 #include <linux/mm_types_task.h>
 #include <linux/task_io_accounting.h>
+#include <linux/audit_task.h>
 
 /* task_struct member predeclarations (sorted alphabetically): */
-struct audit_context;
 struct backing_dev_info;
 struct bio_list;
 struct blk_plug;

@@ -832,10 +832,8 @@ struct task_struct {
 
 	struct callback_head		*task_works;
 
-	struct audit_context		*audit_context;
 #ifdef CONFIG_AUDITSYSCALL
-	kuid_t				loginuid;
-	unsigned int			sessionid;
+	struct audit_task_info		audit;
 #endif
 	struct seccomp			seccomp;

init/init_task.c +6 -2

index c788f91..d33260d 100644
@@ -9,6 +9,7 @@
 #include <linux/init.h>
 #include <linux/fs.h>
 #include <linux/mm.h>
+#include <linux/audit.h>
 
 #include <asm/pgtable.h>
 #include <linux/uaccess.h>

@@ -118,8 +119,11 @@ struct task_struct init_task
 	.thread_group	= LIST_HEAD_INIT(init_task.thread_group),
 	.thread_node	= LIST_HEAD_INIT(init_signals.thread_head),
 #ifdef CONFIG_AUDITSYSCALL
-	.loginuid	= INVALID_UID,
-	.sessionid	= AUDIT_SID_UNSET,
+	.audit		= {
+		.loginuid	= INVALID_UID,
+		.sessionid	= AUDIT_SID_UNSET,
+		.ctx		= NULL,
+	},
 #endif
 #ifdef CONFIG_PERF_EVENTS
 	.perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),

kernel/auditsc.c +2 -2

index f294e4a..b5d8bff 100644
@@ -2068,8 +2068,8 @@ int audit_set_loginuid(kuid_t loginuid)
 			sessionid = (unsigned int)atomic_inc_return(&session_id);
 	}
 
-	task->sessionid = sessionid;
-	task->loginuid = loginuid;
+	task->audit.sessionid = sessionid;
+	task->audit.loginuid = loginuid;
 out:
 	audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid, sessionid, rc);
 	return rc;

Signature · 1 line

1.8.3.1

Paul Moore

maintainer

2018-05-09 15:46 UTC #15 lore raw

Re: [PATCH ghak81 RFC V1 5/5] audit: collect audit task parameters

Quoted 121 lines · Richard Guy Briggs

The audit-related parameters in struct task_struct should ideally be collected together and accessed through a standard audit API.

Collect the existing loginuid, sessionid and audit_context together in a new struct audit_task_info pointer called "audit" in struct task_struct.

Use kmem_cache to manage this pool of memory. Un-inline audit_free() to be able to always recover that memory.

See: https://github.com/linux-audit/audit-kernel/issues/81

Signed-off-by Richard Guy Briggs <[email protected]>

---
 MAINTAINERS                |  2 +-
 include/linux/audit.h      |  8 ++++----
 include/linux/audit_task.h | 31 +++++++++++++++++++++++++++++++
 include/linux/sched.h      |  6 ++----
 init/init_task.c           |  8 ++++++--
 kernel/auditsc.c           |  4 ++--
 6 files changed, 46 insertions(+), 13 deletions(-)
 create mode 100644 include/linux/audit_task.h

MAINTAINERS +1 -1

index 0a1410d..8c7992d 100644
@@ -2510,7 +2510,7 @@ L:        [email protected] (moderated for non-subscribers)
 W:     https://github.com/linux-audit
 T:     git git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git
 S:     Supported
-F:     include/linux/audit.h
+F:     include/linux/audit*.h
 F:     include/uapi/linux/audit.h
 F:     kernel/audit*

include/linux/audit.h +4 -4

index dba0d45..1324969 100644
@@ -237,11 +237,11 @@ extern void __audit_inode_child(struct inode *parent,
 
 static inline void audit_set_context(struct task_struct *task, struct audit_context *ctx)
 {
-       task->audit_context = ctx;
+       task->audit.ctx = ctx;
 }
 static inline struct audit_context *audit_context(struct task_struct *task)
 {
-       return task->audit_context;
+       return task->audit.ctx;
 }
 static inline bool audit_dummy_context(void)
 {

@@ -330,12 +330,12 @@ extern int auditsc_get_stamp(struct audit_context *ctx,
 
 static inline kuid_t audit_get_loginuid(struct task_struct *tsk)
 {
-       return tsk->loginuid;
+       return tsk->audit.loginuid;
 }
 
 static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 {
-       return tsk->sessionid;
+       return tsk->audit.sessionid;
 }
 
 extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);

include/linux/audit_task.h new file +31 -0

new file mode 100644
index 0000000..d4b3a20
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* audit_task.h -- definition of audit_task_info structure
+ *
+ * Copyright 2018 Red Hat Inc., Raleigh, North Carolina.
+ * All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Written by Richard Guy Briggs <[email protected]>
+ *
+ */
+
+#ifndef _LINUX_AUDIT_TASK_H_
+#define _LINUX_AUDIT_TASK_H_
+
+struct audit_context;
+struct audit_task_info {
+       kuid_t                  loginuid;
+       unsigned int            sessionid;
+       struct audit_context    *ctx;
+};
+
+#endif

include/linux/sched.h +1 -1

index b3d697f..b58eca0 100644
@@ -27,9 +27,9 @@
 #include <linux/signal_types.h>
 #include <linux/mm_types_task.h>
 #include <linux/task_io_accounting.h>
+#include <linux/audit_task.h>
 
 /* task_struct member predeclarations (sorted alphabetically): */
-struct audit_context;
 struct backing_dev_info;
 struct bio_list;
 struct blk_plug;

include/linux/sched.h struct task_struct { · line 837

Quoted 9 lines

include/linux/sched.h +1 -3

@@ -832,10 +832,8 @@ struct task_struct {
 
        struct callback_head            *task_works;
 
-       struct audit_context            *audit_context;
 #ifdef CONFIG_AUDITSYSCALL
-       kuid_t                          loginuid;
-       unsigned int                    sessionid;
+       struct audit_task_info          audit;
 #endif

Considering that the audit_context pointer is now in the audit_task_info struct, should the audit_task_info struct be placed outside the CONFIG_AUDITSYSCALL protections? Or rather, shouldn't the CONFIG_AUDITSYSCALL protections be moved inside audit_task_info or removed entirely?

Quoted 43 lines

init/init_task.c +6 -2

index c788f91..d33260d 100644
@@ -9,6 +9,7 @@
 #include <linux/init.h>
 #include <linux/fs.h>
 #include <linux/mm.h>
+#include <linux/audit.h>
 
 #include <asm/pgtable.h>
 #include <linux/uaccess.h>

@@ -118,8 +119,11 @@ struct task_struct init_task
        .thread_group   = LIST_HEAD_INIT(init_task.thread_group),
        .thread_node    = LIST_HEAD_INIT(init_signals.thread_head),
 #ifdef CONFIG_AUDITSYSCALL
-       .loginuid       = INVALID_UID,
-       .sessionid      = AUDIT_SID_UNSET,
+       .audit          = {
+               .loginuid       = INVALID_UID,
+               .sessionid      = AUDIT_SID_UNSET,
+               .ctx            = NULL,
+       },
 #endif
 #ifdef CONFIG_PERF_EVENTS
        .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),

kernel/auditsc.c +2 -3

index f294e4a..b5d8bff 100644
@@ -2068,8 +2068,8 @@ int audit_set_loginuid(kuid_t loginuid)
                        sessionid = (unsigned int)atomic_inc_return(&session_id);
        }
 
-       task->sessionid = sessionid;
-       task->loginuid = loginuid;
+       task->audit.sessionid = sessionid;
+       task->audit.loginuid = loginuid;
 out:
        audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid, sessionid, rc);
        return rc;
--

1.8.3.1

Signature · 2 lines

paul moore
www.paul-moore.com

Richard Guy Briggs

author

2018-05-10 21:26 UTC #16 lore raw

Re: [PATCH ghak81 RFC V1 5/5] audit: collect audit task parameters

Quoted 125 lines · Paul Moore

Quoted 124 lines · Richard Guy Briggs

The audit-related parameters in struct task_struct should ideally be collected together and accessed through a standard audit API.

Collect the existing loginuid, sessionid and audit_context together in a new struct audit_task_info pointer called "audit" in struct task_struct.

Use kmem_cache to manage this pool of memory. Un-inline audit_free() to be able to always recover that memory.

See: https://github.com/linux-audit/audit-kernel/issues/81

Signed-off-by Richard Guy Briggs <[email protected]>

---
 MAINTAINERS                |  2 +-
 include/linux/audit.h      |  8 ++++----
 include/linux/audit_task.h | 31 +++++++++++++++++++++++++++++++
 include/linux/sched.h      |  6 ++----
 init/init_task.c           |  8 ++++++--
 kernel/auditsc.c           |  4 ++--
 6 files changed, 46 insertions(+), 13 deletions(-)
 create mode 100644 include/linux/audit_task.h

MAINTAINERS +1 -1

index 0a1410d..8c7992d 100644
@@ -2510,7 +2510,7 @@ L:        [email protected] (moderated for non-subscribers)
 W:     https://github.com/linux-audit
 T:     git git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git
 S:     Supported
-F:     include/linux/audit.h
+F:     include/linux/audit*.h
 F:     include/uapi/linux/audit.h
 F:     kernel/audit*

include/linux/audit.h +4 -4

index dba0d45..1324969 100644
@@ -237,11 +237,11 @@ extern void __audit_inode_child(struct inode *parent,
 
 static inline void audit_set_context(struct task_struct *task, struct audit_context *ctx)
 {
-       task->audit_context = ctx;
+       task->audit.ctx = ctx;
 }
 static inline struct audit_context *audit_context(struct task_struct *task)
 {
-       return task->audit_context;
+       return task->audit.ctx;
 }
 static inline bool audit_dummy_context(void)
 {

@@ -330,12 +330,12 @@ extern int auditsc_get_stamp(struct audit_context *ctx,
 
 static inline kuid_t audit_get_loginuid(struct task_struct *tsk)
 {
-       return tsk->loginuid;
+       return tsk->audit.loginuid;
 }
 
 static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 {
-       return tsk->sessionid;
+       return tsk->audit.sessionid;
 }
 
 extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);

include/linux/audit_task.h new file +31 -0

new file mode 100644
index 0000000..d4b3a20
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* audit_task.h -- definition of audit_task_info structure
+ *
+ * Copyright 2018 Red Hat Inc., Raleigh, North Carolina.
+ * All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Written by Richard Guy Briggs <[email protected]>
+ *
+ */
+
+#ifndef _LINUX_AUDIT_TASK_H_
+#define _LINUX_AUDIT_TASK_H_
+
+struct audit_context;
+struct audit_task_info {
+       kuid_t                  loginuid;
+       unsigned int            sessionid;
+       struct audit_context    *ctx;
+};
+
+#endif

include/linux/sched.h +1 -1

index b3d697f..b58eca0 100644
@@ -27,9 +27,9 @@
 #include <linux/signal_types.h>
 #include <linux/mm_types_task.h>
 #include <linux/task_io_accounting.h>
+#include <linux/audit_task.h>
 
 /* task_struct member predeclarations (sorted alphabetically): */
-struct audit_context;
 struct backing_dev_info;
 struct bio_list;
 struct blk_plug;

@@ -832,10 +832,8 @@ struct task_struct {
 
        struct callback_head            *task_works;

Quoted 12 lines

include/linux/sched.h struct task_struct { · line 837
Quoted 6 lines
include/linux/sched.h +1 -3
-       struct audit_context            *audit_context;
 #ifdef CONFIG_AUDITSYSCALL
-       kuid_t                          loginuid;
-       unsigned int                    sessionid;
+       struct audit_task_info          audit;
 #endif
Considering that the audit_context pointer is now in the audit_task_info struct, should the audit_task_info struct be placed outside the CONFIG_AUDITSYSCALL protections? Or rather, shouldn't the CONFIG_AUDITSYSCALL protections be moved inside audit_task_info or removed entirely?

Well, I wondered about that anyways. audit_context is only meaningful in CONFIG_AUDIT_SYSCALL, and loginuid and sessionid were already there, so the whole thing should be inside, but given that CONFIG_AUDIT_SYSCALL is forced on when CONFIG_AUDIT is set I don't see that it matters. Perhaps CONFIG_AUDIT_SYSCALL should be ripped out completely and the code flattenned to the CONFIG_AUDIT case.

I see your point though, moving CONFIG_AUDIT_SYSCALL protections to within the audit_task_info struct definition makes more sense than this above.

Quoted 34 lines

init/init_task.c +6 -2

index c788f91..d33260d 100644
@@ -9,6 +9,7 @@
 #include <linux/init.h>
 #include <linux/fs.h>
 #include <linux/mm.h>
+#include <linux/audit.h>
 
 #include <asm/pgtable.h>
 #include <linux/uaccess.h>

@@ -118,8 +119,11 @@ struct task_struct init_task
        .thread_group   = LIST_HEAD_INIT(init_task.thread_group),
        .thread_node    = LIST_HEAD_INIT(init_signals.thread_head),
 #ifdef CONFIG_AUDITSYSCALL
-       .loginuid       = INVALID_UID,
-       .sessionid      = AUDIT_SID_UNSET,
+       .audit          = {
+               .loginuid       = INVALID_UID,
+               .sessionid      = AUDIT_SID_UNSET,
+               .ctx            = NULL,
+       },
 #endif
 #ifdef CONFIG_PERF_EVENTS
        .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),

kernel/auditsc.c +0 -0

index f294e4a..b5d8bff 100644
@@ -2068,8 +2068,8 @@ int audit_set_loginuid(kuid_t loginuid)
                        sessionid = (unsigned int)atomic_inc_return(&session_id);
        }

Quoted 13 lines

Quoted 9 lines

kernel/auditsc.c +2 -3

-       task->sessionid = sessionid;
-       task->loginuid = loginuid;
+       task->audit.sessionid = sessionid;
+       task->audit.loginuid = loginuid;
 out:
        audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid, sessionid, rc);
        return rc;
--

1.8.3.1

Signature · 2 lines

paul moore
www.paul-moore.com

- RGB

--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Paul Moore

maintainer

2018-05-09 15:53 UTC #17 lore raw

In reply to Richard Guy Briggs

Quoted 6 lines · Richard Guy Briggs

Group the audit parameters for each task into one structure. In particular, remove the loginuid and sessionid values and the audit context pointer from the task structure, replacing them with an audit task information structure to contain them. Use access functions to access audit values.

Quoted 8 lines

Note: Use static allocation of the audit task information structure initially. Dynamic allocation was considered and attempted, but isn't ready yet. Static allocation has the limitation that future audit task information structure changes would cause a visible change to the rest of the kernel, whereas dynamic allocation would mostly hide any future changes.
The first four access normalization patches could stand alone.

I agree that the first four patches have some standalone value, and since we are currently at -rc4, did you want to post another patchset of just those four patches with feedback incorporated? I imagine that should be quick work, and that way they aren't help up with any problems/discussion regarding the take_struct changes.

Quoted 36 lines

Passes audit-testsuite.

 MAINTAINERS                          |  2 +-
 include/linux/audit.h                | 30 ++++++++++---
 include/linux/audit_task.h           | 31 ++++++++++++++
 include/linux/sched.h                |  6 +--
 include/net/xfrm.h                   |  4 +-
 include/uapi/linux/audit.h           |  1 +
 init/init_task.c                     |  8 +++-
 kernel/audit.c                       |  4 +-
 kernel/audit_watch.c                 |  2 +-
 kernel/auditsc.c                     | 82 ++++++++++++++++++------------------
 kernel/fork.c                        |  2 +-
 net/bridge/netfilter/ebtables.c      |  2 +-
 net/core/dev.c                       |  2 +-
 net/netfilter/x_tables.c             |  2 +-
 net/netlabel/netlabel_user.c         |  2 +-
 security/integrity/ima/ima_api.c     |  2 +-
 security/integrity/integrity_audit.c |  2 +-
 security/lsm_audit.c                 |  2 +-
 security/selinux/hooks.c             |  4 +-
 security/selinux/selinuxfs.c         |  6 +--
 security/selinux/ss/services.c       | 12 +++---
 21 files changed, 129 insertions(+), 79 deletions(-)
 create mode 100644 include/linux/audit_task.h

--
1.8.3.1

Signature · 2 lines

paul moore
www.paul-moore.com