From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 1A1E35A0319 for ; Fri, 28 Jun 2024 09:19:51 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1719559186; bh=W38f2vfkfUxb9QvxC7eSdzNNbdyoBIfypV8uGLFlxng=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=RLGAyUgKwpO83jW/ZlW8HZ5r4HMXZmR9+gdIGiNFep02+PPkfW7u1B5GkkJdeJ9XL ytvL3JziCGb9ga1jDJFC/09ZHfmQD1Ub6JO9U95a/OXpfy2Vh0s3V4U3irkLvCzoZE Bzesw+MrB8IaMog61KNLA401PB5stMTpz+2qtONmCC6DXhq6sR6nvADQ98XDnjfaZM jz7Fl5WZfCDh5NOwrMIrVE1Ew0dCobaGxw9uKfYb960JuKhcP6tUP2A22Zu8YGnYD6 78mGOW1iKVc4oz8HnJeoF0+MsHLKn5XEVbUqvBV/0eiZ3Ho9EHSy/mgE5KFzBHIUEC dblsSivoEQ/BA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4W9Rdt3tmBz4wc8; Fri, 28 Jun 2024 17:19:46 +1000 (AEST) Date: Fri, 28 Jun 2024 17:11:43 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 3/4] util, lineread, tap: Overflow checks on long signed sums and subtractions Message-ID: References: <20240626234536.3306466-1-sbrivio@redhat.com> <20240626234536.3306466-4-sbrivio@redhat.com> <20240627095537.4d3f20f8@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="79nMmf18nec6gMhv" Content-Disposition: inline In-Reply-To: <20240627095537.4d3f20f8@elisabeth> Message-ID-Hash: 3WUK6PVYFBJ7W24CB5ANM2LXN6LOOMB4 X-Message-ID-Hash: 3WUK6PVYFBJ7W24CB5ANM2LXN6LOOMB4 X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Matej Hrica X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --79nMmf18nec6gMhv Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jun 27, 2024 at 09:55:46AM +0200, Stefano Brivio wrote: > On Thu, 27 Jun 2024 11:13:14 +1000 > David Gibson wrote: >=20 > > On Thu, Jun 27, 2024 at 01:45:35AM +0200, Stefano Brivio wrote: > > > Potential sum and subtraction overflows were reported by Coverity in a > > > few places where we use size_t and ssize_t types. > > >=20 > > > Strictly speaking, those are not false positives even if I don't see a > > > way to trigger those: for instance, if we receive up to n bytes from a > > > socket up, the return value from recv() is already constrained and > > > can't overflow for the usage we make in tap_handler_passt(). =20 > >=20 > > Actually, I think they are false positives. In a bunch of cases the > > reasoning for that does rely on assuming the kernel will never return > > a value greater than the buffer size for read(), write() or similar. >=20 > No, that was exactly my point: return values are constrained by the > kernel, but a static checker doesn't necessarily have to assume a kernel > that's properly functioning. Well, yes. > In general, static checkers do, especially if POSIX has a clear > definition of a system call, and for what matters to us, they should. Right, that's the assumption I was working under. > But here Coverity is ignoring that, and I'm not sure we should call it > a false positive. It's kind of arbitrary really. I think Coverity in > these cases just prefers to "blindly" apply CERT C INT32-C locally, > which is not necessarily a bad choice, because "false positives" are > not so much of a nuisance. >=20 > > So possibly just ASSERT()ing that would suffice. >=20 > In some cases yes, but as we have built-ins in gcc and Clang that aim > at keeping the cost of the checks down by, quoting gcc documentation, > using "hardware instructions to implement these built-in functions where > possible", and they already implement the operation, open-coding our own > checks for assertions looks redundant and might result in slower > code. It's not runtime cost I'm concerned about, I'm sure that's trivial. What does concern me is: - the overflow checking functions look like line noise - it introduces extra error paths which make it harder to see what the code's doing - it doesn't really save reasoning about what ranges things can have, because we need to know where to put them, unless we put them on every single operation, which makes the above points much worse I prefer checking that the syscall return values are within the bounds we expect, rather than checking for later overflows, because as well as the above, if we ever do get weird values out of the syscalls it should show up the problem as close to the source as possible. > > > In any case, take care of those by adding two functions that > > > explicitly check for overflows in sums and subtractions of long signed > > > values, and using them. > > >=20 > > > Signed-off-by: Stefano Brivio > > > --- > > > lineread.c | 5 +++-- > > > tap.c | 21 +++++++++++++++------ > > > util.c | 7 +++++-- > > > util.h | 46 +++++++++++++++++++++++++++++++++++++++++++++- > > > 4 files changed, 68 insertions(+), 11 deletions(-) > > >=20 > > > diff --git a/lineread.c b/lineread.c > > > index 0387f4a..12f2d24 100644 > > > --- a/lineread.c > > > +++ b/lineread.c > > > @@ -17,6 +17,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > =20 > > > #include "lineread.h" > > > #include "util.h" > > > @@ -102,8 +103,8 @@ ssize_t lineread_get(struct lineread *lr, char **= line) > > > =20 > > > if (rc =3D=3D 0) > > > eof =3D true; > > > - else > > > - lr->count +=3D rc; =20 > >=20 > > From the construction of the read, lr->count + rc can never exceed > > LINEREAD_BUFFER_SIZE - lr->next_line, so this can't overflow. >=20 > Sure. But, especially as package maintainer, in this case I prefer to > have a useless check than carrying suppressions around. Right, but my hope is that if we ASSERT() or otherwise check that property of the read return value, then that will allow Coverity to reason out the rest without needing an explicit suppression. >=20 > > > + else if (saddl_overflow(lr->count, rc, &lr->count)) > > > + return -ERANGE; > > > } > > > =20 > > > *line =3D lr->buf + lr->next_line; > > > diff --git a/tap.c b/tap.c > > > index ec994a2..7f8c26d 100644 > > > --- a/tap.c > > > +++ b/tap.c > > > @@ -1031,7 +1031,11 @@ redo: > > > */ > > > if (l2len > n) { > > > rem =3D recv(c->fd_tap, p + n, l2len - n, 0); > > > - if ((n +=3D rem) !=3D l2len) =20 > >=20 > > Similarly, rem <=3D l2len - n, and therefore n + rem <=3D l2len. >=20 > Same here. >=20 > > > + > > > + if (saddl_overflow(n, rem, &n)) > > > + return; > > > + > > > + if (n !=3D l2len) > > > return; > > > } > > > =20 > > > @@ -1046,7 +1050,9 @@ redo: > > > =20 > > > next: > > > p +=3D l2len; > > > - n -=3D l2len; =20 > >=20 > > We already checked that l2len <=3D n, so this one can't overflow either. >=20 > Same here. >=20 > > Not sure why Coverity can't see that itself, though :/. Possibly it > > doesn't understand gotos well enough to see that the only goto next is > > after that check. >=20 > It sees that, that's the path it takes in reporting a potential > overflow here. I think here, again, it's just blindly requesting > INT32-C from CERT C rules, locally. Hmmm... in each of these cases the reasoning to show that there can't be an overflow isn't very complicated - at least once you put in the assumption about the syscall return values, which is why I'm hoping asserting that fact will let Coverity sort it out. If it's reasoning more locally than the function, I can't see how that won't devolve into anything other than "never use signed arithmetic in C, at all, ever". --=20 David Gibson (he or they) | 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 --79nMmf18nec6gMhv Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZ+Yi4ACgkQzQJF27ox 2Ge2TA//Y7CrC0Roqmqi3EAl2lp6s5fQCP8qkkCzs3DZGPXJ6smsg4Dzlg1j5kgD CfrkaktcLd+9F/DJFuGCE2FbImAaEB+W0l5HZKawCQKJOtDr2eck8RozqyjHOH3n U/BBm75w/L8QbkQ5a62jk9Qac9X0vdI8aKLyukecORBMJU3YaNQmyN1JLGM5AV2d lT+BW2zHisCecjITqjNI6FogJ1rg88LIUj49Rihi147BSkGAzplGPGaUITxsmcDV k2+LHYiXU48QGRRL3P7SVi9Pl1AN3hABA8tzUUht4n59MYPNwxlwPENCDKSnLgKN QD1gDKj2tjZ/9dllBclaYF0rFlg0rlNUjcNITbI6bPotFWWigeVgRcRRTqYhpULD pkPfb1/VHZfsrXjt+kBIFC94Pn+3wO3aTo09kE3p0b1tfok9FCCnvJ49O51QKg7R zwHXBwepPqYgGm8E2ft895AVO7Ie9SF2pkvFy1CVAFpLtnjwyCCBk5tePfIZ/UBW 0tk37NfX9gzzcoUAMTaAvLa67Wlk74Rt4WP+u0EQ+6YAXpTQ8utdVudA3UW1Niyy AZc+iug0xEBjDnFXzN14hopuPZFvIYZb4uPeN19A0x3qu6hoBbbIj5hIs5YXBlp0 uQMvyScFSgvbKhSeLEHpDjbWu90NOEAI19bCsiWWirdNyS/rj9A= =hA9t -----END PGP SIGNATURE----- --79nMmf18nec6gMhv--