[PATCH 0/4] Small, assorted "hardening" fixes

[PATCH 0/4] Small, assorted "hardening" fixes

[Stefano Brivio]

All harmless issues as far as I can tell, but nice to fix.

Stefano Brivio (4):
  conf: Copy up to MAXDNSRCH - 1 bytes, not MAXDNSRCH
  tcp_splice: Check return value of setsockopt() for SO_RCVLOWAT
  util, lineread, tap: Overflow checks on long signed sums and
    subtractions
  tap: Drop frames from guest whose length is more than remaining buffer

 conf.c       |  2 +-
 lineread.c   |  5 +++--
 tap.c        | 24 ++++++++++++++++++------
 tcp_splice.c | 15 ++++++++++-----
 util.c       |  7 +++++--
 util.h       | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 6 files changed, 82 insertions(+), 17 deletions(-)

-- 
2.43.0

[PATCH 1/4] conf: Copy up to MAXDNSRCH - 1 bytes, not MAXDNSRCH

[Stefano Brivio]

Spotted by Coverity just recently. Not that it really matters as
MAXDNSRCH always appears to be defined as 1025, while a full domain
name can have up to 253 characters: it would be a bit pointless to
have a longer search domain.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 conf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/conf.c b/conf.c
index e1f5422..9e47e9a 100644
--- a/conf.c
+++ b/conf.c
@@ -453,7 +453,7 @@ static void get_dns(struct ctx *c)
 			while (s - c->dns_search < ARRAY_SIZE(c->dns_search) - 1
 			       /* cppcheck-suppress strtokCalled */
 			       && (p = strtok(NULL, " \t"))) {
-				strncpy(s->n, p, sizeof(c->dns_search[0]));
+				strncpy(s->n, p, sizeof(c->dns_search[0]) - 1);
 				s++;
 				*s->n = 0;
 			}
-- 
@@ -453,7 +453,7 @@ static void get_dns(struct ctx *c)
 			while (s - c->dns_search < ARRAY_SIZE(c->dns_search) - 1
 			       /* cppcheck-suppress strtokCalled */
 			       && (p = strtok(NULL, " \t"))) {
-				strncpy(s->n, p, sizeof(c->dns_search[0]));
+				strncpy(s->n, p, sizeof(c->dns_search[0]) - 1);
 				s++;
 				*s->n = 0;
 			}
-- 
2.43.0

Re: [PATCH 1/4] conf: Copy up to MAXDNSRCH - 1 bytes, not MAXDNSRCH

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1336 bytes --]

On Thu, Jun 27, 2024 at 01:45:33AM +0200, Stefano Brivio wrote:
> Spotted by Coverity just recently. Not that it really matters as
> MAXDNSRCH always appears to be defined as 1025, while a full domain
> name can have up to 253 characters: it would be a bit pointless to
> have a longer search domain.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Hm.  So, IIRC strncpy() won't \0 terminate in the case where it
truncates.  I guess we'll get away with that here since we expect
c->dns_search to be filled with \0 before hand.  That's... more
fragile than ideal, though.

> ---
>  conf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/conf.c b/conf.c
> index e1f5422..9e47e9a 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -453,7 +453,7 @@ static void get_dns(struct ctx *c)
>  			while (s - c->dns_search < ARRAY_SIZE(c->dns_search) - 1
>  			       /* cppcheck-suppress strtokCalled */
>  			       && (p = strtok(NULL, " \t"))) {
> -				strncpy(s->n, p, sizeof(c->dns_search[0]));
> +				strncpy(s->n, p, sizeof(c->dns_search[0]) - 1);
>  				s++;
>  				*s->n = 0;
>  			}

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/4] conf: Copy up to MAXDNSRCH - 1 bytes, not MAXDNSRCH

[Stefano Brivio]

On Thu, 27 Jun 2024 10:45:28 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 27, 2024 at 01:45:33AM +0200, Stefano Brivio wrote:
> > Spotted by Coverity just recently. Not that it really matters as
> > MAXDNSRCH always appears to be defined as 1025, while a full domain
> > name can have up to 253 characters: it would be a bit pointless to
> > have a longer search domain.
> > 
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> 
> Hm.  So, IIRC strncpy() won't \0 terminate in the case where it
> truncates.  I guess we'll get away with that here since we expect
> c->dns_search to be filled with \0 before hand.  That's... more
> fragile than ideal, though.

Well, we know we start from a zero-initialised area, that's by design,
it's not that we get away with it. Without that consideration not many
things would work in this function.

Are you suggesting to use snprintf()? It looks a bit pedantic to me but
I'm fine with it. Otherwise, feel free to post a patch fixing it in a
way you feel it's ideal...

> > ---
> >  conf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/conf.c b/conf.c
> > index e1f5422..9e47e9a 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -453,7 +453,7 @@ static void get_dns(struct ctx *c)
> >  			while (s - c->dns_search < ARRAY_SIZE(c->dns_search) - 1
> >  			       /* cppcheck-suppress strtokCalled */
> >  			       && (p = strtok(NULL, " \t"))) {
> > -				strncpy(s->n, p, sizeof(c->dns_search[0]));
> > +				strncpy(s->n, p, sizeof(c->dns_search[0]) - 1);
> >  				s++;
> >  				*s->n = 0;
> >  			}  
> 

-- 
Stefano

Re: [PATCH 1/4] conf: Copy up to MAXDNSRCH - 1 bytes, not MAXDNSRCH

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2082 bytes --]

On Thu, Jun 27, 2024 at 09:27:01AM +0200, Stefano Brivio wrote:
> On Thu, 27 Jun 2024 10:45:28 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Jun 27, 2024 at 01:45:33AM +0200, Stefano Brivio wrote:
> > > Spotted by Coverity just recently. Not that it really matters as
> > > MAXDNSRCH always appears to be defined as 1025, while a full domain
> > > name can have up to 253 characters: it would be a bit pointless to
> > > have a longer search domain.
> > > 
> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> > 
> > Hm.  So, IIRC strncpy() won't \0 terminate in the case where it
> > truncates.  I guess we'll get away with that here since we expect
> > c->dns_search to be filled with \0 before hand.  That's... more
> > fragile than ideal, though.
> 
> Well, we know we start from a zero-initialised area, that's by design,
> it's not that we get away with it. Without that consideration not many
> things would work in this function.

That's a fair point.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> Are you suggesting to use snprintf()? It looks a bit pedantic to me but
> I'm fine with it. Otherwise, feel free to post a patch fixing it in a
> way you feel it's ideal...
> 
> > > ---
> > >  conf.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/conf.c b/conf.c
> > > index e1f5422..9e47e9a 100644
> > > --- a/conf.c
> > > +++ b/conf.c
> > > @@ -453,7 +453,7 @@ static void get_dns(struct ctx *c)
> > >  			while (s - c->dns_search < ARRAY_SIZE(c->dns_search) - 1
> > >  			       /* cppcheck-suppress strtokCalled */
> > >  			       && (p = strtok(NULL, " \t"))) {
> > > -				strncpy(s->n, p, sizeof(c->dns_search[0]));
> > > +				strncpy(s->n, p, sizeof(c->dns_search[0]) - 1);
> > >  				s++;
> > >  				*s->n = 0;
> > >  			}  
> > 
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/4] tcp_splice: Check return value of setsockopt() for SO_RCVLOWAT

