From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Gibson To: passt-dev@passt.top Subject: Re: [PATCH 4/7] conf, tcp, udp: Arrays for ports need 2^16 values, not 2^16-8 Date: Thu, 22 Sep 2022 16:43:24 +1000 Message-ID: In-Reply-To: <20220921205507.2742203-5-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8380172004081135501==" --===============8380172004081135501== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Wed, Sep 21, 2022 at 10:55:04PM +0200, Stefano Brivio wrote: > Reported by David but also by Coverity (CWE-119): >=20 > In conf_ports: Out-of-bounds access to a buffer >=20 > ...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=3D=3D0). 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? >=20 > Reported-by: David Gibson > Signed-off-by: Stefano Brivio > --- > conf.c | 2 +- > tcp.h | 4 ++-- > udp.h | 4 ++-- > 3 files changed, 5 insertions(+), 5 deletions(-) >=20 > 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, cons= t char *optarg, > { > int start_src, end_src, start_dst, end_dst, exclude_only =3D 1, i, port; > char addr_buf[sizeof(struct in6_addr)] =3D { 0 }, *addr =3D addr_buf; > + uint8_t *map, exclude[DIV_ROUND_UP(USHRT_MAX, 8)] =3D { 0 }; > void (*remap)(in_port_t port, in_port_t delta); > - uint8_t *map, exclude[USHRT_MAX / 8] =3D { 0 }; > char buf[BUFSIZ], *sep, *spec, *p; > sa_family_t af =3D AF_UNSPEC; > =20 > 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; > }; --=20 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 --===============8380172004081135501== Content-Type: application/pgp-signature Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="signature.asc" MIME-Version: 1.0 LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KCmlRSXpCQUVCQ0FBZEZpRUVvVUx4V3U0L1dz MGRCK1h0Z3lwWTRnRXdZU0lGQW1Nc0JBVUFDZ2tRZ3lwWTRnRXcKWVNLaVdnLzdCZjZDNGFmcWRL N2FGVVpycXp6Y2p2a2plNURuS09OS2p6NHAxSDN0eTRvbUtzZ2pNVzhRSDJTZApoMjdaTytic3ZM TkZKZE9raWFwR2tCQWVjZWVLcWRaTjhyb1VpT0lnUzRhYVl5c3pQNERaSklDUE16MTcvSE41ClRx WHZSVVljbk1ZeDlEZSttTWFFblNNclpSZzRPRDVuNS93WlZVVGxrL1pLQkh1d0F1TndGbzJFdUlr dXZFZDkKRzhhU0xyUFRRVS9MejM0MHZmaURtdWJINVBXaTF3Qk0rVTVUZW5SNCtySzZhcFlLbDZX MU1qcUJ0R0MwNWdHagpMZGlBTURPa0xLVytFcnJ5dExWTjJZbWlJYVhYR3Yva1dXKzdYYzY5ZDZY bzBmOU85T0FTQjh1U0xwM3FtVlZCCmVDUG9CVVQ5YW5vY1l4aUpqOXgxWm9JeDlRb213MllnMWpD aSt6Mk56K3NFWTJMZHIzTHRzenRXLzB1d0RjdlQKck9Pcjg1RjQ0bjc5RFJ3MUdHN1dJRnZ1WEtL bnEyc2hvcFpyOVNMN1g3SWNESStCeDB1R1IxWnpTTitsODlsRAo5M0hLblU0Q1RubHQ1REhFdmZp QlFuc2xHazVrMjlnSHU0TlY3bnAwZWFhdENEeTJXOGdBSVRleE5BWWp0anoxCmw4TEpUYmIyQ1VR RmZDaW5iMlRqenV3enkreGEvUlUyc3RnTzBVSno2SVF1MDZDWElBK1p6Y09MMWp1cmhWeFgKTWtC ZEZHUWc1UVkvUkdLdHNMd2l4NCtiODFKcmg3OGIzdk5Scmxhenord0RMV0JjNDVKdGtwMGM3Rit5 aDYrbAp5cUJFRGhmcWpHZ1lQeUNkdFgyUVk1eGJWZ0gyOFYybmR6UGtMRlBvUjUvaExsdjh1dVE9 Cj1Qdnd2Ci0tLS0tRU5EIFBHUCBTSUdOQVRVUkUtLS0tLQo= --===============8380172004081135501==--