bsdacct: switch credentials for writing to the accounting file

PATCH Superseded Michal Schmidt 2 participants 4 messages Aug 19, 2009 – Aug 20, 2009

Share & tools

Michal Schmidt

author

2009-08-19 16:33 UTC #0 lore raw

PATCH 1/1 bsdacct: switch credentials for writing to the accounting file

When process accounting is enabled, every exiting process writes a log to the account file. In addition, every once in a while one of the exiting processes checks whether there's enough free space for the log.

SELinux policy may or may not allow the exiting process to stat the fs. So unsuspecting processes start generating AVC denials just because someone enabled process accounting.

For these filesystem operations, the exiting process's credentials should be temporarily switched to that of the process which enabled accounting, because it's really that process who wanted to have the accounting information logged.

Signed-off-by Michal Schmidt <[email protected]>

---

 kernel/acct.c |   25 ++++++++++++++++++++-----
 1 files changed, 20 insertions(+), 5 deletions(-)

kernel/acct.c +20 -5

index 9f33910..54956cc 100644
@@ -76,7 +76,7 @@ int acct_parm[3] = {4, 2, 30};
  * External references and all of the globals.
  */
 static void do_acct_process(struct bsd_acct_struct *acct,
-		struct pid_namespace *ns, struct file *);
+		struct pid_namespace *ns, struct file *, const struct cred *);
 
 /*
  * This structure is used so that all the data protected by lock

@@ -90,6 +90,7 @@ struct bsd_acct_struct {
 	struct pid_namespace	*ns;
 	struct timer_list	timer;
 	struct list_head	list;
+	const struct cred	*cred;
 };
 
 static DEFINE_SPINLOCK(acct_lock);

@@ -181,20 +182,24 @@ static void acct_file_reopen(struct bsd_acct_struct *acct, struct file *file,
 {
 	struct file *old_acct = NULL;
 	struct pid_namespace *old_ns = NULL;
+	const struct cred *old_cred = NULL;
 
 	if (acct->file) {
 		old_acct = acct->file;
 		old_ns = acct->ns;
+		old_cred = acct->cred;
 		del_timer(&acct->timer);
 		acct->active = 0;
 		acct->needcheck = 0;
 		acct->file = NULL;
 		acct->ns = NULL;
+		acct->cred = NULL;
 		list_del(&acct->list);
 	}
 	if (file) {
 		acct->file = file;
 		acct->ns = ns;
+		acct->cred = get_current_cred();
 		acct->needcheck = 0;
 		acct->active = 1;
 		list_add(&acct->list, &acct_list);

@@ -206,7 +211,8 @@ static void acct_file_reopen(struct bsd_acct_struct *acct, struct file *file,
 	if (old_acct) {
 		mnt_unpin(old_acct->f_path.mnt);
 		spin_unlock(&acct_lock);
-		do_acct_process(acct, old_ns, old_acct);
+		do_acct_process(acct, old_ns, old_acct, old_cred);
+		put_cred(old_cred);
 		filp_close(old_acct, NULL);
 		spin_lock(&acct_lock);
 	}

@@ -481,7 +487,7 @@ static u32 encode_float(u64 value)
  *  do_acct_process does all actual work. Caller holds the reference to file.
  */
 static void do_acct_process(struct bsd_acct_struct *acct,
-		struct pid_namespace *ns, struct file *file)
+	struct pid_namespace *ns, struct file *file, const struct cred *cred)
 {
 	struct pacct_struct *pacct = &current->signal->pacct;
 	acct_t ac;

@@ -491,13 +497,17 @@ static void do_acct_process(struct bsd_acct_struct *acct,
 	u64 run_time;
 	struct timespec uptime;
 	struct tty_struct *tty;
+	const struct cred *orig_cred;
+
+	/* Perform file operations on behalf of whoever enabled accounting */
+	orig_cred = override_creds(cred);
 
 	/*
 	 * First check to see if there is enough free_space to continue
 	 * the process accounting system.
 	 */
 	if (!check_free_space(acct, file))
-		return;
+		goto out;
 
 	/*
 	 * Fill the accounting struct with the needed info as recorded

@@ -578,6 +588,8 @@ static void do_acct_process(struct bsd_acct_struct *acct,
 			       sizeof(acct_t), &file->f_pos);
 	current->signal->rlim[RLIMIT_FSIZE].rlim_cur = flim;
 	set_fs(fs);
+out:
+	revert_creds(orig_cred);
 }
 
 /**

@@ -636,6 +648,7 @@ static void acct_process_in_ns(struct pid_namespace *ns)
 {
 	struct file *file = NULL;
 	struct bsd_acct_struct *acct;
+	const struct cred *cred;
 
 	acct = ns->bacct;
 	/*

@@ -651,9 +664,11 @@ static void acct_process_in_ns(struct pid_namespace *ns)
 		return;
 	}
 	get_file(file);
+	cred = get_cred(acct->cred);
 	spin_unlock(&acct_lock);
 
-	do_acct_process(acct, ns, file);
+	do_acct_process(acct, ns, file, cred);
+	put_cred(cred);
 	fput(file);
 }

David Howells

2009-08-20 10:02 UTC #1 lore raw

Quoted 8 lines · Michal Schmidt

When process accounting is enabled, every exiting process writes a log to the account file. In addition, every once in a while one of the exiting processes checks whether there's enough free space for the log.
SELinux policy may or may not allow the exiting process to stat the fs. So unsuspecting processes start generating AVC denials just because someone enabled process accounting.

Quoted 6 lines

For these filesystem operations, the exiting process's credentials should be temporarily switched to that of the process which enabled accounting, because it's really that process who wanted to have the accounting information logged.
Signed-off-by Michal Schmidt <[email protected]>

Do you really need to keep creds in acct? Can you used acct->file->f_cred instead? Those are the credentials of the process that opened the file, so acct->cred may be redundant.

Other than that, it looks reasonable.

David

Michal Schmidt

author

2009-08-20 13:44 UTC #2 lore raw

PATCH v2 1/1 bsdacct: switch credentials for writing to the accounting file

SELinux policy may or may not allow the exiting process to stat the fs. So unsuspecting processes start generating AVC denials just because someone enabled process accounting.

Signed-off-by Michal Schmidt <[email protected]>

---

Dne Thu, 20 Aug 2009 11:02:06 +0100 David Howells napsal:

Quoted 5 lines

Do you really need to keep creds in acct? Can you used acct->file->f_cred instead? Those are the credentials of the process that opened the file, so acct->cred may be redundant.
Other than that, it looks reasonable.

You're right, I didn't realize that. That makes the patch much simpler.

 kernel/acct.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

kernel/acct.c +7 -1

index 9f33910..9a4715a 100644
@@ -491,13 +491,17 @@ static void do_acct_process(struct bsd_acct_struct *acct,
 	u64 run_time;
 	struct timespec uptime;
 	struct tty_struct *tty;
+	const struct cred *orig_cred;
+
+	/* Perform file operations on behalf of whoever enabled accounting */
+	orig_cred = override_creds(file->f_cred);
 
 	/*
 	 * First check to see if there is enough free_space to continue
 	 * the process accounting system.
 	 */
 	if (!check_free_space(acct, file))
-		return;
+		goto out;
 
 	/*
 	 * Fill the accounting struct with the needed info as recorded

@@ -578,6 +582,8 @@ static void do_acct_process(struct bsd_acct_struct *acct,
 			       sizeof(acct_t), &file->f_pos);
 	current->signal->rlim[RLIMIT_FSIZE].rlim_cur = flim;
 	set_fs(fs);
+out:
+	revert_creds(orig_cred);
 }
 
 /**

Signature · 1 line

1.6.2.5

David Howells

Acked-by on v2 1/1

2009-08-20 14:04 UTC #3 lore raw

Quoted 8 lines · Michal Schmidt

When process accounting is enabled, every exiting process writes a log to the account file. In addition, every once in a while one of the exiting processes checks whether there's enough free space for the log.
SELinux policy may or may not allow the exiting process to stat the fs. So unsuspecting processes start generating AVC denials just because someone enabled process accounting.

Quoted 6 lines

For these filesystem operations, the exiting process's credentials should be temporarily switched to that of the process which enabled accounting, because it's really that process who wanted to have the accounting information logged.
Signed-off-by Michal Schmidt <[email protected]>

Acked-by David Howells <[email protected]>