Incorrect value returned from cx_bn_cnt_bits

[aido]

The following function seems to return the bit position of the most significant bit in a big number:

speculos/src/bolos/cx_mpi.c

Lines 599 to 637 in 56ed996

	uint32_t cx_mpi_cnt_bits(const cx_mpi_t *x)
	{
	uint8_t a[MAX_MPI_BYTE_SIZE];
	uint32_t len, nbits;
	uint8_t b;
	uint8_t *p;
	nbits = 0;
	// Convert the bignum into big-endian form and store it in 'a':
	// (no need to expand mod 16 and fill with 0, just go as fast as possible)
	len = BN_num_bytes(x);
	if (len > MAX_MPI_BYTE_SIZE) {
	return CX_INVALID_PARAMETER;
	}
	// Convert a cx_mpi_t into big-endian bytes form:
	len = BN_bn2bin(x, a);
	p = a;
	while (*p == 0) {
	p++;
	len--;
	if (len == 0)
	break;
	}
	if (len != 0) {
	len = len * 8;
	b = *p;
	while ((b & 0x80) == 0) {
	b = b << 1;
	len--;
	}
	}
	nbits = len;
	return nbits;
	}

Whereas the documentation suggests that it should instead return the number of bits set to 1 in a big number:

https://github.com/LedgerHQ/ledger-secure-sdk/blob/b82be6fd5a082132ee08bf0d105d1bb7bb4d0b41/include/ox_bn.h#L490-L502

Is the function incorrect or the documentation?

[aido]

Apologies, This is an accidental duplicate issue.
I forgot that I opened a similar issue a few months ago. #460

However. following on from my comment above, if the cx_mpi_cnt_bits() function is supposed to return the bit position of the most significant bit in a big number could it be simplified as follows?

uint32_t cx_mpi_cnt_bits(const cx_mpi_t *x)
{
  uint32_t nbits = BN_num_bits(x);

  return nbits;
}

but if, as the documentation suggests, it is supposed to return the number of bits set to 1 would this work instead?

uint32_t cx_mpi_cnt_bits(const cx_mpi_t *x)
{
  uint32_t nbits = BN_num_bits(x);
  uint32_t count = 0;

  for (int i = 0; i < nbits; ++i) {
    count += BN_is_bit_set(x, i);
  }

  return count;
}

[tdejoigny-ledger]

cc: @srasoamiaramanana-ledger

[srasoamiaramanana-ledger]

Yes cx_mpi_cnt_bits returns the number of (significant) bits of the big number which is indeed the position of the MSB (if you start counting from 1). I will reword the documentation, thank you.
And yes, the function can direclty call BN_num_bits.