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
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> --- 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; } -- 2.43.0
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, 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: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...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.-- Stefano--- 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; }
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:That's a fair point. Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>On Thu, Jun 27, 2024 at 01:45:33AM +0200, Stefano Brivio wrote: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.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.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...-- 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--- 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; }
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> --- 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; -- 2.43.0
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
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(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; + 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 * -- 2.43.0
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 valuess/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 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: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.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 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.Sure. But, especially as package maintainer, in this case I prefer to have a useless check than carrying suppressions around.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.Same here.+ 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.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.Same here.+ + 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.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.+ 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().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.} 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.Oops, fixed.+{ +#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 valuess/saddl_overflow/ssubl_overflow/-- Stefano+ * @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 *
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: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:On Thu, Jun 27, 2024 at 01:45:35AM +0200, Stefano Brivio wrote: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.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 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.Dropped, by:Sure. But, especially as package maintainer, in this case I prefer to have a useless check than carrying suppressions around.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.Same here.+ 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.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.Same here.+ + 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; > > + > > + 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.+ if (saddl_overflow(n, len, &n)) + return; + continue; } - tap_add_packet(c, len, pkt_buf + n); - if ((n += len) == TAP_BUF_BYTES)Same here....checking for overflow in the unsigned sum, without any built-in. Same lines of code, but the logic is simpler.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.{ 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().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.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.} 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.-- StefanoOops, fixed.+{ +#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 valuess/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 *
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(a)redhat.com> wrote: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.On Thu, 27 Jun 2024 11:13:14 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote: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;On Thu, Jun 27, 2024 at 01:45:35AM +0200, Stefano Brivio wrote: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.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 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.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
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:Well, yes.On Thu, Jun 27, 2024 at 01:45:35AM +0200, Stefano Brivio wrote: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.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.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.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.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.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.Sure. But, especially as package maintainer, in this case I prefer to have a useless check than carrying suppressions around.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.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/~dgibsonSame here.+ 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.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.
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: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"?On Thu, 27 Jun 2024 11:13:14 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:Well, yes.On Thu, Jun 27, 2024 at 01:45:35AM +0200, Stefano Brivio wrote: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.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.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.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 noiseSo 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 introduces extra error paths which make it harder to see what the code's doingWe 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 worseI 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.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.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.Sure. But, especially as package maintainer, in this case I prefer to have a useless check than carrying suppressions around.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.It doesn't.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.Same here.+ 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.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 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 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:...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:On Thu, Jun 27, 2024 at 09:55:46AM +0200, Stefano Brivio wrote: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"?On Thu, 27 Jun 2024 11:13:14 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:Well, yes.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.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.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 noiseSo 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 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.- it introduces extra error paths which make it harder to see what the code's doingWe 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 worseI 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.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.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.> 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.-- StefanoIt doesn't.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.> + 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 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.
On Fri, 28 Jun 2024 20:30:54 +0200 Stefano Brivio <sbrivio(a)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
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: 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
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. 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
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:We can't, because the length is anyway limited to the maximum IPv4 path MTU in any hypervisor we might be talking to.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.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.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.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.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.+ 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.-- Stefano+ /* At most one packet might not fit in a single read, and this * needs to be blocking. */
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: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.On Thu, Jun 27, 2024 at 01:45:36AM +0200, Stefano Brivio wrote:We can't, because the length is anyway limited to the maximum IPv4 path MTU in any hypervisor we might be talking to.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.Hmm.. I'll have a look.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.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.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.-- 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/~dgibsonIt'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.+ 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. */
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:Right... see v2? -- StefanoOn Thu, 27 Jun 2024 11:30:02 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote: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.On Thu, Jun 27, 2024 at 01:45:36AM +0200, Stefano Brivio wrote:We can't, because the length is anyway limited to the maximum IPv4 path MTU in any hypervisor we might be talking to.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.