lkml.org 
[lkml]   [2020]   [Nov]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v7 2/8] blk-crypto: don't require user buffer alignment
    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?

    > @@ -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

    \
     
     \ /
      Last update: 2020-11-18 00:53    [W:7.555 / U:0.320 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site