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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
