Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d59de44c-4716-cf5d-906b-5a3d8685f53b@redhat.com>
Date:   Thu, 14 Jul 2022 13:01:39 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH] binddynport.c honor ip_local_reserved_ports
Content-Language: en-US
To:     Otto Hollmann <ohollmann@suse.cz>,
        libtirpc-devel@lists.sourceforge.net
Cc:     linux-nfs@vger.kernel.org, Thomas Blume <thomas.blume@suse.com>
References: <1654766776.2720.14.camel@suse.cz>
From:   Steve Dickson <steved@redhat.com>
In-Reply-To: <1654766776.2720.14.camel@suse.cz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hey,

My apologies for taking so long to get to this...

A couple questions:

1) How well was tested... Is in your distro already?
2) Those new functions the patch introduces...
    Don't effect the API? Meaning shouldn't they
    declared as static?

steved.

On 6/9/22 4:26 AM, Otto Hollmann wrote:
> Read reserved ports from /proc/sys/net/ipv4/ip_local_reserved_ports,
> store them into bit-wise array and before binding to random port check
> if port is not reserved.
> 
> 
> Currently, there is no way how to reserve ports so then will not be
> used by rpcbind.
> 
> Random ports are opened by rpcbind because of rmtcalls. There is
> compile-time flag for disabling them, but in some cases we can not
> simply disable them.
> 
> One solution would be run time option --enable-rmtcalls as already
> discussed, but it was rejected. So if we want to keep rmtcalls enabled
> and also be able to reserve some ports, there is no other way than
> filtering available ports. The easiest and clearest way seems to be
> just respect kernel list of ip_reserved_ports.
> 
> Unfortunately there is one known disadvantage/side effect - it affects
> probability of ports which are right after reserved ones. The bigger
> reserved block is, the higher is probability of selecting following
> unreserved port. But if there is no reserved port, impact of this patch
> is minimal/none.
> 
> Signed-off-by: Otto Hollmann <otto.hollmann@suse.com>
> ---
>   src/binddynport.c | 107 ++++++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 99 insertions(+), 8 deletions(-)
> 
> diff --git a/src/binddynport.c b/src/binddynport.c
> index 062629a..6f78ebe 100644
> --- a/src/binddynport.c
> +++ b/src/binddynport.c
> @@ -37,6 +37,7 @@
>   #include <unistd.h>
>   #include <errno.h>
>   #include <string.h>
> +#include <syslog.h>
>   
>   #include <rpc/rpc.h>
>   
> @@ -56,6 +57,84 @@ enum {
>   	NPORTS		= ENDPORT - LOWPORT + 1,
>   };
>   
> +/*
> + * This function decodes information about given port from provided array and
> + * return if port is reserved or not.
> + *
> + * @reserved_ports an array of size at least "NPORTS / (8*sizeof(char)) + 1".
> + * @port port number within range LOWPORT and ENDPORT
> + *
> + * Returns 0 if port is not reserved, non-negative if port is reserved.
> + */
> +int is_reserved(char *reserved_ports, int port) {
> +	port -= LOWPORT;
> +	if (port < 0 || port >= NPORTS)
> +		return 0;
> +	return reserved_ports[port/(8*sizeof(char))] & 1<<(port%(8*sizeof(char)));
> +}
> +
> +/*
> + * This function encodes information about given *reserved* port into provided
> + * array. Don't call this function for ports which are not reserved.
> + *
> + * @reserved_ports array TODO .
> + * @port port number within range LOWPORT and ENDPORT
> + *
> + */
> +void set_reserved(char *reserved_ports, int port) {
> +	port -= LOWPORT;
> +	if (port < 0 || port >= NPORTS)
> +		return;
> +	reserved_ports[port/(8*sizeof(char))] |= 1<<(port%(8*sizeof(char)));
> +}
> +
> +/*
> + * Parse local reserved ports obtained from
> + * /proc/sys/net/ipv4/ip_local_reserved_ports into bit array.
> + *
> + * @reserved_ports a zeroed array of size at least
> + * "NPORTS / (8*sizeof(char)) + 1". Will be used for bit-wise encoding of
> + * reserved ports.
> + *
> + * On each call, reserved ports are read from /proc and bit-wise stored into
> + * provided array
> + *
> + * Returns 0 on success, -1 on failure.
> + */
> +
> +int parse_reserved_ports(char *reserved_ports) {
> +	int from, to;
> +	char delimiter = ',';
> +	int res;
> +	FILE * file_ptr = fopen("/proc/sys/net/ipv4/ip_local_reserved_ports","r");
> +	if (file_ptr == NULL) {
> +		(void) syslog(LOG_ERR,
> +			"Unable to open open /proc/sys/net/ipv4/ip_local_reserved_ports.");
> +		return -1;
> +	}
> +	do {
> +		if ((res = fscanf(file_ptr, "%d", &to)) != 1) {
> +			if (res == EOF) break;
> +			goto err;
> +		}
> +		if (delimiter != '-') {
> +			from = to;
> +		}
> +		for (int i = from; i <= to; ++i) {
> +			set_reserved(reserved_ports, i);
> +		}
> +	} while ((res = fscanf(file_ptr, "%c", &delimiter)) == 1);
> +	if (res != EOF)
> +		goto err;
> +	fclose(file_ptr);
> +	return 0;
> +err:
> +	(void) syslog(LOG_ERR,
> +		"An error occurred while parsing ip_local_reserved_ports.");
> +	fclose(file_ptr);
> +	return -1;
> +}
> +
>   /*
>    * Bind a socket to a dynamically-assigned IP port.
>    *
> @@ -81,7 +160,8 @@ int __binddynport(int fd)
>   	in_port_t port, *portp;
>   	struct sockaddr *sap;
>   	socklen_t salen;
> -	int i, res;
> +	int i, res, array_size;
> +	char *reserved_ports;
>   
>   	if (__rpc_sockisbound(fd))
>   		return 0;
> @@ -119,21 +199,32 @@ int __binddynport(int fd)
>   		gettimeofday(&tv, NULL);
>   		seed = tv.tv_usec * getpid();
>   	}
> +	array_size = NPORTS / (8*sizeof(char)) + 1;
> +	reserved_ports = malloc(array_size);
> +	if (!reserved_ports) {
> +		goto out;
> +	}
> +	memset(reserved_ports, 0, array_size);
> +	parse_reserved_ports(reserved_ports);
> +
>   	port = (rand_r(&seed) % NPORTS) + LOWPORT;
>   	for (i = 0; i < NPORTS; ++i) {
> -		*portp = htons(port++);
> -		res = bind(fd, sap, salen);
> -		if (res >= 0) {
> -			res = 0;
> -			break;
> +		*portp = htons(port);
> +		if (!is_reserved(reserved_ports, port++)) {
> +			res = bind(fd, sap, salen);
> +			if (res >= 0) {
> +				res = 0;
> +				break;
> +			}
> +			if (errno != EADDRINUSE)
> +				break;
>   		}
> -		if (errno != EADDRINUSE)
> -			break;
>   		if (port > ENDPORT)
>   			port = LOWPORT;
>   	}
>   
>   out:
> +	free(reserved_ports);
>   	mutex_unlock(&port_lock);
>   	return res;
>   }