Bug 1022178

BTW, there are tons of

secure_memset (foo_array, '\0', IPMI_FOO_ARRAY_SIZE);

lines which just beg to be cleaned up using sizeof(foo_array) for the size parameter, so that a typo doesn't turn into a disaster.

Better yet, create and use a zero_out_array(foo) which hides '\0' and sizeof() and also checks that foo is an array, not a pointer (using sizeof would be a hidden bug if foo is a ptr).

From kernel source:

# define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))

/* &a[0] degrades to a pointer: a different type from an array */
#define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))

(In reply to Denys Vlasenko from comment #0)
> This code attempts to store a byte with user name length:
> 
> freeipmi-1.3.2/libfreeipmi/util/ipmi-rmcpplus-util.c
> 
>   memcpy (hash_data + hash_data_len,
>           (void *)&user_name_len,
>           sizeof (uint8_t));
>   hash_data_len += sizeof (uint8_t);
> 
> It works on little-endian, but not on big-endian machine.
> 
> The easy fix is to replace that monstrosity with:
> 
>   hash_data[hash_data_len++] = user_name_len;

Forgot to mention that I tested this fix and it works.

(In reply to Denys Vlasenko from comment #0)
>   hash_data[hash_data_len++] = user_name_len;
 
Patch applied.  Will be in the FreeIPMI 1.3.3 release.

I didn't apply the second half of the patch w/ the memset cleanup.  Acknowledged that the memsets should be cleaned up globally, but would like to do a huge fix/audit everywhere later.

> also checks that foo is an array, not a pointer

I don't believe the macro you posted is portable to non-gcc compilers?

> I noticed another bug,
> but it's a relatively harmless one (failure to memset to 0 one of the auth
> buffers after work).

Can you tell me the location?  I wouldn't be surprised if it was missed somewhere.

(In reply to Albert Chu from comment #6)
> > I noticed another bug,
> > but it's a relatively harmless one (failure to memset to 0 one of the auth
> > buffers after work).
> 
> Can you tell me the location?  I wouldn't be surprised if it was missed
> somewhere.

Here:

  secure_memset (buf, '\0', IPMI_MAX_KEY_DATA_LENGTH);
  secure_memset (buf, '\0', IPMI_MAX_KEY_EXCHANGE_AUTHENTICATION_CODE_LENGTH);

the second memset operates on the wrong array of chars, it should be:

  secure_memset (digest, '\0', IPMI_MAX_KEY_EXCHANGE_AUTHENTICATION_CODE_LENGTH);

(In reply to Denys Vlasenko from comment #11)
> Here:
> 
>   secure_memset (buf, '\0', IPMI_MAX_KEY_DATA_LENGTH);
>   secure_memset (buf, '\0',
> IPMI_MAX_KEY_EXCHANGE_AUTHENTICATION_CODE_LENGTH);
> 
> the second memset operates on the wrong array of chars, it should be:
> 
>   secure_memset (digest, '\0',
> IPMI_MAX_KEY_EXCHANGE_AUTHENTICATION_CODE_LENGTH);

Doh!  Thanks for the catch.

(In reply to Albert Chu from comment #6)
> > also checks that foo is an array, not a pointer
> 
> I don't believe the macro you posted is portable to non-gcc compilers?

Array-ness check can be made conditional on #ifdef __GNUC__