linux-kernel - [PATCH] tools/nolibc/string: fix memcmp() implementation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-Id: <20221021060153.7378-1-w@1wt.eu>
Date:   Fri, 21 Oct 2022 08:01:53 +0200
From:   Willy Tarreau <w@....eu>
To:     "Paul E. McKenney" <paulmck@...nel.org>
Cc:     Rasmus Villemoes <linux@...musvillemoes.dk>,
        linux-kernel@...r.kernel.org, Willy Tarreau <w@....eu>
Subject: [PATCH] tools/nolibc/string: fix memcmp() implementation

From: Rasmus Villemoes <linux@...musvillemoes.dk>

The C standard says that memcmp() must treat the buffers as consisting
of "unsigned chars". If char happens to be unsigned, the casts are ok,
but then obviously the c1 variable can never contain a negative
value. And when char is signed, the casts are wrong, and there's still
a problem with using an 8-bit quantity to hold the difference, because
that can range from -255 to +255.

For example, assuming char is signed, comparing two 1-byte buffers,
one containing 0x00 and another 0x80, the current implementation would
return -128 for both memcmp(a, b, 1) and memcmp(b, a, 1), whereas one
of those should of course return something positive.

Signed-off-by: Rasmus Villemoes <linux@...musvillemoes.dk>
Fixes: 66b6f755ad45 ("rcutorture: Import a copy of nolibc")
Cc: stable@...r.kernel.org # v5.0+
Signed-off-by: Willy Tarreau <w@....eu>
---

Paul,

please also include this patch to your series of fixes. I messed up
with memcmp() making it possibly return a wrong sign depending on the
distance between input bytes. It could for example affect a use case
where it would be used in the same way as with qsort() (even though
we do not implement qsort). I'm sending you separately a second patch
to include a selftest for this one; the selftest does not need to be
backported.

Thank you!
Willy

---

 tools/include/nolibc/string.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/include/nolibc/string.h b/tools/include/nolibc/string.h
index bef35bee9c44..cc1bddcb5927 100644
--- a/tools/include/nolibc/string.h
+++ b/tools/include/nolibc/string.h
@@ -19,9 +19,9 @@ static __attribute__((unused))
 int memcmp(const void *s1, const void *s2, size_t n)
 {
 	size_t ofs = 0;
-	char c1 = 0;
+	int c1 = 0;

-	while (ofs < n && !(c1 = ((char *)s1)[ofs] - ((char *)s2)[ofs])) {
+	while (ofs < n && !(c1 = ((unsigned char *)s1)[ofs] - ((unsigned char *)s2)[ofs])) {
 		ofs++;
 	}
 	return c1;
-- 
2.17.5