Hi Andrey and Marcel,
Den mån 21 jan. 2019 kl 15:53 skrev Marcel Holtmann <marcel@xxxxxxxxxxxx>:
>
> Hi Emil,
>
> >>>> On Sat, Dec 29, 2018 at 9:35 AM Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
> >>>> I think that our ECDH code was endian safe, but then it got changed at some point to use standard crypto and maybe something went wrong there. Can just provide the btmon -w trace.log for the SMP pairing so that I can have a look at the binary trace.
> >>>
> >>> I found out that if I change "swap_digits" method in
> >>> "net/bluetooth/ecdh_helper.c" to
> >>>
> >>> static inline void swap_digits(u64 *in, u64 *out, unsigned int ndigits)
> >>> {
> >>> int i;
> >>>
> >>> for (i = 0; i < ndigits; i++)
> >>> out[i] = in[ndigits - 1 - i];
> >>> }
> >>>
> >>> then BLE pairing on big-endian become operational. I'm not sure what
> >>> proper fix should be: is it a problem with crypto API usage or a
> >>> problem with crypto itself?
> >>
> >> if the kernel ECC and ECDH crypto already swaps for us, then we don’t need to do it again. So all the swap_digits most likely can be removed from net/bluetooth/.
> >>
> >> Regards
> >>
> >> Marcel
> >>
> >
> > The Bluetooth standard is a bit strange. It assumes the AES standard
> > is big endian (although it is really just defined on a byte level),
> > but since Bluetooth is little-endian everywhere, all AES 128-bit
> > values must be reversed when a standard AES library is used. In
> > particular, SMP reverses the AES values. So the swap_digits should be
> > kept.
>
> so you are saying just reversing is needed, but not swapping? But then this is no longer swap_digits, it is just a reverse. What do you want us to do in this case now?
First, I was a bit too quick. This is not about AES but about
NIST-ECDH. Nevertheless, the kernel API seems to assume big endian
order of the values regardless of platform, per the NIST
specification.
By looking some more into the kernel ECDH code, the Bluetooth code is
not the buggy one. Instead the ECDH kernel code turns out to be wrong.
The function ecc_swap_digits in crypto/ecc.c looks like the following:
static inline void ecc_swap_digits(const u64 *in, u64 *out,
unsigned int ndigits)
{
int i;
for (i = 0; i < ndigits; i++)
out[i] = __swab64(in[ndigits - 1 - i]);
}
It is basically used to load a pointer-casted byte buffer into an
internal little endian representation of u64 values and vice versa. So
it works on little endian platforms but will fail on big endian
platforms. Applying your patch here is the correct thing to do, and
not in the Bluetooth code. But instead of using no swap for big endian
and __swab64 for little endian platforms, simply use the be64_to_cpu
helper.
/Emil
