csiphash alignment issue on non x86-64 platforms

[389-ds-bot]

Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/49424

• Created at 2017-10-25 02:05:47 by firstyear (@Firstyear)
• Closed at 2017-11-02 03:38:06 as fixed
• Assigned to firstyear (@Firstyear)

---

Issue Description

On ppc64 and sparc64 csiphash relies on an alignment that is not correct. This can cause a crash condition to occur,

> >#include <stdio.h>
> 
> >#include <sys/types.h>
> 
> >
> 
> >int func(const void *str, size_t sz, const char key[16]){
> 
> > uint64_t *ip = (uint64_t*) str;
> 
> > printf ("str: %lx:%lx\n", ip, *ip);
> 
> >}
> 
> >
> 
> >int main()
> 
> >{
> 
> > char str[25] = "ABCDEFGH12345678";
> 
> > char key[16];
> 
> >
> 
> But following code should work. Please correct me if I am wrong. I didn't test.
>   char *str = strdup("ABCDEFGH12345678");
>   char *key = malloc(16);
> 
> yes, function sds_siphash13 is not ideal because it rely on properly alligned
> input data.
> 

We should correct this issue asap

CC: @cgrzemba @lslebodn @vashirov

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-10-25 02:12:16

Metadata Update from @Firstyear:

• Issue assigned to Firstyear

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-10-25 02:12:41

Metadata Update from @Firstyear:

• Custom field component adjusted to None
• Custom field origin adjusted to None
• Custom field reviewstatus adjusted to None
• Custom field type adjusted to None
• Custom field version adjusted to None
• Issue priority set to: critical
• Issue set to the milestone: 1.3.7.0

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-10-30 03:03:52

@lslebodn @vashirov Would really appreciate you both looking at this - to check my solution matches expectation and also that it works on ppc64 which had the issue,

Thanks!

[389-ds-bot]

Comment from lslebodn at 2017-10-30 14:10:52

+    size_t input_sz = src_sz / sizeof(uint64_t);
+    /* Account for non-uint64_t alligned input */
+    if (src_sz % sizeof(uint64_t) > 0) {
+        input_sz += 1;
+    }

IMHO you can always add +1 to input_sz But that's micro optimisation :-)
and I'm fine with current version.

I think there still can be a problem with following code:

    118     uint64_t t = 0;
    119     uint8_t *pt = (uint8_t *)&t;
    120     uint8_t *m = (uint8_t *)in;
    121 
    122     switch (src_sz) {
    123     case 7:
    124         pt[6] = m[6]; /* FALLTHRU */
    125     case 6:
    126         pt[5] = m[5]; /* FALLTHRU */
    127     case 5:
    128         pt[4] = m[4]; /* FALLTHRU */
    129     case 4:
    130         *((uint32_t *)&pt[0]) = *((uint32_t *)&m[0]);

I am not sure whether ((intptr_t)m % sizeof(uint32_t) == 0)

[389-ds-bot]

Comment from vashirov (@vashirov) at 2017-10-30 14:12:38

Test no longer crashes, but still fails:

[==========] Running 1 test(s).
[ RUN      ] test_siphash
[  ERROR   ] --- hashout == test_b
[   LINE   ] --- src/libsds/test/test_sds_csiphash.c:44: error: Failure!
[  FAILED  ] test_siphash

[389-ds-bot]

Comment from lslebodn at 2017-10-30 14:25:22

BTW if you would like to get rid of dynamic allocation you can try following change. I didn't test taht

diff --git a/src/libsds/external/csiphash/csiphash.c b/src/libsds/external/csiphash/csiphash.c
index 0089c82f7..07406f602 100644
--- a/src/libsds/external/csiphash/csiphash.c
+++ b/src/libsds/external/csiphash/csiphash.c
@@ -79,7 +79,8 @@ sds_siphash13(const void *src, size_t src_sz, const char key[16])
     uint64_t k0 = _le64toh(_key[0]);
     uint64_t k1 = _le64toh(_key[1]);
     uint64_t b = (uint64_t)src_sz << 56;
-    const uint64_t *in = (uint64_t *)src;
+    const uint8_t *in = (uint8_t *)src;
+    uint64_t temp;
 
     uint64_t v0 = k0 ^ 0x736f6d6570736575ULL;
     uint64_t v1 = k1 ^ 0x646f72616e646f6dULL;
@@ -87,8 +88,9 @@ sds_siphash13(const void *src, size_t src_sz, const char key[16])
     uint64_t v3 = k1 ^ 0x7465646279746573ULL;
 
     while (src_sz >= 8) {
-        uint64_t mi = _le64toh(*in);
-        in += 1;
+        memcpy(&temp, in, 8);
+        uint64_t mi = _le64toh(temp);
+        in += 8;
         src_sz -= 8;
         v3 ^= mi;
         // cround

[389-ds-bot]

Comment from lslebodn at 2017-10-30 14:28:25

@vashirov We might get better error reporting with different cmocka assertions:

diff --git a/src/libsds/test/test_sds_csiphash.c b/src/libsds/test/test_sds_csiphash.c
index cdb6b7f46..1a50401ee 100644
--- a/src/libsds/test/test_sds_csiphash.c
+++ b/src/libsds/test/test_sds_csiphash.c
@@ -37,11 +37,11 @@ test_siphash(void **state __attribute__((unused)))
     // Initial simple test
     value = htole64(5);
     hashout = sds_siphash13(&value, sizeof(uint64_t), key);
-    assert_true(hashout == test_a);
+    assert_int_equal(hashout, test_a);
 
     char *test = "abc";
     hashout = sds_siphash13(test, 4, key);
-    assert_true(hashout == test_b);
+    assert_int_equal(hashout, test_b);
 }

[389-ds-bot]

Comment from vashirov (@vashirov) at 2017-10-30 14:39:16

[==========] Running 1 test(s).
[ RUN      ] test_siphash
[  ERROR   ] --- 0xcc2247b79ac48af0 != 0xb500a9140224413b
[   LINE   ] --- src/libsds/test/test_sds_csiphash.c:44: error: Failure!
[  FAILED  ] test_siphash

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-10-31 02:28:32

Perhaps this is an endianness issues with the memcpy?

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-10-31 02:33:18

BTW if you would like to get rid of dynamic allocation you can try following change. I didn't test taht
diff --git a/src/libsds/external/csiphash/csiphash.c b/src/libsds/external/csiphash/csiphash.c
index 0089c82f7..07406f602 100644
--- a/src/libsds/external/csiphash/csiphash.c
+++ b/src/libsds/external/csiphash/csiphash.c
@@ -79,7 +79,8 @@ sds_siphash13(const void *src, size_t src_sz, const char key[16])
uint64_t k0 = _le64toh(_key[0]);
uint64_t k1 = _le64toh(_key[1]);
uint64_t b = (uint64_t)src_sz << 56;

• const uint64_t *in = (uint64_t *)src;

• const uint8_t *in = (uint8_t *)src;
• uint64_t temp;

uint64_t v0 = k0 ^ 0x736f6d6570736575ULL;
uint64_t v1 = k1 ^ 0x646f72616e646f6dULL;
@@ -87,8 +88,9 @@ sds_siphash13(const void *src, size_t src_sz, const char key[16])
uint64_t v3 = k1 ^ 0x7465646279746573ULL;

while (src_sz >= 8) {

•    uint64_t mi = _le64toh(*in);
  

•    in += 1;
  

•    memcpy(&temp, in, 8);
  

•    uint64_t mi = _le64toh(temp);
  

•    in += 8;
  

src_sz -= 8;
v3 ^= mi;
// cround

But we don't know how large the input is, so I think this may have a memcpy issues if in is not large enough (stack over-read).

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-10-31 02:33:30

@vashirov We might get better error reporting with different cmocka assertions:
diff --git a/src/libsds/test/test_sds_csiphash.c b/src/libsds/test/test_sds_csiphash.c
index cdb6b7f46..1a50401ee 100644
--- a/src/libsds/test/test_sds_csiphash.c
+++ b/src/libsds/test/test_sds_csiphash.c
@@ -37,11 +37,11 @@ test_siphash(void **state attribute((unused)))
// Initial simple test
value = htole64(5);
hashout = sds_siphash13(&value, sizeof(uint64_t), key);

• assert_true(hashout == test_a);

• assert_int_equal(hashout, test_a);

char *test = "abc";
hashout = sds_siphash13(test, 4, key);

• assert_true(hashout == test_b);

• assert_int_equal(hashout, test_b);
  }

Done, added to the patch :)

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-10-31 02:53:45

• size_t input_sz = src_sz / sizeof(uint64_t);
• /* Account for non-uint64_t alligned input */
• if (src_sz % sizeof(uint64_t) > 0) {

•    input_sz += 1;
  

• }

IMHO you can always add +1 to input_sz But that's micro optimisation :-)

Done, it's simple :)

and I'm fine with current version.
I think there still can be a problem with following code:
118 uint64_t t = 0;
119 uint8_t *pt = (uint8_t *)&t;
120 uint8_t *m = (uint8_t )in;
121
122 switch (src_sz) {
123 case 7:
124 pt[6] = m[6]; / FALLTHRU /
125 case 6:
126 pt[5] = m[5]; / FALLTHRU /
127 case 5:
128 pt[4] = m[4]; / FALLTHRU */
129 case 4:
130 *((uint32_t *)&pt[0]) = *((uint32_t *)&m[0]);

I am not sure whether ((intptr_t)m % sizeof(uint32_t) == 0)

I think at this point it shouldn't matter, because it's just doing 32bit reads of the values, and pt is from &t which is a uint64_t, so that will be valid, and &m is from *in, which we just fixed to be aligned. So in both, this should be okay now.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-10-31 02:59:15

    while (src_sz >= 8) {
        uint64_t mi = _le64toh(*in);
        in += 1;
        src_sz -= 8;
        v3 ^= mi;
        // cround
        cROUND(v0, v1, v2, v3);
        v0 ^= mi;
    }

    uint64_t t = 0;
    uint8_t *pt = (uint8_t *)&t;
    uint8_t *m = (uint8_t *)in;

Actually, I think the issue is here. We correctly copy in everything with le64toh for mi in uint64_t groups, but when we have a src without an input of size 8, we will hit uint8_t *m = (uint8_t *)in; which does not do the same le64toh. So this could be the issue.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-10-31 03:01:26

[   LINE   ] --- src/libsds/test/test_sds_csiphash.c:44: error: Failure!

^^-- Yep, that's the failure. On an aligned source we pass, but line 44 is an input of 24 bytes, so we'll skip that loop and go to the uint8_t *m = (uint8_t *)in; line, which is not correctly using endianness controls. As a result we get the failure.

The first test will be src_sz = 8, and the second is src_sz = 3, so that's why this happens. I'm still not sure of the fix though ....

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-10-31 04:06:35

This is what I have now, but it's still not solving the issues from the line 44 test.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-10-31 06:36:27

So a build/cmocka test on copr with ppc64le works, so this is definitely an endianness issue with non-multiple of 8 sized input data.

[389-ds-bot]

Comment from lslebodn at 2017-10-31 10:24:38

So a build/cmocka test on copr with ppc64le works, so this is definitely an endianness issue with non-multiple of 8 sized input data.

That's possible :-)

BTW it might be good idea to extend test suite and test string with length from 8 till 16. Unit tests are fast and it will cover all cases with endian issues.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-11-01 01:43:56

Yes, this is a good idea. But you are also still assuming that I know how to solve this issue in a meaningful way :) I'll update the test cases for a start,

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-11-01 05:52:06

Okay, @vashirov here is a new patch which uses a different strategy to do the memcpy for excess bytes. I'll stay online for your morning to work through it with you. Thanks to @lslebodn for his advice, I added extra tests too and they all pass on LE x86_64

[389-ds-bot]

Comment from cgrzemba at 2017-11-01 10:07:15

seems to work on Solaris11.3 Sparc:

[==========] Running 1 test(s).
[ RUN      ] test_siphash
[       OK ] test_siphash
[==========] 1 test(s) run.
[  PASSED  ] 1 test(s).

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-11-01 10:21:03

Great! @cgrzemba thanks for testing! @vashirov is about to check this on a bigendian system to be sure the fix is correct.

Thanks for your patience with this,

[389-ds-bot]

Comment from vashirov (@vashirov) at 2017-11-01 10:30:56

make  check-TESTS
make[2]: Entering directory `/root/rpmbuild/BUILD/389-ds-base-1.3.7.5'
make[3]: Entering directory `/root/rpmbuild/BUILD/389-ds-base-1.3.7.5'
PASS: test_slapd
PASS: test_libsds
PASS: test_nuncstans
PASS: test_nuncstans_stress_small
make[4]: Entering directory `/root/rpmbuild/BUILD/389-ds-base-1.3.7.5'
make  all-am
make[5]: Entering directory `/root/rpmbuild/BUILD/389-ds-base-1.3.7.5'
make[5]: Nothing to be done for `all-am'.
make[5]: Leaving directory `/root/rpmbuild/BUILD/389-ds-base-1.3.7.5'
make[4]: Leaving directory `/root/rpmbuild/BUILD/389-ds-base-1.3.7.5'
============================================================================
Testsuite summary for 389-ds-base 1.3.7.5
============================================================================
# TOTAL: 4
# PASS:  4
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================

All tests pass on ppc64 :)

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2017-11-02 03:25:30

LGTM, ack

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2017-11-02 03:25:31

Metadata Update from @mreynolds389:

• Custom field reviewstatus adjusted to ack (was: None)

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-11-02 03:38:08

commit 7514464
To ssh://git@pagure.io/389-ds-base.git
e3d4a52..7514464 master -> master

commit 92c56e9
To ssh://git@pagure.io/389-ds-base.git
01530f4..92c56e9 389-ds-base-1.3.7 -> 389-ds-base-1.3.7

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2017-11-02 03:38:08

Metadata Update from @Firstyear:

• Issue close_status updated to: fixed
• Issue status updated to: Closed (was: Open)