[Stefano Brivio]

Spotted by Coverity, harmless as we would consider that successful
and check on the socket later from the timer, but printing a debug
message in that case is definitely wise, should it ever happen.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 tcp_splice.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/tcp_splice.c b/tcp_splice.c
index 5a406c6..f2d4fc6 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -598,11 +598,16 @@ eintr:
 			    readlen > (long)c->tcp.pipe_size / 10) {
 				int lowat = c->tcp.pipe_size / 4;
 
-				setsockopt(conn->s[fromside], SOL_SOCKET,
-					   SO_RCVLOWAT, &lowat, sizeof(lowat));
-
-				conn_flag(c, conn, lowat_set_flag);
-				conn_flag(c, conn, lowat_act_flag);
+				if (setsockopt(conn->s[fromside], SOL_SOCKET,
+					       SO_RCVLOWAT,
+					       &lowat, sizeof(lowat))) {
+					flow_trace(conn,
+						   "Setting SO_RCVLOWAT %i: %s",
+						   lowat, strerror(errno));
+				} else {
+					conn_flag(c, conn, lowat_set_flag);
+					conn_flag(c, conn, lowat_act_flag);
+				}
 			}
 
 			break;
-- 
@@ -598,11 +598,16 @@ eintr:
 			    readlen > (long)c->tcp.pipe_size / 10) {
 				int lowat = c->tcp.pipe_size / 4;
 
-				setsockopt(conn->s[fromside], SOL_SOCKET,
-					   SO_RCVLOWAT, &lowat, sizeof(lowat));
-
-				conn_flag(c, conn, lowat_set_flag);
-				conn_flag(c, conn, lowat_act_flag);
+				if (setsockopt(conn->s[fromside], SOL_SOCKET,
+					       SO_RCVLOWAT,
+					       &lowat, sizeof(lowat))) {
+					flow_trace(conn,
+						   "Setting SO_RCVLOWAT %i: %s",
+						   lowat, strerror(errno));
+				} else {
+					conn_flag(c, conn, lowat_set_flag);
+					conn_flag(c, conn, lowat_act_flag);
+				}
 			}
 
 			break;
-- 
2.43.0

Re: [PATCH 2/4] tcp_splice: Check return value of setsockopt() for SO_RCVLOWAT

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1512 bytes --]

On Thu, Jun 27, 2024 at 01:45:34AM +0200, Stefano Brivio wrote:
> Spotted by Coverity, harmless as we would consider that successful
> and check on the socket later from the timer, but printing a debug
> message in that case is definitely wise, should it ever happen.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  tcp_splice.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/tcp_splice.c b/tcp_splice.c
> index 5a406c6..f2d4fc6 100644
> --- a/tcp_splice.c
> +++ b/tcp_splice.c
> @@ -598,11 +598,16 @@ eintr:
>  			    readlen > (long)c->tcp.pipe_size / 10) {
>  				int lowat = c->tcp.pipe_size / 4;
>  
> -				setsockopt(conn->s[fromside], SOL_SOCKET,
> -					   SO_RCVLOWAT, &lowat, sizeof(lowat));
> -
> -				conn_flag(c, conn, lowat_set_flag);
> -				conn_flag(c, conn, lowat_act_flag);
> +				if (setsockopt(conn->s[fromside], SOL_SOCKET,
> +					       SO_RCVLOWAT,
> +					       &lowat, sizeof(lowat))) {
> +					flow_trace(conn,
> +						   "Setting SO_RCVLOWAT %i: %s",
> +						   lowat, strerror(errno));
> +				} else {
> +					conn_flag(c, conn, lowat_set_flag);
> +					conn_flag(c, conn, lowat_act_flag);
> +				}
>  			}
>  
>  			break;

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/4] util, lineread, tap: Overflow checks on long signed sums and subtractions

[Stefano Brivio]

Potential sum and subtraction overflows were reported by Coverity in a
few places where we use size_t and ssize_t types.

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().

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.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 lineread.c |  5 +++--
 tap.c      | 21 +++++++++++++++------
 util.c     |  7 +++++--
 util.h     | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 68 insertions(+), 11 deletions(-)

diff --git a/lineread.c b/lineread.c
index 0387f4a..12f2d24 100644
--- a/lineread.c
+++ b/lineread.c
@@ -17,6 +17,7 @@
 #include <string.h>
 #include <stdbool.h>
 #include <unistd.h>
+#include <errno.h>
 
 #include "lineread.h"
 #include "util.h"
@@ -102,8 +103,8 @@ ssize_t lineread_get(struct lineread *lr, char **line)
 
 		if (rc == 0)
 			eof = true;
-		else
-			lr->count += rc;
+		else if (saddl_overflow(lr->count, rc, &lr->count))
+			return -ERANGE;
 	}
 
 	*line = 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 = recv(c->fd_tap, p + n, l2len - n, 0);
-			if ((n += rem) != l2len)
+
+			if (saddl_overflow(n, rem, &n))
+				return;
+
+			if (n != l2len)
 				return;
 		}
 
@@ -1046,7 +1050,9 @@ redo:
 
 next:
 		p += l2len;
-		n -= l2len;
+
+		if (ssubl_overflow(n, l2len, &n))
+			return;
 	}
 
 	tap_handler(c, now);
