linux-kernel - Re: [PATCH 3/5] audit: store audit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20131230175103.GC2457@redhat.com>
Date:	Mon, 30 Dec 2013 18:51:03 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Richard Guy Briggs <rgb@...hat.com>
Cc:	linux-audit@...hat.com, linux-kernel@...r.kernel.org,
	Eric Paris <eparis@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>
Subject: Re: [PATCH 3/5] audit: store audit_pid as a struct pid pointer

On 12/23, Richard Guy Briggs wrote:
>
> - PID will be reported in the relevant querying PID namespace.
>
> - Refuse to change the current audit_pid if the new value does not
>   point to an existing process.
>
> - Refuse to set the current audit_pid if the new value is not its own PID
>   (unless it is being unset).

I started to read the changelog only after I failed to understand the
patch. And it looks confusing too.

"Refuse to set the current audit_pid if the new value is not its own PID",
OK, but if the new value is the caller's pid then it should obviously
point to the existing process, current?

> - Convert audit_pid into the initial pid namespace for reports

I can't apply this patch (and the previous one) due to multiple rejects,
I guess it depends on other changes?

In any case, this patch looks wrong.

> @@ -412,9 +412,11 @@ static void kauditd_send_skb(struct sk_buff *skb)
>  		BUG_ON(err != -ECONNREFUSED); /* Shouldn't happen */
>  		if (audit_pid) {
>  			pr_err("audit: *NO* daemon at audit_pid=%d\n",
> -				audit_pid);
> +				pid_nr(audit_pid));
>  			audit_log_lost("auditd disappeared\n");
> -			audit_pid = 0;
> +			put_pid(audit_pid);

But this can actually free this pid. Why it is safe to use it elsewhere?

> +			audit_pid = NULL;

This also means that every "if (audit_pid)" is racy.

> @@ -814,12 +816,26 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>  				return err;
>  		}
>  		if (s.mask & AUDIT_STATUS_PID) {
> -			int new_pid = s.pid;
> -
> -			if ((!new_pid) && (task_tgid_vnr(current) != audit_pid))
> +			struct pid *new_pid = find_get_pid(s.pid);
> +			if (s.pid && !new_pid)
> +				return -ESRCH;

can't understand... find_get_pid(s.pid) is pointless if s.pid == 0 ?

			struct pid *new_pid = NULL;

			if (new_pid) {
				new_pid = find_get_pid(s.pid);
				if (!new_pid)
					return -ESRCH;
			}

looks more understandable.

> +			/* check that non-zero pid it is trying to set is its
> +			 * own*/
> +			if (s.pid && (s.pid != task_pid_vnr(current)))

again, this looks a bit confusing and suboptimal. Imho it would be better
to add

			if (new_pid != task_tgid(current))

into the "if (s.pid)" above. Hmm, actually task_pid_vnr() above looks
simply wrong, we need tgid or I am totally confused.

OTOH, if we require s.pid == task_pid_vnr(current), then why do we need
find_pid() at all?

> +				return -EPERM;

this lacks put_pid(new_pid).

> +			/* check that it isn't orphaning another process */
> +			if ((!new_pid) &&
> +			    (task_tgid_vnr(current) != pid_vnr(audit_pid)))
>  				return -EACCES;

and this can go into the "else" branch.

And I can't understand the "it isn't orphaning another process" logic...

OK, what if s.pid == 0 and audit_pid == NULL, should we fail in this case?

And I do not see how this matches "Refuse to set the current audit_pid
if the new value is not its own PID" from the changelog.

> +
>  			audit_pid = new_pid;

Another leak, it seems.

> @@ -1331,7 +1347,8 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
>  		return NULL;
>
>  	if (gfp_mask & __GFP_WAIT) {
> -		if (audit_pid && audit_pid == current->pid)
> +		if (pid_nr(audit_pid) == task_pid_nr(current))

So is this audit_pid tid or tgid? Its usage looks totally confusing.

Anyway,

		if (audit_pid == task_pid(current))

(or probably task_tgid) looks much better.

> +		//if (pid_task(audit_pid, PIDTYPE_PID) == current)

in this case you need to update Documentation/CodingStyle ;)

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/