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

[Eric Blake]

On 8/14/19 4:11 AM, 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>
>>> ---

>>> +     * 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?

The badness is that the guest can theoretically reverse-engineer the
encryption keys if they are savvy enough to grab the contents of the
buffer before and after.  The guest must NEVER be able to see the
encrypted bits, which means decryption requires a bounce buffer.

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

Yes, this is the security risk.

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

Possible wording tweak:

Also, decryption in a separate buffer is better as it prevents the guest
from learning information about the encrypted nature of the virtual disk.


>>> +    }
>>> +
>>> +    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.

Both are used. abort() is shorter to type, but g_assert_not_reach() is
slightly friendlier to developers (which are the only people that would
ever see the failure).  As both are marked noreturn, the real fix is to
drop the dead return -EIO line, the compiler is smart enough to not need
a return statement after a noreturn function.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature