Chris Wright [off-list ref] wrote:

* Chris Wright (chrisw@sous-sol.org) wrote:

quoted

* Ingo Oeser (netdev@axxeo.de) wrote:

quoted

Hi Chris,

Andrew Morton wrote:

quoted

Ingo Oeser [off-list ref] wrote:

quoted

 -int scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm)
 -{
 -	struct task_struct *p = current;
 -	scm->creds = (struct ucred) {
 -		.uid = p->uid,
 -		.gid = p->gid,
 -		.pid = p->tgid
 -	};
 -	scm->fp = NULL;
 -	scm->sid = security_sk_sid(sock->sk, NULL, 0);
 -	scm->seq = 0;
 -	if (msg->msg_controllen <= 0)
 -		return 0;
 -	return __scm_send(sock, msg, scm);
 -}

It's worth noting that scm_send() will call security_sk_sid() even if
(msg->msg_controllen <= 0).

Chris, do you know if this is needed in this case?

This whole thing is looking broken.  I'm still trying to find the original
patch which caused the series of broken patches on top.

OK, it starts here from Catherine's patch:

include/net/scm.h::scm_recv()
+	if (test_bit(SOCK_PASSSEC, &sock->flags)) {
+		err = security_sid_to_context(scm->sid, &scontext, &scontext_len);
+		if (!err)
+ 			put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, scontext_len, scontext);
+ }

Catherine, the security_sid_to_context() is a raw SELinux function which
crept into core code and should not have been there.  The fallout fixes
included conditionally exporting security_sid_to_context, and finally
scm_send/recv unlining.

Yes.  So we're OK up the uninlining, right?

 The end result in -mm looks broken to me.
Specifically, it now does:

	ucred->uid = tsk->uid;
	ucred->gid = tsk->gid;
	ucred->pid = tsk->tgid;
	scm->fp = NULL;
	scm->seq = 0;
	if (msg->msg_controllen <= 0)
		return 0;

	scm->sid = security_sk_sid(sock->sk, NULL, 0);

The point of Catherine's original patch was to make sure there's always
a security identifier associated with AF_UNIX messages.  So receiver
can always check it (same as having credentials even w/out sender
control message passing them).  Now we will have garbage for sid.

This answers the question I've been asking all and sundry for a week, thanks ;)

So:

- scm-fold-__scm_send-into-scm_send.patch is OK

- scm_send-speedup.patch is wrong

- Catherine's patch introduces a possibly-significant performance
  problem: we're now calling the expensive-on-SELinux security_sk_sid()
  more frequently than we used to.

- That "initialise scm->creds via a temporary struct" trick still
  generates bad code.


I actually have enough to be going on with here - I'll drop it all.