Re: [PATCH] crypto: qat - move temp buffers off the stack

[Kees Cook]

On Wed, Sep 26, 2018 at 2:51 AM, Ard Biesheuvel
 wrote:
> Arnd reports that with Kees's latest VLA patches applied, the HMAC
> handling in the QAT driver uses a worst case estimate of 160 bytes
> for the SHA blocksize, allowing the compiler to determine the size
> of the stack frame at runtime and throw a warning:
>
>   drivers/crypto/qat/qat_common/qat_algs.c: In function 
> 'qat_alg_do_precomputes':
>   drivers/crypto/qat/qat_common/qat_algs.c:257:1: error: the frame size
>   of 1112 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>
> Given that this worst case estimate is only 32 bytes larger than the
> actual block size of SHA-512, the use of a VLA here was hiding the
> excessive size of the stack frame from the compiler, and so we should
> try to move these buffers off the stack.
>
> So move the ipad/opad buffers and the various SHA state descriptors
> into the tfm context struct. Since qat_alg_do_precomputes() is only
> called in the context of a setkey() operation, this should be safe.
> Using SHA512_BLOCK_SIZE for the size of the ipad/opad buffers allows
> them to be used by SHA-1/SHA-256 as well.
>
> Reported-by: Arnd Bergmann 
> Signed-off-by: Ard Biesheuvel 
> ---
> This applies against v4.19-rc while Arnd's report was about -next. However,
> since Kees's VLA change results in a warning about a pre-existing condition,
> we may decide to apply it as a fix, and handle the conflict with Kees's
> patch in cryptodev. Otherwise, I can respin it to apply onto cryptodev
> directly. The patch was build tested only - I don't have the hardware.

I think the depth warning is minor (90 bytes over), so I don't think
it's high priority to backport the fix. I'm fine either way, of
course.

Reviewed-by: Kees Cook 

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v6 10/18] x86/power/64: Remove VLA usage

[Kees Cook]

On Wed, Jul 25, 2018 at 4:32 AM, Rafael J. Wysocki  wrote:
> On Tue, Jul 24, 2018 at 6:49 PM, Kees Cook  wrote:
>> In the quest to remove all stack VLA usage from the kernel[1], this
>> removes the discouraged use of AHASH_REQUEST_ON_STACK by switching to
>> shash directly and allocating the descriptor in heap memory (which should
>> be fine: the tfm has already been allocated there too).
>>
>> [1] 
>> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
>>
>> Signed-off-by: Kees Cook 
>> Acked-by: Pavel Machek 
>
> I think I can queue this up if there are no objections from others.
>
> Do you want me to do that?

Sure thing. It looks like the other stand-alone patches like this one
are getting taken into the non-crypto trees, so that's fine.

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2] async_pq: Remove VLA usage

[Kees Cook]

On Sat, May 5, 2018 at 12:58 AM, Kyle Spiers <ksspi...@google.com> wrote:
> In the quest to remove VLAs from the kernel[1], this moves the
> allocation of coefs and blocks from the stack to being kmalloc()ed.
>
> [1] https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Kyle Spiers <ksspi...@google.com>

Reviewed-by: Kees Cook <keesc...@chromium.org>

Is this something that should go via Vinod, Dan, or direct through
Herbert's crypto tree?

Thanks!

-Kees

