On 14.08.19 11:11, Vladimir Sementsov-Ogievskiy wrote: > 14.08.2019 0:31, Max Reitz wrote: >> On 30.07.19 16:18, Vladimir Sementsov-Ogievskiy wrote: >>> Further patch will run partial requests of iterations of >>> qcow2_co_preadv in parallel for performance reasons. To prepare for >>> this, separate part which may be parallelized into separate function >>> (qcow2_co_preadv_task). >>> >>> While being here, also separate encrypted clusters reading to own >>> function, like it is done for compressed reading. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]> >>> --- >>> qapi/block-core.json | 2 +- >>> block/qcow2.c | 206 +++++++++++++++++++++++-------------------- >>> 2 files changed, 112 insertions(+), 96 deletions(-) >> >> Looks good to me overall, just wondering about some details, as always. >> >>> diff --git a/block/qcow2.c b/block/qcow2.c >>> index 93ab7edcea..7fa71968b2 100644 >>> --- a/block/qcow2.c >>> +++ b/block/qcow2.c >>> @@ -1967,17 +1967,115 @@ out: >>> return ret; >>> } >>> >>> +static coroutine_fn int >>> +qcow2_co_preadv_encrypted(BlockDriverState *bs, >>> + uint64_t file_cluster_offset, >>> + uint64_t offset, >>> + uint64_t bytes, >>> + QEMUIOVector *qiov, >>> + uint64_t qiov_offset) >>> +{ >>> + int ret; >>> + BDRVQcow2State *s = bs->opaque; >>> + uint8_t *buf; >>> + >>> + assert(bs->encrypted && s->crypto); >>> + assert(bytes <= QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size); >>> + >>> + /* >>> + * For encrypted images, read everything into a temporary >>> + * contiguous buffer on which the AES functions can work. >>> + * Note, that we can implement enctyption, working on qiov, >> >> -, and s/enctyption/encryption/ >> >>> + * but we must not do decryption in guest buffers for security >>> + * reasons. >> >> "for security reasons" is a bit handwave-y, no? > > Hmm, let's think of it a bit. > > WRITE > > 1. We can't do any operations on write buffers, as guest may use them for > something else and not prepared for their change. [thx to Den, pointed to > this fact]
Yep. So we actually cannot implement encryption in the guest buffer. :-)
> READ
>
> Hmm, here otherwise, guest should not expect something meaningful in buffers
> until the
> end of read operation, so theoretically we may decrypt directly in guest
> buffer.. What is
> bad with it?
>
> 1. Making read-part different from write and implementing support of qiov for
> decryptin for
> little outcome (hmm, don't double allocation for reads, is it little or not?
> [*]).
>
> 2. Guest can read its buffers.
> So, it may see encrypted data and guess something about it. Ideally guest
> should know nothing about encryption, but on the other hand, is there any
> real damage? I don't sure..
>
> 3. Guest can modify its buffers.
> 3.1 I think there is no guarantee that guest will not modify its data before
> we finished
> copying to separate buffer, so what guest finally reads is not predictable
> anyway.
> 3.2 But, modifying during decryption may possibly lead to guest visible error
> (which will never be if we operate on separated cluster)
>
> So if we don't afraid of [2] and [3.2], and in a specific case [*] is
> significant, we may want
> implement decryption on guest buffers at least as an option..
> But all it looks for me like we'll never do it.
Well, I do think it would be weird from a guest perspective to see data
changing twice. I just couldn’t figure out what the security problem
might be.
> ===
>
> So, I'd rewrite my "Note" like this:
>
> Also, decryption in separate buffer is better as it hides from the guest
> information
> it doesn't own (about encrypted nature of virtual disk).
Sounds good.
>> [...]
>>
>>> +static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
>>> + QCow2ClusterType cluster_type,
>>> + uint64_t file_cluster_offset,
>>> + uint64_t offset, uint64_t
>>> bytes,
>>> + QEMUIOVector *qiov,
>>> + size_t qiov_offset)
>>> +{
>>> + BDRVQcow2State *s = bs->opaque;
>>> + int offset_in_cluster = offset_into_cluster(s, offset);
>>> +
>>> + switch (cluster_type) {
>>
>> [...]
>>
>>> + default:
>>> + g_assert_not_reached();
>>> + /*
>>> + * QCOW2_CLUSTER_ZERO_PLAIN and QCOW2_CLUSTER_ZERO_ALLOC handled
>>> + * in qcow2_co_preadv_part
>>
>> Hmm, I’d still add them explicitly as cases and put their own
>> g_assert_not_reach() there.
>>
>>> + */
>>> + }
>>> +
>>> + g_assert_not_reached();
>>> +
>>> + return -EIO;
>>
>> Maybe abort()ing instead of g_assert_not_reach() would save you from
>> having to return here?
>>
>
> Hmm, will check. Any reason to use g_assert_not_reached() instead of abort()
> in "default"?
g_assert_not_reached() makes it more explicit what it’s about.
> I just kept it like it was. But it seems to be more often practice to use
> just abort() in
> Qemu code.
Yes, we often use abort() instead because it always has _Noreturn (and
thus saves us from such useless return statements).
Max
signature.asc
Description: OpenPGP digital signature
