Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

[Harsh Jain]

On 26-09-2017 00:16, Casey Leedom wrote:
> | From: Raj, Ashok 
> | Sent: Monday, September 25, 2017 8:54 AM
> |
> | Not sure how the page->offset would end up being greater than page-size?
Refer below
> |
> | If you have additional traces, please send them by.
> |
> | Is this a new driver? wondering how we didn't run into this?
>
>   According to Herbert Xu and one of our own engineers, it's actually legal
> for Scatter/Gather Lists to have this.  This isn't my area of expertise
> though so I'm just passing that on.
>
>   I've asked our team to produce a detailed trace of the exact
> Scatter/Gather Lists they're seeing and what ends up coming out of the DMA
> Mappings, etc.  They're in India, so I expect that they'll have this for you
> by tomorrow morning.
Below mentioned log was already there in 1st mail. Copied here for easy 
reference. Let me know if you need
additional traces.

1) IN esp_output() "__skb_to_sgvec()" convert skb frags to scatter gather list. 
At that moment sg->offset was 4094.
2) From esp_output control reaches to "crypto_authenc_encrypt()". Here in 
"scatterwalk_ffwd()" sg->offset become 4110.
3) Same sg list received by chelsio crypto driver(chcr). When chcr try to do 
DMA mapping it starts giving DMA errors.

Following error observed. first two prints are added for debugging in chcr. 
Kernel version used to reproduce is 4.9.28 on x86_64 with Page size 4K.

Sep 15 12:40:52 heptagon kernel: process_cipher req src 8803cb41f0a8
Sep 15 12:40:52 heptagon kernel: = issuehit offset:4110 === 
dma_addr f24b000e ==> DMA mapped address returned by dma_map_sg()

Sep 15 12:40:52 heptagon kernel: DMAR: DRHD: handling fault status reg 2
Sep 15 12:40:52 heptagon kernel: DMAR: [DMA Write] Request device [02:00.4] 
fault addr f24b [fault reason 05] PTE Write access is not set

>
> Casey

Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

[Casey Leedom]

| From: Raj, Ashok 
| Sent: Monday, September 25, 2017 12:03 PM
|   
| On Mon, Sep 25, 2017 at 01:11:04PM -0700, Dan Williams wrote:
| > On Mon, Sep 25, 2017 at 1:05 PM, Casey Leedom  wrote:
| > > | From: Dan Williams 
| > > | Sent: Monday, September 25, 2017 12:31 PM
| > > | ...
| > > | IIUC it looks like this has been broken ever since commit e1605495c716
| > > | "intel-iommu: Introduce domain_sg_mapping() to speed up
| > > | intel_map_sg()". I.e. it looks like the calculation for pte_val should
| > > | be:
| > > |
| > > | pteval = (page_to_phys(sg_page(sg)) + sg->offset) | prot;
| > >
| > > Hhmmm, shouldn't that be:
| > >
| > > pteval = (page_to_phys(sg_page(sg)) + (sg->offset>>PAGE_SHIFT)) | 
prot;
| >
| > Yes, I think you're right. We do want to mask off the page-unaligned
| > portion of sg->offset.
|
| Shoulnd't we normalize the entire sg_page(sg) + sg_offset.
|
| if when you only mask the page-unaligned portion i suspect you might be
| pointing to a different region?
|
| something like (sg_page(sg) + (sg->offset << VTD_PAGE_SHIFT))
|
| then add the unaligned part.. sg->offset>>VTD_PAGE_SHIFT
|
| Is this happening because you are using a 2M page? not sure what triggers
| this or causes the driver to get passed in larger than 4K offset, or
| running 32bit kernel?
|
| if its legal to get passed in such odd values, we should fix IOMMU driver to
| handle it properly, otherwise we should atleast fail those requests.

  (woof) This is all above me.  I've spent a chunk of time fruitlessly
trying to find documentation which says that scatterlist's are allowed to
have offset/length values which extend outside the sg_page(sg).  So someone
much more familiar with this stuff is going to need to say what's allowed.

  As I said, I've asked Harsh to provide us with a detailed trace of exactly
what he's seeing and what the Scatter/Gather Lists are getting translated
into.  That information may make it easier to understand if/how
__domain_mapping() is screwing up ...

Casey

Re: [PATCH v5 REPOST 1/6] hw_random: place mutex around read functions and buffers.

[Dmitry Torokhov]

A bit late to a party, but:

On Mon, Dec 8, 2014 at 12:50 AM, Amos Kong  wrote:
> From: Rusty Russell 
>
> There's currently a big lock around everything, and it means that we
> can't query sysfs (eg /sys/devices/virtual/misc/hw_random/rng_current)
> while the rng is reading.  This is a real problem when the rng is slow,
> or blocked (eg. virtio_rng with qemu's default /dev/random backend)
>
> This doesn't help (it leaves the current lock untouched), just adds a
> lock to protect the read function and the static buffers, in preparation
> for transition.
>
> Signed-off-by: Rusty Russell 
> ---
...
>
> @@ -160,13 +166,14 @@ static ssize_t rng_dev_read(struct file *filp, char 
> __user *buf,
> goto out_unlock;
> }
>
> +   mutex_lock(_mutex);

I think this breaks O_NONBLOCK: we have hwrng core thread that is
constantly pumps underlying rng for data; the thread takes the mutex
and calls rng_get_data() that blocks until RNG responds. This means
that even user specified O_NONBLOCK here we'll be waiting until
[hwrng] thread releases reading_mutex before we can continue.

> if (!data_avail) {
> bytes_read = rng_get_data(current_rng, rng_buffer,
> rng_buffer_size(),
> !(filp->f_flags & O_NONBLOCK));
> if (bytes_read < 0) {
> err = bytes_read;
> -   goto out_unlock;
> +   goto out_unlock_reading;
> }
> data_avail = bytes_read;
> }

Thanks.

-- 
Dmitry

Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

[Dan Williams]

On Mon, Sep 25, 2017 at 1:05 PM, Casey Leedom  wrote:
> | From: Dan Williams 
> | Sent: Monday, September 25, 2017 12:31 PM
> | ...
> | IIUC it looks like this has been broken ever since commit e1605495c716
> | "intel-iommu: Introduce domain_sg_mapping() to speed up
> | intel_map_sg()". I.e. it looks like the calculation for pte_val should
> | be:
> |
> | pteval = (page_to_phys(sg_page(sg)) + sg->offset) | prot;
>
> Hhmmm, shouldn't that be:
>
> pteval = (page_to_phys(sg_page(sg)) + (sg->offset>>PAGE_SHIFT)) | prot;

Yes, I think you're right. We do want to mask off the page-unaligned
portion of sg->offset.

Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

[Casey Leedom]

| From: Dan Williams 
| Sent: Monday, September 25, 2017 12:31 PM
| ...
| IIUC it looks like this has been broken ever since commit e1605495c716
| "intel-iommu: Introduce domain_sg_mapping() to speed up
| intel_map_sg()". I.e. it looks like the calculation for pte_val should
| be:
| 
|     pteval = (page_to_phys(sg_page(sg)) + sg->offset) | prot;

Hhmmm, shouldn't that be:

pteval = (page_to_phys(sg_page(sg)) + (sg->offset>>PAGE_SHIFT)) | prot;

???

Casey

Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

[Dan Williams]

On Mon, Sep 25, 2017 at 10:46 AM, Casey Leedom  wrote:
> | From: Robin Murphy 
> | Sent: Wednesday, September 20, 2017 3:12 AM
> |
> | On 20/09/17 09:01, Herbert Xu wrote:
> | >
> | > Harsh Jain  wrote:
> | >>
> | >> While debugging DMA mapping error in chelsio crypto driver we
> | >> observed that when scatter/gather list received by driver has
> | >> some entry with page->offset > 4096 (PAGE_SIZE). It starts
> | >> giving DMA error.  Without IOMMU it works fine.
> | >
> | > This is not a bug.  The network stack can and will feed us such
> | > SG lists.
> | >
> | >> 2) It cannot be driver's responsibilty to update received sg
> | >> entries to adjust offset and page because we are not the only
> | >> one who directly uses received sg list.
> | >
> | > No the driver must deal with this.  Having said that, if we can
> | > improve our driver helper interface to make this easier then we
> | > should do that too.  What we certainly shouldn't do is to take a
> | > whack-a-mole approach like this patch does.
> |
> | AFAICS this is entirely on intel-iommu - from a brief look it appears
> | that all the IOVA calculations would handle the offset correctly, but
> | then __domain_mapping() blindly uses sg_page() for the physical address,
> | so if offset is larger than a page it would end up with the DMA mapping
> | covering the wrong part of the buffer.
> |
> | Does the diff below help?
> |
> | Robin.
> |
> | ->8-
> | diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> | index b3914fce8254..2ed43d928135 100644
> | --- a/drivers/iommu/intel-iommu.c
> | +++ b/drivers/iommu/intel-iommu.c
> | @@ -2253,7 +2253,7 @@ static int __domain_mapping(struct dmar_domain 
> *domain, unsigned long iov_pfn,
> |  sg_res = aligned_nrpages(sg->offset, sg->length);
> |  sg->dma_address = ((dma_addr_t)iov_pfn << 
> VTD_PAGE_SHIFT) + sg->offset;
> |  sg->dma_length = sg->length;
> | -   pteval = page_to_phys(sg_page(sg)) | prot;
> | +   pteval = (sg_phys(sg) & PAGE_MASK) | prot;

This breaks on platforms where sizeof(phys_addr_t) > sizeof(unsigned
long). I.e. it's not always safe to assume that PAGE_MASK is the
correct width.

> |  phys_pfn = pteval >> VTD_PAGE_SHIFT;
> |  }
>
>   Adding some likely people to the Cc list so they can comment on this.
> Dan Williams submitted that specific piece of code in kernel.org:3e6110fd54
> ... but there are lots of similar bits in that function.  Hopefully one of
> the Intel I/O MMU Gurus will have a better idea of what may be going wrong
> here.  In the mean time I've asked our team to gather far more detailed
> debug traces showing the exact Scatter/Gather Lists we're getting, what they
> get translated to in the DMA Mappings, and what DMA Addresses were seeing in
> error.

