Re: [PATCH 2/2] crypto: skcipher: Remove VLA usage for SKCIPHER_REQUEST_ON_STACK

[Ard Biesheuvel]

On 6 September 2018 at 09:21, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:
> On 6 September 2018 at 06:53, Gilad Ben-Yossef <gilad@xxxxxxxxxxxxx> wrote:
>> On Thu, Sep 6, 2018 at 1:49 AM, Ard Biesheuvel
>> <ard.biesheuvel@xxxxxxxxxx> wrote:
>>> On 5 September 2018 at 23:05, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>>>> On Wed, Sep 5, 2018 at 2:18 AM, Ard Biesheuvel
>>>> <ard.biesheuvel@xxxxxxxxxx> wrote:
>>>>> On 4 September 2018 at 20:16, Kees Cook <keescook@xxxxxxxxxxxx> 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. 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:
>>>>
>>>> The core part of the VLA is using this in the ON_STACK macro:
>>>>
>>>> static inline unsigned int crypto_skcipher_reqsize(struct crypto_skcipher *tfm)
>>>> {
>>>>         return tfm->reqsize;
>>>> }
>>>>
>>>> I don't find any struct crypto_skcipher .reqsize static initializers,
>>>> and the initial reqsize is here:
>>>>
>>>> static int crypto_init_skcipher_ops_ablkcipher(struct crypto_tfm *tfm)
>>>> {
>>>> ...
>>>>         skcipher->reqsize = crypto_ablkcipher_reqsize(ablkcipher) +
>>>>                             sizeof(struct ablkcipher_request);
>>>>
>>>> with updates via crypto_skcipher_set_reqsize().
>>>>
>>>> So I have to examine ablkcipher reqsize too:
>>>>
>>>> static inline unsigned int crypto_ablkcipher_reqsize(
>>>>         struct crypto_ablkcipher *tfm)
>>>> {
>>>>         return crypto_ablkcipher_crt(tfm)->reqsize;
>>>> }
>>>>
>>>> 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
>>>>
>>>> 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.
>>
>> According to Herbert, SKCIPHER_REQUEST_ON_STACK() may only be used
>> for invoking synchronous ciphers.
>>
>> In fact, due to the way the crypto API is built, if you try using it
>> with any transformation that uses DMA
>> you would most probably end up trying to DMA to/from the stack which
>> as we all know is not a great idea.
>>
>
> Ah yes, I found [0] which contains that quote.
>
> So that means that Kees can disregard the occurrences that are async
> only, but it still implies that we cannot limit the reqsize like he
> proposes unless we take the sync/async nature into account.
> It also means we should probably BUG() or WARN() in
> SKCIPHER_REQUEST_ON_STACK() when used with an async algo.
>

Something like this should do the trick:

diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
index 2f327f090c3e..70584e0f26bc 100644
--- a/include/crypto/skcipher.h
+++ b/include/crypto/skcipher.h
@@ -142,7 +142,9 @@ struct skcipher_alg {
 #define SKCIPHER_REQUEST_ON_STACK(name, tfm) \
        char __##name##_desc[sizeof(struct skcipher_request) + \
                crypto_skcipher_reqsize(tfm)] CRYPTO_MINALIGN_ATTR; \
-       struct skcipher_request *name = (void *)__##name##_desc
+       struct skcipher_request *name = WARN_ON( \
+               crypto_skcipher_alg(tfm)->base.cra_flags & CRYPTO_ALG_ASYNC) \
+               ? NULL : (void *)__##name##_desc

 /**
  * DOC: Symmetric Key Cipher API

That way, we will almost certainly oops on a NULL pointer dereference
right after, but we at least the stack corruption.

>>>
>>> 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.
>>
>> Any such occurrences are almost for sure broken already due to the DMA
>> issue I've mentioned.
>>
>
> I am not convinced of this. The skcipher request struct does not
> contain any payload buffers, and whether the algo specific ctx struct
> is used for DMA is completely up to the driver. So I am quite sure
> there are plenty of async algos that work fine with
> SKCIPHER_REQUEST_ON_STACK() and vmapped stacks.
>
>> Gilad
>>
>> --
>> Gilad Ben-Yossef
>> Chief Coffee Drinker
>>
>> values of Î will give rise to dom!
>
> [0] https://www.redhat.com/archives/dm-devel/2018-January/msg00087.html