From 8336aafae1451d54c81dd2b187b45f7c45d2428e Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Tue, 12 May 2015 17:09:18 +0100 Subject: [PATCH] qcow2/qcow: protect against uninitialized encryption key 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 to do 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 Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- 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 ab893284d2..911e59fd0b 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 d74426c681..1a5c97a5ae 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -400,7 +400,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 b9a72e39d4..f7b4cc6a32 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, cluster_data, cur_nr_sectors, 0, &s->aes_decrypt_key); qemu_iovec_from_buf(qiov, bytes_done, @@ -1315,7 +1319,7 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs, trace_qcow2_writev_start_part(qemu_coroutine_self()); index_in_cluster = sector_num & (s->cluster_sectors - 1); cur_nr_sectors = remaining_sectors; - if (s->crypt_method && + if (bs->encrypted && cur_nr_sectors > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors - index_in_cluster) { cur_nr_sectors = @@ -1334,7 +1338,8 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs, qemu_iovec_concat(&hd_qiov, qiov, bytes_done, cur_nr_sectors * 512); - if (s->crypt_method) { + if (bs->encrypted) { + assert(s->crypt_method); if (!cluster_data) { cluster_data = qemu_try_blockalign(bs->file, QCOW_MAX_CRYPT_CLUSTERS @@ -1484,7 +1489,8 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp) * that means we don't have to worry about reopening them here. */ - if (s->crypt_method) { + if (bs->encrypted) { + assert(s->crypt_method); crypt_method = s->crypt_method; memcpy(&aes_encrypt_key, &s->aes_encrypt_key, sizeof(aes_encrypt_key)); memcpy(&aes_decrypt_key, &s->aes_decrypt_key, sizeof(aes_decrypt_key)); @@ -1513,7 +1519,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp) return; } - if (crypt_method) { + if (bs->encrypted) { s->crypt_method = crypt_method; memcpy(&s->aes_encrypt_key, &aes_encrypt_key, sizeof(aes_encrypt_key)); memcpy(&s->aes_decrypt_key, &aes_decrypt_key, sizeof(aes_decrypt_key));