IIUC it looks like this has been broken ever since commit e1605495c716
"intel-iommu: Introduce domain_sg_mapping() to speed up
intel_map_sg()". I.e. it looks like the calculation for pte_val should
be:

pteval = (page_to_phys(sg_page(sg)) + sg->offset) | prot;

Re: [PATCH] block: cryptoloop - Fix build warning

[Jens Axboe]

On 09/25/2017 11:10 AM, Corentin Labbe wrote:
> This patch fix the following build warning:
> drivers/block/cryptoloop.c:46:8: warning: variable 'cipher' set but not used 
> [-Wunused-but-set-variable]

Thanks, added for 4.15.

-- 
Jens Axboe

Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

[David Woodhouse]

On Wed, 2017-09-20 at 16:01 +0800, Herbert Xu wrote:
> Harsh Jain  wrote:
> > 
> > While debugging DMA mapping error in chelsio crypto driver we
> observed that when scatter/gather list received by driver has some
> entry with page->offset > 4096 (PAGE_SIZE). It starts giving DMA
> error.  Without IOMMU it works fine.
> 
> This is not a bug.  The network stack can and will feed us such
> SG lists.

Hm? Under what circumstances is the offset permitted to be >
PAGE_SIZE? 

smime.p7s
Description: S/MIME cryptographic signature

Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

[Raj, Ashok]

Hi Casey

Sorry, somehow didn't see this one come by.


On Mon, Sep 25, 2017 at 05:46:40PM +, Casey Leedom wrote:
> | From: Robin Murphy 
> | Sent: Wednesday, September 20, 2017 3:12 AM
> |
> | On 20/09/17 09:01, Herbert Xu wrote:
> | >
> | > Harsh Jain  wrote:
> | >>
> | >> While debugging DMA mapping error in chelsio crypto driver we
> | >> observed that when scatter/gather list received by driver has
> | >> some entry with page->offset > 4096 (PAGE_SIZE). It starts
> | >> giving DMA error.  Without IOMMU it works fine.

Not sure how the page->offset would end up being greater than page-size?

If you have additional traces, please send them by. 

Is this a new driver? wondering how we didn't run into this?


Cheers,
Ashok

Re: [PATCH v2] crypto: s5p-sss: Add HASH support for Exynos

[Krzysztof Kozlowski]

On Fri, Sep 22, 2017 at 08:00:17PM +0200, Kamil Konieczny wrote:
> On 19.09.2017 21:03, Krzysztof Kozlowski wrote:
> > On Fri, Sep 15, 2017 at 07:50:06PM +0200, Kamil Konieczny wrote:

(...)

> >> +
> >> +/* HASH flags */
> >
> > All defines below have "HASH_FLAGS" prefix... so actually useful would
> > be to explain also what is a hash flag.
> 
> These are bit numbers used by device driver, setting in dev->hash_flags,
> used with set_bit(), clear_bit() or test_bit(), and with macro BIT(),
> to keep device state BUSY or FREE, or to signal state from irq_handler
> to hash_tasklet.

Then add something similar as comment to the code.

