[PULL 00/20] Crypto and I/O patches

[PULL 00/20] Crypto and I/O patches

[Daniel P. Berrangé]

The following changes since commit e750a7ace492f0b450653d4ad368a77d6f660fb8:

  Merge tag 'pull-9p-20221024' of https://github.com/cschoenebeck/qemu into staging (2022-10-24 14:27:12 -0400)

are available in the Git repository at:

  https://gitlab.com/berrange/qemu tags/misc-next-pull-request

for you to fetch changes up to da0ab2c4c4d22dece12acd9ddaed901a10a5edee:

  crypto: add test cases for many malformed LUKS header scenarios (2022-10-27 13:06:12 +0100)

----------------------------------------------------------------
Pending crypto and io queue

 * Many LUKS header robustness checks
 * Fix TLS PSK error reporting
 * Enable LUKS creation on macOS
 * Report useful errnos from seccomp
 * I/O chanel Windows portability fix

----------------------------------------------------------------

Bin Meng (4):
  util/qemu-sockets: Use g_get_tmp_dir() to get the directory for
    temporary files
  io/channel-watch: Drop a superfluous '#ifdef WIN32'
  io/channel-watch: Drop the unnecessary cast
  io/channel-watch: Fix socket watch on Windows

Daniel P. Berrangé (14):
  scripts: check if .git exists before checking submodule status
  crypto: check for and report errors setting PSK credentials
  tests: avoid DOS line endings in PSK file
  crypto: sanity check that LUKS header strings are NUL-terminated
  crypto: enforce that LUKS stripes is always a fixed value
  crypto: enforce that key material doesn't overlap with LUKS header
  crypto: validate that LUKS payload doesn't overlap with header
  crypto: strengthen the check for key slots overlapping with LUKS
    header
  crypto: check that LUKS PBKDF2 iterations count is non-zero
  crypto: split LUKS header definitions off into file
  crypto: split off helpers for converting LUKS header endianess
  crypto: quote algorithm names in error messages
  crypto: ensure LUKS tests run with GNUTLS crypto provider
  crypto: add test cases for many malformed LUKS header scenarios

Jungmin Park (1):
  crypto/luks: Support creating LUKS image on Darwin

Michal Privoznik (1):
  seccomp: Get actual errno value from failed seccomp functions

 crypto/block-luks-priv.h            | 143 +++++++++++++
 crypto/block-luks.c                 | 228 +++++++++------------
 crypto/pbkdf.c                      |  23 +++
 crypto/tlscredspsk.c                |  16 +-
 io/channel-watch.c                  |  12 +-
 meson.build                         |   9 +
 scripts/git-submodule.sh            |  12 +-
 softmmu/qemu-seccomp.c              |  13 ++
 tests/unit/crypto-tls-psk-helpers.c |  11 +-
 tests/unit/test-crypto-block.c      | 302 +++++++++++++++++++++++++++-
 util/qemu-sockets.c                 |   5 +-
 11 files changed, 616 insertions(+), 158 deletions(-)
 create mode 100644 crypto/block-luks-priv.h

-- 
2.37.3

[PULL 01/20] crypto/luks: Support creating LUKS image on Darwin

[Daniel P. Berrangé]

From: Jungmin Park <pjm0616@gmail.com>

When the user creates a LUKS-encrypted qcow2 image using the qemu-img
program, the passphrase is hashed using PBKDF2 with a dynamic
number of iterations. The number of iterations is determined by
measuring thread cpu time usage, such that it takes approximately
2 seconds to compute the hash.

Because Darwin doesn't implement getrusage(RUSAGE_THREAD), we get an
error message:
> qemu-img: test.qcow2: Unable to calculate thread CPU usage on this platform
for this command:
> qemu-img create --object secret,id=key,data=1234 -f qcow2 -o 'encrypt.format=luks,encrypt.key-secret=key' test.qcow2 100M

This patch implements qcrypto_pbkdf2_get_thread_cpu() for Darwin so that
the above command works.

Signed-off-by: Jungmin Park <pjm0616@gmail.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/pbkdf.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/crypto/pbkdf.c b/crypto/pbkdf.c
index 3775ddc6c5..8d198c152c 100644
--- a/crypto/pbkdf.c
+++ b/crypto/pbkdf.c
@@ -24,6 +24,11 @@
 #ifndef _WIN32
 #include <sys/resource.h>
 #endif
