[PATCH v2 0/5] Small, assorted "hardening" fixes

[PATCH v2 0/5] Small, assorted "hardening" fixes

[Stefano Brivio]

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

v2:
- in 3/5:
  - keep 'skip' in write_remainder() unsigned, and check for
    unsigned overflow instead
  - refactor sadd_overflow() and ssub_overflow() to use built-ins
    with automatic types, take ssize_t arguments, and deal with
    different ssize_t type widths
- in 4/5:
  - switch l2len in tap_handler_passt() to uint32_t, as it really
    is unsigned and 32-bit wide
  - return if the length descriptor mismatches, instead of trying
    to proceed to the next frame
- add 5/5

Stefano Brivio (5):
  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: Discard guest data on length descriptor mismatch
  conf: Use the right maximum buffer size for c->sock_path

 conf.c       |  4 ++--
 lineread.c   |  5 +++--
 tap.c        | 31 +++++++++++++++++++----------
 tcp_splice.c | 15 +++++++++-----
 util.c       |  5 +++++
 util.h       | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 96 insertions(+), 19 deletions(-)

-- 
2.43.0

[PATCH v2 1/5] 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>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 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

[PATCH v2 2/5] 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>
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;
-- 
@@ -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

[PATCH v2 3/5] 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     |  5 +++++
 util.h     | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 78 insertions(+), 8 deletions(-)

diff --git a/lineread.c b/lineread.c
index 0387f4a..f72a14e 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 (sadd_overflow(lr->count, rc, &lr->count))
+			return -ERANGE;
 	}
 
 	*line = lr->buf + lr->next_line;
diff --git a/tap.c b/tap.c
index ec994a2..e5c1693 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 (sadd_overflow(n, rem, &n))
+				return;
+
+			if (n != l2len)
 				return;
 		}
 
@@ -1046,7 +1050,9 @@ redo:
 
 next:
 		p += l2len;
-		n -= l2len;
+
+		if (ssub_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 (sadd_overflow(n, len, &n))
+				return;
+
 			continue;
 		}
 
-
 		tap_add_packet(c, len, pkt_buf + n);
 
-		if ((n += len) == TAP_BUF_BYTES)
+		if (sadd_overflow(n, len, &n))
+			return;
+
+		if (n == TAP_BUF_BYTES)
 			break;
 	}
 
diff --git a/util.c b/util.c
index dd2e57f..4de872b 100644
--- a/util.c
+++ b/util.c
@@ -585,6 +585,11 @@ int write_remainder(int fd, const struct iovec *iov, int iovcnt, size_t skip)
 		if (rc < 0)
 			return -1;
 
+		if (skip > (size_t)SIZE_MAX - rc) {
+			errno = -ERANGE;
+			return -1;
+		}
+
 		skip += rc;
 	}
 
diff --git a/util.h b/util.h
index eebb027..1da2616 100644
--- a/util.h
+++ b/util.h
@@ -223,6 +223,61 @@ static inline bool mod_between(unsigned x, unsigned i, unsigned j, unsigned m)
 	return mod_sub(x, i, m) < mod_sub(j, i, m);
 }
 
+#if defined __has_builtin
+# if __has_builtin(__builtin_add_overflow)
+#  define sadd_overflow(a, b, res) __builtin_add_overflow(a, b, res)
+# endif
+# if __has_builtin(__builtin_sub_overflow)
+#  define ssub_overflow(a, b, res) __builtin_sub_overflow(a, b, res)
+# endif
+#endif
+
+#ifndef SSIZE_MIN
+# if SSIZE_MAX == LONG_MAX
+#  define SSIZE_MIN	LONG_MIN
+# else
+#  define SSIZE_MIN	INT_MIN
+# endif
+#endif
+
+#ifndef sadd_overflow
+/**
+ * sadd_overflow() - Sum with overflow check for ssize_t 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 sadd_overflow(ssize_t a, ssize_t b, ssize_t *sum)
+{
+	if ((a > 0 && a > SSIZE_MAX - b) || (b < 0 && a < SSIZE_MIN - b))
+		return true;
+
+	*sum = a + b;
+	return false;
+}
+#endif
+
+#ifndef ssub_overflow
+/**
+ * ssub_overflow() - Subtraction with overflow check for ssize_t 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 ssub_overflow(ssize_t a, ssize_t b, ssize_t *diff)
+{
+	if ((b > 0 && a < SSIZE_MIN + b) || (b < 0 && a > SSIZE_MAX + b))
+		return true;
+
+	*diff = a - b;
+	return false;
+}
+#endif
+
 /*
  * Workarounds for https://github.com/llvm/llvm-project/issues/58992
  *
-- 
@@ -223,6 +223,61 @@ static inline bool mod_between(unsigned x, unsigned i, unsigned j, unsigned m)
 	return mod_sub(x, i, m) < mod_sub(j, i, m);
 }
 
+#if defined __has_builtin
+# if __has_builtin(__builtin_add_overflow)
+#  define sadd_overflow(a, b, res) __builtin_add_overflow(a, b, res)
+# endif
+# if __has_builtin(__builtin_sub_overflow)
+#  define ssub_overflow(a, b, res) __builtin_sub_overflow(a, b, res)
+# endif
+#endif
+
+#ifndef SSIZE_MIN
+# if SSIZE_MAX == LONG_MAX
+#  define SSIZE_MIN	LONG_MIN
+# else
+#  define SSIZE_MIN	INT_MIN
+# endif
+#endif
+
+#ifndef sadd_overflow
+/**
+ * sadd_overflow() - Sum with overflow check for ssize_t 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 sadd_overflow(ssize_t a, ssize_t b, ssize_t *sum)
+{
+	if ((a > 0 && a > SSIZE_MAX - b) || (b < 0 && a < SSIZE_MIN - b))
+		return true;
+
+	*sum = a + b;
+	return false;
+}
+#endif
+
+#ifndef ssub_overflow
+/**
+ * ssub_overflow() - Subtraction with overflow check for ssize_t 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 ssub_overflow(ssize_t a, ssize_t b, ssize_t *diff)
+{
+	if ((b > 0 && a < SSIZE_MIN + b) || (b < 0 && a > SSIZE_MAX + b))
+		return true;
+
+	*diff = a - b;
+	return false;
+}
+#endif
+
 /*
  * Workarounds for https://github.com/llvm/llvm-project/issues/58992
  *
-- 
2.43.0

[PATCH v2 4/5] tap: Discard guest data on length descriptor mismatch

[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 discard
the data we read with a single recv(), as we had a few cases where
QEMU would get the length descriptor wrong, in the past.

While at it, change l2len in tap_handler_passt() to uint32_t, as the
length descriptor is logically unsigned and 32-bit wide.

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

diff --git a/tap.c b/tap.c
index e5c1693..d24a935 100644
--- a/tap.c
+++ b/tap.c
@@ -1021,15 +1021,18 @@ redo:
 	}
 
 	while (n > (ssize_t)sizeof(uint32_t)) {
-		ssize_t l2len = ntohl(*(uint32_t *)p);
+		uint32_t l2len = ntohl(*(uint32_t *)p);
 
 		p += sizeof(uint32_t);
 		n -= sizeof(uint32_t);
 
+		if (l2len > (ssize_t)TAP_BUF_BYTES - n)
+			return;
+
 		/* At most one packet might not fit in a single read, and this
 		 * needs to be blocking.
 		 */
