Re: [PATCH] dm-crypt: Fix access beyond the end of allocated space

[Milan Broz <gmazyland@gmail.com>]

On 08/28/2014 05:09 PM, Mikulas Patocka wrote:
> dm-crypt has a bug that it accesses memory beyond allocated space.
> 
> To minimize allocation overhead, dm-crypt puts several structures into one
> block allocated with kmalloc. The block holds struct ablkcipher_request,
> cipher-specific scratch pad (crypto_ablkcipher_reqsize(any_tfm(cc))),
> struct dm_crypt_request and initialization vector.
> 
> The variable dmreq_start is set to offset of struct dm_crypt_request
> within this memory block. dm-crypt allocates block with this size:
> cc->dmreq_start + sizeof(struct dm_crypt_request) + cc->iv_size.
> 
> When accessing the initialization vector, dm-crypt uses the function
> iv_of_dmreq, which performs this calculation: ALIGN((unsigned long)(dmreq
> + 1), crypto_ablkcipher_alignmask(any_tfm(cc)) + 1).
> 
> dm-crypt allocated "cc->iv_size" bytes beyond the end of dm_crypt_request
> structure. However, when dm-crypt accesses the initialization vector, it
> takes a pointer to the end of dm_crypt_request, aligns it, and then uses
> it as the initialization vector.
> 
> If the end of dm_crypt_request is not aligned on
> crypto_ablkcipher_alignmask(any_tfm(cc)), the alignment causes
> initialization vector to point beyond the allocated space. This bug is
> very old (it dates back to commit 3a7f6c990ad04e6f576a159876c602d14d6f7fef
> in 2.6.25). However, the bug was masked by the fact that kmalloc rounds up
> the size to the next power of two. Recent change in dm-crypt that puts
> this structure to per-bio data (298a9fa08a1577211d42a75e8fc073baef61e0d9)
> made this bug show up, because there is no longer any padding beyond the
> end of iv_size.
> 
> This patch fixes the bug by calculating the variable iv_size_padding and
> adding it to the allocated size.
> 
> The patch also corrects alignment of dm_crypt_request. struct
> dm_crypt_request is specific to dm-crypt (it isn't used by the crypto
> subsystem at all), so it is aligned on __alignof__(struct
> dm_crypt_request).
> 
> The patch also aligns per_bio_data_size on ARCH_KMALLOC_MINALIGN, so that
> it is aligned as if the block was allocated with kmalloc.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

Thanks for fixing this!

I tried all reproducers I have and no problems here with your patch.
(Except another unrelated oops in scsi_debug :-)

Tested-by: Milan Broz <gmazyland@gmail.com>

Milan