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 045835A004E for ; Thu, 27 Jun 2024 09:56:28 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1719474988; 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=UvUFd8ym0lnTmtVfuctAIlgfKCiSZWCcL7tbeFyXKBU=; b=fNb/OCp6tv8xGlUNYpcAGvoLYgZBDf26d/XtlRHuD2xwA26KXfZX7mEwXmh1Wddf3Xjvim HeffAzUf1nVxqe+9KtdV3DOKtoiG/dlDNkRFqMKFvS+6JYOcTJdo+wN6SEsfOqtanFMDOt IWMXW24Qq9MycvbcO3N3Ob5B3FK7XIY= Received: from mail-vk1-f200.google.com (mail-vk1-f200.google.com [209.85.221.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-542-6JveoNRCM-SK_GgfflN3Xg-1; Thu, 27 Jun 2024 03:56:24 -0400 X-MC-Unique: 6JveoNRCM-SK_GgfflN3Xg-1 Received: by mail-vk1-f200.google.com with SMTP id 71dfb90a1353d-4ef21f8f344so3307653e0c.2 for ; Thu, 27 Jun 2024 00:56:24 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719474984; x=1720079784; 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=UvUFd8ym0lnTmtVfuctAIlgfKCiSZWCcL7tbeFyXKBU=; b=KC5AvJ8tEKbH7OlKkTovMH2rl1zpuZfBlZtI5iXGKstjtwip+4fly41MFoHszL9yuP RKMwh4ke8PsANKsvbUyoHoQf1RbjPyQc9I7fwfK7jpc/VwExHaMDzWskqUuI/kXN4C44 p716cE0ki4CG7IuslTxUTz9NXDmTFX6f/kPmZs6iU2X2K+prGnGvKSXHTSkNJcHTLfet hMQy0gOlR4+oVoD9p83RwWeUIqcYwxTnX2iR1pOLCVmxBHa4YgSRNncfDHX/FoVyLqJP WnUxG9COifeWZhAjBm6MgAgX7+87JHDMbymryrOgCUafGs7FrF83jopd3vWfL7neIAoF JZqw== X-Gm-Message-State: AOJu0Yz6erzKWdLVhYPdqpEX3xYsFothmc+rd+pdGoQGkBC39vi+zd8E 9kibpA/Aiv+8Jkx4W3r7eBUuYskFjWczid/El956cDM0zEEXFFycJd6VVUj50eBApY8XsR/X/T1 zYhCYRioBo2HyCREO8dMzhPvr2l9hwAkGGG+xZqrGHYtHrTy7UA== X-Received: by 2002:a05:6122:1e10:b0:4ef:61c2:6331 with SMTP id 71dfb90a1353d-4ef6a56017emr10997929e0c.1.1719474982257; Thu, 27 Jun 2024 00:56:22 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHTvHSGM4SIRn1yEYu3XHcQAARCkmkUUG4HC/KGLe6gorQOywkXEJwguLBa6CxPJFOfcfjS+w== X-Received: by 2002:a05:6122:1e10:b0:4ef:61c2:6331 with SMTP id 71dfb90a1353d-4ef6a56017emr10997914e0c.1.1719474981718; Thu, 27 Jun 2024 00:56:21 -0700 (PDT) Received: from maya.cloud.tilaa.com (maya.cloud.tilaa.com. [164.138.29.33]) by smtp.gmail.com with ESMTPSA id af79cd13be357-79d5c8bc515sm32863785a.114.2024.06.27.00.56.20 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 27 Jun 2024 00:56:21 -0700 (PDT) Date: Thu, 27 Jun 2024 09:55:46 +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: <20240627095537.4d3f20f8@elisabeth> In-Reply-To: References: <20240626234536.3306466-1-sbrivio@redhat.com> <20240626234536.3306466-4-sbrivio@redhat.com> 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: A4LC4EXMA6ZQ4CZO4QJDKYKPMT3HKN6S X-Message-ID-Hash: A4LC4EXMA6ZQ4CZO4QJDKYKPMT3HKN6S 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 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. > > 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. > > > { > > int i; > > size_t offset; > > @@ -585,7 +585,10 @@ int write_remainder(int fd, const struct iovec *iov, int iovcnt, size_t skip) > > if (rc < 0) > > return -1; > > > > - skip += rc; > > Ok, here it's not a false positive. I believe this really could > overflow if you had an iov where the sum of the iov_len exceeded a > size_t. > > > + if (saddl_overflow(skip, rc, &skip)) { > > + errno = -ERANGE; > > + return -1; > > + } > > If you leave skip an unsigned, you've already checked for negative rc, > so this is essentially an unsigned add. Checking for overflow on an > unsigned addition is simpler than the logic of saddl_overflow(). I'm fairly sure I tried that and it looked rather bulky, because I couldn't use __builtin_uaddl_overflow() if I recall correctly, I can try again. > > } > > > > return 0; > > diff --git a/util.h b/util.h > > index eebb027..497d2fd 100644 > > --- a/util.h > > +++ b/util.h > > @@ -161,7 +161,7 @@ void pidfile_write(int fd, pid_t pid); > > int __daemon(int pidfile_fd, int devnull_fd); > > int fls(unsigned long x); > > int write_file(const char *path, const char *buf); > > -int write_remainder(int fd, const struct iovec *iov, int iovcnt, size_t skip); > > +int write_remainder(int fd, const struct iovec *iov, int iovcnt, ssize_t skip); > > > > /** > > * af_name() - Return name of an address family > > @@ -223,6 +223,50 @@ static inline bool mod_between(unsigned x, unsigned i, unsigned j, unsigned m) > > return mod_sub(x, i, m) < mod_sub(j, i, m); > > } > > > > +/** > > + * saddl_overflow() - Sum with overflow check for long signed values > > + * @a: First value > > + * @b: Second value > > + * @sum: Pointer to result of sum, if it doesn't overflow > > + * > > + * Return: true if the sum would overflow, false otherwise > > + */ > > +static inline bool saddl_overflow(long a, long b, long *sum) > > These take long, but you're often calling them with ssize_t. That's > _probably_ the same thing, but not necessarily. Right, yes, ssize_t can be long or int, even though I'm fairly sure it's always long on all the architectures we are able to build for. There's no integer overflow built-in for ssize_t, but I'll probably need to add a macro conditional for the whole thing anyway, based on the type of ssize_t. > > +{ > > +#if __GNUC__ > > + return __builtin_saddl_overflow(a, b, sum); > > +#else > > + if ((a > 0 && a > LONG_MAX - b) || > > + (b < 0 && a < LONG_MIN - b)) > > + return true; > > + > > + *sum = a + b; > > + return false; > > +#endif > > +} > > + > > +/** > > + * saddl_overflow() - Subtraction with overflow check for long signed values > > s/saddl_overflow/ssubl_overflow/ Oops, fixed. > > + * @a: Minuend > > + * @b: Subtrahend > > + * @sum: Pointer to result of subtraction, if it doesn't overflow > > + * > > + * Return: true if the subtraction would overflow, false otherwise > > + */ > > +static inline bool ssubl_overflow(long a, long b, long *diff) > > +{ > > +#if __GNUC__ > > + return __builtin_ssubl_overflow(a, b, diff); > > +#else > > + if ((b > 0 && a < LONG_MIN + b) || > > + (b < 0 && a > LONG_MAX + b)) > > + return true; > > + > > + *diff = a - b; > > + return false; > > +#endif > > +} > > + > > /* > > * Workarounds for https://github.com/llvm/llvm-project/issues/58992 > > * > -- Stefano