On Thu, 22 Sep 2022 16:43:24 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:On Wed, Sep 21, 2022 at 10:55:04PM +0200, Stefano Brivio wrote:Ah, yes, I indeed wanted to have 65536 values in the bitmap (see commit title), and this does it, but there's no reason to make it implicit.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?If it's not too much effort for you, yes, that would be appreciated. I hope to be able to push out everything else later today, it's just one glitch still occurring in test_iperf3() -- sometimes, clients and servers terminate properly, but we don't see it for some reason. -- Stefano