@@ -1077,17 +1083,20 @@ redo:
 	tap_flush_pools();
 restart:
 	while ((len = 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 += len;
+			if (saddl_overflow(n, len, &n))
+				return;
+
 			continue;
 		}
 
-
 		tap_add_packet(c, len, pkt_buf + n);
 
-		if ((n += len) == TAP_BUF_BYTES)
+		if (saddl_overflow(n, len, &n))
+			return;
+
+		if (n == TAP_BUF_BYTES)
 			break;
 	}
 
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, size_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)
 {
 	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;
 
-		skip += rc;
+		if (saddl_overflow(skip, rc, &skip)) {
+			errno = -ERANGE;
+			return -1;
+		}
 	}
 
 	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);
 
 /**
  * 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);
 }
 
+/**
+ * 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)
+{
+#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 = a + b;
+	return false;
+#endif
+}
+
+/**
+ * saddl_overflow() - Subtraction with overflow check for long signed values
+ * @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 = a - b;
+	return false;
+#endif
+}
+
 /*
  * Workarounds for https://github.com/llvm/llvm-project/issues/58992
  *
-- 
@@ -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);
 
 /**
  * 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);
 }
 
+/**
+ * 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)
+{
+#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 = a + b;
+	return false;
+#endif
+}
+
+/**
+ * saddl_overflow() - Subtraction with overflow check for long signed values
+ * @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 = a - b;
+	return false;
+#endif
+}
+
 /*
  * Workarounds for https://github.com/llvm/llvm-project/issues/58992
  *
-- 
2.43.0

Re: [PATCH 3/4] util, lineread, tap: Overflow checks on long signed sums and subtractions

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6854 bytes --]

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.
> 
> 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.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  lineread.c |  5 +++--
>  tap.c      | 21 +++++++++++++++------
>  util.c     |  7 +++++--
>  util.h     | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 68 insertions(+), 11 deletions(-)
> 
> diff --git a/lineread.c b/lineread.c
> index 0387f4a..12f2d24 100644
> --- a/lineread.c
> +++ b/lineread.c
> @@ -17,6 +17,7 @@
>  #include <string.h>
>  #include <stdbool.h>
>  #include <unistd.h>
> +#include <errno.h>
>  
>  #include "lineread.h"
>  #include "util.h"
> @@ -102,8 +103,8 @@ ssize_t lineread_get(struct lineread *lr, char **line)
>  
>  		if (rc == 0)
>  			eof = true;
> -		else
> -			lr->count += rc;

From 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;
>  	}
>  
>  	*line = 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 = recv(c->fd_tap, p + n, l2len - n, 0);
> -			if ((n += rem) != l2len)

Similarly, rem <= l2len - n, and therefore n + rem <= l2len.

> +
> +			if (saddl_overflow(n, rem, &n))
> +				return;
> +
> +			if (n != l2len)
>  				return;
>  		}
>  
> @@ -1046,7 +1050,9 @@ redo:
>  
>  next:
>  		p += l2len;
> -		n -= l2len;

We already checked that l2len <= 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;
>  	}
>  
>  	tap_handler(c, now);
> @@ -1077,17 +1083,20 @@ redo:
>  	tap_flush_pools();
>  restart:
>  	while ((len = 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 += len;

Here n+len can't exceed TAP_BUF_BYTES, so again, no overflow.

> +			if (saddl_overflow(n, len, &n))
> +				return;
> +
>  			continue;
>  		}
>  
> -
>  		tap_add_packet(c, len, pkt_buf + n);
>  
> -		if ((n += len) == TAP_BUF_BYTES)

Same here.

> +		if (saddl_overflow(n, len, &n))
> +			return;
> +
> +		if (n == TAP_BUF_BYTES)
>  			break;
>  	}
>  
> 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, size_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;
>  
> -		skip += 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 = -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().

>  	}
>  
>  	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);
>  
>  /**
>   * 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);
>  }
>  
> +/**
> + * 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 = a + b;
> +	return false;
> +#endif
> +}
> +
> +/**
> + * saddl_overflow() - Subtraction with overflow check for long signed values

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 = a - b;
> +	return false;
> +#endif
> +}
> +
>  /*
>   * Workarounds for https://github.com/llvm/llvm-project/issues/58992
>   *

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/4] util, lineread, tap: Overflow checks on long signed sums and subtractions

[Stefano Brivio]

On Thu, 27 Jun 2024 11:13:14 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> 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.
> > 
> > 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.

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.

In general, static checkers do, especially if POSIX has a clear
definition of a system call, and for what matters to us, they should.

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.

> So possibly just ASSERT()ing that would suffice.

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.

> > 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.
> > 
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> >  lineread.c |  5 +++--
> >  tap.c      | 21 +++++++++++++++------
> >  util.c     |  7 +++++--
> >  util.h     | 46 +++++++++++++++++++++++++++++++++++++++++++++-
> >  4 files changed, 68 insertions(+), 11 deletions(-)
> > 
> > diff --git a/lineread.c b/lineread.c
> > index 0387f4a..12f2d24 100644
> > --- a/lineread.c
> > +++ b/lineread.c
> > @@ -17,6 +17,7 @@
> >  #include <string.h>
> >  #include <stdbool.h>
> >  #include <unistd.h>
> > +#include <errno.h>
> >  
> >  #include "lineread.h"
> >  #include "util.h"
> > @@ -102,8 +103,8 @@ ssize_t lineread_get(struct lineread *lr, char **line)
> >  
> >  		if (rc == 0)
> >  			eof = true;
> > -		else
> > -			lr->count += rc;  
> 
> From the construction of the read, lr->count + rc can never exceed
> LINEREAD_BUFFER_SIZE - lr->next_line, so this can't overflow.

Sure. But, especially as package maintainer, in this case I prefer to
have a useless check than carrying suppressions around.

> > +		else if (saddl_overflow(lr->count, rc, &lr->count))
> > +			return -ERANGE;
> >  	}
> >  
> >  	*line = 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 = recv(c->fd_tap, p + n, l2len - n, 0);
> > -			if ((n += rem) != l2len)  
> 
> Similarly, rem <= l2len - n, and therefore n + rem <= l2len.

Same here.

> > +
> > +			if (saddl_overflow(n, rem, &n))
> > +				return;
> > +
> > +			if (n != l2len)
> >  				return;
> >  		}
> >  
> > @@ -1046,7 +1050,9 @@ redo:
> >  
> >  next:
> >  		p += l2len;
> > -		n -= l2len;  
> 
> We already checked that l2len <= n, so this one can't overflow either.

Same here.

> 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.

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.

> > +
> > +		if (ssubl_overflow(n, l2len, &n))
> > +			return;
> >  	}
> >  
> >  	tap_handler(c, now);
> > @@ -1077,17 +1083,20 @@ redo:
> >  	tap_flush_pools();
> >  restart:
> >  	while ((len = 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 += len;  
> 
> Here n+len can't exceed TAP_BUF_BYTES, so again, no overflow.

Same here.

> > +			if (saddl_overflow(n, len, &n))
> > +				return;
> > +
> >  			continue;
> >  		}
> >  
> > -
> >  		tap_add_packet(c, len, pkt_buf + n);
> >  
> > -		if ((n += len) == TAP_BUF_BYTES)  
> 
> Same here.

Same here.

> > +		if (saddl_overflow(n, len, &n))
> > +			return;
> > +
> > +		if (n == TAP_BUF_BYTES)
> >  			break;
> >  	}
> >  
> > 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, size_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;
> >  
> > -		skip += 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 = -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().

I'm fairly sure I tried that and it looked rather bulky, because I
couldn't use __builtin_uaddl_overflow() if I recall correctly, I can try
again.

> >  	}
> >  
> >  	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);
> >  
> >  /**
> >   * 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);
> >  }
> >  
> > +/**
> > + * 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.

Right, yes, ssize_t can be long or int, even though I'm fairly sure it's
always long on all the architectures we are able to build for.

There's no integer overflow built-in for ssize_t, but I'll probably
need to add a macro conditional for the whole thing anyway, based on
the type of ssize_t.

> > +{
> > +#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 = a + b;
> > +	return false;
> > +#endif
> > +}
> > +
> > +/**
> > + * saddl_overflow() - Subtraction with overflow check for long signed values  
> 
> s/saddl_overflow/ssubl_overflow/

Oops, fixed.

> > + * @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 = a - b;
> > +	return false;
> > +#endif
> > +}
> > +
> >  /*
> >   * Workarounds for https://github.com/llvm/llvm-project/issues/58992
> >   *  
> 

-- 
Stefano

Re: [PATCH 3/4] util, lineread, tap: Overflow checks on long signed sums and subtractions

[Stefano Brivio]

On Thu, 27 Jun 2024 09:55:46 +0200
Stefano Brivio <sbrivio@redhat.com> wrote:

> On Thu, 27 Jun 2024 11:13:14 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > 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.
> > > 
> > > 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.  
> 
> 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.
> 
> In general, static checkers do, especially if POSIX has a clear
> definition of a system call, and for what matters to us, they should.
> 
> 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.
> 
> > So possibly just ASSERT()ing that would suffice.  
> 
> 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.

I tried, but in most cases, not even open-coding the whole thing, as an
ASSERT() or an early return, say:

			if ((rc > 0 && lr->count < LONG_MIN + rc) ||
			    (rc < 0 && lr->count > LONG_MAX + rc))
				return -1;

or its ASSERT() equivalent is enough. That's because, without using
built-in functions, we can't have (of course) "infinite precision"
types. From:

  https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html

  These built-in functions promote the first two operands into infinite
  precision signed type and perform addition on those promoted operands

And it looks like Coverity wants to see operations executed in those
types: size_t or unsigned long long is not enough.

In two cases, I could make Coverity happy with some rather bulky
asserts, if I turn operations into size_t plus ssize_t, in size_t
domain. But the result looks really bulky.

I managed to change a couple of things, though:

> > > 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.
> > > 
> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > ---
> > >  lineread.c |  5 +++--
> > >  tap.c      | 21 +++++++++++++++------
> > >  util.c     |  7 +++++--
> > >  util.h     | 46 +++++++++++++++++++++++++++++++++++++++++++++-
> > >  4 files changed, 68 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/lineread.c b/lineread.c
> > > index 0387f4a..12f2d24 100644
> > > --- a/lineread.c
> > > +++ b/lineread.c
> > > @@ -17,6 +17,7 @@
> > >  #include <string.h>
> > >  #include <stdbool.h>
> > >  #include <unistd.h>
> > > +#include <errno.h>
> > >  
> > >  #include "lineread.h"
> > >  #include "util.h"
> > > @@ -102,8 +103,8 @@ ssize_t lineread_get(struct lineread *lr, char **line)
> > >  
> > >  		if (rc == 0)
> > >  			eof = true;
> > > -		else
> > > -			lr->count += rc;    
> > 
> > From the construction of the read, lr->count + rc can never exceed
> > LINEREAD_BUFFER_SIZE - lr->next_line, so this can't overflow.  
> 
> Sure. But, especially as package maintainer, in this case I prefer to
> have a useless check than carrying suppressions around.
> 
> > > +		else if (saddl_overflow(lr->count, rc, &lr->count))
> > > +			return -ERANGE;
> > >  	}
> > >  
> > >  	*line = 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 = recv(c->fd_tap, p + n, l2len - n, 0);
> > > -			if ((n += rem) != l2len)    
> > 
> > Similarly, rem <= l2len - n, and therefore n + rem <= l2len.  
> 
> Same here.
> 
> > > +
> > > +			if (saddl_overflow(n, rem, &n))
> > > +				return;
> > > +
> > > +			if (n != l2len)
> > >  				return;
> > >  		}
> > >  
> > > @@ -1046,7 +1050,9 @@ redo:
> > >  
> > >  next:
> > >  		p += l2len;
> > > -		n -= l2len;    
> > 
> > We already checked that l2len <= n, so this one can't overflow either.  
> 
> Same here.
> 
> > 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.  
> 
> 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.
> 
> > > +
> > > +		if (ssubl_overflow(n, l2len, &n))
> > > +			return;
> > >  	}
> > >  
> > >  	tap_handler(c, now);
> > > @@ -1077,17 +1083,20 @@ redo:
> > >  	tap_flush_pools();
> > >  restart:
> > >  	while ((len = 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 += len;    
> > 
> > Here n+len can't exceed TAP_BUF_BYTES, so again, no overflow.  
> 
> Same here.
> 
> > > +			if (saddl_overflow(n, len, &n))
> > > +				return;
> > > +
> > >  			continue;
> > >  		}
> > >  
> > > -
> > >  		tap_add_packet(c, len, pkt_buf + n);
> > >  
> > > -		if ((n += len) == TAP_BUF_BYTES)    
> > 
> > Same here.  
> 
> Same here.
> 
> > > +		if (saddl_overflow(n, len, &n))
> > > +			return;
> > > +
> > > +		if (n == TAP_BUF_BYTES)
> > >  			break;
> > >  	}
> > >  
> > > 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, size_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.

Dropped, by:

> > >  {
> > >  	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;
> > >  
> > > -		skip += 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 = -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().  
> 
> I'm fairly sure I tried that and it looked rather bulky, because I
> couldn't use __builtin_uaddl_overflow() if I recall correctly, I can try
> again.

...checking for overflow in the unsigned sum, without any built-in. Same
lines of code, but the logic is simpler.

> > >  	}
> > >  
> > >  	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);
> > >  
> > >  /**
> > >   * 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);
> > >  }
> > >  
> > > +/**
> > > + * 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.  
> 
> Right, yes, ssize_t can be long or int, even though I'm fairly sure it's
> always long on all the architectures we are able to build for.
> 
> There's no integer overflow built-in for ssize_t, but I'll probably
> need to add a macro conditional for the whole thing anyway, based on
> the type of ssize_t.

It was a bit simpler than I thought: first off, gcc, Clang and icc all
have __builtin_{add,sub}_overflow(type1 a, type2 b, type3 res).

If those built-ins are not available, all we need to check is what
SSIZE_MIN corresponds to. Now, glibc goes with a __WORDSIZE == 32 to
find that out, but it doesn't look very portable. Checking if SSIZE_MAX
is LONG_MAX, instead, should be.

> > > +{
> > > +#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 = a + b;
> > > +	return false;
> > > +#endif
> > > +}
> > > +
> > > +/**
> > > + * saddl_overflow() - Subtraction with overflow check for long signed values    
> > 
> > s/saddl_overflow/ssubl_overflow/  
> 
> Oops, fixed.
> 
> > > + * @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 = a - b;
> > > +	return false;
> > > +#endif
> > > +}
> > > +
> > >  /*
> > >   * Workarounds for https://github.com/llvm/llvm-project/issues/58992
> > >   *    
> >   
> 

-- 
Stefano

Re: [PATCH 3/4] util, lineread, tap: Overflow checks on long signed sums and subtractions

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3549 bytes --]

On Thu, Jun 27, 2024 at 10:46:45PM +0200, Stefano Brivio wrote:
> On Thu, 27 Jun 2024 09:55:46 +0200
> Stefano Brivio <sbrivio@redhat.com> wrote:
> 
> > On Thu, 27 Jun 2024 11:13:14 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > 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.
> > > > 
> > > > 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.  
> > 
> > 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.
> > 
> > In general, static checkers do, especially if POSIX has a clear
> > definition of a system call, and for what matters to us, they should.
> > 
> > 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.
> > 
> > > So possibly just ASSERT()ing that would suffice.  
> > 
> > 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.
> 
> I tried, but in most cases, not even open-coding the whole thing, as an
> ASSERT() or an early return, say:
> 
> 			if ((rc > 0 && lr->count < LONG_MIN + rc) ||
> 			    (rc < 0 && lr->count > LONG_MAX + rc))
> 				return -1;

I'm not talking about open-coding a general overflow checking add/sub,
I'm talking about checking the much tighter range we actually have.
That should let Coverity reason that an overflow isn't possible, and
also check for other possible weirdness.

> or its ASSERT() equivalent is enough. That's because, without using
> built-in functions, we can't have (of course) "infinite precision"
> types. From:
> 
>   https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html
> 
>   These built-in functions promote the first two operands into infinite
>   precision signed type and perform addition on those promoted operands
> 
> And it looks like Coverity wants to see operations executed in those
> types: size_t or unsigned long long is not enough.
> 
> In two cases, I could make Coverity happy with some rather bulky
> asserts, if I turn operations into size_t plus ssize_t, in size_t
> domain. But the result looks really bulky.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/4] util, lineread, tap: Overflow checks on long signed sums and subtractions

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6160 bytes --]

On Thu, Jun 27, 2024 at 09:55:46AM +0200, Stefano Brivio wrote:
> On Thu, 27 Jun 2024 11:13:14 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > 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.
> > > 
> > > 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.
> 
> 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.
> 
> > So possibly just ASSERT()ing that would suffice.
> 
> 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.
> > > 
> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > ---
> > >  lineread.c |  5 +++--
> > >  tap.c      | 21 +++++++++++++++------
> > >  util.c     |  7 +++++--
> > >  util.h     | 46 +++++++++++++++++++++++++++++++++++++++++++++-
> > >  4 files changed, 68 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/lineread.c b/lineread.c
> > > index 0387f4a..12f2d24 100644
> > > --- a/lineread.c
> > > +++ b/lineread.c
> > > @@ -17,6 +17,7 @@
> > >  #include <string.h>
> > >  #include <stdbool.h>
> > >  #include <unistd.h>
> > > +#include <errno.h>
> > >  
> > >  #include "lineread.h"
> > >  #include "util.h"
> > > @@ -102,8 +103,8 @@ ssize_t lineread_get(struct lineread *lr, char **line)
> > >  
> > >  		if (rc == 0)
> > >  			eof = true;
> > > -		else
> > > -			lr->count += rc;  
> > 
> > From the construction of the read, lr->count + rc can never exceed
> > LINEREAD_BUFFER_SIZE - lr->next_line, so this can't overflow.
> 
> 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.

> 
> > > +		else if (saddl_overflow(lr->count, rc, &lr->count))
> > > +			return -ERANGE;
> > >  	}
> > >  
> > >  	*line = 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 = recv(c->fd_tap, p + n, l2len - n, 0);
> > > -			if ((n += rem) != l2len)  
> > 
> > Similarly, rem <= l2len - n, and therefore n + rem <= l2len.
> 
> Same here.
> 
> > > +
> > > +			if (saddl_overflow(n, rem, &n))
> > > +				return;
> > > +
> > > +			if (n != l2len)
> > >  				return;
> > >  		}
> > >  
> > > @@ -1046,7 +1050,9 @@ redo:
> > >  
> > >  next:
> > >  		p += l2len;
> > > -		n -= l2len;  
> > 
> > We already checked that l2len <= n, so this one can't overflow either.
> 
> Same here.
> 
> > 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.
> 
> 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".

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/4] util, lineread, tap: Overflow checks on long signed sums and subtractions

[Stefano Brivio]

On Fri, 28 Jun 2024 17:11:43 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 27, 2024 at 09:55:46AM +0200, Stefano Brivio wrote:
> > On Thu, 27 Jun 2024 11:13:14 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > 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.
> > > > 
> > > > 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.  
> > 
> > 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.
> >   
> > > So possibly just ASSERT()ing that would suffice.  
> > 
> > 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

Sure, that bothers me as well. I'm not sure: would comments improve that? Say:

	/* n += len; */
	if (sadd_overflow(n, len, &n))
		return;

