[PATCH 4/4] md: dm-crypt: Initialize the sector number for one request

[Baolin Wang]

If the crypto engine can support the bulk mode, that means the contiguous
requests from one block can be merged into one request to be handled by
crypto engine. If so, the crypto engine need the sector number of one request
to do merging action.

Signed-off-by: Baolin Wang 
---
 drivers/md/dm-crypt.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 3147c8d..9e2dbfd 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -866,6 +866,7 @@ static int crypt_convert_block(struct crypt_config *cc,
return r;
}
 
+   req->sector = ctx->cc_sector;
ablkcipher_request_set_crypt(req, >sg_in, >sg_out,
 1 << SECTOR_SHIFT, iv);
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] crypto: Introduce the bulk mode for crypto engine framework

[Baolin Wang]

Now some cipher hardware engines prefer to handle bulk block by merging
requests to increase the block size and thus increase the hardware engine
processing speed.

This patch introduces request bulk mode to help the crypto hardware drivers
improve in efficiency, and chooses the suitable mode (SECTOR_MODE) for
initializing aes engine.

Signed-off-by: Baolin Wang 
---
 crypto/Kconfig|1 +
 crypto/crypto_engine.c|  122 +++--
 drivers/crypto/omap-aes.c |2 +-
 include/crypto/algapi.h   |   23 -
 4 files changed, 143 insertions(+), 5 deletions(-)

diff --git a/crypto/Kconfig b/crypto/Kconfig
index c844227..6a2f9a6 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -229,6 +229,7 @@ config CRYPTO_GLUE_HELPER_X86
 
 config CRYPTO_ENGINE
tristate
+   select CRYPTO_ABLK_HELPER
 
 comment "Authenticated Encryption with Associated Data"
 
diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
index a55c82d..0de5829 100644
--- a/crypto/crypto_engine.c
+++ b/crypto/crypto_engine.c
@@ -14,6 +14,7 @@
 
 #include 
 #include 
+#include 
 #include "internal.h"
 
 #define CRYPTO_ENGINE_MAX_QLEN 10
@@ -84,6 +85,17 @@ static void crypto_pump_requests(struct crypto_engine 
*engine,
 
req = ablkcipher_request_cast(async_req);
 
+   /*
+* If the engine supports the bulk mode and the request is allocated the
+* sg table to expand scatterlists entries, then it need to point the
+* scatterlists from the sg table.
+*/
+   if (engine->mode == SECTOR_BULK_MODE && req->sgt_src.orig_nents &&
+   req->sgt_dst.orig_nents) {
+   req->src = req->sgt_src.sgl;
+   req->dst = req->sgt_dst.sgl;
+   }
+
engine->cur_req = req;
if (backlog)
backlog->complete(backlog, -EINPROGRESS);
@@ -137,9 +149,46 @@ static void crypto_pump_work(struct kthread_work *work)
 }
 
 /**
+ * crypto_merge_request_to_engine - try to merge one request into previous one
+ * @engine: the hardware engine
+ * @req: the request need to be merged
+ *
+ * If the crypto engine supports bulk mode, then try to merge the new request
+ * into the listed one from engine queue to handle them together.
+ *
+ * Return 0 on success and others are failed.
+ */
+static bool crypto_merge_request_to_engine(struct crypto_engine *engine,
+  struct ablkcipher_request *req)
+{
+   /*
+* The request is allocated from memory pool in dm-crypt, here need to
+* do initialization for sg table in case some random values.
+*/
+   req->sgt_src.orig_nents = 0;
+   req->sgt_dst.orig_nents = 0;
+
+   /*
+* If the hardware engine can not support the bulk mode encryption,
+* just return 1 means merging failed.
+*/
+   if (engine->mode != SECTOR_BULK_MODE)
+   return 1;
+
+   return ablk_try_merge(>queue, req);
+}
+
+/**
  * crypto_transfer_request - transfer the new request into the engine queue
  * @engine: the hardware engine
  * @req: the request need to be listed into the engine queue
+ *
+ * Firstly it will check if the new request can be merged into previous one
+ * if their secotr numbers are continuous, if not should list it into engine
+ * queue.
+ *
+ * If the new request can be merged into the previous request, then just
+ * finalize the new request.
  */
 int crypto_transfer_request(struct crypto_engine *engine,
struct ablkcipher_request *req, bool need_pump)
@@ -154,6 +203,26 @@ int crypto_transfer_request(struct crypto_engine *engine,
return -ESHUTDOWN;
}
 
