Invalid read of size 8 in strncmp() from is_dst()

This is likely a duplicate of https://bugzilla.redhat.com/show_bug.cgi?id=2081583, which was fixed in valgrind in https://bugs.kde.org/show_bug.cgi?id=434764. Based on our analysis, it is not a security bug.

Thanks for the links, Florian, much appreciated. I'll prepare a valgrind upload with the appropriate patch.

I'm also going to dump below a rough explanation of the relevant parts of that routine as I understand it. It's mostly for my benefit, as I was curious to understand what was going on.

Disclaimer: I'm not fluent in asm *at all*, nor am I particularly well versed in security issues.

The relevant code is this (with plenty of ifdefs removed):

ENTRY (STRCMP)
 test %RDX_LP, %RDX_LP
 je LABEL(strcmp_exitz)
 cmp $1, %RDX_LP
 je LABEL(Byte0)
 mov %RDX_LP, %R11_LP

 mov %esi, %ecx
 mov %edi, %eax
        /* Use 64bit AND here to avoid long NOP padding. */
 and $0x3f, %rcx /* rsi alignment in cache line */
 and $0x3f, %rax /* rdi alignment in cache line */

 cmp $0x30, %ecx
 ja LABEL(crosscache) /* rsi: 16-byte load will cross cache line */
 cmp $0x30, %eax
 ja LABEL(crosscache) /* rdi: 16-byte load will cross cache line */
 movlpd (%rdi), %xmm1
 movlpd (%rsi), %xmm2
 movhpd 8(%rdi), %xmm1
 movhpd 8(%rsi), %xmm2 <- valgrind complains.

 pxor %xmm0, %xmm0 /* clear %xmm0 for null char checks */
 pcmpeqb %xmm1, %xmm0 /* Any null chars? */
 pcmpeqb %xmm2, %xmm1 /* compare first 16 bytes for equality */
 psubb %xmm0, %xmm1 /* packed sub of comparison results*/
 pmovmskb %xmm1, %edx

 sub $0xffff, %edx /* if first 16 bytes are same, edx == 0xffff */
 jnz LABEL(less16bytes) /* If not, find different value or null char */

 sub $16, %r11
 jbe LABEL(strcmp_exitz) /* finish comparision */

 add $16, %rsi /* prepare to search next 16 bytes */
 add $16, %rdi /* prepare to search next 16 bytes */
/* SNIP: after that control flow becomes... complicated, and it's not relevant for our use case since we're with small buffers here. */

LABEL(less16bytes):
 bsf %rdx, %rdx /* find and store bit index in %rdx */

 sub %rdx, %r11
 jbe LABEL(strcmp_exitz)

 movzbl (%rsi, %rdx), %ecx
 movzbl (%rdi, %rdx), %eax

 sub %ecx, %eax
 ret

LABEL(strcmp_exitz):
 xor %eax, %eax
 ret

The pointers to our buffers are stored in rdi and rsi.
First we're copying the LSB of the addresses in eax/ecx, which we then mask off further, under the assumption that a cache line is 64 bytes. Then, if one ptr is > 48, ptr+16 will be in another cache line, so we abort in that case.

So now we know our next 16 bytes are all in the same cache line. A fortiori they're in the same memory page. Those two conditions mean that once you read the first byte, reading the others doesn't have any visible side-effect regarding the cache. Since we're not in C, the usual notion of undefined behaviour due to reading outside a given buffer doesn't apply, either, so we don't have to fear the compiler eating our children.

Further down the line there are some SSE calculations done on the whole 16 bytes which end up with a bitmask of which bytes are unequal or null. The position of its least significant set bit can then be checked to see if it starts to diverge in our buffer or not. If not, we simply return success, otherwise we just return the result of the comparison on that first divergent byte.

Attaching a debdiff of the uploaded package for the FFe.

I'm not convinced we have the best fix even if it is the upstream fix and only fix right now. What concerns me is that it looks like a workaround specifically for strncmp(), which even if correct suggests there's a more fundamental inaccuracy in Valgrind's machine emulation. And if so then that might affect other functions again in future.

As I understand it, there is a fundamental inaccuracy in Valgrind's machine emulation. It's not emulating hardware but rather some kind of C machine model, with some extra bells & whistles when it interacts with the OS. That works very well because pretty much everything in our systems boils down to C code (I'd be curious to see how valgrind and golang play together, though).

Glibc is also implementing that C machine model against actual hardware, and is thus sometimes stepping outside of it to take advantage of hardware specificities that valgrind doesn't know about. Valgrind is aware of it, that's why it's been intercepting calls to glibc functions that don't play by its rules.

Now, I dig a bit further into the issue, and it turns out that Valgrind has suppression files to, well, suppress some outputs that are known false positives. There's a file for glibc in the upstream sources, but it isn't shipped in our package. I'll be looking into that.

That actually sounds encouraging because it suggests Valgrind is more portable than I thought :)

Again, I don't have any deep knowledge of Valgrind. Regarding portability, it still needs pretty intimate knowledge of the underlying system since real world programs don't just use malloc() to get addresses, there are stuff like mmap() to take into account. Also, tracking *access* would likely require hooking into very lowlevel machinery.

Regarding the suppression files, the glibc suppression file is actually merged into the default suppression and so is shipped in the package, but it's tailored to the build-time version of glibc, and based on the file patterns it probably doesn't work on Debian-based systems anyway. I confirmed this by rebuilding the current package (without the patch), and the strncmp issue still shows (I'm using the test program outlined at https://bugs.kde.org/show_bug.cgi?id=434764 ).

I'll file a bug in Debian for the suppression file pattern, but in the mean time we still want the patch.

This bug was fixed in the package valgrind - 1:3.19.0-1ubuntu2

---------------
valgrind (1:3.19.0-1ubuntu2) lunar; urgency=medium

  * d/p/lp2015216.patch: Cherry-picked from upstream to silence errors caused
    by asm-optimised strncmp in ld.so from recent glibc (LP: #2015216)

 -- Simon Chopin <email address hidden> Tue, 04 Apr 2023 20:25:00 +0200