or a different macro? Or an ASSERT() on sadd_overflow() itself, so that
it doesn't really look "inline"?

>    - it introduces extra error paths which make it harder to see what
>      the code's doing

We already have equivalent error paths in all the cases I'm touching
here, though.

>    - 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 wouldn't put them on every single operation. Again, it's clear that
in all these cases, we *can't* hit those overflows. Not even in the
write_remainder() case 

I would just continue with the only possible reasonable approach, which
is, reasoning about which ranges things can have, and then test things
with static checkers, and if they have false positives, well, a bit of
cruft here and there (be it suppressions or redundant checks) is a
very reasonable price to pay for what they offer, I think.

> 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.
> > > > 
> > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > > ---
> > > >  lineread.c |  5 +++--
> > > >  tap.c      | 21 +++++++++++++++------
> > > >  util.c     |  7 +++++--
> > > >  util.h     | 46 +++++++++++++++++++++++++++++++++++++++++++++-
> > > >  4 files changed, 68 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/lineread.c b/lineread.c
> > > > index 0387f4a..12f2d24 100644
> > > > --- a/lineread.c
> > > > +++ b/lineread.c
> > > > @@ -17,6 +17,7 @@
> > > >  #include <string.h>
> > > >  #include <stdbool.h>
> > > >  #include <unistd.h>
> > > > +#include <errno.h>
> > > >  
> > > >  #include "lineread.h"
> > > >  #include "util.h"
> > > > @@ -102,8 +103,8 @@ ssize_t lineread_get(struct lineread *lr, char **line)
> > > >  
> > > >  		if (rc == 0)
> > > >  			eof = true;
> > > > -		else
> > > > -			lr->count += rc;    
> > > 
> > > From the construction of the read, lr->count + rc can never exceed
> > > LINEREAD_BUFFER_SIZE - lr->next_line, so this can't overflow.  
> > 
> > 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.

