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

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

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(a)redhat.com> 
Reviewed-by: David Gibson <david(a)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

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

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

...
  On Thu, 27 Jun 2024 11:13:14 +1000
 David Gibson <david(a)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 &lt;sbrivio(a)redhat.com&gt;
 ---
  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

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

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

...
  On Fri, 28 Jun 2024 17:11:43 +1000
 David Gibson <david(a)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(a)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 &lt;sbrivio(a)redhat.com&gt;
 > ---
 >  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

On Thu, 27 Jun 2024 11:30:02 +1000
David Gibson <david(a)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(a)redhat.com>
 Suggested-by: Matej Hrica <mhrica(a)redhat.com>
 Signed-off-by: Stefano Brivio <sbrivio(a)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

On Fri, 28 Jun 2024 17:19:37 +1000
David Gibson <david(a)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(a)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(a)redhat.com>
 Suggested-by: Matej Hrica <mhrica(a)redhat.com>
 Signed-off-by: Stefano Brivio <sbrivio(a)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