On Wed, Sep 5, 2018 at 5:43 PM, Kees Cook [off-list ref] wrote:

quoted

On Wed, Sep 5, 2018 at 3:49 PM, Ard Biesheuvel
[off-list ref] wrote:

quoted

On 5 September 2018 at 23:05, Kees Cook [off-list ref] wrote:

quoted

On Wed, Sep 5, 2018 at 2:18 AM, Ard Biesheuvel
[off-list ref] wrote:

quoted

On 4 September 2018 at 20:16, Kees Cook [off-list ref] wrote:

quoted

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. Looking at instrumented tcrypt output, the largest
is for lrw:

        crypt: testing lrw(aes)
        crypto_skcipher_set_reqsize: 8
        crypto_skcipher_set_reqsize: 88
        crypto_skcipher_set_reqsize: 472

Are you sure this is a representative sampling? I haven't double
checked myself, but we have plenty of drivers for peripherals in
drivers/crypto that implement block ciphers, and they would not turn
up in tcrypt unless you are running on a platform that provides the
hardware in question.

Hrm, excellent point. Looking at this again:
[...]
And of the crt_ablkcipher.reqsize assignments/initializers, I found:

ablkcipher reqsize:
1       struct dcp_aes_req_ctx
8       struct atmel_tdes_reqctx
8       struct cryptd_blkcipher_request_ctx
8       struct mtk_aes_reqctx
8       struct omap_des_reqctx
8       struct s5p_aes_reqctx
8       struct sahara_aes_reqctx
8       struct stm32_cryp_reqctx
8       struct stm32_cryp_reqctx
16      struct ablk_ctx
24      struct atmel_aes_reqctx
48      struct omap_aes_reqctx
48      struct omap_aes_reqctx
48      struct qat_crypto_request
56      struct artpec6_crypto_request_context
64      struct chcr_blkcipher_req_ctx
80      struct spacc_req
80      struct virtio_crypto_sym_request
136     struct qce_cipher_reqctx
168     struct n2_request_context
328     struct ccp_des3_req_ctx
400     struct ccp_aes_req_ctx
536     struct hifn_request_context
992     struct cvm_req_ctx
2456    struct iproc_reqctx_s

All of these are ASYNC (they're all crt_ablkcipher), so IIUC, I can ignore them.

quoted

quoted

quoted

The base ablkcipher wrapper is:
80      struct ablkcipher_request

And in my earlier skcipher wrapper analysis, lrw was the largest
skcipher wrapper:
384     struct rctx

iproc_reqctx_s is an extreme outlier, with cvm_req_ctx at a bit less than half.

Making this a 2920 byte fixed array doesn't seem sensible at all
(though that's what's already possible to use with existing
SKCIPHER_REQUEST_ON_STACK users).

What's the right path forward here?

The skcipher implementations based on crypto IP blocks are typically
asynchronous, and I wouldn't be surprised if a fair number of
SKCIPHER_REQUEST_ON_STACK() users are limited to synchronous
skciphers.

Looks similar to ahash vs shash. :) Yes, so nearly all
crypto_alloc_skcipher() users explicitly mask away ASYNC. What's left
appears to be:

crypto/drbg.c:  sk_tfm = crypto_alloc_skcipher(ctr_name, 0, 0);
crypto/tcrypt.c:        tfm = crypto_alloc_skcipher(algo, 0, async ? 0
: CRYPTO_ALG_ASYNC);
drivers/crypto/omap-aes.c:      ctx->ctr =
crypto_alloc_skcipher("ecb(aes)", 0, 0);
drivers/md/dm-crypt.c:          cc->cipher_tfm.tfms[i] =
crypto_alloc_skcipher(ciphermode, 0, 0);
drivers/md/dm-integrity.c:              ic->journal_crypt =
crypto_alloc_skcipher(ic->journal_crypt_alg.alg_string, 0, 0);
fs/crypto/keyinfo.c:    struct crypto_skcipher *tfm =
crypto_alloc_skcipher("ecb(aes)", 0, 0);
fs/crypto/keyinfo.c:    ctfm = crypto_alloc_skcipher(mode->cipher_str, 0, 0);
fs/ecryptfs/crypto.c:   crypt_stat->tfm =
crypto_alloc_skcipher(full_alg_name, 0, 0);

I'll cross-reference this with SKCIPHER_REQUEST_ON_STACK...

None of these use SKCIPHER_REQUEST_ON_STACK that I can find.

quoted

quoted

So we could formalize this and limit SKCIPHER_REQUEST_ON_STACK() to
synchronous skciphers, which implies that the reqsize limit only has
to apply synchronous skciphers as well. But before we can do this, we
have to identify the remaining occurrences that allow asynchronous
skciphers to be used, and replace them with heap allocations.

Sounds good; thanks!

crypto_init_skcipher_ops_blkcipher() doesn't touch reqsize at all, so
the only places I can find it gets changed are with direct callers of
crypto_skcipher_set_reqsize(), which, when wrapping a sync blkcipher
start with a reqsize == 0. So, the remaining non-ASYNC callers ask
for:

4       struct sun4i_cipher_req_ctx
96      struct crypto_rfc3686_req_ctx
375     sum:
                160     crypto_skcipher_blocksize(cipher) (max)
                152     struct crypto_cts_reqctx
                63      align_mask (max)
384     struct rctx

So, following your patch to encrypt/decrypt, I can add reqsize check
there. How does this look, on top of your patch?

--- a/include/crypto/skcipher.h
+++ b/include/crypto/skcipher.h

@@ -144,9 +144,10 @@ struct skcipher_alg {
 /*
  * This must only ever be used with synchronous algorithms.
  */
+#define MAX_SYNC_SKCIPHER_REQSIZE      384
 #define SKCIPHER_REQUEST_ON_STACK(name, tfm) \
        char __##name##_desc[sizeof(struct skcipher_request) + \
-               crypto_skcipher_reqsize(tfm)] CRYPTO_MINALIGN_ATTR = { 1 } \
+               MAX_SYNC_SKCIPHER_REQSIZE] CRYPTO_MINALIGN_ATTR = { 1 } \
        struct skcipher_request *name = (void *)__##name##_desc