From: Vitaliy Makkoveev <otto@bsdbox.dev>
Subject: Re: TCP port reuse at bind and connect
To: Alexander Bluhm <alexander.bluhm@gmx.net>
Cc: tech@openbsd.org
Date: Fri, 22 Mar 2024 19:50:22 +0300

> On 22 Mar 2024, at 13:46, Alexander Bluhm <alexander.bluhm@gmx.net> wrote:
> 
> On Thu, Mar 21, 2024 at 10:58:26PM +0100, Alexander Bluhm wrote:
>> On top of that I found another problem.  in_pcbconnect() does not
>> pass down the address it got from in_pcbselsrc() to in_pcbpickport().
>> As a consequence local port numbers selected during connect(2) must
>> be unique although they belong to different addresses.  This kind
>> of uniqueness is not necessary and wastes more ports.
>> 
>> To solve this, I pass ina from in_pcbconnect() to in_pcbbind_locked().
>> This does not interfere how wildcard sockets are matched with
>> specific sockets during bind(2).  It only allows non-wildcard sockets
>> to share a local port during connect(2).
>> 
>> I can split the latter bugfix from the diff if wanted.  The problem
>> is just hard to see and test if all ports are random.
> 
> I think we should commit this part first.
> 
> ok?
> 

ok mvs

> bluhm
> 
> Index: netinet/in_pcb.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in_pcb.c,v
> diff -u -p -r1.296 in_pcb.c
> --- netinet/in_pcb.c	29 Feb 2024 12:01:59 -0000	1.296
> +++ netinet/in_pcb.c	22 Mar 2024 10:39:06 -0000
> @@ -277,12 +277,12 @@ in_pcballoc(struct socket *so, struct in
> }
> 
> int
> -in_pcbbind_locked(struct inpcb *inp, struct mbuf *nam, struct proc *p)
> +in_pcbbind_locked(struct inpcb *inp, struct mbuf *nam, const void *laddr,
> +    struct proc *p)
> {
> 	struct socket *so = inp->inp_socket;
> 	u_int16_t lport = 0;
> 	int wild = 0;
> -	const void *laddr = &zeroin46_addr;
> 	int error;
> 
> 	if (inp->inp_lport)
> @@ -359,7 +359,7 @@ in_pcbbind(struct inpcb *inp, struct mbu
> 
> 	/* keep lookup, modification, and rehash in sync */
> 	mtx_enter(&table->inpt_mtx);
> -	error = in_pcbbind_locked(inp, nam, p);
> +	error = in_pcbbind_locked(inp, nam, &zeroin46_addr, p);
> 	mtx_leave(&table->inpt_mtx);
> 
> 	return error;
> @@ -542,7 +542,7 @@ in_pcbconnect(struct inpcb *inp, struct 
> 
> 	if (inp->inp_laddr.s_addr == INADDR_ANY) {
> 		if (inp->inp_lport == 0) {
> -			error = in_pcbbind_locked(inp, NULL, curproc);
> +			error = in_pcbbind_locked(inp, NULL, &ina, curproc);
> 			if (error) {
> 				mtx_leave(&table->inpt_mtx);
> 				return (error);
> Index: netinet/in_pcb.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in_pcb.h,v
> diff -u -p -r1.152 in_pcb.h
> --- netinet/in_pcb.h	13 Feb 2024 12:22:09 -0000	1.152
> +++ netinet/in_pcb.h	22 Mar 2024 10:39:37 -0000
> @@ -307,7 +307,8 @@ extern int in_pcbnotifymiss;
> void	 in_init(void);
> void	 in_losing(struct inpcb *);
> int	 in_pcballoc(struct socket *, struct inpcbtable *, int);
> -int	 in_pcbbind_locked(struct inpcb *, struct mbuf *, struct proc *);
> +int	 in_pcbbind_locked(struct inpcb *, struct mbuf *, const void *,
> +	    struct proc *);
> int	 in_pcbbind(struct inpcb *, struct mbuf *, struct proc *);
> int	 in_pcbaddrisavail(const struct inpcb *, struct sockaddr_in *, int,
> 	    struct proc *);
> Index: netinet6/in6_pcb.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/in6_pcb.c,v
> diff -u -p -r1.141 in6_pcb.c
> --- netinet6/in6_pcb.c	29 Feb 2024 12:01:59 -0000	1.141
> +++ netinet6/in6_pcb.c	22 Mar 2024 10:40:47 -0000
> @@ -313,7 +313,7 @@ in6_pcbconnect(struct inpcb *inp, struct
> 
> 	if (IN6_IS_ADDR_UNSPECIFIED(&inp->inp_laddr6)) {
> 		if (inp->inp_lport == 0) {
> -			error = in_pcbbind_locked(inp, NULL, curproc);
> +			error = in_pcbbind_locked(inp, NULL, in6a, curproc);
> 			if (error) {
> 				mtx_leave(&table->inpt_mtx);
> 				return (error);
>