Re: RFC: [PATCH] SCM_CREDS support

[Samuel Thibault]

Svante Signell, le Sat 21 Feb 2015 16:09:46 +0100, a écrit :
> Most glib2.0 and dbus tests pass (after bootstrapping).

Most, i.e. not all?  Are the failing ones related with SCM_CREDS?  Was
the synchronous/asynchronous issue solved?

> +  /* FIXME: Currently only ONE port is supported, error out if more */
> +  if (ncreds != 0 && ncreds != 1)
> +    {
> +      /* FIXME: Which error to issue here? */
> +      err = EGRATUITOUS;
> +      goto out;
> +    }

I don't really understand: is it really hard to support more than one
SCM_CREDS?  I haven't looked at the details, but it seems to me your
implementation would already be almost working fine for more than one
SCM_CREDS, i.e. it seems to be only missing accessing cports[something]
instead of cports[0], and using an array of rendezvous ports instead of
just one.

> +         idx[k] = 'c';

It seems quite cumbersome to me to be going through an idx array:
can't all the loops just work on the single ports array?  AIUI it's a
matter of just stuffing the SCM_CREDS part inside the existing loop for
SCM_RIGHTS.  That would also save going through the messages several
times.  Same for the receiving side.

> +#define UIDSIZE sizeof (uid_t) * CMGROUP_MAX
> +#define GIDSIZE sizeof (gid_t) * CMGROUP_MAX
> +    __uid_t euids_buf[UIDSIZE], auids_buf[UIDSIZE];
> +    __gid_t egids_buf[GIDSIZE], agids_buf[GIDSIZE];

?? sizes of the array are in number of elements, not in bytes.

> +    HURD_CRITICAL_BEGIN;
> +    __mutex_lock (&_hurd_id.lock);

Why taking these?

> +    /* Check IDs */
> +    if (euid != euids_buf[0] || auid != auids_buf[0] ||
> +     egid != egids_buf[0] || agid != agids_buf[0])
> +      {
> +     err = EPERM;

I'm not sure we should return EPERM here: the problem is not that the
receiving side didn't have some permission, but that the sending side
lied.  Perhaps EIO would be more appropriate?

> +     /* FIXME: is sorted check needed? */
> +     if (groups[i] != egids_buf[i])

Mmm. AIUI it should be already in the right order: on the sending side,
__getgroups will return them in the same order as what auth would
provide.

> +    /* Check PID  */
> +    /* XXX: Use proc_getallpids and proc_getprocinfo until
> +       proc_user_authenticate proc_server_authenticate is implemented

s/authenticate/identify/

> +    err = __USEPORT (PROC, __proc_getallpids (port, &pids, &npids));
...
> +         err = __USEPORT (PROC, __proc_getprocinfo (port, pids[i], &flags,
...
> +             if (pi->owner == euid)

This is not completely what we want to check, but it's probably fine for
a first implementation: even if we don't check the pid precisely, we
check that the pid belongs to the same user.  Please document that in
comments along the TODO.

> +     /* FIXME: which return code to use?
> +        EACCES = permission denied
> +        ESRCH = no such process
> +     */
> +     err = ESRCH;

Same as above: the client lied.

>             nfds = (cmsg->cmsg_len - CMSG_ALIGN (sizeof (struct cmsghdr)))
> -                  / sizeof (int);
> +             / sizeof (int);

Avoid space-changing hooks.

> -           if (cmsg->cmsg_level == SOL_SOCKET
> -               && cmsg->cmsg_type == SCM_RIGHTS)
> +           if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == 
> SCM_RIGHTS)

Ditto.  And even more so since it makes it longer than 80 characters!

> -               nfds = (cmsg->cmsg_len
> -                       - CMSG_ALIGN (sizeof (struct cmsghdr)))
> -                      / sizeof (int);
> -               for (i = 0; i < nfds && j < nports; i++, j++)
> +               nfds = (cmsg->cmsg_len - CMSG_ALIGN (sizeof (struct cmsghdr)))
> +                 / sizeof (int);
> +               for (i = 0; i < nfds && j < nrights; i++, j++)

Ditto.  This even hides some change in the code, this is really not
something to do for proper review.

> +     if (err)
> +       return __hurd_fail (err);
> +      }
> +
>    __vm_deallocate (__mach_task_self (), (vm_address_t) cdata, clen);

Take care of proper complete deallocations on error.

Samuel