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/[email protected]
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
2018-04-09 16:35 GMT+02:00 David Laight <[email protected]>:
> 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
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
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/[email protected]
>
> 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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
2018-04-20 18:51 GMT+02:00 Herbert Xu <[email protected]>:
> 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/[email protected]
>>
>> 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