>  
> >> +#define HASH_FLAGS_BUSY   0
> >> +#define HASH_FLAGS_FINAL  1
> >> +#define HASH_FLAGS_DMA_ACTIVE 2
> >> +#define HASH_FLAGS_OUTPUT_READY   3
> >> +#define HASH_FLAGS_INIT   4
> >> +#define HASH_FLAGS_DMA_READY  6
> >> +
> >> +#define HASH_FLAGS_SGS_COPIED 9
> >> +#define HASH_FLAGS_SGS_ALLOCED10
> >> +/* HASH context flags */
> >
> > Same here.
> 
> These are bit numbers internal for request context, reqctx->flags
> 
> >
> >> +#define HASH_FLAGS_FINUP  16
> >> +#define HASH_FLAGS_ERROR  17
> >> +
> >> +#define HASH_FLAGS_MODE_MD5   18
> >> +#define HASH_FLAGS_MODE_SHA1  19
> >> +#define HASH_FLAGS_MODE_SHA25620
> >
> > These are set and not read?
> 
> It is leftover from omap driver, it was used in hmac alg.
> I will remove it.
> 
> >
> >> +
> >> +#define HASH_FLAGS_MODE_MASK  (BIT(18) | BIT(19) | BIT(20))
> >
> > Not used.
> >
> >> +/* HASH op codes */
> >> +#define HASH_OP_UPDATE1
> >> +#define HASH_OP_FINAL 2
> >> +
> >> +/* HASH HW constants */
> >> +#define HASH_ALIGN_MASK   (HASH_BLOCK_SIZE-1)
> >
> > Not used.
> >
> >> +
> >> +#define BUFLENHASH_BLOCK_SIZE
> >> +
> >> +#define SSS_DMA_ALIGN 16
> >> +#define SSS_ALIGNED   __attribute__((aligned(SSS_DMA_ALIGN)))
> >> +#define SSS_DMA_ALIGN_MASK(SSS_DMA_ALIGN-1)
> >
> > Please make it consistent with existing code, e.g.: by replacing
> > AES .cra_alignmask with same macro. In separate patch of course.
> 
> Thank you, I will do it in next patches.
> 
> DMA align for AES is 16, and for HASH is 8 (both aligned up),
> but I think it is simpler to have it both 16. This align is for length,
> e.g when len is not divisible by 8 (HASH case), DMA FC will round it up
> by 8, but HASH IP consumes only bytes set by len registers (so HASH will
> be correctly computed).
> 
> >
> >> +
> >> +/* HASH queue constant */
> >
> > Pretty useless comment. Do not use comments which copy the code. You
> > could explain here why you have chosen 10 (existing AES code uses 1).
> >
> >> +#define SSS_HASH_QUEUE_LENGTH 10
> 
> Number 10 was copied from omap-sham.c
> 
> The question why to use queue in device driver is good topic for RFC,
> I do not see any advantages for it except for corner cases,
> when driver receives many digest() request,
> or many update() from different contexts.
> 
> Basically it will not queue more than one update() from one context
> because the state for HASH is kept in request context, e.g. once 
> crypto user calls update() it must wait for its end before sending next.
> 
> It does have some effect on design, as it allows copy bytes for small
> update into request context (instead of putting it in queue).
> 
> >> +
> >> +/**
> >> + * struct sss_hash_algs_info - platform specific SSS HASH algorithms
> >> + * @algs_list:array of transformations (algorithms)
> >> + * @size: size
> >> + * @registered:   counter used at probe/remove
> >> + *
> >> + * Specifies platform specific information about hash algorithms
> >> + * of SSS module.
> >> + */
> >> +struct sss_hash_algs_info {
> >> +  struct ahash_alg*algs_list;
> >> +  unsigned intsize;
> >> +  unsigned intregistered;
> >> +};
> >> +
> >>  /**
> >>   * struct samsung_aes_variant - platform specific SSS driver data
> >>   * @aes_offset: AES register offset from SSS module's base.
> >> + * @hash_offset: HASH register offset from SSS module's base.
> >> + *
> >> + * @hash_algs_info: HASH transformations provided by SS module
> >> + * @hash_algs_size: size of hash_algs_info
> >>   *
> >>   * Specifies platform specific configuration of SSS module.
> >>   * Note: A structure for driver specific platform data is used for future
> >> @@ -156,6 +279,10 @@
> >>   */
> >>  struct samsung_aes_variant {
> >>unsigned intaes_offset;
> >> +  unsigned inthash_offset;
> >> +
> >> +  struct sss_hash_algs_info   *hash_algs_info;
> >> +  unsigned inthash_algs_size;
> >>  };
> >>  
> >>  struct s5p_aes_reqctx {
> >> @@ -194,7 +321,21 @@ struct s5p_aes_ctx {
> >>   *req, ctx, sg_src/dst (and copies).  This essentially
> >>   *protects against concurrent access to these fields.
> >>   * @lock: Lock for protecting both 

Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

[Casey Leedom]

| From: Robin Murphy 
| Sent: Wednesday, September 20, 2017 3:12 AM
|
| On 20/09/17 09:01, Herbert Xu wrote:
| >
| > Harsh Jain  wrote:
| >>
| >> While debugging DMA mapping error in chelsio crypto driver we
| >> observed that when scatter/gather list received by driver has
| >> some entry with page->offset > 4096 (PAGE_SIZE). It starts
| >> giving DMA error.  Without IOMMU it works fine.
| >
| > This is not a bug.  The network stack can and will feed us such
| > SG lists.
| >
| >> 2) It cannot be driver's responsibilty to update received sg
| >> entries to adjust offset and page because we are not the only
| >> one who directly uses received sg list.
| >
| > No the driver must deal with this.  Having said that, if we can
| > improve our driver helper interface to make this easier then we
| > should do that too.  What we certainly shouldn't do is to take a
| > whack-a-mole approach like this patch does.
|
| AFAICS this is entirely on intel-iommu - from a brief look it appears
| that all the IOVA calculations would handle the offset correctly, but
| then __domain_mapping() blindly uses sg_page() for the physical address,
| so if offset is larger than a page it would end up with the DMA mapping
| covering the wrong part of the buffer.
|
| Does the diff below help?
|
| Robin.
|
| ->8-
| diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
| index b3914fce8254..2ed43d928135 100644
| --- a/drivers/iommu/intel-iommu.c
| +++ b/drivers/iommu/intel-iommu.c
| @@ -2253,7 +2253,7 @@ static int __domain_mapping(struct dmar_domain *domain, 
unsigned long iov_pfn,
|  sg_res = aligned_nrpages(sg->offset, sg->length);
|  sg->dma_address = ((dma_addr_t)iov_pfn << 
VTD_PAGE_SHIFT) + sg->offset;
|  sg->dma_length = sg->length;
| -   pteval = page_to_phys(sg_page(sg)) | prot;
| +   pteval = (sg_phys(sg) & PAGE_MASK) | prot;
|  phys_pfn = pteval >> VTD_PAGE_SHIFT;
|  }

  Adding some likely people to the Cc list so they can comment on this.
Dan Williams submitted that specific piece of code in kernel.org:3e6110fd54
... but there are lots of similar bits in that function.  Hopefully one of
the Intel I/O MMU Gurus will have a better idea of what may be going wrong
here.  In the mean time I've asked our team to gather far more detailed
debug traces showing the exact Scatter/Gather Lists we're getting, what they
get translated to in the DMA Mappings, and what DMA Addresses were seeing in
error.

Casey

[PATCH] block: cryptoloop - Fix build warning

[Corentin Labbe]

This patch fix the following build warning:
drivers/block/cryptoloop.c:46:8: warning: variable 'cipher' set but not used 
[-Wunused-but-set-variable]

Signed-off-by: Corentin Labbe 
---
 drivers/block/cryptoloop.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/block/cryptoloop.c b/drivers/block/cryptoloop.c
index 74e03aa537ad..7033a4beda66 100644
--- a/drivers/block/cryptoloop.c
+++ b/drivers/block/cryptoloop.c
@@ -43,7 +43,6 @@ cryptoloop_init(struct loop_device *lo, const struct 
loop_info64 *info)
int cipher_len;
int mode_len;
char cms[LO_NAME_SIZE]; /* cipher-mode string */
-   char *cipher;
char *mode;
char *cmsp = cms;   /* c-m string pointer */
struct crypto_skcipher *tfm;
@@ -56,7 +55,6 @@ cryptoloop_init(struct loop_device *lo, const struct 
loop_info64 *info)
strncpy(cms, info->lo_crypt_name, LO_NAME_SIZE);
cms[LO_NAME_SIZE - 1] = 0;
 
-   cipher = cmsp;
cipher_len = strcspn(cmsp, "-");
 
mode = cmsp + cipher_len;
-- 
2.13.5

Re: [RESEND RFC PATCH 2/2] Bluetooth: let the crypto subsystem generate the ecc privkey

[Tudor Ambarus]

Hi, Marcel,

Agreed on all suggestions, I will send a v2 patch set.

Thanks,
ta

Re: [RESEND RFC PATCH 1/2] Bluetooth: move ecdh allocation outside of ecdh_helper

[Tudor Ambarus]

Hi, Marcel,

On 09/25/2017 04:02 PM, Marcel Holtmann wrote:


diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index a0ef897..6532689 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c


[cut]


@@ -2677,7 +2695,16 @@ static int smp_cmd_public_key(struct l2cap_conn *conn, 
struct sk_buff *skb)
SMP_DBG("Remote Public Key X: %32phN", smp->remote_pk);
SMP_DBG("Remote Public Key Y: %32phN", smp->remote_pk + 32);

-   if (!compute_ecdh_secret(smp->remote_pk, smp->local_sk, smp->dhkey))
+   if (test_bit(SMP_FLAG_LOCAL_OOB, >flags)) {
+   struct smp_dev *smp_dev = chan->data;
+
+   tfm = smp_dev->tfm_ecdh;
+   } else {
+   tfm = smp->tfm_ecdh;
+   }


this looks all good, but I do not understand the need of this extra if clause 
here. Can you explain why OOB case is different or maybe add some comment in 
the code to make this clear.


This particular change should have been in patch 2/2.
It's needed because I need to compute the shared secret on the
same tfm on which the private key was set/generated.

Thanks,
ta

Re: [RESEND RFC PATCH 2/2] Bluetooth: let the crypto subsystem generate the ecc privkey

[Marcel Holtmann]

Hi Tudor,

> That Bluetooth SMP knows about the private key is pointless, since the
> detection of debug key usage is actually via the public key portion.
> With this patch, the Bluetooth SMP will stop keeping a copy of the
> ecdh private key, except when using debug keys. This way we let the
> crypto subsystem to generate and handle the ecdh private key,
> potentially benefiting of hardware ecc private key generation and
> retention.
> 
> The loop that tries to generate a correct private key is now removed and
> we trust the crypto subsystem to generate a correct private key. This
> backup logic should be done in crypto, if really needed.
> 
> Keeping the private key in the crypto subsystem implies that we can't
> check if we accidentally generated a debug key. As this event is
> unlikely we can live with it when comparing with the benefit of the
> overall change: hardware private key generation and retention.
> 
> Signed-off-by: Tudor Ambarus 
> ---
> net/bluetooth/ecdh_helper.c | 102 +---
> net/bluetooth/smp.c |  55 +---
> 2 files changed, 67 insertions(+), 90 deletions(-)
> 
> diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
> index ac2c708..56e9374 100644
> --- a/net/bluetooth/ecdh_helper.c
> +++ b/net/bluetooth/ecdh_helper.c
> @@ -53,10 +53,10 @@ bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
> public_key[64],
>const u8 private_key[32], u8 secret[32])
> {
>   struct kpp_request *req;
> - struct ecdh p;
> + struct ecdh p = {0};
>   struct ecdh_completion result;
>   struct scatterlist src, dst;
> - u8 *tmp, *buf;
> + u8 *tmp, *buf = NULL;
>   unsigned int buf_len;
>   int err = -ENOMEM;
> 
> @@ -70,25 +70,29 @@ bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
> public_key[64],
> 
>   init_completion();
> 
> - /* Security Manager Protocol holds digits in litte-endian order
> -  * while ECC API expect big-endian data
> -  */
> - swap_digits((u64 *)private_key, (u64 *)tmp, 4);
> - p.key = (char *)tmp;
> - p.key_size = 32;
>   /* Set curve_id */
>   p.curve_id = ECC_CURVE_NIST_P256;
> - buf_len = crypto_ecdh_key_len();
> - buf = kmalloc(buf_len, GFP_KERNEL);
> - if (!buf)
> - goto free_req;
> 
> - crypto_ecdh_encode_key(buf, buf_len, );
> + if (private_key) {
> + /* Security Manager Protocol holds digits in little-endian order
> +  * while ECC API expect big-endian data.
> +  */
> + swap_digits((u64 *)private_key, (u64 *)tmp, 4);
> + p.key = (char *)tmp;
> + p.key_size = 32;
> 
> - /* Set A private Key */
> - err = crypto_kpp_set_secret(tfm, (void *)buf, buf_len);
> - if (err)
> - goto free_all;
> + buf_len = crypto_ecdh_key_len();
> + buf = kmalloc(buf_len, GFP_KERNEL);
> + if (!buf)
> + goto free_req;
> +
> + crypto_ecdh_encode_key(buf, buf_len, );
> +
> + /* Set A private Key */
> + err = crypto_kpp_set_secret(tfm, (void *)buf, buf_len);
> + if (err)
> + goto free_all;
> + }
> 
>   swap_digits((u64 *)public_key, (u64 *)tmp, 4); /* x */
>   swap_digits((u64 *)_key[32], (u64 *)[32], 4); /* y */
> @@ -126,14 +130,12 @@ bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 
> public_key[64],
>   u8 private_key[32])
> {
>   struct kpp_request *req;
> - struct ecdh p;
> + struct ecdh p = {0};
>   struct ecdh_completion result;
>   struct scatterlist dst;
>   u8 *tmp, *buf;
>   unsigned int buf_len;
>   int err = -ENOMEM;
> - const unsigned short max_tries = 16;
> - unsigned short tries = 0;
> 
>   tmp = kmalloc(64, GFP_KERNEL);
>   if (!tmp)
> @@ -145,56 +147,48 @@ bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 
> public_key[64],
> 
>   init_completion();
> 
> + /* Set private Key */
> + if (private_key) {
> + p.key = (char *)private_key;
> + p.key_size = 32;
> + }
> +
>   /* Set curve_id */
>   p.curve_id = ECC_CURVE_NIST_P256;
> - p.key_size = 32;
>   buf_len = crypto_ecdh_key_len();
>   buf = kmalloc(buf_len, GFP_KERNEL);
>   if (!buf)
>   goto free_req;
> 
> - do {
> - if (tries++ >= max_tries)
> - goto free_all;
> -
> - /* Set private Key */
> - p.key = (char *)private_key;
> - crypto_ecdh_encode_key(buf, buf_len, );
> - err = crypto_kpp_set_secret(tfm, buf, buf_len);
> - if (err)
> - goto free_all;
> -
> - sg_init_one(, tmp, 64);
> - kpp_request_set_input(req, NULL, 0);
> - kpp_request_set_output(req, , 64);
> -

Re: [RESEND RFC PATCH 1/2] Bluetooth: move ecdh allocation outside of ecdh_helper

[Marcel Holtmann]

Hi Tudor,

> This change is a prerequisite for letting the crypto subsystem generate
> the ecc private key for ecdh. Before this change, a new crypto tfm was
> allocated, each time, for both key generation and shared secret
> computation. With this change, we allocate a single tfm for both cases.
> We need to bind the key pair generation with the shared secret
> computation via the same crypto tfm. Once the key is set, we can
> compute the shared secret without referring to the private key.
> 
> Signed-off-by: Tudor Ambarus 
> ---
> net/bluetooth/ecdh_helper.c | 32 ---
> net/bluetooth/ecdh_helper.h |  8 +++--
> net/bluetooth/selftest.c| 29 -
> net/bluetooth/smp.c | 77 +
> 4 files changed, 96 insertions(+), 50 deletions(-)
> 
> diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
> index c7b1a9a..ac2c708 100644
> --- a/net/bluetooth/ecdh_helper.c
> +++ b/net/bluetooth/ecdh_helper.c
> @@ -23,7 +23,6 @@
> #include "ecdh_helper.h"
> 
> #include 
> -#include 
> #include 
> 
> struct ecdh_completion {
> @@ -50,10 +49,9 @@ static inline void swap_digits(u64 *in, u64 *out, unsigned 
> int ndigits)
>   out[i] = __swab64(in[ndigits - 1 - i]);
> }
> 
> -bool compute_ecdh_secret(const u8 public_key[64], const u8 private_key[32],
> -  u8 secret[32])
> +bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 public_key[64],
> +  const u8 private_key[32], u8 secret[32])
> {
> - struct crypto_kpp *tfm;
>   struct kpp_request *req;
>   struct ecdh p;
>   struct ecdh_completion result;
> @@ -66,16 +64,9 @@ bool compute_ecdh_secret(const u8 public_key[64], const u8 
> private_key[32],
>   if (!tmp)
>   return false;
> 
> - tfm = crypto_alloc_kpp("ecdh", CRYPTO_ALG_INTERNAL, 0);
> - if (IS_ERR(tfm)) {
> - pr_err("alg: kpp: Failed to load tfm for kpp: %ld\n",
> -PTR_ERR(tfm));
> - goto free_tmp;
> - }
> -
>   req = kpp_request_alloc(tfm, GFP_KERNEL);
>   if (!req)
> - goto free_kpp;
> + goto free_tmp;
> 
>   init_completion();
> 
> @@ -126,16 +117,14 @@ bool compute_ecdh_secret(const u8 public_key[64], const 
> u8 private_key[32],
>   kzfree(buf);
> free_req:
>   kpp_request_free(req);
> -free_kpp:
> - crypto_free_kpp(tfm);
> free_tmp:
>   kfree(tmp);
>   return (err == 0);
> }
> 
> -bool generate_ecdh_keys(u8 public_key[64], u8 private_key[32])
> +bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
> + u8 private_key[32])
> {
> - struct crypto_kpp *tfm;
>   struct kpp_request *req;
>   struct ecdh p;
>   struct ecdh_completion result;
> @@ -150,16 +139,9 @@ bool generate_ecdh_keys(u8 public_key[64], u8 
> private_key[32])
>   if (!tmp)
>   return false;
> 
> - tfm = crypto_alloc_kpp("ecdh", CRYPTO_ALG_INTERNAL, 0);
> - if (IS_ERR(tfm)) {
> - pr_err("alg: kpp: Failed to load tfm for kpp: %ld\n",
> -PTR_ERR(tfm));
> - goto free_tmp;
> - }
> -
>   req = kpp_request_alloc(tfm, GFP_KERNEL);
>   if (!req)
> - goto free_kpp;
> + goto free_tmp;
> 
>   init_completion();
> 
> @@ -218,8 +200,6 @@ bool generate_ecdh_keys(u8 public_key[64], u8 
> private_key[32])
>   kzfree(buf);
> free_req:
>   kpp_request_free(req);
> -free_kpp:
> - crypto_free_kpp(tfm);
> free_tmp:
>   kfree(tmp);
>   return (err == 0);
> diff --git a/net/bluetooth/ecdh_helper.h b/net/bluetooth/ecdh_helper.h
> index 7a423fa..5cde37d 100644
> --- a/net/bluetooth/ecdh_helper.h
> +++ b/net/bluetooth/ecdh_helper.h
> @@ -20,8 +20,10 @@
>  * COPYRIGHTS, TRADEMARKS OR OTHER RIGHTS, RELATING TO USE OF THIS
>  * SOFTWARE IS DISCLAIMED.
>  */
> +#include 
> #include 
> 
> -bool compute_ecdh_secret(const u8 pub_a[64], const u8 priv_b[32],
> -  u8 secret[32]);
> -bool generate_ecdh_keys(u8 public_key[64], u8 private_key[32]);
> +bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 pub_a[64],
> +  const u8 priv_b[32], u8 secret[32]);
> +bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
> + u8 private_key[32]);
> diff --git a/net/bluetooth/selftest.c b/net/bluetooth/selftest.c
> index 34a1227..126bdc5 100644
> --- a/net/bluetooth/selftest.c
> +++ b/net/bluetooth/selftest.c
> @@ -138,9 +138,9 @@ static const u8 dhkey_3[32] __initconst = {
>   0x7c, 0x1c, 0xf9, 0x49, 0xe6, 0xd7, 0xaa, 0x70,
> };
> 
> -static int __init test_ecdh_sample(const u8 priv_a[32], const u8 priv_b[32],
> -const u8 pub_a[64], const u8 pub_b[64],
> -const u8 dhkey[32])
> +static int __init test_ecdh_sample(struct crypto_kpp *tfm, const u8 
> priv_a[32],
> + 

[PATCH] crypto: talitos - fix setkey to check key weakness

[Christophe Leroy]

Crypto manager test report the following failures:
[3.061081] alg: skcipher: setkey failed on test 5 for ecb-des-talitos: 
flags=100
[3.069342] alg: skcipher-ddst: setkey failed on test 5 for ecb-des-talitos: 
flags=100
[3.077754] alg: skcipher-ddst: setkey failed on test 5 for ecb-des-talitos: 
flags=100

This is due to setkey being expected to detect weak keys.

Signed-off-by: Christophe Leroy 
---
 drivers/crypto/talitos.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index e7e31f8fd3d1..09a2cd3a31d2 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1494,12 +1494,20 @@ static int ablkcipher_setkey(struct crypto_ablkcipher 
*cipher,
 const u8 *key, unsigned int keylen)
 {
struct talitos_ctx *ctx = crypto_ablkcipher_ctx(cipher);
+   u32 tmp[DES_EXPKEY_WORDS];
 
if (keylen > TALITOS_MAX_KEY_SIZE) {
crypto_ablkcipher_set_flags(cipher, CRYPTO_TFM_RES_BAD_KEY_LEN);
return -EINVAL;
}
 
+   if (unlikely(crypto_ablkcipher_get_flags(cipher) &
+CRYPTO_TFM_REQ_WEAK_KEY) &&
+   !des_ekey(tmp, key)) {
+   crypto_ablkcipher_set_flags(cipher, CRYPTO_TFM_RES_WEAK_KEY);
+   return -EINVAL;
+   }
+
memcpy(>key, key, keylen);
ctx->keylen = keylen;
 
-- 
2.13.3

[PATCH] crypto: talitos - fix AEAD for sha224 on non sha224 capable chips

[Christophe Leroy]

sha224 AEAD test fails with:

[2.803125] talitos ff02.crypto: DEUISR 0x_
[2.808743] talitos ff02.crypto: MDEUISR 0x8010_
[2.814678] talitos ff02.crypto: DESCBUF 0x20731f21_0018
[2.820616] talitos ff02.crypto: DESCBUF 0x0628d64c_0010
[2.826554] talitos ff02.crypto: DESCBUF 0x0631005c_0018
[2.832492] talitos ff02.crypto: DESCBUF 0x0628d664_0008
[2.838430] talitos ff02.crypto: DESCBUF 0x061b13a0_0080
[2.844369] talitos ff02.crypto: DESCBUF 0x0631006c_0080
[2.850307] talitos ff02.crypto: DESCBUF 0x0631006c_0018
[2.856245] talitos ff02.crypto: DESCBUF 0x063100ec_
[2.884972] talitos ff02.crypto: failed to reset channel 0
[2.890503] talitos ff02.crypto: done overflow, internal time out, or 
rngu error: ISR 0x2000_0002
[2.900652] alg: aead: encryption failed on test 1 for 
authenc-hmac-sha224-cbc-3des-talitos: ret=22

This is due to SHA224 not being supported by the HW. Allthough for
hash we are able to init the hash contet by SW, it is not
possible for AEAD. Therefore SHA224 AEAD has to be deactivated.

Signed-off-by: Christophe Leroy 
---
 drivers/crypto/talitos.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 09a2cd3a31d2..9bfda9d59ace 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -3045,6 +3045,11 @@ static struct talitos_crypto_alg 
*talitos_alg_alloc(struct device *dev,
t_alg->algt.alg.aead.setkey = aead_setkey;
t_alg->algt.alg.aead.encrypt = aead_encrypt;
t_alg->algt.alg.aead.decrypt = aead_decrypt;
+   if (!(priv->features & TALITOS_FTR_SHA224_HWINIT) &&
+   !strncmp(alg->cra_name, "authenc(hmac(sha224)", 20)) {
+   kfree(t_alg);
+   return ERR_PTR(-ENOTSUPP);
+   }
break;
case CRYPTO_ALG_TYPE_AHASH:
alg = _alg->algt.alg.hash.halg.base;
-- 
2.13.3

Re: [PATCH v2] crypto: s5p-sss: Add HASH support for Exynos

[Kamil Konieczny]

On 19.09.2017 21:03, Krzysztof Kozlowski wrote:
> On Fri, Sep 15, 2017 at 07:50:06PM +0200, Kamil Konieczny wrote:
>> Add support for MD5, SHA1, SHA256 hash algorithms for Exynos HW.
>> It uses the crypto framework asynchronous hash api.
>> It is based on omap-sham.c driver.
>> S5P has some HW differencies and is not implemented.
>>
>> Modifications in s5p-sss: [...]

Below I will address only 'const' questions.

>>[...]
>> +struct crypto_queue hash_queue;
>> +struct ahash_request*hash_req;
>> +struct scatterlist  *hash_sg_iter;
>> +int hash_sg_cnt;
>> +
>> +struct samsung_aes_variant  *pdata;
> 
> This should be const as pdata should not be modified.

I will remove this.

>>  [...]
>> -static const struct samsung_aes_variant s5p_aes_data = {
>> +static struct samsung_aes_variant s5p_aes_data = {
> 
> Why do you need to drop the const? This should not be modified.

OK, I will not modify this.

> [...]
>> [...]
>> + */
>> +static struct sss_hash_algs_info exynos_hash_algs_info[] = {
> 
> Can it be const?

No, it contains '.registered' var, used at probe/error path/remove.

>>[...]

-- 
Best regards,
Kamil Konieczny
Samsung R Institute Poland

[PATCH 1/5] crypto: omap-aes: pr_err() strings should end with newlines

[Arvind Yadav]

pr_err() messages should terminated with a new-line to avoid
other messages being concatenated onto the end.

Signed-off-by: Arvind Yadav 
---
 drivers/crypto/omap-aes-gcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/omap-aes-gcm.c b/drivers/crypto/omap-aes-gcm.c
index 7d4f8a4..9f8a1f7 100644
--- a/drivers/crypto/omap-aes-gcm.c
+++ b/drivers/crypto/omap-aes-gcm.c
@@ -214,7 +214,7 @@ static int do_encrypt_iv(struct aead_request *req, u32 
*tag, u32 *iv)
}
/* fall through */
default:
-   pr_err("Encryption of IV failed for GCM mode");
+   pr_err("Encryption of IV failed for GCM mode\n");
break;
}
 
-- 
1.9.1

[PATCH 2/5] crypto: virtio: pr_err() strings should end with newlines

[Arvind Yadav]

pr_err() messages should terminated with a new-line to avoid
other messages being concatenated onto the end.

Signed-off-by: Arvind Yadav 
---
 drivers/crypto/virtio/virtio_crypto_algs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c 
b/drivers/crypto/virtio/virtio_crypto_algs.c
index 5035b0d..abe8c15 100644
--- a/drivers/crypto/virtio/virtio_crypto_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_algs.c
@@ -319,7 +319,7 @@ static int virtio_crypto_ablkcipher_setkey(struct 
crypto_ablkcipher *tfm,
struct virtio_crypto *vcrypto =
  virtcrypto_get_dev_node(node);
if (!vcrypto) {
-   pr_err("virtio_crypto: Could not find a virtio device 
in the system");
+   pr_err("virtio_crypto: Could not find a virtio device 
in the system\n");
return -ENODEV;
}
 
-- 
1.9.1

[PATCH 5/5] crypto: bcm: pr_err() strings should end with newlines

[Arvind Yadav]

pr_err() messages should terminated with a new-line to avoid
other messages being concatenated onto the end.

Signed-off-by: Arvind Yadav 
---
 drivers/crypto/bcm/util.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/crypto/bcm/util.c b/drivers/crypto/bcm/util.c
index 430c557..d543c01 100644
--- a/drivers/crypto/bcm/util.c
+++ b/drivers/crypto/bcm/util.c
@@ -271,7 +271,7 @@ int do_shash(unsigned char *name, unsigned char *result,
hash = crypto_alloc_shash(name, 0, 0);
if (IS_ERR(hash)) {
rc = PTR_ERR(hash);
-   pr_err("%s: Crypto %s allocation error %d", __func__, name, rc);
+   pr_err("%s: Crypto %s allocation error %d\n", __func__, name, 
rc);
return rc;
}
 
@@ -279,7 +279,7 @@ int do_shash(unsigned char *name, unsigned char *result,
sdesc = kmalloc(size, GFP_KERNEL);
if (!sdesc) {
rc = -ENOMEM;
-   pr_err("%s: Memory allocation failure", __func__);
+   pr_err("%s: Memory allocation failure\n", __func__);
goto do_shash_err;
}
sdesc->shash.tfm = hash;
@@ -288,31 +288,31 @@ int do_shash(unsigned char *name, unsigned char *result,
if (key_len > 0) {
rc = crypto_shash_setkey(hash, key, key_len);
if (rc) {
-   pr_err("%s: Could not setkey %s shash", __func__, name);
+   pr_err("%s: Could not setkey %s shash\n", __func__, 
name);
goto do_shash_err;
}
}
 
rc = crypto_shash_init(>shash);
if (rc) {
-   pr_err("%s: Could not init %s shash", __func__, name);
+   pr_err("%s: Could not init %s shash\n", __func__, name);
goto do_shash_err;
}
rc = crypto_shash_update(>shash, data1, data1_len);
if (rc) {
-   pr_err("%s: Could not update1", __func__);
+   pr_err("%s: Could not update1\n", __func__);
goto do_shash_err;
}
if (data2 && data2_len) {
rc = crypto_shash_update(>shash, data2, data2_len);
if (rc) {
-   pr_err("%s: Could not update2", __func__);
+   pr_err("%s: Could not update2\n", __func__);
goto do_shash_err;
}
}
rc = crypto_shash_final(>shash, result);
if (rc)
-   pr_err("%s: Could not generate %s hash", __func__, name);
+   pr_err("%s: Could not generate %s hash\n", __func__, name);
 
 do_shash_err:
crypto_free_shash(hash);
-- 
1.9.1

[PATCH 3/5] crypto: chelsio: pr_err() strings should end with newlines

[Arvind Yadav]

pr_err() messages should terminated with a new-line to avoid
other messages being concatenated onto the end.

Signed-off-by: Arvind Yadav 
---
 drivers/crypto/chelsio/chcr_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/chelsio/chcr_core.c 
b/drivers/crypto/chelsio/chcr_core.c
index b6dd9cb..32618bf 100644
--- a/drivers/crypto/chelsio/chcr_core.c
+++ b/drivers/crypto/chelsio/chcr_core.c
@@ -224,7 +224,7 @@ static int chcr_uld_state_change(void *handle, enum 
cxgb4_state state)
 static int __init chcr_crypto_init(void)
 {
if (cxgb4_register_uld(CXGB4_ULD_CRYPTO, _uld_info))
-   pr_err("ULD register fail: No chcr crypto support in cxgb4");
+   pr_err("ULD register fail: No chcr crypto support in cxgb4\n");
 
return 0;
 }
-- 
1.9.1

[PATCH 0/5] crypto pr_err() strings should end with newlines

[Arvind Yadav]

pr_err() messages should terminated with a new-line to avoid
other messages being concatenated onto the end.

Arvind Yadav (5):
  [PATCH 1/5] crypto: omap-aes: pr_err() strings should end with newlines
  [PATCH 2/5] crypto: virtio: pr_err() strings should end with newlines
  [PATCH 3/5] crypto: chelsio: pr_err() strings should end with newlines
  [PATCH 4/5] crypto: qat: pr_err() strings should end with newlines
  [PATCH 5/5] crypto: bcm: pr_err() strings should end with newlines

 drivers/crypto/bcm/util.c  | 14 +++---
 drivers/crypto/chelsio/chcr_core.c |  2 +-
 drivers/crypto/omap-aes-gcm.c  |  2 +-
 drivers/crypto/qat/qat_common/qat_uclo.c   | 12 ++--
 drivers/crypto/virtio/virtio_crypto_algs.c |  2 +-
 5 files changed, 16 insertions(+), 16 deletions(-)

-- 
1.9.1

[RESEND RFC PATCH 2/2] Bluetooth: let the crypto subsystem generate the ecc privkey

[Tudor Ambarus]

That Bluetooth SMP knows about the private key is pointless, since the
detection of debug key usage is actually via the public key portion.
With this patch, the Bluetooth SMP will stop keeping a copy of the
ecdh private key, except when using debug keys. This way we let the
crypto subsystem to generate and handle the ecdh private key,
potentially benefiting of hardware ecc private key generation and
retention.

The loop that tries to generate a correct private key is now removed and
we trust the crypto subsystem to generate a correct private key. This
backup logic should be done in crypto, if really needed.

Keeping the private key in the crypto subsystem implies that we can't
check if we accidentally generated a debug key. As this event is
unlikely we can live with it when comparing with the benefit of the
overall change: hardware private key generation and retention.

Signed-off-by: Tudor Ambarus 
---
 net/bluetooth/ecdh_helper.c | 102 +---
 net/bluetooth/smp.c |  55 +---
 2 files changed, 67 insertions(+), 90 deletions(-)

diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
index ac2c708..56e9374 100644
--- a/net/bluetooth/ecdh_helper.c
+++ b/net/bluetooth/ecdh_helper.c
@@ -53,10 +53,10 @@ bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
public_key[64],
 const u8 private_key[32], u8 secret[32])
 {
struct kpp_request *req;
-   struct ecdh p;
+   struct ecdh p = {0};
struct ecdh_completion result;
struct scatterlist src, dst;
-   u8 *tmp, *buf;
+   u8 *tmp, *buf = NULL;
unsigned int buf_len;
int err = -ENOMEM;
 
@@ -70,25 +70,29 @@ bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
public_key[64],
 
init_completion();
 
-   /* Security Manager Protocol holds digits in litte-endian order
-* while ECC API expect big-endian data
-*/
-   swap_digits((u64 *)private_key, (u64 *)tmp, 4);
-   p.key = (char *)tmp;
-   p.key_size = 32;
/* Set curve_id */
p.curve_id = ECC_CURVE_NIST_P256;
-   buf_len = crypto_ecdh_key_len();
-   buf = kmalloc(buf_len, GFP_KERNEL);
-   if (!buf)
-   goto free_req;
 
-   crypto_ecdh_encode_key(buf, buf_len, );
+   if (private_key) {
+   /* Security Manager Protocol holds digits in little-endian order
+* while ECC API expect big-endian data.
+*/
+   swap_digits((u64 *)private_key, (u64 *)tmp, 4);
+   p.key = (char *)tmp;
+   p.key_size = 32;
 
-   /* Set A private Key */
-   err = crypto_kpp_set_secret(tfm, (void *)buf, buf_len);
-   if (err)
-   goto free_all;
+   buf_len = crypto_ecdh_key_len();
+   buf = kmalloc(buf_len, GFP_KERNEL);
+   if (!buf)
+   goto free_req;
+
+   crypto_ecdh_encode_key(buf, buf_len, );
+
+   /* Set A private Key */
+   err = crypto_kpp_set_secret(tfm, (void *)buf, buf_len);
+   if (err)
+   goto free_all;
+   }
 
swap_digits((u64 *)public_key, (u64 *)tmp, 4); /* x */
swap_digits((u64 *)_key[32], (u64 *)[32], 4); /* y */
@@ -126,14 +130,12 @@ bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 
public_key[64],
u8 private_key[32])
 {
struct kpp_request *req;
-   struct ecdh p;
+   struct ecdh p = {0};
struct ecdh_completion result;
struct scatterlist dst;
u8 *tmp, *buf;
unsigned int buf_len;
int err = -ENOMEM;
-   const unsigned short max_tries = 16;
-   unsigned short tries = 0;
 
tmp = kmalloc(64, GFP_KERNEL);
if (!tmp)
@@ -145,56 +147,48 @@ bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 
public_key[64],
 
init_completion();
 
+   /* Set private Key */
+   if (private_key) {
+   p.key = (char *)private_key;
+   p.key_size = 32;
+   }
+
/* Set curve_id */
p.curve_id = ECC_CURVE_NIST_P256;
-   p.key_size = 32;
buf_len = crypto_ecdh_key_len();
buf = kmalloc(buf_len, GFP_KERNEL);
if (!buf)
goto free_req;
 
-   do {
-   if (tries++ >= max_tries)
-   goto free_all;
-
-   /* Set private Key */
-   p.key = (char *)private_key;
-   crypto_ecdh_encode_key(buf, buf_len, );
-   err = crypto_kpp_set_secret(tfm, buf, buf_len);
-   if (err)
-   goto free_all;
-
-   sg_init_one(, tmp, 64);
-   kpp_request_set_input(req, NULL, 0);
-   kpp_request_set_output(req, , 64);
-   kpp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
-

[RESEND RFC PATCH 0/2] Bluetooth: let the crypto subsystem generate the ecc privkey

[Tudor Ambarus]

That Bluetooth SMP knows about the private key is pointless, since the
detection of debug key usage is actually via the public key portion.
With this patch set, the Bluetooth SMP will stop keeping a copy of the
ecdh private key, except when using debug keys. This way we let the
crypto subsystem to generate and handle the ecdh private key,
potentially benefiting of hardware ecc private key generation and
retention.

Tested with selftest and with btmon and smp-tester on top of hci_vhci,
with ecdh done in both software and hardware (through atmel-ecc driver).
All tests passed.


Tudor Ambarus (2):
  Bluetooth: move ecdh allocation outside of ecdh_helper
  Bluetooth: let the crypto subsystem generate the ecc privkey

 net/bluetooth/ecdh_helper.c | 134 ++--
 net/bluetooth/ecdh_helper.h |   8 ++-
 net/bluetooth/selftest.c|  29 +++---
 net/bluetooth/smp.c | 120 +--
 4 files changed, 157 insertions(+), 134 deletions(-)

-- 
2.9.4

[PATCH] hwrng: pr_err() strings should end with newlines

[Arvind Yadav]

pr_err() messages should terminated with a new-line to avoid
other messages being concatenated onto the end.

Signed-off-by: Arvind Yadav 
---
 drivers/char/hw_random/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 9701ac7..ff79844 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -423,7 +423,7 @@ static void start_khwrngd(void)
 {
hwrng_fill = kthread_run(hwrng_fillfn, NULL, "hwrng");
if (IS_ERR(hwrng_fill)) {
-   pr_err("hwrng_fill thread creation failed");
+   pr_err("hwrng_fill thread creation failed\n");
hwrng_fill = NULL;
}
 }
-- 
1.9.1

[PATCH 2/2] crypto/nx: Do not initialize workmem allocation

[Haren Myneni]

[PATCH 2/2] crypto/nx: Do not initialize workmem allocation

We are using percpu send window on P9 NX (powerNV) instead of opening /
closing per each crypto session. Means txwin is removed from workmem.
So we do not need to initialize workmem for each request.

Signed-off-by: Haren Myneni 
---
 drivers/crypto/nx/nx-842.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/nx/nx-842.c b/drivers/crypto/nx/nx-842.c
index da3cb8c..d94e25d 100644
--- a/drivers/crypto/nx/nx-842.c
+++ b/drivers/crypto/nx/nx-842.c
@@ -116,7 +116,7 @@ int nx842_crypto_init(struct crypto_tfm *tfm, struct 
nx842_driver *driver)
 
spin_lock_init(>lock);
ctx->driver = driver;
-   ctx->wmem = kzalloc(driver->workmem_size, GFP_KERNEL);
+   ctx->wmem = kmalloc(driver->workmem_size, GFP_KERNEL);
ctx->sbounce = (u8 *)__get_free_pages(GFP_KERNEL, BOUNCE_BUFFER_ORDER);
ctx->dbounce = (u8 *)__get_free_pages(GFP_KERNEL, BOUNCE_BUFFER_ORDER);
if (!ctx->wmem || !ctx->sbounce || !ctx->dbounce) {
-- 
1.8.3.1

[PATCH 1/2] crypto/nx: Use percpu send window for NX requests

[Haren Myneni]

[PATCH 1/2] crypto/nx: Use percpu send window for NX requests

For P9 NX, the send window is opened for each crypto session and closed
upon free. But VAS supports 64K windows per chip for all coprocessors
including in user space support. So there is a possibility of not
getting the window for kernel requests.

This patch reserves windows for each coprocessor type (NX842) and are
available forever for kernel requests, Opens each window for each CPU
on the corresponding chip during driver initialization. So then use the
percpu txwin for NX requests depends on the CPU on which the process
is executing.

Signed-off-by: Haren Myneni 
---
 drivers/crypto/nx/nx-842-powernv.c | 149 +
 1 file changed, 68 insertions(+), 81 deletions(-)

diff --git a/drivers/crypto/nx/nx-842-powernv.c 
b/drivers/crypto/nx/nx-842-powernv.c
index eb221ed..3780ae1 100644
--- a/drivers/crypto/nx/nx-842-powernv.c
+++ b/drivers/crypto/nx/nx-842-powernv.c
@@ -46,7 +46,6 @@ struct nx842_workmem {
 
ktime_t start;
 
-   struct vas_window *txwin;   /* Used with VAS function */
char padding[WORKMEM_ALIGN]; /* unused, to allow alignment */
 } __packed __aligned(WORKMEM_ALIGN);
 
@@ -65,7 +64,7 @@ struct nx842_coproc {
  * Send the request to NX engine on the chip for the corresponding CPU
  * where the process is executing. Use with VAS function.
  */
-static DEFINE_PER_CPU(struct nx842_coproc *, coproc_inst);
+static DEFINE_PER_CPU(struct vas_window *, cpu_txwin);
 
 /* no cpu hotplug on powernv, so this list never changes after init */
 static LIST_HEAD(nx842_coprocs);
@@ -586,16 +585,11 @@ static int nx842_exec_vas(const unsigned char *in, 
unsigned int inlen,
ccw = SET_FIELD(CCW_FC_842, ccw, fc);
crb->ccw = cpu_to_be32(ccw);
 
-   txwin = wmem->txwin;
-   /* shoudn't happen, we don't load without a coproc */
-   if (!txwin) {
-   pr_err_ratelimited("NX-842 coprocessor is not available");
-   return -ENODEV;
-   }
-
do {
wmem->start = ktime_get();
preempt_disable();
+   txwin = this_cpu_read(cpu_txwin);
+
/*
 * VAS copy CRB into L2 cache. Refer .
 * @crb and @offset.
@@ -689,25 +683,6 @@ static inline void nx842_add_coprocs_list(struct 
nx842_coproc *coproc,
list_add(>list, _coprocs);
 }
 
-/*
- * Identify chip ID for each CPU and save coprocesor adddress for the
- * corresponding NX engine in percpu coproc_inst.
- * coproc_inst is used in crypto_init to open send window on the NX instance
- * for the corresponding CPU / chip where the open request is executed.
- */
-static void nx842_set_per_cpu_coproc(struct nx842_coproc *coproc)
-{
-   unsigned int i, chip_id;
-
-   for_each_possible_cpu(i) {
-   chip_id = cpu_to_chip_id(i);
-
-   if (coproc->chip_id == chip_id)
-   per_cpu(coproc_inst, i) = coproc;
-   }
-}
-
-
 static struct vas_window *nx842_alloc_txwin(struct nx842_coproc *coproc)
 {
struct vas_window *txwin = NULL;
@@ -725,15 +700,58 @@ static struct vas_window *nx842_alloc_txwin(struct 
nx842_coproc *coproc)
 * Open a VAS send window which is used to send request to NX.
 */
txwin = vas_tx_win_open(coproc->vas.id, coproc->ct, );
-   if (IS_ERR(txwin)) {
+   if (IS_ERR(txwin))
pr_err("ibm,nx-842: Can not open TX window: %ld\n",
PTR_ERR(txwin));
-   return NULL;
-   }
 
return txwin;
 }
 
+/*
+ * Identify chip ID for each CPU, open send wndow for the corresponding NX
+ * engine and save txwin in percpu cpu_txwin.
+ * cpu_txwin is used in copy/paste operation for each compression /
+ * decompression request.
+ */
+static int nx842_open_percpu_txwins(void)
+{
+   struct nx842_coproc *coproc, *n;
+   unsigned int i, chip_id;
+
+   for_each_possible_cpu(i) {
+   struct vas_window *txwin = NULL;
+
+   chip_id = cpu_to_chip_id(i);
+
+   list_for_each_entry_safe(coproc, n, _coprocs, list) {
+   /*
+* Kernel requests use only high priority FIFOs. So
+* open send windows for these FIFOs.
+*/
+
+   if (coproc->ct != VAS_COP_TYPE_842_HIPRI)
+   continue;
+
+   if (coproc->chip_id == chip_id) {
+   txwin = nx842_alloc_txwin(coproc);
+   if (IS_ERR(txwin))
+   return PTR_ERR(txwin);
+
+   per_cpu(cpu_txwin, i) = txwin;
+   break;
+   }
+   }
+
+   if (!per_cpu(cpu_txwin, i)) {
+   /* shoudn't happen, Each chip will have NX engine */
+ 

Re: [PATCH] mac80211: aead api to reduce redundancy

[Herbert Xu]

On Mon, Sep 25, 2017 at 07:22:26AM +0200, Johannes Berg wrote:
> 
> The code moves to crypto/ though, and I'm not even sure I can vouch for
> the Makefile choice there.

Thanks, I missed that.  I don't think this belongs in crypto.
This proposed helper is only useful for wireless so it should
stay there.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt