Re: [PATCH v7 2/8] blk-crypto: don't require user buffer alignment
From: Eric Biggers <ebiggers@kernel.org>
Date: 2020-11-17 23:51:52
Also in:
linux-ext4, linux-f2fs-devel, linux-fscrypt, linux-xfs, lkml
On Tue, Nov 17, 2020 at 02:07:02PM +0000, Satya Tangirala wrote:
Previously, blk-crypto-fallback required the offset and length of each bvec in a bio to be aligned to the crypto data unit size. This patch enables blk-crypto-fallback to work even if that's not the case - the requirement now is only that the total length of the data in the bio is aligned to the crypto data unit size. Now that blk-crypto-fallback can handle bvecs not aligned to crypto data units, and we've ensured that bios are not split in the middle of a crypto data unit, we can relax the alignment check done by blk-crypto.
I think the blk-crypto.c and blk-crypto-fallback.c changes belong in different patches. There should also be a brief explanation of why this is needed (make the alignment requirements on direct I/O to encrypted files somewhat more similar to standard unencrypted files, right)?. Also, how were the blk-crypto-fallback changes tested?
quoted hunk ↗ jump to hunk
@@ -305,45 +374,57 @@ static bool blk_crypto_fallback_encrypt_bio(struct bio **bio_ptr) } memcpy(curr_dun, bc->bc_dun, sizeof(curr_dun)); - sg_init_table(&src, 1); - sg_init_table(&dst, 1); - skcipher_request_set_crypt(ciph_req, &src, &dst, data_unit_size, + skcipher_request_set_crypt(ciph_req, src, dst, data_unit_size, iv.bytes); - /* Encrypt each page in the bounce bio */ + /* + * Encrypt each data unit in the bounce bio. + * + * Take care to handle the case where a data unit spans bio segments. + * This can happen when data_unit_size > logical_block_size. + */ for (i = 0; i < enc_bio->bi_vcnt; i++) { - struct bio_vec *enc_bvec = &enc_bio->bi_io_vec[i]; - struct page *plaintext_page = enc_bvec->bv_page; + struct bio_vec *bv = &enc_bio->bi_io_vec[i]; + struct page *plaintext_page = bv->bv_page; struct page *ciphertext_page = mempool_alloc(blk_crypto_bounce_page_pool, GFP_NOIO); + unsigned int offset_in_bv = 0; - enc_bvec->bv_page = ciphertext_page; + bv->bv_page = ciphertext_page; if (!ciphertext_page) { src_bio->bi_status = BLK_STS_RESOURCE; goto out_free_bounce_pages; } - sg_set_page(&src, plaintext_page, data_unit_size, - enc_bvec->bv_offset); - sg_set_page(&dst, ciphertext_page, data_unit_size, - enc_bvec->bv_offset); - - /* Encrypt each data unit in this page */ - for (j = 0; j < enc_bvec->bv_len; j += data_unit_size) { - blk_crypto_dun_to_iv(curr_dun, &iv); - if (crypto_wait_req(crypto_skcipher_encrypt(ciph_req), - &wait)) { - i++; - src_bio->bi_status = BLK_STS_IOERR; - goto out_free_bounce_pages; + while (offset_in_bv < bv->bv_len) { + unsigned int n = min(bv->bv_len - offset_in_bv, + data_unit_size - du_filled); + sg_set_page(&src[sg_idx], plaintext_page, n, + bv->bv_offset + offset_in_bv); + sg_set_page(&dst[sg_idx], ciphertext_page, n, + bv->bv_offset + offset_in_bv); + sg_idx++; + offset_in_bv += n; + du_filled += n; + if (du_filled == data_unit_size) { + blk_crypto_dun_to_iv(curr_dun, &iv); + if (crypto_wait_req(crypto_skcipher_encrypt(ciph_req), + &wait)) { + src_bio->bi_status = BLK_STS_IOERR; + goto out_free_bounce_pages; + } + bio_crypt_dun_increment(curr_dun, 1); + sg_idx = 0; + du_filled = 0;
This is leaking a bounce page if crypto_skcipher_encrypt() fails. This can be fixed either by keeping the 'i++' that was on the error path before, or by moving the i++ in the for statement to just below to where the bounce page was successfully allocated. - Eric