Thread (41 messages) 41 messages, 6 authors, 2018-09-28

Re: [PATCH crypto-next 07/23] block: cryptoloop: Remove VLA usage of skcipher

From: Jens Axboe <axboe@kernel.dk>
Date: 2018-09-25 16:03:34
Also in: linux-block, lkml

On 9/25/18 3:25 AM, Ard Biesheuvel wrote:
quoted hunk ↗ jump to hunk
On Mon, 24 Sep 2018 at 19:53, Kees Cook [off-list ref] wrote:
quoted
On Mon, Sep 24, 2018 at 4:52 AM, Ard Biesheuvel
[off-list ref] wrote:
quoted
On Wed, 19 Sep 2018 at 04:11, Kees Cook [off-list ref] wrote:
quoted
@@ -119,7 +119,7 @@ cryptoloop_transfer(struct loop_device *lo, int cmd,
        unsigned in_offs, out_offs;
        int err;

-       skcipher_request_set_tfm(req, tfm);
+       skcipher_request_set_sync_tfm(req, tfm);
        skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP,
                                      NULL, NULL);
Does this work?
Everything is a direct wrapper for existing types and functions, so I
wouldn't expect any functional change. I haven't been able to test
this particular interface, though. cryptoloop is very deprecated,
isn't it?
Ah yes, I managed to confuse myself there. This looks all fine to me.

In any case, this is another example where we may decide to fix the
code rather than retain the request allocation on the stack (but that
is Jens's call ultimately, I suppose)
diff --git a/drivers/block/cryptoloop.c b/drivers/block/cryptoloop.c
index 7033a4beda66..5ed2167219ba 100644
--- a/drivers/block/cryptoloop.c
+++ b/drivers/block/cryptoloop.c
@@ -110,7 +110,7 @@ cryptoloop_transfer(struct loop_device *lo, int cmd,
                    int size, sector_t IV)
 {
        struct crypto_skcipher *tfm = lo->key_data;
-       SKCIPHER_REQUEST_ON_STACK(req, tfm);
+       struct skcipher_request *req;
        struct scatterlist sg_out;
        struct scatterlist sg_in;
@@ -119,7 +119,10 @@ cryptoloop_transfer(struct loop_device *lo, int cmd,
        unsigned in_offs, out_offs;
        int err;

-       skcipher_request_set_tfm(req, tfm);
+       req = skcipher_request_alloc(tfm, GFP_NOIO);
+       if (!req)
+               return -ENOMEM;
Is this going to be reliable? ->transfer() is called when we're doing IO,
and you'd normally need a mempool backed allocation to make this safe
and guarantee forward progress.

-- 
Jens Axboe
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help