| Message ID | 20180919021100.3380-8-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: skcipher - Remove VLA usage | expand |
On Wed, 19 Sep 2018 at 04:11, Kees Cook <keescook@chromium.org> wrote: > > In the quest to remove all stack VLA usage from the kernel[1], this > replaces struct crypto_skcipher and SKCIPHER_REQUEST_ON_STACK() usage > with struct crypto_sync_skcipher and SYNC_SKCIPHER_REQUEST_ON_STACK(), > which uses a fixed stack size. > > [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com > > Cc: Jens Axboe <axboe@kernel.dk> > Cc: linux-block@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > drivers/block/cryptoloop.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/drivers/block/cryptoloop.c b/drivers/block/cryptoloop.c > index 7033a4beda66..254ee7d54e91 100644 > --- a/drivers/block/cryptoloop.c > +++ b/drivers/block/cryptoloop.c > @@ -45,7 +45,7 @@ cryptoloop_init(struct loop_device *lo, const struct loop_info64 *info) > char cms[LO_NAME_SIZE]; /* cipher-mode string */ > char *mode; > char *cmsp = cms; /* c-m string pointer */ > - struct crypto_skcipher *tfm; > + struct crypto_sync_skcipher *tfm; > > /* encryption breaks for non sector aligned offsets */ > > @@ -80,13 +80,13 @@ cryptoloop_init(struct loop_device *lo, const struct loop_info64 *info) > *cmsp++ = ')'; > *cmsp = 0; > > - tfm = crypto_alloc_skcipher(cms, 0, CRYPTO_ALG_ASYNC); > + tfm = crypto_alloc_sync_skcipher(cms, 0, 0); > if (IS_ERR(tfm)) > return PTR_ERR(tfm); > > - err = crypto_skcipher_setkey(tfm, info->lo_encrypt_key, > - info->lo_encrypt_key_size); > - > + err = crypto_sync_skcipher_setkey(tfm, info->lo_encrypt_key, > + info->lo_encrypt_key_size); > + > if (err != 0) > goto out_free_tfm; > > @@ -94,7 +94,7 @@ cryptoloop_init(struct loop_device *lo, const struct loop_info64 *info) > return 0; > > out_free_tfm: > - crypto_free_skcipher(tfm); > + crypto_free_sync_skcipher(tfm); > > out: > return err; > @@ -109,8 +109,8 @@ cryptoloop_transfer(struct loop_device *lo, int cmd, > struct page *loop_page, unsigned loop_off, > int size, sector_t IV) > { > - struct crypto_skcipher *tfm = lo->key_data; > - SKCIPHER_REQUEST_ON_STACK(req, tfm); > + struct crypto_sync_skcipher *tfm = lo->key_data; > + SYNC_SKCIPHER_REQUEST_ON_STACK(req, tfm); > struct scatterlist sg_out; > struct scatterlist sg_in; > > @@ -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? > @@ -175,9 +175,9 @@ cryptoloop_ioctl(struct loop_device *lo, int cmd, unsigned long arg) > static int > cryptoloop_release(struct loop_device *lo) > { > - struct crypto_skcipher *tfm = lo->key_data; > + struct crypto_sync_skcipher *tfm = lo->key_data; > if (tfm != NULL) { > - crypto_free_skcipher(tfm); > + crypto_free_sync_skcipher(tfm); > lo->key_data = NULL; > return 0; > } > -- > 2.17.1 >
On Mon, Sep 24, 2018 at 4:52 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On Wed, 19 Sep 2018 at 04:11, Kees Cook <keescook@chromium.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? -Kees
On Mon, 24 Sep 2018 at 19:53, Kees Cook <keescook@chromium.org> wrote: > > On Mon, Sep 24, 2018 at 4:52 AM, Ard Biesheuvel > <ard.biesheuvel@linaro.org> wrote: > > On Wed, 19 Sep 2018 at 04:11, Kees Cook <keescook@chromium.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@linaro.org>
On 9/25/18 3:25 AM, Ard Biesheuvel wrote: > On Mon, 24 Sep 2018 at 19:53, Kees Cook <keescook@chromium.org> wrote: >> >> On Mon, Sep 24, 2018 at 4:52 AM, Ard Biesheuvel >> <ard.biesheuvel@linaro.org> wrote: >>> On Wed, 19 Sep 2018 at 04:11, Kees Cook <keescook@chromium.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; 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.
On Tue, 25 Sep 2018 at 18:03, Jens Axboe <axboe@kernel.dk> wrote: > > On 9/25/18 3:25 AM, Ard Biesheuvel wrote: > > On Mon, 24 Sep 2018 at 19:53, Kees Cook <keescook@chromium.org> wrote: > >> > >> On Mon, Sep 24, 2018 at 4:52 AM, Ard Biesheuvel > >> <ard.biesheuvel@linaro.org> wrote: > >>> On Wed, 19 Sep 2018 at 04:11, Kees Cook <keescook@chromium.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; > > 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. > As far as I can tell, this function is only called from lo_read_transfer/lo_write_transfer, both of which do an unconditional alloc_page(GFP_NOIO), which is why I assumed that kmalloc(GFP_NOIO) would be permissible in the same context. Are you saying this may not be the case?
On 9/25/18 10:16 AM, Ard Biesheuvel wrote: > On Tue, 25 Sep 2018 at 18:03, Jens Axboe <axboe@kernel.dk> wrote: >> >> On 9/25/18 3:25 AM, Ard Biesheuvel wrote: >>> On Mon, 24 Sep 2018 at 19:53, Kees Cook <keescook@chromium.org> wrote: >>>> >>>> On Mon, Sep 24, 2018 at 4:52 AM, Ard Biesheuvel >>>> <ard.biesheuvel@linaro.org> wrote: >>>>> On Wed, 19 Sep 2018 at 04:11, Kees Cook <keescook@chromium.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; >> >> 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. >> > > As far as I can tell, this function is only called from > lo_read_transfer/lo_write_transfer, both of which do an unconditional > alloc_page(GFP_NOIO), which is why I assumed that kmalloc(GFP_NOIO) > would be permissible in the same context. Are you saying this may not > be the case? Doesn't appear to be safe for either your case, nor the page it's allocating. If the allocator fails this allocation, then you'll get an EIO on that request. The more likely case is the allocator taking forever to satisfy the request, in which case you'll have very large latencies for IO when you are close to being out of memory. The preferred setup for allocating memory for IO is having a mempool of at least one item. If you end up blocking for memory, you'll at most get to wait for the existing IO that's using that memory to complete (per waiter, of course).
On Tue, 25 Sep 2018 at 18:32, Jens Axboe <axboe@kernel.dk> wrote: > > On 9/25/18 10:16 AM, Ard Biesheuvel wrote: > > On Tue, 25 Sep 2018 at 18:03, Jens Axboe <axboe@kernel.dk> wrote: > >> > >> On 9/25/18 3:25 AM, Ard Biesheuvel wrote: > >>> On Mon, 24 Sep 2018 at 19:53, Kees Cook <keescook@chromium.org> wrote: > >>>> > >>>> On Mon, Sep 24, 2018 at 4:52 AM, Ard Biesheuvel > >>>> <ard.biesheuvel@linaro.org> wrote: > >>>>> On Wed, 19 Sep 2018 at 04:11, Kees Cook <keescook@chromium.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; > >> > >> 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. > >> > > > > As far as I can tell, this function is only called from > > lo_read_transfer/lo_write_transfer, both of which do an unconditional > > alloc_page(GFP_NOIO), which is why I assumed that kmalloc(GFP_NOIO) > > would be permissible in the same context. Are you saying this may not > > be the case? > > Doesn't appear to be safe for either your case, nor the page it's > allocating. If the allocator fails this allocation, then you'll get > an EIO on that request. The more likely case is the allocator taking > forever to satisfy the request, in which case you'll have very > large latencies for IO when you are close to being out of memory. > The preferred setup for allocating memory for IO is having a mempool > of at least one item. If you end up blocking for memory, you'll at > most get to wait for the existing IO that's using that memory to > complete (per waiter, of course). > Ah, great. So the code is already broken to begin with. In that case, may we have your ack for Kees's original patch, which is effectively a no-op except for the fact that the size of the stack buffer is no longer decided at runtime?
diff --git a/drivers/block/cryptoloop.c b/drivers/block/cryptoloop.c index 7033a4beda66..254ee7d54e91 100644 --- a/drivers/block/cryptoloop.c +++ b/drivers/block/cryptoloop.c @@ -45,7 +45,7 @@ cryptoloop_init(struct loop_device *lo, const struct loop_info64 *info) char cms[LO_NAME_SIZE]; /* cipher-mode string */ char *mode; char *cmsp = cms; /* c-m string pointer */ - struct crypto_skcipher *tfm; + struct crypto_sync_skcipher *tfm; /* encryption breaks for non sector aligned offsets */ @@ -80,13 +80,13 @@ cryptoloop_init(struct loop_device *lo, const struct loop_info64 *info) *cmsp++ = ')'; *cmsp = 0; - tfm = crypto_alloc_skcipher(cms, 0, CRYPTO_ALG_ASYNC); + tfm = crypto_alloc_sync_skcipher(cms, 0, 0); if (IS_ERR(tfm)) return PTR_ERR(tfm); - err = crypto_skcipher_setkey(tfm, info->lo_encrypt_key, - info->lo_encrypt_key_size); - + err = crypto_sync_skcipher_setkey(tfm, info->lo_encrypt_key, + info->lo_encrypt_key_size); + if (err != 0) goto out_free_tfm; @@ -94,7 +94,7 @@ cryptoloop_init(struct loop_device *lo, const struct loop_info64 *info) return 0; out_free_tfm: - crypto_free_skcipher(tfm); + crypto_free_sync_skcipher(tfm); out: return err; @@ -109,8 +109,8 @@ cryptoloop_transfer(struct loop_device *lo, int cmd, struct page *loop_page, unsigned loop_off, int size, sector_t IV) { - struct crypto_skcipher *tfm = lo->key_data; - SKCIPHER_REQUEST_ON_STACK(req, tfm); + struct crypto_sync_skcipher *tfm = lo->key_data; + SYNC_SKCIPHER_REQUEST_ON_STACK(req, tfm); struct scatterlist sg_out; struct scatterlist sg_in; @@ -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); @@ -175,9 +175,9 @@ cryptoloop_ioctl(struct loop_device *lo, int cmd, unsigned long arg) static int cryptoloop_release(struct loop_device *lo) { - struct crypto_skcipher *tfm = lo->key_data; + struct crypto_sync_skcipher *tfm = lo->key_data; if (tfm != NULL) { - crypto_free_skcipher(tfm); + crypto_free_sync_skcipher(tfm); lo->key_data = NULL; return 0; }
In the quest to remove all stack VLA usage from the kernel[1], this replaces struct crypto_skcipher and SKCIPHER_REQUEST_ON_STACK() usage with struct crypto_sync_skcipher and SYNC_SKCIPHER_REQUEST_ON_STACK(), which uses a fixed stack size. [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com Cc: Jens Axboe <axboe@kernel.dk> Cc: linux-block@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/block/cryptoloop.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)