From 6775e2c4298618828de9bb3c5584d4de20120e46 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Fri, 24 Jul 2015 13:23:54 +0100 Subject: [PATCH 1/4] crypto: fix built-in AES decrypt function The qcrypto_cipher_decrypt_aes method was using the wrong key material, and passing the wrong mode. This caused it to incorrectly decrypt ciphertext. Signed-off-by: Daniel P. Berrange Message-Id: <1437740634-6261-1-git-send-email-berrange@redhat.com> Signed-off-by: Paolo Bonzini --- crypto/cipher-builtin.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crypto/cipher-builtin.c b/crypto/cipher-builtin.c index 912c1b947dbe..30f4853c86a9 100644 --- a/crypto/cipher-builtin.c +++ b/crypto/cipher-builtin.c @@ -117,7 +117,7 @@ static int qcrypto_cipher_decrypt_aes(QCryptoCipher *cipher, uint8_t *outptr = out; while (len) { if (len > AES_BLOCK_SIZE) { - AES_decrypt(inptr, outptr, &ctxt->state.aes.encrypt_key); + AES_decrypt(inptr, outptr, &ctxt->state.aes.decrypt_key); inptr += AES_BLOCK_SIZE; outptr += AES_BLOCK_SIZE; len -= AES_BLOCK_SIZE; @@ -126,15 +126,15 @@ static int qcrypto_cipher_decrypt_aes(QCryptoCipher *cipher, memcpy(tmp1, inptr, len); /* Fill with 0 to avoid valgrind uninitialized reads */ memset(tmp1 + len, 0, sizeof(tmp1) - len); - AES_decrypt(tmp1, tmp2, &ctxt->state.aes.encrypt_key); + AES_decrypt(tmp1, tmp2, &ctxt->state.aes.decrypt_key); memcpy(outptr, tmp2, len); len = 0; } } } else { AES_cbc_encrypt(in, out, len, - &ctxt->state.aes.encrypt_key, - ctxt->state.aes.iv, 1); + &ctxt->state.aes.decrypt_key, + ctxt->state.aes.iv, 0); } return 0; From 019c2ab8623daee210df8b1085a33b1e83c9ee11 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Tue, 21 Jul 2015 09:55:02 +0100 Subject: [PATCH 2/4] crypto: extend unit tests to cover decryption too The current unit test only verifies the encryption API, resulting in us missing a recently introduced bug in the decryption API from commit d3462e3. It was fortunately later discovered & fixed by commit bd09594, thanks to the QEMU I/O tests for qcow2 encryption, but we should really detect this directly in the crypto unit tests. Also remove an accidental debug message and simplify some asserts. Signed-off-by: Daniel P. Berrange Message-Id: <1437468902-23230-1-git-send-email-berrange@redhat.com> Signed-off-by: Paolo Bonzini --- tests/test-crypto-cipher.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/tests/test-crypto-cipher.c b/tests/test-crypto-cipher.c index f9b1a03a029f..9d38d2640acc 100644 --- a/tests/test-crypto-cipher.c +++ b/tests/test-crypto-cipher.c @@ -226,12 +226,10 @@ static void test_cipher(const void *opaque) const QCryptoCipherTestData *data = opaque; QCryptoCipher *cipher; - Error *err = NULL; uint8_t *key, *iv, *ciphertext, *plaintext, *outtext; size_t nkey, niv, nciphertext, nplaintext; char *outtexthex; - g_test_message("foo"); nkey = unhex_string(data->key, &key); niv = unhex_string(data->iv, &iv); nciphertext = unhex_string(data->ciphertext, &ciphertext); @@ -244,28 +242,42 @@ static void test_cipher(const void *opaque) cipher = qcrypto_cipher_new( data->alg, data->mode, key, nkey, - &err); + &error_abort); g_assert(cipher != NULL); - g_assert(err == NULL); if (iv) { g_assert(qcrypto_cipher_setiv(cipher, iv, niv, - &err) == 0); - g_assert(err == NULL); + &error_abort) == 0); } g_assert(qcrypto_cipher_encrypt(cipher, plaintext, outtext, nplaintext, - &err) == 0); - g_assert(err == NULL); + &error_abort) == 0); outtexthex = hex_string(outtext, nciphertext); g_assert_cmpstr(outtexthex, ==, data->ciphertext); + g_free(outtexthex); + + if (iv) { + g_assert(qcrypto_cipher_setiv(cipher, + iv, niv, + &error_abort) == 0); + } + g_assert(qcrypto_cipher_decrypt(cipher, + ciphertext, + outtext, + nplaintext, + &error_abort) == 0); + + outtexthex = hex_string(outtext, nplaintext); + + g_assert_cmpstr(outtexthex, ==, data->plaintext); + g_free(outtext); g_free(outtexthex); g_free(key); From 55875fc4ca45a35e36134663ade946d86fe7bfcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Salva=20Peir=C3=B3?= Date: Mon, 27 Jul 2015 10:51:52 +0200 Subject: [PATCH 3/4] megasas: Add write function to handle write access to PCI BAR 3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch fixes a QEMU SEGFAULT when a write operation is performed on the memory region of the PCI BAR 3 (base address space). When a writeb(0xe0000000) is performed the .write function is invoked to handle the write access, however, since the .write is not initialised, the call to 0, causes QEMU to SEGFAULT. Signed-off-by: Salva Peiró Acked-by: Hannes Reinecke Message-Id: <1437987112-24744-1-git-send-email-speirofr@gmail.com> Signed-off-by: Paolo Bonzini --- hw/scsi/megasas.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index 51ba9e0e6e78..a04369c5adde 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -2202,8 +2202,15 @@ static uint64_t megasas_queue_read(void *opaque, hwaddr addr, return 0; } +static void megasas_queue_write(void *opaque, hwaddr addr, + uint64_t val, unsigned size) +{ + return; +} + static const MemoryRegionOps megasas_queue_ops = { .read = megasas_queue_read, + .write = megasas_queue_write, .endianness = DEVICE_LITTLE_ENDIAN, .impl = { .min_access_size = 8, From 52c91dac6bd891656f297dab76da51fc8bc61309 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 27 Jul 2015 16:29:56 +0200 Subject: [PATCH 4/4] memory: do not add a reference to the owner of aliased regions Very often the owner of the aliased region is the same as the owner of the alias region itself. When this happens, the reference count can never go back to 0 and the owner is leaked. This is for example breaking hot-unplug of virtio-pci devices (the device cannot be plugged back again with the same id). Another common use for alias is to transform the system I/O address space into an MMIO regions; in this case the aliased region never dies, so there is no problem. Otherwise the owner is always the same for aliasing and aliased region. I checked all calls to memory_region_init_alias introduced after commit dfde4e6 (memory: add ref/unref calls, 2013-05-06) and they do not need the reference in order to keep the owner of the aliased region alive. Reported-by: Michael S. Tsirkin Tested-by: Michael S. Tsirkin Signed-off-by: Paolo Bonzini --- memory.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/memory.c b/memory.c index 5e5f32540ea8..4eb138a42903 100644 --- a/memory.c +++ b/memory.c @@ -859,11 +859,6 @@ static void memory_region_destructor_ram(MemoryRegion *mr) qemu_ram_free(mr->ram_addr); } -static void memory_region_destructor_alias(MemoryRegion *mr) -{ - memory_region_unref(mr->alias); -} - static void memory_region_destructor_ram_from_ptr(MemoryRegion *mr) { qemu_ram_free_from_ptr(mr->ram_addr); @@ -1272,8 +1267,6 @@ void memory_region_init_alias(MemoryRegion *mr, uint64_t size) { memory_region_init(mr, owner, name, size); - memory_region_ref(orig); - mr->destructor = memory_region_destructor_alias; mr->alias = orig; mr->alias_offset = offset; }