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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKv+Gu-xKWsiegZXnsMvmMVgKTo7S2cm4wEqdaL7w9z64X7SAA@mail.gmail.com>
Date:   Tue, 25 Sep 2018 11:25:57 +0200
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     Herbert Xu <herbert@...dor.apana.org.au>,
        Jens Axboe <axboe@...nel.dk>, linux-block@...r.kernel.org,
        Eric Biggers <ebiggers@...gle.com>,
        "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" 
        <linux-crypto@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH crypto-next 07/23] block: cryptoloop: Remove VLA usage of skcipher

On Mon, 24 Sep 2018 at 19:53, Kees Cook <keescook@...omium.org> wrote:
>
> On Mon, Sep 24, 2018 at 4:52 AM, Ard Biesheuvel
> <ard.biesheuvel@...aro.org> wrote:
> > On Wed, 19 Sep 2018 at 04:11, Kees Cook <keescook@...omium.org> wrote:
> >> @@ -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;
+
        skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP,
                                      NULL, NULL);


or if we stick with the current change to sync:

Acked-by: Ard Biesheuvel <ard.biesheuvel@...aro.org>