Re: [PATCH 4/7] conf, tcp, udp: Arrays for ports need 2^16 values, not 2^16-8

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3482 bytes --]

On Wed, Sep 21, 2022 at 10:55:04PM +0200, Stefano Brivio wrote:
> Reported by David but also by Coverity (CWE-119):
> 
> 	In conf_ports: Out-of-bounds access to a buffer
> 
> ...not in practice, because the allocation size is rounded up
> anyway, but not nice either.

So... this helps, but it's not enough.  For starters, this is still
conceptually wrong - there should be 65536 bits in the bitmap, not
65535 (USHRT_MAX).  This patch just masks it by rounding up to 65536
rather than down to 65528.

Alas, this is not the only buffer overrun caused by an off-by-one in
port numbers: the delta_to_tap etc. global arrays in tcp.c and udp.c
should also have length 65536, rather than USHRT_MAX.  So should
tcp_sock_init_lo and friends.  There are also similar problems in
icmp.c with echo request id rather than port number.

Not a buffer overrun, but the USHRT_MAX in udp_remap_to_tap() and
udp_remap_to_init() is also out by one (consider the case of
delta==0).

I have a series which makes a number of cleanups to the port mapping
handling.  It fixes this bug and adds typing stuff that should make it
harder to make similar mistakes in future.

I held off on sending, since it's currently based on a lot of my
outstanding patches.  I think it wouldn't be too hard to rebase
directly on master, should I do that so you can fix this before going
on to debug the rest of my outstanding stuff?

> 
> Reported-by: David Gibson <david(a)gibson.dropbear.id.au>
> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
> ---
>  conf.c | 2 +-
>  tcp.h  | 4 ++--
>  udp.h  | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index d80233c..7ecfa1e 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -127,8 +127,8 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg,
>  {
>  	int start_src, end_src, start_dst, end_dst, exclude_only = 1, i, port;
>  	char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf;
> +	uint8_t *map, exclude[DIV_ROUND_UP(USHRT_MAX, 8)] = { 0 };
>  	void (*remap)(in_port_t port, in_port_t delta);
> -	uint8_t *map, exclude[USHRT_MAX / 8] = { 0 };
>  	char buf[BUFSIZ], *sep, *spec, *p;
>  	sa_family_t af = AF_UNSPEC;
>  
> diff --git a/tcp.h b/tcp.h
> index 7b720c1..6431b75 100644
> --- a/tcp.h
> +++ b/tcp.h
> @@ -69,9 +69,9 @@ struct tcp_ctx {
>  	uint64_t hash_secret[2];
>  	int conn_count;
>  	int splice_conn_count;
> -	uint8_t port_to_tap	[USHRT_MAX / 8];
> +	uint8_t port_to_tap	[DIV_ROUND_UP(USHRT_MAX, 8)];
>  	int init_detect_ports;
> -	uint8_t port_to_init	[USHRT_MAX / 8];
> +	uint8_t port_to_init	[DIV_ROUND_UP(USHRT_MAX, 8)];
>  	int ns_detect_ports;
>  	struct timespec timer_run;
>  #ifdef HAS_SND_WND
> diff --git a/udp.h b/udp.h
> index f16fe5e..8f82842 100644
> --- a/udp.h
> +++ b/udp.h
> @@ -53,9 +53,9 @@ union udp_epoll_ref {
>   * @timer_run:		Timestamp of most recent timer run
>   */
>  struct udp_ctx {
> -	uint8_t port_to_tap		[USHRT_MAX / 8];
> +	uint8_t port_to_tap		[DIV_ROUND_UP(USHRT_MAX, 8)];
>  	int init_detect_ports;
> -	uint8_t port_to_init		[USHRT_MAX / 8];
> +	uint8_t port_to_init		[DIV_ROUND_UP(USHRT_MAX, 8)];
>  	int ns_detect_ports;
>  	struct timespec timer_run;
>  };

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]