nicehash / nheqminer

Equihash miner for NiceHash
https://www.nicehash.com
MIT License
769 stars 583 forks source link

Unsafe structure `blake2b_param` initialization #381

Open YihaoPeng opened 6 years ago

YihaoPeng commented 6 years ago

https://github.com/nicehash/nheqminer/blob/b9900ff8e3c6f8e5a46af18db454f2a2082d9f46/cpu_tromp/equi.h#L56

  blake2b_param P[1];
  P->digest_length = HASHOUT;
  P->key_length    = 0;
  P->fanout        = 1;
  P->depth         = 1;
  P->leaf_length   = 0;
  P->node_offset   = 0;
  P->node_depth    = 0;
  P->inner_length  = 0;
  memset(P->reserved, 0, sizeof(P->reserved));
  memset(P->salt,     0, sizeof(P->salt));
  memcpy(P->personal, (const uint8_t *)personal, 16);

The problem is that blake2b_param doesn't have to be 11 members, sometimes it can be 12:

https://github.com/BLAKE2/BLAKE2/blob/320c325437539ae91091ce62efec1913cd8093c2/ref/blake2.h#L108

This non-SSE version of the blake2 implementation has 12 members in blake2b_param:

  BLAKE2_PACKED(struct blake2b_param__
  {
    uint8_t  digest_length; /* 1 */
    uint8_t  key_length;    /* 2 */
    uint8_t  fanout;        /* 3 */
    uint8_t  depth;         /* 4 */
    uint32_t leaf_length;   /* 8 */
    uint32_t node_offset;   /* 12 */
    uint32_t xof_length;    /* 16 */
    uint8_t  node_depth;    /* 17 */
    uint8_t  inner_length;  /* 18 */
    uint8_t  reserved[14];  /* 32 */
    uint8_t  salt[BLAKE2B_SALTBYTES]; /* 48 */
    uint8_t  personal[BLAKE2B_PERSONALBYTES];  /* 64 */
});

So, when I ported the code to a device that didn't support SSE, I experienced an "unexplained result error" in release build (because of uninitialized xof_length has the random value). The debug build is completely correct.

I spent two days debugging and finally found the problem here. I think the following code will be safer:

  blake2b_param P[1];
  memset(P, 0, sizeof(blake2b_param));
  P->digest_length = HASHOUT;
  P->fanout        = 1;
  P->depth         = 1;
  memcpy(P->personal, (const uint8_t *)personals, 16);
YihaoPeng commented 6 years ago

I have created the same issue in https://github.com/tromp/equihash/issues/20