Re: [PATCH v7 2/8] blk-crypto: don't require user buffer alignment

From: Eric Biggers <ebiggers@kernel.org>
To: Satya Tangirala <satyat@google.com>
Cc: "Theodore Y . Ts'o" <tytso@mit.edu>,
	Jaegeuk Kim <jaegeuk@kernel.org>, Chao Yu <chao@kernel.org>,
	Jens Axboe <axboe@kernel.dk>,
	"Darrick J . Wong" <darrick.wong@oracle.com>,
	linux-kernel@vger.kernel.org, linux-fscrypt@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-xfs@vger.kernel.org, linux-block@vger.kernel.org,
	linux-ext4@vger.kernel.org
Subject: Re: [PATCH v7 2/8] blk-crypto: don't require user buffer alignment
Date: Tue, 17 Nov 2020 15:51:27 -0800	[thread overview]
Message-ID: <X7Rh/5ADHVywDtpq@sol.localdomain> (raw)
In-Reply-To: <20201117140708.1068688-3-satyat@google.com>

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