RE: [PATCH v2] Bluetooth: convert smp and selftest to crypto kpp API

Hi Johan,

-----Original Message-----
From: linux-crypto-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-crypto-
owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Johan Hedberg
Sent: Thursday, May 12, 2016 7:05 PM
To: Benedetto, Salvatore <redacted>
Cc: herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org; gustavo-THi1TnShQwVAfugRpC6u6w@public.gmane.org; linux-
bluetooth-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-crypto-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org
Subject: Re: [PATCH v2] Bluetooth: convert smp and selftest to crypto kpp
API

Hi Salvatore,

On Mon, May 09, 2016, Salvatore Benedetto wrote:

quoted

 * Convert both smp and selftest to crypto kpp API
 * Remove module ecc as not more required
 * Add ecdh_helper functions for wrapping kpp async calls

This patch has been tested *only* with selftest, which is called on
module loading. smp-tester passes all tests but the first one, which
often times out. Same behavior is observed without this patch though.

This patch is based on https://patchwork.kernel.org/patch/9050061/

Signed-off-by: Salvatore Benedetto <redacted>
---
 net/bluetooth/Makefile      |   2 +-
 net/bluetooth/ecc.c         | 816 --------------------------------------------
 net/bluetooth/ecc.h         |  54 ---
 net/bluetooth/ecdh_helper.c | 204 +++++++++++
net/bluetooth/ecdh_helper.h |  32 ++
 net/bluetooth/selftest.c    |   6 +-
 net/bluetooth/smp.c         |   8 +-
 7 files changed, 244 insertions(+), 878 deletions(-)  delete mode
100644 net/bluetooth/ecc.c  delete mode 100644 net/bluetooth/ecc.h
create mode 100644 net/bluetooth/ecdh_helper.c  create mode 100644
net/bluetooth/ecdh_helper.h

I tried this together with your kpp patches and I was able to successfully pair,
so from that perspective we're fine and I have no objections for those
patches being merged.

There are a couple of things to improve with this patch however:

The first issue is that you're missing a 'select CRYPTO_ECDH' in
net/bluetooth/Kconfig. Without this you'll end up creating a kernel that
either doesn't build or a module that doesn't load.

Right the module won't load. I'll fix this.

quoted

+#ifndef __ECDH_HELPER_H
+#define __ECDH_HELPER_H

Please leave out these include guards for internal headers. We try to avoid
them there to force keeping the dependency chains simple. I realize you
might have copied this from smp.h (which is the only internal header I could
see having this) so that might need fixing in a separate patch.

Actually I didn't pay much attention to this as I always use them.
I'll remove it.

quoted

+bool compute_ecdh_shared_secret(const u8 pub_a[64], const u8

priv_b[32],

quoted

+				u8 secret[32]);
+bool generate_ecdh_key_pair(u8 public_key[64], u8 private_key[32]);

Could you try to come up with some shorter names than these since they
cause the line lengths to go past 80 chars. In fact, can't you just keep the
same names as our ecc.c was using. I don't think those names are taken by
your new kpp code?

They actually are used internally by the ecc module I reworked.
I'll rename them to compute_ecdh_secret() and generate_ecdh_keys() which
fixes the 80 chars per line issue.

I'll send a v3.

Thanks for reviewing.

Regards,
Salvatore