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