-		if (l2len > n) {
+		if (l2len > (size_t)n) {
 			rem = recv(c->fd_tap, p + n, l2len - n, 0);
 
 			if (sadd_overflow(n, rem, &n))
@@ -1042,8 +1045,7 @@ redo:
 		/* Complete the partial read above before discarding a malformed
 		 * frame, otherwise the stream will be inconsistent.
 		 */
-		if (l2len < (ssize_t)sizeof(struct ethhdr) ||
-		    l2len > (ssize_t)ETH_MAX_MTU)
+		if (l2len < sizeof(struct ethhdr) || l2len > ETH_MAX_MTU)
 			goto next;
 
 		tap_add_packet(c, l2len, p);
-- 
@@ -1021,15 +1021,18 @@ redo:
 	}
 
 	while (n > (ssize_t)sizeof(uint32_t)) {
-		ssize_t l2len = ntohl(*(uint32_t *)p);
+		uint32_t l2len = ntohl(*(uint32_t *)p);
 
 		p += sizeof(uint32_t);
 		n -= sizeof(uint32_t);
 
+		if (l2len > (ssize_t)TAP_BUF_BYTES - n)
+			return;
+
 		/* At most one packet might not fit in a single read, and this
 		 * needs to be blocking.
 		 */
-		if (l2len > n) {
+		if (l2len > (size_t)n) {
 			rem = recv(c->fd_tap, p + n, l2len - n, 0);
 
 			if (sadd_overflow(n, rem, &n))
@@ -1042,8 +1045,7 @@ redo:
 		/* Complete the partial read above before discarding a malformed
 		 * frame, otherwise the stream will be inconsistent.
 		 */
-		if (l2len < (ssize_t)sizeof(struct ethhdr) ||
-		    l2len > (ssize_t)ETH_MAX_MTU)
+		if (l2len < sizeof(struct ethhdr) || l2len > ETH_MAX_MTU)
 			goto next;
 
 		tap_add_packet(c, l2len, p);
-- 
2.43.0

[PATCH v2 5/5] conf: Use the right maximum buffer size for c->sock_path

[Stefano Brivio]

UNIX_SOCK_MAX is the maximum number we'll append to the socket path
if we generate it automatically. If it's given on the command line,
it can be up to UNIX_PATH_MAX (including the terminating character)
long.

UNIX_SOCK_MAX happened to kind of fit because it's 100 (instead of
108).

Commit ceddcac74a6e ("conf, tap: False "Buffer not null terminated"
positives, CWE-170") fixed the wrong problem: the right fix for the
problem at hand was actually commit cc287af173ca ("conf: Fix
incorrect bounds checking for sock_path parameter").

Fixes: ceddcac74a6e ("conf, tap: False "Buffer not null terminated" positives, CWE-170")
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 9e47e9a..3c38ceb 100644
--- a/conf.c
+++ b/conf.c
@@ -1398,7 +1398,7 @@ void conf(struct ctx *c, int argc, char **argv)
 			c->foreground = 1;
 			break;
 		case 's':
-			ret = snprintf(c->sock_path, UNIX_SOCK_MAX - 1, "%s",
+			ret = snprintf(c->sock_path, sizeof(c->sock_path), "%s",
 				       optarg);
 			if (ret <= 0 || ret >= (int)sizeof(c->sock_path))
 				die("Invalid socket path: %s", optarg);
-- 
@@ -1398,7 +1398,7 @@ void conf(struct ctx *c, int argc, char **argv)
 			c->foreground = 1;
 			break;
 		case 's':
-			ret = snprintf(c->sock_path, UNIX_SOCK_MAX - 1, "%s",
+			ret = snprintf(c->sock_path, sizeof(c->sock_path), "%s",
 				       optarg);
 			if (ret <= 0 || ret >= (int)sizeof(c->sock_path))
 				die("Invalid socket path: %s", optarg);
-- 
2.43.0

Re: [PATCH v2 5/5] conf: Use the right maximum buffer size for c->sock_path

[David Gibson]

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

On Thu, Jun 27, 2024 at 10:46:41PM +0200, Stefano Brivio wrote:
> UNIX_SOCK_MAX is the maximum number we'll append to the socket path
> if we generate it automatically. If it's given on the command line,
> it can be up to UNIX_PATH_MAX (including the terminating character)
> long.
> 
> UNIX_SOCK_MAX happened to kind of fit because it's 100 (instead of
> 108).
> 
> Commit ceddcac74a6e ("conf, tap: False "Buffer not null terminated"
> positives, CWE-170") fixed the wrong problem: the right fix for the
> problem at hand was actually commit cc287af173ca ("conf: Fix
> incorrect bounds checking for sock_path parameter").
> 
> Fixes: ceddcac74a6e ("conf, tap: False "Buffer not null terminated" positives, CWE-170")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

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


> ---
>  conf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/conf.c b/conf.c
> index 9e47e9a..3c38ceb 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1398,7 +1398,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  			c->foreground = 1;
>  			break;
>  		case 's':
> -			ret = snprintf(c->sock_path, UNIX_SOCK_MAX - 1, "%s",
> +			ret = snprintf(c->sock_path, sizeof(c->sock_path), "%s",
>  				       optarg);
>  			if (ret <= 0 || ret >= (int)sizeof(c->sock_path))
>  				die("Invalid socket path: %s", optarg);

-- 
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 v2 0/5] Small, assorted "hardening" fixes

[Stefano Brivio]

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

> All harmless issues as far as I can tell, but nice to fix.
> 
> v2:
> - in 3/5:
>   - keep 'skip' in write_remainder() unsigned, and check for
>     unsigned overflow instead
>   - refactor sadd_overflow() and ssub_overflow() to use built-ins
>     with automatic types, take ssize_t arguments, and deal with
>     different ssize_t type widths
> - in 4/5:
>   - switch l2len in tap_handler_passt() to uint32_t, as it really
>     is unsigned and 32-bit wide
>   - return if the length descriptor mismatches, instead of trying
>     to proceed to the next frame
> - add 5/5
> 
> Stefano Brivio (5):
>   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: Discard guest data on length descriptor mismatch
>   conf: Use the right maximum buffer size for c->sock_path

I applied 1/5, 2/5, and 5/5. I'll post a new version of 4/5 without the
"fix" for the integer overflow false positive soon, and I'll leave 3/5
alone for the moment.

-- 
Stefano