[PATCH v2 0/2] crypto: removing various VLAs

[PATCH v2 0/2] crypto: removing various VLAs

[Salvatore Mesoraca]

v2:
	As suggested by Herbert Xu, the blocksize and alignmask checks
	have been moved to crypto_check_alg.
	So, now, all the other separate checks are not necessary.
	Also, the defines have been moved to include/crypto/algapi.h.

v1:
	As suggested by Laura Abbott[1], I'm resending my patch with
	MAX_BLOCKSIZE and MAX_ALIGNMASK defined in an header, so they
	can be used in other places.
	I took this opportunity to deal with some other VLAs not
	handled in the old patch.

[1] http://lkml.kernel.org/r/4e536889-439a-49e6-dd95-2d4286913202@redhat.com

Salvatore Mesoraca (2):
  crypto: api - laying defines and checks for statically allocated
    buffers
  crypto: remove several VLAs

 crypto/algapi.c         | 10 ++++++++++
 crypto/cfb.c            |  7 +++----
 crypto/cipher.c         |  3 ++-
 crypto/ctr.c            |  4 ++--
 crypto/cts.c            |  5 +++--
 crypto/pcbc.c           |  5 +++--
 include/crypto/algapi.h |  8 ++++++++
 7 files changed, 31 insertions(+), 11 deletions(-)

-- 
1.9.1

[PATCH v2 1/2] crypto: api - laying defines and checks for statically allocated buffers

[Salvatore Mesoraca]

In preparation for the removal of VLAs[1] from crypto code.
We create 2 new compile-time constants: all ciphers implemented
in Linux have a block size less than or equal to 16 bytes and
the most demanding hw require 16 bytes alignment for the block
buffer.
We also enforce these limits in crypto_check_alg when a new
cipher is registered.

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

Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
---
 crypto/algapi.c         | 10 ++++++++++
 include/crypto/algapi.h |  8 ++++++++
 2 files changed, 18 insertions(+)

diff --git a/crypto/algapi.c b/crypto/algapi.c
index 2a0271b..c0755cf 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -10,6 +10,7 @@
  *
  */
 
+#include <crypto/algapi.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/fips.h>
@@ -59,6 +60,15 @@ static int crypto_check_alg(struct crypto_alg *alg)
 	if (alg->cra_blocksize > PAGE_SIZE / 8)
 		return -EINVAL;
 
+	if (!alg->cra_type && (alg->cra_flags & CRYPTO_ALG_TYPE_MASK) ==
+			       CRYPTO_ALG_TYPE_CIPHER) {
+		if (alg->cra_alignmask > MAX_CIPHER_ALIGNMASK)
+			return -EINVAL;
+
+		if (alg->cra_blocksize > MAX_CIPHER_BLOCKSIZE)
+			return -EINVAL;
+	}
+
 	if (alg->cra_priority < 0)
 		return -EINVAL;
 
diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h
index 1aba888..bd5e8cc 100644
--- a/include/crypto/algapi.h
+++ b/include/crypto/algapi.h
@@ -17,6 +17,14 @@
 #include <linux/kernel.h>
 #include <linux/skbuff.h>
 
+/*
+ * Maximum values for blocksize and alignmask, used to allocate
+ * static buffers that are big enough for any combination of
+ * ciphers and architectures.
+ */
+#define MAX_CIPHER_BLOCKSIZE		16
+#define MAX_CIPHER_ALIGNMASK		15
+
 struct crypto_aead;
 struct crypto_instance;
 struct module;
-- 
1.9.1

[PATCH v2 2/2] crypto: remove several VLAs

[Salvatore Mesoraca]

We avoid various VLAs[1] by using constant expressions for block size
and alignment mask.

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

Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
---
 crypto/cfb.c    | 7 +++----
 crypto/cipher.c | 3 ++-
 crypto/ctr.c    | 4 ++--
 crypto/cts.c    | 5 +++--
 crypto/pcbc.c   | 5 +++--
 5 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/crypto/cfb.c b/crypto/cfb.c
index 94ee39b..a0d68c0 100644
--- a/crypto/cfb.c
+++ b/crypto/cfb.c
@@ -53,9 +53,8 @@ static void crypto_cfb_encrypt_one(struct crypto_skcipher *tfm,
 static void crypto_cfb_final(struct skcipher_walk *walk,
 			     struct crypto_skcipher *tfm)
 {
-	const unsigned int bsize = crypto_cfb_bsize(tfm);
 	const unsigned long alignmask = crypto_skcipher_alignmask(tfm);
-	u8 tmp[bsize + alignmask];
+	u8 tmp[MAX_CIPHER_BLOCKSIZE + MAX_CIPHER_ALIGNMASK];
 	u8 *stream = PTR_ALIGN(tmp + 0, alignmask + 1);
 	u8 *src = walk->src.virt.addr;
 	u8 *dst = walk->dst.virt.addr;
@@ -94,7 +93,7 @@ static int crypto_cfb_encrypt_inplace(struct skcipher_walk *walk,
 	unsigned int nbytes = walk->nbytes;
 	u8 *src = walk->src.virt.addr;
 	u8 *iv = walk->iv;
-	u8 tmp[bsize];
+	u8 tmp[MAX_CIPHER_BLOCKSIZE];
 
 	do {
 		crypto_cfb_encrypt_one(tfm, iv, tmp);
@@ -164,7 +163,7 @@ static int crypto_cfb_decrypt_inplace(struct skcipher_walk *walk,
 	unsigned int nbytes = walk->nbytes;
 	u8 *src = walk->src.virt.addr;
 	u8 *iv = walk->iv;
-	u8 tmp[bsize];
+	u8 tmp[MAX_CIPHER_BLOCKSIZE];
 
 	do {
 		crypto_cfb_encrypt_one(tfm, iv, tmp);
diff --git a/crypto/cipher.c b/crypto/cipher.c
index 94fa355..57836c3 100644
--- a/crypto/cipher.c
+++ b/crypto/cipher.c
@@ -13,6 +13,7 @@
  *
  */
 
+#include <crypto/algapi.h>
 #include <linux/kernel.h>
 #include <linux/crypto.h>
 #include <linux/errno.h>
@@ -67,7 +68,7 @@ static void cipher_crypt_unaligned(void (*fn)(struct crypto_tfm *, u8 *,
 {
 	unsigned long alignmask = crypto_tfm_alg_alignmask(tfm);
 	unsigned int size = crypto_tfm_alg_blocksize(tfm);
-	u8 buffer[size + alignmask];
+	u8 buffer[MAX_CIPHER_BLOCKSIZE + MAX_CIPHER_ALIGNMASK];
 	u8 *tmp = (u8 *)ALIGN((unsigned long)buffer, alignmask + 1);
 
 	memcpy(tmp, src, size);
diff --git a/crypto/ctr.c b/crypto/ctr.c
index 854d924..435b75b 100644
--- a/crypto/ctr.c
+++ b/crypto/ctr.c
@@ -58,7 +58,7 @@ static void crypto_ctr_crypt_final(struct blkcipher_walk *walk,
 	unsigned int bsize = crypto_cipher_blocksize(tfm);
 	unsigned long alignmask = crypto_cipher_alignmask(tfm);
 	u8 *ctrblk = walk->iv;
-	u8 tmp[bsize + alignmask];
+	u8 tmp[MAX_CIPHER_BLOCKSIZE + MAX_CIPHER_ALIGNMASK];
 	u8 *keystream = PTR_ALIGN(tmp + 0, alignmask + 1);
 	u8 *src = walk->src.virt.addr;
 	u8 *dst = walk->dst.virt.addr;
@@ -106,7 +106,7 @@ static int crypto_ctr_crypt_inplace(struct blkcipher_walk *walk,
 	unsigned int nbytes = walk->nbytes;
 	u8 *ctrblk = walk->iv;
 	u8 *src = walk->src.virt.addr;
-	u8 tmp[bsize + alignmask];
+	u8 tmp[MAX_CIPHER_BLOCKSIZE + MAX_CIPHER_ALIGNMASK];
 	u8 *keystream = PTR_ALIGN(tmp + 0, alignmask + 1);
 
 	do {
diff --git a/crypto/cts.c b/crypto/cts.c
index 4773c18..4e28d83 100644
--- a/crypto/cts.c
+++ b/crypto/cts.c
@@ -40,6 +40,7 @@
  * rfc3962 includes errata information in its Appendix A.
  */
 
+#include <crypto/algapi.h>
 #include <crypto/internal/skcipher.h>
 #include <linux/err.h>
 #include <linux/init.h>
@@ -104,7 +105,7 @@ static int cts_cbc_encrypt(struct skcipher_request *req)
 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
 	struct skcipher_request *subreq = &rctx->subreq;
 	int bsize = crypto_skcipher_blocksize(tfm);
-	u8 d[bsize * 2] __aligned(__alignof__(u32));
+	u8 d[MAX_CIPHER_BLOCKSIZE * 2] __aligned(__alignof__(u32));
 	struct scatterlist *sg;
 	unsigned int offset;
 	int lastn;
@@ -183,7 +184,7 @@ static int cts_cbc_decrypt(struct skcipher_request *req)
 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
 	struct skcipher_request *subreq = &rctx->subreq;
 	int bsize = crypto_skcipher_blocksize(tfm);
-	u8 d[bsize * 2] __aligned(__alignof__(u32));
+	u8 d[MAX_CIPHER_BLOCKSIZE * 2] __aligned(__alignof__(u32));
 	struct scatterlist *sg;
 	unsigned int offset;
 	u8 *space;
diff --git a/crypto/pcbc.c b/crypto/pcbc.c
index d9e45a9..ef802f6 100644
--- a/crypto/pcbc.c
+++ b/crypto/pcbc.c
@@ -14,6 +14,7 @@
  *
  */
 
+#include <crypto/algapi.h>
 #include <crypto/internal/skcipher.h>
 #include <linux/err.h>
 #include <linux/init.h>
@@ -72,7 +73,7 @@ static int crypto_pcbc_encrypt_inplace(struct skcipher_request *req,
 	unsigned int nbytes = walk->nbytes;
 	u8 *src = walk->src.virt.addr;
 	u8 *iv = walk->iv;
-	u8 tmpbuf[bsize];
+	u8 tmpbuf[MAX_CIPHER_BLOCKSIZE];
 
 	do {
 		memcpy(tmpbuf, src, bsize);
@@ -144,7 +145,7 @@ static int crypto_pcbc_decrypt_inplace(struct skcipher_request *req,
 	unsigned int nbytes = walk->nbytes;
 	u8 *src = walk->src.virt.addr;
 	u8 *iv = walk->iv;
-	u8 tmpbuf[bsize] __aligned(__alignof__(u32));
+	u8 tmpbuf[MAX_CIPHER_BLOCKSIZE] __aligned(__alignof__(u32));
 
 	do {
 		memcpy(tmpbuf, src, bsize);
-- 
1.9.1

RE: [PATCH v2 0/2] crypto: removing various VLAs

[David Laight]

From: Salvatore Mesoraca
> Sent: 09 April 2018 14:55
> 
> v2:
> 	As suggested by Herbert Xu, the blocksize and alignmask checks
> 	have been moved to crypto_check_alg.
> 	So, now, all the other separate checks are not necessary.
> 	Also, the defines have been moved to include/crypto/algapi.h.
> 
> v1:
> 	As suggested by Laura Abbott[1], I'm resending my patch with
> 	MAX_BLOCKSIZE and MAX_ALIGNMASK defined in an header, so they
> 	can be used in other places.
> 	I took this opportunity to deal with some other VLAs not
> 	handled in the old patch.

If the constants are visible they need better names.
Maybe CRYPTO_MAX_xxx.

You can also do much better than allocating MAX_BLOCKSIZE + MAX_ALIGNMASK
bytes by requesting 'long' aligned on-stack memory.
The easiest way is to define a union like:

union crypto_tmp {
	u8 buf[CRYPTO_MAX_TMP_BUF];
	long buf_align;
};

Then in each function:

	union tmp crypto_tmp;
	u8 *keystream = PTR_ALIGN(tmp.buf, alignmask + 1);

I think CRYPTO_MAX_TMP_BUF needs to be MAX_BLOCKSIZE + MAX_ALIGNMASK - sizeof (long).

	David

Re: [PATCH v2 0/2] crypto: removing various VLAs

[Salvatore Mesoraca]

2018-04-09 16:35 GMT+02:00 David Laight <David.Laight@aculab.com>:
> From: Salvatore Mesoraca
>> Sent: 09 April 2018 14:55
>>
>> v2:
>>       As suggested by Herbert Xu, the blocksize and alignmask checks
>>       have been moved to crypto_check_alg.
>>       So, now, all the other separate checks are not necessary.
>>       Also, the defines have been moved to include/crypto/algapi.h.
>>
>> v1:
>>       As suggested by Laura Abbott[1], I'm resending my patch with
>>       MAX_BLOCKSIZE and MAX_ALIGNMASK defined in an header, so they
>>       can be used in other places.
>>       I took this opportunity to deal with some other VLAs not
>>       handled in the old patch.
>
> If the constants are visible they need better names.
> Maybe CRYPTO_MAX_xxx.

You are right, in fact I renamed them, but forget to write about this
in the change log.
The new names look like MAX_CIPHER_*.

> You can also do much better than allocating MAX_BLOCKSIZE + MAX_ALIGNMASK
> bytes by requesting 'long' aligned on-stack memory.
> The easiest way is to define a union like:
>
> union crypto_tmp {
>         u8 buf[CRYPTO_MAX_TMP_BUF];
>         long buf_align;
> };
>
> Then in each function:
>
>         union tmp crypto_tmp;
>         u8 *keystream = PTR_ALIGN(tmp.buf, alignmask + 1);
>
> I think CRYPTO_MAX_TMP_BUF needs to be MAX_BLOCKSIZE + MAX_ALIGNMASK - sizeof (long).

Yeah, that would be nice, it might save us 4-8 bytes on the stack.
But I was thinking, wouldn't it be even better to do something like:

u8 buf[CRYPTO_MAX_TMP_BUF] __aligned(__alignof__(long));
u8 *keystream = PTR_ALIGN(buf, alignmask + 1);

In this case __aligned should work, if I'm not missing some other
subtle GCC caveat.

Thank you,

Salvatore

RE: [PATCH v2 0/2] crypto: removing various VLAs

[David Laight]

From: Salvatore Mesoraca
> Sent: 09 April 2018 17:38
...
> > You can also do much better than allocating MAX_BLOCKSIZE + MAX_ALIGNMASK
> > bytes by requesting 'long' aligned on-stack memory.
> > The easiest way is to define a union like:
> >
> > union crypto_tmp {
> >         u8 buf[CRYPTO_MAX_TMP_BUF];
> >         long buf_align;
> > };
> >
> > Then in each function:
> >
> >         union tmp crypto_tmp;
> >         u8 *keystream = PTR_ALIGN(tmp.buf, alignmask + 1);
> >
> > I think CRYPTO_MAX_TMP_BUF needs to be MAX_BLOCKSIZE + MAX_ALIGNMASK - sizeof (long).
> 
> Yeah, that would be nice, it might save us 4-8 bytes on the stack.
> But I was thinking, wouldn't it be even better to do something like:
> 
> u8 buf[CRYPTO_MAX_TMP_BUF] __aligned(__alignof__(long));
> u8 *keystream = PTR_ALIGN(buf, alignmask + 1);
> 
> In this case __aligned should work, if I'm not missing some other
> subtle GCC caveat.

Thinking further, there is no point aligning the buffer to less than
the maximum alignment allowed - it just adds code.

So you end up with:
#define MAX_STACK_ALIGN __alignof__(long)  /* Largest type the compiler can align on stack */
#define CRYPTO_MAX_TMP_BUF (MAX_BLOCKSIZE + MAX_ALIGNMASK + 1 - MAX_STACK_ALIGN)
u8 buf[CRYPTO_MAX_TMP_BUF] __aligned(MAX_STACK_ALIGN);
u8 *keystream = PTR_ALIGN(buf, MAX_ALIGNMASK + 1);

The last two lines could be put into a #define of their own so that the 'call sites'
don't need to know the gory details of how the buffer is defined.

In principle you could just have:
u8 keystream[MAX_BLOCKSIZE] __aligned(MAX_ALIGNMASK + 1);

But that will go wrong if the stack alignment has gone wrong somewhere
and generates a double stack frame if the requested alignment is larger
than the expected stack alignment.

IIRC there is a gcc command line option to enforce stack alignment on
some/all function entry prologues. The gory details are held in some
old brain cells somewhere.

	David

Re: [PATCH v2 0/2] crypto: removing various VLAs

[Herbert Xu]

On Mon, Apr 09, 2018 at 03:54:45PM +0200, Salvatore Mesoraca wrote:
> v2:
> 	As suggested by Herbert Xu, the blocksize and alignmask checks
> 	have been moved to crypto_check_alg.
> 	So, now, all the other separate checks are not necessary.
> 	Also, the defines have been moved to include/crypto/algapi.h.
> 
> v1:
> 	As suggested by Laura Abbott[1], I'm resending my patch with
> 	MAX_BLOCKSIZE and MAX_ALIGNMASK defined in an header, so they
> 	can be used in other places.
> 	I took this opportunity to deal with some other VLAs not
> 	handled in the old patch.
> 
> [1] http://lkml.kernel.org/r/4e536889-439a-49e6-dd95-2d4286913202@redhat.com
> 
> Salvatore Mesoraca (2):
>   crypto: api - laying defines and checks for statically allocated
>     buffers
>   crypto: remove several VLAs
> 
>  crypto/algapi.c         | 10 ++++++++++
>  crypto/cfb.c            |  7 +++----
>  crypto/cipher.c         |  3 ++-
>  crypto/ctr.c            |  4 ++--
>  crypto/cts.c            |  5 +++--
>  crypto/pcbc.c           |  5 +++--
>  include/crypto/algapi.h |  8 ++++++++
>  7 files changed, 31 insertions(+), 11 deletions(-)

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 0/2] crypto: removing various VLAs

[Salvatore Mesoraca]

2018-04-20 18:51 GMT+02:00 Herbert Xu <herbert@gondor.apana.org.au>:
> On Mon, Apr 09, 2018 at 03:54:45PM +0200, Salvatore Mesoraca wrote:
>> v2:
>>       As suggested by Herbert Xu, the blocksize and alignmask checks
>>       have been moved to crypto_check_alg.
>>       So, now, all the other separate checks are not necessary.
>>       Also, the defines have been moved to include/crypto/algapi.h.
>>
>> v1:
>>       As suggested by Laura Abbott[1], I'm resending my patch with
>>       MAX_BLOCKSIZE and MAX_ALIGNMASK defined in an header, so they
>>       can be used in other places.
>>       I took this opportunity to deal with some other VLAs not
>>       handled in the old patch.
>>
>> [1] http://lkml.kernel.org/r/4e536889-439a-49e6-dd95-2d4286913202@redhat.com
>>
>> Salvatore Mesoraca (2):
>>   crypto: api - laying defines and checks for statically allocated
>>     buffers
>>   crypto: remove several VLAs
>>
>>  crypto/algapi.c         | 10 ++++++++++
>>  crypto/cfb.c            |  7 +++----
>>  crypto/cipher.c         |  3 ++-
>>  crypto/ctr.c            |  4 ++--
>>  crypto/cts.c            |  5 +++--
>>  crypto/pcbc.c           |  5 +++--
>>  include/crypto/algapi.h |  8 ++++++++
>>  7 files changed, 31 insertions(+), 11 deletions(-)
>
> All applied.  Thanks.

Thank you very much.

Salvatore