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 1B8635A004F for ; Thu, 27 Jun 2024 03:13:23 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1719450799; bh=M41VjElluGnKD+QHYZs6s6kV3a2HCzTCYx/4vOmuRIg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=kVZI9Pj8sD5wBzPFPi1xEX13Gpc3xEeNSV22Bj8M2vxV46wy519hZwhenIhQZdY2m H+QLLBxO+bVg0XYfk24IrurfdtG+rotmIPj22oqjVgCwerkrowixDe3v698y7jAnso kwpoT0HBd+wsekol/MmboD4pHX4LcGjwTWFjM8Leqt0R/OPj90+PXucJJ/bLadnwb9 R0CuWOIw4k5h/fUCrO//oc/lCLGVM1mbnBstK0mzZZu+Uq+fRe+2mEyq5ia0OG9+Or r6gb7YpgUD1vU6qDRc/l5O37RaD4eNmzgbP3mUdaJWjKCg6TkTBine+IG0ea3G5Jgd jGfHPL9mEcgIQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4W8gYW3PrBz4x0y; Thu, 27 Jun 2024 11:13:19 +1000 (AEST) Date: Thu, 27 Jun 2024 11:13:14 +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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ztZYi9GBc4/d/fmW" Content-Disposition: inline In-Reply-To: <20240626234536.3306466-4-sbrivio@redhat.com> Message-ID-Hash: SZB75CJGDZNLMZS2UZVNOMUPDRTOX473 X-Message-ID-Hash: SZB75CJGDZNLMZS2UZVNOMUPDRTOX473 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: --ztZYi9GBc4/d/fmW Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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(). 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. So possibly just ASSERT()ing that would suffice. > 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; =46rom the construction of the read, lr->count + rc can never exceed LINEREAD_BUFFER_SIZE - lr->next_line, so this can't overflow. > + 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) Similarly, rem <=3D l2len - n, and therefore n + rem <=3D l2len. > + > + 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; We already checked that l2len <=3D n, so this one can't overflow either. 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. > + > + if (ssubl_overflow(n, l2len, &n)) > + return; > } > =20 > tap_handler(c, now); > @@ -1077,17 +1083,20 @@ redo: > tap_flush_pools(); > restart: > while ((len =3D read(c->fd_tap, pkt_buf + n, TAP_BUF_BYTES - n)) > 0) { > - > if (len < (ssize_t)sizeof(struct ethhdr) || > len > (ssize_t)ETH_MAX_MTU) { > - n +=3D len; Here n+len can't exceed TAP_BUF_BYTES, so again, no overflow. > + if (saddl_overflow(n, len, &n)) > + return; > + > continue; > } > =20 > - > tap_add_packet(c, len, pkt_buf + n); > =20 > - if ((n +=3D len) =3D=3D TAP_BUF_BYTES) Same here. > + if (saddl_overflow(n, len, &n)) > + return; > + > + if (n =3D=3D TAP_BUF_BYTES) > break; > } > =20 > diff --git a/util.c b/util.c > index dd2e57f..a72d6c5 100644 > --- a/util.c > +++ b/util.c > @@ -567,7 +567,7 @@ int do_clone(int (*fn)(void *), char *stack_area, siz= e_t stack_size, int flags, > * > * #syscalls write writev > */ > -int write_remainder(int fd, const struct iovec *iov, int iovcnt, size_t = skip) > +int write_remainder(int fd, const struct iovec *iov, int iovcnt, ssize_t= skip) I don't love this change, since negative skip values make no sense. > { > int i; > size_t offset; > @@ -585,7 +585,10 @@ int write_remainder(int fd, const struct iovec *iov,= int iovcnt, size_t skip) > if (rc < 0) > return -1; > =20 > - skip +=3D rc; Ok, here it's not a false positive. I believe this really could overflow if you had an iov where the sum of the iov_len exceeded a size_t. > + if (saddl_overflow(skip, rc, &skip)) { > + errno =3D -ERANGE; > + return -1; > + } If you leave skip an unsigned, you've already checked for negative rc, so this is essentially an unsigned add. Checking for overflow on an unsigned addition is simpler than the logic of saddl_overflow(). > } > =20 > return 0; > diff --git a/util.h b/util.h > index eebb027..497d2fd 100644 > --- a/util.h > +++ b/util.h > @@ -161,7 +161,7 @@ void pidfile_write(int fd, pid_t pid); > int __daemon(int pidfile_fd, int devnull_fd); > int fls(unsigned long x); > int write_file(const char *path, const char *buf); > -int write_remainder(int fd, const struct iovec *iov, int iovcnt, size_t = skip); > +int write_remainder(int fd, const struct iovec *iov, int iovcnt, ssize_t= skip); > =20 > /** > * af_name() - Return name of an address family > @@ -223,6 +223,50 @@ static inline bool mod_between(unsigned x, unsigned = i, unsigned j, unsigned m) > return mod_sub(x, i, m) < mod_sub(j, i, m); > } > =20 > +/** > + * saddl_overflow() - Sum with overflow check for long signed values > + * @a: First value > + * @b: Second value > + * @sum: Pointer to result of sum, if it doesn't overflow > + * > + * Return: true if the sum would overflow, false otherwise > + */ > +static inline bool saddl_overflow(long a, long b, long *sum) These take long, but you're often calling them with ssize_t. That's _probably_ the same thing, but not necessarily. > +{ > +#if __GNUC__ > + return __builtin_saddl_overflow(a, b, sum); > +#else > + if ((a > 0 && a > LONG_MAX - b) || > + (b < 0 && a < LONG_MIN - b)) > + return true; > + > + *sum =3D a + b; > + return false; > +#endif > +} > + > +/** > + * saddl_overflow() - Subtraction with overflow check for long signed va= lues s/saddl_overflow/ssubl_overflow/ > + * @a: Minuend > + * @b: Subtrahend > + * @sum: Pointer to result of subtraction, if it doesn't overflow > + * > + * Return: true if the subtraction would overflow, false otherwise > + */ > +static inline bool ssubl_overflow(long a, long b, long *diff) > +{ > +#if __GNUC__ > + return __builtin_ssubl_overflow(a, b, diff); > +#else > + if ((b > 0 && a < LONG_MIN + b) || > + (b < 0 && a > LONG_MAX + b)) > + return true; > + > + *diff =3D a - b; > + return false; > +#endif > +} > + > /* > * Workarounds for https://github.com/llvm/llvm-project/issues/58992 > * --=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 --ztZYi9GBc4/d/fmW Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZ8vKkACgkQzQJF27ox 2Gf8mw//Uvj2tKvtKZ8TF1bnhn4FxNZrZoSzPXJ9pO1QU3C6qBpN91xXSEknW55M emRVW7L/aW0Sz0vuphG++rAiQQzUyl5b29Tw2yIXoOsRrTrln/Y+/wQqKAcMDkBz H0R4FlO10hTMHncHw3BYG7oGSJEyHkbag6z20ltPLArdafGDVlDOCxcj/YJArJ2o lLDY/VheGglvXVmkKA0X9KZ5C2yfARjT9bCUmSHL1BAm1tkrowJQFhLW66M1eibq Hm8o0aRn2N+CWlMHO8HaUll6Ma4BKL1v82GBqMhUThSTu7hYFTGrBI70EABm3d6g 2WAtiM88RtiMZqVX7WJxnDJPkwyEsn/jHUEKfIZqY6dLaEko9841HquaYKwMboYO Z/9P+I6GQoilDh3JXdWATV7C+kd0qJQnk7A52n9lJOxPulTPNn8V5r5DcaW2AOt6 KXCEbI9Yi2D5xf2PlLCV7tUlPq7uq2NQF25m7aSuzyEkkfYiLagOuuHKILtW67C3 qy8ejB1LUe7kY3gC2UqsoIR87HfRswCpGHzA/UpEFVUadgJzcq0tkIoN4rlWCjEm IT1VJEZV7yPx6f0zDRCmjzUHiFEaqmnzJ/r/NStP/0g9qNV5qWf9J62rQiVX9gvd fX1fDTJiVxSCKmc0uLnNEKUmfVBBHR/rOzKrDekFaDzOvqiSaTM= =HOTq -----END PGP SIGNATURE----- --ztZYi9GBc4/d/fmW--