> ---
> Forgot to add slab.h
> ---
>  crypto/async_tx/async_pq.c  | 18 ++
>  crypto/async_tx/raid6test.c |  9 -
>  2 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/crypto/async_tx/async_pq.c b/crypto/async_tx/async_pq.c
> index 56bd612927ab..af1912313a23 100644
> --- a/crypto/async_tx/async_pq.c
> +++ b/crypto/async_tx/async_pq.c
> @@ -194,9 +194,9 @@ async_gen_syndrome(struct page **blocks, unsigned int 
> offset, int disks,
> (src_cnt <= dma_maxpq(device, 0) ||
>  dma_maxpq(device, DMA_PREP_CONTINUE) > 0) &&
> is_dma_pq_aligned(device, offset, 0, len)) {
> -   struct dma_async_tx_descriptor *tx;
> +   struct dma_async_tx_descriptor *tx = NULL;
> enum dma_ctrl_flags dma_flags = 0;
> -   unsigned char coefs[src_cnt];
> +   unsigned char *coefs;
> int i, j;
>
> /* run the p+q asynchronously */
> @@ -207,6 +207,9 @@ async_gen_syndrome(struct page **blocks, unsigned int 
> offset, int disks,
>  * sources and update the coefficients accordingly
>  */
> unmap->len = len;
> +   coefs = kmalloc_array(src_cnt, sizeof(*coefs), GFP_KERNEL);
> +   if (!coefs)
> +   goto out;
> for (i = 0, j = 0; i < src_cnt; i++) {
> if (blocks[i] == NULL)
> continue;
> @@ -240,7 +243,9 @@ async_gen_syndrome(struct page **blocks, unsigned int 
> offset, int disks,
> }
>
> tx = do_async_gen_syndrome(chan, coefs, j, unmap, dma_flags, 
> submit);
> +out:
> dmaengine_unmap_put(unmap);
> +   kfree(coefs);
> return tx;
> }
>
> @@ -298,8 +303,8 @@ async_syndrome_val(struct page **blocks, unsigned int 
> offset, int disks,
>  {
> struct dma_chan *chan = pq_val_chan(submit, blocks, disks, len);
> struct dma_device *device = chan ? chan->device : NULL;
> -   struct dma_async_tx_descriptor *tx;
> -   unsigned char coefs[disks-2];
> +   struct dma_async_tx_descriptor *tx = NULL;
> +   unsigned char *coefs = NULL;
> enum dma_ctrl_flags dma_flags = submit->cb_fn ? DMA_PREP_INTERRUPT : 
> 0;
> struct dmaengine_unmap_data *unmap = NULL;
>
> @@ -318,6 +323,9 @@ async_syndrome_val(struct page **blocks, unsigned int 
> offset, int disks,
>  __func__, disks, len);
>
> unmap->len = len;
> +   coefs = kmalloc_array(disks - 2, sizeof(*coefs), GFP_KERNEL);
> +   if (!coefs)
> +   goto out;
> for (i = 0; i < disks-2; i++)
> if (likely(blocks[i])) {
> unmap->addr[j] = dma_map_page(dev, blocks[i],
> @@ -423,6 +431,8 @@ async_syndrome_val(struct page **blocks, unsigned int 
> offset, int disks,
> async_tx_sync_epilog(submit);
> tx = NULL;
> }
> +out:
> +   kfree(coefs);
> dmaengine_unmap_put(unmap);
>
> return tx;
> diff --git a/crypto/async_tx/raid6test.c b/crypto/async_tx/raid6test.c
> index dad95f45b88f..4237a5ae8f42 100644
> --- a/crypto/async_tx/raid6test.c
> +++ b/crypto/async_tx/raid6test.c
> @@ -24,6 +24,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #undef pr
>  #define pr(fmt, args...) pr_info("raid6test: " fmt, ##args)
> @@ -81,11 +82,16 @@ static void raid6_dual_recov(int disks, size_t bytes, int 
> faila, int failb, stru
> init_async_submit(, 0, NULL, NULL, NULL, 
> addr_conv);
> tx = async_gen_syndrome(ptrs, 0, disks, bytes, 
> );
> } else {
> -   struct page *blocks[disks];
> +   struct page **blocks;
> struct page *dest;
> int count = 0;
> int i;
>
> +   blocks = kmalloc_array(disks, sizeof(*blocks),
> + 

Re: [PATCH] async_pq: Remove VLA usage

[Kees Cook]

On Thu, May 3, 2018 at 3:57 PM, Kyle Spiers <ksspi...@google.com> wrote:
> In the quest to remove VLAs from the kernel[1], this moves the
> allocation of coefs and blocks from the stack to being kmalloc()ed.
>
> [1] https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Kyle Spiers <ksspi...@google.com>

Reviewed-by: Kees Cook <keesc...@chromium.org>

Thanks for working on this one!

-Kees

> ---
>  crypto/async_tx/async_pq.c  | 18 ++
>  crypto/async_tx/raid6test.c |  8 +++-
>  2 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/crypto/async_tx/async_pq.c b/crypto/async_tx/async_pq.c
> index 56bd612927ab..af1912313a23 100644
> --- a/crypto/async_tx/async_pq.c
> +++ b/crypto/async_tx/async_pq.c
> @@ -194,9 +194,9 @@ async_gen_syndrome(struct page **blocks, unsigned int 
> offset, int disks,
> (src_cnt <= dma_maxpq(device, 0) ||
>  dma_maxpq(device, DMA_PREP_CONTINUE) > 0) &&
> is_dma_pq_aligned(device, offset, 0, len)) {
> -   struct dma_async_tx_descriptor *tx;
> +   struct dma_async_tx_descriptor *tx = NULL;
> enum dma_ctrl_flags dma_flags = 0;
> -   unsigned char coefs[src_cnt];
> +   unsigned char *coefs;
> int i, j;
>
> /* run the p+q asynchronously */
> @@ -207,6 +207,9 @@ async_gen_syndrome(struct page **blocks, unsigned int 
> offset, int disks,
>  * sources and update the coefficients accordingly
>  */
> unmap->len = len;
> +   coefs = kmalloc_array(src_cnt, sizeof(*coefs), GFP_KERNEL);
> +   if (!coefs)
> +   goto out;
> for (i = 0, j = 0; i < src_cnt; i++) {
> if (blocks[i] == NULL)
> continue;
> @@ -240,7 +243,9 @@ async_gen_syndrome(struct page **blocks, unsigned int 
> offset, int disks,
> }
>
> tx = do_async_gen_syndrome(chan, coefs, j, unmap, dma_flags, 
> submit);
> +out:
> dmaengine_unmap_put(unmap);
> +   kfree(coefs);
> return tx;
> }
>
> @@ -298,8 +303,8 @@ async_syndrome_val(struct page **blocks, unsigned int 
> offset, int disks,
>  {
> struct dma_chan *chan = pq_val_chan(submit, blocks, disks, len);
> struct dma_device *device = chan ? chan->device : NULL;
> -   struct dma_async_tx_descriptor *tx;
> -   unsigned char coefs[disks-2];
> +   struct dma_async_tx_descriptor *tx = NULL;
> +   unsigned char *coefs = NULL;
> enum dma_ctrl_flags dma_flags = submit->cb_fn ? DMA_PREP_INTERRUPT : 
> 0;
> struct dmaengine_unmap_data *unmap = NULL;
>
> @@ -318,6 +323,9 @@ async_syndrome_val(struct page **blocks, unsigned int 
> offset, int disks,
>  __func__, disks, len);
>
> unmap->len = len;
> +   coefs = kmalloc_array(disks - 2, sizeof(*coefs), GFP_KERNEL);
> +   if (!coefs)
> +   goto out;
> for (i = 0; i < disks-2; i++)
> if (likely(blocks[i])) {
> unmap->addr[j] = dma_map_page(dev, blocks[i],
> @@ -423,6 +431,8 @@ async_syndrome_val(struct page **blocks, unsigned int 
> offset, int disks,
> async_tx_sync_epilog(submit);
> tx = NULL;
> }
> +out:
> +   kfree(coefs);
> dmaengine_unmap_put(unmap);
>
> return tx;
> diff --git a/crypto/async_tx/raid6test.c b/crypto/async_tx/raid6test.c
> index dad95f45b88f..ea036b531ef2 100644
> --- a/crypto/async_tx/raid6test.c
> +++ b/crypto/async_tx/raid6test.c
> @@ -81,11 +81,16 @@ static void raid6_dual_recov(int disks, size_t bytes, int 
> faila, int failb, stru
> init_async_submit(, 0, NULL, NULL, NULL, 
> addr_conv);
> tx = async_gen_syndrome(ptrs, 0, disks, bytes, 
> );
> } else {
> -   struct page *blocks[disks];
> +   struct page **blocks;
> struct page *dest;
> int count = 0;
> int i;
>
> +   blocks = kmalloc_array(disks, sizeof(*blocks),
> +   GFP_KERNEL);
> +   if (!blocks)
> +   return;
> +
> /* data+Q failure.  Reconstruct data from P,
>  * then rebuild syn

[PATCH] crypto: tcrypt: Remove VLA usage

[Kees Cook]

In the quest to remove all stack VLA usage from the kernel[1], this
allocates the return code buffers before starting jiffie timers, rather
than using stack space for the array. Additionally cleans up some exit
paths and make sure that the num_mb module_param() is used only once
per execution to avoid possible races in the value changing.

[1] 
https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com

Signed-off-by: Kees Cook <keesc...@chromium.org>
---
 crypto/tcrypt.c | 118 +---
 1 file changed, 79 insertions(+), 39 deletions(-)

diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 51fe7c8744ae..e721faab6fc8 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -158,9 +158,9 @@ struct test_mb_aead_data {
 };
 
 static int do_mult_aead_op(struct test_mb_aead_data *data, int enc,
-   u32 num_mb)
+   u32 num_mb, int *rc)
 {
-   int i, rc[num_mb], err = 0;
+   int i, err = 0;
 
/* Fire up a bunch of concurrent requests */
for (i = 0; i < num_mb; i++) {
@@ -188,18 +188,26 @@ static int test_mb_aead_jiffies(struct test_mb_aead_data 
*data, int enc,
 {
unsigned long start, end;
int bcount;
-   int ret;
+   int ret = 0;
+   int *rc;
+
+   rc = kcalloc(num_mb, sizeof(*rc), GFP_KERNEL);
+   if (!rc)
+   return -ENOMEM;
 
for (start = jiffies, end = start + secs * HZ, bcount = 0;
 time_before(jiffies, end); bcount++) {
-   ret = do_mult_aead_op(data, enc, num_mb);
+   ret = do_mult_aead_op(data, enc, num_mb, rc);
if (ret)
-   return ret;
+   goto out;
}
 
pr_cont("%d operations in %d seconds (%ld bytes)\n",
bcount * num_mb, secs, (long)bcount * blen * num_mb);
-   return 0;
+
+out:
+   kfree(rc);
+   return ret;
 }
 
 static int test_mb_aead_cycles(struct test_mb_aead_data *data, int enc,
@@ -208,10 +216,15 @@ static int test_mb_aead_cycles(struct test_mb_aead_data 
*data, int enc,
unsigned long cycles = 0;
int ret = 0;
int i;
+   int *rc;
+
+   rc = kcalloc(num_mb, sizeof(*rc), GFP_KERNEL);
+   if (!rc)
+   return -ENOMEM;
 
/* Warm-up run. */
for (i = 0; i < 4; i++) {
-   ret = do_mult_aead_op(data, enc, num_mb);
+   ret = do_mult_aead_op(data, enc, num_mb, rc);
if (ret)
goto out;
}
@@ -221,7 +234,7 @@ static int test_mb_aead_cycles(struct test_mb_aead_data 
*data, int enc,
cycles_t start, end;
 
start = get_cycles();
-   ret = do_mult_aead_op(data, enc, num_mb);
+   ret = do_mult_aead_op(data, enc, num_mb, rc);
end = get_cycles();
 
if (ret)
@@ -230,11 +243,11 @@ static int test_mb_aead_cycles(struct test_mb_aead_data 
*data, int enc,
cycles += end - start;
}
 
-out:
-   if (ret == 0)
-   pr_cont("1 operation in %lu cycles (%d bytes)\n",
-   (cycles + 4) / (8 * num_mb), blen);
+   pr_cont("1 operation in %lu cycles (%d bytes)\n",
+   (cycles + 4) / (8 * num_mb), blen);
 
+out:
+   kfree(rc);
return ret;
 }
 
@@ -705,9 +718,10 @@ struct test_mb_ahash_data {
char *xbuf[XBUFSIZE];
 };
 
-static inline int do_mult_ahash_op(struct test_mb_ahash_data *data, u32 num_mb)
+static inline int do_mult_ahash_op(struct test_mb_ahash_data *data, u32 num_mb,
+  int *rc)
 {
-   int i, rc[num_mb], err = 0;
+   int i, err = 0;
 
/* Fire up a bunch of concurrent requests */
for (i = 0; i < num_mb; i++)
@@ -731,18 +745,26 @@ static int test_mb_ahash_jiffies(struct 
test_mb_ahash_data *data, int blen,
 {
unsigned long start, end;
int bcount;
-   int ret;
+   int ret = 0;
+   int *rc;
+
+   rc = kcalloc(num_mb, sizeof(*rc), GFP_KERNEL);
+   if (!rc)
+   return -ENOMEM;
 
for (start = jiffies, end = start + secs * HZ, bcount = 0;
 time_before(jiffies, end); bcount++) {
-   ret = do_mult_ahash_op(data, num_mb);
+   ret = do_mult_ahash_op(data, num_mb, rc);
if (ret)
-   return ret;
+   goto out;
}
 
pr_cont("%d operations in %d seconds (%ld bytes)\n",
bcount * num_mb, secs, (long)bcount * blen * num_mb);
-   return 0;
+
+out:
+   kfree(rc);
+   return ret;
 }
 
 static int test_mb_ahash_cycles(struct test_mb_ahash_data *data, int blen,
@@ -751,10 +773,15 @@ static int test_mb_ahash_cycles(struct test_mb_ahash_data 
*data, int blen,
unsigned long cycles =

Re: [PATCH v2] crypto/ecc: Actually remove stack VLA usage

[Kees Cook]

On Fri, Mar 30, 2018 at 9:55 AM, Kees Cook <keesc...@chromium.org> wrote:
> On the quest to remove all VLAs from the kernel[1], this avoids VLAs
> by just using the maximum allocation size (4 bytes) for stack arrays.
> All the VLAs in ecc were either 3 or 4 bytes (or a multiple), so just
> make it 4 bytes all the time. Initialization routines are adjusted to
> check that ndigits does not end up larger than the arrays.
>
> This includes a removal of the earlier attempt at this fix from
> commit a963834b4742 ("crypto/ecc: Remove stack VLA usage")
>
> [1] https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Kees Cook <keesc...@chromium.org>

Friendly ping. Please apply to fix a963834b4742 ("crypto/ecc: Remove
stack VLA usage").

Thanks!

-Kees

> ---
> v2:
> - Squash revert (herbert)
> ---
>  crypto/ecc.c  | 66 
> +--
>  crypto/ecc.h  |  4 +++-
>  crypto/ecdh.c |  4 ++--
>  3 files changed, 33 insertions(+), 41 deletions(-)
>
> diff --git a/crypto/ecc.c b/crypto/ecc.c
> index 9c066b5ac12d..815541309a95 100644
> --- a/crypto/ecc.c
> +++ b/crypto/ecc.c
> @@ -515,7 +515,7 @@ static void vli_mmod_fast_256(u64 *result, const u64 
> *product,
>  static bool vli_mmod_fast(u64 *result, u64 *product,
>   const u64 *curve_prime, unsigned int ndigits)
>  {
> -   u64 tmp[2 * ndigits];
> +   u64 tmp[2 * ECC_MAX_DIGITS];
>
> switch (ndigits) {
> case 3:
> @@ -536,7 +536,7 @@ static bool vli_mmod_fast(u64 *result, u64 *product,
>  static void vli_mod_mult_fast(u64 *result, const u64 *left, const u64 *right,
>   const u64 *curve_prime, unsigned int ndigits)
>  {
> -   u64 product[2 * ndigits];
> +   u64 product[2 * ECC_MAX_DIGITS];
>
> vli_mult(product, left, right, ndigits);
> vli_mmod_fast(result, product, curve_prime, ndigits);
> @@ -546,7 +546,7 @@ static void vli_mod_mult_fast(u64 *result, const u64 
> *left, const u64 *right,
>  static void vli_mod_square_fast(u64 *result, const u64 *left,
> const u64 *curve_prime, unsigned int ndigits)
>  {
> -   u64 product[2 * ndigits];
> +   u64 product[2 * ECC_MAX_DIGITS];
>
> vli_square(product, left, ndigits);
> vli_mmod_fast(result, product, curve_prime, ndigits);
> @@ -560,8 +560,8 @@ static void vli_mod_square_fast(u64 *result, const u64 
> *left,
>  static void vli_mod_inv(u64 *result, const u64 *input, const u64 *mod,
> unsigned int ndigits)
>  {
> -   u64 a[ndigits], b[ndigits];
> -   u64 u[ndigits], v[ndigits];
> +   u64 a[ECC_MAX_DIGITS], b[ECC_MAX_DIGITS];
> +   u64 u[ECC_MAX_DIGITS], v[ECC_MAX_DIGITS];
> u64 carry;
> int cmp_result;
>
> @@ -649,8 +649,8 @@ static void ecc_point_double_jacobian(u64 *x1, u64 *y1, 
> u64 *z1,
>   u64 *curve_prime, unsigned int ndigits)
>  {
> /* t1 = x, t2 = y, t3 = z */
> -   u64 t4[ndigits];
> -   u64 t5[ndigits];
> +   u64 t4[ECC_MAX_DIGITS];
> +   u64 t5[ECC_MAX_DIGITS];
>
> if (vli_is_zero(z1, ndigits))
> return;
> @@ -711,7 +711,7 @@ static void ecc_point_double_jacobian(u64 *x1, u64 *y1, 
> u64 *z1,
>  static void apply_z(u64 *x1, u64 *y1, u64 *z, u64 *curve_prime,
> unsigned int ndigits)
>  {
> -   u64 t1[ndigits];
> +   u64 t1[ECC_MAX_DIGITS];
>
> vli_mod_square_fast(t1, z, curve_prime, ndigits);/* z^2 */
> vli_mod_mult_fast(x1, x1, t1, curve_prime, ndigits); /* x1 * z^2 */
> @@ -724,7 +724,7 @@ static void xycz_initial_double(u64 *x1, u64 *y1, u64 
> *x2, u64 *y2,
> u64 *p_initial_z, u64 *curve_prime,
> unsigned int ndigits)
>  {
> -   u64 z[ndigits];
> +   u64 z[ECC_MAX_DIGITS];
>
> vli_set(x2, x1, ndigits);
> vli_set(y2, y1, ndigits);
> @@ -750,7 +750,7 @@ static void xycz_add(u64 *x1, u64 *y1, u64 *x2, u64 *y2, 
> u64 *curve_prime,
>  unsigned int ndigits)
>  {
> /* t1 = X1, t2 = Y1, t3 = X2, t4 = Y2 */
> -   u64 t5[ndigits];
> +   u64 t5[ECC_MAX_DIGITS];
>
> /* t5 = x2 - x1 */
> vli_mod_sub(t5, x2, x1, curve_prime, ndigits);
> @@ -791,9 +791,9 @@ static void xycz_add_c(u64 *x1, u64 *y1, u64 *x2, u64 
> *y2, u64 *curve_prime,
>unsigned int ndigits)
>  {
> /* t1 = X1, t2 = Y1, t3 = X2, t4 = Y2 */
> -   u64 t5[ndigits];
> -   u64 t6[ndigits];
> -   u64 t7[ndigits];
> +  

Re: [PATCH 0/6] Remove several VLAs in the crypto subsystem

[Kees Cook]

On Sat, Apr 7, 2018 at 11:38 AM, Salvatore Mesoraca
<s.mesorac...@gmail.com> wrote:
> As suggested by Laura Abbott[2], I'm resending my patch with
> MAX_BLOCKSIZE and MAX_ALIGNMASK defined in an header, so they
> can be used in other places.
> I take this opportuinuty to deal with some other VLAs not
> handled in the old patch.
>
> [1] 
> http://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
> [2] http://lkml.kernel.org/r/4e536889-439a-49e6-dd95-2d4286913...@redhat.com
>
> Salvatore Mesoraca (6):
>   crypto: api - laying macros for statically allocated buffers
>   crypto: ctr - avoid VLA use
>   crypto: api - avoid VLA use
>   crypto: pcbc - avoid VLA use
>   crypto: cts - avoid VLA use
>   crypto: cfb - avoid VLA use
>
>  crypto/cfb.c  | 14 ++
>  crypto/cipher.c   |  7 ++-
>  crypto/ctr.c  | 13 +++--
>  crypto/cts.c  |  8 ++--
>  crypto/internal.h |  8 
>  crypto/pcbc.c |  9 +++--
>  6 files changed, 48 insertions(+), 11 deletions(-)

These all look good to me! Thanks for the refactoring. :)

Reviewed-by: Kees Cook <keesc...@chromium.org>

-Kees

-- 
Kees Cook
Pixel Security

[PATCH v2] crypto/ecc: Actually remove stack VLA usage

[Kees Cook]

On the quest to remove all VLAs from the kernel[1], this avoids VLAs
by just using the maximum allocation size (4 bytes) for stack arrays.
All the VLAs in ecc were either 3 or 4 bytes (or a multiple), so just
make it 4 bytes all the time. Initialization routines are adjusted to
check that ndigits does not end up larger than the arrays.

This includes a removal of the earlier attempt at this fix from
commit a963834b4742 ("crypto/ecc: Remove stack VLA usage")

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Kees Cook <keesc...@chromium.org>
---
v2:
- Squash revert (herbert)
---
 crypto/ecc.c  | 66 +--
 crypto/ecc.h  |  4 +++-
 crypto/ecdh.c |  4 ++--
 3 files changed, 33 insertions(+), 41 deletions(-)

diff --git a/crypto/ecc.c b/crypto/ecc.c
index 9c066b5ac12d..815541309a95 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -515,7 +515,7 @@ static void vli_mmod_fast_256(u64 *result, const u64 
*product,
 static bool vli_mmod_fast(u64 *result, u64 *product,
  const u64 *curve_prime, unsigned int ndigits)
 {
-   u64 tmp[2 * ndigits];
+   u64 tmp[2 * ECC_MAX_DIGITS];
 
switch (ndigits) {
case 3:
@@ -536,7 +536,7 @@ static bool vli_mmod_fast(u64 *result, u64 *product,
 static void vli_mod_mult_fast(u64 *result, const u64 *left, const u64 *right,
  const u64 *curve_prime, unsigned int ndigits)
 {
-   u64 product[2 * ndigits];
+   u64 product[2 * ECC_MAX_DIGITS];
 
vli_mult(product, left, right, ndigits);
vli_mmod_fast(result, product, curve_prime, ndigits);
@@ -546,7 +546,7 @@ static void vli_mod_mult_fast(u64 *result, const u64 *left, 
const u64 *right,
 static void vli_mod_square_fast(u64 *result, const u64 *left,
const u64 *curve_prime, unsigned int ndigits)
 {
-   u64 product[2 * ndigits];
+   u64 product[2 * ECC_MAX_DIGITS];
 
vli_square(product, left, ndigits);
vli_mmod_fast(result, product, curve_prime, ndigits);
@@ -560,8 +560,8 @@ static void vli_mod_square_fast(u64 *result, const u64 
*left,
 static void vli_mod_inv(u64 *result, const u64 *input, const u64 *mod,
unsigned int ndigits)
 {
-   u64 a[ndigits], b[ndigits];
-   u64 u[ndigits], v[ndigits];
+   u64 a[ECC_MAX_DIGITS], b[ECC_MAX_DIGITS];
+   u64 u[ECC_MAX_DIGITS], v[ECC_MAX_DIGITS];
u64 carry;
int cmp_result;
 
@@ -649,8 +649,8 @@ static void ecc_point_double_jacobian(u64 *x1, u64 *y1, u64 
*z1,
  u64 *curve_prime, unsigned int ndigits)
 {
/* t1 = x, t2 = y, t3 = z */
-   u64 t4[ndigits];
-   u64 t5[ndigits];
+   u64 t4[ECC_MAX_DIGITS];
+   u64 t5[ECC_MAX_DIGITS];
 
if (vli_is_zero(z1, ndigits))
return;
@@ -711,7 +711,7 @@ static void ecc_point_double_jacobian(u64 *x1, u64 *y1, u64 
*z1,
 static void apply_z(u64 *x1, u64 *y1, u64 *z, u64 *curve_prime,
unsigned int ndigits)
 {
-   u64 t1[ndigits];
+   u64 t1[ECC_MAX_DIGITS];
 
vli_mod_square_fast(t1, z, curve_prime, ndigits);/* z^2 */
vli_mod_mult_fast(x1, x1, t1, curve_prime, ndigits); /* x1 * z^2 */
@@ -724,7 +724,7 @@ static void xycz_initial_double(u64 *x1, u64 *y1, u64 *x2, 
u64 *y2,
u64 *p_initial_z, u64 *curve_prime,
unsigned int ndigits)
 {
-   u64 z[ndigits];
+   u64 z[ECC_MAX_DIGITS];
 
vli_set(x2, x1, ndigits);
vli_set(y2, y1, ndigits);
@@ -750,7 +750,7 @@ static void xycz_add(u64 *x1, u64 *y1, u64 *x2, u64 *y2, 
u64 *curve_prime,
 unsigned int ndigits)
 {
/* t1 = X1, t2 = Y1, t3 = X2, t4 = Y2 */
-   u64 t5[ndigits];
+   u64 t5[ECC_MAX_DIGITS];
 
/* t5 = x2 - x1 */
vli_mod_sub(t5, x2, x1, curve_prime, ndigits);
@@ -791,9 +791,9 @@ static void xycz_add_c(u64 *x1, u64 *y1, u64 *x2, u64 *y2, 
u64 *curve_prime,
   unsigned int ndigits)
 {
/* t1 = X1, t2 = Y1, t3 = X2, t4 = Y2 */
-   u64 t5[ndigits];
-   u64 t6[ndigits];
-   u64 t7[ndigits];
+   u64 t5[ECC_MAX_DIGITS];
+   u64 t6[ECC_MAX_DIGITS];
+   u64 t7[ECC_MAX_DIGITS];
 
/* t5 = x2 - x1 */
vli_mod_sub(t5, x2, x1, curve_prime, ndigits);
@@ -846,9 +846,9 @@ static void ecc_point_mult(struct ecc_point *result,
   unsigned int ndigits)
 {
/* R0 and R1 */
-   u64 rx[2][ndigits];
-   u64 ry[2][ndigits];
-   u64 z[ndigits];
+   u64 rx[2][ECC_MAX_DIGITS];
+   u64 ry[2][ECC_MAX_DIGITS];
+   u64 z[ECC_MAX_DIGITS];
int i, nb;
int num_bits = vli_num_bits(scalar, ndigits);
 
@@ -943,13 +943,13 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int 
ndigits,
 int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64 *privkey)
 {
const 

[PATCH v3] crypto: ecc: Remove stack VLA usage

[Kees Cook]

On the quest to remove all VLAs from the kernel[1], this avoids VLAs
by just using the maximum allocation size (4 bytes) for stack arrays.
All the VLAs in ecc were either 3 or 4 bytes (or a multiple), so just
make it 4 bytes all the time. Initialization routines are adjusted to
check that ndigits does not end up larger than the arrays.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Kees Cook <keesc...@chromium.org>
---
This expects 14de52112ee70ca289fa77bf2d9cbc79fd2c811f to be reverted.
---
 crypto/ecc.c  | 47 ---
 crypto/ecc.h  |  4 +++-
 crypto/ecdh.c |  4 ++--
 3 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/crypto/ecc.c b/crypto/ecc.c
index 18f32f2a5e1c..815541309a95 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -515,7 +515,7 @@ static void vli_mmod_fast_256(u64 *result, const u64 
*product,
 static bool vli_mmod_fast(u64 *result, u64 *product,
  const u64 *curve_prime, unsigned int ndigits)
 {
-   u64 tmp[2 * ndigits];
+   u64 tmp[2 * ECC_MAX_DIGITS];
 
switch (ndigits) {
case 3:
@@ -536,7 +536,7 @@ static bool vli_mmod_fast(u64 *result, u64 *product,
 static void vli_mod_mult_fast(u64 *result, const u64 *left, const u64 *right,
  const u64 *curve_prime, unsigned int ndigits)
 {
-   u64 product[2 * ndigits];
+   u64 product[2 * ECC_MAX_DIGITS];
 
vli_mult(product, left, right, ndigits);
vli_mmod_fast(result, product, curve_prime, ndigits);
@@ -546,7 +546,7 @@ static void vli_mod_mult_fast(u64 *result, const u64 *left, 
const u64 *right,
 static void vli_mod_square_fast(u64 *result, const u64 *left,
const u64 *curve_prime, unsigned int ndigits)
 {
-   u64 product[2 * ndigits];
+   u64 product[2 * ECC_MAX_DIGITS];
 
vli_square(product, left, ndigits);
vli_mmod_fast(result, product, curve_prime, ndigits);
@@ -560,8 +560,8 @@ static void vli_mod_square_fast(u64 *result, const u64 
*left,
 static void vli_mod_inv(u64 *result, const u64 *input, const u64 *mod,
unsigned int ndigits)
 {
-   u64 a[ndigits], b[ndigits];
-   u64 u[ndigits], v[ndigits];
+   u64 a[ECC_MAX_DIGITS], b[ECC_MAX_DIGITS];
+   u64 u[ECC_MAX_DIGITS], v[ECC_MAX_DIGITS];
u64 carry;
int cmp_result;
 
@@ -649,8 +649,8 @@ static void ecc_point_double_jacobian(u64 *x1, u64 *y1, u64 
*z1,
  u64 *curve_prime, unsigned int ndigits)
 {
/* t1 = x, t2 = y, t3 = z */
-   u64 t4[ndigits];
-   u64 t5[ndigits];
+   u64 t4[ECC_MAX_DIGITS];
+   u64 t5[ECC_MAX_DIGITS];
 
if (vli_is_zero(z1, ndigits))
return;
@@ -711,7 +711,7 @@ static void ecc_point_double_jacobian(u64 *x1, u64 *y1, u64 
*z1,
 static void apply_z(u64 *x1, u64 *y1, u64 *z, u64 *curve_prime,
unsigned int ndigits)
 {
-   u64 t1[ndigits];
+   u64 t1[ECC_MAX_DIGITS];
 
vli_mod_square_fast(t1, z, curve_prime, ndigits);/* z^2 */
vli_mod_mult_fast(x1, x1, t1, curve_prime, ndigits); /* x1 * z^2 */
@@ -724,7 +724,7 @@ static void xycz_initial_double(u64 *x1, u64 *y1, u64 *x2, 
u64 *y2,
u64 *p_initial_z, u64 *curve_prime,
unsigned int ndigits)
 {
-   u64 z[ndigits];
+   u64 z[ECC_MAX_DIGITS];
 
vli_set(x2, x1, ndigits);
vli_set(y2, y1, ndigits);
@@ -750,7 +750,7 @@ static void xycz_add(u64 *x1, u64 *y1, u64 *x2, u64 *y2, 
u64 *curve_prime,
 unsigned int ndigits)
 {
/* t1 = X1, t2 = Y1, t3 = X2, t4 = Y2 */
-   u64 t5[ndigits];
+   u64 t5[ECC_MAX_DIGITS];
 
/* t5 = x2 - x1 */
vli_mod_sub(t5, x2, x1, curve_prime, ndigits);
@@ -791,9 +791,9 @@ static void xycz_add_c(u64 *x1, u64 *y1, u64 *x2, u64 *y2, 
u64 *curve_prime,
   unsigned int ndigits)
 {
/* t1 = X1, t2 = Y1, t3 = X2, t4 = Y2 */
-   u64 t5[ndigits];
-   u64 t6[ndigits];
-   u64 t7[ndigits];
+   u64 t5[ECC_MAX_DIGITS];
+   u64 t6[ECC_MAX_DIGITS];
+   u64 t7[ECC_MAX_DIGITS];
 
/* t5 = x2 - x1 */
vli_mod_sub(t5, x2, x1, curve_prime, ndigits);
@@ -846,9 +846,9 @@ static void ecc_point_mult(struct ecc_point *result,
   unsigned int ndigits)
 {
/* R0 and R1 */
-   u64 rx[2][ndigits];
-   u64 ry[2][ndigits];
-   u64 z[ndigits];
+   u64 rx[2][ECC_MAX_DIGITS];
+   u64 ry[2][ECC_MAX_DIGITS];
+   u64 z[ECC_MAX_DIGITS];
int i, nb;
int num_bits = vli_num_bits(scalar, ndigits);
 
@@ -943,13 +943,13 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int 
ndigits,
 int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64 *privkey)
 {
const struct ecc_curve *curve = ecc_get_curve(curve_id);
-   u64 priv[ndigits];
+   u64 priv[ECC_MAX_

Re: [PATCH v2] crypto/ecc: Remove stack VLA usage

[Kees Cook]

On Fri, Mar 16, 2018 at 8:56 AM, Herbert Xu <herb...@gondor.apana.org.au> wrote:
> On Thu, Mar 08, 2018 at 01:57:02PM -0800, Kees Cook wrote:
>> On the quest to remove all VLAs from the kernel[1], this switches to
>> a pair of kmalloc regions instead of using the stack. This also moves
>> the get_random_bytes() after all allocations (and drops the needless
>> "nbytes" variable).
>>
>> [1] https://lkml.org/lkml/2018/3/7/621
>>
>> Signed-off-by: Kees Cook <keesc...@chromium.org>
>
> Patch applied.  Thanks.

Hi, sorry for the noise on this one: I messed up looking at the ecc
code (I confused myself into thinking there was only a single instance
of the problem). The applied patch is both incomplete and inefficient.
I have a much simpler solution, and I'll send that with a revert...

-Kees

-- 
Kees Cook
Pixel Security

[PATCH v2] crypto/ecc: Remove stack VLA usage

[Kees Cook]

On the quest to remove all VLAs from the kernel[1], this switches to
a pair of kmalloc regions instead of using the stack. This also moves
the get_random_bytes() after all allocations (and drops the needless
"nbytes" variable).

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Kees Cook <keesc...@chromium.org>
---
 crypto/ecc.c | 23 +--
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/crypto/ecc.c b/crypto/ecc.c
index 18f32f2a5e1c..9c066b5ac12d 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -1025,9 +1025,7 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, 
unsigned int ndigits,
 {
int ret = 0;
struct ecc_point *product, *pk;
-   u64 priv[ndigits];
-   u64 rand_z[ndigits];
-   unsigned int nbytes;
+   u64 *priv, *rand_z;
const struct ecc_curve *curve = ecc_get_curve(curve_id);
 
if (!private_key || !public_key || !curve) {
@@ -1035,14 +1033,22 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, 
unsigned int ndigits,
goto out;
}
 
-   nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
+   priv = kmalloc_array(ndigits, sizeof(*priv), GFP_KERNEL);
+   if (!priv) {
+   ret = -ENOMEM;
+   goto out;
+   }
 
-   get_random_bytes(rand_z, nbytes);
+   rand_z = kmalloc_array(ndigits, sizeof(*rand_z), GFP_KERNEL);
+   if (!rand_z) {
+   ret = -ENOMEM;
+   goto kfree_out;
+   }
 
pk = ecc_alloc_point(ndigits);
if (!pk) {
ret = -ENOMEM;
-   goto out;
+   goto kfree_out;
}
 
product = ecc_alloc_point(ndigits);
@@ -1051,6 +1057,8 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, 
unsigned int ndigits,
goto err_alloc_product;
}
 
+   get_random_bytes(rand_z, ndigits << ECC_DIGITS_TO_BYTES_SHIFT);
+
ecc_swap_digits(public_key, pk->x, ndigits);
ecc_swap_digits(_key[ndigits], pk->y, ndigits);
ecc_swap_digits(private_key, priv, ndigits);
@@ -1065,6 +1073,9 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, 
unsigned int ndigits,
ecc_free_point(product);
 err_alloc_product:
ecc_free_point(pk);
+kfree_out:
+   kzfree(priv);
+   kzfree(rand_z);
 out:
    return ret;
 }
-- 
2.7.4


-- 
Kees Cook
Pixel Security

Re: [PATCH] crypto/ecc: Remove stack VLA usage

[Kees Cook]

On Thu, Mar 8, 2018 at 1:43 AM, Tudor Ambarus
<tudor.amba...@microchip.com> wrote:
> Hi, Kees,
>
>
> On 03/07/2018 11:56 PM, Kees Cook wrote:
>>
>> On the quest to remove all VLAs from the kernel[1], this switches to
>> a pair of kmalloc regions instead of using the stack. This also moves
>> the get_random_bytes() after all allocations (and drops the needless
>> "nbytes" variable).
>>
>> [1] https://lkml.org/lkml/2018/3/7/621
>>
>> Signed-off-by: Kees Cook <keesc...@chromium.org>
>> ---
>>   crypto/ecc.c | 23 +--
>>   1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/crypto/ecc.c b/crypto/ecc.c
>> index 18f32f2a5e1c..5bfa63603da0 100644
>> --- a/crypto/ecc.c
>> +++ b/crypto/ecc.c
>> @@ -1025,9 +1025,7 @@ int crypto_ecdh_shared_secret(unsigned int curve_id,
>> unsigned int ndigits,
>>   {
>> int ret = 0;
>> struct ecc_point *product, *pk;
>> -   u64 priv[ndigits];
>> -   u64 rand_z[ndigits];
>> -   unsigned int nbytes;
>> +   u64 *priv, *rand_z;
>> const struct ecc_curve *curve = ecc_get_curve(curve_id);
>> if (!private_key || !public_key || !curve) {
>> @@ -1035,14 +1033,22 @@ int crypto_ecdh_shared_secret(unsigned int
>> curve_id, unsigned int ndigits,
>> goto out;
>> }
>>   - nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
>> +   priv = kmalloc_array(ndigits, sizeof(*priv), GFP_KERNEL);
>> +   if (!priv) {
>> +   ret = -ENOMEM;
>> +   goto out;
>> +   }
>>   - get_random_bytes(rand_z, nbytes);
>> +   rand_z = kmalloc_array(ndigits, sizeof(*rand_z), GFP_KERNEL);
>> +   if (!rand_z) {
>> +   ret = -ENOMEM;
>> +   goto kfree_out;
>> +   }
>> pk = ecc_alloc_point(ndigits);
>> if (!pk) {
>> ret = -ENOMEM;
>> -   goto out;
>> +   goto kfree_out;
>> }
>> product = ecc_alloc_point(ndigits);
>> @@ -1051,6 +1057,8 @@ int crypto_ecdh_shared_secret(unsigned int curve_id,
>> unsigned int ndigits,
>> goto err_alloc_product;
>> }
>>   + get_random_bytes(rand_z, ndigits << ECC_DIGITS_TO_BYTES_SHIFT);
>> +
>> ecc_swap_digits(public_key, pk->x, ndigits);
>> ecc_swap_digits(_key[ndigits], pk->y, ndigits);
>> ecc_swap_digits(private_key, priv, ndigits);
>> @@ -1065,6 +1073,9 @@ int crypto_ecdh_shared_secret(unsigned int curve_id,
>> unsigned int ndigits,
>> ecc_free_point(product);
>>   err_alloc_product:
>> ecc_free_point(pk);
>> +kfree_out:
>> +   kfree(priv);
>
>
> I think we should use kzfree here.
>
>> +   kfree(rand_z);
>
>
> Probably here too.

Ah yeah, good idea. I'll send a v2.

> Looks like there are few intermediate buffers in ecc
> that should be zeroized as well.

Can you send a patch for those?

Thanks!

-Kees

>
> Best,
> ta
>>
>>   out:
>> return ret;
>>   }
>>
>



-- 
Kees Cook
Pixel Security

[PATCH] crypto/ecc: Remove stack VLA usage

[Kees Cook]

On the quest to remove all VLAs from the kernel[1], this switches to
a pair of kmalloc regions instead of using the stack. This also moves
the get_random_bytes() after all allocations (and drops the needless
"nbytes" variable).

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Kees Cook <keesc...@chromium.org>
---
 crypto/ecc.c | 23 +--
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/crypto/ecc.c b/crypto/ecc.c
index 18f32f2a5e1c..5bfa63603da0 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -1025,9 +1025,7 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, 
unsigned int ndigits,
 {
int ret = 0;
struct ecc_point *product, *pk;
-   u64 priv[ndigits];
-   u64 rand_z[ndigits];
-   unsigned int nbytes;
+   u64 *priv, *rand_z;
const struct ecc_curve *curve = ecc_get_curve(curve_id);
 
if (!private_key || !public_key || !curve) {
@@ -1035,14 +1033,22 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, 
unsigned int ndigits,
goto out;
}
 
-   nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
+   priv = kmalloc_array(ndigits, sizeof(*priv), GFP_KERNEL);
+   if (!priv) {
+   ret = -ENOMEM;
+   goto out;
+   }
 
-   get_random_bytes(rand_z, nbytes);
+   rand_z = kmalloc_array(ndigits, sizeof(*rand_z), GFP_KERNEL);
+   if (!rand_z) {
+   ret = -ENOMEM;
+   goto kfree_out;
+   }
 
pk = ecc_alloc_point(ndigits);
if (!pk) {
ret = -ENOMEM;
-   goto out;
+   goto kfree_out;
}
 
product = ecc_alloc_point(ndigits);
@@ -1051,6 +1057,8 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, 
unsigned int ndigits,
goto err_alloc_product;
}
 
+   get_random_bytes(rand_z, ndigits << ECC_DIGITS_TO_BYTES_SHIFT);
+
ecc_swap_digits(public_key, pk->x, ndigits);
ecc_swap_digits(_key[ndigits], pk->y, ndigits);
ecc_swap_digits(private_key, priv, ndigits);
@@ -1065,6 +1073,9 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, 
unsigned int ndigits,
ecc_free_point(product);
 err_alloc_product:
ecc_free_point(pk);
+kfree_out:
+   kfree(priv);
+   kfree(rand_z);
 out:
    return ret;
 }
-- 
2.7.4


-- 
Kees Cook
Pixel Security

Re: [PATCH 2/8] fs: pstore: remove unused hardirq.h

[Kees Cook]

On Fri, Nov 17, 2017 at 3:02 PM, Yang Shi <yan...@alibaba-inc.com> wrote:
> Preempt counter APIs have been split out, currently, hardirq.h just
> includes irq_enter/exit APIs which are not used by pstore at all.
>
> So, remove the unused hardirq.h.
>
> Signed-off-by: Yang Shi <yan...@alibaba-inc.com>
> Cc: Kees Cook <keesc...@chromium.org>
> Cc: Anton Vorontsov <an...@enomsg.org>
> Cc: Colin Cross <ccr...@android.com>
> Cc: Tony Luck <tony.l...@intel.com>

Thanks! I've applied this for -next.

-Kees

> ---
>  fs/pstore/platform.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 2b21d18..25dcef4 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -41,7 +41,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>
> --
> 1.8.3.1
>



-- 
Kees Cook
Pixel Security

[PATCH] drivers/crypto: Convert timers to use timer_setup()

[Kees Cook]

In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.

Cc: Herbert Xu <herb...@gondor.apana.org.au>
Cc: Jesper Nilsson <jesper.nils...@axis.com>
Cc: Lars Persson <lars.pers...@axis.com>
Cc: Niklas Cassel <niklas.cas...@axis.com>
Cc: "David S. Miller" <da...@davemloft.net>
Cc: Jamie Iles <ja...@jamieiles.com>
Cc: linux-arm-ker...@axis.com
Cc: linux-crypto@vger.kernel.org
Cc: linux-arm-ker...@lists.infradead.org
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
 drivers/crypto/axis/artpec6_crypto.c | 6 +++---
 drivers/crypto/mv_cesa.c | 4 ++--
 drivers/crypto/picoxcell_crypto.c| 7 +++
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/crypto/axis/artpec6_crypto.c 
b/drivers/crypto/axis/artpec6_crypto.c
index 0f9754e07719..456278440863 100644
--- a/drivers/crypto/axis/artpec6_crypto.c
+++ b/drivers/crypto/axis/artpec6_crypto.c
@@ -2072,9 +2072,9 @@ static void artpec6_crypto_process_queue(struct 
artpec6_crypto *ac)
del_timer(>timer);
 }
 
-static void artpec6_crypto_timeout(unsigned long data)
+static void artpec6_crypto_timeout(struct timer_list *t)
 {
-   struct artpec6_crypto *ac = (struct artpec6_crypto *) data;
+   struct artpec6_crypto *ac = from_timer(ac, t, timer);
 
dev_info_ratelimited(artpec6_crypto_dev, "timeout\n");
 
@@ -3063,7 +3063,7 @@ static int artpec6_crypto_probe(struct platform_device 
*pdev)
spin_lock_init(>queue_lock);
INIT_LIST_HEAD(>queue);
INIT_LIST_HEAD(>pending);
-   setup_timer(>timer, artpec6_crypto_timeout, (unsigned long) ac);
+   timer_setup(>timer, artpec6_crypto_timeout, 0);
 
ac->base = base;
 
diff --git a/drivers/crypto/mv_cesa.c b/drivers/crypto/mv_cesa.c
index c3883b49f56e..6a8275f017a8 100644
--- a/drivers/crypto/mv_cesa.c
+++ b/drivers/crypto/mv_cesa.c
@@ -149,7 +149,7 @@ struct mv_req_hash_ctx {
int count_add;
 };
 
-static void mv_completion_timer_callback(unsigned long unused)
+static void mv_completion_timer_callback(struct timer_list *unused)
 {
int active = readl(cpg->reg + SEC_ACCEL_CMD) & SEC_CMD_EN_SEC_ACCL0;
 
@@ -167,7 +167,7 @@ static void mv_completion_timer_callback(unsigned long 
unused)
 
 static void mv_setup_timer(void)
 {
-   setup_timer(>completion_timer, _completion_timer_callback, 0);
+   timer_setup(>completion_timer, mv_completion_timer_callback, 0);
mod_timer(>completion_timer,
jiffies + msecs_to_jiffies(MV_CESA_EXPIRE));
 }
diff --git a/drivers/crypto/picoxcell_crypto.c 
b/drivers/crypto/picoxcell_crypto.c
index b6f14844702e..5a6dc53b2b9d 100644
--- a/drivers/crypto/picoxcell_crypto.c
+++ b/drivers/crypto/picoxcell_crypto.c
@@ -1125,9 +1125,9 @@ static irqreturn_t spacc_spacc_irq(int irq, void *dev)
return IRQ_HANDLED;
 }
 
-static void spacc_packet_timeout(unsigned long data)
+static void spacc_packet_timeout(struct timer_list *t)
 {
-   struct spacc_engine *engine = (struct spacc_engine *)data;
+   struct spacc_engine *engine = from_timer(engine, t, packet_timeout);
 
spacc_process_done(engine);
 }
@@ -1714,8 +1714,7 @@ static int spacc_probe(struct platform_device *pdev)
writel(SPA_IRQ_EN_STAT_EN | SPA_IRQ_EN_GLBL_EN,
   engine->regs + SPA_IRQ_EN_REG_OFFSET);
 
-   setup_timer(>packet_timeout, spacc_packet_timeout,
-   (unsigned long)engine);
+   timer_setup(>packet_timeout, spacc_packet_timeout, 0);
 
INIT_LIST_HEAD(>pending);
INIT_LIST_HEAD(>completed);
-- 
2.7.4


-- 
Kees Cook
Pixel Security

Re: [PATCH 1/2] random: always call random ready function

[Kees Cook]

On Thu, Oct 19, 2017 at 1:45 PM, Jason A. Donenfeld <ja...@zx2c4.com> wrote:
> As this interface becomes more heavily used, it will be painful for
> callers to always need to check for -EALREADY.
>
> Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
> ---
>  drivers/char/random.c | 24 
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 8ad92707e45f..e161acf35d4a 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1556,40 +1556,45 @@ EXPORT_SYMBOL(wait_for_random_bytes);
>
>  /*
>   * Add a callback function that will be invoked when the nonblocking
> - * pool is initialised.
> + * pool is initialised, or calls that function if it already is.
>   *
>   * returns: 0 if callback is successfully added
> - * -EALREADY if pool is already initialised (callback not called)
>   * -ENOENT if module for callback is not alive
>   */
>  int add_random_ready_callback(struct random_ready_callback *rdy)
>  {
> struct module *owner;
> unsigned long flags;
> -   int err = -EALREADY;
>
> -   if (crng_ready())
> -   return err;
> +   BUG_ON(!rdy->func);

Better to WARN_ON() and return a failure.

> +
> +   if (crng_ready()) {
> +   rdy->func(rdy);
> +   rdy->func = NULL;
> +   return 0;
> +   }
>
> owner = rdy->owner;
> if (!try_module_get(owner))
> return -ENOENT;
>
> spin_lock_irqsave(_ready_list_lock, flags);
> -   if (crng_ready())
> +   if (crng_ready()) {
> +   rdy->func(rdy);
> +   rdy->func = NULL;
> goto out;
> +   }
>
> owner = NULL;
>
> list_add(>list, _ready_list);
> -   err = 0;
>
>  out:
> spin_unlock_irqrestore(_ready_list_lock, flags);
>
> module_put(owner);
>
> -   return err;
> +   return 0;
>  }
>  EXPORT_SYMBOL(add_random_ready_callback);
>
> @@ -1601,6 +1606,9 @@ void del_random_ready_callback(struct 
> random_ready_callback *rdy)
> unsigned long flags;
> struct module *owner = NULL;
>
> +   if (!rdy->func)
> +   return;

Perhaps add a note here about what a NULL func means here? (e.g.
"Already run, not in the list.")

> +
> spin_lock_irqsave(_ready_list_lock, flags);
> if (!list_empty(>list)) {
> list_del_init(>list);
> --
> 2.14.2
>

Otherwise, yeah, looks sensible.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] random: warn when kernel uses unseeded randomness

[Kees Cook]

On Tue, Jun 20, 2017 at 5:03 PM, Jason A. Donenfeld <ja...@zx2c4.com> wrote:
> This enables an important dmesg notification about when drivers have
> used the crng without it being seeded first. Prior, these errors would
> occur silently, and so there hasn't been a great way of diagnosing these
> types of bugs for obscure setups. By adding this as a config option, we
> can leave it on by default, so that we learn where these issues happen,
> in the field, will still allowing some people to turn it off, if they
> really know what they're doing and do not want the log entries.
>
> However, we don't leave it _completely_ by default. An earlier version
> of this patch simply had `default y`. I'd really love that, but it turns
> out, this problem with unseeded randomness being used is really quite
> present and is going to take a long time to fix. Thus, as a compromise
> between log-messages-for-all and nobody-knows, this is `default y`,
> except it is also `depends on DEBUG_KERNEL`. This will ensure that the
> curious see the messages while others don't have to.

This commit log needs updating (default DEBUG_KERNEL, not depends).

But otherwise:

Reviewed-by: Kees Cook <keesc...@chromium.org>

-Kees

>
> Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
> Signed-off-by: Theodore Ts'o <ty...@mit.edu>
> ---
> Hi Ted,
>
> This patch is meant to replace d06bfd1989fe97623b32d6df4ffa6e4338c99dc8,
> which is currently in your dev tree. It switches from using `default y`
> and `depends on DEBUG_KERNEL` to using the more simple `default DEBUG_KERNEL`.
> This kind of change I think should satisfy most potential objections, by
> being present for those who might find it useful, but invisble for those
> who don't want the spam.
>
> If you'd like to replace the earlier commit with this one, feel free. If
> not, that's fine too.
>
> Jason
>
>  drivers/char/random.c | 15 +--
>  lib/Kconfig.debug | 15 +++
>  2 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 3853dd4f92e7..fa5bbd5a7ca0 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -288,7 +288,6 @@
>  #define SEC_XFER_SIZE  512
>  #define EXTRACT_SIZE   10
>
> -#define DEBUG_RANDOM_BOOT 0
>
>  #define LONGS(x) (((x) + sizeof(unsigned long) - 1)/sizeof(unsigned long))
>
> @@ -1481,7 +1480,7 @@ void get_random_bytes(void *buf, int nbytes)
>  {
> __u8 tmp[CHACHA20_BLOCK_SIZE];
>
> -#if DEBUG_RANDOM_BOOT > 0
> +#ifdef CONFIG_WARN_UNSEEDED_RANDOM
> if (!crng_ready())
> printk(KERN_NOTICE "random: %pF get_random_bytes called "
>"with crng_init = %d\n", (void *) _RET_IP_, crng_init);
> @@ -2075,6 +2074,12 @@ u64 get_random_u64(void)
> return ret;
>  #endif
>
> +#ifdef CONFIG_WARN_UNSEEDED_RANDOM
> +   if (!crng_ready())
> +   printk(KERN_NOTICE "random: %pF get_random_u64 called "
> +  "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
> +#endif
> +
> batch = _cpu_var(batched_entropy_u64);
> if (use_lock)
> read_lock_irqsave(_entropy_reset_lock, flags);
> @@ -2101,6 +2106,12 @@ u32 get_random_u32(void)
> if (arch_get_random_int())
> return ret;
>
> +#ifdef CONFIG_WARN_UNSEEDED_RANDOM
> +   if (!crng_ready())
> +   printk(KERN_NOTICE "random: %pF get_random_u32 called "
> +  "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
> +#endif
> +
> batch = _cpu_var(batched_entropy_u32);
> if (use_lock)
> read_lock_irqsave(_entropy_reset_lock, flags);
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index e4587ebe52c7..41cf12288369 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1209,6 +1209,21 @@ config STACKTRACE
>   It is also used by various kernel debugging features that require
>   stack trace generation.
>
> +config WARN_UNSEEDED_RANDOM
> +   bool "Warn when kernel uses unseeded randomness"
> +   default DEBUG_KERNEL
> +   help
> + Some parts of the kernel contain bugs relating to their use of
> + cryptographically secure random numbers before it's actually 
> possible
> + to generate those numbers securely. This setting ensures that these
> + flaws don't go unnoticed, by enabling a message, should this ever
> + occur. This will allow people with obscure setups to know when 
> things
> + are going wrong, so that they might contact developers about fixing
> + it.
> +
> + Say Y here, unless you simply do not care about using unseeded
> + randomness and do not want a potential warning message in your logs.
> +
>  config DEBUG_KOBJECT
> bool "kobject debugging"
> depends on DEBUG_KERNEL
> --
> 2.13.1
>



-- 
Kees Cook
Pixel Security

Re: [kernel-hardening] Re: [PATCH] random: silence compiler warnings and fix race

[Kees Cook]

On Tue, Jun 20, 2017 at 10:50 AM, Sandy Harris <sandyinch...@gmail.com> wrote:
> On Tue, Jun 20, 2017 at 5:49 AM, Jeffrey Walton <noloa...@gmail.com> wrote:
>> On Tue, Jun 20, 2017 at 5:36 AM, Theodore Ts'o <ty...@mit.edu> wrote:
>>> On Tue, Jun 20, 2017 at 10:53:35AM +0200, Jason A. Donenfeld wrote:
>
>>>> > Suppressing all messages for all configurations cast a wider net than
>>>> > necessary. Configurations that could potentially be detected and fixed
>>>> > likely will go unnoticed. If the problem is not brought to light, then
>>>> > it won't be fixed.
>
>> Are there compelling reasons a single dmesg warning cannot be provided?
>>
>> A single message avoids spamming the logs. It also informs the system
>> owner of the problem. An individual or organization can then take
>> action based on their risk posture. Finally, it avoids the kernel
>> making policy decisions for a user or organization.
>
> I'd say the best solution is to have no configuration option
> specifically for these messages. Always give some, but let
> DEBUG_KERNEL control how many.
>
> If DEBUG_KERNEL is not set, emit exactly one message & ignore any
> other errors of this type. On some systems, that message may have to
> be ignored, on some it might start an incremental process where one
> problem gets fixed only to have another crop up & on some it might
> prompt the admin to explore further by compiling with DEBUG_KERNEL.
>
> If DEBUG_KERNEL is set, emit a message for every error of this type.

How about doing this:

   default DEBUG_KERNEL

Most distro kernel select DEBUG_KERNEL because it unhides a bunch of
other useful configs. Since it doesn't strictly _depend_ on
DEBUG_KERNEL, I think it's probably a mistake to enforce a false
dependency. Using it as a hint for the default seems maybe like a good
middle ground. (And if people can't agree on that, then I guess
"default n"...)

-Kees

-- 
Kees Cook
Pixel Security

Re: [kernel-hardening] Re: get_random_bytes returns bad randomness before seeding is complete

[Kees Cook]

On Fri, Jun 2, 2017 at 10:41 AM, Daniel Micay <danielmi...@gmail.com> wrote:
> On Fri, 2017-06-02 at 17:53 +0200, Jason A. Donenfeld wrote:
>> (Meanwhile...)
>>
>> In my own code, I'm currently playing with a workaround that looks
>> like this:
>>
>> --- a/src/main.c
>> +++ b/src/main.c
>>
>> +#include 
>> +#include 
>>
>> +struct rng_initializer {
>> +   struct completion done;
>> +   struct random_ready_callback cb;
>> +};
>> +static void rng_initialized_callback(struct random_ready_callback
>> *cb)
>> +{
>> +   complete(_of(cb, struct rng_initializer, cb)->done);
>> +}
>> +
>> static int __init mod_init(void)
>> {
>>int ret;
>> +   struct rng_initializer rng = {
>> +   .done = COMPLETION_INITIALIZER(rng.done),
>> +   .cb = { .owner = THIS_MODULE, .func =
>> rng_initialized_callback }
>> +   };
>> +
>> +   ret = add_random_ready_callback();
>> +   if (!ret)
>> +   wait_for_completion();
>> +   else if (ret != -EALREADY)
>> +   return ret;
>>
>>do_things_with_get_random_bytes_maybe();
>>
>> Depending on the situation, however, I could imagine that
>> wait_for_completion never returning, if its blocking activity that
>> contributes to the seed actually being available, if this is called
>> from a compiled-in module, so I find this a bit sub-optimal...
>
> One of the early uses is initializing the stack canary value for SSP in
> very early boot. If that blocks, it's going to be blocking nearly
> anything else from happening.
>
> On x86, that's only the initial canary since the per-task canaries end
> up being used, but elsewhere at least without SMP disabled or changes to
> GCC that's all there is so the entropy matters.

And just to note, building with GCC_PLUGIN_LATENT_ENTROPY, while it
(correctly) doesn't credit entropy to the pool, should at least make
the pool less deterministic between boots.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 4/4] fs/pstore: fs/squashfs: Change usage of LZ4 to comply with new LZ4 module version

[Kees Cook]

On Sat, Jan 7, 2017 at 8:55 AM, Sven Schmidt
<4ssch...@informatik.uni-hamburg.de> wrote:
> This patch updates fs/pstore and fs/squashfs to use the updated functions from
> the new LZ4 module.
>
> Signed-off-by: Sven Schmidt <4ssch...@informatik.uni-hamburg.de>
> ---
>  fs/pstore/platform.c  | 14 --
>  fs/squashfs/lz4_wrapper.c | 12 ++--
>  2 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 729677e..a0d8ca8 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -342,31 +342,33 @@ static int compress_lz4(const void *in, void *out, 
> size_t inlen, size_t outlen)
>  {
> int ret;
>
> -   ret = lz4_compress(in, inlen, out, , workspace);
> +   ret = LZ4_compress_default(in, out, inlen, outlen, workspace);
> if (ret) {
> +   // ret is 0 means an error occured

If that's true, then shouldn't the "if" logic be changed? Also, here
and in all following comments are C++ style instead of kernel C-style.
This should be "/* ret == 0 means an error occured */", though really,
that should be obvious from the code and the comment isn't really
needed.

> pr_err("lz4_compress error, ret = %d!\n", ret);

If it's always going to be zero here, is there a better place to get
details on why it failed?

> return -EIO;
> }
>
> -   return outlen;
> +   return ret;
>  }
>
>  static int decompress_lz4(void *in, void *out, size_t inlen, size_t outlen)
>  {
> int ret;
>
> -   ret = lz4_decompress_unknownoutputsize(in, inlen, out, );
> -   if (ret) {
> +   ret = LZ4_decompress_safe(in, out, inlen, outlen);
> +   if (ret < 0) {
> +   // return value is < 0 in case of error
> pr_err("lz4_decompress error, ret = %d!\n", ret);
> return -EIO;
> }
>
> -   return outlen;
> +   return ret;
>  }
>
>  static void allocate_lz4(void)
>  {
> -   big_oops_buf_sz = lz4_compressbound(psinfo->bufsize);
> +   big_oops_buf_sz = LZ4_compressBound(psinfo->bufsize);
> big_oops_buf = kmalloc(big_oops_buf_sz, GFP_KERNEL);
> if (big_oops_buf) {
> workspace = kmalloc(LZ4_MEM_COMPRESS, GFP_KERNEL);
> diff --git a/fs/squashfs/lz4_wrapper.c b/fs/squashfs/lz4_wrapper.c
> index ff4468b..a512399 100644
> --- a/fs/squashfs/lz4_wrapper.c
> +++ b/fs/squashfs/lz4_wrapper.c
> @@ -97,7 +97,6 @@ static int lz4_uncompress(struct squashfs_sb_info *msblk, 
> void *strm,
> struct squashfs_lz4 *stream = strm;
> void *buff = stream->input, *data;
> int avail, i, bytes = length, res;
> -   size_t dest_len = output->length;
>
> for (i = 0; i < b; i++) {
> avail = min(bytes, msblk->devblksize - offset);
> @@ -108,12 +107,13 @@ static int lz4_uncompress(struct squashfs_sb_info 
> *msblk, void *strm,
> put_bh(bh[i]);
> }
>
> -   res = lz4_decompress_unknownoutputsize(stream->input, length,
> -   stream->output, _len);
> -   if (res)
> +   res = LZ4_decompress_safe(stream->input, stream->output, length, 
> output->length);
> +   if (res < 0) {
> +   // res of less than 0 means an error occured
> return -EIO;
> +   }
>
> -   bytes = dest_len;
> +   bytes = res;
> data = squashfs_first_page(output);
>     buff = stream->output;
> while (data) {
> @@ -128,7 +128,7 @@ static int lz4_uncompress(struct squashfs_sb_info *msblk, 
> void *strm,
> }
> squashfs_finish_page(output);
>
> -   return dest_len;
> +   return res;
>  }
>
>  const struct squashfs_decompressor squashfs_lz4_comp_ops = {
> --
> 2.1.4
>

-Kees

-- 
Kees Cook
Nexus Security
--
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] jent: use safe format string parameters

[Kees Cook]

Since the API for jent_panic() does not include format string parameters,
adjust the call to panic() to use a literal string to avoid any future
callers from leaking format strings into the panic message.

Signed-off-by: Kees Cook keesc...@chromium.org
---
 crypto/jitterentropy-kcapi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/jitterentropy-kcapi.c b/crypto/jitterentropy-kcapi.c
index b32d834144cd..ceea83d13168 100644
--- a/crypto/jitterentropy-kcapi.c
+++ b/crypto/jitterentropy-kcapi.c
@@ -79,7 +79,7 @@ int jent_fips_enabled(void)
 
 void jent_panic(char *s)
 {
-   panic(s);
+   panic(%s, s);
 }
 
 void jent_memcpy(void *dest, const void *src, unsigned int n)
-- 
1.9.1


-- 
Kees Cook
Chrome OS Security
--
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 RFC v2 1/2] crypto: add PKE API

[Kees Cook]

 +#define CRYPTO_ALG_TYPE_PKEY   0x000d
  #define CRYPTO_ALG_TYPE_PCOMPRESS  0x000f

  #define CRYPTO_ALG_TYPE_HASH_MASK  0x000e
 diff --git a/include/linux/cryptouser.h b/include/linux/cryptouser.h
 index 4abf2ea..fdf9120 100644
 --- a/include/linux/cryptouser.h
 +++ b/include/linux/cryptouser.h
 @@ -43,6 +43,7 @@ enum crypto_attr_type_t {
 CRYPTOCFGA_REPORT_COMPRESS, /* struct crypto_report_comp */
 CRYPTOCFGA_REPORT_RNG,  /* struct crypto_report_rng */
 CRYPTOCFGA_REPORT_CIPHER,   /* struct crypto_report_cipher */
 +   CRYPTOCFGA_REPORT_PKEY, /* struct crypto_report_pkey */
 __CRYPTOCFGA_MAX

  #define CRYPTOCFGA_MAX (__CRYPTOCFGA_MAX - 1)
 @@ -101,5 +102,11 @@ struct crypto_report_rng {
 unsigned int seedsize;
  };

 +struct crypto_report_pkey {
 +   char type[CRYPTO_MAX_NAME];
 +   char subtype[CRYPTO_MAX_NAME];
 +   unsigned int capabilities;
 +};
 +
  #define CRYPTO_REPORT_MAXSIZE (sizeof(struct crypto_user_alg) + \
sizeof(struct crypto_report_blkcipher))


Thanks!

-Kees

-- 
Kees Cook
Chrome OS Security
--
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: [BISECTED] 4943ba16 (include crypto- module prefix) breaks wifi

[Kees Cook]

On Tue, Feb 17, 2015 at 11:56 PM, Mathias Krause mini...@googlemail.com wrote:
 On 18 February 2015 at 07:34, George Spelvin li...@horizon.com wrote:

 It's trying to load modules named:

 crypto-ccm(aes)
 crypto-ccm(aes)-all
 crypto-ctr(aes)
 crypto-ctr(aes)-all

 depmod -n doesn't show any aliases with parens in their names,

 That's okay. Also that it fails to load these as it'll fall back
 trying to load modules for the templates in that case, as can be seen
 in your log:

 Wed Feb 18 06:58:10 UTC 2015 /sbin/modprobe -q -- crypto-ctr

 What's curious, however, that it only tries to load the template for
 ctr, not for ccm. :/
 Are the logs complete? Could you please simplify your /sbin/x/modprobe
 wrapper to just output the modprobe call, as Kees suggested?

George, any updates on this?

 Also, could you please provide the output of depmod -n | grep
 crypto-? There should be lines for crypto-ccm and crypto-ctr if you
 build them as modules.

I still haven't been able to reproduce this. :(

-Kees

-- 
Kees Cook
Chrome OS Security
--
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: [BISECTED] 4943ba16 (include crypto- module prefix) breaks wifi

[Kees Cook]

On Mon, Feb 16, 2015 at 10:49 PM, Mathias Krause mini...@googlemail.com wrote:
 On 17 February 2015 at 04:09, George Spelvin li...@horizon.com wrote:
 I discovered when (belatedly) testing 3.19-rc7 the other week that
 my laptop wifi was broken and would no longer associate.

 Apparently this is causing some necessary crypto algorithms to fail to
 load, breaking my wifi.

 Perhaps I'm displaying my ignorance of what's supposed to happen,
 but shouldn't make install have installed some files with names like
 /lib/modules/`uname r`/kernel/crypto/crypto-*.ko?

 No, the module names do not change. They just got another module alias
 with the crypto- prefix.

You can look at module aliases with modinfo, for example:

$ modinfo raid5
...
alias:  raid6
alias:  raid5
alias:  md-level-6
alias:  md-raid6
alias:  md-personality-8
...


 Or is it something only I'm hitting because I have a lot of common
 crypto algorithms statically compiled into my kernel?

 CONFIG_CRYPTO_CBC=y
 CONFIG_CRYPTO_HMAC=y
 CONFIG_CRYPTO_MD5=y
 CONFIG_CRYPTO_SHA1=y
 CONFIG_CRYPTO_AES=y
 CONFIG_CRYPTO_AES_586=y
 CONFIG_CRYPTO_ARC4=y

 Trying this on kernel 4943ba16 produces instead an endless loop of:

 wlan1: SME: Trying to authenticate with aa:bb:cc:dd:ee:ff (SSID='FOO' 
 freq=24xx MHz)
 wlan1: Trying to associate with aa:bb:cc:dd:ee:ff (SSID='FOO' freq=24xx MHz)
 wlan1: Associated with aa:bb:cc:dd:ee:ff
 wlan1: WPA: Failed to set PTK to the driver (alg=3 keylen=16 
 bssid=aa:bb:cc:dd:ee:ff)
 wlan1: CTRL-EVENT-DISCONNECTED bssid=aa:bb:cc:dd:ee:ff reason=1


 The kernel logs are not particularly informative.

 They just go through the usual successful series, but end with

 wlan1: RxAssocResp from aa:bb:cc:dd:ee:ff (capab=0x431 status=0 aid=1)
 wlan1: associated
 wlan1: deauthenticating from 11:bb:cc:dd:ee:ff by local choice (Reason: 
 1=UNSPECIFIED)

 While successful connection ends before that last line.

 Commit 4943ba16bbc2 was incomplete and could have caused regressions
 like the above. Those should have been fixed with commits 4943ba16bbc2
 + 3e14dcf7cb80. However, those should be in v3.19-rc7 already, so I'm
 not much of a help here :(

Just so I can keep things straight, the commits, in order, were:

Crypto prefix added: 5d26a105b5a7
Template prefixing fixed: 4943ba16bbc2d
Arch-specific aliases fixed: 3e14dcf7cb80

And now my head-scratching: your bisect shows that v3.19-rc7 fails,
but that prior to 4943ba16bbc2, things work correctly? As in, when
_only_ 5d26a105b5a7 is in your tree things _work_?

I would expect 5d26a105b5a7 to be what breaks things, not
4943ba16bbc2d. I wonder if userspace is calling modprobe (via
wpa_supplicant) directly on old aliases that got removed? For example,
gcm.ko switched from rfc4106 to crypto-rfc4106.

Could you rename and instrument your /sbin/modprobe to do something like:

#!/bin/sh
echo $@  /root/modprobe.log
exec /sbin/modprobe.bin $@

Perhaps there's something being explicitly requested rather than
having it be kernel auto-loaded?

-Kees

-- 
Kees Cook
Chrome OS Security
--
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 1/6] crypto: add missing crypto module aliases

[Kees Cook]

On Sun, Jan 11, 2015 at 9:17 AM, Mathias Krause mini...@googlemail.com wrote:
 Commit 5d26a105b5a7 (crypto: prefix module autoloading with crypto-)
 changed the automatic module loading when requesting crypto algorithms
 to prefix all module requests with crypto-. This requires all crypto
 modules to have a crypto specific module alias even if their file name
 would otherwise match the requested crypto algorithm.

 Even though commit 5d26a105b5a7 added those aliases for a vast amount of
 modules, it was missing a few. Add the required MODULE_ALIAS_CRYPTO
 annotations to those files to make them get loaded automatically, again.
 This fixes, e.g., requesting 'ecb(blowfish-generic)', which used to work
 with kernels v3.18 and below.

 Also change MODULE_ALIAS() lines to MODULE_ALIAS_CRYPTO(). The former
 won't work for crypto modules any more.

 Fixes: 5d26a105b5a7 (crypto: prefix module autoloading with crypto-)
 Cc: Kees Cook keesc...@chromium.org
 Signed-off-by: Mathias Krause mini...@googlemail.com

Ah, perfect! Thanks for finding the missing ones!

Acked-by: Kees Cook keesc...@chromium.org

-Kees

 ---
  arch/powerpc/crypto/sha1.c   |1 +
  arch/x86/crypto/sha-mb/sha1_mb.c |2 +-
  crypto/aes_generic.c |1 +
  crypto/ansi_cprng.c  |1 +
  crypto/blowfish_generic.c|1 +
  crypto/camellia_generic.c|1 +
  crypto/cast5_generic.c   |1 +
  crypto/cast6_generic.c   |1 +
  crypto/crc32c_generic.c  |1 +
  crypto/crct10dif_generic.c   |1 +
  crypto/des_generic.c |7 ---
  crypto/ghash-generic.c   |1 +
  crypto/krng.c|1 +
  crypto/salsa20_generic.c |1 +
  crypto/serpent_generic.c |1 +
  crypto/sha1_generic.c|1 +
  crypto/sha256_generic.c  |2 ++
  crypto/sha512_generic.c  |2 ++
  crypto/tea.c |1 +
  crypto/tgr192.c  |1 +
  crypto/twofish_generic.c |1 +
  crypto/wp512.c   |1 +
  22 files changed, 27 insertions(+), 4 deletions(-)

 diff --git a/arch/powerpc/crypto/sha1.c b/arch/powerpc/crypto/sha1.c
 index d3feba5a275f..c154cebc1041 100644
 --- a/arch/powerpc/crypto/sha1.c
 +++ b/arch/powerpc/crypto/sha1.c
 @@ -154,4 +154,5 @@ module_exit(sha1_powerpc_mod_fini);
  MODULE_LICENSE(GPL);
  MODULE_DESCRIPTION(SHA1 Secure Hash Algorithm);

 +MODULE_ALIAS_CRYPTO(sha1);
  MODULE_ALIAS_CRYPTO(sha1-powerpc);
 diff --git a/arch/x86/crypto/sha-mb/sha1_mb.c 
 b/arch/x86/crypto/sha-mb/sha1_mb.c
 index a225a5ca1037..fd9f6b035b16 100644
 --- a/arch/x86/crypto/sha-mb/sha1_mb.c
 +++ b/arch/x86/crypto/sha-mb/sha1_mb.c
 @@ -931,4 +931,4 @@ module_exit(sha1_mb_mod_fini);
  MODULE_LICENSE(GPL);
  MODULE_DESCRIPTION(SHA1 Secure Hash Algorithm, multi buffer accelerated);

 -MODULE_ALIAS(sha1);
 +MODULE_ALIAS_CRYPTO(sha1);
 diff --git a/crypto/aes_generic.c b/crypto/aes_generic.c
 index 9b3c54c1cbe8..3dd101144a58 100644
 --- a/crypto/aes_generic.c
 +++ b/crypto/aes_generic.c
 @@ -1475,3 +1475,4 @@ module_exit(aes_fini);
  MODULE_DESCRIPTION(Rijndael (AES) Cipher Algorithm);
  MODULE_LICENSE(Dual BSD/GPL);
  MODULE_ALIAS_CRYPTO(aes);
 +MODULE_ALIAS_CRYPTO(aes-generic);
 diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
 index b4485a108389..6f5bebc9bf01 100644
 --- a/crypto/ansi_cprng.c
 +++ b/crypto/ansi_cprng.c
 @@ -477,3 +477,4 @@ MODULE_PARM_DESC(dbg, Boolean to enable debugging (0/1 
 == off/on));
  module_init(prng_mod_init);
  module_exit(prng_mod_fini);
  MODULE_ALIAS_CRYPTO(stdrng);
 +MODULE_ALIAS_CRYPTO(ansi_cprng);
 diff --git a/crypto/blowfish_generic.c b/crypto/blowfish_generic.c
 index 7bd71f02d0dd..87b392a77a93 100644
 --- a/crypto/blowfish_generic.c
 +++ b/crypto/blowfish_generic.c
 @@ -139,3 +139,4 @@ module_exit(blowfish_mod_fini);
  MODULE_LICENSE(GPL);
  MODULE_DESCRIPTION(Blowfish Cipher Algorithm);
  MODULE_ALIAS_CRYPTO(blowfish);
 +MODULE_ALIAS_CRYPTO(blowfish-generic);
 diff --git a/crypto/camellia_generic.c b/crypto/camellia_generic.c
 index 1b74c5a3e891..a02286bf319e 100644
 --- a/crypto/camellia_generic.c
 +++ b/crypto/camellia_generic.c
 @@ -1099,3 +1099,4 @@ module_exit(camellia_fini);
  MODULE_DESCRIPTION(Camellia Cipher Algorithm);
  MODULE_LICENSE(GPL);
  MODULE_ALIAS_CRYPTO(camellia);
 +MODULE_ALIAS_CRYPTO(camellia-generic);
 diff --git a/crypto/cast5_generic.c b/crypto/cast5_generic.c
 index 84c86db67ec7..df5c72629383 100644
 --- a/crypto/cast5_generic.c
 +++ b/crypto/cast5_generic.c
 @@ -550,3 +550,4 @@ module_exit(cast5_mod_fini);
  MODULE_LICENSE(GPL);
  MODULE_DESCRIPTION(Cast5 Cipher Algorithm);
  MODULE_ALIAS_CRYPTO(cast5);
 +MODULE_ALIAS_CRYPTO(cast5-generic);
 diff --git a/crypto/cast6_generic.c b/crypto/cast6_generic.c
 index f408f0bd8de2..058c8d755d03 100644
 --- a/crypto/cast6_generic.c
 +++ b/crypto/cast6_generic.c
 @@ -292,3 +292,4 @@ module_exit(cast6_mod_fini);
  MODULE_LICENSE(GPL

[PATCH] crypto: include crypto- module prefix in template

[Kees Cook]

This adds the module loading prefix crypto- to the template lookup
as well.

For example, attempting to load 'vfat(blowfish)' via AF_ALG now correctly
includes the crypto- prefix at every level, correctly rejecting vfat:

net-pf-38
algif-hash
crypto-vfat(blowfish)
crypto-vfat(blowfish)-all
crypto-vfat

Reported-by: Mathias Krause mini...@googlemail.com
Signed-off-by: Kees Cook keesc...@chromium.org
---
 crypto/algapi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crypto/algapi.c b/crypto/algapi.c
index e8d3a7dca8c4..71a8143e23b1 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -509,8 +509,8 @@ static struct crypto_template 
*__crypto_lookup_template(const char *name)
 
 struct crypto_template *crypto_lookup_template(const char *name)
 {
-   return try_then_request_module(__crypto_lookup_template(name), %s,
-  name);
+   return try_then_request_module(__crypto_lookup_template(name),
+  crypto-%s, name);
 }
 EXPORT_SYMBOL_GPL(crypto_lookup_template);
 
-- 
1.9.1


-- 
Kees Cook
Chrome OS Security
--
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 v2] crypto: include crypto- module prefix in template

[Kees Cook]

This adds the module loading prefix crypto- to the template lookup
as well.

For example, attempting to load 'vfat(blowfish)' via AF_ALG now correctly
includes the crypto- prefix at every level, correctly rejecting vfat:

net-pf-38
algif-hash
crypto-vfat(blowfish)
crypto-vfat(blowfish)-all
crypto-vfat

Reported-by: Mathias Krause mini...@googlemail.com
Signed-off-by: Kees Cook keesc...@chromium.org
---
 crypto/algapi.c | 4 ++--
 crypto/authenc.c| 1 +
 crypto/authencesn.c | 1 +
 crypto/cbc.c| 1 +
 crypto/chainiv.c| 1 +
 crypto/cmac.c   | 1 +
 crypto/cts.c| 1 +
 crypto/ecb.c| 1 +
 crypto/eseqiv.c | 1 +
 crypto/hmac.c   | 1 +
 crypto/lrw.c| 1 +
 crypto/pcbc.c   | 1 +
 crypto/seqiv.c  | 1 +
 crypto/vmac.c   | 1 +
 crypto/xcbc.c   | 1 +
 crypto/xts.c| 1 +
 16 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/crypto/algapi.c b/crypto/algapi.c
index e8d3a7dca8c4..71a8143e23b1 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -509,8 +509,8 @@ static struct crypto_template 
*__crypto_lookup_template(const char *name)
 
 struct crypto_template *crypto_lookup_template(const char *name)
 {
-   return try_then_request_module(__crypto_lookup_template(name), %s,
-  name);
+   return try_then_request_module(__crypto_lookup_template(name),
+  crypto-%s, name);
 }
 EXPORT_SYMBOL_GPL(crypto_lookup_template);
 
diff --git a/crypto/authenc.c b/crypto/authenc.c
index e1223559d5df..78fb16cab13f 100644
--- a/crypto/authenc.c
+++ b/crypto/authenc.c
@@ -721,3 +721,4 @@ module_exit(crypto_authenc_module_exit);
 
 MODULE_LICENSE(GPL);
 MODULE_DESCRIPTION(Simple AEAD wrapper for IPsec);
+MODULE_ALIAS_CRYPTO(authenc);
diff --git a/crypto/authencesn.c b/crypto/authencesn.c
index 4be0dd4373a9..024bff2344fc 100644
--- a/crypto/authencesn.c
+++ b/crypto/authencesn.c
@@ -814,3 +814,4 @@ module_exit(crypto_authenc_esn_module_exit);
 MODULE_LICENSE(GPL);
 MODULE_AUTHOR(Steffen Klassert steffen.klass...@secunet.com);
 MODULE_DESCRIPTION(AEAD wrapper for IPsec with extended sequence numbers);
+MODULE_ALIAS_CRYPTO(authencesn);
diff --git a/crypto/cbc.c b/crypto/cbc.c
index 61ac42e1e32b..780ee27b2d43 100644
--- a/crypto/cbc.c
+++ b/crypto/cbc.c
@@ -289,3 +289,4 @@ module_exit(crypto_cbc_module_exit);
 
 MODULE_LICENSE(GPL);
 MODULE_DESCRIPTION(CBC block cipher algorithm);
+MODULE_ALIAS_CRYPTO(cbc);
diff --git a/crypto/chainiv.c b/crypto/chainiv.c
index 9c294c8f9a07..63c17d5992f7 100644
--- a/crypto/chainiv.c
+++ b/crypto/chainiv.c
@@ -359,3 +359,4 @@ module_exit(chainiv_module_exit);
 
 MODULE_LICENSE(GPL);
 MODULE_DESCRIPTION(Chain IV Generator);
+MODULE_ALIAS_CRYPTO(chainiv);
diff --git a/crypto/cmac.c b/crypto/cmac.c
index 50880cf17fad..7a8bfbd548f6 100644
--- a/crypto/cmac.c
+++ b/crypto/cmac.c
@@ -313,3 +313,4 @@ module_exit(crypto_cmac_module_exit);
 
 MODULE_LICENSE(GPL);
 MODULE_DESCRIPTION(CMAC keyed hash algorithm);
+MODULE_ALIAS_CRYPTO(cmac);
diff --git a/crypto/cts.c b/crypto/cts.c
index 133f0874c95e..bd9405820e8a 100644
--- a/crypto/cts.c
+++ b/crypto/cts.c
@@ -351,3 +351,4 @@ module_exit(crypto_cts_module_exit);
 
 MODULE_LICENSE(Dual BSD/GPL);
 MODULE_DESCRIPTION(CTS-CBC CipherText Stealing for CBC);
+MODULE_ALIAS_CRYPTO(cts);
diff --git a/crypto/ecb.c b/crypto/ecb.c
index 935cfef4aa84..12011aff0971 100644
--- a/crypto/ecb.c
+++ b/crypto/ecb.c
@@ -185,3 +185,4 @@ module_exit(crypto_ecb_module_exit);
 
 MODULE_LICENSE(GPL);
 MODULE_DESCRIPTION(ECB block cipher algorithm);
+MODULE_ALIAS_CRYPTO(ecb);
diff --git a/crypto/eseqiv.c b/crypto/eseqiv.c
index bf7ab4a89493..f116fae766f8 100644
--- a/crypto/eseqiv.c
+++ b/crypto/eseqiv.c
@@ -267,3 +267,4 @@ module_exit(eseqiv_module_exit);
 
 MODULE_LICENSE(GPL);
 MODULE_DESCRIPTION(Encrypted Sequence Number IV Generator);
+MODULE_ALIAS_CRYPTO(eseqiv);
diff --git a/crypto/hmac.c b/crypto/hmac.c
index e392219ddc61..72e38c098bb3 100644
--- a/crypto/hmac.c
+++ b/crypto/hmac.c
@@ -268,3 +268,4 @@ module_exit(hmac_module_exit);
 
 MODULE_LICENSE(GPL);
 MODULE_DESCRIPTION(HMAC hash algorithm);
+MODULE_ALIAS_CRYPTO(hmac);
diff --git a/crypto/lrw.c b/crypto/lrw.c
index ba42acc4deba..6f9908a7ebcb 100644
--- a/crypto/lrw.c
+++ b/crypto/lrw.c
@@ -400,3 +400,4 @@ module_exit(crypto_module_exit);
 
 MODULE_LICENSE(GPL);
 MODULE_DESCRIPTION(LRW block cipher mode);
+MODULE_ALIAS_CRYPTO(lrw);
diff --git a/crypto/pcbc.c b/crypto/pcbc.c
index d1b8bdfb5855..f654965f0933 100644
--- a/crypto/pcbc.c
+++ b/crypto/pcbc.c
@@ -295,3 +295,4 @@ module_exit(crypto_pcbc_module_exit);
 
 MODULE_LICENSE(GPL);
 MODULE_DESCRIPTION(PCBC block cipher algorithm);
+MODULE_ALIAS_CRYPTO(pcbc);
diff --git a/crypto/seqiv.c b/crypto/seqiv.c
index ee190fcedcd2..9daa854cc485 100644
--- a/crypto/seqiv.c
+++ b/crypto/seqiv.c
@@ -362,3 +362,4 @@ module_exit(seqiv_module_exit);
 
 MODULE_LICENSE(GPL);
 MODULE_DESCRIPTION

[PATCH v3] crypto: include crypto- module prefix in template

[Kees Cook]

This adds the module loading prefix crypto- to the template lookup
as well.

For example, attempting to load 'vfat(blowfish)' via AF_ALG now correctly
includes the crypto- prefix at every level, correctly rejecting vfat:

net-pf-38
algif-hash
crypto-vfat(blowfish)
crypto-vfat(blowfish)-all
crypto-vfat

Reported-by: Mathias Krause mini...@googlemail.com
Signed-off-by: Kees Cook keesc...@chromium.org
Acked-by: Mathias Krause mini...@googlemail.com
---
v3:
 - added a few more missing aliases, minipli
v2:
 - added missing aliases, minipli
---
 arch/x86/crypto/fpu.c | 3 +++
 crypto/algapi.c   | 4 ++--
 crypto/authenc.c  | 1 +
 crypto/authencesn.c   | 1 +
 crypto/cbc.c  | 1 +
 crypto/ccm.c  | 1 +
 crypto/chainiv.c  | 1 +
 crypto/cmac.c | 1 +
 crypto/cryptd.c   | 1 +
 crypto/ctr.c  | 1 +
 crypto/cts.c  | 1 +
 crypto/ecb.c  | 1 +
 crypto/eseqiv.c   | 1 +
 crypto/gcm.c  | 1 +
 crypto/hmac.c | 1 +
 crypto/lrw.c  | 1 +
 crypto/mcryptd.c  | 1 +
 crypto/pcbc.c | 1 +
 crypto/pcrypt.c   | 1 +
 crypto/seqiv.c| 1 +
 crypto/vmac.c | 1 +
 crypto/xcbc.c | 1 +
 crypto/xts.c  | 1 +
 23 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/arch/x86/crypto/fpu.c b/arch/x86/crypto/fpu.c
index 98d7a188f46b..f368ba261739 100644
--- a/arch/x86/crypto/fpu.c
+++ b/arch/x86/crypto/fpu.c
@@ -17,6 +17,7 @@
 #include linux/kernel.h
 #include linux/module.h
 #include linux/slab.h
+#include linux/crypto.h
 #include asm/i387.h
 
 struct crypto_fpu_ctx {
@@ -159,3 +160,5 @@ void __exit crypto_fpu_exit(void)
 {
crypto_unregister_template(crypto_fpu_tmpl);
 }
+
+MODULE_ALIAS_CRYPTO(fpu);
diff --git a/crypto/algapi.c b/crypto/algapi.c
index e8d3a7dca8c4..71a8143e23b1 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -509,8 +509,8 @@ static struct crypto_template 
*__crypto_lookup_template(const char *name)
 
 struct crypto_template *crypto_lookup_template(const char *name)
 {
-   return try_then_request_module(__crypto_lookup_template(name), %s,
-  name);
+   return try_then_request_module(__crypto_lookup_template(name),
+  crypto-%s, name);
 }
 EXPORT_SYMBOL_GPL(crypto_lookup_template);
 
diff --git a/crypto/authenc.c b/crypto/authenc.c
index e1223559d5df..78fb16cab13f 100644
--- a/crypto/authenc.c
+++ b/crypto/authenc.c
@@ -721,3 +721,4 @@ module_exit(crypto_authenc_module_exit);
 
 MODULE_LICENSE(GPL);
 MODULE_DESCRIPTION(Simple AEAD wrapper for IPsec);
+MODULE_ALIAS_CRYPTO(authenc);
diff --git a/crypto/authencesn.c b/crypto/authencesn.c
index 4be0dd4373a9..024bff2344fc 100644
--- a/crypto/authencesn.c
+++ b/crypto/authencesn.c
@@ -814,3 +814,4 @@ module_exit(crypto_authenc_esn_module_exit);
 MODULE_LICENSE(GPL);
 MODULE_AUTHOR(Steffen Klassert steffen.klass...@secunet.com);
 MODULE_DESCRIPTION(AEAD wrapper for IPsec with extended sequence numbers);
+MODULE_ALIAS_CRYPTO(authencesn);
diff --git a/crypto/cbc.c b/crypto/cbc.c
index 61ac42e1e32b..780ee27b2d43 100644
--- a/crypto/cbc.c
+++ b/crypto/cbc.c
@@ -289,3 +289,4 @@ module_exit(crypto_cbc_module_exit);
 
 MODULE_LICENSE(GPL);
 MODULE_DESCRIPTION(CBC block cipher algorithm);
+MODULE_ALIAS_CRYPTO(cbc);
diff --git a/crypto/ccm.c b/crypto/ccm.c
index 647575b41281..003bbbd21a2b 100644
--- a/crypto/ccm.c
+++ b/crypto/ccm.c
@@ -881,3 +881,4 @@ MODULE_LICENSE(GPL);
 MODULE_DESCRIPTION(Counter with CBC MAC);
 MODULE_ALIAS_CRYPTO(ccm_base);
 MODULE_ALIAS_CRYPTO(rfc4309);
+MODULE_ALIAS_CRYPTO(ccm);
diff --git a/crypto/chainiv.c b/crypto/chainiv.c
index 9c294c8f9a07..63c17d5992f7 100644
--- a/crypto/chainiv.c
+++ b/crypto/chainiv.c
@@ -359,3 +359,4 @@ module_exit(chainiv_module_exit);
 
 MODULE_LICENSE(GPL);
 MODULE_DESCRIPTION(Chain IV Generator);
+MODULE_ALIAS_CRYPTO(chainiv);
diff --git a/crypto/cmac.c b/crypto/cmac.c
index 50880cf17fad..7a8bfbd548f6 100644
--- a/crypto/cmac.c
+++ b/crypto/cmac.c
@@ -313,3 +313,4 @@ module_exit(crypto_cmac_module_exit);
 
 MODULE_LICENSE(GPL);
 MODULE_DESCRIPTION(CMAC keyed hash algorithm);
+MODULE_ALIAS_CRYPTO(cmac);
diff --git a/crypto/cryptd.c b/crypto/cryptd.c
index e592c90abebb..650afac10fd7 100644
--- a/crypto/cryptd.c
+++ b/crypto/cryptd.c
@@ -955,3 +955,4 @@ module_exit(cryptd_exit);
 
 MODULE_LICENSE(GPL);
 MODULE_DESCRIPTION(Software async crypto daemon);
+MODULE_ALIAS_CRYPTO(cryptd);
diff --git a/crypto/ctr.c b/crypto/ctr.c
index 3d81ff7e6b48..2386f7313952 100644
--- a/crypto/ctr.c
+++ b/crypto/ctr.c
@@ -467,3 +467,4 @@ module_exit(crypto_ctr_module_exit);
 MODULE_LICENSE(GPL);
 MODULE_DESCRIPTION(CTR Counter block mode);
 MODULE_ALIAS_CRYPTO(rfc3686);
+MODULE_ALIAS_CRYPTO(ctr);
diff --git a/crypto/cts.c b/crypto/cts.c
index 133f0874c95e..bd9405820e8a 100644
--- a/crypto/cts.c
+++ b/crypto/cts.c
@@ -351,3 +351,4 @@ module_exit(crypto_cts_module_exit);
 
 MODULE_LICENSE(Dual BSD

[PATCH v3] crypto: prefix module autoloading with crypto-

[Kees Cook]

This prefixes all crypto module loading with crypto- so we never run
the risk of exposing module auto-loading to userspace via a crypto API,
as demonstrated by Mathias Krause:

https://lkml.org/lkml/2013/3/4/70

Signed-off-by: Kees Cook keesc...@chromium.org
---
v3:
 - added unprefixed alias back for userspace compat, thanks to minipli
v2:
 - added missing #include, thanks to minipli
 - built with allmodconfig
---
 arch/arm/crypto/aes_glue.c  |  4 ++--
 arch/arm/crypto/sha1_glue.c |  2 +-
 arch/arm/crypto/sha1_neon_glue.c|  2 +-
 arch/arm/crypto/sha512_neon_glue.c  |  4 ++--
 arch/arm64/crypto/aes-ce-ccm-glue.c |  2 +-
 arch/arm64/crypto/aes-glue.c|  8 
 arch/powerpc/crypto/sha1.c  |  2 +-
 arch/s390/crypto/aes_s390.c |  2 +-
 arch/s390/crypto/des_s390.c |  4 ++--
 arch/s390/crypto/ghash_s390.c   |  2 +-
 arch/s390/crypto/sha1_s390.c|  2 +-
 arch/s390/crypto/sha256_s390.c  |  4 ++--
 arch/s390/crypto/sha512_s390.c  |  4 ++--
 arch/sparc/crypto/aes_glue.c|  2 +-
 arch/sparc/crypto/camellia_glue.c   |  2 +-
 arch/sparc/crypto/crc32c_glue.c |  2 +-
 arch/sparc/crypto/des_glue.c|  2 +-
 arch/sparc/crypto/md5_glue.c|  2 +-
 arch/sparc/crypto/sha1_glue.c   |  2 +-
 arch/sparc/crypto/sha256_glue.c |  4 ++--
 arch/sparc/crypto/sha512_glue.c |  4 ++--
 arch/x86/crypto/aes_glue.c  |  4 ++--
 arch/x86/crypto/aesni-intel_glue.c  |  2 +-
 arch/x86/crypto/blowfish_glue.c |  4 ++--
 arch/x86/crypto/camellia_aesni_avx2_glue.c  |  4 ++--
 arch/x86/crypto/camellia_aesni_avx_glue.c   |  4 ++--
 arch/x86/crypto/camellia_glue.c |  4 ++--
 arch/x86/crypto/cast5_avx_glue.c|  2 +-
 arch/x86/crypto/cast6_avx_glue.c|  2 +-
 arch/x86/crypto/crc32-pclmul_glue.c |  4 ++--
 arch/x86/crypto/crc32c-intel_glue.c |  4 ++--
 arch/x86/crypto/crct10dif-pclmul_glue.c |  4 ++--
 arch/x86/crypto/des3_ede_glue.c |  8 
 arch/x86/crypto/ghash-clmulni-intel_glue.c  |  2 +-
 arch/x86/crypto/salsa20_glue.c  |  4 ++--
 arch/x86/crypto/serpent_avx2_glue.c |  4 ++--
 arch/x86/crypto/serpent_avx_glue.c  |  2 +-
 arch/x86/crypto/serpent_sse2_glue.c |  2 +-
 arch/x86/crypto/sha1_ssse3_glue.c   |  2 +-
 arch/x86/crypto/sha256_ssse3_glue.c |  4 ++--
 arch/x86/crypto/sha512_ssse3_glue.c |  4 ++--
 arch/x86/crypto/twofish_avx_glue.c  |  2 +-
 arch/x86/crypto/twofish_glue.c  |  4 ++--
 arch/x86/crypto/twofish_glue_3way.c |  4 ++--
 crypto/842.c|  1 +
 crypto/aes_generic.c|  2 +-
 crypto/ansi_cprng.c |  2 +-
 crypto/anubis.c |  1 +
 crypto/api.c|  4 ++--
 crypto/arc4.c   |  1 +
 crypto/blowfish_generic.c   |  2 +-
 crypto/camellia_generic.c   |  2 +-
 crypto/cast5_generic.c  |  2 +-
 crypto/cast6_generic.c  |  2 +-
 crypto/ccm.c|  4 ++--
 crypto/crc32.c  |  1 +
 crypto/crc32c_generic.c |  2 +-
 crypto/crct10dif_generic.c  |  2 +-
 crypto/crypto_null.c|  6 +++---
 crypto/ctr.c|  2 +-
 crypto/deflate.c|  2 +-
 crypto/des_generic.c|  2 +-
 crypto/fcrypt.c |  1 +
 crypto/gcm.c|  6 +++---
 crypto/ghash-generic.c  |  2 +-
 crypto/khazad.c |  1 +
 crypto/krng.c   |  2 +-
 crypto/lz4.c|  1 +
 crypto/lz4hc.c  |  1 +
 crypto/lzo.c|  1 +
 crypto/md4.c|  2 +-
 crypto/md5.c|  1 +
 crypto/michael_mic.c|  1 +
 crypto/rmd128.c |  1 +
 crypto/rmd160.c |  1 +
 crypto/rmd256.c |  1 +
 crypto/rmd320.c |  1 +
 crypto/salsa20_generic.c|  2 +-
 crypto/seed.c   |  1 +
 crypto/serpent_generic.c|  4 ++--
 crypto/sha1_generic.c   |  2 +-
 crypto/sha256_generic.c |  4 ++--
 crypto/sha512_generic.c |  4 ++--
 crypto/tea.c|  4 ++--
 crypto/tgr192.c |  4 ++--
 crypto

Re: [PATCH v2] crypto: prefix module autoloading with crypto-

[Kees Cook]

On Tue, Nov 18, 2014 at 3:12 PM, Mathias Krause mini...@googlemail.com wrote:
 On 18 November 2014 01:45, Kees Cook keesc...@chromium.org wrote:
 On Mon, Nov 17, 2014 at 3:20 PM, Mathias Krause mini...@googlemail.com 
 wrote:
 On 17 November 2014 21:02, Kees Cook keesc...@chromium.org wrote:
 This prefixes all crypto module loading with crypto- so we never run
 the risk of exposing module auto-loading to userspace via a crypto API,
 as demonstrated by Mathias Krause:

 https://lkml.org/lkml/2013/3/4/70

 Signed-off-by: Kees Cook keesc...@chromium.org
 ---
 v2:
  - added missing #include, thanks to minipli
  - built with allmodconfig
 [...snip...]
 diff --git a/include/linux/crypto.h b/include/linux/crypto.h
 index d45e949699ea..d14230f6e977 100644
 --- a/include/linux/crypto.h
 +++ b/include/linux/crypto.h
 @@ -26,6 +26,13 @@
  #include linux/uaccess.h

  /*
 + * Autoloaded crypto modules should only use a prefixed name to avoid 
 allowing
 + * arbitrary modules to be loaded.
 + */
 +#define MODULE_ALIAS_CRYPTO(name)  \
 +   MODULE_ALIAS(crypto- name)

 This would break userland relying on the old aliases, e.g. 'modprobe
 aes' no longer works.

 Why not have both aliases, one with the crypto- prefix for on-demand
 loading within the crypto API and one without for manual loading from
 userland? E.g., something like this:

 #define MODULE_ALIAS_CRYPTO(name)  \
MODULE_ALIAS(name); \
MODULE_ALIAS(crypto- name)

 That would prevent the userland breakage and still achieves the goal
 of restricting the request_module() call offered by the means of the
 AF_ALG API.

 That was my intention originally, and I should go back to it. The
 trouble is with the use of __UNIQUE_ID in the MODULE_ALIAS macro. It
 uses __LINE__ to produce the id, so the suggested macro expansion
 (which is what I started with) won't work on non-gcc compilers.

 I haven't found any solutions for C89 version of gcc's __COUNTER__,
 and I haven't found any C89 ways to force a macro to be expanded as
 being multi-line.

 Well, clang should support it as well, according to [1]. But still, a
 compiler independent solution would be nice.
 Anyway, the __COUNTER__ support is gcc = 4.3 only. So, according to
 Documentation/Changes, stating gcc 3.2 is the minimum supported
 version for compiling the kernel, this would be a no-go, too.

 [1] http://clang.llvm.org/docs/LanguageExtensions.html#builtin-macros

Yeah, it's the avr32 auto builder that throws the warnings, so I
assume it's using a pre-4.3 gcc.

 I'd like to avoid having to open-code both MODULE_ALIAS and
 MODULE_ALIAS_CRYPTO in each module's source.

 Anyone see some sneaky way to accomplish this?

 Unfortunately, I do not ... beside this, maybe:

 #define MODULE_ALIAS_CRYPTO(name)  \
__MODULE_INFO(alias, alias_userland, name); \
__MODULE_INFO(alias, alias_crypto, crypto- name)

 Looks ugly, but works. ;)

AAh! Yes, that's perfect! I will spin it this way. Thank you thank you!

-Kees

-- 
Kees Cook
Chrome OS Security
--
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: prefix module autoloading with crypto-

[Kees Cook]

On Mon, Nov 17, 2014 at 10:38 AM, Mathias Krause mini...@googlemail.com wrote:
 On 17 November 2014 16:09, Herbert Xu herb...@gondor.apana.org.au wrote:
 On Fri, Nov 14, 2014 at 01:34:59PM -0800, Kees Cook wrote:
 This prefixes all crypto module loading with crypto- so we never run
 the risk of exposing module auto-loading to userspace via a crypto API,
 as demonstrated by Mathias Krause:

 https://lkml.org/lkml/2013/3/4/70

 Signed-off-by: Kees Cook keesc...@chromium.org

 Sorry but this doesn't build for me:

   CC [M]  drivers/crypto/qat/qat_common/adf_ctl_drv.o
 drivers/crypto/qat/qat_common/adf_ctl_drv.c:490:21: error: expected 
 declaration specifiers or ‘...’ before string constant
 make[3]: *** [drivers/crypto/qat/qat_common/adf_ctl_drv.o] Error 1
 make[2]: *** [drivers/crypto/qat/qat_common] Error 2
 make[1]: *** [drivers/crypto/qat] Error 2
 make[1]: *** Waiting for unfinished jobs

 Looks like drivers/crypto/qat/qat_common/adf_ctl_drv.c is missing
 '#include linux/crypto.h'.

Ah, looks like I missed a few in my config. I'll attempt allmodconfig
and send a v2.

Thanks!

-Kees

-- 
Kees Cook
Chrome OS Security
--
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 v2] crypto: prefix module autoloading with crypto-

[Kees Cook]

This prefixes all crypto module loading with crypto- so we never run
the risk of exposing module auto-loading to userspace via a crypto API,
as demonstrated by Mathias Krause:

https://lkml.org/lkml/2013/3/4/70

Signed-off-by: Kees Cook keesc...@chromium.org
---
v2:
 - added missing #include, thanks to minipli
 - built with allmodconfig
---
 arch/arm/crypto/aes_glue.c  | 4 ++--
 arch/arm/crypto/sha1_glue.c | 2 +-
 arch/arm/crypto/sha1_neon_glue.c| 2 +-
 arch/arm/crypto/sha512_neon_glue.c  | 4 ++--
 arch/arm64/crypto/aes-ce-ccm-glue.c | 2 +-
 arch/arm64/crypto/aes-glue.c| 8 
 arch/powerpc/crypto/sha1.c  | 2 +-
 arch/s390/crypto/aes_s390.c | 2 +-
 arch/s390/crypto/des_s390.c | 4 ++--
 arch/s390/crypto/ghash_s390.c   | 2 +-
 arch/s390/crypto/sha1_s390.c| 2 +-
 arch/s390/crypto/sha256_s390.c  | 4 ++--
 arch/s390/crypto/sha512_s390.c  | 4 ++--
 arch/sparc/crypto/aes_glue.c| 2 +-
 arch/sparc/crypto/camellia_glue.c   | 2 +-
 arch/sparc/crypto/crc32c_glue.c | 2 +-
 arch/sparc/crypto/des_glue.c| 2 +-
 arch/sparc/crypto/md5_glue.c| 2 +-
 arch/sparc/crypto/sha1_glue.c   | 2 +-
 arch/sparc/crypto/sha256_glue.c | 4 ++--
 arch/sparc/crypto/sha512_glue.c | 4 ++--
 arch/x86/crypto/aes_glue.c  | 4 ++--
 arch/x86/crypto/aesni-intel_glue.c  | 2 +-
 arch/x86/crypto/blowfish_glue.c | 4 ++--
 arch/x86/crypto/camellia_aesni_avx2_glue.c  | 4 ++--
 arch/x86/crypto/camellia_aesni_avx_glue.c   | 4 ++--
 arch/x86/crypto/camellia_glue.c | 4 ++--
 arch/x86/crypto/cast5_avx_glue.c| 2 +-
 arch/x86/crypto/cast6_avx_glue.c| 2 +-
 arch/x86/crypto/crc32-pclmul_glue.c | 4 ++--
 arch/x86/crypto/crc32c-intel_glue.c | 4 ++--
 arch/x86/crypto/crct10dif-pclmul_glue.c | 4 ++--
 arch/x86/crypto/des3_ede_glue.c | 8 
 arch/x86/crypto/ghash-clmulni-intel_glue.c  | 2 +-
 arch/x86/crypto/salsa20_glue.c  | 4 ++--
 arch/x86/crypto/serpent_avx2_glue.c | 4 ++--
 arch/x86/crypto/serpent_avx_glue.c  | 2 +-
 arch/x86/crypto/serpent_sse2_glue.c | 2 +-
 arch/x86/crypto/sha1_ssse3_glue.c   | 2 +-
 arch/x86/crypto/sha256_ssse3_glue.c | 4 ++--
 arch/x86/crypto/sha512_ssse3_glue.c | 4 ++--
 arch/x86/crypto/twofish_avx_glue.c  | 2 +-
 arch/x86/crypto/twofish_glue.c  | 4 ++--
 arch/x86/crypto/twofish_glue_3way.c | 4 ++--
 crypto/842.c| 1 +
 crypto/aes_generic.c| 2 +-
 crypto/ansi_cprng.c | 2 +-
 crypto/anubis.c | 1 +
 crypto/api.c| 4 ++--
 crypto/arc4.c   | 1 +
 crypto/blowfish_generic.c   | 2 +-
 crypto/camellia_generic.c   | 2 +-
 crypto/cast5_generic.c  | 2 +-
 crypto/cast6_generic.c  | 2 +-
 crypto/ccm.c| 4 ++--
 crypto/crc32.c  | 1 +
 crypto/crc32c_generic.c | 2 +-
 crypto/crct10dif_generic.c  | 2 +-
 crypto/crypto_null.c| 6 +++---
 crypto/ctr.c| 2 +-
 crypto/deflate.c| 2 +-
 crypto/des_generic.c| 2 +-
 crypto/fcrypt.c | 1 +
 crypto/gcm.c| 6 +++---
 crypto/ghash-generic.c  | 2 +-
 crypto/khazad.c | 1 +
 crypto/krng.c   | 2 +-
 crypto/lz4.c| 1 +
 crypto/lz4hc.c  | 1 +
 crypto/lzo.c| 1 +
 crypto/md4.c| 2 +-
 crypto/md5.c| 1 +
 crypto/michael_mic.c| 1 +
 crypto/rmd128.c | 1 +
 crypto/rmd160.c | 1 +
 crypto/rmd256.c | 1 +
 crypto/rmd320.c | 1 +
 crypto/salsa20_generic.c| 2 +-
 crypto/seed.c   | 1 +
 crypto/serpent_generic.c| 4 ++--
 crypto/sha1_generic.c   | 2 +-
 crypto/sha256_generic.c | 4 ++--
 crypto/sha512_generic.c | 4 ++--
 crypto/tea.c| 4 ++--
 crypto/tgr192.c | 4 ++--
 crypto/twofish_generic.c| 2 +-
 crypto/wp512.c  | 4 ++--
 crypto/zlib.c   | 1 +
 drivers/crypto

Re: [PATCH v2] crypto: prefix module autoloading with crypto-

[Kees Cook]

On Mon, Nov 17, 2014 at 3:20 PM, Mathias Krause mini...@googlemail.com wrote:
 On 17 November 2014 21:02, Kees Cook keesc...@chromium.org wrote:
 This prefixes all crypto module loading with crypto- so we never run
 the risk of exposing module auto-loading to userspace via a crypto API,
 as demonstrated by Mathias Krause:

 https://lkml.org/lkml/2013/3/4/70

 Signed-off-by: Kees Cook keesc...@chromium.org
 ---
 v2:
  - added missing #include, thanks to minipli
  - built with allmodconfig
 [...snip...]
 diff --git a/include/linux/crypto.h b/include/linux/crypto.h
 index d45e949699ea..d14230f6e977 100644
 --- a/include/linux/crypto.h
 +++ b/include/linux/crypto.h
 @@ -26,6 +26,13 @@
  #include linux/uaccess.h

  /*
 + * Autoloaded crypto modules should only use a prefixed name to avoid 
 allowing
 + * arbitrary modules to be loaded.
 + */
 +#define MODULE_ALIAS_CRYPTO(name)  \
 +   MODULE_ALIAS(crypto- name)

 This would break userland relying on the old aliases, e.g. 'modprobe
 aes' no longer works.

 Why not have both aliases, one with the crypto- prefix for on-demand
 loading within the crypto API and one without for manual loading from
 userland? E.g., something like this:

 #define MODULE_ALIAS_CRYPTO(name)  \
MODULE_ALIAS(name); \
MODULE_ALIAS(crypto- name)

 That would prevent the userland breakage and still achieves the goal
 of restricting the request_module() call offered by the means of the
 AF_ALG API.

That was my intention originally, and I should go back to it. The
trouble is with the use of __UNIQUE_ID in the MODULE_ALIAS macro. It
uses __LINE__ to produce the id, so the suggested macro expansion
(which is what I started with) won't work on non-gcc compilers.

I haven't found any solutions for C89 version of gcc's __COUNTER__,
and I haven't found any C89 ways to force a macro to be expanded as
being multi-line.

I'd like to avoid having to open-code both MODULE_ALIAS and
MODULE_ALIAS_CRYPTO in each module's source.

Anyone see some sneaky way to accomplish this?

-Kees

-- 
Kees Cook
Chrome OS Security
--
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] crypto: prefix module autoloading with crypto-

[Kees Cook]

This prefixes all crypto module loading with crypto- so we never run
the risk of exposing module auto-loading to userspace via a crypto API,
as demonstrated by Mathias Krause:

https://lkml.org/lkml/2013/3/4/70

Signed-off-by: Kees Cook keesc...@chromium.org
---
 arch/arm/crypto/aes_glue.c  | 4 ++--
 arch/arm/crypto/sha1_glue.c | 2 +-
 arch/arm/crypto/sha1_neon_glue.c| 2 +-
 arch/arm/crypto/sha512_neon_glue.c  | 4 ++--
 arch/arm64/crypto/aes-ce-ccm-glue.c | 2 +-
 arch/arm64/crypto/aes-glue.c| 8 
 arch/powerpc/crypto/sha1.c  | 2 +-
 arch/s390/crypto/aes_s390.c | 2 +-
 arch/s390/crypto/des_s390.c | 4 ++--
 arch/s390/crypto/ghash_s390.c   | 2 +-
 arch/s390/crypto/sha1_s390.c| 2 +-
 arch/s390/crypto/sha256_s390.c  | 4 ++--
 arch/s390/crypto/sha512_s390.c  | 4 ++--
 arch/sparc/crypto/aes_glue.c| 2 +-
 arch/sparc/crypto/camellia_glue.c   | 2 +-
 arch/sparc/crypto/crc32c_glue.c | 2 +-
 arch/sparc/crypto/des_glue.c| 2 +-
 arch/sparc/crypto/md5_glue.c| 2 +-
 arch/sparc/crypto/sha1_glue.c   | 2 +-
 arch/sparc/crypto/sha256_glue.c | 4 ++--
 arch/sparc/crypto/sha512_glue.c | 4 ++--
 arch/x86/crypto/aes_glue.c  | 4 ++--
 arch/x86/crypto/aesni-intel_glue.c  | 2 +-
 arch/x86/crypto/blowfish_glue.c | 4 ++--
 arch/x86/crypto/camellia_aesni_avx2_glue.c  | 4 ++--
 arch/x86/crypto/camellia_aesni_avx_glue.c   | 4 ++--
 arch/x86/crypto/camellia_glue.c | 4 ++--
 arch/x86/crypto/cast5_avx_glue.c| 2 +-
 arch/x86/crypto/cast6_avx_glue.c| 2 +-
 arch/x86/crypto/crc32-pclmul_glue.c | 4 ++--
 arch/x86/crypto/crc32c-intel_glue.c | 4 ++--
 arch/x86/crypto/crct10dif-pclmul_glue.c | 4 ++--
 arch/x86/crypto/des3_ede_glue.c | 8 
 arch/x86/crypto/ghash-clmulni-intel_glue.c  | 2 +-
 arch/x86/crypto/salsa20_glue.c  | 4 ++--
 arch/x86/crypto/serpent_avx2_glue.c | 4 ++--
 arch/x86/crypto/serpent_avx_glue.c  | 2 +-
 arch/x86/crypto/serpent_sse2_glue.c | 2 +-
 arch/x86/crypto/sha1_ssse3_glue.c   | 2 +-
 arch/x86/crypto/sha256_ssse3_glue.c | 4 ++--
 arch/x86/crypto/sha512_ssse3_glue.c | 4 ++--
 arch/x86/crypto/twofish_avx_glue.c  | 2 +-
 arch/x86/crypto/twofish_glue.c  | 4 ++--
 arch/x86/crypto/twofish_glue_3way.c | 4 ++--
 crypto/842.c| 1 +
 crypto/aes_generic.c| 2 +-
 crypto/ansi_cprng.c | 2 +-
 crypto/anubis.c | 1 +
 crypto/api.c| 4 ++--
 crypto/arc4.c   | 1 +
 crypto/blowfish_generic.c   | 2 +-
 crypto/camellia_generic.c   | 2 +-
 crypto/cast5_generic.c  | 2 +-
 crypto/cast6_generic.c  | 2 +-
 crypto/ccm.c| 4 ++--
 crypto/crc32.c  | 1 +
 crypto/crc32c_generic.c | 2 +-
 crypto/crct10dif_generic.c  | 2 +-
 crypto/crypto_null.c| 6 +++---
 crypto/ctr.c| 2 +-
 crypto/deflate.c| 2 +-
 crypto/des_generic.c| 2 +-
 crypto/fcrypt.c | 1 +
 crypto/gcm.c| 6 +++---
 crypto/ghash-generic.c  | 2 +-
 crypto/khazad.c | 1 +
 crypto/krng.c   | 2 +-
 crypto/lz4.c| 1 +
 crypto/lz4hc.c  | 1 +
 crypto/lzo.c| 1 +
 crypto/md4.c| 2 +-
 crypto/md5.c| 1 +
 crypto/michael_mic.c| 1 +
 crypto/rmd128.c | 1 +
 crypto/rmd160.c | 1 +
 crypto/rmd256.c | 1 +
 crypto/rmd320.c | 1 +
 crypto/salsa20_generic.c| 2 +-
 crypto/seed.c   | 1 +
 crypto/serpent_generic.c| 4 ++--
 crypto/sha1_generic.c   | 2 +-
 crypto/sha256_generic.c | 4 ++--
 crypto/sha512_generic.c | 4 ++--
 crypto/tea.c| 4 ++--
 crypto/tgr192.c | 4 ++--
 crypto/twofish_generic.c| 2 +-
 crypto/wp512.c  | 4 ++--
 crypto/zlib.c   | 1 +
 drivers/crypto/padlock-aes.c| 2 +-
 drivers/crypto/padlock-sha.c

Re: [PATCH 00/11] crypto: caam: Error reporting fixes

[Kees Cook]

On Apr 24, Marek wrote:
 First stab at reworking the error.c thing in Freescale CAAM.
 This patchset cleans it up so it's not doing any too insane
 string messing anymore.

 NOTE: Can someone please test this on real hardware? I have
   none at hand, so THIS IS COMPILE-TESTED ONLY!

 Marek Vasut (11):
   crypto: caam: Contain caam_jr_strstatus() ugliness
   crypto: caam: Pull all the error codes out
   crypto: caam: Implement fast-path for error codes with no handler
   crypto: caam: Pass error type into the functions
   crypto: caam: Kill the easy targets
   crypto: caam: Dissolve report_jump_idx()
   crypto: caam: Clean up report_ccb_status()
   crypto: caam: Clean up report_deco_status()
   crypto: caam: Kill SPRINTFCAT() with fire
   crypto: caam: Sweep the remnants
   crypto: caam: Fix the 'quoted string split across lines'

  drivers/crypto/caam/caamalg.c  |  28 +--
  drivers/crypto/caam/caamhash.c |  28 +--
  drivers/crypto/caam/caamrng.c  |   7 +-
  drivers/crypto/caam/error.c| 389
 +++--
  drivers/crypto/caam/error.h|   2 +-
  drivers/crypto/caam/key_gen.c  |   7 +-
  6 files changed, 200 insertions(+), 261 deletions(-)

 Signed-off-by: Marek Vasut ma...@denx.de
 Cc: Herbert Xu herb...@gondor.apana.org.au
 Cc: Horia Geanta horia.gea...@freescale.com

Thanks for doing this! The final result looks good to me, though I too
can't test with real hardware. Please consider the whole series:

Reviewed-by: Kees Cook keesc...@chromium.org

Thanks,

-Kees

-- 
Kees Cook
Chrome OS Security
--
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][RESEND 3] hwrng: add randomness to system from rng sources

[Kees Cook]

On Wed, Mar 5, 2014 at 1:11 PM, Jason Cooper ja...@lakedaemon.net wrote:
 Matt, Kees,

 On Tue, Mar 04, 2014 at 04:39:57PM -0600, Matt Mackall wrote:
 On Tue, 2014-03-04 at 11:59 -0800, Kees Cook wrote:
  On Tue, Mar 4, 2014 at 11:53 AM, Jason Cooper ja...@lakedaemon.net wrote:
   On Tue, Mar 04, 2014 at 11:01:49AM -0800, Kees Cook wrote:
   On Tue, Mar 4, 2014 at 7:38 AM, Jason Cooper ja...@lakedaemon.net 
   wrote:
On Mon, Mar 03, 2014 at 03:51:48PM -0800, Kees Cook wrote:
 ...
  Well, I think there's confusion here over the hwrng and a hwrng. I
  have devices with multiple entropy sources, and all my hwrngs are
  built as modules, so I choose when to load them into my kernel. The
  arch-specific entropy source (e.g. RDRAND) is very different.

 Dynamically loading modules _after_ boot is fine.  Especially with
 Matt's clear explanation.  I'm not concerned about that scenario.

   Please understand, my point-of-view is as someone who installs Linux on
   equipment *after* purchase (hobbyist, tinkers).  If I control the part
   selection and sourcing of the board components, of course I have more
   trust in the hwrng.
  
   So my situation is similar to buying an Intel based laptop.  I can't do
   a special order at Bestbuy and ask for a system without the RDRAND
   instruction.  Same with the hobbyist market.  We buy the gear, but we
   have no control over what's inside it.
  
   In that situation, without this patch, I would enable the hwrng for the
   board.  With the patch in it's current form, I would start looking for
   research papers and discussions regarding using the hwrng for input.  If
   the patch provided arch_get_random_long(), I would feel comfortable
   enabling the hwrng.
  
   Perhaps I'm being too conservative, but I'd rather have the discussion
   now and have concerns proven unfounded than have someone say How the
   hell did this happen? three releases down the road.
 
  Sure, and I don't want to be the one weakening the entropy pool.

 [temporarily coming out of retirement to provide a clue]

 Thanks, Matt!

 The pool mixing function is intentionally _reversible_. This is a
 crucial security property.

 That means, if I have an initial secret pool state X, and hostile
 attacker controlled data Y, then we can do:

 X' = mix(X, Y)

  and

 X = unmix(X', Y)

 We can see from this that the combination of (X' and Y) still contain
 the information that was originally in X. Since it's clearly not in Y..
 it must all remain in X'.

 In other words, if there are 4096 bits of unknownness in X to start
 with, and I can get those same 4096 bits of unknownness back by
 unmixing X' and Y, then there must still be 4096 bits of unknownness
 in X'. If X' is 4096 bits long, then we've just proven that
 reversibility means the attacker can know nothing about the contents of
 X' by his choice of Y.

 Well, this reinforces my comfortability with loadable modules.  The pool
 is already initialized by the point at which the driver is loaded.

 Unfortunately, any of the drivers in hw_random can be built in.  When
 built in, hwrng_register is going to be called during the kernel
 initialization process.  In that case, the unknownness in X is not 4096
 bits, but far less.  Also, the items that may have seeded X (MAC addr,
 time, etc) are discoverable by a potential attacker.  This is also well
 before random-seed has been fed in.

Would reducing the size of the buffer used for this help your
concerns? Ironically, a non-malicious hwng using this code would
actually improve defense against other early-boot rng badness since
unlike time and MAC, this is much less discoverable by an attacker.


 That's the crux of my concern with this patch.  Basically, the user can
 choose 'y' over 'm', say when building a dedicated system w/o loadable
 modules, and not realize how much more trust he has placed in the hwrng.

 How does this patch look?  I'm not claiming my change is acceptable for
 mainline, just seeking feedback on the concept.  IS_MODULE() really
 doesn't have many users afaict.

Regardless, making this change to the patch would be fine with me,
yeah. I think the module case is much more common.

-Kees


 This builds without warning on ARCH=arm with multi_v7_defconfig when
 HW_RANDOM={y,m,n}

 thx,

 Jason.

 8--
 commit 0fcc0669ee4bbeae355c0f61f79a6b319a32ba12
 Author: Kees Cook keesc...@chromium.org
 Date:   Mon Mar 3 15:51:48 2014 -0800

 hwrng: add randomness to system from rng sources

 When bringing a new RNG source online, it seems like it would make sense
 to use some of its bytes to make the system entropy pool more random,
 as done with all sorts of other devices that contain per-device or
 per-boot differences.

 [jac] prevent the code from being called at boot up, before the entropy
 pool has had time to build and be well initialized.  We do this by
 making the code conditional on being built as a module.

 Signed-off

Re: [PATCH][RESEND 3] hwrng: add randomness to system from rng sources

[Kees Cook]

On Wed, Mar 5, 2014 at 4:52 PM, Matt Mackall m...@selenic.com wrote:
 On Wed, 2014-03-05 at 16:11 -0500, Jason Cooper wrote:
  In other words, if there are 4096 bits of unknownness in X to start
  with, and I can get those same 4096 bits of unknownness back by
  unmixing X' and Y, then there must still be 4096 bits of unknownness
  in X'. If X' is 4096 bits long, then we've just proven that
  reversibility means the attacker can know nothing about the contents of
  X' by his choice of Y.

 Well, this reinforces my comfortability with loadable modules.  The pool
 is already initialized by the point at which the driver is loaded.

 Unfortunately, any of the drivers in hw_random can be built in.  When
 built in, hwrng_register is going to be called during the kernel
 initialization process.  In that case, the unknownness in X is not 4096
 bits, but far less.  Also, the items that may have seeded X (MAC addr,
 time, etc) are discoverable by a potential attacker.  This is also well
 before random-seed has been fed in.

 To which I would respond.. so?

 If the pool is in an attacker-knowable state at early boot, adding
 attacker-controlled data does not make the situation any worse. In fact,
 if the attacker has less-than-perfect control of the inputs, mixing more
 things in will make things exponentially harder for the attacker.

 Put another way: mixing can't ever removes unknownness from the pool, it
 can only add more. So the only reason you should ever choose not to mix
 something into the pool is performance.

Excellent. So it sounds like you're okay with my original patch as-is?

-Kees

-- 
Kees Cook
Chrome OS Security
--
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][RESEND 3] hwrng: add randomness to system from rng sources

[Kees Cook]

On Tue, Mar 4, 2014 at 7:38 AM, Jason Cooper ja...@lakedaemon.net wrote:
 Kees, Ted,

 On Mon, Mar 03, 2014 at 03:51:48PM -0800, Kees Cook wrote:
 When bringing a new RNG source online, it seems like it would make sense
 to use some of its bytes to make the system entropy pool more random,
 as done with all sorts of other devices that contain per-device or
 per-boot differences.

 Why is this necessary?  init_std_data() already calls
 arch_get_random_long() while stirring each of the pools.

I may be misunderstanding something here, but hwrng isn't going to get
hit by a arch_get_random_long(). That's just for arch-specific RNGs
(e.g. RDRAND), where as hwrng is for, effectively, add-on devices
(e.g. TPMs).

 I'm a little concerned here because this gives potentially untrusted
 hwrngs more influence over the entropy pools initial state than most
 users of random.c expect.  Many of the drivers in hw_random/ are
 platform drivers and are initialized before random.c.

 I'm comfortable with the design decisions Ted has made wrt random.c and
 hwrngs.  However, I think that this changes that trust relationship in a
 fundamental way.  I'm ok with building support into my kernels for
 hwrngs as long as random.c's internal use of them is limited to the
 mixing in extract_buf() and init_std_data().

 By adding this patch, even without crediting entropy to the pool, a
 rogue hwrng now has significantly more influence over the initial state
 of the entropy pools.  Or, am I missing something?

I wasn't viewing this as dealing with rouge hwrngs (though shouldn't
that state still be covered due to the existing mixing), but more as a
hey this thing has some randomness associated with it, similar to
the mixing done for things like NIC MAC, etc. (Better, actually, since
NIC MAC is going to be the same every boot.) It seemed silly to ignore
an actual entropy source when seeding.

-Kees


 thx,

 Jason.

 Signed-off-by: Kees Cook keesc...@chromium.org
 ---
  drivers/char/hw_random/core.c |7 +++
  1 file changed, 7 insertions(+)

 diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
 index a0f7724852eb..6e5bb68a708c 100644
 --- a/drivers/char/hw_random/core.c
 +++ b/drivers/char/hw_random/core.c
 @@ -41,6 +41,7 @@
  #include linux/miscdevice.h
  #include linux/delay.h
  #include linux/slab.h
 +#include linux/random.h
  #include asm/uaccess.h


 @@ -305,6 +306,8 @@ int hwrng_register(struct hwrng *rng)
   int must_register_misc;
   int err = -EINVAL;
   struct hwrng *old_rng, *tmp;
 + unsigned char bytes[16];
 + int bytes_read;

   if (rng-name == NULL ||
   (rng-data_read == NULL  rng-read == NULL))
 @@ -348,6 +351,10 @@ int hwrng_register(struct hwrng *rng)
   }
   INIT_LIST_HEAD(rng-list);
   list_add_tail(rng-list, rng_list);
 +
 + bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
 + if (bytes_read  0)
 + add_device_randomness(bytes, bytes_read);
  out_unlock:
   mutex_unlock(rng_mutex);
  out:
 --
 1.7.9.5


 --
 Kees Cook
 Chrome OS Security
 --
 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



-- 
Kees Cook
Chrome OS Security
--
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][RESEND 3] hwrng: add randomness to system from rng sources

[Kees Cook]

On Tue, Mar 4, 2014 at 11:53 AM, Jason Cooper ja...@lakedaemon.net wrote:
 On Tue, Mar 04, 2014 at 11:01:49AM -0800, Kees Cook wrote:
 On Tue, Mar 4, 2014 at 7:38 AM, Jason Cooper ja...@lakedaemon.net wrote:
  Kees, Ted,
 
  On Mon, Mar 03, 2014 at 03:51:48PM -0800, Kees Cook wrote:
  When bringing a new RNG source online, it seems like it would make sense
  to use some of its bytes to make the system entropy pool more random,
  as done with all sorts of other devices that contain per-device or
  per-boot differences.
 
  Why is this necessary?  init_std_data() already calls
  arch_get_random_long() while stirring each of the pools.

 I may be misunderstanding something here, but hwrng isn't going to get
 hit by a arch_get_random_long().

 ahh, you are correct.  It appears it's only used on x86 and powerpc.
 Bad assumption on my part.

 That's just for arch-specific RNGs (e.g. RDRAND), where as hwrng is
 for, effectively, add-on devices (e.g. TPMs).

  I'm a little concerned here because this gives potentially untrusted
  hwrngs more influence over the entropy pools initial state than most
  users of random.c expect.  Many of the drivers in hw_random/ are
  platform drivers and are initialized before random.c.
 
  I'm comfortable with the design decisions Ted has made wrt random.c and
  hwrngs.  However, I think that this changes that trust relationship in a
  fundamental way.  I'm ok with building support into my kernels for
  hwrngs as long as random.c's internal use of them is limited to the
  mixing in extract_buf() and init_std_data().
 
  By adding this patch, even without crediting entropy to the pool, a
  rogue hwrng now has significantly more influence over the initial state
  of the entropy pools.  Or, am I missing something?

 I wasn't viewing this as dealing with rouge hwrngs (though shouldn't
 that state still be covered due to the existing mixing), but more as a
 hey this thing has some randomness associated with it, similar to
 the mixing done for things like NIC MAC, etc. (Better, actually, since
 NIC MAC is going to be the same every boot.) It seemed silly to ignore
 an actual entropy source when seeding.

 Agreed, but I think we need to be careful about how random.c interacts
 with any hwrng.  Ideally, the drivers in hw_random/ could provide
 arch_get_random_long().  This way, random.c still determines when and
 how to use the hwrng.

 Ultimately, the user (person compiling the kernel) will decide to trust
 or not trust the hwrng by enabling support for it or not.  My concern
 with this patch is that it changes the magnitude of that trust decision.
 And only the most diligent user would discover the change.

 To date, all discussion wrt random.c and hwrngs are that the output of
 the hwrng (in particular, RDRAND) is XORd with the output of the mixer.
 Now, we're saying it can provide input as well.

Well, I think there's confusion here over the hwrng and a hwrng. I
have devices with multiple entropy sources, and all my hwrngs are
built as modules, so I choose when to load them into my kernel. The
arch-specific entropy source (e.g. RDRAND) is very different.


 Please understand, my point-of-view is as someone who installs Linux on
 equipment *after* purchase (hobbyist, tinkers).  If I control the part
 selection and sourcing of the board components, of course I have more
 trust in the hwrng.

 So my situation is similar to buying an Intel based laptop.  I can't do
 a special order at Bestbuy and ask for a system without the RDRAND
 instruction.  Same with the hobbyist market.  We buy the gear, but we
 have no control over what's inside it.

 In that situation, without this patch, I would enable the hwrng for the
 board.  With the patch in it's current form, I would start looking for
 research papers and discussions regarding using the hwrng for input.  If
 the patch provided arch_get_random_long(), I would feel comfortable
 enabling the hwrng.

 Perhaps I'm being too conservative, but I'd rather have the discussion
 now and have concerns proven unfounded than have someone say How the
 hell did this happen? three releases down the road.

Sure, and I don't want to be the one weakening the entropy pool.
However, I think this patch is no different from the ones that stuff a
NIC MAC into the pool -- it's taking something from my system that is
unique or random and pushing the entropy seed around. It seems silly
that if I've loaded the hwrng-tpm module, my system entropy pool isn't
bumped.

-Kees


 thx,

 Jason.


  Signed-off-by: Kees Cook keesc...@chromium.org
  ---
   drivers/char/hw_random/core.c |7 +++
   1 file changed, 7 insertions(+)
 
  diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
  index a0f7724852eb..6e5bb68a708c 100644
  --- a/drivers/char/hw_random/core.c
  +++ b/drivers/char/hw_random/core.c
  @@ -41,6 +41,7 @@
   #include linux/miscdevice.h
   #include linux/delay.h
   #include linux/slab.h
  +#include linux/random.h
   #include asm

[PATCH][RESEND 3] hwrng: add randomness to system from rng sources

[Kees Cook]

When bringing a new RNG source online, it seems like it would make sense
to use some of its bytes to make the system entropy pool more random,
as done with all sorts of other devices that contain per-device or
per-boot differences.

Signed-off-by: Kees Cook keesc...@chromium.org
---
 drivers/char/hw_random/core.c |7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index a0f7724852eb..6e5bb68a708c 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -41,6 +41,7 @@
 #include linux/miscdevice.h
 #include linux/delay.h
 #include linux/slab.h
+#include linux/random.h
 #include asm/uaccess.h
 
 
@@ -305,6 +306,8 @@ int hwrng_register(struct hwrng *rng)
int must_register_misc;
int err = -EINVAL;
struct hwrng *old_rng, *tmp;
+   unsigned char bytes[16];
+   int bytes_read;
 
if (rng-name == NULL ||
(rng-data_read == NULL  rng-read == NULL))
@@ -348,6 +351,10 @@ int hwrng_register(struct hwrng *rng)
}
INIT_LIST_HEAD(rng-list);
list_add_tail(rng-list, rng_list);
+
+   bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
+   if (bytes_read  0)
+   add_device_randomness(bytes, bytes_read);
 out_unlock:
mutex_unlock(rng_mutex);
 out:
-- 
1.7.9.5


-- 
Kees Cook
Chrome OS Security
--
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

[RESEND 2][PATCH] hwrng: add randomness to system from rng sources

[Kees Cook]

When bringing a new RNG source online, it seems like it would make sense
to use some of its bytes to make the system entropy pool more random,
as done with all sorts of other devices that contain per-device or
per-boot differences.

Signed-off-by: Kees Cook keesc...@chromium.org
---
Added linux-crypto list to CC.
---
 drivers/char/hw_random/core.c |7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index a0f7724852eb..6e5bb68a708c 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -41,6 +41,7 @@
 #include linux/miscdevice.h
 #include linux/delay.h
 #include linux/slab.h
+#include linux/random.h
 #include asm/uaccess.h
 
 
@@ -305,6 +306,8 @@ int hwrng_register(struct hwrng *rng)
int must_register_misc;
int err = -EINVAL;
struct hwrng *old_rng, *tmp;
+   unsigned char bytes[16];
+   int bytes_read;
 
if (rng-name == NULL ||
(rng-data_read == NULL  rng-read == NULL))
@@ -348,6 +351,10 @@ int hwrng_register(struct hwrng *rng)
}
INIT_LIST_HEAD(rng-list);
list_add_tail(rng-list, rng_list);
+
+   bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
+   if (bytes_read  0)
+   add_device_randomness(bytes, bytes_read);
 out_unlock:
mutex_unlock(rng_mutex);
 out:
-- 
1.7.9.5


-- 
Kees Cook
Chrome OS Security
--
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: race condition in crypto larval handling

[Kees Cook]

On Sat, Sep 7, 2013 at 9:54 PM, Herbert Xu herb...@gondor.apana.org.au wrote:
 On Sun, Sep 08, 2013 at 02:37:03PM +1000, Herbert Xu wrote:
 On Sat, Sep 07, 2013 at 08:34:15PM -0700, Kees Cook wrote:
 
  However, I noticed on the good path (even without the above patch),
  I sometimes see a double-kfree triggered by the modprobe process. I
  can't, however, see how that's happening, since larval_destroy should
  only be called when refcnt == 0.

 Do you still see this double free with this patch? Without the
 patch it is completely expected as killing the same lavral twice
 will cause memory corruption leading to all sorts of weirdness,
 even if you stop it from deleting the list entry twice.

I noticed while testing the larval_kill fix, and then tried it again
after reverting the fix -- both showed the behavior.

 Actually I know what it is.  sha512 registers two algorithms.
 Therefore, it will create two larvals in sequence and then destroy
 them in turn.  So it's not a double free at all.  If you put a
 printk in crypto_larval_alloc that should confirm this.

Ah! That would make sense; it just happens to re-allocate to the exact
same location, yes. Whew, that's certainly what's happening. I can
retest to confirm in my morning.

Thanks again for the larval_kill fix! I'll get it rolled out for wider
testing to confirm that it make our crash numbers go down.

-Kees

-- 
Kees Cook
Chrome OS Security
--
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: race condition in crypto larval handling

[Kees Cook]

On Sat, Sep 7, 2013 at 11:01 PM, Kees Cook keesc...@chromium.org wrote:
 On Sat, Sep 7, 2013 at 9:54 PM, Herbert Xu herb...@gondor.apana.org.au 
 wrote:
 On Sun, Sep 08, 2013 at 02:37:03PM +1000, Herbert Xu wrote:
 On Sat, Sep 07, 2013 at 08:34:15PM -0700, Kees Cook wrote:
 
  However, I noticed on the good path (even without the above patch),
  I sometimes see a double-kfree triggered by the modprobe process. I
  can't, however, see how that's happening, since larval_destroy should
  only be called when refcnt == 0.

 Do you still see this double free with this patch? Without the
 patch it is completely expected as killing the same lavral twice
 will cause memory corruption leading to all sorts of weirdness,
 even if you stop it from deleting the list entry twice.

 Actually I know what it is.  sha512 registers two algorithms.
 Therefore, it will create two larvals in sequence and then destroy
 them in turn.  So it's not a double free at all.  If you put a
 printk in crypto_larval_alloc that should confirm this.

 Ah! That would make sense; it just happens to re-allocate to the exact
 same location, yes. Whew, that's certainly what's happening. I can
 retest to confirm in my morning.

Confirmed: 2 allocs happen, and then 2 kfrees. :)

-Kees

-- 
Kees Cook
Chrome OS Security
--
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: race condition in crypto larval handling

[Kees Cook]

On Sat, Sep 7, 2013 at 7:39 AM, Neil Horman nhor...@tuxdriver.com wrote:
 On Fri, Sep 06, 2013 at 04:20:50PM -0700, Kees Cook wrote:
 Hi,

 I've tracked down a race condition and ref counting problem in the
 crypto API internals. We've been seeing it under Chrome OS, but it
 seems it's not isolated to just us:

 https://code.google.com/p/chromium/issues/detail?id=244581
 http://marc.info/?l=linux-crypto-vgerm=135429403609373w=2
 https://bugzilla.redhat.com/show_bug.cgi?id=983682
 http://www.mail-archive.com/linux-cifs@vger.kernel.org/msg07933.html

 I think I've found the basic origin of the problem.
 crypto_larval_add() has synchronization to make sure only a single
 larval can ever be created. That logic seems totally fine. However,
 this means that crypto_larval_lookup() from two threads can return the
 same larval, which means that crypto_alg_mod_lookup() runs the risk of
 calling crypto_larval_kill() on the same larval twice, which does not
 appear to be handled sanely.

 I can easily crash the kernel by forcing a synchronization point just
 before the return crypt_larval_add(...) call in
 crypto_larval_lookup(). Basically, I added this sloppy sync code (I
 feel like there must be a better way to do this):

 +static atomic_t global_sync = ATOMIC_INIT(0);
 ...
 struct crypto_alg *crypto_larval_lookup(const char *name, u32 type, u32 mask)
 ...
 +   if (strncmp(name, asdf, 4) == 0) {
 +   int value;
 +
 +   value = atomic_add_return(1, global_sync);
 +   if (value == 1) {
 +   /* I was first to stall here, wait for inc. */
 +   while (atomic_read(global_sync) != 2)
 +   cpu_relax();
 +   } else if (value == 2) {
 +   /* I was second to stall here, wait for dec. */
 +   while (atomic_read(global_sync) != 1)
 +   cpu_relax();
 +   } else {
 +   BUG();
 +   }
 +   atomic_dec(global_sync);
 +   }

 return crypto_larval_add(name, type, mask);
  }

 And then ran code from userspace that did, effectively:

 struct sockaddr_alg sa = {
 .salg_family = AF_ALG,
 .salg_type   = hash,
 };
 strcpy(sa.salg_name, argv[1]);

 fork();
 ...
 sds[0] = socket(AF_ALG, SOCK_SEQPACKET, 0);
 bind(sds[0], (struct sockaddr *) sa, sizeof(sa));

 And ran this as ./tickle asdf1 to generate the two threads trying to
 find an invalid alg. The race looks possible even with valid algs, but
 this was the fastest path I could see to tickle the issue.

 With added printks to the kernel, it was clear that crypto_larval_kill
 was being called twice on the same larval, and the list_del() call was
 doing bad things. When I fixed that, the refcnt bug became very
 obvious. Here's the change I made for crypto_larval_kill:

 @@ -161,7 +166,8 @@ void crypto_larval_kill(struct crypto_alg *alg)
 struct crypto_larval *larval = (void *)alg;

 down_write(crypto_alg_sem);
 -   list_del(alg-cra_list);
 +   if (!list_empty(alg-cra_list))
 +   list_del_init(alg-cra_list);
 up_write(crypto_alg_sem);
 complete_all(larval-completion);
 crypto_alg_put(alg);

 It seems that there are too many put calls (mixed between
 crypto_alg_put() and crypto_mod_put(), which also calls
 crypto_alg_put()). See this annotated portion of
 crypto_alg_mod_lookup:

 /* This can (correctly) return the same larval for two threads. */
 larval = crypto_larval_lookup(name, type, mask);
 if (IS_ERR(larval) || !crypto_is_larval(larval))
 return larval;

 ok = crypto_probing_notify(CRYPTO_MSG_ALG_REQUEST, larval);

 if (ok == NOTIFY_STOP)
 /* This calls crypto_mod_put(), but sometimes also get?? */
 alg = crypto_larval_wait(larval);
 else {
 /* This directly calls crypto_mod_put */
 crypto_mod_put(larval);
 alg = ERR_PTR(-ENOENT);
 }
 /* This calls crypto_alg_put */
 crypto_larval_kill(larval);
 return alg;

 In the two-thread situation, the first thread gets a larval with
 refcnt 2 via crypto_larval_add. (Why 2?) The next thread finds the
 larval via crypto_larval_add's call to __crypto_alg_lookup() and sees
 the ref bump to 3. While exiting crypto_alg_mod_lookup, each thread
 decrements the ref count twice.

 It seems to me like either each call to crypto_larval_lookup() should
 result in a refcount bumped by two, or crypto_alg_mod_lookup() should
 decrement only once, and the initial refcount should be 1 not 2 from
 crypto_larval_add. But it's not clear to me which is sensible here.

 What's the right solution here?

 Thanks,

 -Kees

 --
 Kees Cook
 Chrome OS Security

 I've been tracking this problem too:
 https://bugzilla.redhat.com/show_bug.cgi?id=1002351

Re: race condition in crypto larval handling

[Kees Cook]

On Sat, Sep 7, 2013 at 6:32 PM, Herbert Xu herb...@gondor.apana.org.au wrote:
 On Fri, Sep 06, 2013 at 04:20:50PM -0700, Kees Cook wrote:

 In the two-thread situation, the first thread gets a larval with
 refcnt 2 via crypto_larval_add. (Why 2?) The next thread finds the
 larval via crypto_larval_add's call to __crypto_alg_lookup() and sees
 the ref bump to 3. While exiting crypto_alg_mod_lookup, each thread
 decrements the ref count twice.

 It seems to me like either each call to crypto_larval_lookup() should
 result in a refcount bumped by two, or crypto_alg_mod_lookup() should
 decrement only once, and the initial refcount should be 1 not 2 from
 crypto_larval_add. But it's not clear to me which is sensible here.

 What's the right solution here?

 First of all thanks a lot for tracking this problem down! It's
 been bothering me for months but I was unable to find a good
 reproducer.

 So now that you've identified the problem, the solution is easy.
 crypto_larval_lookup should only ever return a larval if it created
 one.  Any larval created earlier must be waited on first before we
 return.

 So does this patch fix the crash for you?

 diff --git a/crypto/api.c b/crypto/api.c
 index 320ea4d..a2b39c5 100644
 --- a/crypto/api.c
 +++ b/crypto/api.c
 @@ -34,6 +34,8 @@ EXPORT_SYMBOL_GPL(crypto_alg_sem);
  BLOCKING_NOTIFIER_HEAD(crypto_chain);
  EXPORT_SYMBOL_GPL(crypto_chain);

 +static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg);
 +
  struct crypto_alg *crypto_mod_get(struct crypto_alg *alg)
  {
 return try_module_get(alg-cra_module) ? crypto_alg_get(alg) : NULL;
 @@ -144,8 +146,11 @@ static struct crypto_alg *crypto_larval_add(const char 
 *name, u32 type,
 }
 up_write(crypto_alg_sem);

 -   if (alg != larval-alg)
 +   if (alg != larval-alg) {
 kfree(larval);
 +   if (crypto_is_larval(alg))
 +   alg = crypto_larval_wait(alg);
 +   }

 return alg;
  }

Thanks! This does stop the crash I was seeing on the race with no
valid alg path, yay!

However, I noticed on the good path (even without the above patch),
I sometimes see a double-kfree triggered by the modprobe process. I
can't, however, see how that's happening, since larval_destroy should
only be called when refcnt == 0.

Here's the debugging output I added that noticed this. pid 15797 and
15798 are my hash tool that forks before doing identical AF_ALG
calls for sha512, which has to be modprobed. The trailing 31 and
32 was an atomic counter I added just to make sure I wasn't going
crazy:

[  112.495789] crypto_alg_mod_lookup 15797: looking up [sha512]
[  112.495815] crypto_larval_lookup 15797: looking up [sha512]
[  112.495839] crypto_larval_lookup 15797: requesting module [sha512]
[  112.495915] crypto_alg_mod_lookup 15798: looking up [sha512]
[  112.495937] crypto_larval_lookup 15798: looking up [sha512]
[  112.495960] crypto_larval_lookup 15798: requesting module [sha512]
[  112.498691] crypto_larval_destroy 15808(modprobe): larval
kfree(880123e9da00) refcnt:0 (serial:31)
[  112.498771] crypto_larval_destroy 15808(modprobe): larval
kfree(880123e9da00) refcnt:0 (serial:32)
[  112.500888] crypto_larval_lookup 15797: after requesting module
[sha512], got alg c0299050
[  112.500904] crypto_larval_lookup 15797: for [sha512], have alg
c0299050
[  112.500934] crypto_larval_lookup 15797: for [sha512], alg
c0299050 is NOT larval
[  112.500953] crypto_alg_mod_lookup 15797: [sha512] is not larval
c0299050
[  112.501384] crypto_larval_lookup 15798: after requesting module
[sha512], got alg c0299050
[  112.501404] crypto_larval_lookup 15798: for [sha512], have alg
c0299050
[  112.501417] crypto_larval_lookup 15798: for [sha512], alg
c0299050 is NOT larval
[  112.501432] crypto_alg_mod_lookup 15798: [sha512] is not larval
c0299050

Regardless, this seems to be a separate bug.

-Kees

-- 
Kees Cook
Chrome OS Security
--
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

race condition in crypto larval handling

[Kees Cook]

Hi,

I've tracked down a race condition and ref counting problem in the
crypto API internals. We've been seeing it under Chrome OS, but it
seems it's not isolated to just us:

https://code.google.com/p/chromium/issues/detail?id=244581
http://marc.info/?l=linux-crypto-vgerm=135429403609373w=2
https://bugzilla.redhat.com/show_bug.cgi?id=983682
http://www.mail-archive.com/linux-cifs@vger.kernel.org/msg07933.html

I think I've found the basic origin of the problem.
crypto_larval_add() has synchronization to make sure only a single
larval can ever be created. That logic seems totally fine. However,
this means that crypto_larval_lookup() from two threads can return the
same larval, which means that crypto_alg_mod_lookup() runs the risk of
calling crypto_larval_kill() on the same larval twice, which does not
appear to be handled sanely.

I can easily crash the kernel by forcing a synchronization point just
before the return crypt_larval_add(...) call in
crypto_larval_lookup(). Basically, I added this sloppy sync code (I
feel like there must be a better way to do this):

+static atomic_t global_sync = ATOMIC_INIT(0);
...
struct crypto_alg *crypto_larval_lookup(const char *name, u32 type, u32 mask)
...
+   if (strncmp(name, asdf, 4) == 0) {
+   int value;
+
+   value = atomic_add_return(1, global_sync);
+   if (value == 1) {
+   /* I was first to stall here, wait for inc. */
+   while (atomic_read(global_sync) != 2)
+   cpu_relax();
+   } else if (value == 2) {
+   /* I was second to stall here, wait for dec. */
+   while (atomic_read(global_sync) != 1)
+   cpu_relax();
+   } else {
+   BUG();
+   }
+   atomic_dec(global_sync);
+   }

return crypto_larval_add(name, type, mask);
 }

And then ran code from userspace that did, effectively:

struct sockaddr_alg sa = {
.salg_family = AF_ALG,
.salg_type   = hash,
};
strcpy(sa.salg_name, argv[1]);

fork();
...
sds[0] = socket(AF_ALG, SOCK_SEQPACKET, 0);
bind(sds[0], (struct sockaddr *) sa, sizeof(sa));

And ran this as ./tickle asdf1 to generate the two threads trying to
find an invalid alg. The race looks possible even with valid algs, but
this was the fastest path I could see to tickle the issue.

With added printks to the kernel, it was clear that crypto_larval_kill
was being called twice on the same larval, and the list_del() call was
doing bad things. When I fixed that, the refcnt bug became very
obvious. Here's the change I made for crypto_larval_kill:

@@ -161,7 +166,8 @@ void crypto_larval_kill(struct crypto_alg *alg)
struct crypto_larval *larval = (void *)alg;

down_write(crypto_alg_sem);
-   list_del(alg-cra_list);
+   if (!list_empty(alg-cra_list))
+   list_del_init(alg-cra_list);
up_write(crypto_alg_sem);
complete_all(larval-completion);
crypto_alg_put(alg);

It seems that there are too many put calls (mixed between
crypto_alg_put() and crypto_mod_put(), which also calls
crypto_alg_put()). See this annotated portion of
crypto_alg_mod_lookup:

/* This can (correctly) return the same larval for two threads. */
larval = crypto_larval_lookup(name, type, mask);
if (IS_ERR(larval) || !crypto_is_larval(larval))
return larval;

ok = crypto_probing_notify(CRYPTO_MSG_ALG_REQUEST, larval);

if (ok == NOTIFY_STOP)
/* This calls crypto_mod_put(), but sometimes also get?? */
alg = crypto_larval_wait(larval);
else {
/* This directly calls crypto_mod_put */
crypto_mod_put(larval);
alg = ERR_PTR(-ENOENT);
}
/* This calls crypto_alg_put */
crypto_larval_kill(larval);
return alg;

In the two-thread situation, the first thread gets a larval with
refcnt 2 via crypto_larval_add. (Why 2?) The next thread finds the
larval via crypto_larval_add's call to __crypto_alg_lookup() and sees
the ref bump to 3. While exiting crypto_alg_mod_lookup, each thread
decrements the ref count twice.

It seems to me like either each call to crypto_larval_lookup() should
result in a refcount bumped by two, or crypto_alg_mod_lookup() should
decrement only once, and the initial refcount should be 1 not 2 from
crypto_larval_add. But it's not clear to me which is sensible here.

What's the right solution here?

Thanks,

-Kees

-- 
Kees Cook
Chrome OS Security
--
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