Uid comparison in sbreserve()

[Elad Efrat <elad%NetBSD.org@localhost>]

Hi,

I think at least one part of the kern/uipc_socket2.c:sbreserve() code
is wrong, where it compares uids before applying a resource limit.

The relevant code is

    600         if (kauth_cred_geteuid(l->l_cred) == so->so_uidinfo->ui_uid)
    601                 maxcc = l->l_proc->p_rlimit[RLIMIT_SBSIZE].rlim_cur;
    602         else
    603                 maxcc = RLIM_INFINITY;

where 'l' is 'curlwp' in this context, and 'so' is the passed socket.

The evolution of this particular test boils down to two commits;
revision 1.59, introducing the following:

+       if (p && p->p_ucred->cr_uid == so->so_uid)
+               maxcc = p->p_rlimit[RLIMIT_SBSIZE].rlim_cur;
+       else
+               maxcc = RLIM_INFINITY;

and later on, once we always had a lwp context, revision 1.88:

-               if (l && kauth_cred_geteuid(l->l_cred) == 
so->so_uidinfo->ui_uid)
+               if (kauth_cred_geteuid(l->l_cred) == so->so_uidinfo->ui_uid)

I think the first change here was an oversight, because the logic
doesn't seem right to me. The process resource limits are only used
when both the process is not NULL and the uid from the process'
credentials matches the uid on the socket. What if the process is not
NULL, but the uids don't match? I don't think it's intended to fall
back on RLIM_INFINITY for such a case...

It seems that the original logic should have been

if (p != NULL)
    use resource limits from p;
else
    use infinity;

as in those days a NULL process pointer would mean the request is
coming from "the kernel." (Other networking code had that assumption
too in a lot of credential checks with a rather inconsistent policy on
what to do when the process or lwp context is NULL.) Once 'p' can
never be NULL, the logic should have obviously be rewritten

use resource limits from p;

without comparing any uids.

Approaching it differently, let's say the uids match, so we use the
limits from the process. What do we do if they don't match? Unless we
can somehow "guarantee" that whenever they don't match one of them is
always root, using infinity is wrong for such a case... And I don't
see how we can guarantee such a thing.

Therefore, I'd like to remove the uid comparison and always use the
limits from the process. :)

Thoughts?

-e.