Re: [Qemu-devel] [PATCH v2 2/4] block/qcow2: refactor qcow2_co_preadv_part
On 14.08.19 17:15, Eric Blake wrote: > 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 --- > + * 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. Our encryption does not protect against a known-plaintext attack? Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 2/4] block/qcow2: refactor qcow2_co_preadv_part
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 >>> --- >>> + * 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
Re: [Qemu-devel] [PATCH v2 2/4] block/qcow2: refactor qcow2_co_preadv_part
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
>>> ---
>>> 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, wi
Re: [Qemu-devel] [PATCH v2 2/4] block/qcow2: refactor qcow2_co_preadv_part
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
>> ---
>> 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
Re: [Qemu-devel] [PATCH v2 2/4] block/qcow2: refactor qcow2_co_preadv_part
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
> ---
> 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?
[...]
> +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?
Max
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v2 2/4] block/qcow2: refactor qcow2_co_preadv_part
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
---
qapi/block-core.json | 2 +-
block/qcow2.c| 206 +++
2 files changed, 112 insertions(+), 96 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0d43d4f37c..dd80aa11db 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3266,7 +3266,7 @@
'pwritev_rmw_tail', 'pwritev_rmw_after_tail', 'pwritev',
'pwritev_zero', 'pwritev_done', 'empty_image_prepare',
'l1_shrink_write_table', 'l1_shrink_free_l2_clusters',
-'cor_write', 'cluster_alloc_space', 'none'] }
+'cor_write', 'cluster_alloc_space', 'none', 'read_encrypted'] }
##
# @BlkdebugIOType:
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,
+ * but we must not do decryption in guest buffers for security
+ * reasons.
+ */
+
+buf = qemu_try_blockalign(s->data_file->bs, bytes);
+if (buf == NULL) {
+return -ENOMEM;
+}
+
+BLKDBG_EVENT(bs->file, BLKDBG_READ_ENCRYPTED);
+ret = bdrv_co_pread(s->data_file,
+file_cluster_offset + offset_into_cluster(s, offset),
+bytes, buf, 0);
+if (ret < 0) {
+goto fail;
+}
+
+assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
+assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+if (qcow2_co_decrypt(bs, file_cluster_offset, offset, buf, bytes) < 0) {
+ret = -EIO;
+goto fail;
+}
+qemu_iovec_from_buf(qiov, qiov_offset, buf, bytes);
+
+fail:
+qemu_vfree(buf);
+
+return ret;
+}
+
+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) {
+case QCOW2_CLUSTER_UNALLOCATED:
+assert(bs->backing); /* otherwise handled in qcow2_co_preadv_part */
+
+BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
+return bdrv_co_preadv_part(bs->backing, offset, bytes,
+ qiov, qiov_offset, 0);
+
+case QCOW2_CLUSTER_COMPRESSED:
+return qcow2_co_preadv_compressed(bs, file_cluster_offset,
+ offset, bytes, qiov, qiov_offset);
+
+case QCOW2_CLUSTER_NORMAL:
+if ((file_cluster_offset & 511) != 0) {
+return -EIO;
+}
+
+if (bs->encrypted) {
+return qcow2_co_preadv_encrypted(bs, file_cluster_offset,
+ offset, bytes, qiov, qiov_offset);
+}
+
+BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
+return bdrv_co_preadv_part(s->data_file,
+ file_cluster_offset + offset_in_cluster,
+ bytes, qiov, qiov_offset, 0);
+
+default:
+g_assert_not_reached();
+/*
+ * QCOW2_CLUSTER_ZERO_PLAIN and QCOW2_CLUSTER_ZERO_ALLOC handled
+ * in qcow2_co_preadv_part
+ */
+}
+
+g_assert_not_reached();
+
+return -EIO;
+}
+
static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
uint64_t offset, uint64_t bytes,
QEMUIOVector *qiov,
size_t qiov_offset, int flags)
{
BDRVQcow2State *s = bs->opaque;
-int offset_in_cluster;
int ret;
unsi