Well, I tried (of course...), along with dozens of other attempts, but
it didn't work for any of these cases. For example, here, other than an
ASSERT() on the return value of the read(), I also tried stuff like:

	ASSERT(lr->count < SSIZE_MAX && rc < SSIZE_MAX)
	lr->count += (size_t)rc;

but no, it doesn't work. I think what Coverity wants to see is the sum
tested in infinite precision, first.

> > > > +		else if (saddl_overflow(lr->count, rc, &lr->count))
> > > > +			return -ERANGE;
> > > >  	}
> > > >  
> > > >  	*line = 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 = recv(c->fd_tap, p + n, l2len - n, 0);
> > > > -			if ((n += rem) != l2len)    
> > > 
> > > Similarly, rem <= l2len - n, and therefore n + rem <= l2len.  
> > 
> > Same here.
> >   
> > > > +
> > > > +			if (saddl_overflow(n, rem, &n))
> > > > +				return;
> > > > +
> > > > +			if (n != l2len)
> > > >  				return;
> > > >  		}
> > > >  
> > > > @@ -1046,7 +1050,9 @@ redo:
> > > >  
> > > >  next:
> > > >  		p += l2len;
> > > > -		n -= l2len;    
> > > 
> > > We already checked that l2len <= n, so this one can't overflow either.  
> > 
> > Same here.
> >   
> > > 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.  
> > 
> > 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.

