Re: [Qemu-devel] [PATCH v2 2/4] block/qcow2: refactor qcow2_co_preadv_part

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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 <vsementsov@virtuozzo.com>
>> ---
>>   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]

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.

===

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).

> 
> [...]
> 
>> +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"?
I just kept it like it was. But it seems to be more often practice to use just abort() in
Qemu code.

-- 
Best regards,
Vladimir