| Message ID | 20180807211843.47586-6-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | crypto: Remove VLA usage | expand |
On Tue, Aug 07, 2018 at 02:18:39PM -0700, Kees Cook wrote: > In the quest to remove all stack VLA usage from the kernel[1], this uses > the new HASH_MAX_DIGESTSIZE from the crypto layer to allocate the upper > bounds on stack usage. > > [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com > > Signed-off-by: Kees Cook <keescook@chromium.org> Can the dm folks please review this patch? Thanks,
On Mon, Sep 3, 2018 at 8:13 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Tue, Aug 07, 2018 at 02:18:39PM -0700, Kees Cook wrote: >> In the quest to remove all stack VLA usage from the kernel[1], this uses >> the new HASH_MAX_DIGESTSIZE from the crypto layer to allocate the upper >> bounds on stack usage. >> >> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com >> >> Signed-off-by: Kees Cook <keescook@chromium.org> > > Can the dm folks please review this patch? Mike or Alasdair, can you Ack this patch so Herbert can include it in the crypto tree? This is blocking some VLA removals[1]... Thanks! -Kees [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
On Tue, Aug 07 2018 at 5:18pm -0400, Kees Cook <keescook@chromium.org> wrote: > In the quest to remove all stack VLA usage from the kernel[1], this uses > the new HASH_MAX_DIGESTSIZE from the crypto layer to allocate the upper > bounds on stack usage. > > [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > drivers/md/dm-integrity.c | 23 +++++++++++++++++------ > drivers/md/dm-verity-fec.c | 5 ++++- > 2 files changed, 21 insertions(+), 7 deletions(-) > > diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c > index 86438b2f10dd..884edd7cf1d0 100644 > --- a/drivers/md/dm-integrity.c > +++ b/drivers/md/dm-integrity.c > @@ -521,7 +521,12 @@ static void section_mac(struct dm_integrity_c *ic, unsigned section, __u8 result > } > memset(result + size, 0, JOURNAL_MAC_SIZE - size); > } else { > - __u8 digest[size]; > + __u8 digest[HASH_MAX_DIGESTSIZE]; > + > + if (WARN_ON(size > sizeof(digest))) { > + dm_integrity_io_error(ic, "digest_size", -EINVAL); > + goto err; > + } > r = crypto_shash_final(desc, digest); > if (unlikely(r)) { > dm_integrity_io_error(ic, "crypto_shash_final", r); > @@ -1244,7 +1249,7 @@ static void integrity_metadata(struct work_struct *w) > struct bio *bio = dm_bio_from_per_bio_data(dio, sizeof(struct dm_integrity_io)); > char *checksums; > unsigned extra_space = unlikely(digest_size > ic->tag_size) ? digest_size - ic->tag_size : 0; > - char checksums_onstack[ic->tag_size + extra_space]; > + char checksums_onstack[HASH_MAX_DIGESTSIZE]; > unsigned sectors_to_process = dio->range.n_sectors; > sector_t sector = dio->range.logical_sector; > > @@ -1253,8 +1258,14 @@ static void integrity_metadata(struct work_struct *w) > > checksums = kmalloc((PAGE_SIZE >> SECTOR_SHIFT >> ic->sb->log2_sectors_per_block) * ic->tag_size + extra_space, > GFP_NOIO | __GFP_NORETRY | __GFP_NOWARN); > - if (!checksums) > + if (!checksums) { > checksums = checksums_onstack; > + if (WARN_ON(extra_space && > + digest_size > sizeof(checksums_onstack))) { > + r = -EINVAL; > + goto error; > + } > + } > > __bio_for_each_segment(bv, bio, iter, dio->orig_bi_iter) { > unsigned pos; Given the length of the kmalloc() just prior to this new WARN_ON() line I'm not seeing why you've elected to split the WARN_ON across multiple lines. But that style nit aside: Acked-by: Mike Snitzer <snitzer@redhat.com> -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, Sep 13, 2018 at 01:54:39PM -0400, Mike Snitzer wrote: > > Acked-by: Mike Snitzer <snitzer@redhat.com> Patch applied. Thanks.
diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c index 86438b2f10dd..884edd7cf1d0 100644 --- a/drivers/md/dm-integrity.c +++ b/drivers/md/dm-integrity.c @@ -521,7 +521,12 @@ static void section_mac(struct dm_integrity_c *ic, unsigned section, __u8 result } memset(result + size, 0, JOURNAL_MAC_SIZE - size); } else { - __u8 digest[size]; + __u8 digest[HASH_MAX_DIGESTSIZE]; + + if (WARN_ON(size > sizeof(digest))) { + dm_integrity_io_error(ic, "digest_size", -EINVAL); + goto err; + } r = crypto_shash_final(desc, digest); if (unlikely(r)) { dm_integrity_io_error(ic, "crypto_shash_final", r); @@ -1244,7 +1249,7 @@ static void integrity_metadata(struct work_struct *w) struct bio *bio = dm_bio_from_per_bio_data(dio, sizeof(struct dm_integrity_io)); char *checksums; unsigned extra_space = unlikely(digest_size > ic->tag_size) ? digest_size - ic->tag_size : 0; - char checksums_onstack[ic->tag_size + extra_space]; + char checksums_onstack[HASH_MAX_DIGESTSIZE]; unsigned sectors_to_process = dio->range.n_sectors; sector_t sector = dio->range.logical_sector; @@ -1253,8 +1258,14 @@ static void integrity_metadata(struct work_struct *w) checksums = kmalloc((PAGE_SIZE >> SECTOR_SHIFT >> ic->sb->log2_sectors_per_block) * ic->tag_size + extra_space, GFP_NOIO | __GFP_NORETRY | __GFP_NOWARN); - if (!checksums) + if (!checksums) { checksums = checksums_onstack; + if (WARN_ON(extra_space && + digest_size > sizeof(checksums_onstack))) { + r = -EINVAL; + goto error; + } + } __bio_for_each_segment(bv, bio, iter, dio->orig_bi_iter) { unsigned pos; @@ -1466,7 +1477,7 @@ static bool __journal_read_write(struct dm_integrity_io *dio, struct bio *bio, } while (++s < ic->sectors_per_block); #ifdef INTERNAL_VERIFY if (ic->internal_hash) { - char checksums_onstack[max(crypto_shash_digestsize(ic->internal_hash), ic->tag_size)]; + char checksums_onstack[max(HASH_MAX_DIGESTSIZE, MAX_TAG_SIZE)]; integrity_sector_checksum(ic, logical_sector, mem + bv.bv_offset, checksums_onstack); if (unlikely(memcmp(checksums_onstack, journal_entry_tag(ic, je), ic->tag_size))) { @@ -1516,7 +1527,7 @@ static bool __journal_read_write(struct dm_integrity_io *dio, struct bio *bio, if (ic->internal_hash) { unsigned digest_size = crypto_shash_digestsize(ic->internal_hash); if (unlikely(digest_size > ic->tag_size)) { - char checksums_onstack[digest_size]; + char checksums_onstack[HASH_MAX_DIGESTSIZE]; integrity_sector_checksum(ic, logical_sector, (char *)js, checksums_onstack); memcpy(journal_entry_tag(ic, je), checksums_onstack, ic->tag_size); } else @@ -1937,7 +1948,7 @@ static void do_journal_write(struct dm_integrity_c *ic, unsigned write_start, unlikely(from_replay) && #endif ic->internal_hash) { - char test_tag[max(crypto_shash_digestsize(ic->internal_hash), ic->tag_size)]; + char test_tag[max_t(size_t, HASH_MAX_DIGESTSIZE, MAX_TAG_SIZE)]; integrity_sector_checksum(ic, sec + ((l - j) << ic->sb->log2_sectors_per_block), (char *)access_journal_data(ic, i, l), test_tag); diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c index 684af08d0747..0ce04e5b4afb 100644 --- a/drivers/md/dm-verity-fec.c +++ b/drivers/md/dm-verity-fec.c @@ -212,12 +212,15 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io, struct dm_verity_fec_io *fio = fec_io(io); u64 block, ileaved; u8 *bbuf, *rs_block; - u8 want_digest[v->digest_size]; + u8 want_digest[HASH_MAX_DIGESTSIZE]; unsigned n, k; if (neras) *neras = 0; + if (WARN_ON(v->digest_size > sizeof(want_digest))) + return -EINVAL; + /* * read each of the rsn data blocks that are part of the RS block, and * interleave contents to available bufs
In the quest to remove all stack VLA usage from the kernel[1], this uses the new HASH_MAX_DIGESTSIZE from the crypto layer to allocate the upper bounds on stack usage. [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/md/dm-integrity.c | 23 +++++++++++++++++------ drivers/md/dm-verity-fec.c | 5 ++++- 2 files changed, 21 insertions(+), 7 deletions(-)