It doesn't.

> 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".

We do a lot of signed arithmetic, and yet, just those five cases are
problematic. I guess there's something peculiar with system calls
return values, or with ssize_t / size_t, even. Maybe I could try to
show Coverity a re-definition of those types... other than that I'm
pretty much out of ideas.

-- 
Stefano

Re: [PATCH 3/4] util, lineread, tap: Overflow checks on long signed sums and subtractions

[Stefano Brivio]

On Fri, 28 Jun 2024 09:55:18 +0200
Stefano Brivio <sbrivio@redhat.com> wrote:

> On Fri, 28 Jun 2024 17:11:43 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Jun 27, 2024 at 09:55:46AM +0200, Stefano Brivio wrote:  
> > > On Thu, 27 Jun 2024 11:13:14 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >     
> > > > 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.
> > > > > 
> > > > > 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.    
> > > 
> > > 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.
> > >     
> > > > So possibly just ASSERT()ing that would suffice.    
> > > 
> > > 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  
> 
> Sure, that bothers me as well. I'm not sure: would comments improve that? Say:
> 
> 	/* n += len; */
> 	if (sadd_overflow(n, len, &n))
> 		return;
> 
> or a different macro? Or an ASSERT() on sadd_overflow() itself, so that
> it doesn't really look "inline"?

...that doesn't "work" either, which is rather puzzling. With this
diff, on top of this series:

diff --git a/lineread.c b/lineread.c
index f72a14e..82f262a 100644
--- a/lineread.c
+++ b/lineread.c
@@ -77,6 +77,7 @@ ssize_t lineread_get(struct lineread *lr, char **line)
 {
 	bool eof = false;
 	ssize_t line_len;
+	ssize_t test;
 
 	while ((line_len = peek_line(lr, eof)) < 0) {
 		ssize_t rc;
@@ -103,8 +104,10 @@ ssize_t lineread_get(struct lineread *lr, char
**line) 
 		if (rc == 0)
 			eof = true;
-		else if (sadd_overflow(lr->count, rc, &lr->count))
-			return -ERANGE;
+		else {
+			ASSERT(!sadd_overflow(lr->count, rc, &test));
+			lr->count += rc;
+		}
 	}
 
 	*line = lr->buf + lr->next_line;

I get these "events":

  Called function "read(lr->fd, lr->buf + lr->next_line + lr->count, 8192L - lr->next_line - lr->count)", and a possible return value may be less than zero.
  Assigning: "rc" = "read(lr->fd, lr->buf + lr->next_line + lr->count, 8192L - lr->next_line - lr->count)".

around read(), then:

  Condition "!!__builtin_add_overflow(lr->count, rc, &test)", taking false branch.

and at the sum:

  The expression "lr->count" is considered to have possibly overflowed.

More attempts below:

> >    - it introduces extra error paths which make it harder to see what
> >      the code's doing  
> 
> We already have equivalent error paths in all the cases I'm touching
> here, though.
> 
> >    - 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 wouldn't put them on every single operation. Again, it's clear that
> in all these cases, we *can't* hit those overflows. Not even in the
> write_remainder() case 
> 
> I would just continue with the only possible reasonable approach, which
> is, reasoning about which ranges things can have, and then test things
> with static checkers, and if they have false positives, well, a bit of
> cruft here and there (be it suppressions or redundant checks) is a
> very reasonable price to pay for what they offer, I think.
> 
> > 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.
> > > > > 
> > > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > > > ---
> > > > >  lineread.c |  5 +++--
> > > > >  tap.c      | 21 +++++++++++++++------
> > > > >  util.c     |  7 +++++--
> > > > >  util.h     | 46 +++++++++++++++++++++++++++++++++++++++++++++-
> > > > >  4 files changed, 68 insertions(+), 11 deletions(-)
> > > > > 
> > > > > diff --git a/lineread.c b/lineread.c
> > > > > index 0387f4a..12f2d24 100644
> > > > > --- a/lineread.c
> > > > > +++ b/lineread.c
> > > > > @@ -17,6 +17,7 @@
> > > > >  #include <string.h>
> > > > >  #include <stdbool.h>
> > > > >  #include <unistd.h>
> > > > > +#include <errno.h>
> > > > >  
> > > > >  #include "lineread.h"
> > > > >  #include "util.h"
> > > > > @@ -102,8 +103,8 @@ ssize_t lineread_get(struct lineread *lr, char **line)
> > > > >  
> > > > >  		if (rc == 0)
> > > > >  			eof = true;
> > > > > -		else
> > > > > -			lr->count += rc;      
> > > > 
> > > > From the construction of the read, lr->count + rc can never exceed
> > > > LINEREAD_BUFFER_SIZE - lr->next_line, so this can't overflow.    
> > > 
> > > 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.  
> 
> Well, I tried (of course...), along with dozens of other attempts, but
> it didn't work for any of these cases. For example, here, other than an
> ASSERT() on the return value of the read(), I also tried stuff like:
> 
> 	ASSERT(lr->count < SSIZE_MAX && rc < SSIZE_MAX)
> 	lr->count += (size_t)rc;
> 
> but no, it doesn't work. I think what Coverity wants to see is the sum
> tested in infinite precision, first.

I also tried to check the return value of the read() call in the most
obvious way, that is, just after read():

		ASSERT(rc <= LINEREAD_BUFFER_SIZE - lr->next_line - lr->count);

but lr->count is still reported as potentially overflowed. I think the
reason is that the subtraction in the ASSERT() itself is prone to
overflow.

Anyway, given that both rc and lr->count are ssize_t, the condition of
this ASSERT() can't overflow:

		ASSERT((size_t)rc + lr->count < SSIZE_MAX);

but at this point Coverity says:

  The check "(size_t)rc + lr->count < 9223372036854775807UL", which appears to be a guard against integer overflow, is not a useful guard because it is either always true, or never true. This taints "rc".

And simpler, rather obvious stuff like:

		ASSERT(rc <= LINEREAD_BUFFER_SIZE);
		ASSERT(lr->count <= LINEREAD_BUFFER_SIZE);

doesn't work either.

I think this proves that Coverity really isn't happy unless the sum
itself happens in infinite precision (not even size_t domain with
ssize_t operands is enough).

So, well, I can report the false positive, unless you have further
ideas to check.

Meanwhile, we can either try to make this patch more acceptable, or
I'll suppress checks (downstream) as needed.

> > > > > +		else if (saddl_overflow(lr->count, rc, &lr->count))
> > > > > +			return -ERANGE;
> > > > >  	}
> > > > >  
> > > > >  	*line = 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 = recv(c->fd_tap, p + n, l2len - n, 0);
> > > > > -			if ((n += rem) != l2len)      
> > > > 
> > > > Similarly, rem <= l2len - n, and therefore n + rem <= l2len.    
> > > 
> > > Same here.
> > >     
> > > > > +
> > > > > +			if (saddl_overflow(n, rem, &n))
> > > > > +				return;
> > > > > +
> > > > > +			if (n != l2len)
> > > > >  				return;
> > > > >  		}
> > > > >  
> > > > > @@ -1046,7 +1050,9 @@ redo:
> > > > >  
> > > > >  next:
> > > > >  		p += l2len;
> > > > > -		n -= l2len;      
> > > > 
> > > > We already checked that l2len <= n, so this one can't overflow either.    
> > > 
> > > Same here.
> > >     
> > > > 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.    
> > > 
> > > 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.  
> 
> It doesn't.
> 
> > 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".  
> 
> We do a lot of signed arithmetic, and yet, just those five cases are
> problematic. I guess there's something peculiar with system calls
> return values, or with ssize_t / size_t, even. Maybe I could try to
> show Coverity a re-definition of those types... other than that I'm
> pretty much out of ideas.

