[PATCH] Allow either tid or pid in SCM_CREDENTIALS struct ucred

[PATCH] Allow either tid or pid in SCM_CREDENTIALS struct ucred

[Jeremy Fitzhardinge]

Andrew,

Could you stick this in -mm and see if anyone complains?  It fixes an
apparent bug in the validation of the SCM_CREDENTIALS structure in a
unix-domain socket sendmsg().

I found this because with Valgrind, the sendmsg call is being done in a
different thread from the one which did a getpid() to fill out the
SCM_CREDENTIALS structure, which causes the kernel to fail the sendmsg
with EPERM.  In the general case, this would cause a multithreaded
program sending messages with SCM_CREDENTIALS to appear schizophrenic to
a recipient, because every message would have a different pid depending
on which thread happened to send it.

If you use SCM_CREDENTIALS with a unix domain socket, and you're
non-root, then the kernel double-checks the values you supply for pid,
uid and gid in struct ucred.  In the case of uid or gid, it allows any
of effective, saved or real uid/gid.  In the case of pid, it only allows
current->pid, which is actually the tid.

This patch also makes it accept tgid in the SCM_CREDENTIALS pid field. 
That is, a threaded program can either supply the ID of the whole
process (tgid) or a particular thread (pid).  

Thanks,
	J

 net/core/scm.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

diff -puN net/core/scm.c~scm_allow_tgid net/core/scm.c
--- local-2.6/net/core/scm.c~scm_allow_tgid	2003-08-20 19:52:40.000000000 -0700
+++ local-2.6-jeremy/net/core/scm.c	2003-08-21 00:28:10.295629745 -0700
@@ -41,7 +41,8 @@
 
 static __inline__ int scm_check_creds(struct ucred *creds)
 {
-	if ((creds->pid == current->pid || capable(CAP_SYS_ADMIN)) &&
+	if (((creds->pid == current->pid || creds->pid == current->tgid) ||
+	     capable(CAP_SYS_ADMIN)) &&
 	    ((creds->uid == current->uid || creds->uid == current->euid ||
 	      creds->uid == current->suid) || capable(CAP_SETUID)) &&
 	    ((creds->gid == current->gid || creds->gid == current->egid ||

_

Re: [PATCH] Allow either tid or pid in SCM_CREDENTIALS struct ucred

[Ulrich Drepper]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jeremy Fitzhardinge wrote:

> This patch also makes it accept tgid in the SCM_CREDENTIALS pid field. 
> That is, a threaded program can either supply the ID of the whole
> process (tgid) or a particular thread (pid).  

I don't think ->pid should be tested.  Just replace it with ->tgid.
It's really not intended for the user to have any contact with the TID
(i.e., ->pid).  This is how it's done in other place.  What this shows
is that more searches for ->pid are needed which need to be replaced
with ->tgid.

- -- 
- --------------.                        ,-.            444 Castro Street
Ulrich Drepper \    ,-----------------'   \ Mountain View, CA 94041 USA
Red Hat         `--' drepper at redhat.com `---------------------------
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (GNU/Linux)

iD8DBQE/Rkzk2ijCOnn/RHQRAi7PAKCT+l2NCmoDzpHgq/yqFcqXmBArKQCgkxzW
wy2BYybK6yXrJRpNd6957tA=
=IW8f
-----END PGP SIGNATURE-----

Re: [PATCH] Allow either tid or pid in SCM_CREDENTIALS struct ucred

[Trond Myklebust]

>>>>> " " == Ulrich Drepper <drepper@redhat.com> writes:

     > I don't think ->pid should be tested.  Just replace it with
     > ->tgid.  It's really not intended for the user to have any
     > contact with the TID (i.e., ->pid).  This is how it's done in
     > other place.  What this shows is that more searches for ->pid
     > are needed which need to be replaced with ->tgid.

There's one remaining case in the NFS locking code:
nlmclnt_setlockargs() is using ->pid in order to label the lock owner.

I have a feeling that for that particular case, we'll just want to
drop the entire process-crap. The reason is that spec just says

   "The oh field is an opaque object that identifies the host or
   process that is making the request."

So as long as we're doing the lock accounting correctly on the client,
the server should be happy with just the hostname.
AFAIK, the word 'process' in the above sentence was added mainly in
order to allow userland NFS clients to push the accounting over onto
the server.

Cheers,
  Trond