From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id 9BD365A004E for ; Thu, 27 Jun 2024 22:47:26 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1719521245; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=CAph991xkYQShFSTl0C6Vxi9Ms67ktS1PpK8MK8vQRI=; b=hTO33oiKExKCJp6vddgYa720nQiN1E6jNrll7zxL+ZH0yFDOVEtKy5bKbXDzuCTZecncaf YXNpAf86IIiHWvg68ul5zQ7ABa88E76lBxTrQsoOO9hQSnke80ITdgUNkkOsiablCLPCv1 8E+slmjMmEIQpgYv7ybskMl8vIpPDc8= Received: from mail-qv1-f69.google.com (mail-qv1-f69.google.com [209.85.219.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-588-QwKd-MAIPheRVOEryA6V3A-1; Thu, 27 Jun 2024 16:47:24 -0400 X-MC-Unique: QwKd-MAIPheRVOEryA6V3A-1 Received: by mail-qv1-f69.google.com with SMTP id 6a1803df08f44-6b50f5610cdso129465956d6.2 for ; Thu, 27 Jun 2024 13:47:24 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719521243; x=1720126043; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=CAph991xkYQShFSTl0C6Vxi9Ms67ktS1PpK8MK8vQRI=; b=hMJtFk7g8p33YZoQrSHekxz0xrmq//RNczbLJx2Jd7jJnGyTonjT0P+Z7Mkfi1gHKe wBkpnrY9fsOT7E2tB4G4ei/Q8w73VMx697BjS8BDlQdZDpcCb11owQBA6oaUZMbu4JGH 8UxKERC04uAPVPc/PgfDi0kcYbOGIxy9v5nYvVGIfnQQbm7IdMq9ZNnq415r3xUsKjGt sbbJ3X40Qv6V8TdFpcL9X+xpyEP+azMdCbnVcdHqoqeEixfvfTtlo2Xh9/bM4eK87EIz paOP6XvjDO1JHV/7Y5QpseJfhX678Mui6qB+4YGjLBnFGvVou97Min4hqFYgu/Egsd5W hm8g== X-Gm-Message-State: AOJu0YwmAb45WOrdvCOoGcMijkUsmTkDibGpZXgw6M1wV2jooDq579iK NbZ2YjiRLGmYC+hzROcNnXni7InqYkO2hfRqYBJb6FdRTydG0bupXtA5T/Ubj2c6d+C1oMbp5ea e5VWfB9+9WCrQvTwMG2Wh7q3RpeGrd07HZQO4GSDhcivmNr3+hQ== X-Received: by 2002:a0c:8d4d:0:b0:6b5:4aa9:9686 with SMTP id 6a1803df08f44-6b54aa99d5dmr128805486d6.42.1719521243407; Thu, 27 Jun 2024 13:47:23 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHfYITd+jYWCwX979DqigPW7jChxgpA2dYQC4+/ip1Ycyb39vFp3RuFzsIXJWKBuPTJNxr/jQ== X-Received: by 2002:a0c:8d4d:0:b0:6b5:4aa9:9686 with SMTP id 6a1803df08f44-6b54aa99d5dmr128805256d6.42.1719521242780; Thu, 27 Jun 2024 13:47:22 -0700 (PDT) Received: from maya.cloud.tilaa.com (maya.cloud.tilaa.com. [164.138.29.33]) by smtp.gmail.com with ESMTPSA id 6a1803df08f44-6b59e5746f5sm1981246d6.52.2024.06.27.13.47.21 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 27 Jun 2024 13:47:22 -0700 (PDT) Date: Thu, 27 Jun 2024 22:46:45 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 3/4] util, lineread, tap: Overflow checks on long signed sums and subtractions Message-ID: <20240627224645.6cad668d@elisabeth> In-Reply-To: <20240627095537.4d3f20f8@elisabeth> References: <20240626234536.3306466-1-sbrivio@redhat.com> <20240626234536.3306466-4-sbrivio@redhat.com> <20240627095537.4d3f20f8@elisabeth> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: GPMZK7UXROSJBAP5X3WTT7YW7BMYODBU X-Message-ID-Hash: GPMZK7UXROSJBAP5X3WTT7YW7BMYODBU X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Matej Hrica X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Thu, 27 Jun 2024 09:55:46 +0200 Stefano Brivio wrote: > On Thu, 27 Jun 2024 11:13:14 +1000 > David Gibson 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 > > > --- > > > 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 > > > #include > > > #include > > > +#include > > > > > > #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