+   /*
+* Here need to check if the request can be merged into previous
+* request. If the hardware engine can support encryption with
+* bulk block, we can merge the new request into previous request
+* if their secotr numbers are continuous, which can be handled
+* together by engine to improve the encryption efficiency.
+* Return -EINPROGRESS means it has been merged into previous request,
+* so just end up this request.
+*/
+   ret = crypto_merge_request_to_engine(engine, req);
+   if (!ret) {
+   spin_unlock_irqrestore(>queue_lock, flags);
+   crypto_finalize_request(engine, req, 0);
+   return -EINPROGRESS;
+   }
+
+   /*
+* If the request can not be merged into previous request, then list it
+* into the queue of engine, and will be handled by kthread worker.
+*/
ret = ablkcipher_enqueue_request(>queue, req);
 
if (!engine->busy && need_pump)
@@ -178,7 +247,8 @@ int crypto_transfer_request_to_engine(struct crypto_engine 
*engine,
 EXPORT_SYMBOL_GPL(crypto_transfer_request_to_engine);
 
 /**
- * crypto_finalize_request - finalize one request if the request is done
+ * 

[PATCH 2/4] crypto: Introduce some helper functions to help to merge requests

[Baolin Wang]

Usually the dm-crypt subsystem will send encryption/descryption requests to
the crypto layer one block at a time, making each request 512 bytes long,
which is a much smaller size for hardware engine, that means the hardware
engine can not play its best performance.

Now some cipher hardware engines prefer to handle bulk block rather than one
sector (512 bytes) created by dm-crypt, cause these cipher engines can handle
the intermediate values (IV) by themselves in one bulk block. This means we
can increase the size of the request by merging request rather than always 512
bytes and thus increase the hardware engine processing speed.

This patch introduces some helper functions to help to merge requests to improve
hardware engine efficiency.

Signed-off-by: Baolin Wang 
---
 crypto/ablk_helper.c |  135 ++
 include/crypto/ablk_helper.h |3 +
 include/linux/crypto.h   |5 ++
 3 files changed, 143 insertions(+)

diff --git a/crypto/ablk_helper.c b/crypto/ablk_helper.c
index e1fcf53..3cf15cb 100644
--- a/crypto/ablk_helper.c
+++ b/crypto/ablk_helper.c
@@ -26,6 +26,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -34,6 +35,140 @@
 #include 
 #include 
 
+/**
+ * ablk_link_request_if_contigous - try to link one request into previous one
+ * if the page address is contiguous.
+ * @list_req: the request from queue list
+ * @req: the new request need to be merged
+ *
+ * If the listed request and new request's pages of the scatterlists are
+ * contiguous, then merge the scatterlists of new request into the listed one.
+ *
+ * Return true on success, others means failed.
+ */
+static bool ablk_link_request_if_contigous(struct ablkcipher_request *list_req,
+  struct ablkcipher_request *req)
+{
+   struct scatterlist *last_src_sg =
+   sg_last(list_req->sgt_src.sgl, list_req->sgt_src.nents);
+   struct scatterlist *last_dst_sg =
+   sg_last(list_req->sgt_dst.sgl, list_req->sgt_dst.nents);
+   struct scatterlist *req_src_sg = req->src;
+   struct scatterlist *req_dst_sg = req->dst;
+
+   if (!last_src_sg || !last_dst_sg)
+   return false;
+
+   /* Check if the src/dst scatterlists are contiguous */
+   if (!sg_is_contiguous(last_src_sg, req_src_sg) ||
+   !sg_is_contiguous(last_dst_sg, req_dst_sg))
+   return false;
+
+   /*
+* If the request can be merged into the listed request after the
+* checking, then expand the listed request scatterlists' length.
+*/
+   last_src_sg->length += req_src_sg->length;
+   last_dst_sg->length += req_dst_sg->length;
+   list_req->nbytes += req->nbytes;
+
+   return true;
+}
+
+/**
+ * ablk_merge_request - try to merge one request into previous one
+ * @list_req: the request from queue list
+ * @req: the request need to be merged
+ *
+ * This function will create a dynamic scatterlist table for both source
+ * and destination if the request is the first coming in.
+ *
+ * Return true on success, others means failed.
+ */
+static bool ablk_merge_request(struct ablkcipher_request *list_req,
+  struct ablkcipher_request *req)
+{
+   struct sg_table *sgt_src = _req->sgt_src;
+   struct sg_table *sgt_dst = _req->sgt_dst;
+   unsigned int nents = SG_MAX_SINGLE_ALLOC;
+
+   if (sg_table_is_empty(sgt_src)) {
+   if (sg_alloc_empty_table(sgt_src, nents, GFP_ATOMIC))
+   return false;
+
+   if (sg_add_sg_to_table(sgt_src, list_req->src))
+   return false;
+   }
+
+   if (sg_table_is_empty(sgt_dst)) {
+   if (sg_alloc_empty_table(sgt_dst, nents, GFP_ATOMIC))
+   return false;
+
+   if (sg_add_sg_to_table(sgt_dst, list_req->dst))
+   return false;
+   }
+
+   /*
+* Check if the new request is contiguous for the listed request,
+* if it is contiguous then merge the new request into the listed one.
+*/
+   if (ablk_link_request_if_contigous(list_req, req))
+   return true;
+
+   if (sg_add_sg_to_table(sgt_src, req->src))
+   return false;
+
+   if (sg_add_sg_to_table(sgt_dst, req->dst))
+   return false;
+
+   list_req->nbytes += req->nbytes;
+   return true;
+}
+
+/**
+ * ablk_try_merge - try to merge one request into previous one
+ * @queue: the crypto queue list
+ * @req: the request need to be merged
+ *
+ * Note: The merging action should be under the spinlock or mutex protection.
+ *
+ * Return 0 on success and others are failed.
+ */
+int ablk_try_merge(struct crypto_queue *queue,
+  struct ablkcipher_request *req)
+{
+   struct ablkcipher_request *list_req;
+   struct crypto_async_request *async_req;
+
+   

[PATCH 1/4] scatterlist: Introduce some helper functions

[Baolin Wang]

In crypto engine framework, one request can combine other requests'
scatterlists into its sg table to improve engine efficency with
handling bulk block. Thus we need some helper functions to manage
dynamic scattertables.

This patch introduces 'sg_is_contiguous()' function to check if two
scatterlists are contiguous, 'sg_alloc_empty_table()' function to
allocate one empty sg table, 'sg_add_sg_to_table()' function to add
one scatterlist into sg table and 'sg_table_is_empty' function to
check if the sg table is empty.

Signed-off-by: Baolin Wang 
---
 include/linux/scatterlist.h |   33 
 lib/scatterlist.c   |   59 +++
 2 files changed, 92 insertions(+)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 556ec1e..ae9aee6 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -212,6 +212,37 @@ static inline void sg_unmark_end(struct scatterlist *sg)
 }
 
 /**
+ * sg_is_contiguous - Check if the scatterlists are contiguous
+ * @sga: SG entry
+ * @sgb: SG entry
+ *
+ * Description:
+ *   If the sga scatterlist is contiguous with the sgb scatterlist,
+ *   that means they can be merged together.
+ *
+ **/
+static inline bool sg_is_contiguous(struct scatterlist *sga,
+   struct scatterlist *sgb)
+{
+   return ((sga->page_link & ~0x3UL) + sga->offset + sga->length ==
+   (sgb->page_link & ~0x3UL));
+}
+
+/**
+ * sg_table_is_empty - Check if the sg table is empty
+ * @sgt: sg table
+ *
+ * Description:
+ *   The 'orig_nents' member of one sg table is used to indicate how many
+ *   scatterlists in the sg table.
+ *
+ **/
+static inline bool sg_table_is_empty(struct sg_table *sgt)
+{
+   return !sgt->orig_nents;
+}
+
+/**
  * sg_phys - Return physical address of an sg entry
  * @sg: SG entry
  *
@@ -261,6 +292,8 @@ void sg_free_table(struct sg_table *);
 int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int,
 struct scatterlist *, gfp_t, sg_alloc_fn *);
 int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);
+int sg_alloc_empty_table(struct sg_table *, unsigned int, gfp_t);
+int sg_add_sg_to_table(struct sg_table *, struct scatterlist *);
 int sg_alloc_table_from_pages(struct sg_table *sgt,
struct page **pages, unsigned int n_pages,
unsigned long offset, unsigned long size,
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 004fc70..e9e5d5a 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -370,6 +370,65 @@ int sg_alloc_table(struct sg_table *table, unsigned int 
nents, gfp_t gfp_mask)
 EXPORT_SYMBOL(sg_alloc_table);
 
 /**
+ * sg_add_sg_to_table - Add one scatterlist into sg table
+ * @sgt:   The sg table header to use
+ * @src:   The sg need to be added into sg table
+ *
+ * Description:
+ *   The 'nents' member indicates how many scatterlists added in the sg table.
+ *   Copy the @src@ scatterlist into sg table and increase 'nents' member.
+ *
+ **/
+int sg_add_sg_to_table(struct sg_table *sgt, struct scatterlist *src)
+{
+   unsigned int i = 0, orig_nents = sgt->orig_nents;
+   struct scatterlist *sgl = sgt->sgl;
+   struct scatterlist *sg;
+
+   /* Check if there are enough space for the new sg to be added */
+   if (sgt->nents >= sgt->orig_nents)
+   return -EINVAL;
+
+   for_each_sg(sgl, sg, orig_nents, i) {
+   if (sgt->nents > 0 && i == (sgt->nents - 1)) {
+   sg_unmark_end(sg);
+   } else if (i == sgt->nents) {
+   memcpy(sg, src, sizeof(struct scatterlist));
+   sg_mark_end(sg);
+   sgt->nents++;
+   break;
+   }
+   }
+
+   return 0;
+}
+
+/**
+ * sg_alloc_empty_table - Allocate one empty sg table
+ * @sgt:   The sg table header to use
+ * @nents: Number of entries in sg list
+ * @gfp_mask:  GFP allocation mask
+ *
+ *  Description:
+ *Allocate and initialize an sg table. The 'nents' member of sg_table
+ *indicates how many scatterlists added in the sg table. It should set
+ *0 which means there are no scatterlists added in this sg table now.
+ *
+ **/
+int sg_alloc_empty_table(struct sg_table *sgt, unsigned int nents,
+gfp_t gfp_mask)
+{
+   int ret;
+
+   ret = sg_alloc_table(sgt, nents, gfp_mask);
+   if (ret)
+   return ret;
+
+   sgt->nents = 0;
+   return 0;
+}
+
+/**
  * sg_alloc_table_from_pages - Allocate and initialize an sg table from
  *an array of pages
  * @sgt:   The sg table header to use
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/14] hw_random: exynos: use __maybe_unused to hide pm functions

[Krzysztof Kozlowski]

On 03.03.2016 00:58, Arnd Bergmann wrote:
> The exynos random driver uses #ifdef to check for CONFIG_PM, but
> then uses SIMPLE_DEV_PM_OPS, which leaves the references out when
> CONFIG_PM_SLEEP is not defined, so we get a warning with
> PM=y && PM_SLEEP=n:
> 
> drivers/char/hw_random/exynos-rng.c:166:12: error: 'exynos_rng_suspend' 
> defined but not used [-Werror=unused-function]
> drivers/char/hw_random/exynos-rng.c:171:12: error: 'exynos_rng_resume' 
> defined but not used [-Werror=unused-function]
> 
> This removes the incorrect #ifdef and instead uses a __maybe_unused
> annotation to let the compiler know it can silently drop
> the function definition.
> 
> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/char/hw_random/exynos-rng.c | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 

Reviewed-by: Krzysztof Kozlowski 

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 07/14] hw_random: exynos: use __maybe_unused to hide pm functions

[Arnd Bergmann]

The exynos random driver uses #ifdef to check for CONFIG_PM, but
then uses SIMPLE_DEV_PM_OPS, which leaves the references out when
CONFIG_PM_SLEEP is not defined, so we get a warning with
PM=y && PM_SLEEP=n:

drivers/char/hw_random/exynos-rng.c:166:12: error: 'exynos_rng_suspend' defined 
but not used [-Werror=unused-function]
drivers/char/hw_random/exynos-rng.c:171:12: error: 'exynos_rng_resume' defined 
but not used [-Werror=unused-function]

This removes the incorrect #ifdef and instead uses a __maybe_unused
annotation to let the compiler know it can silently drop
the function definition.

Signed-off-by: Arnd Bergmann 
---
 drivers/char/hw_random/exynos-rng.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/char/hw_random/exynos-rng.c 
b/drivers/char/hw_random/exynos-rng.c
index 30cf4623184f..ada081232528 100644
--- a/drivers/char/hw_random/exynos-rng.c
+++ b/drivers/char/hw_random/exynos-rng.c
@@ -144,8 +144,7 @@ static int exynos_rng_probe(struct platform_device *pdev)
return devm_hwrng_register(>dev, _rng->rng);
 }
 
-#ifdef CONFIG_PM
-static int exynos_rng_runtime_suspend(struct device *dev)
+static int __maybe_unused exynos_rng_runtime_suspend(struct device *dev)
 {
struct platform_device *pdev = to_platform_device(dev);
struct exynos_rng *exynos_rng = platform_get_drvdata(pdev);
@@ -155,7 +154,7 @@ static int exynos_rng_runtime_suspend(struct device *dev)
return 0;
 }
 
-static int exynos_rng_runtime_resume(struct device *dev)
+static int __maybe_unused exynos_rng_runtime_resume(struct device *dev)
 {
struct platform_device *pdev = to_platform_device(dev);
struct exynos_rng *exynos_rng = platform_get_drvdata(pdev);
@@ -163,12 +162,12 @@ static int exynos_rng_runtime_resume(struct device *dev)
return clk_prepare_enable(exynos_rng->clk);
 }
 
-static int exynos_rng_suspend(struct device *dev)
+static int __maybe_unused exynos_rng_suspend(struct device *dev)
 {
return pm_runtime_force_suspend(dev);
 }
 
-static int exynos_rng_resume(struct device *dev)
+static int __maybe_unused exynos_rng_resume(struct device *dev)
 {
struct platform_device *pdev = to_platform_device(dev);
struct exynos_rng *exynos_rng = platform_get_drvdata(pdev);
@@ -180,7 +179,6 @@ static int exynos_rng_resume(struct device *dev)
 
return exynos_rng_configure(exynos_rng);
 }
-#endif
 
 static const struct dev_pm_ops exynos_rng_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(exynos_rng_suspend, exynos_rng_resume)
-- 
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 00/14] drivers: use __maybe_unused to hide pm functions

[Arnd Bergmann]

I found many variations of the bug in these device drivers (and some
USB drivers I already send patches for in a separate series).

In each case, the power management operations structure conditionally
references suspend/resume functions, but the functions are hidden
in an incorrect #ifdef or not hidden at all.

We could try to correct the #ifdefs, but it seems easier to just
mark those functions as __maybe_unused, which has the same effect
but provides better compile-time test coverage and (subjectively)
looks a bit nicer.

I have a patch series that avoids all warnings in ARM randconfig
builds, and I have verified that all these patches fix a warning that
is still present in today's linux-next, and that they do not
introduce new warnings in any configuration I found.

Note that all these drivers are ARM specific, so I assume that
all portable drivers got fixed already when someone rand into
the problem on x86.

There are no dependencies between the patches, so I'd appreciate
subsystem maintainers to put them directly into their git trees.

Arnd

Arnd Bergmann (14):
  pinctrl: at91: use __maybe_unused to hide pm functions
  irqchip: st: use __maybe_unused to hide st_irq_syscfg_resume
  power: ipaq-micro-battery: use __maybe_unused to hide pm functions
  power: pm2301-charger: use __maybe_unused to hide pm functions
  mfd: ipaq-micro: use __maybe_unused to hide pm functions
  dma: sirf: use __maybe_unused to hide pm functions
  hw_random: exynos: use __maybe_unused to hide pm functions
  scsi: mvumi: use __maybe_unused to hide pm functions
  amd-xgbe: use __maybe_unused to hide pm functions
  wireless: cw1200: use __maybe_unused to hide pm functions_
  input: spear-keyboard: use __maybe_unused to hide pm functions
  keyboard: snvs-pwrkey: use __maybe_unused to hide pm functions
  [media] omap3isp: use IS_ENABLED() to hide pm functions
  ASoC: rockchip: use __maybe_unused to hide st_irq_syscfg_resume

 drivers/char/hw_random/exynos-rng.c | 10 --
 drivers/dma/sirf-dma.c  | 10 --
 drivers/input/keyboard/snvs_pwrkey.c|  4 ++--
 drivers/input/keyboard/spear-keyboard.c |  6 ++
 drivers/irqchip/irq-st.c|  2 +-
 drivers/media/platform/omap3isp/isp.c   | 13 +
 drivers/mfd/ipaq-micro.c|  2 +-
 drivers/net/ethernet/amd/xgbe/xgbe-main.c   |  6 ++
 drivers/net/wireless/st/cw1200/cw1200_spi.c |  9 ++---
 drivers/net/wireless/st/cw1200/pm.h |  9 +++--
 drivers/pinctrl/pinctrl-at91-pio4.c |  4 ++--
 drivers/power/ipaq_micro_battery.c  |  4 ++--
 drivers/power/pm2301_charger.c  | 22 ++
 drivers/scsi/mvumi.c|  4 ++--
 sound/soc/rockchip/rockchip_spdif.c |  4 ++--
 15 files changed, 40 insertions(+), 69 deletions(-)

-- 
2.7.0
Cc: herb...@gondor.apana.org.au
Cc: k.kozlow...@samsung.com
Cc: dan.j.willi...@intel.com
Cc: vinod.k...@intel.com
Cc: bao...@kernel.org
Cc: dmitry.torok...@gmail.com
Cc: t...@linutronix.de
Cc: ja...@lakedaemon.net
Cc: marc.zyng...@arm.com
Cc: laurent.pinch...@ideasonboard.com
Cc: mche...@osg.samsung.com
Cc: lee.jo...@linaro.org
Cc: kv...@codeaurora.org
Cc: ludovic.desroc...@atmel.com
Cc: linus.wall...@linaro.org
Cc: s...@kernel.org
Cc: dbarysh...@gmail.com
Cc: jbottom...@odin.com
Cc: martin.peter...@oracle.com
Cc: broo...@kernel.org
Cc: linux-crypto@vger.kernel.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-samsung-...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Cc: dmaeng...@vger.kernel.org
Cc: linux-in...@vger.kernel.org
Cc: linux-me...@vger.kernel.org
Cc: net...@vger.kernel.org
Cc: linux-wirel...@vger.kernel.org
Cc: linux-g...@vger.kernel.org
Cc: linux...@vger.kernel.org
Cc: linux-s...@vger.kernel.org
Cc: alsa-de...@alsa-project.org
Cc: linux-rockc...@lists.infradead.org
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: implement DH primitives under akcipher API

[Marcel Holtmann]

Hi Stephan,

>>> +static int dh_check_params_length(unsigned int p_len)
>>> +{
>>> +   switch (p_len) {
>>> +   case 768:
>>> +   case 1024:
>>> +   case 1536:
>>> +   case 2048:
>>> +   case 3072:
>>> +   case 4096:
>>> +   return 0;
>>> +   }
>>> +   return -EINVAL;
>>> +}
>> 
>> As far back as 1999, the FreeS/WAN project refused to
>> implement the 768-bit IPsec group 1 (even though it was
>> the only one required by the RFCs) because it was not thought
>> secure enough. I think the most-used group was 1536-bit
>> group 5; it wasn't in the original RFCs but nearly everyone
>> implemented it.
>> 
 And besides, I would like to disallow all < 2048 right from the start.
>> 
>> I'm not up-to-date on the performance of attacks. You may be right,
>> or perhaps the minimum should be even higher. Certainly there is
>> no reason to support 768 or 1024-bit groups.
>> 
>> On the other hand, we should consider keeping the 1536-bit
>> group since it is very widely used, likely including by people
>> we'll want to interoperate with.
> 
> Considering the recent attacks which kind of broke DH <= 768, all smaller 
> than 
> 1024 is not to be used any more. Even 1024 bit is not too far off from 768 so 
> I would consider not using 1024 to keep a safety margin.
> 
> It is widely suggested to use 2048 and higher. But for compatibility, I agree 
> with 1536.
> 
> However, that leaves us with the open question for the upper bound. I have no 
> idea which sizes hardware may implement. But considering that OpenSSH uses DH 
> up to and including 8192 these days (see /etc/ssh/moduli), I would suggest to 
> allow 8192 too.
> 
> In addition, we should all be prepared to increase the limit with a 
> reasonable 
> effort. Thus, if we have implementations covering hardware support, they 
> should cope with reasonable efforts.

and why is this not an userspace policy decision on what minimum it allows? Why 
are we discussing this in the context of the kernel in the first place? The 
kernel should just expose the support for it.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: implement DH primitives under akcipher API

[Stephan Mueller]

Am Mittwoch, 2. März 2016, 08:03:44 schrieb Sandy Harris:

Hi Sandy,

> Salvatore Benedetto  wrote:
> >> > > > +static int dh_check_params_length(unsigned int p_len)
> >> > > > +{
> >> > > > +   switch (p_len) {
> >> > > > +   case 768:
> >> > > > +   case 1024:
> >> > > > +   case 1536:
> >> > > > +   case 2048:
> >> > > > +   case 3072:
> >> > > > +   case 4096:
> >> > > > +   return 0;
> >> > > > +   }
> >> > > > +   return -EINVAL;
> >> > > > +}
> 
> As far back as 1999, the FreeS/WAN project refused to
> implement the 768-bit IPsec group 1 (even though it was
> the only one required by the RFCs) because it was not thought
> secure enough. I think the most-used group was 1536-bit
> group 5; it wasn't in the original RFCs but nearly everyone
> implemented it.
> 
> >> And besides, I would like to disallow all < 2048 right from the start.
> 
> I'm not up-to-date on the performance of attacks. You may be right,
> or perhaps the minimum should be even higher. Certainly there is
> no reason to support 768 or 1024-bit groups.
> 
> On the other hand, we should consider keeping the 1536-bit
> group since it is very widely used, likely including by people
> we'll want to interoperate with.

Considering the recent attacks which kind of broke DH <= 768, all smaller than 
1024 is not to be used any more. Even 1024 bit is not too far off from 768 so 
I would consider not using 1024 to keep a safety margin.

It is widely suggested to use 2048 and higher. But for compatibility, I agree 
with 1536.

However, that leaves us with the open question for the upper bound. I have no 
idea which sizes hardware may implement. But considering that OpenSSH uses DH 
up to and including 8192 these days (see /etc/ssh/moduli), I would suggest to 
allow 8192 too.

In addition, we should all be prepared to increase the limit with a reasonable 
effort. Thus, if we have implementations covering hardware support, they 
should cope with reasonable efforts.

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: implement DH primitives under akcipher API

[Marcel Holtmann]

Hi Salvatore,

>>> Implement Diffie-Hellman primitives required by the scheme under the
>>> akcipher API. Here is how it works.
>>> 1) Call set_pub_key() by passing DH parameters (p,g) in PKCS3 format
>>> 2) Call set_priv_key() to set your own private key (xa) in raw format
>> 
>> this combination seems odd since it is normally the remote public key and 
>> the local private key. Generally the public key and private key are both 
>> remote ones.
> 
> I'm not sure I understand what you mean here. Usually the public key is
> remote and the private key is local. How can the private key be remote?

I accidentally mistyped. I meant of course local.

>> 
>> For using PKCS3 format is this standardized somewhere? I don't think it is a 
>> good idea to invent new ones here.
> 
> PKCS3 is the format used by openssl for genating DH params, that's why I
> used it.

Is that OpenSSL specific or backed up by a RFC?

>> 
>> In addition, how would this work for ECDH?
> 
> Don't know. There is not even ECC support right now.

If you call something dh-generic, then we need to think about how it would work 
for all the other ciphers that it might be used with. Making this RSA specific 
is not a good idea.

> 
>> 
>>> 3) Call decrypt() without passing any data as input to get back the
>>>  public part which will be computed as g^xa mod p
>>> 4) Call encrypt() by passing the counter part public key (yb) in raw format
>>>  as input to get back the shared secret calculated as zz = yb^xa mod p
>>> 
>>> A test is included in the patch. Test vector has been generated with
>>> openssl
>>> 
>>> Signed-off-by: Salvatore Benedetto 
>>> ---
>>> crypto/Kconfig|   8 ++
>>> crypto/Makefile   |   7 ++
>>> crypto/dh.c   | 264 
>>> ++
>>> crypto/pkcs3.asn1 |   5 ++
>>> crypto/tcrypt.c   |   4 +
>>> crypto/testmgr.c  | 140 +++--
>>> crypto/testmgr.h  | 208 +-
>>> 7 files changed, 627 insertions(+), 9 deletions(-)
>>> create mode 100644 crypto/dh.c
>>> create mode 100644 crypto/pkcs3.asn1
>>> 
>>> diff --git a/crypto/Kconfig b/crypto/Kconfig
>>> index f6bfdda..fd5b78d 100644
>>> --- a/crypto/Kconfig
>>> +++ b/crypto/Kconfig
>>> @@ -101,6 +101,14 @@ config CRYPTO_RSA
>>> help
>>>   Generic implementation of the RSA public key algorithm.
>>> 
>>> +config CRYPTO_DH
>>> +   tristate "Diffie-Hellman algorithm"
>>> +   select CRYPTO_AKCIPHER
>>> +   select MPILIB
>>> +   select ASN1
>> 
>> I really wonder that depending on ASN1 is a good idea here. As mentioned 
>> above ECDH would make sense to actually have supported from the beginning. 
>> The Bluetooth subsystem could be then converted to utilize in kernel ECC key 
>> generation and ECDH shared secret computation. It would be good to show this 
>> is truly generic DH.
>> 
> 
> This is an RFC. I understand it is not the best approach, but
> the idea behind was to try to reuse the akcipher for DH.

And I have the feeling that akcipher is not the best approach for adding a key 
exchange method. I think we need a new method for doing exactly that. At the 
base of it, the key exchange is fundamentally different.

>From an API point of view, I am also not convinced that it is a good idea to 
>generate the private keys used on the fly. I think this all needs to be a lot 
>more deterministic and flexible. In addition there are cases where you want to 
>point to specific private / public key pair that you locally have. There are 
>even protocols like Bluetooth that have defined fixed debug key pairs. If we 
>can not support that, then this approach is not generic enough.

So my thinking actually is that we need a new key exchange abstraction in the 
crypto stack. However I am not sure that an userspace facing API should be done 
via AF_ALG. I think that does not fit. I think that doing it via keyctl is a 
lot more logical place.

It also means that we need a separate keyctl to actually generate the local 
private / public key pairs first. I think that makes sense no matter what. You 
can generate the keys, the private key stays in kernel memory forever and you 
can read out the public key. Some protocols will throw away the keys after 
single use, but others might actually reuse them. Or as mentioned above has 
fixed keys for debugging purposes. Using keyctl should then also make it easy 
to handle RSA vs ECC for the key generation since we need to be able to store 
both types at some point anyway. Also in cases where keys are RSA keys in ASN.1 
format in the first place or are learned from certificates are already present 
and uniquely presented in the kernel. No need to invent yet another format for 
keys.

Especially in the case where you create a session key based out of certificates 
and existing public / private key pairs, it makes sense that keyctl can turn 
them directly into a new key. In most cases these are symmetric keys that can 

Re: [PATCH] crypto: implement DH primitives under akcipher API

[Sandy Harris]

Salvatore Benedetto  wrote:

>> > > > +static int dh_check_params_length(unsigned int p_len)
>> > > > +{
>> > > > +   switch (p_len) {
>> > > > +   case 768:
>> > > > +   case 1024:
>> > > > +   case 1536:
>> > > > +   case 2048:
>> > > > +   case 3072:
>> > > > +   case 4096:
>> > > > +   return 0;
>> > > > +   }
>> > > > +   return -EINVAL;
>> > > > +}
As far back as 1999, the FreeS/WAN project refused to
implement the 768-bit IPsec group 1 (even though it was
the only one required by the RFCs) because it was not thought
secure enough. I think the most-used group was 1536-bit
group 5; it wasn't in the original RFCs but nearly everyone
implemented it.

>> And besides, I would like to disallow all < 2048 right from the start.

I'm not up-to-date on the performance of attacks. You may be right,
or perhaps the minimum should be even higher. Certainly there is
no reason to support 768 or 1024-bit groups.

On the other hand, we should consider keeping the 1536-bit
group since it is very widely used, likely including by people
we'll want to interoperate with.

> Hmm.. What range would you suggest?

There are at least two RFCs which define additional groups.
Why not just add some or all of those?
https://tools.ietf.org/html/rfc3526
https://tools.ietf.org/html/rfc5114
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: implement DH primitives under akcipher API

[Salvatore Benedetto]

On Tue, Mar 01, 2016 at 12:25:33PM -0800, Marcel Holtmann wrote:

Hi Marcel,

> Hi Salvatore,
> 
> > Implement Diffie-Hellman primitives required by the scheme under the
> > akcipher API. Here is how it works.
> > 1) Call set_pub_key() by passing DH parameters (p,g) in PKCS3 format
> > 2) Call set_priv_key() to set your own private key (xa) in raw format
> 
> this combination seems odd since it is normally the remote public key and the 
> local private key. Generally the public key and private key are both remote 
> ones.

I'm not sure I understand what you mean here. Usually the public key is
remote and the private key is local. How can the private key be remote?

> 
> For using PKCS3 format is this standardized somewhere? I don't think it is a 
> good idea to invent new ones here.

PKCS3 is the format used by openssl for genating DH params, that's why I
used it.

> 
> In addition, how would this work for ECDH?

Don't know. There is not even ECC support right now.

> 
> > 3) Call decrypt() without passing any data as input to get back the
> >   public part which will be computed as g^xa mod p
> > 4) Call encrypt() by passing the counter part public key (yb) in raw format
> >   as input to get back the shared secret calculated as zz = yb^xa mod p
> > 
> > A test is included in the patch. Test vector has been generated with
> > openssl
> > 
> > Signed-off-by: Salvatore Benedetto 
> > ---
> > crypto/Kconfig|   8 ++
> > crypto/Makefile   |   7 ++
> > crypto/dh.c   | 264 
> > ++
> > crypto/pkcs3.asn1 |   5 ++
> > crypto/tcrypt.c   |   4 +
> > crypto/testmgr.c  | 140 +++--
> > crypto/testmgr.h  | 208 +-
> > 7 files changed, 627 insertions(+), 9 deletions(-)
> > create mode 100644 crypto/dh.c
> > create mode 100644 crypto/pkcs3.asn1
> > 
> > diff --git a/crypto/Kconfig b/crypto/Kconfig
> > index f6bfdda..fd5b78d 100644
> > --- a/crypto/Kconfig
> > +++ b/crypto/Kconfig
> > @@ -101,6 +101,14 @@ config CRYPTO_RSA
> > help
> >   Generic implementation of the RSA public key algorithm.
> > 
> > +config CRYPTO_DH
> > +   tristate "Diffie-Hellman algorithm"
> > +   select CRYPTO_AKCIPHER
> > +   select MPILIB
> > +   select ASN1
> 
> I really wonder that depending on ASN1 is a good idea here. As mentioned 
> above ECDH would make sense to actually have supported from the beginning. 
> The Bluetooth subsystem could be then converted to utilize in kernel ECC key 
> generation and ECDH shared secret computation. It would be good to show this 
> is truly generic DH.
> 

This is an RFC. I understand it is not the best approach, but
the idea behind was to try to reuse the akcipher for DH.

Thanks for your comments.

Regards,
Salvatore

> Regards
> 
> Marcel
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html