Hi,
This is nearly the last of the VLA removals[1], but it's one of the
largest because crypto gets used in lots of places. After looking
through code, usage, reading the threads Gustavo started, and comparing
the use-cases to the other VLA removals that have landed in the kernel,
I think this series is likely the best way forward to shut the door on
VLAs forever.
As background, the crypto stack usage is for callers to do an immediate
bit of work that doesn't allocate new memory. This means that other VLA
removal techniques (like just using kmalloc) aren't workable, and the
next common technique is needed: examination of maximum stack usage and
the addition of sanity checks. This series does that, and in several
cases, these maximums were already implicit in the code.
This series is intended to land via the crypto tree, though it touches
dm as well, since there are dependent patches (new crypto #defines
being used).
Thanks!
-Kees
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Kees Cook (11):
crypto: shash: Remove VLA usage
dm integrity: Remove VLA usage
crypto: ahash: Remove VLA usage
dm verity fec: Remove VLA usage
crypto alg: Introduce max blocksize and alignmask
crypto: cbc: Remove VLA usage
crypto: xcbc: Remove VLA usage
crypto: qat: Remove VLA usage
crypto: shash: Remove VLA usage in unaligned hashing
crypto: ahash: Remove VLA usage for AHASH_REQUEST_ON_STACK
crypto: skcipher: Remove VLA usage for SKCIPHER_REQUEST_ON_STACK
crypto/ahash.c | 4 ++--
crypto/algapi.c | 5 ++++-
crypto/algif_hash.c | 2 +-
crypto/shash.c | 27 ++++++++++++------------
crypto/xcbc.c | 5 ++++-
drivers/crypto/qat/qat_common/Makefile | 2 ++
drivers/crypto/qat/qat_common/qat_algs.c | 8 +++++--
drivers/md/dm-integrity.c | 23 ++++++++++++++------
drivers/md/dm-verity-fec.c | 5 ++++-
include/crypto/cbc.h | 2 +-
include/crypto/hash.h | 12 +++++++++--
include/crypto/internal/hash.h | 1 +
include/crypto/internal/skcipher.h | 1 +
include/crypto/skcipher.h | 4 +++-
include/linux/crypto.h | 4 ++++
15 files changed, 73 insertions(+), 32 deletions(-)
--
2.17.1
In the quest to remove all stack VLA usage from the kernel[1], this uses
the new SHASH_MAX_DIGESTSIZE from the crypto layer to allocate the upper
bounds on stack usage.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by: Kees Cook <[email protected]>
---
drivers/md/dm-integrity.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index 86438b2f10dd..d057b41f8690 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -521,7 +521,12 @@ static void section_mac(struct dm_integrity_c *ic, unsigned section, __u8 result
}
memset(result + size, 0, JOURNAL_MAC_SIZE - size);
} else {
- __u8 digest[size];
+ __u8 digest[SHASH_MAX_DIGESTSIZE];
+
+ if (WARN_ON(size > sizeof(digest))) {
+ dm_integrity_io_error(ic, "digest_size", -EINVAL);
+ goto err;
+ }
r = crypto_shash_final(desc, digest);
if (unlikely(r)) {
dm_integrity_io_error(ic, "crypto_shash_final", r);
@@ -1244,7 +1249,7 @@ static void integrity_metadata(struct work_struct *w)
struct bio *bio = dm_bio_from_per_bio_data(dio, sizeof(struct dm_integrity_io));
char *checksums;
unsigned extra_space = unlikely(digest_size > ic->tag_size) ? digest_size - ic->tag_size : 0;
- char checksums_onstack[ic->tag_size + extra_space];
+ char checksums_onstack[SHASH_MAX_DIGESTSIZE];
unsigned sectors_to_process = dio->range.n_sectors;
sector_t sector = dio->range.logical_sector;
@@ -1253,8 +1258,14 @@ static void integrity_metadata(struct work_struct *w)
checksums = kmalloc((PAGE_SIZE >> SECTOR_SHIFT >> ic->sb->log2_sectors_per_block) * ic->tag_size + extra_space,
GFP_NOIO | __GFP_NORETRY | __GFP_NOWARN);
- if (!checksums)
+ if (!checksums) {
checksums = checksums_onstack;
+ if (WARN_ON(extra_space &&
+ digest_size > sizeof(checksums_onstack))) {
+ r = -EINVAL;
+ goto error;
+ }
+ }
__bio_for_each_segment(bv, bio, iter, dio->orig_bi_iter) {
unsigned pos;
@@ -1466,7 +1477,7 @@ static bool __journal_read_write(struct dm_integrity_io *dio, struct bio *bio,
} while (++s < ic->sectors_per_block);
#ifdef INTERNAL_VERIFY
if (ic->internal_hash) {
- char checksums_onstack[max(crypto_shash_digestsize(ic->internal_hash), ic->tag_size)];
+ char checksums_onstack[max(SHASH_MAX_DIGESTSIZE, MAX_TAG_SIZE)];
integrity_sector_checksum(ic, logical_sector, mem + bv.bv_offset, checksums_onstack);
if (unlikely(memcmp(checksums_onstack, journal_entry_tag(ic, je), ic->tag_size))) {
@@ -1516,7 +1527,7 @@ static bool __journal_read_write(struct dm_integrity_io *dio, struct bio *bio,
if (ic->internal_hash) {
unsigned digest_size = crypto_shash_digestsize(ic->internal_hash);
if (unlikely(digest_size > ic->tag_size)) {
- char checksums_onstack[digest_size];
+ char checksums_onstack[SHASH_MAX_DIGESTSIZE];
integrity_sector_checksum(ic, logical_sector, (char *)js, checksums_onstack);
memcpy(journal_entry_tag(ic, je), checksums_onstack, ic->tag_size);
} else
@@ -1937,7 +1948,7 @@ static void do_journal_write(struct dm_integrity_c *ic, unsigned write_start,
unlikely(from_replay) &&
#endif
ic->internal_hash) {
- char test_tag[max(crypto_shash_digestsize(ic->internal_hash), ic->tag_size)];
+ char test_tag[max(SHASH_MAX_DIGESTSIZE, MAX_TAG_SIZE)];
integrity_sector_checksum(ic, sec + ((l - j) << ic->sb->log2_sectors_per_block),
(char *)access_journal_data(ic, i, l), test_tag);
--
2.17.1
In the quest to remove all stack VLA usage from the kernel[1], this uses
the maximum blocksize and adds a sanity check.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by: Kees Cook <[email protected]>
---
crypto/xcbc.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/crypto/xcbc.c b/crypto/xcbc.c
index 25c75af50d3f..016481b1f3ba 100644
--- a/crypto/xcbc.c
+++ b/crypto/xcbc.c
@@ -65,7 +65,10 @@ static int crypto_xcbc_digest_setkey(struct crypto_shash *parent,
int bs = crypto_shash_blocksize(parent);
u8 *consts = PTR_ALIGN(&ctx->ctx[0], alignmask + 1);
int err = 0;
- u8 key1[bs];
+ u8 key1[CRYPTO_ALG_MAX_BLOCKSIZE];
+
+ if (WARN_ON(bs > sizeof(key1)))
+ return -EINVAL;
if ((err = crypto_cipher_setkey(ctx->child, inkey, keylen)))
return err;
--
2.17.1
In the quest to remove all stack VLA usage from the kernel[1], this
caps the skcipher request size similar to other limits and adds a sanity
check at registration.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by: Kees Cook <[email protected]>
---
include/crypto/internal/skcipher.h | 1 +
include/crypto/skcipher.h | 4 +++-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/crypto/internal/skcipher.h b/include/crypto/internal/skcipher.h
index e42f7063f245..5035482cbe68 100644
--- a/include/crypto/internal/skcipher.h
+++ b/include/crypto/internal/skcipher.h
@@ -130,6 +130,7 @@ static inline struct crypto_skcipher *crypto_spawn_skcipher(
static inline void crypto_skcipher_set_reqsize(
struct crypto_skcipher *skcipher, unsigned int reqsize)
{
+ BUG_ON(reqsize > SKCIPHER_MAX_REQSIZE);
skcipher->reqsize = reqsize;
}
diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
index 2f327f090c3e..25294d0089b2 100644
--- a/include/crypto/skcipher.h
+++ b/include/crypto/skcipher.h
@@ -139,9 +139,11 @@ struct skcipher_alg {
struct crypto_alg base;
};
+#define SKCIPHER_MAX_REQSIZE (PAGE_SIZE / 8)
+
#define SKCIPHER_REQUEST_ON_STACK(name, tfm) \
char __##name##_desc[sizeof(struct skcipher_request) + \
- crypto_skcipher_reqsize(tfm)] CRYPTO_MINALIGN_ATTR; \
+ SKCIPHER_MAX_REQSIZE] CRYPTO_MINALIGN_ATTR; \
struct skcipher_request *name = (void *)__##name##_desc
/**
--
2.17.1
In the quest to remove all stack VLA usage from the kernel[1], this
uses the upper bounds on blocksize.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by: Kees Cook <[email protected]>
---
include/crypto/cbc.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/crypto/cbc.h b/include/crypto/cbc.h
index f5b8bfc22e6d..260096bcf99b 100644
--- a/include/crypto/cbc.h
+++ b/include/crypto/cbc.h
@@ -113,7 +113,7 @@ static inline int crypto_cbc_decrypt_inplace(
unsigned int bsize = crypto_skcipher_blocksize(tfm);
unsigned int nbytes = walk->nbytes;
u8 *src = walk->src.virt.addr;
- u8 last_iv[bsize];
+ u8 last_iv[CRYPTO_ALG_MAX_BLOCKSIZE];
/* Start of the last block. */
src += nbytes - (nbytes & (bsize - 1)) - bsize;
--
2.17.1
In the quest to remove all stack VLA usage from the kernel[1], this
exposes the existing upper bound on crypto block sizes for VLA removal,
and introduces a new check for alignmask (current maximum in the kernel
is 63 from manual inspection of all cra_alignmask settings).
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by: Kees Cook <[email protected]>
---
crypto/algapi.c | 5 ++++-
include/linux/crypto.h | 4 ++++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/crypto/algapi.c b/crypto/algapi.c
index c0755cf4f53f..760a412b059c 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -57,7 +57,10 @@ static int crypto_check_alg(struct crypto_alg *alg)
if (alg->cra_alignmask & (alg->cra_alignmask + 1))
return -EINVAL;
- if (alg->cra_blocksize > PAGE_SIZE / 8)
+ if (alg->cra_blocksize > CRYPTO_ALG_MAX_BLOCKSIZE)
+ return -EINVAL;
+
+ if (alg->cra_alignmask > CRYPTO_ALG_MAX_ALIGNMASK)
return -EINVAL;
if (!alg->cra_type && (alg->cra_flags & CRYPTO_ALG_TYPE_MASK) ==
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 6eb06101089f..e76ffcbd5aa6 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -134,6 +134,10 @@
*/
#define CRYPTO_MAX_ALG_NAME 128
+/* Maximum values for registered algorithms. */
+#define CRYPTO_ALG_MAX_BLOCKSIZE (PAGE_SIZE / 8)
+#define CRYPTO_ALG_MAX_ALIGNMASK 63
+
/*
* The macro CRYPTO_MINALIGN_ATTR (along with the void * type in the actual
* declaration) is used to ensure that the crypto_tfm context structure is
--
2.17.1
In the quest to remove all stack VLA usage from the kernel[1], this uses
the upper bound for the stack buffer. Also adds a sanity check. This
additionally raises the stack size limit during the build, to avoid
a compiler warning while keeping it reasonably close to expected stack
size. The warning was just exposing the existing max stack size, so there
is nothing new here; now that it is not hidden in a VLA, the compiler
can see how large it might get.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by: Kees Cook <[email protected]>
---
drivers/crypto/qat/qat_common/Makefile | 2 ++
drivers/crypto/qat/qat_common/qat_algs.c | 8 ++++++--
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/crypto/qat/qat_common/Makefile b/drivers/crypto/qat/qat_common/Makefile
index 47a8e3d8b81a..c2a042023dde 100644
--- a/drivers/crypto/qat/qat_common/Makefile
+++ b/drivers/crypto/qat/qat_common/Makefile
@@ -19,3 +19,5 @@ intel_qat-objs := adf_cfg.o \
intel_qat-$(CONFIG_DEBUG_FS) += adf_transport_debug.o
intel_qat-$(CONFIG_PCI_IOV) += adf_sriov.o adf_pf2vf_msg.o \
adf_vf2pf_msg.o adf_vf_isr.o
+
+CFLAGS_qat_algs.o := $(call cc-option,-Wframe-larger-than=2300)
diff --git a/drivers/crypto/qat/qat_common/qat_algs.c b/drivers/crypto/qat/qat_common/qat_algs.c
index 1138e41d6805..257269126601 100644
--- a/drivers/crypto/qat/qat_common/qat_algs.c
+++ b/drivers/crypto/qat/qat_common/qat_algs.c
@@ -153,8 +153,8 @@ static int qat_alg_do_precomputes(struct icp_qat_hw_auth_algo_blk *hash,
struct sha512_state sha512;
int block_size = crypto_shash_blocksize(ctx->hash_tfm);
int digest_size = crypto_shash_digestsize(ctx->hash_tfm);
- char ipad[block_size];
- char opad[block_size];
+ char ipad[CRYPTO_ALG_MAX_BLOCKSIZE];
+ char opad[CRYPTO_ALG_MAX_BLOCKSIZE];
__be32 *hash_state_out;
__be64 *hash512_state_out;
int i, offset;
@@ -164,6 +164,10 @@ static int qat_alg_do_precomputes(struct icp_qat_hw_auth_algo_blk *hash,
shash->tfm = ctx->hash_tfm;
shash->flags = 0x0;
+ if (WARN_ON(block_size > sizeof(ipad) ||
+ sizeof(ipad) != sizeof(opad)))
+ return -EINVAL;
+
if (auth_keylen > block_size) {
int ret = crypto_shash_digest(shash, auth_key,
auth_keylen, ipad);
--
2.17.1
In the quest to remove all stack VLA usage from the kernel[1], this caps
the ahash request size similar to the other limits and adds a sanity
check at registration.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by: Kees Cook <[email protected]>
---
include/crypto/hash.h | 3 ++-
include/crypto/internal/hash.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/crypto/hash.h b/include/crypto/hash.h
index b9df568dc98e..7d62309baf0b 100644
--- a/include/crypto/hash.h
+++ b/include/crypto/hash.h
@@ -66,10 +66,11 @@ struct ahash_request {
#define AHASH_MAX_DIGESTSIZE (PAGE_SIZE / 8)
#define AHASH_MAX_STATESIZE (PAGE_SIZE / 8)
+#define AHASH_MAX_REQSIZE (PAGE_SIZE / 8)
#define AHASH_REQUEST_ON_STACK(name, ahash) \
char __##name##_desc[sizeof(struct ahash_request) + \
- crypto_ahash_reqsize(ahash)] CRYPTO_MINALIGN_ATTR; \
+ AHASH_MAX_REQSIZE] CRYPTO_MINALIGN_ATTR; \
struct ahash_request *name = (void *)__##name##_desc
/**
diff --git a/include/crypto/internal/hash.h b/include/crypto/internal/hash.h
index a0b0ad9d585e..d96ae5f52125 100644
--- a/include/crypto/internal/hash.h
+++ b/include/crypto/internal/hash.h
@@ -142,6 +142,7 @@ static inline struct ahash_alg *__crypto_ahash_alg(struct crypto_alg *alg)
static inline void crypto_ahash_set_reqsize(struct crypto_ahash *tfm,
unsigned int reqsize)
{
+ BUG_ON(reqsize > AHASH_MAX_REQSIZE);
tfm->reqsize = reqsize;
}
--
2.17.1
In the quest to remove all stack VLA usage from the kernel[1], this uses
the newly defined max alignment to perform unaligned hashing to avoid
VLAs, and drops the helper function while adding sanity checks on the
resulting buffer sizes.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by: Kees Cook <[email protected]>
---
crypto/shash.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/crypto/shash.c b/crypto/shash.c
index ab6902c6dae7..1bb58209330a 100644
--- a/crypto/shash.c
+++ b/crypto/shash.c
@@ -73,13 +73,6 @@ int crypto_shash_setkey(struct crypto_shash *tfm, const u8 *key,
}
EXPORT_SYMBOL_GPL(crypto_shash_setkey);
-static inline unsigned int shash_align_buffer_size(unsigned len,
- unsigned long mask)
-{
- typedef u8 __aligned_largest u8_aligned;
- return len + (mask & ~(__alignof__(u8_aligned) - 1));
-}
-
static int shash_update_unaligned(struct shash_desc *desc, const u8 *data,
unsigned int len)
{
@@ -88,11 +81,14 @@ static int shash_update_unaligned(struct shash_desc *desc, const u8 *data,
unsigned long alignmask = crypto_shash_alignmask(tfm);
unsigned int unaligned_len = alignmask + 1 -
((unsigned long)data & alignmask);
- u8 ubuf[shash_align_buffer_size(unaligned_len, alignmask)]
- __aligned_largest;
+ u8 ubuf[CRYPTO_ALG_MAX_ALIGNMASK]
+ __aligned(CRYPTO_ALG_MAX_ALIGNMASK + 1);
u8 *buf = PTR_ALIGN(&ubuf[0], alignmask + 1);
int err;
+ if (WARN_ON(buf + unaligned_len > ubuf + sizeof(ubuf)))
+ return -EINVAL;
+
if (unaligned_len > len)
unaligned_len = len;
@@ -124,11 +120,14 @@ static int shash_final_unaligned(struct shash_desc *desc, u8 *out)
unsigned long alignmask = crypto_shash_alignmask(tfm);
struct shash_alg *shash = crypto_shash_alg(tfm);
unsigned int ds = crypto_shash_digestsize(tfm);
- u8 ubuf[shash_align_buffer_size(ds, alignmask)]
- __aligned_largest;
+ u8 ubuf[SHASH_MAX_DIGESTSIZE]
+ __aligned(CRYPTO_ALG_MAX_ALIGNMASK + 1);
u8 *buf = PTR_ALIGN(&ubuf[0], alignmask + 1);
int err;
+ if (WARN_ON(buf + ds > ubuf + sizeof(ubuf)))
+ return -EINVAL;
+
err = shash->final(desc, buf);
if (err)
goto out;
--
2.17.1
In the quest to remove all stack VLA usage from the kernel[1], this
removes the VLAs in SHASH_DESC_ON_STACK (via crypto_shash_descsize())
by using the maximum allowable size (which is now more clearly captured
in a macro). Similar limits are turned into macros as well.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by: Kees Cook <[email protected]>
---
crypto/shash.c | 6 +++---
include/crypto/hash.h | 6 +++++-
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/crypto/shash.c b/crypto/shash.c
index 5d732c6bb4b2..ab6902c6dae7 100644
--- a/crypto/shash.c
+++ b/crypto/shash.c
@@ -458,9 +458,9 @@ static int shash_prepare_alg(struct shash_alg *alg)
{
struct crypto_alg *base = &alg->base;
- if (alg->digestsize > PAGE_SIZE / 8 ||
- alg->descsize > PAGE_SIZE / 8 ||
- alg->statesize > PAGE_SIZE / 8)
+ if (alg->digestsize > SHASH_MAX_DIGESTSIZE ||
+ alg->descsize > SHASH_MAX_DESCSIZE ||
+ alg->statesize > SHASH_MAX_STATESIZE)
return -EINVAL;
base->cra_type = &crypto_shash_type;
diff --git a/include/crypto/hash.h b/include/crypto/hash.h
index 76e432cab75d..308aad8bf523 100644
--- a/include/crypto/hash.h
+++ b/include/crypto/hash.h
@@ -151,9 +151,13 @@ struct shash_desc {
void *__ctx[] CRYPTO_MINALIGN_ATTR;
};
+#define SHASH_MAX_DIGESTSIZE (PAGE_SIZE / 8)
+#define SHASH_MAX_DESCSIZE (PAGE_SIZE / 8)
+#define SHASH_MAX_STATESIZE (PAGE_SIZE / 8)
+
#define SHASH_DESC_ON_STACK(shash, ctx) \
char __##shash##_desc[sizeof(struct shash_desc) + \
- crypto_shash_descsize(ctx)] CRYPTO_MINALIGN_ATTR; \
+ SHASH_MAX_DESCSIZE] CRYPTO_MINALIGN_ATTR; \
struct shash_desc *shash = (struct shash_desc *)__##shash##_desc
/**
--
2.17.1
In the quest to remove all stack VLA usage from the kernel[1], this
introduces max size macros for ahash, as already done for shash, and
adjust the crypto user to max state size.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by: Kees Cook <[email protected]>
---
crypto/ahash.c | 4 ++--
crypto/algif_hash.c | 2 +-
include/crypto/hash.h | 3 +++
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/crypto/ahash.c b/crypto/ahash.c
index a64c143165b1..6435bdbe42fd 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -550,8 +550,8 @@ static int ahash_prepare_alg(struct ahash_alg *alg)
{
struct crypto_alg *base = &alg->halg.base;
- if (alg->halg.digestsize > PAGE_SIZE / 8 ||
- alg->halg.statesize > PAGE_SIZE / 8 ||
+ if (alg->halg.digestsize > AHASH_MAX_DIGESTSIZE ||
+ alg->halg.statesize > AHASH_MAX_STATESIZE ||
alg->halg.statesize == 0)
return -EINVAL;
diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
index bfcf595fd8f9..8974ee8ebead 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -239,7 +239,7 @@ static int hash_accept(struct socket *sock, struct socket *newsock, int flags,
struct alg_sock *ask = alg_sk(sk);
struct hash_ctx *ctx = ask->private;
struct ahash_request *req = &ctx->req;
- char state[crypto_ahash_statesize(crypto_ahash_reqtfm(req)) ? : 1];
+ char state[AHASH_MAX_STATESIZE];
struct sock *sk2;
struct alg_sock *ask2;
struct hash_ctx *ctx2;
diff --git a/include/crypto/hash.h b/include/crypto/hash.h
index 308aad8bf523..b9df568dc98e 100644
--- a/include/crypto/hash.h
+++ b/include/crypto/hash.h
@@ -64,6 +64,9 @@ struct ahash_request {
void *__ctx[] CRYPTO_MINALIGN_ATTR;
};
+#define AHASH_MAX_DIGESTSIZE (PAGE_SIZE / 8)
+#define AHASH_MAX_STATESIZE (PAGE_SIZE / 8)
+
#define AHASH_REQUEST_ON_STACK(name, ahash) \
char __##name##_desc[sizeof(struct ahash_request) + \
crypto_ahash_reqsize(ahash)] CRYPTO_MINALIGN_ATTR; \
--
2.17.1
In the quest to remove all stack VLA usage from the kernel[1], this
uses the newly defined max digest size macro. Also adds a sanity-check
at use-time.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by: Kees Cook <[email protected]>
---
drivers/md/dm-verity-fec.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index 684af08d0747..0dfcc52835bc 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -212,12 +212,15 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
struct dm_verity_fec_io *fio = fec_io(io);
u64 block, ileaved;
u8 *bbuf, *rs_block;
- u8 want_digest[v->digest_size];
+ u8 want_digest[AHASH_MAX_DIGESTSIZE];
unsigned n, k;
if (neras)
*neras = 0;
+ if (WARN_ON(v->digest_size < sizeof(want_digest)))
+ return -EINVAL;
+
/*
* read each of the rsn data blocks that are part of the RS block, and
* interleave contents to available bufs
--
2.17.1
On Wed, Jun 20, 2018 at 9:04 PM, Kees Cook <[email protected]> wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this
> caps the skcipher request size similar to other limits and adds a sanity
> check at registration.
>
>
> +#define SKCIPHER_MAX_REQSIZE (PAGE_SIZE / 8)
> +
> #define SKCIPHER_REQUEST_ON_STACK(name, tfm) \
> char __##name##_desc[sizeof(struct skcipher_request) + \
> - crypto_skcipher_reqsize(tfm)] CRYPTO_MINALIGN_ATTR; \
> + SKCIPHER_MAX_REQSIZE] CRYPTO_MINALIGN_ATTR; \
> struct skcipher_request *name = (void *)__##name##_desc
>
This is probably a bad idea on kernels with large values of PAGE_SIZE.
Some users on ppc64 and arm64 use 64KB here, but still limit
the per-function stack size to 2KB.
Arnd
On 06/20/2018 07:03 PM, Kees Cook wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this
> removes the VLAs in SHASH_DESC_ON_STACK (via crypto_shash_descsize())
> by using the maximum allowable size (which is now more clearly captured
> in a macro). Similar limits are turned into macros as well.
>
> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>
> Signed-off-by: Kees Cook <[email protected]>
Got the following warnings:
crypto/testmgr.c: In function ‘alg_test_crc32c.part.4’:
crypto/testmgr.c:1896:1: warning: the frame size of 2088 bytes is larger
than 1024 bytes [-Wframe-larger-than=]
crypto/hmac.c: In function ‘hmac_setkey’:
crypto/hmac.c:88:1: warning: the frame size of 2088 bytes is larger than
1024 bytes [-Wframe-larger-than=]
Christophe
> ---
> crypto/shash.c | 6 +++---
> include/crypto/hash.h | 6 +++++-
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/crypto/shash.c b/crypto/shash.c
> index 5d732c6bb4b2..ab6902c6dae7 100644
> --- a/crypto/shash.c
> +++ b/crypto/shash.c
> @@ -458,9 +458,9 @@ static int shash_prepare_alg(struct shash_alg *alg)
> {
> struct crypto_alg *base = &alg->base;
>
> - if (alg->digestsize > PAGE_SIZE / 8 ||
> - alg->descsize > PAGE_SIZE / 8 ||
> - alg->statesize > PAGE_SIZE / 8)
> + if (alg->digestsize > SHASH_MAX_DIGESTSIZE ||
> + alg->descsize > SHASH_MAX_DESCSIZE ||
> + alg->statesize > SHASH_MAX_STATESIZE)
> return -EINVAL;
>
> base->cra_type = &crypto_shash_type;
> diff --git a/include/crypto/hash.h b/include/crypto/hash.h
> index 76e432cab75d..308aad8bf523 100644
> --- a/include/crypto/hash.h
> +++ b/include/crypto/hash.h
> @@ -151,9 +151,13 @@ struct shash_desc {
> void *__ctx[] CRYPTO_MINALIGN_ATTR;
> };
>
> +#define SHASH_MAX_DIGESTSIZE (PAGE_SIZE / 8)
> +#define SHASH_MAX_DESCSIZE (PAGE_SIZE / 8)
> +#define SHASH_MAX_STATESIZE (PAGE_SIZE / 8)
> +
> #define SHASH_DESC_ON_STACK(shash, ctx) \
> char __##shash##_desc[sizeof(struct shash_desc) + \
> - crypto_shash_descsize(ctx)] CRYPTO_MINALIGN_ATTR; \
> + SHASH_MAX_DESCSIZE] CRYPTO_MINALIGN_ATTR; \
> struct shash_desc *shash = (struct shash_desc *)__##shash##_desc
>
> /**
>
On 06/20/2018 07:04 PM, Kees Cook wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this
> introduces max size macros for ahash, as already done for shash, and
> adjust the crypto user to max state size.
>
> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>
> Signed-off-by: Kees Cook <[email protected]>
I get:
crypto/algif_hash.c: In function ‘hash_accept’:
crypto/algif_hash.c:276:1: warning: the frame size of 2048 bytes is
larger than 1024 bytes [-Wframe-larger-than=]
Christophe
> ---
> crypto/ahash.c | 4 ++--
> crypto/algif_hash.c | 2 +-
> include/crypto/hash.h | 3 +++
> 3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/crypto/ahash.c b/crypto/ahash.c
> index a64c143165b1..6435bdbe42fd 100644
> --- a/crypto/ahash.c
> +++ b/crypto/ahash.c
> @@ -550,8 +550,8 @@ static int ahash_prepare_alg(struct ahash_alg *alg)
> {
> struct crypto_alg *base = &alg->halg.base;
>
> - if (alg->halg.digestsize > PAGE_SIZE / 8 ||
> - alg->halg.statesize > PAGE_SIZE / 8 ||
> + if (alg->halg.digestsize > AHASH_MAX_DIGESTSIZE ||
> + alg->halg.statesize > AHASH_MAX_STATESIZE ||
> alg->halg.statesize == 0)
> return -EINVAL;
>
> diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
> index bfcf595fd8f9..8974ee8ebead 100644
> --- a/crypto/algif_hash.c
> +++ b/crypto/algif_hash.c
> @@ -239,7 +239,7 @@ static int hash_accept(struct socket *sock, struct socket *newsock, int flags,
> struct alg_sock *ask = alg_sk(sk);
> struct hash_ctx *ctx = ask->private;
> struct ahash_request *req = &ctx->req;
> - char state[crypto_ahash_statesize(crypto_ahash_reqtfm(req)) ? : 1];
> + char state[AHASH_MAX_STATESIZE];
> struct sock *sk2;
> struct alg_sock *ask2;
> struct hash_ctx *ctx2;
> diff --git a/include/crypto/hash.h b/include/crypto/hash.h
> index 308aad8bf523..b9df568dc98e 100644
> --- a/include/crypto/hash.h
> +++ b/include/crypto/hash.h
> @@ -64,6 +64,9 @@ struct ahash_request {
> void *__ctx[] CRYPTO_MINALIGN_ATTR;
> };
>
> +#define AHASH_MAX_DIGESTSIZE (PAGE_SIZE / 8)
> +#define AHASH_MAX_STATESIZE (PAGE_SIZE / 8)
> +
> #define AHASH_REQUEST_ON_STACK(name, ahash) \
> char __##name##_desc[sizeof(struct ahash_request) + \
> crypto_ahash_reqsize(ahash)] CRYPTO_MINALIGN_ATTR; \
>
On 06/20/2018 07:04 PM, Kees Cook wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this
> uses the upper bounds on blocksize.
>
> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>
> Signed-off-by: Kees Cook <[email protected]>
crypto/cbc.c: In function ‘crypto_cbc_decrypt’:
crypto/cbc.c:79:1: warning: the frame size of 2144 bytes is larger than
1024 bytes [-Wframe-larger-than=]
Christophe
> ---
> include/crypto/cbc.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/crypto/cbc.h b/include/crypto/cbc.h
> index f5b8bfc22e6d..260096bcf99b 100644
> --- a/include/crypto/cbc.h
> +++ b/include/crypto/cbc.h
> @@ -113,7 +113,7 @@ static inline int crypto_cbc_decrypt_inplace(
> unsigned int bsize = crypto_skcipher_blocksize(tfm);
> unsigned int nbytes = walk->nbytes;
> u8 *src = walk->src.virt.addr;
> - u8 last_iv[bsize];
> + u8 last_iv[CRYPTO_ALG_MAX_BLOCKSIZE];
>
> /* Start of the last block. */
> src += nbytes - (nbytes & (bsize - 1)) - bsize;
>
On 06/20/2018 07:04 PM, Kees Cook wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this
> caps the skcipher request size similar to other limits and adds a sanity
> check at registration.
>
> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>
> Signed-off-by: Kees Cook <[email protected]>
crypto/echainiv.c: In function ‘echainiv_encrypt’:
crypto/echainiv.c:88:1: warning: the frame size of 2120 bytes is larger
than 1024 bytes [-Wframe-larger-than=]
crypto/authenc.c: In function ‘crypto_authenc_copy_assoc’:
crypto/authenc.c:197:1: warning: the frame size of 2120 bytes is larger
than 1024 bytes [-Wframe-larger-than=]
crypto/authencesn.c: In function ‘crypto_authenc_esn_copy’:
crypto/authencesn.c:194:1: warning: the frame size of 2120 bytes is
larger than 1024 bytes [-Wframe-larger-than=]
crypto/algif_aead.c: In function ‘crypto_aead_copy_sgl’:
crypto/algif_aead.c:90:1: warning: the frame size of 2120 bytes is
larger than 1024 bytes [-Wframe-larger-than=]
Christophe
> ---
> include/crypto/internal/skcipher.h | 1 +
> include/crypto/skcipher.h | 4 +++-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/crypto/internal/skcipher.h b/include/crypto/internal/skcipher.h
> index e42f7063f245..5035482cbe68 100644
> --- a/include/crypto/internal/skcipher.h
> +++ b/include/crypto/internal/skcipher.h
> @@ -130,6 +130,7 @@ static inline struct crypto_skcipher *crypto_spawn_skcipher(
> static inline void crypto_skcipher_set_reqsize(
> struct crypto_skcipher *skcipher, unsigned int reqsize)
> {
> + BUG_ON(reqsize > SKCIPHER_MAX_REQSIZE);
> skcipher->reqsize = reqsize;
> }
>
> diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
> index 2f327f090c3e..25294d0089b2 100644
> --- a/include/crypto/skcipher.h
> +++ b/include/crypto/skcipher.h
> @@ -139,9 +139,11 @@ struct skcipher_alg {
> struct crypto_alg base;
> };
>
> +#define SKCIPHER_MAX_REQSIZE (PAGE_SIZE / 8)
> +
> #define SKCIPHER_REQUEST_ON_STACK(name, tfm) \
> char __##name##_desc[sizeof(struct skcipher_request) + \
> - crypto_skcipher_reqsize(tfm)] CRYPTO_MINALIGN_ATTR; \
> + SKCIPHER_MAX_REQSIZE] CRYPTO_MINALIGN_ATTR; \
> struct skcipher_request *name = (void *)__##name##_desc
>
> /**
>
On Wed, Jun 20, 2018 at 12:30 PM, Christophe Leroy
<[email protected]> wrote:
>
>
> On 06/20/2018 07:03 PM, Kees Cook wrote:
>>
>> In the quest to remove all stack VLA usage from the kernel[1], this
>> removes the VLAs in SHASH_DESC_ON_STACK (via crypto_shash_descsize())
>> by using the maximum allowable size (which is now more clearly captured
>> in a macro). Similar limits are turned into macros as well.
>>
>> [1]
>> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>>
>> Signed-off-by: Kees Cook <[email protected]>
>
>
> Got the following warnings:
>
> crypto/testmgr.c: In function ‘alg_test_crc32c.part.4’:
> crypto/testmgr.c:1896:1: warning: the frame size of 2088 bytes is larger
> than 1024 bytes [-Wframe-larger-than=]
> crypto/hmac.c: In function ‘hmac_setkey’:
> crypto/hmac.c:88:1: warning: the frame size of 2088 bytes is larger than
> 1024 bytes [-Wframe-larger-than=]
Ah yes. I didn't do 32-bit builds. So, here's the issue: this uncovers
the frame size problems that were hidden by in being a VLA before. It
was always possible for the frame to get this big, it's just that the
compiler couldn't see it.
For qat, I raised the -Wframe-larger-than flag. It seems we'll need to
do this in some other places too.
-Kees
--
Kees Cook
Pixel Security
On Wed, Jun 20, 2018 at 12:44 PM, Arnd Bergmann <[email protected]> wrote:
> On Wed, Jun 20, 2018 at 9:04 PM, Kees Cook <[email protected]> wrote:
>> In the quest to remove all stack VLA usage from the kernel[1], this
>> caps the skcipher request size similar to other limits and adds a sanity
>> check at registration.
>>
>>
>> +#define SKCIPHER_MAX_REQSIZE (PAGE_SIZE / 8)
>> +
>> #define SKCIPHER_REQUEST_ON_STACK(name, tfm) \
>> char __##name##_desc[sizeof(struct skcipher_request) + \
>> - crypto_skcipher_reqsize(tfm)] CRYPTO_MINALIGN_ATTR; \
>> + SKCIPHER_MAX_REQSIZE] CRYPTO_MINALIGN_ATTR; \
>> struct skcipher_request *name = (void *)__##name##_desc
>>
>
> This is probably a bad idea on kernels with large values of PAGE_SIZE.
> Some users on ppc64 and arm64 use 64KB here, but still limit
> the per-function stack size to 2KB.
We could make all of these PAGE_SIZE-related limits explicitly 512? I
think that was the intent originally.
-Kees
--
Kees Cook
Pixel Security
Le 20/06/2018 à 22:36, Kees Cook a écrit :
> On Wed, Jun 20, 2018 at 12:30 PM, Christophe Leroy
> <[email protected]> wrote:
>>
>>
>> On 06/20/2018 07:03 PM, Kees Cook wrote:
>>>
>>> In the quest to remove all stack VLA usage from the kernel[1], this
>>> removes the VLAs in SHASH_DESC_ON_STACK (via crypto_shash_descsize())
>>> by using the maximum allowable size (which is now more clearly captured
>>> in a macro). Similar limits are turned into macros as well.
>>>
>>> [1]
>>> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>>>
>>> Signed-off-by: Kees Cook <[email protected]>
>>
>>
>> Got the following warnings:
>>
>> crypto/testmgr.c: In function ‘alg_test_crc32c.part.4’:
>> crypto/testmgr.c:1896:1: warning: the frame size of 2088 bytes is larger
>> than 1024 bytes [-Wframe-larger-than=]
>> crypto/hmac.c: In function ‘hmac_setkey’:
>> crypto/hmac.c:88:1: warning: the frame size of 2088 bytes is larger than
>> 1024 bytes [-Wframe-larger-than=]
>
> Ah yes. I didn't do 32-bit builds. So, here's the issue: this uncovers
> the frame size problems that were hidden by in being a VLA before. It
> was always possible for the frame to get this big, it's just that the
> compiler couldn't see it.
>
> For qat, I raised the -Wframe-larger-than flag. It seems we'll need to
> do this in some other places too.
Maybe the issue is because I have selected 16k pages ?
Christophe
>
> -Kees
>
On Wed, Jun 20, 2018 at 1:39 PM, Christophe LEROY
<[email protected]> wrote:
>
>
> Le 20/06/2018 à 22:36, Kees Cook a écrit :
>>
>> On Wed, Jun 20, 2018 at 12:30 PM, Christophe Leroy
>> <[email protected]> wrote:
>>>
>>>
>>>
>>> On 06/20/2018 07:03 PM, Kees Cook wrote:
>>>>
>>>>
>>>> In the quest to remove all stack VLA usage from the kernel[1], this
>>>> removes the VLAs in SHASH_DESC_ON_STACK (via crypto_shash_descsize())
>>>> by using the maximum allowable size (which is now more clearly captured
>>>> in a macro). Similar limits are turned into macros as well.
>>>>
>>>> [1]
>>>>
>>>> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>>>>
>>>> Signed-off-by: Kees Cook <[email protected]>
>>>
>>>
>>>
>>> Got the following warnings:
>>>
>>> crypto/testmgr.c: In function ‘alg_test_crc32c.part.4’:
>>> crypto/testmgr.c:1896:1: warning: the frame size of 2088 bytes is larger
>>> than 1024 bytes [-Wframe-larger-than=]
>>> crypto/hmac.c: In function ‘hmac_setkey’:
>>> crypto/hmac.c:88:1: warning: the frame size of 2088 bytes is larger than
>>> 1024 bytes [-Wframe-larger-than=]
>>
>>
>> Ah yes. I didn't do 32-bit builds. So, here's the issue: this uncovers
>> the frame size problems that were hidden by in being a VLA before. It
>> was always possible for the frame to get this big, it's just that the
>> compiler couldn't see it.
>>
>> For qat, I raised the -Wframe-larger-than flag. It seems we'll need to
>> do this in some other places too.
>
>
> Maybe the issue is because I have selected 16k pages ?
That would do it! And that's exactly the problem Arnd mentioned. For
v2, I will switch all the PAGE_SIZE-related limits to an explicit 512.
(And I'll do some 32-bit builds too to see if any other cases pop up
that I need to mask out.)
-Kees
--
Kees Cook
Pixel Security
On Wed, Jun 20, 2018 at 12:04:01PM -0700, Kees Cook wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this
> uses the newly defined max digest size macro. Also adds a sanity-check
> at use-time.
>
> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> drivers/md/dm-verity-fec.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
> index 684af08d0747..0dfcc52835bc 100644
> --- a/drivers/md/dm-verity-fec.c
> +++ b/drivers/md/dm-verity-fec.c
> @@ -212,12 +212,15 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
> struct dm_verity_fec_io *fio = fec_io(io);
> u64 block, ileaved;
> u8 *bbuf, *rs_block;
> - u8 want_digest[v->digest_size];
> + u8 want_digest[AHASH_MAX_DIGESTSIZE];
> unsigned n, k;
>
> if (neras)
> *neras = 0;
>
> + if (WARN_ON(v->digest_size < sizeof(want_digest)))
> + return -EINVAL;
> +
This is backwards; it should be 'v->digest_size > sizeof(want_digest)'.
- Eric
On Wed, Jun 20, 2018 at 12:04:02PM -0700, Kees Cook wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this
> exposes the existing upper bound on crypto block sizes for VLA removal,
> and introduces a new check for alignmask (current maximum in the kernel
> is 63 from manual inspection of all cra_alignmask settings).
>
> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> crypto/algapi.c | 5 ++++-
> include/linux/crypto.h | 4 ++++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index c0755cf4f53f..760a412b059c 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -57,7 +57,10 @@ static int crypto_check_alg(struct crypto_alg *alg)
> if (alg->cra_alignmask & (alg->cra_alignmask + 1))
> return -EINVAL;
>
> - if (alg->cra_blocksize > PAGE_SIZE / 8)
> + if (alg->cra_blocksize > CRYPTO_ALG_MAX_BLOCKSIZE)
> + return -EINVAL;
> +
> + if (alg->cra_alignmask > CRYPTO_ALG_MAX_ALIGNMASK)
> return -EINVAL;
>
> if (!alg->cra_type && (alg->cra_flags & CRYPTO_ALG_TYPE_MASK) ==
> diff --git a/include/linux/crypto.h b/include/linux/crypto.h
> index 6eb06101089f..e76ffcbd5aa6 100644
> --- a/include/linux/crypto.h
> +++ b/include/linux/crypto.h
> @@ -134,6 +134,10 @@
> */
> #define CRYPTO_MAX_ALG_NAME 128
>
> +/* Maximum values for registered algorithms. */
> +#define CRYPTO_ALG_MAX_BLOCKSIZE (PAGE_SIZE / 8)
> +#define CRYPTO_ALG_MAX_ALIGNMASK 63
> +
How do these differ from MAX_CIPHER_BLOCKSIZE and MAX_CIPHER_ALIGNMASK, and why
are they declared in different places?
- Eric
On Wed, Jun 20, 2018 at 12:04:03PM -0700, Kees Cook wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this
> uses the upper bounds on blocksize.
>
> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> include/crypto/cbc.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/crypto/cbc.h b/include/crypto/cbc.h
> index f5b8bfc22e6d..260096bcf99b 100644
> --- a/include/crypto/cbc.h
> +++ b/include/crypto/cbc.h
> @@ -113,7 +113,7 @@ static inline int crypto_cbc_decrypt_inplace(
> unsigned int bsize = crypto_skcipher_blocksize(tfm);
> unsigned int nbytes = walk->nbytes;
> u8 *src = walk->src.virt.addr;
> - u8 last_iv[bsize];
> + u8 last_iv[CRYPTO_ALG_MAX_BLOCKSIZE];
>
Why not MAX_CIPHER_BLOCKSIZE?
- Eric
On Wed, Jun 20, 2018 at 4:33 PM, Eric Biggers <[email protected]> wrote:
> On Wed, Jun 20, 2018 at 12:04:01PM -0700, Kees Cook wrote:
>> In the quest to remove all stack VLA usage from the kernel[1], this
>> uses the newly defined max digest size macro. Also adds a sanity-check
>> at use-time.
>>
>> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>>
>> Signed-off-by: Kees Cook <[email protected]>
>> ---
>> drivers/md/dm-verity-fec.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
>> index 684af08d0747..0dfcc52835bc 100644
>> --- a/drivers/md/dm-verity-fec.c
>> +++ b/drivers/md/dm-verity-fec.c
>> @@ -212,12 +212,15 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
>> struct dm_verity_fec_io *fio = fec_io(io);
>> u64 block, ileaved;
>> u8 *bbuf, *rs_block;
>> - u8 want_digest[v->digest_size];
>> + u8 want_digest[AHASH_MAX_DIGESTSIZE];
>> unsigned n, k;
>>
>> if (neras)
>> *neras = 0;
>>
>> + if (WARN_ON(v->digest_size < sizeof(want_digest)))
>> + return -EINVAL;
>> +
>
> This is backwards; it should be 'v->digest_size > sizeof(want_digest)'.
Yikes. Thank you for catching that! I will fix in v2.
-Kees
--
Kees Cook
Pixel Security
On Wed, Jun 20, 2018 at 12:04:04PM -0700, Kees Cook wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this uses
> the maximum blocksize and adds a sanity check.
>
> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> crypto/xcbc.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/crypto/xcbc.c b/crypto/xcbc.c
> index 25c75af50d3f..016481b1f3ba 100644
> --- a/crypto/xcbc.c
> +++ b/crypto/xcbc.c
> @@ -65,7 +65,10 @@ static int crypto_xcbc_digest_setkey(struct crypto_shash *parent,
> int bs = crypto_shash_blocksize(parent);
> u8 *consts = PTR_ALIGN(&ctx->ctx[0], alignmask + 1);
> int err = 0;
> - u8 key1[bs];
> + u8 key1[CRYPTO_ALG_MAX_BLOCKSIZE];
> +
> + if (WARN_ON(bs > sizeof(key1)))
> + return -EINVAL;
Similarly, why not MAX_CIPHER_BLOCKSIZE?
Also, xcbc_create() only allows a 16-byte block size, and you made the API
enforce the maximum for algorithms anyway. So I think the extra check here
isn't very worthwhile.
- Eric
On Wed, Jun 20, 2018 at 12:04:06PM -0700, Kees Cook wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this uses
> the newly defined max alignment to perform unaligned hashing to avoid
> VLAs, and drops the helper function while adding sanity checks on the
> resulting buffer sizes.
>
> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> crypto/shash.c | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/crypto/shash.c b/crypto/shash.c
> index ab6902c6dae7..1bb58209330a 100644
> --- a/crypto/shash.c
> +++ b/crypto/shash.c
> @@ -73,13 +73,6 @@ int crypto_shash_setkey(struct crypto_shash *tfm, const u8 *key,
> }
> EXPORT_SYMBOL_GPL(crypto_shash_setkey);
>
> -static inline unsigned int shash_align_buffer_size(unsigned len,
> - unsigned long mask)
> -{
> - typedef u8 __aligned_largest u8_aligned;
> - return len + (mask & ~(__alignof__(u8_aligned) - 1));
> -}
> -
> static int shash_update_unaligned(struct shash_desc *desc, const u8 *data,
> unsigned int len)
> {
> @@ -88,11 +81,14 @@ static int shash_update_unaligned(struct shash_desc *desc, const u8 *data,
> unsigned long alignmask = crypto_shash_alignmask(tfm);
> unsigned int unaligned_len = alignmask + 1 -
> ((unsigned long)data & alignmask);
> - u8 ubuf[shash_align_buffer_size(unaligned_len, alignmask)]
> - __aligned_largest;
> + u8 ubuf[CRYPTO_ALG_MAX_ALIGNMASK]
> + __aligned(CRYPTO_ALG_MAX_ALIGNMASK + 1);
> u8 *buf = PTR_ALIGN(&ubuf[0], alignmask + 1);
> int err;
Are you sure that __attribute__((aligned(64))) works correctly on stack
variables on all architectures?
And if it is expected to work, then why is the buffer still aligned by hand on
the very next line?
>
> + if (WARN_ON(buf + unaligned_len > ubuf + sizeof(ubuf)))
> + return -EINVAL;
> +
> if (unaligned_len > len)
> unaligned_len = len;
>
> @@ -124,11 +120,14 @@ static int shash_final_unaligned(struct shash_desc *desc, u8 *out)
> unsigned long alignmask = crypto_shash_alignmask(tfm);
> struct shash_alg *shash = crypto_shash_alg(tfm);
> unsigned int ds = crypto_shash_digestsize(tfm);
> - u8 ubuf[shash_align_buffer_size(ds, alignmask)]
> - __aligned_largest;
> + u8 ubuf[SHASH_MAX_DIGESTSIZE]
> + __aligned(CRYPTO_ALG_MAX_ALIGNMASK + 1);
> u8 *buf = PTR_ALIGN(&ubuf[0], alignmask + 1);
> int err;
Same questions here.
>
> + if (WARN_ON(buf + ds > ubuf + sizeof(ubuf)))
> + return -EINVAL;
> +
> err = shash->final(desc, buf);
> if (err)
> goto out;
> --
- Eric
On Wed, Jun 20, 2018 at 4:40 PM, Eric Biggers <[email protected]> wrote:
> On Wed, Jun 20, 2018 at 12:04:02PM -0700, Kees Cook wrote:
>> In the quest to remove all stack VLA usage from the kernel[1], this
>> exposes the existing upper bound on crypto block sizes for VLA removal,
>> and introduces a new check for alignmask (current maximum in the kernel
>> is 63 from manual inspection of all cra_alignmask settings).
>>
>> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>>
>> Signed-off-by: Kees Cook <[email protected]>
>> ---
>> crypto/algapi.c | 5 ++++-
>> include/linux/crypto.h | 4 ++++
>> 2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/crypto/algapi.c b/crypto/algapi.c
>> index c0755cf4f53f..760a412b059c 100644
>> --- a/crypto/algapi.c
>> +++ b/crypto/algapi.c
>> @@ -57,7 +57,10 @@ static int crypto_check_alg(struct crypto_alg *alg)
>> if (alg->cra_alignmask & (alg->cra_alignmask + 1))
>> return -EINVAL;
>>
>> - if (alg->cra_blocksize > PAGE_SIZE / 8)
>> + if (alg->cra_blocksize > CRYPTO_ALG_MAX_BLOCKSIZE)
>> + return -EINVAL;
>> +
>> + if (alg->cra_alignmask > CRYPTO_ALG_MAX_ALIGNMASK)
>> return -EINVAL;
>>
>> if (!alg->cra_type && (alg->cra_flags & CRYPTO_ALG_TYPE_MASK) ==
>> diff --git a/include/linux/crypto.h b/include/linux/crypto.h
>> index 6eb06101089f..e76ffcbd5aa6 100644
>> --- a/include/linux/crypto.h
>> +++ b/include/linux/crypto.h
>> @@ -134,6 +134,10 @@
>> */
>> #define CRYPTO_MAX_ALG_NAME 128
>>
>> +/* Maximum values for registered algorithms. */
>> +#define CRYPTO_ALG_MAX_BLOCKSIZE (PAGE_SIZE / 8)
>> +#define CRYPTO_ALG_MAX_ALIGNMASK 63
>> +
>
> How do these differ from MAX_CIPHER_BLOCKSIZE and MAX_CIPHER_ALIGNMASK, and why
> are they declared in different places?
This is what I get for staring at crypto code for so long. I entirely
missed these checks... even though they're 8 line away:
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;
}
However, this is only checking CRYPTO_ALG_TYPE_CIPHER, and
cra_blocksize can be used for all kinds of things.
include/crypto/algapi.h:#define MAX_CIPHER_ALIGNMASK 15
...
drivers/crypto/mxs-dcp.c: .cra_flags
= CRYPTO_ALG_ASYNC,
drivers/crypto/mxs-dcp.c: .cra_alignmask = 63,
Is this one broken? It has no CRYPTO_ALG_TYPE_... ?
For my CRYPTO_ALG_MAX_BLOCKSIZE, there is:
crypto/xcbc.c: u8 key1[CRYPTO_ALG_MAX_BLOCKSIZE];
drivers/crypto/qat/qat_common/qat_algs.c: char
ipad[CRYPTO_ALG_MAX_BLOCKSIZE];
drivers/crypto/qat/qat_common/qat_algs.c: char
opad[CRYPTO_ALG_MAX_BLOCKSIZE];
It looks like both xcbc and qat are used with shash, so that needs a
separate max blocksize.
For my CRYPTO_ALG_MAX_ALIGNMASK, there is:
crypto/shash.c: u8 ubuf[CRYPTO_ALG_MAX_ALIGNMASK]
crypto/shash.c: __aligned(CRYPTO_ALG_MAX_ALIGNMASK + 1);
crypto/shash.c: __aligned(CRYPTO_ALG_MAX_ALIGNMASK + 1);
which is also shash.
How should I rename these and best apply the registration-time sanity checks?
-Kees
--
Kees Cook
Pixel Security
On Wed, Jun 20, 2018 at 4:46 PM, Eric Biggers <[email protected]> wrote:
> On Wed, Jun 20, 2018 at 12:04:04PM -0700, Kees Cook wrote:
>> In the quest to remove all stack VLA usage from the kernel[1], this uses
>> the maximum blocksize and adds a sanity check.
>>
>> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>>
>> Signed-off-by: Kees Cook <[email protected]>
>> ---
>> crypto/xcbc.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/crypto/xcbc.c b/crypto/xcbc.c
>> index 25c75af50d3f..016481b1f3ba 100644
>> --- a/crypto/xcbc.c
>> +++ b/crypto/xcbc.c
>> @@ -65,7 +65,10 @@ static int crypto_xcbc_digest_setkey(struct crypto_shash *parent,
>> int bs = crypto_shash_blocksize(parent);
>> u8 *consts = PTR_ALIGN(&ctx->ctx[0], alignmask + 1);
>> int err = 0;
>> - u8 key1[bs];
>> + u8 key1[CRYPTO_ALG_MAX_BLOCKSIZE];
>> +
>> + if (WARN_ON(bs > sizeof(key1)))
>> + return -EINVAL;
>
> Similarly, why not MAX_CIPHER_BLOCKSIZE?
>
> Also, xcbc_create() only allows a 16-byte block size, and you made the API
> enforce the maximum for algorithms anyway. So I think the extra check here
> isn't very worthwhile.
Is the "parent" argument in crypto_xcbc_digest_setkey() always going
to be the "alg" from xcbc_create()? I couldn't convince myself that
was true. If it is, then yes, this VLA can trivially made to be 16,
but it seemed like they were separate instances...
-Kees
--
Kees Cook
Pixel Security
On Wed, Jun 20, 2018 at 4:57 PM, Eric Biggers <[email protected]> wrote:
> On Wed, Jun 20, 2018 at 12:04:06PM -0700, Kees Cook wrote:
>> In the quest to remove all stack VLA usage from the kernel[1], this uses
>> the newly defined max alignment to perform unaligned hashing to avoid
>> VLAs, and drops the helper function while adding sanity checks on the
>> resulting buffer sizes.
>>
>> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>>
>> Signed-off-by: Kees Cook <[email protected]>
>> ---
>> crypto/shash.c | 21 ++++++++++-----------
>> 1 file changed, 10 insertions(+), 11 deletions(-)
>>
>> diff --git a/crypto/shash.c b/crypto/shash.c
>> index ab6902c6dae7..1bb58209330a 100644
>> --- a/crypto/shash.c
>> +++ b/crypto/shash.c
>> @@ -73,13 +73,6 @@ int crypto_shash_setkey(struct crypto_shash *tfm, const u8 *key,
>> }
>> EXPORT_SYMBOL_GPL(crypto_shash_setkey);
>>
>> -static inline unsigned int shash_align_buffer_size(unsigned len,
>> - unsigned long mask)
>> -{
>> - typedef u8 __aligned_largest u8_aligned;
>> - return len + (mask & ~(__alignof__(u8_aligned) - 1));
>> -}
>> -
>> static int shash_update_unaligned(struct shash_desc *desc, const u8 *data,
>> unsigned int len)
>> {
>> @@ -88,11 +81,14 @@ static int shash_update_unaligned(struct shash_desc *desc, const u8 *data,
>> unsigned long alignmask = crypto_shash_alignmask(tfm);
>> unsigned int unaligned_len = alignmask + 1 -
>> ((unsigned long)data & alignmask);
>> - u8 ubuf[shash_align_buffer_size(unaligned_len, alignmask)]
>> - __aligned_largest;
>> + u8 ubuf[CRYPTO_ALG_MAX_ALIGNMASK]
>> + __aligned(CRYPTO_ALG_MAX_ALIGNMASK + 1);
>> u8 *buf = PTR_ALIGN(&ubuf[0], alignmask + 1);
>> int err;
>
> Are you sure that __attribute__((aligned(64))) works correctly on stack
> variables on all architectures?
>
> And if it is expected to work, then why is the buffer still aligned by hand on
> the very next line?
I really don't know -- the existing code was doing both the __align
bit and the manual alignment, so I was trying to simplify it while
removing the VLA. I'm totally open to suggestions here.
BTW, these are also the only users of __aligned_largest() in the
kernel, and the only use of unsized __attribute__((aligned))
-Kees
--
Kees Cook
Pixel Security
On Wed, Jun 20, 2018 at 05:04:08PM -0700, Kees Cook wrote:
> On Wed, Jun 20, 2018 at 4:40 PM, Eric Biggers <[email protected]> wrote:
> > On Wed, Jun 20, 2018 at 12:04:02PM -0700, Kees Cook wrote:
> >> In the quest to remove all stack VLA usage from the kernel[1], this
> >> exposes the existing upper bound on crypto block sizes for VLA removal,
> >> and introduces a new check for alignmask (current maximum in the kernel
> >> is 63 from manual inspection of all cra_alignmask settings).
> >>
> >> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
> >>
> >> Signed-off-by: Kees Cook <[email protected]>
> >> ---
> >> crypto/algapi.c | 5 ++++-
> >> include/linux/crypto.h | 4 ++++
> >> 2 files changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/crypto/algapi.c b/crypto/algapi.c
> >> index c0755cf4f53f..760a412b059c 100644
> >> --- a/crypto/algapi.c
> >> +++ b/crypto/algapi.c
> >> @@ -57,7 +57,10 @@ static int crypto_check_alg(struct crypto_alg *alg)
> >> if (alg->cra_alignmask & (alg->cra_alignmask + 1))
> >> return -EINVAL;
> >>
> >> - if (alg->cra_blocksize > PAGE_SIZE / 8)
> >> + if (alg->cra_blocksize > CRYPTO_ALG_MAX_BLOCKSIZE)
> >> + return -EINVAL;
> >> +
> >> + if (alg->cra_alignmask > CRYPTO_ALG_MAX_ALIGNMASK)
> >> return -EINVAL;
> >>
> >> if (!alg->cra_type && (alg->cra_flags & CRYPTO_ALG_TYPE_MASK) ==
> >> diff --git a/include/linux/crypto.h b/include/linux/crypto.h
> >> index 6eb06101089f..e76ffcbd5aa6 100644
> >> --- a/include/linux/crypto.h
> >> +++ b/include/linux/crypto.h
> >> @@ -134,6 +134,10 @@
> >> */
> >> #define CRYPTO_MAX_ALG_NAME 128
> >>
> >> +/* Maximum values for registered algorithms. */
> >> +#define CRYPTO_ALG_MAX_BLOCKSIZE (PAGE_SIZE / 8)
> >> +#define CRYPTO_ALG_MAX_ALIGNMASK 63
> >> +
> >
> > How do these differ from MAX_CIPHER_BLOCKSIZE and MAX_CIPHER_ALIGNMASK, and why
> > are they declared in different places?
>
> This is what I get for staring at crypto code for so long. I entirely
> missed these checks... even though they're 8 line away:
>
> 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;
> }
>
> However, this is only checking CRYPTO_ALG_TYPE_CIPHER, and
> cra_blocksize can be used for all kinds of things.
>
It's overloaded for different purposes, depending on the type of algorithm.
It's poorly documented, but the uses I see are:
(1) Block size for "ciphers", i.e. what the rest of the world calls "block ciphers".
(2) Minimum input size for "skciphers" -- usually either 1 or the block size of
the underlying block cipher, in the case that the skcipher is something like
"cbc(aes)", where a block cipher is wrapped in a mode of operation.
(3) Block size for hash functions that use an internal compression function,
e.g. SHA-1 has a block size of 64 bytes.
I'm not sure it makes sense to have a single limit for all these uses. All the
block ciphers supported by Linux have a block size of 16 bytes or less, while
hash functions usually have larger "block sizes".
> include/crypto/algapi.h:#define MAX_CIPHER_ALIGNMASK 15
> ...
> drivers/crypto/mxs-dcp.c: .cra_flags
> = CRYPTO_ALG_ASYNC,
> drivers/crypto/mxs-dcp.c: .cra_alignmask = 63,
>
> Is this one broken? It has no CRYPTO_ALG_TYPE_... ?
>
> For my CRYPTO_ALG_MAX_BLOCKSIZE, there is:
>
> crypto/xcbc.c: u8 key1[CRYPTO_ALG_MAX_BLOCKSIZE];
> drivers/crypto/qat/qat_common/qat_algs.c: char
> ipad[CRYPTO_ALG_MAX_BLOCKSIZE];
> drivers/crypto/qat/qat_common/qat_algs.c: char
> opad[CRYPTO_ALG_MAX_BLOCKSIZE];
>
> It looks like both xcbc and qat are used with shash, so that needs a
> separate max blocksize.
Actually, xcbc is a 'shash' template (CRYPTO_ALG_TYPE_SHASH) that wraps a block
cipher (CRYPTO_ALG_TYPE_CIPHER) and sets its own cra_blocksize to the block
cipher's block size. So the same block size can be gotten from either
'crypto_shash_blocksize(parent)' or 'crypto_cipher_blocksize(ctx->child)'.
It can only be 16 bytes, currently, since xcbc_create() only allows
instantiating the template if that's the block size.
But in the case of qat_alg_do_precomputes(), yes it appears to need the hash
block size.
>
> For my CRYPTO_ALG_MAX_ALIGNMASK, there is:
>
> crypto/shash.c: u8 ubuf[CRYPTO_ALG_MAX_ALIGNMASK]
> crypto/shash.c: __aligned(CRYPTO_ALG_MAX_ALIGNMASK + 1);
> crypto/shash.c: __aligned(CRYPTO_ALG_MAX_ALIGNMASK + 1);
>
> which is also shash.
>
> How should I rename these and best apply the registration-time sanity checks?
I'm not sure, but it may make sense to enforce a smaller limit for algorithm
types like CRYPTO_ALG_TYPE_CIPHER and maybe even CRYPTO_ALG_TYPE_SHASH that
can't be implemented in a hardware driver, as their APIs are not asynchronous
and don't operate on scatterlists. Only hardware drivers can need very large
alignmasks like 64 bytes, I believe.
Eric
On Wed, Jun 20, 2018 at 05:10:04PM -0700, Kees Cook wrote:
> On Wed, Jun 20, 2018 at 4:46 PM, Eric Biggers <[email protected]> wrote:
> > On Wed, Jun 20, 2018 at 12:04:04PM -0700, Kees Cook wrote:
> >> In the quest to remove all stack VLA usage from the kernel[1], this uses
> >> the maximum blocksize and adds a sanity check.
> >>
> >> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
> >>
> >> Signed-off-by: Kees Cook <[email protected]>
> >> ---
> >> crypto/xcbc.c | 5 ++++-
> >> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/crypto/xcbc.c b/crypto/xcbc.c
> >> index 25c75af50d3f..016481b1f3ba 100644
> >> --- a/crypto/xcbc.c
> >> +++ b/crypto/xcbc.c
> >> @@ -65,7 +65,10 @@ static int crypto_xcbc_digest_setkey(struct crypto_shash *parent,
> >> int bs = crypto_shash_blocksize(parent);
> >> u8 *consts = PTR_ALIGN(&ctx->ctx[0], alignmask + 1);
> >> int err = 0;
> >> - u8 key1[bs];
> >> + u8 key1[CRYPTO_ALG_MAX_BLOCKSIZE];
> >> +
> >> + if (WARN_ON(bs > sizeof(key1)))
> >> + return -EINVAL;
> >
> > Similarly, why not MAX_CIPHER_BLOCKSIZE?
> >
> > Also, xcbc_create() only allows a 16-byte block size, and you made the API
> > enforce the maximum for algorithms anyway. So I think the extra check here
> > isn't very worthwhile.
>
> Is the "parent" argument in crypto_xcbc_digest_setkey() always going
> to be the "alg" from xcbc_create()? I couldn't convince myself that
> was true. If it is, then yes, this VLA can trivially made to be 16,
> but it seemed like they were separate instances...
Yes, it's guaranteed to be an instance of "xcbc" which was created by
xcbc_create(), so it will have 'cra_blocksize == 16'.
So until someone actually tests and enables support in the "xcbc" template for
other block sizes (if the XCBC specification allows them), it would also be fine
to just '#define XCBC_BLOCK_SIZE 16' at the top of the file and use that
everywhere that references the block size.
Eric
On Wed, Jun 20, 2018 at 12:04:01PM -0700, Kees Cook wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this
> uses the newly defined max digest size macro. Also adds a sanity-check
> at use-time.
>
> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> drivers/md/dm-verity-fec.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
> index 684af08d0747..0dfcc52835bc 100644
> --- a/drivers/md/dm-verity-fec.c
> +++ b/drivers/md/dm-verity-fec.c
> @@ -212,12 +212,15 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
> struct dm_verity_fec_io *fio = fec_io(io);
> u64 block, ileaved;
> u8 *bbuf, *rs_block;
> - u8 want_digest[v->digest_size];
> + u8 want_digest[AHASH_MAX_DIGESTSIZE];
> unsigned n, k;
>
> if (neras)
> *neras = 0;
>
> + if (WARN_ON(v->digest_size < sizeof(want_digest)))
> + return -EINVAL;
How about verifying digest_size in the ahash API when algorithms
are registered?
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Wed, Jun 20, 2018 at 7:30 PM, Herbert Xu <[email protected]> wrote:
> On Wed, Jun 20, 2018 at 12:04:01PM -0700, Kees Cook wrote:
>> In the quest to remove all stack VLA usage from the kernel[1], this
>> uses the newly defined max digest size macro. Also adds a sanity-check
>> at use-time.
>>
>> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>>
>> Signed-off-by: Kees Cook <[email protected]>
>> ---
>> drivers/md/dm-verity-fec.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
>> index 684af08d0747..0dfcc52835bc 100644
>> --- a/drivers/md/dm-verity-fec.c
>> +++ b/drivers/md/dm-verity-fec.c
>> @@ -212,12 +212,15 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
>> struct dm_verity_fec_io *fio = fec_io(io);
>> u64 block, ileaved;
>> u8 *bbuf, *rs_block;
>> - u8 want_digest[v->digest_size];
>> + u8 want_digest[AHASH_MAX_DIGESTSIZE];
>> unsigned n, k;
>>
>> if (neras)
>> *neras = 0;
>>
>> + if (WARN_ON(v->digest_size < sizeof(want_digest)))
>> + return -EINVAL;
>
> How about verifying digest_size in the ahash API when algorithms
> are registered?
That happens already in ahash_prepare_alg() (and see the tweak in
patch 3 "crypto: ahash: Remove VLA usage"), but I wanted to keep these
run-time checks to avoid any problems in the future of things change.
As it's marked as "unlikely" internal to WARN_ON, this shouldn't have
a performance impact.
-Kees
--
Kees Cook
Pixel Security