On 6 September 2018 at 15:11, Herbert Xu [off-list ref] wrote:

On Thu, Sep 06, 2018 at 11:29:41AM +0200, Ard Biesheuvel wrote:

quoted

Perhaps not, but it is not enforced atm.

In any case, limiting the reqsize is going to break things, so that
needs to occur based on the sync/async nature of the algo. That also
means we'll corrupt the stack if we ever end up using
SKCIPHER_REQUEST_ON_STACK() with an async algo whose reqsize is
greater than the sync reqsize limit, so I do think some additional
sanity check is appropriate.

I'd prefer compile-time based checks.  Perhaps we can introduce
a wrapper around crypto_skcipher, say crypto_skcipher_sync which
could then be used by SKCIPHER_REQUEST_ON_STACK to ensure that
only sync algorithms can use this construct.

That would require lots of changes in the callers, including ones that
already take care to use sync algos only.

How about we do something like the below instead?

diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
index 2f327f090c3e..ace707d59cd9 100644
--- a/include/crypto/skcipher.h
+++ b/include/crypto/skcipher.h

@@ -19,6 +19,7 @@

 /**
  *     struct skcipher_request - Symmetric key cipher request
+ *     @__onstack: 1 if the request was allocated by SKCIPHER_REQUEST_ON_STACK
  *     @cryptlen: Number of bytes to encrypt or decrypt
  *     @iv: Initialisation Vector
  *     @src: Source SG list

@@ -27,6 +28,7 @@
  *     @__ctx: Start of private context data
  */
 struct skcipher_request {
+       unsigned char __onstack;
        unsigned int cryptlen;

        u8 *iv;

@@ -141,7 +143,7 @@ struct skcipher_alg {

 #define SKCIPHER_REQUEST_ON_STACK(name, tfm) \
        char __##name##_desc[sizeof(struct skcipher_request) + \
-               crypto_skcipher_reqsize(tfm)] CRYPTO_MINALIGN_ATTR; \
+               crypto_skcipher_reqsize(tfm)] CRYPTO_MINALIGN_ATTR = { 1 }; \
        struct skcipher_request *name = (void *)__##name##_desc

 /**

@@ -437,6 +439,10 @@ static inline int crypto_skcipher_encrypt(struct

skcipher_request *req)
 {
        struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);

+       if (req->__onstack &&
+           (crypto_skcipher_alg(tfm)->base.cra_flags & CRYPTO_ALG_ASYNC))
+               return -EINVAL;
+
        if (crypto_skcipher_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
                return -ENOKEY;

@@ -458,6 +464,10 @@ static inline int crypto_skcipher_decrypt(struct

skcipher_request *req)
 {
        struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);

+       if (req->__onstack &&
+           (crypto_skcipher_alg(tfm)->base.cra_flags & CRYPTO_ALG_ASYNC))
+               return -EINVAL;
+
        if (crypto_skcipher_get_flags(tfm) & CRYPTO_TFM_NEED_KEY)
                return -ENOKEY;