+#ifdef CONFIG_DARWIN
+#include <mach/mach_init.h>
+#include <mach/thread_act.h>
+#include <mach/mach_port.h>
+#endif
 
 
 static int qcrypto_pbkdf2_get_thread_cpu(unsigned long long *val_ms,
@@ -45,6 +50,24 @@ static int qcrypto_pbkdf2_get_thread_cpu(unsigned long long *val_ms,
     /* QuadPart is units of 100ns and we want ms as unit */
     *val_ms = thread_time.QuadPart / 10000ll;
     return 0;
+#elif defined(CONFIG_DARWIN)
+    mach_port_t thread;
+    kern_return_t kr;
+    mach_msg_type_number_t count;
+    thread_basic_info_data_t info;
+
+    thread = mach_thread_self();
+    count = THREAD_BASIC_INFO_COUNT;
+    kr = thread_info(thread, THREAD_BASIC_INFO, (thread_info_t)&info, &count);
+    mach_port_deallocate(mach_task_self(), thread);
+    if (kr != KERN_SUCCESS || (info.flags & TH_FLAGS_IDLE) != 0) {
+        error_setg_errno(errp, errno, "Unable to get thread CPU usage");
+        return -1;
+    }
+
+    *val_ms = ((info.user_time.seconds * 1000ll) +
+               (info.user_time.microseconds / 1000));
+    return 0;
 #elif defined(RUSAGE_THREAD)
     struct rusage ru;
     if (getrusage(RUSAGE_THREAD, &ru) < 0) {
-- 
2.37.3

[PULL 02/20] util/qemu-sockets: Use g_get_tmp_dir() to get the directory for temporary files

[Daniel P. Berrangé]

From: Bin Meng <bin.meng@windriver.com>

Replace the existing logic to get the directory for temporary files
with g_get_tmp_dir(), which works for win32 too.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 util/qemu-sockets.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 83f4bd6fd2..0c41ca9e42 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -919,9 +919,8 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
     if (saddr->path[0] || abstract) {
         path = saddr->path;
     } else {
-        const char *tmpdir = getenv("TMPDIR");
-        tmpdir = tmpdir ? tmpdir : "/tmp";
-        path = pathbuf = g_strdup_printf("%s/qemu-socket-XXXXXX", tmpdir);
+        path = pathbuf = g_strdup_printf("%s/qemu-socket-XXXXXX",
+                                         g_get_tmp_dir());
     }
 
     pathlen = strlen(path);
-- 
2.37.3

[PULL 03/20] io/channel-watch: Drop a superfluous '#ifdef WIN32'

[Daniel P. Berrangé]

From: Bin Meng <bin.meng@windriver.com>

In the win32 version qio_channel_create_socket_watch() body there is
no need to do a '#ifdef WIN32'.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 io/channel-watch.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/io/channel-watch.c b/io/channel-watch.c
index 0289b3647c..89f3c8a88a 100644
--- a/io/channel-watch.c
+++ b/io/channel-watch.c
@@ -285,11 +285,9 @@ GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
     GSource *source;
     QIOChannelSocketSource *ssource;
 
-#ifdef WIN32
     WSAEventSelect(socket, ioc->event,
                    FD_READ | FD_ACCEPT | FD_CLOSE |
                    FD_CONNECT | FD_WRITE | FD_OOB);
-#endif
 
     source = g_source_new(&qio_channel_socket_source_funcs,
                           sizeof(QIOChannelSocketSource));
-- 
2.37.3

[PULL 04/20] io/channel-watch: Drop the unnecessary cast

[Daniel P. Berrangé]

From: Bin Meng <bin.meng@windriver.com>

There is no need to do a type cast on ssource->socket as it is
already declared as a SOCKET.

Suggested-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 io/channel-watch.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/io/channel-watch.c b/io/channel-watch.c
index 89f3c8a88a..43d38494f7 100644
--- a/io/channel-watch.c
+++ b/io/channel-watch.c
@@ -130,13 +130,13 @@ qio_channel_socket_source_check(GSource *source)
     FD_ZERO(&wfds);
     FD_ZERO(&xfds);
     if (ssource->condition & G_IO_IN) {
-        FD_SET((SOCKET)ssource->socket, &rfds);
+        FD_SET(ssource->socket, &rfds);
     }
     if (ssource->condition & G_IO_OUT) {
-        FD_SET((SOCKET)ssource->socket, &wfds);
+        FD_SET(ssource->socket, &wfds);
     }
     if (ssource->condition & G_IO_PRI) {
-        FD_SET((SOCKET)ssource->socket, &xfds);
+        FD_SET(ssource->socket, &xfds);
     }
     ssource->revents = 0;
     if (select(0, &rfds, &wfds, &xfds, &tv0) == 0) {
-- 
2.37.3

[PULL 05/20] io/channel-watch: Fix socket watch on Windows

[Daniel P. Berrangé]

From: Bin Meng <bin.meng@windriver.com>

Random failure was observed when running qtests on Windows due to
"Broken pipe" detected by qmp_fd_receive(). What happened is that
the qtest executable sends testing data over a socket to the QEMU
under test but no response is received. The errno of the recv()
call from the qtest executable indicates ETIMEOUT, due to the qmp
chardev's tcp_chr_read() is never called to receive testing data
hence no response is sent to the other side.

tcp_chr_read() is registered as the callback of the socket watch
GSource. The reason of the callback not being called by glib, is
that the source check fails to indicate the source is ready. There
are two socket watch sources created to monitor the same socket
event object from the char-socket backend in update_ioc_handlers().
During the source check phase, qio_channel_socket_source_check()
calls WSAEnumNetworkEvents() to discover occurrences of network
events for the indicated socket, clear internal network event records,
and reset the event object. Testing shows that if we don't reset the
event object by not passing the event handle to WSAEnumNetworkEvents()
the symptom goes away and qtest runs very stably.

It seems we don't need to call WSAEnumNetworkEvents() at all, as we
don't parse the result of WSANETWORKEVENTS returned from this API.
We use select() to poll the socket status. Fix this instability by
dropping the WSAEnumNetworkEvents() call.

Some side notes:

During the testing, I removed the following codes in update_ioc_handlers():

  remove_hup_source(s);
  s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP);
  g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup,
                        chr, NULL);
  g_source_attach(s->hup_source, chr->gcontext);

and such change also makes the symptom go away.

And if I moved the above codes to the beginning, before the call to
io_add_watch_poll(), the symptom also goes away.

It seems two sources watching on the same socket event object is
the key that leads to the instability. The order of adding a source
watch seems to also play a role but I can't explain why.
Hopefully a Windows and glib expert could explain this behavior.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 io/channel-watch.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/io/channel-watch.c b/io/channel-watch.c
index 43d38494f7..ad7c568a84 100644
--- a/io/channel-watch.c
+++ b/io/channel-watch.c
@@ -115,17 +115,13 @@ static gboolean
 qio_channel_socket_source_check(GSource *source)
 {
     static struct timeval tv0;
-
     QIOChannelSocketSource *ssource = (QIOChannelSocketSource *)source;
-    WSANETWORKEVENTS ev;
     fd_set rfds, wfds, xfds;
 
     if (!ssource->condition) {
         return 0;
     }
 
-    WSAEnumNetworkEvents(ssource->socket, ssource->ioc->event, &ev);
-
     FD_ZERO(&rfds);
     FD_ZERO(&wfds);
     FD_ZERO(&xfds);
-- 
2.37.3

[PULL 06/20] seccomp: Get actual errno value from failed seccomp functions

[Daniel P. Berrangé]

From: Michal Privoznik <mprivozn@redhat.com>

Upon failure, a libseccomp API returns actual errno value very
rarely. Fortunately, after its commit 34bf78ab (contained in
2.5.0 release), the SCMP_FLTATR_API_SYSRAWRC attribute can be set
which makes subsequent APIs return true errno on failure.

This is especially critical when seccomp_load() fails, because
generic -ECANCELED says nothing.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 meson.build            |  9 +++++++++
 softmmu/qemu-seccomp.c | 13 +++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/meson.build b/meson.build
index b686dfef75..5f114c89d9 100644
--- a/meson.build
+++ b/meson.build
@@ -636,10 +636,16 @@ if vmnet.found() and not cc.has_header_symbol('vmnet/vmnet.h',
 endif
 
 seccomp = not_found
+seccomp_has_sysrawrc = false
 if not get_option('seccomp').auto() or have_system or have_tools
   seccomp = dependency('libseccomp', version: '>=2.3.0',
                        required: get_option('seccomp'),
                        method: 'pkg-config', kwargs: static_kwargs)
+  if seccomp.found()
+    seccomp_has_sysrawrc = cc.has_header_symbol('seccomp.h',
+                                                'SCMP_FLTATR_API_SYSRAWRC',
+                                                dependencies: seccomp)
+  endif
 endif
 
 libcap_ng = not_found
@@ -1849,6 +1855,9 @@ config_host_data.set('CONFIG_RDMA', rdma.found())
 config_host_data.set('CONFIG_SDL', sdl.found())
 config_host_data.set('CONFIG_SDL_IMAGE', sdl_image.found())
 config_host_data.set('CONFIG_SECCOMP', seccomp.found())
+if seccomp.found()
+  config_host_data.set('CONFIG_SECCOMP_SYSRAWRC', seccomp_has_sysrawrc)
+endif
 config_host_data.set('CONFIG_SNAPPY', snappy.found())
 config_host_data.set('CONFIG_TPM', have_tpm)
 config_host_data.set('CONFIG_USB_LIBUSB', libusb.found())
diff --git a/softmmu/qemu-seccomp.c b/softmmu/qemu-seccomp.c
index deaf8a4ef5..d66a2a1226 100644
--- a/softmmu/qemu-seccomp.c
+++ b/softmmu/qemu-seccomp.c
@@ -312,6 +312,19 @@ static int seccomp_start(uint32_t seccomp_opts, Error **errp)
         goto seccomp_return;
     }
 
+#if defined(CONFIG_SECCOMP_SYSRAWRC)
+    /*
+     * This must be the first seccomp_attr_set() call to have full
+     * error propagation from subsequent seccomp APIs.
+     */
+    rc = seccomp_attr_set(ctx, SCMP_FLTATR_API_SYSRAWRC, 1);
+    if (rc != 0) {
+        error_setg_errno(errp, -rc,
+                         "failed to set seccomp rawrc attribute");
+        goto seccomp_return;
+    }
+#endif
+
     rc = seccomp_attr_set(ctx, SCMP_FLTATR_CTL_TSYNC, 1);
     if (rc != 0) {
         error_setg_errno(errp, -rc,
-- 
2.37.3

[PULL 07/20] scripts: check if .git exists before checking submodule status

[Daniel P. Berrangé]

Currently we check status of each submodule, before actually checking
if we're in a git repo. These status commands will all fail, but we
are hiding their output so we don't see it currently.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/git-submodule.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/scripts/git-submodule.sh b/scripts/git-submodule.sh
index e225d3a963..7be41f5948 100755
--- a/scripts/git-submodule.sh
+++ b/scripts/git-submodule.sh
@@ -51,6 +51,12 @@ validate_error() {
     exit 1
 }
 
+if test -n "$maybe_modules" && ! test -e ".git"
+then
+    echo "$0: unexpectedly called with submodules but no git checkout exists"
+    exit 1
+fi
+
 modules=""
 for m in $maybe_modules
 do
@@ -63,12 +69,6 @@ do
     fi
 done
 
-if test -n "$maybe_modules" && ! test -e ".git"
-then
-    echo "$0: unexpectedly called with submodules but no git checkout exists"
-    exit 1
-fi
-
 case "$command" in
 status|validate)
     if test -z "$maybe_modules"
-- 
2.37.3

[PULL 08/20] crypto: check for and report errors setting PSK credentials

[Daniel P. Berrangé]

If setting credentials fails, the handshake will later fail to complete
with an obscure error message which is hard to diagnose.

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Tested-by: Bin Meng <bmeng.cn@gmail.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/tlscredspsk.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/crypto/tlscredspsk.c b/crypto/tlscredspsk.c
index a4f9891274..546cad1c5a 100644
--- a/crypto/tlscredspsk.c
+++ b/crypto/tlscredspsk.c
@@ -109,7 +109,12 @@ qcrypto_tls_creds_psk_load(QCryptoTLSCredsPSK *creds,
             goto cleanup;
         }
 
-        gnutls_psk_set_server_credentials_file(creds->data.server, pskfile);
+        ret = gnutls_psk_set_server_credentials_file(creds->data.server, pskfile);
+        if (ret < 0) {
+            error_setg(errp, "Cannot set PSK server credentials: %s",
+                       gnutls_strerror(ret));
+            goto cleanup;
+        }
         gnutls_psk_set_server_dh_params(creds->data.server,
                                         creds->parent_obj.dh_params);
     } else {
@@ -135,8 +140,13 @@ qcrypto_tls_creds_psk_load(QCryptoTLSCredsPSK *creds,
             goto cleanup;
         }
 
-        gnutls_psk_set_client_credentials(creds->data.client,
-                                          username, &key, GNUTLS_PSK_KEY_HEX);
+        ret = gnutls_psk_set_client_credentials(creds->data.client,
+                                                username, &key, GNUTLS_PSK_KEY_HEX);
+        if (ret < 0) {
+            error_setg(errp, "Cannot set PSK client credentials: %s",
+                       gnutls_strerror(ret));
+            goto cleanup;
+        }
     }
 
     rv = 0;
-- 
2.37.3

[PULL 09/20] tests: avoid DOS line endings in PSK file

[Daniel P. Berrangé]

Using FILE * APIs for writing the PSK file results in translation from
UNIX to DOS line endings on Windows. When the crypto PSK code later
loads the credentials the stray \r will result in failure to load the
PSK credentials into GNUTLS.

Rather than switching the FILE* APIs to open in binary format, just
switch to the more concise g_file_set_contents API.

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Tested-by: Bin Meng <bmeng.cn@gmail.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/unit/crypto-tls-psk-helpers.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/tests/unit/crypto-tls-psk-helpers.c b/tests/unit/crypto-tls-psk-helpers.c
index 511e08cc9c..c6cc740772 100644
--- a/tests/unit/crypto-tls-psk-helpers.c
+++ b/tests/unit/crypto-tls-psk-helpers.c
@@ -27,15 +27,14 @@
 static void
 test_tls_psk_init_common(const char *pskfile, const char *user, const char *key)
 {
-    FILE *fp;
+    g_autoptr(GError) gerr = NULL;
+    g_autofree char *line = g_strdup_printf("%s:%s\n", user, key);
 
-    fp = fopen(pskfile, "w");
-    if (fp == NULL) {
-        g_critical("Failed to create pskfile %s: %s", pskfile, strerror(errno));
+    g_file_set_contents(pskfile, line, strlen(line), &gerr);
+    if (gerr != NULL) {
+        g_critical("Failed to create pskfile %s: %s", pskfile, gerr->message);
         abort();
     }
-    fprintf(fp, "%s:%s\n", user, key);
-    fclose(fp);
 }
 
 void test_tls_psk_init(const char *pskfile)
-- 
2.37.3

[PULL 10/20] crypto: sanity check that LUKS header strings are NUL-terminated

[Daniel P. Berrangé]

The LUKS spec requires that header strings are NUL-terminated, and our
code relies on that. Protect against maliciously crafted headers by
adding validation.

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/block-luks.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index f62be6836b..27d1b34c1d 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -554,6 +554,24 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
         return -1;
     }
 
+    if (!memchr(luks->header.cipher_name, '\0',
+                sizeof(luks->header.cipher_name))) {
+        error_setg(errp, "LUKS header cipher name is not NUL terminated");
+        return -1;
+    }
+
+    if (!memchr(luks->header.cipher_mode, '\0',
+                sizeof(luks->header.cipher_mode))) {
+        error_setg(errp, "LUKS header cipher mode is not NUL terminated");
+        return -1;
+    }
+
+    if (!memchr(luks->header.hash_spec, '\0',
+                sizeof(luks->header.hash_spec))) {
+        error_setg(errp, "LUKS header hash spec is not NUL terminated");
+        return -1;
+    }
+
     /* Check all keyslots for corruption  */
     for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) {
 
-- 
2.37.3

[PULL 11/20] crypto: enforce that LUKS stripes is always a fixed value

[Daniel P. Berrangé]

Although the LUKS stripes are encoded in the keyslot header and so
potentially configurable, in pratice the cryptsetup impl mandates
this has the fixed value 4000. To avoid incompatibility apply the
same enforcement in QEMU too. This also caps the memory usage for
key material when QEMU tries to open a LUKS volume.

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/block-luks.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 27d1b34c1d..81744e2a8e 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -582,8 +582,9 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
                                                    header_sectors,
                                                    slot1->stripes);
 
-        if (slot1->stripes == 0) {
-            error_setg(errp, "Keyslot %zu is corrupted (stripes == 0)", i);
+        if (slot1->stripes != QCRYPTO_BLOCK_LUKS_STRIPES) {
+            error_setg(errp, "Keyslot %zu is corrupted (stripes %d != %d)",
+                       i, slot1->stripes, QCRYPTO_BLOCK_LUKS_STRIPES);
             return -1;
         }
 
-- 
2.37.3

[PULL 12/20] crypto: enforce that key material doesn't overlap with LUKS header

[Daniel P. Berrangé]

We already check that key material doesn't overlap between key slots,
and that it doesn't overlap with the payload. We didn't check for
overlap with the LUKS header.

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/block-luks.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 81744e2a8e..6ef9a89ffa 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -595,6 +595,14 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
             return -1;
         }
 
+        if (start1 < DIV_ROUND_UP(sizeof(QCryptoBlockLUKSHeader),
+                                  QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) {
+            error_setg(errp,
+                       "Keyslot %zu is overlapping with the LUKS header",
+                       i);
+            return -1;
+        }
+
         if (start1 + len1 > luks->header.payload_offset_sector) {
             error_setg(errp,
                        "Keyslot %zu is overlapping with the encrypted payload",
-- 
2.37.3

[PULL 13/20] crypto: validate that LUKS payload doesn't overlap with header

[Daniel P. Berrangé]

We already validate that LUKS keyslots don't overlap with the
header, or with each other. This closes the remaining hole in
validation of LUKS file regions.

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/block-luks.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 6ef9a89ffa..f22bc63e54 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -572,6 +572,13 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
         return -1;
     }
 
+    if (luks->header.payload_offset_sector <
+        DIV_ROUND_UP(QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET,
+                     QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) {
+        error_setg(errp, "LUKS payload is overlapping with the header");
+        return -1;
+    }
+
     /* Check all keyslots for corruption  */
     for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) {
 
-- 
2.37.3

[PULL 14/20] crypto: strengthen the check for key slots overlapping with LUKS header

[Daniel P. Berrangé]

The LUKS header data on disk is a fixed size, however, there's expected
to be a gap between the end of the header and the first key slot to get
alignment with the 2nd sector on 4k drives. This wasn't originally part
of the LUKS spec, but was always part of the reference implementation,
so it is worth validating this.

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/block-luks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index f22bc63e54..e6ee8506b2 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -602,7 +602,7 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
             return -1;
         }
 
-        if (start1 < DIV_ROUND_UP(sizeof(QCryptoBlockLUKSHeader),
+        if (start1 < DIV_ROUND_UP(QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET,
                                   QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) {
             error_setg(errp,
                        "Keyslot %zu is overlapping with the LUKS header",
-- 
2.37.3

[PULL 15/20] crypto: check that LUKS PBKDF2 iterations count is non-zero

[Daniel P. Berrangé]

Both the master key and key slot passphrases are run through the PBKDF2
algorithm. The iterations count is expected to be generally very large
(many 10's or 100's of 1000s). It is hard to define a low level cutoff,
but we can certainly say that iterations count should be non-zero. A
zero count likely indicates an initialization mistake so reject it.

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/block-luks.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index e6ee8506b2..254490c256 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -579,6 +579,11 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
         return -1;
     }
 
+    if (luks->header.master_key_iterations == 0) {
+        error_setg(errp, "LUKS key iteration count is zero");
+        return -1;
+    }
+
     /* Check all keyslots for corruption  */
     for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) {
 
@@ -602,6 +607,12 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
             return -1;
         }
 
+        if (slot1->active == QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED &&
+            slot1->iterations == 0) {
+            error_setg(errp, "Keyslot %zu iteration count is zero", i);
+            return -1;
+        }
+
         if (start1 < DIV_ROUND_UP(QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET,
                                   QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) {
             error_setg(errp,
-- 
2.37.3

[PULL 16/20] crypto: split LUKS header definitions off into file

[Daniel P. Berrangé]

This will allow unit testing code to use the structs.

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/block-luks-priv.h | 137 +++++++++++++++++++++++++++++++++++++++
 crypto/block-luks.c      |  94 +--------------------------
 2 files changed, 138 insertions(+), 93 deletions(-)
 create mode 100644 crypto/block-luks-priv.h

diff --git a/crypto/block-luks-priv.h b/crypto/block-luks-priv.h
new file mode 100644
index 0000000000..1516571dcb
--- /dev/null
+++ b/crypto/block-luks-priv.h
@@ -0,0 +1,137 @@
+/*
+ * QEMU Crypto block device encryption LUKS format
+ *
+ * Copyright (c) 2015-2016 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/bswap.h"
+
+#include "block-luks.h"
+
+#include "crypto/hash.h"
+#include "crypto/afsplit.h"
+#include "crypto/pbkdf.h"
+#include "crypto/secret.h"
+#include "crypto/random.h"
+#include "qemu/uuid.h"
+
+#include "qemu/coroutine.h"
+#include "qemu/bitmap.h"
+
+/*
+ * Reference for the LUKS format implemented here is
+ *
+ *   docs/on-disk-format.pdf
+ *
+ * in 'cryptsetup' package source code
+ *
+ * This file implements the 1.2.1 specification, dated
+ * Oct 16, 2011.
+ */
+
+typedef struct QCryptoBlockLUKSHeader QCryptoBlockLUKSHeader;
+typedef struct QCryptoBlockLUKSKeySlot QCryptoBlockLUKSKeySlot;
+
+
+/* The following constants are all defined by the LUKS spec */
+#define QCRYPTO_BLOCK_LUKS_VERSION 1
+
+#define QCRYPTO_BLOCK_LUKS_MAGIC_LEN 6
+#define QCRYPTO_BLOCK_LUKS_CIPHER_NAME_LEN 32
+#define QCRYPTO_BLOCK_LUKS_CIPHER_MODE_LEN 32
+#define QCRYPTO_BLOCK_LUKS_HASH_SPEC_LEN 32
+#define QCRYPTO_BLOCK_LUKS_DIGEST_LEN 20
+#define QCRYPTO_BLOCK_LUKS_SALT_LEN 32
+#define QCRYPTO_BLOCK_LUKS_UUID_LEN 40
+#define QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS 8
+#define QCRYPTO_BLOCK_LUKS_STRIPES 4000
+#define QCRYPTO_BLOCK_LUKS_MIN_SLOT_KEY_ITERS 1000
+#define QCRYPTO_BLOCK_LUKS_MIN_MASTER_KEY_ITERS 1000
+#define QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET 4096
+
+#define QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED 0x0000DEAD
+#define QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED 0x00AC71F3
+
+#define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL
+
+#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS 2000
+#define QCRYPTO_BLOCK_LUKS_ERASE_ITERATIONS 40
+
+static const char qcrypto_block_luks_magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN] = {
+    'L', 'U', 'K', 'S', 0xBA, 0xBE
+};
+
+/*
+ * This struct is written to disk in big-endian format,
+ * but operated upon in native-endian format.
+ */
+struct QCryptoBlockLUKSKeySlot {
+    /* state of keyslot, enabled/disable */
+    uint32_t active;
+    /* iterations for PBKDF2 */
+    uint32_t iterations;
+    /* salt for PBKDF2 */
+    uint8_t salt[QCRYPTO_BLOCK_LUKS_SALT_LEN];
+    /* start sector of key material */
+    uint32_t key_offset_sector;
+    /* number of anti-forensic stripes */
+    uint32_t stripes;
+};
+
+/*
+ * This struct is written to disk in big-endian format,
+ * but operated upon in native-endian format.
+ */
+struct QCryptoBlockLUKSHeader {
+    /* 'L', 'U', 'K', 'S', '0xBA', '0xBE' */
+    char magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN];
+
+    /* LUKS version, currently 1 */
+    uint16_t version;
+
+    /* cipher name specification (aes, etc) */
+    char cipher_name[QCRYPTO_BLOCK_LUKS_CIPHER_NAME_LEN];
+
+    /* cipher mode specification (cbc-plain, xts-essiv:sha256, etc) */
+    char cipher_mode[QCRYPTO_BLOCK_LUKS_CIPHER_MODE_LEN];
+
+    /* hash specification (sha256, etc) */
+    char hash_spec[QCRYPTO_BLOCK_LUKS_HASH_SPEC_LEN];
+
+    /* start offset of the volume data (in 512 byte sectors) */
+    uint32_t payload_offset_sector;
+
+    /* Number of key bytes */
+    uint32_t master_key_len;
+
+    /* master key checksum after PBKDF2 */
+    uint8_t master_key_digest[QCRYPTO_BLOCK_LUKS_DIGEST_LEN];
+
+    /* salt for master key PBKDF2 */
+    uint8_t master_key_salt[QCRYPTO_BLOCK_LUKS_SALT_LEN];
+
+    /* iterations for master key PBKDF2 */
+    uint32_t master_key_iterations;
+
+    /* UUID of the partition in standard ASCII representation */
+    uint8_t uuid[QCRYPTO_BLOCK_LUKS_UUID_LEN];
+
+    /* key slots */
+    QCryptoBlockLUKSKeySlot key_slots[QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS];
+};
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 254490c256..375cce44cd 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -23,6 +23,7 @@
 #include "qemu/bswap.h"
 
 #include "block-luks.h"
+#include "block-luks-priv.h"
 
 #include "crypto/hash.h"
 #include "crypto/afsplit.h"
@@ -46,37 +47,6 @@
  */
 
 typedef struct QCryptoBlockLUKS QCryptoBlockLUKS;
-typedef struct QCryptoBlockLUKSHeader QCryptoBlockLUKSHeader;
-typedef struct QCryptoBlockLUKSKeySlot QCryptoBlockLUKSKeySlot;
-
-
-/* The following constants are all defined by the LUKS spec */
-#define QCRYPTO_BLOCK_LUKS_VERSION 1
-
-#define QCRYPTO_BLOCK_LUKS_MAGIC_LEN 6
-#define QCRYPTO_BLOCK_LUKS_CIPHER_NAME_LEN 32
-#define QCRYPTO_BLOCK_LUKS_CIPHER_MODE_LEN 32
-#define QCRYPTO_BLOCK_LUKS_HASH_SPEC_LEN 32
-#define QCRYPTO_BLOCK_LUKS_DIGEST_LEN 20
-#define QCRYPTO_BLOCK_LUKS_SALT_LEN 32
-#define QCRYPTO_BLOCK_LUKS_UUID_LEN 40
-#define QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS 8
-#define QCRYPTO_BLOCK_LUKS_STRIPES 4000
-#define QCRYPTO_BLOCK_LUKS_MIN_SLOT_KEY_ITERS 1000
-#define QCRYPTO_BLOCK_LUKS_MIN_MASTER_KEY_ITERS 1000
-#define QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET 4096
-
-#define QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED 0x0000DEAD
-#define QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED 0x00AC71F3
-
-#define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL
-
-#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS 2000
-#define QCRYPTO_BLOCK_LUKS_ERASE_ITERATIONS 40
-
-static const char qcrypto_block_luks_magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN] = {
-    'L', 'U', 'K', 'S', 0xBA, 0xBE
-};
 
 typedef struct QCryptoBlockLUKSNameMap QCryptoBlockLUKSNameMap;
 struct QCryptoBlockLUKSNameMap {
@@ -134,69 +104,7 @@ qcrypto_block_luks_cipher_name_map[] = {
     { "twofish", qcrypto_block_luks_cipher_size_map_twofish },
 };
 
-
-/*
- * This struct is written to disk in big-endian format,
- * but operated upon in native-endian format.
- */
-struct QCryptoBlockLUKSKeySlot {
-    /* state of keyslot, enabled/disable */
-    uint32_t active;
-    /* iterations for PBKDF2 */
-    uint32_t iterations;
-    /* salt for PBKDF2 */
-    uint8_t salt[QCRYPTO_BLOCK_LUKS_SALT_LEN];
-    /* start sector of key material */
-    uint32_t key_offset_sector;
-    /* number of anti-forensic stripes */
-    uint32_t stripes;
-};
-
 QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSKeySlot) != 48);
-
-
-/*
- * This struct is written to disk in big-endian format,
- * but operated upon in native-endian format.
- */
-struct QCryptoBlockLUKSHeader {
-    /* 'L', 'U', 'K', 'S', '0xBA', '0xBE' */
-    char magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN];
-
-    /* LUKS version, currently 1 */
-    uint16_t version;
-
-    /* cipher name specification (aes, etc) */
-    char cipher_name[QCRYPTO_BLOCK_LUKS_CIPHER_NAME_LEN];
-
-    /* cipher mode specification (cbc-plain, xts-essiv:sha256, etc) */
-    char cipher_mode[QCRYPTO_BLOCK_LUKS_CIPHER_MODE_LEN];
-
-    /* hash specification (sha256, etc) */
-    char hash_spec[QCRYPTO_BLOCK_LUKS_HASH_SPEC_LEN];
-
-    /* start offset of the volume data (in 512 byte sectors) */
-    uint32_t payload_offset_sector;
-
-    /* Number of key bytes */
-    uint32_t master_key_len;
-
-    /* master key checksum after PBKDF2 */
-    uint8_t master_key_digest[QCRYPTO_BLOCK_LUKS_DIGEST_LEN];
-
-    /* salt for master key PBKDF2 */
-    uint8_t master_key_salt[QCRYPTO_BLOCK_LUKS_SALT_LEN];
-
-    /* iterations for master key PBKDF2 */
-    uint32_t master_key_iterations;
-
-    /* UUID of the partition in standard ASCII representation */
-    uint8_t uuid[QCRYPTO_BLOCK_LUKS_UUID_LEN];
-
-    /* key slots */
-    QCryptoBlockLUKSKeySlot key_slots[QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS];
-};
-
 QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSHeader) != 592);
 
 
-- 
2.37.3

[PULL 17/20] crypto: split off helpers for converting LUKS header endianess

[Daniel P. Berrangé]

The unit test suite is shortly going to want to convert header
endianness separately from the main I/O functions.

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/block-luks-priv.h |  6 +++
 crypto/block-luks.c      | 79 ++++++++++++++++++++++++----------------
 2 files changed, 53 insertions(+), 32 deletions(-)

diff --git a/crypto/block-luks-priv.h b/crypto/block-luks-priv.h
index 1516571dcb..90a20d432b 100644
--- a/crypto/block-luks-priv.h
+++ b/crypto/block-luks-priv.h
@@ -135,3 +135,9 @@ struct QCryptoBlockLUKSHeader {
     /* key slots */
     QCryptoBlockLUKSKeySlot key_slots[QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS];
 };
+
+
+void
+qcrypto_block_luks_to_disk_endian(QCryptoBlockLUKSHeader *hdr);
+void
+qcrypto_block_luks_from_disk_endian(QCryptoBlockLUKSHeader *hdr);
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 375cce44cd..bb89c10225 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -348,6 +348,51 @@ qcrypto_block_luks_splitkeylen_sectors(const QCryptoBlockLUKS *luks,
     return ROUND_UP(splitkeylen_sectors, header_sectors);
 }
 
+
+void
+qcrypto_block_luks_to_disk_endian(QCryptoBlockLUKSHeader *hdr)
+{
+    size_t i;
+
+    /*
+     * Everything on disk uses Big Endian (tm), so flip header fields
+     * before writing them
+     */
+    cpu_to_be16s(&hdr->version);
+    cpu_to_be32s(&hdr->payload_offset_sector);
+    cpu_to_be32s(&hdr->master_key_len);
+    cpu_to_be32s(&hdr->master_key_iterations);
+
+    for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
+        cpu_to_be32s(&hdr->key_slots[i].active);
+        cpu_to_be32s(&hdr->key_slots[i].iterations);
+        cpu_to_be32s(&hdr->key_slots[i].key_offset_sector);
+        cpu_to_be32s(&hdr->key_slots[i].stripes);
+    }
+}
+
+void
+qcrypto_block_luks_from_disk_endian(QCryptoBlockLUKSHeader *hdr)
+{
+    size_t i;
+
+    /*
+     * The header is always stored in big-endian format, so
+     * convert everything to native
+     */
+    be16_to_cpus(&hdr->version);
+    be32_to_cpus(&hdr->payload_offset_sector);
+    be32_to_cpus(&hdr->master_key_len);
+    be32_to_cpus(&hdr->master_key_iterations);
+
+    for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
+        be32_to_cpus(&hdr->key_slots[i].active);
+        be32_to_cpus(&hdr->key_slots[i].iterations);
+        be32_to_cpus(&hdr->key_slots[i].key_offset_sector);
+        be32_to_cpus(&hdr->key_slots[i].stripes);
+    }
+}
+
 /*
  * Stores the main LUKS header, taking care of endianess
  */
@@ -359,28 +404,13 @@ qcrypto_block_luks_store_header(QCryptoBlock *block,
 {
     const QCryptoBlockLUKS *luks = block->opaque;
     Error *local_err = NULL;
-    size_t i;
     g_autofree QCryptoBlockLUKSHeader *hdr_copy = NULL;
 
     /* Create a copy of the header */
     hdr_copy = g_new0(QCryptoBlockLUKSHeader, 1);
     memcpy(hdr_copy, &luks->header, sizeof(QCryptoBlockLUKSHeader));
 
-    /*
-     * Everything on disk uses Big Endian (tm), so flip header fields
-     * before writing them
-     */
-    cpu_to_be16s(&hdr_copy->version);
-    cpu_to_be32s(&hdr_copy->payload_offset_sector);
-    cpu_to_be32s(&hdr_copy->master_key_len);
-    cpu_to_be32s(&hdr_copy->master_key_iterations);
-
-    for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
-        cpu_to_be32s(&hdr_copy->key_slots[i].active);
-        cpu_to_be32s(&hdr_copy->key_slots[i].iterations);
-        cpu_to_be32s(&hdr_copy->key_slots[i].key_offset_sector);
-        cpu_to_be32s(&hdr_copy->key_slots[i].stripes);
-    }
+    qcrypto_block_luks_to_disk_endian(hdr_copy);
 
     /* Write out the partition header and key slot headers */
     writefunc(block, 0, (const uint8_t *)hdr_copy, sizeof(*hdr_copy),
@@ -404,7 +434,6 @@ qcrypto_block_luks_load_header(QCryptoBlock *block,
                                 Error **errp)
 {
     int rv;
-    size_t i;
     QCryptoBlockLUKS *luks = block->opaque;
 
     /*
@@ -420,21 +449,7 @@ qcrypto_block_luks_load_header(QCryptoBlock *block,
         return rv;
     }
 
-    /*
-     * The header is always stored in big-endian format, so
-     * convert everything to native
-     */
-    be16_to_cpus(&luks->header.version);
-    be32_to_cpus(&luks->header.payload_offset_sector);
-    be32_to_cpus(&luks->header.master_key_len);
-    be32_to_cpus(&luks->header.master_key_iterations);
-
-    for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
-        be32_to_cpus(&luks->header.key_slots[i].active);
-        be32_to_cpus(&luks->header.key_slots[i].iterations);
-        be32_to_cpus(&luks->header.key_slots[i].key_offset_sector);
-        be32_to_cpus(&luks->header.key_slots[i].stripes);
-    }
+    qcrypto_block_luks_from_disk_endian(&luks->header);
 
     return 0;
 }
-- 
2.37.3

[PULL 18/20] crypto: quote algorithm names in error messages

[Daniel P. Berrangé]

If given a malformed LUKS header, it is possible that the algorithm
names end up being an empty string. This leads to confusing error
messages unless quoting is used to highlight where the empty string
is subsituted in the error message.

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/block-luks.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index bb89c10225..df2b4105d6 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -162,7 +162,7 @@ static int qcrypto_block_luks_cipher_name_lookup(const char *name,
         }
     }
 
-    error_setg(errp, "Algorithm %s with key size %d bytes not supported",
+    error_setg(errp, "Algorithm '%s' with key size %d bytes not supported",
                name, key_bytes);
     return 0;
 }
@@ -198,7 +198,7 @@ static int qcrypto_block_luks_name_lookup(const char *name,
     int ret = qapi_enum_parse(map, name, -1, NULL);
 
     if (ret < 0) {
-        error_setg(errp, "%s %s not supported", type, name);
+        error_setg(errp, "%s '%s' not supported", type, name);
         return 0;
     }
     return ret;
@@ -592,7 +592,7 @@ qcrypto_block_luks_parse_header(QCryptoBlockLUKS *luks, Error **errp)
      */
     ivgen_name = strchr(cipher_mode, '-');
     if (!ivgen_name) {
-        error_setg(errp, "Unexpected cipher mode string format %s",
+        error_setg(errp, "Unexpected cipher mode string format '%s'",
                    luks->header.cipher_mode);
         return -1;
     }
-- 
2.37.3

[PULL 19/20] crypto: ensure LUKS tests run with GNUTLS crypto provider

[Daniel P. Berrangé]

GNUTLS is supported as a crypto provider since

  commit cc4c7c738297958b3d1d16269f57d71d22f5a9ff
  Author: Daniel P. Berrangé <berrange@redhat.com>
  Date:   Wed Jun 30 17:20:02 2021 +0100

    crypto: introduce build system for gnutls crypto backend

So enable the LUKS tests in this config.

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/unit/test-crypto-block.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/unit/test-crypto-block.c b/tests/unit/test-crypto-block.c
index 3417b67be5..3d50eb4b6e 100644
--- a/tests/unit/test-crypto-block.c
+++ b/tests/unit/test-crypto-block.c
@@ -30,7 +30,8 @@
 #endif
 
 #if (defined(_WIN32) || defined RUSAGE_THREAD) && \
-    (defined(CONFIG_NETTLE) || defined(CONFIG_GCRYPT))
+    (defined(CONFIG_NETTLE) || defined(CONFIG_GCRYPT) || \
+     defined(CONFIG_GNUTLS_CRYPTO))
 #define TEST_LUKS
 #else
 #undef TEST_LUKS
-- 
2.37.3

[PULL 20/20] crypto: add test cases for many malformed LUKS header scenarios

[Daniel P. Berrangé]

Validate that we diagnose each malformed LUKS header scenario with a
distinct error report.

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/unit/test-crypto-block.c | 299 +++++++++++++++++++++++++++++++++
 1 file changed, 299 insertions(+)

diff --git a/tests/unit/test-crypto-block.c b/tests/unit/test-crypto-block.c
index 3d50eb4b6e..b629e240a9 100644
--- a/tests/unit/test-crypto-block.c
+++ b/tests/unit/test-crypto-block.c
@@ -22,6 +22,7 @@
 #include "qapi/error.h"
 #include "crypto/init.h"
 #include "crypto/block.h"
+#include "crypto/block-luks-priv.h"
 #include "qemu/buffer.h"
 #include "qemu/module.h"
 #include "crypto/secret.h"
@@ -345,6 +346,230 @@ static void test_block(gconstpointer opaque)
 }
 
 
+#ifdef TEST_LUKS
+typedef const char *(*LuksHeaderDoBadStuff)(QCryptoBlockLUKSHeader *hdr);
+
+static void
+test_luks_bad_header(gconstpointer data)
+{
+    LuksHeaderDoBadStuff badstuff = data;
+    QCryptoBlock *blk;
+    Buffer buf;
+    Object *sec = test_block_secret();
+    QCryptoBlockLUKSHeader hdr;
+    Error *err = NULL;
+    const char *msg;
+
+    memset(&buf, 0, sizeof(buf));
+    buffer_init(&buf, "header");
+
+    /* Correctly create the volume initially */
+    blk = qcrypto_block_create(&luks_create_opts_default, NULL,
+                               test_block_init_func,
+                               test_block_write_func,
+                               &buf,
+                               &error_abort);
+    g_assert(blk);
+
+    qcrypto_block_free(blk);
+
+    /* Mangle it in some unpleasant way */
+    g_assert(buf.offset >= sizeof(hdr));
+    memcpy(&hdr, buf.buffer, sizeof(hdr));
+    qcrypto_block_luks_to_disk_endian(&hdr);
+
+    msg = badstuff(&hdr);
+
+    qcrypto_block_luks_from_disk_endian(&hdr);
+    memcpy(buf.buffer, &hdr, sizeof(hdr));
+
+    /* Check that we fail to open it again */
+    blk = qcrypto_block_open(&luks_open_opts, NULL,
+                             test_block_read_func,
+                             &buf,
+                             0,
+                             1,
+                             &err);
+    g_assert(!blk);
+    g_assert(err);
+
+    g_assert_cmpstr(error_get_pretty(err), ==, msg);
+    error_free(err);
+
+    object_unparent(sec);
+
+    buffer_free(&buf);
+}
+
+static const char *luks_bad_null_term_cipher_name(QCryptoBlockLUKSHeader *hdr)
+{
+    /* Replace NUL termination with spaces */
+    char *offset = hdr->cipher_name + strlen(hdr->cipher_name);
+    memset(offset, ' ', sizeof(hdr->cipher_name) - (offset - hdr->cipher_name));
+
+    return "LUKS header cipher name is not NUL terminated";
+}
+
+static const char *luks_bad_null_term_cipher_mode(QCryptoBlockLUKSHeader *hdr)
+{
+    /* Replace NUL termination with spaces */
+    char *offset = hdr->cipher_mode + strlen(hdr->cipher_mode);
+    memset(offset, ' ', sizeof(hdr->cipher_mode) - (offset - hdr->cipher_mode));
+
+    return "LUKS header cipher mode is not NUL terminated";
+}
+
+static const char *luks_bad_null_term_hash_spec(QCryptoBlockLUKSHeader *hdr)
+{
+    /* Replace NUL termination with spaces */
+    char *offset = hdr->hash_spec + strlen(hdr->hash_spec);
+    memset(offset, ' ', sizeof(hdr->hash_spec) - (offset - hdr->hash_spec));
+
+    return "LUKS header hash spec is not NUL terminated";
+}
+
+static const char *luks_bad_cipher_name_empty(QCryptoBlockLUKSHeader *hdr)
+{
+    memcpy(hdr->cipher_name, "", 1);
+
+    return "Algorithm '' with key size 32 bytes not supported";
+}
+
+static const char *luks_bad_cipher_name_unknown(QCryptoBlockLUKSHeader *hdr)
+{
+    memcpy(hdr->cipher_name, "aess", 5);
+
+    return "Algorithm 'aess' with key size 32 bytes not supported";
+}
+
+static const char *luks_bad_cipher_xts_size(QCryptoBlockLUKSHeader *hdr)
+{
+    hdr->master_key_len = 33;
+
+    return "XTS cipher key length should be a multiple of 2";
+}
+
+static const char *luks_bad_cipher_cbc_size(QCryptoBlockLUKSHeader *hdr)
+{
+    hdr->master_key_len = 33;
+    memcpy(hdr->cipher_mode, "cbc-essiv", 10);
+
+    return "Algorithm 'aes' with key size 33 bytes not supported";
+}
+
+static const char *luks_bad_cipher_mode_empty(QCryptoBlockLUKSHeader *hdr)
+{
+    memcpy(hdr->cipher_mode, "", 1);
+
+    return "Unexpected cipher mode string format ''";
+}
+
+static const char *luks_bad_cipher_mode_unknown(QCryptoBlockLUKSHeader *hdr)
+{
+    memcpy(hdr->cipher_mode, "xfs", 4);
+
+    return "Unexpected cipher mode string format 'xfs'";
+}
+
+static const char *luks_bad_ivgen_separator(QCryptoBlockLUKSHeader *hdr)
+{
+    memcpy(hdr->cipher_mode, "xts:plain64", 12);
+
+    return "Unexpected cipher mode string format 'xts:plain64'";
+}
+
+static const char *luks_bad_ivgen_name_empty(QCryptoBlockLUKSHeader *hdr)
+{
+    memcpy(hdr->cipher_mode, "xts-", 5);
+
+    return "IV generator '' not supported";
+}
+
+static const char *luks_bad_ivgen_name_unknown(QCryptoBlockLUKSHeader *hdr)
+{
+    memcpy(hdr->cipher_mode, "xts-plain65", 12);
+
+    return "IV generator 'plain65' not supported";
+}
+
+static const char *luks_bad_ivgen_hash_empty(QCryptoBlockLUKSHeader *hdr)
+{
+    memcpy(hdr->cipher_mode, "xts-plain65:", 13);
+
+    return "Hash algorithm '' not supported";
+}
+
+static const char *luks_bad_ivgen_hash_unknown(QCryptoBlockLUKSHeader *hdr)
+{
+    memcpy(hdr->cipher_mode, "xts-plain65:sha257", 19);
+
+    return "Hash algorithm 'sha257' not supported";
+}
+
+static const char *luks_bad_hash_spec_empty(QCryptoBlockLUKSHeader *hdr)
+{
+    memcpy(hdr->hash_spec, "", 1);
+
+    return "Hash algorithm '' not supported";
+}
+
+static const char *luks_bad_hash_spec_unknown(QCryptoBlockLUKSHeader *hdr)
+{
+    memcpy(hdr->hash_spec, "sha2566", 8);
+
+    return "Hash algorithm 'sha2566' not supported";
+}
+
+static const char *luks_bad_stripes(QCryptoBlockLUKSHeader *hdr)
+{
+    hdr->key_slots[0].stripes = 3999;
+
+    return "Keyslot 0 is corrupted (stripes 3999 != 4000)";
+}
+
+static const char *luks_bad_key_overlap_header(QCryptoBlockLUKSHeader *hdr)
+{
+    hdr->key_slots[0].key_offset_sector = 2;
+
+    return "Keyslot 0 is overlapping with the LUKS header";
+}
+
+static const char *luks_bad_key_overlap_key(QCryptoBlockLUKSHeader *hdr)
+{
+    hdr->key_slots[0].key_offset_sector = hdr->key_slots[1].key_offset_sector;
+
+    return "Keyslots 0 and 1 are overlapping in the header";
+}
+
+static const char *luks_bad_key_overlap_payload(QCryptoBlockLUKSHeader *hdr)
+{
+    hdr->key_slots[0].key_offset_sector = hdr->payload_offset_sector + 42;
+
+    return "Keyslot 0 is overlapping with the encrypted payload";
+}
+
+static const char *luks_bad_payload_overlap_header(QCryptoBlockLUKSHeader *hdr)
+{
+    hdr->payload_offset_sector = 2;
+
+    return "LUKS payload is overlapping with the header";
+}
+
+static const char *luks_bad_key_iterations(QCryptoBlockLUKSHeader *hdr)
+{
+    hdr->key_slots[0].iterations = 0;
+
+    return "Keyslot 0 iteration count is zero";
+}
+
+static const char *luks_bad_iterations(QCryptoBlockLUKSHeader *hdr)
+{
+    hdr->master_key_iterations = 0;
+
+    return "LUKS key iteration count is zero";
+}
+#endif
+
 int main(int argc, char **argv)
 {
     gsize i;
@@ -365,5 +590,79 @@ int main(int argc, char **argv)
         }
     }
 
+#ifdef TEST_LUKS
+    if (g_test_slow()) {
+        g_test_add_data_func("/crypto/block/luks/bad/cipher-name-nul-term",
+                             luks_bad_null_term_cipher_name,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/cipher-mode-nul-term",
+                             luks_bad_null_term_cipher_mode,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/hash-spec-nul-term",
+                             luks_bad_null_term_hash_spec,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/cipher-name-empty",
+                             luks_bad_cipher_name_empty,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/cipher-name-unknown",
+                             luks_bad_cipher_name_unknown,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/cipher-xts-size",
+                             luks_bad_cipher_xts_size,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/cipher-cbc-size",
+                             luks_bad_cipher_cbc_size,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/cipher-mode-empty",
+                             luks_bad_cipher_mode_empty,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/cipher-mode-unknown",
+                             luks_bad_cipher_mode_unknown,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/ivgen-separator",
+                             luks_bad_ivgen_separator,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/ivgen-name-empty",
+                             luks_bad_ivgen_name_empty,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/ivgen-name-unknown",
+                             luks_bad_ivgen_name_unknown,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/ivgen-hash-empty",
+                             luks_bad_ivgen_hash_empty,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/ivgen-hash-unknown",
+                             luks_bad_ivgen_hash_unknown,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/hash-spec-empty",
+                             luks_bad_hash_spec_empty,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/hash-spec-unknown",
+                             luks_bad_hash_spec_unknown,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/stripes",
+                             luks_bad_stripes,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/key-overlap-header",
+                             luks_bad_key_overlap_header,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/key-overlap-key",
+                             luks_bad_key_overlap_key,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/key-overlap-payload",
+                             luks_bad_key_overlap_payload,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/payload-overlap-header",
+                             luks_bad_payload_overlap_header,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/iterations",
+                             luks_bad_iterations,
+                             test_luks_bad_header);
+        g_test_add_data_func("/crypto/block/luks/bad/key-iterations",
+                             luks_bad_key_iterations,
+                             test_luks_bad_header);
+    }
+#endif
+
     return g_test_run();
 }
-- 
2.37.3

Re: [PULL 00/20] Crypto and I/O patches

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 115 bytes --]

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/7.2 for any user-visible changes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]