-- 
Stefano

Re: [PATCH 3/4] util, lineread, tap: Overflow checks on long signed sums and subtractions

[Stefano Brivio]

On Fri, 28 Jun 2024 20:30:54 +0200
Stefano Brivio <sbrivio@redhat.com> wrote:

> I think this proves that Coverity really isn't happy unless the sum
> itself happens in infinite precision (not even size_t domain with
> ssize_t operands is enough).
> 
> So, well, I can report the false positive, unless you have further
> ideas to check.
> 
> Meanwhile, we can either try to make this patch more acceptable, or
> I'll suppress checks (downstream) as needed.

By the way, somewhat on the subject:
  https://lore.kernel.org/all/202404291502.612E0A10@keescook/#r
  https://lwn.net/ml/linux-kernel/202404291502.612E0A10@keescook/
  (I read it from my client, but I can't decide what web interface I
  would otherwise like the most...)

-- 
Stefano

[PATCH 4/4] tap: Drop frames from guest whose length is more than remaining buffer

[Stefano Brivio]

This was reported by Matej a while ago, but we forgot to fix it. Even
if the hypervisor is necessarily trusted by passt, as it can in any
case terminate the guest or disrupt guest connectivity, it's a good
idea to be robust against possible issues.

Instead of resetting the connection to the hypervisor, just skip the
offending frame, as we had a few cases where QEMU would get the
length descriptor wrong, in the past.

Reported-by: Matej Hrica <mhrica@redhat.com>
Suggested-by: Matej Hrica <mhrica@redhat.com>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 tap.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tap.c b/tap.c
index 7f8c26d..bb993e0 100644
--- a/tap.c
+++ b/tap.c
@@ -1026,6 +1026,9 @@ redo:
 		p += sizeof(uint32_t);
 		n -= sizeof(uint32_t);
 
+		if (l2len > (ssize_t)TAP_BUF_BYTES - n)
+			goto next;
+
 		/* At most one packet might not fit in a single read, and this
 		 * needs to be blocking.
 		 */
-- 
@@ -1026,6 +1026,9 @@ redo:
 		p += sizeof(uint32_t);
 		n -= sizeof(uint32_t);
 
+		if (l2len > (ssize_t)TAP_BUF_BYTES - n)
+			goto next;
+
 		/* At most one packet might not fit in a single read, and this
 		 * needs to be blocking.
 		 */
-- 
2.43.0

Re: [PATCH 4/4] tap: Drop frames from guest whose length is more than remaining buffer

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2276 bytes --]

On Thu, Jun 27, 2024 at 01:45:36AM +0200, Stefano Brivio wrote:
> This was reported by Matej a while ago, but we forgot to fix it. Even
> if the hypervisor is necessarily trusted by passt, as it can in any
> case terminate the guest or disrupt guest connectivity, it's a good
> idea to be robust against possible issues.
> 
> Instead of resetting the connection to the hypervisor, just skip the
> offending frame, as we had a few cases where QEMU would get the
> length descriptor wrong, in the past.
> 
> Reported-by: Matej Hrica <mhrica@redhat.com>
> Suggested-by: Matej Hrica <mhrica@redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  tap.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tap.c b/tap.c
> index 7f8c26d..bb993e0 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1026,6 +1026,9 @@ redo:

So.. I just realised there's a different pre-existing problem here,
above what's quoted in the patch we have:

		ssize_t l2len = ntohl(*(uint32_t *)p);

On a platform where ssize_t is only 32-bits we could get a negative
value here, which would be very bad.  So l2len should be a uint32_t,
not ssize_t.  We do then need to make sure that the comparison between
l2len and n is unsigned - it's safe to cast n to size_t there, because
we've verified it's positive as the loop condition.

Or... maybe it's simpler.  The frame length is encoded as 32-bits, but
we can't meaningfully have frames above 64k (maybe 64k+ETH_HLEN).  So
possibly we should just reset the tap connection if we see such a
frame (most likely it means we've somehow gotten out of sync, anyway).

>  		p += sizeof(uint32_t);
>  		n -= sizeof(uint32_t);


> +		if (l2len > (ssize_t)TAP_BUF_BYTES - n)

I hate to discard valid frames from the guest.

> +			goto next;

..and this is not safe.  This skips (l2len > n) check, which means
that the n -= l2len at next could now have a signed overflow, which is
UB.

> +
>  		/* At most one packet might not fit in a single read, and this
>  		 * needs to be blocking.
>  		 */

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/4] tap: Drop frames from guest whose length is more than remaining buffer

[Stefano Brivio]

On Thu, 27 Jun 2024 11:30:02 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 27, 2024 at 01:45:36AM +0200, Stefano Brivio wrote:
> > This was reported by Matej a while ago, but we forgot to fix it. Even
> > if the hypervisor is necessarily trusted by passt, as it can in any
> > case terminate the guest or disrupt guest connectivity, it's a good
> > idea to be robust against possible issues.
> > 
> > Instead of resetting the connection to the hypervisor, just skip the
> > offending frame, as we had a few cases where QEMU would get the
> > length descriptor wrong, in the past.
> > 
> > Reported-by: Matej Hrica <mhrica@redhat.com>
> > Suggested-by: Matej Hrica <mhrica@redhat.com>
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> >  tap.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/tap.c b/tap.c
> > index 7f8c26d..bb993e0 100644
> > --- a/tap.c
> > +++ b/tap.c
> > @@ -1026,6 +1026,9 @@ redo:  
> 
> So.. I just realised there's a different pre-existing problem here,
> above what's quoted in the patch we have:
> 
> 		ssize_t l2len = ntohl(*(uint32_t *)p);
> 
> On a platform where ssize_t is only 32-bits we could get a negative
> value here, which would be very bad.

We can't, because the length is anyway limited to the maximum IPv4 path
MTU in any hypervisor we might be talking to.

> So l2len should be a uint32_t, not ssize_t.

True, I can also change that while at it.

> We do then need to make sure that the comparison between
> l2len and n is unsigned - it's safe to cast n to size_t there, because
> we've verified it's positive as the loop condition.
>
> Or... maybe it's simpler.  The frame length is encoded as 32-bits, but
> we can't meaningfully have frames above 64k (maybe 64k+ETH_HLEN).  So
> possibly we should just reset the tap connection if we see such a
> frame (most likely it means we've somehow gotten out of sync, anyway).

I'd rather "fix" type and comparison, for the sake of whatever static
checkers might eventually come up with.

> >  		p += sizeof(uint32_t);
> >  		n -= sizeof(uint32_t);  
> 
> 
> > +		if (l2len > (ssize_t)TAP_BUF_BYTES - n)  
> 
> I hate to discard valid frames from the guest.

That won't happen because of how TAP_BUF_FILL and TAP_BUF_BYTES are
defined. If this condition is true, the length descriptor actually
mismatched. If you have a better proposal, let me know.

> > +			goto next;  
> 
> ..and this is not safe.  This skips (l2len > n) check, which means
> that the n -= l2len at next could now have a signed overflow, which is
> UB.

It's safe because of the change from 3/4, which will just return on
overflow. In any case, yes, I can add a return directly, it's not very
productive to speculate what kind of issues a hypervisor might have,
let's just avoid crashing in case.

> > +
> >  		/* At most one packet might not fit in a single read, and this
> >  		 * needs to be blocking.
> >  		 */  
> 

-- 
Stefano

Re: [PATCH 4/4] tap: Drop frames from guest whose length is more than remaining buffer

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3909 bytes --]

On Thu, Jun 27, 2024 at 10:21:04AM +0200, Stefano Brivio wrote:
> On Thu, 27 Jun 2024 11:30:02 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Jun 27, 2024 at 01:45:36AM +0200, Stefano Brivio wrote:
> > > This was reported by Matej a while ago, but we forgot to fix it. Even
> > > if the hypervisor is necessarily trusted by passt, as it can in any
> > > case terminate the guest or disrupt guest connectivity, it's a good
> > > idea to be robust against possible issues.
> > > 
> > > Instead of resetting the connection to the hypervisor, just skip the
> > > offending frame, as we had a few cases where QEMU would get the
> > > length descriptor wrong, in the past.
> > > 
> > > Reported-by: Matej Hrica <mhrica@redhat.com>
> > > Suggested-by: Matej Hrica <mhrica@redhat.com>
> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > ---
> > >  tap.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/tap.c b/tap.c
> > > index 7f8c26d..bb993e0 100644
> > > --- a/tap.c
> > > +++ b/tap.c
> > > @@ -1026,6 +1026,9 @@ redo:  
> > 
> > So.. I just realised there's a different pre-existing problem here,
> > above what's quoted in the patch we have:
> > 
> > 		ssize_t l2len = ntohl(*(uint32_t *)p);
> > 
> > On a platform where ssize_t is only 32-bits we could get a negative
> > value here, which would be very bad.
> 
> We can't, because the length is anyway limited to the maximum IPv4 path
> MTU in any hypervisor we might be talking to.

Only if we trust the hypervisor.  And that the user didn't
misconfigure us to point the socket at something that's not actually a
hypervisor.  And that it's not some future version of the hypervisor
configured to use a different framing format.  And that our code is
robust enough to never get out of sync on the stream.

I really think it's better to read this into a u32, and range sanity
check it before we do anything else.

> > So l2len should be a uint32_t, not ssize_t.
> 
> True, I can also change that while at it.
> 
> > We do then need to make sure that the comparison between
> > l2len and n is unsigned - it's safe to cast n to size_t there, because
> > we've verified it's positive as the loop condition.
> >
> > Or... maybe it's simpler.  The frame length is encoded as 32-bits, but
> > we can't meaningfully have frames above 64k (maybe 64k+ETH_HLEN).  So
> > possibly we should just reset the tap connection if we see such a
> > frame (most likely it means we've somehow gotten out of sync, anyway).
> 
> I'd rather "fix" type and comparison, for the sake of whatever static
> checkers might eventually come up with.
> 
> > >  		p += sizeof(uint32_t);
> > >  		n -= sizeof(uint32_t);  
> > 
> > 
> > > +		if (l2len > (ssize_t)TAP_BUF_BYTES - n)  
> > 
> > I hate to discard valid frames from the guest.
> 
> That won't happen because of how TAP_BUF_FILL and TAP_BUF_BYTES are
> defined. If this condition is true, the length descriptor actually
> mismatched. If you have a better proposal, let me know.

Hmm.. I'll have a look.

> > > +			goto next;  
> > 
> > ..and this is not safe.  This skips (l2len > n) check, which means
> > that the n -= l2len at next could now have a signed overflow, which is
> > UB.
> 
> It's safe because of the change from 3/4, which will just return on
> overflow. In any case, yes, I can add a return directly, it's not very
> productive to speculate what kind of issues a hypervisor might have,
> let's just avoid crashing in case.
> 
> > > +
> > >  		/* At most one packet might not fit in a single read, and this
> > >  		 * needs to be blocking.
> > >  		 */  
> > 
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/4] tap: Drop frames from guest whose length is more than remaining buffer

[Stefano Brivio]

On Fri, 28 Jun 2024 17:19:37 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 27, 2024 at 10:21:04AM +0200, Stefano Brivio wrote:
> > On Thu, 27 Jun 2024 11:30:02 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Thu, Jun 27, 2024 at 01:45:36AM +0200, Stefano Brivio wrote:  
> > > > This was reported by Matej a while ago, but we forgot to fix it. Even
> > > > if the hypervisor is necessarily trusted by passt, as it can in any
> > > > case terminate the guest or disrupt guest connectivity, it's a good
> > > > idea to be robust against possible issues.
> > > > 
> > > > Instead of resetting the connection to the hypervisor, just skip the
> > > > offending frame, as we had a few cases where QEMU would get the
> > > > length descriptor wrong, in the past.
> > > > 
> > > > Reported-by: Matej Hrica <mhrica@redhat.com>
> > > > Suggested-by: Matej Hrica <mhrica@redhat.com>
> > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > > ---
> > > >  tap.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/tap.c b/tap.c
> > > > index 7f8c26d..bb993e0 100644
> > > > --- a/tap.c
> > > > +++ b/tap.c
> > > > @@ -1026,6 +1026,9 @@ redo:    
> > > 
> > > So.. I just realised there's a different pre-existing problem here,
> > > above what's quoted in the patch we have:
> > > 
> > > 		ssize_t l2len = ntohl(*(uint32_t *)p);
> > > 
> > > On a platform where ssize_t is only 32-bits we could get a negative
> > > value here, which would be very bad.  
> > 
> > We can't, because the length is anyway limited to the maximum IPv4 path
> > MTU in any hypervisor we might be talking to.  
> 
> Only if we trust the hypervisor.  And that the user didn't
> misconfigure us to point the socket at something that's not actually a
> hypervisor.  And that it's not some future version of the hypervisor
> configured to use a different framing format.  And that our code is
> robust enough to never get out of sync on the stream.
> 
> I really think it's better to read this into a u32, and range sanity
> check it before we do anything else.

Right... see v2?

-- 
Stefano