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

[Max Reitz]

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