[Qemu-devel] [PATCH 1/5] qcow2/qcow: protect against uninitialized encryption key

2015-05-12 Thread Daniel P. Berrange
When a qcow[2] file is opened, if the header reports an
encryption method, this is used to set the 'crypt_method_header'
field on the BDRVQcow[2]State struct, and the 'encrypted' flag
in the BDRVState struct.

When doing I/O operations, the 'crypt_method' field on the
BDRVQcow[2]State struct is checked to determine if encryption
needs to be applied.

The crypt_method_header value is copied into crypt_method when
the bdrv_set_key() method is called.

The QEMU code which opens a block device is expected to always
do a check

   if (bdrv_is_encrypted(bs)) {
   bdrv_set_key(bs, key...);
   }

If code forgets todo this, then 'crypt_method' is never set
and so when I/O is performed, QEMU writes plain text data
into a sector which is expected to contain cipher text, or
when reading, will return cipher text instead of plain
text.

Change the qcow[2] code to consult bs-encrypted when deciding
whether encryption is required, and assert(s-crypt_method)
to protect against cases where the caller forgets to set the
encryption key.

Also put an assert in the set_key methods to protect against
the case where the caller sets an encryption key on a block
device that does not have encryption

Signed-off-by: Daniel P. Berrange berra...@redhat.com
---
 block/qcow.c  | 10 +++---
 block/qcow2-cluster.c |  3 ++-
 block/qcow2.c | 18 --
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index ab89328..911e59f 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -269,6 +269,7 @@ static int qcow_set_key(BlockDriverState *bs, const char 
*key)
 for(i = 0;i  len;i++) {
 keybuf[i] = key[i];
 }
+assert(bs-encrypted);
 s-crypt_method = s-crypt_method_header;
 
 if (AES_set_encrypt_key(keybuf, 128, s-aes_encrypt_key) != 0)
@@ -411,9 +412,10 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
 bdrv_truncate(bs-file, cluster_offset + s-cluster_size);
 /* if encrypted, we must initialize the cluster
content which won't be written */
-if (s-crypt_method 
+if (bs-encrypted 
 (n_end - n_start)  s-cluster_sectors) {
 uint64_t start_sect;
+assert(s-crypt_method);
 start_sect = (offset  ~(s-cluster_size - 1))  9;
 memset(s-cluster_data + 512, 0x00, 512);
 for(i = 0; i  s-cluster_sectors; i++) {
@@ -590,7 +592,8 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, 
int64_t sector_num,
 if (ret  0) {
 break;
 }
-if (s-crypt_method) {
+if (bs-encrypted) {
+assert(s-crypt_method);
 encrypt_sectors(s, sector_num, buf, buf,
 n, 0,
 s-aes_decrypt_key);
@@ -661,7 +664,8 @@ static coroutine_fn int qcow_co_writev(BlockDriverState 
*bs, int64_t sector_num,
 ret = -EIO;
 break;
 }
-if (s-crypt_method) {
+if (bs-encrypted) {
+assert(s-crypt_method);
 if (!cluster_data) {
 cluster_data = g_malloc0(s-cluster_size);
 }
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index ed2b44d..2dd 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -403,7 +403,8 @@ static int coroutine_fn copy_sectors(BlockDriverState *bs,
 goto out;
 }
 
-if (s-crypt_method) {
+if (bs-encrypted) {
+assert(s-crypt_method);
 qcow2_encrypt_sectors(s, start_sect + n_start,
 iov.iov_base, iov.iov_base, n, 1,
 s-aes_encrypt_key);
diff --git a/block/qcow2.c b/block/qcow2.c
index b9a72e3..f7b4cc6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1037,6 +1037,7 @@ static int qcow2_set_key(BlockDriverState *bs, const char 
*key)
 for(i = 0;i  len;i++) {
 keybuf[i] = key[i];
 }
+assert(bs-encrypted);
 s-crypt_method = s-crypt_method_header;
 
 if (AES_set_encrypt_key(keybuf, 128, s-aes_encrypt_key) != 0)
@@ -1224,7 +1225,9 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState 
*bs, int64_t sector_num,
 goto fail;
 }
 
-if (s-crypt_method) {
+if (bs-encrypted) {
+assert(s-crypt_method);
+
 /*
  * For encrypted images, read everything into a temporary
  * contiguous buffer on which the AES functions can work.
@@ -1255,7 +1258,8 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState 
*bs, int64_t sector_num,
 if (ret  0) {
 goto fail;
 }
-if (s-crypt_method) {
+if (bs-encrypted) {
+assert(s-crypt_method);
 qcow2_encrypt_sectors(s, sector_num,  cluster_data,

Re: [Qemu-devel] [PATCH 1/5] qcow2/qcow: protect against uninitialized encryption key

2015-05-12 Thread Eric Blake
On 05/12/2015 10:09 AM, Daniel P. Berrange wrote:
 When a qcow[2] file is opened, if the header reports an
 encryption method, this is used to set the 'crypt_method_header'
 field on the BDRVQcow[2]State struct, and the 'encrypted' flag
 in the BDRVState struct.
 
 When doing I/O operations, the 'crypt_method' field on the
 BDRVQcow[2]State struct is checked to determine if encryption
 needs to be applied.
 
 The crypt_method_header value is copied into crypt_method when
 the bdrv_set_key() method is called.
 
 The QEMU code which opens a block device is expected to always
 do a check
 
if (bdrv_is_encrypted(bs)) {
bdrv_set_key(bs, key...);
}
 
 If code forgets todo this, then 'crypt_method' is never set

s/todo/to do/

 and so when I/O is performed, QEMU writes plain text data
 into a sector which is expected to contain cipher text, or
 when reading, will return cipher text instead of plain
 text.
 
 Change the qcow[2] code to consult bs-encrypted when deciding
 whether encryption is required, and assert(s-crypt_method)
 to protect against cases where the caller forgets to set the
 encryption key.
 
 Also put an assert in the set_key methods to protect against
 the case where the caller sets an encryption key on a block
 device that does not have encryption
 
 Signed-off-by: Daniel P. Berrange berra...@redhat.com
 ---
  block/qcow.c  | 10 +++---
  block/qcow2-cluster.c |  3 ++-
  block/qcow2.c | 18 --
  3 files changed, 21 insertions(+), 10 deletions(-)
 

Reviewed-by: Eric Blake ebl...@redhat.com

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature