Question about in_pcbbind() and in6_pcbbind()

[Elad Efrat]

Hi,

There are three issues that I'm wondering about regarding the two
functions mentioned in the subject:

1. When called from netinet/tcp_usrreq.c:tcp_usrrreq(), in_pcbbind()
takes "l", but in6_pcbbind() takes NULL:

362 case PRU_LISTEN:
363 #ifdef INET
364 if (inp && inp->inp_lport == 0) {
365 error = in_pcbbind(inp, NULL, l);
366 if (error)
367 break;
368 }
369 #endif
370 #ifdef INET6
371 if (in6p && in6p->in6p_lport == 0) {
372 error = in6_pcbbind(in6p, NULL, NULL);
373 if (error)
374 break;
375 }
376 #endif
377 tp->t_state = TCPS_LISTEN;
378 break;

Is there a reason for this? it seems that the use for both is the
same -- check if a privileged port can be bound to, only that for the
inet6 case, the request is denied if l == NULL.

I'd like us to always pass an lwp argument, and in the future
probably just the kauth_cred_t.

2. When called from the same locations, you'll also notice the second
argument (mbuf *nam) is NULL. Passing NULL is functionally equivalent
to passing a zeroed struct sockaddr_in6 wrapped in an mbuf, with one
difference: there's no sockaddr struct to pass to kauth(9). (We could
just pass the address and the port, but I think we'll be wasting
parameter space that way.)

I believe this is something we want to change, as we do want policies
that will cover all common (bind()) as well as uncommon (LOWPORT +
connect) cases that can lead to an address/port bind.

My two proposed solutions:

- Make the caller construct a dummy mbuf with a dummy sockaddr_in
and sockaddr_in6 and pass them when calling in_pcbbind() and
in6_pcbbind respectively.

or

- Put code inside in_pcbbind() and in6_pcbbind() to cope with a
nam == NULL by creating a dummy sockaddr_in and sockaddr_in6
themselves before proceeding.

Which one is preferable?

(Personally, I think we should also slice the logic inside both
functions to two, smaller functions, say in{,6}_pcbbind_{addr,port},
if only to make reading of the code easier. The original functions
will retain original functionality by calling both new ones, of
course.)

3. It seems that in_pcbbind(), when exhausting all free ports when
auto-assigning a port, does the following:

356 if (!in_nullhost(inp->inp_laddr))
357 inp->inp_laddr.s_addr = INADDR_ANY;
358 return (EAGAIN);

To prevent a case where an inpcb will have an address but not a port
(that makes sense; see the test for port/non-"null" address on entry
to both in_pcbbind() and in6_pcbbind()), while in6_pcbbind() does
not:

871 if (t == 0)
872 goto found;
873 }
874
875 return (EAGAIN);
876
877 found:
878 in6p->in6p_flags |= IN6P_ANONPORT;

Is this intended? if yes, why?

Thanks,

-e.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Elad Efrat]

Post by Elad Efrat
Hi,
There are three issues that I'm wondering about regarding the two
1. When called from netinet/tcp_usrreq.c:tcp_usrrreq(), in_pcbbind()
[...]

We now pass always pass the lwp.

Post by Elad Efrat
2. When called from the same locations, you'll also notice the second
argument (mbuf *nam) is NULL. Passing NULL is functionally equivalent
to passing a zeroed struct sockaddr_in6 wrapped in an mbuf, with one
difference: there's no sockaddr struct to pass to kauth(9). (We could
just pass the address and the port, but I think we'll be wasting
parameter space that way.)
[...]

I decided to slice in6_pcbbind(), and attached is the result (for
netinet6 only). It:

- Keeps in6_pcbbind()'s semantics,
- Introduces in6_pcbbind_{addr,port}()
- Uses "dom_sa_any" when the passed argument is NULL

(we could probably use some KASSERT()s in the two new functions to make
sure we don't end up *there* with sin6 == NULL, etc.)

Post by Elad Efrat
3. It seems that in_pcbbind(), when exhausting all free ports when
[...]

This is fixed in the attached patch as well.

I've looked at both the FreeBSD and OpenBSD equivalents and it seems
that FreeBSD does address this issue by resetting the address "any",
but it does it in in6_pcbsetport(), which is also called from
udp6_output(), and I'm not sure resetting the laddr on the in6pcb is
intended (or desired?) in such a context -- so I reset the address if
in6_pcbsetport() fails in in6_pcbbind() itself.

Please review. :)

Thanks,

-e.

[Elad Efrat]

Hi,

Post by Elad Efrat
I decided to slice in6_pcbbind(), and attached is the result (for
 - Keeps in6_pcbbind()'s semantics,
 - Introduces in6_pcbbind_{addr,port}()
 - Uses "dom_sa_any" when the passed argument is NULL

As I did not receive any complaints, I'll commit the patch posted in a
day or two, and do (and post) a version for netinet as well.

Thanks,

-e.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de