[Qemu-devel] [PATCH] crypt: fix build with nettle >= 3.0.0

[Qemu-devel] [PATCH] crypt: fix build with nettle >= 3.0.0

[Radim Krčmář]

In nettle 3, cbc_encrypt() accepts 'nettle_cipher_func' instead of
'nettle_crypt_func' and these two differ in 'const' qualifier of the
first argument.  The build fails with:

  In file included from crypto/cipher.c:71:0:
  ./crypto/cipher-nettle.c: In function ‘qcrypto_cipher_encrypt’:
  ./crypto/cipher-nettle.c:154:38: error: passing argument 2 of
  ‘nettle_cbc_encrypt’ from incompatible pointer type
           cbc_encrypt(ctx->ctx_encrypt, ctx->alg_encrypt,
                                               ^
  In file included from ./crypto/cipher-nettle.c:24:0,
                   from crypto/cipher.c:71:
  /usr/include/nettle/cbc.h:48:1: note: expected
  ‘void (*)(const void *, size_t, uint8_t *, const uint8_t *)
  but argument is of type
  ‘void (*)(      void *, size_t, uint8_t *, const uint8_t *)

To allow both versions, we switch to the new definition and #if typedef
it for old versions.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 I don't know if we want to kill the #if compatibility after a while
 (so QEMU doesn't become glibc-like) -- I could split this patch into
 two, where the first one would just be reverted.

 configure              |  4 +++-
 crypto/cipher-nettle.c | 16 ++++++++++------
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/configure b/configure
index 33b945530e64..cc0338ddbd14 100755
--- a/configure
+++ b/configure
@@ -2183,6 +2183,7 @@ if test "$gnutls_nettle" != "no"; then
     if $pkg_config --exists "nettle"; then
         nettle_cflags=`$pkg_config --cflags nettle`
         nettle_libs=`$pkg_config --libs nettle`
+        nettle_version=`$pkg_config --modversion nettle`
         libs_softmmu="$nettle_libs $libs_softmmu"
         libs_tools="$nettle_libs $libs_tools"
         QEMU_CFLAGS="$QEMU_CFLAGS $nettle_cflags"
@@ -4490,7 +4491,7 @@ echo "GTK support       $gtk"
 echo "GNUTLS support    $gnutls"
 echo "GNUTLS hash       $gnutls_hash"
 echo "GNUTLS gcrypt     $gnutls_gcrypt"
-echo "GNUTLS nettle     $gnutls_nettle"
+echo "GNUTLS nettle     $gnutls_nettle ${gnutls_nettle+($nettle_version)}"
 echo "VTE support       $vte"
 echo "curses support    $curses"
 echo "curl support      $curl"
@@ -4858,6 +4859,7 @@ if test "$gnutls_gcrypt" = "yes" ; then
 fi
 if test "$gnutls_nettle" = "yes" ; then
   echo "CONFIG_GNUTLS_NETTLE=y" >> $config_host_mak
+  echo "CONFIG_NETTLE_VERSION_MAJOR=${nettle_version%%.*}" >> $config_host_mak
 fi
 if test "$vte" = "yes" ; then
   echo "CONFIG_VTE=y" >> $config_host_mak
diff --git a/crypto/cipher-nettle.c b/crypto/cipher-nettle.c
index e5a14bc1393f..e61aaa29f049 100644
--- a/crypto/cipher-nettle.c
+++ b/crypto/cipher-nettle.c
@@ -23,12 +23,16 @@
 #include <nettle/des.h>
 #include <nettle/cbc.h>
 
+#if CONFIG_NETTLE_VERSION_MAJOR < 3
+typedef nettle_crypt_func nettle_cipher_func;
+#endif
+
 typedef struct QCryptoCipherNettle QCryptoCipherNettle;
 struct QCryptoCipherNettle {
     void *ctx_encrypt;
     void *ctx_decrypt;
-    nettle_crypt_func *alg_encrypt;
-    nettle_crypt_func *alg_decrypt;
+    nettle_cipher_func *alg_encrypt;
+    nettle_cipher_func *alg_decrypt;
     uint8_t *iv;
     size_t niv;
 };
@@ -83,8 +87,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
         des_set_key(ctx->ctx_encrypt, rfbkey);
         g_free(rfbkey);
 
-        ctx->alg_encrypt = (nettle_crypt_func *)des_encrypt;
-        ctx->alg_decrypt = (nettle_crypt_func *)des_decrypt;
+        ctx->alg_encrypt = (nettle_cipher_func *)des_encrypt;
+        ctx->alg_decrypt = (nettle_cipher_func *)des_decrypt;
 
         ctx->niv = DES_BLOCK_SIZE;
         break;
@@ -98,8 +102,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
         aes_set_encrypt_key(ctx->ctx_encrypt, nkey, key);
         aes_set_decrypt_key(ctx->ctx_decrypt, nkey, key);
 
-        ctx->alg_encrypt = (nettle_crypt_func *)aes_encrypt;
-        ctx->alg_decrypt = (nettle_crypt_func *)aes_decrypt;
+        ctx->alg_encrypt = (nettle_cipher_func *)aes_encrypt;
+        ctx->alg_decrypt = (nettle_cipher_func *)aes_decrypt;
 
         ctx->niv = AES_BLOCK_SIZE;
         break;
-- 
2.4.5

Re: [Qemu-devel] [PATCH] crypt: fix build with nettle >= 3.0.0

[Peter Maydell]

On 10 July 2015 at 13:33, Radim Krčmář <rkrcmar@redhat.com> wrote:
> In nettle 3, cbc_encrypt() accepts 'nettle_cipher_func' instead of
> 'nettle_crypt_func' and these two differ in 'const' qualifier of the
> first argument.  The build fails with:
>
>   In file included from crypto/cipher.c:71:0:
>   ./crypto/cipher-nettle.c: In function ‘qcrypto_cipher_encrypt’:
>   ./crypto/cipher-nettle.c:154:38: error: passing argument 2 of
>   ‘nettle_cbc_encrypt’ from incompatible pointer type
>            cbc_encrypt(ctx->ctx_encrypt, ctx->alg_encrypt,
>                                                ^
>   In file included from ./crypto/cipher-nettle.c:24:0,
>                    from crypto/cipher.c:71:
>   /usr/include/nettle/cbc.h:48:1: note: expected
>   ‘void (*)(const void *, size_t, uint8_t *, const uint8_t *)
>   but argument is of type
>   ‘void (*)(      void *, size_t, uint8_t *, const uint8_t *)
>
> To allow both versions, we switch to the new definition and #if typedef
> it for old versions.
>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  I don't know if we want to kill the #if compatibility after a while
>  (so QEMU doesn't become glibc-like) -- I could split this patch into
>  two, where the first one would just be reverted.

We might eventually kill the compat, but we'd probably
want to do it while retaining a version check in configure,
so I'd just leave it as one patch.

> @@ -83,8 +87,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
>          des_set_key(ctx->ctx_encrypt, rfbkey);
>          g_free(rfbkey);
>
> -        ctx->alg_encrypt = (nettle_crypt_func *)des_encrypt;
> -        ctx->alg_decrypt = (nettle_crypt_func *)des_decrypt;
> +        ctx->alg_encrypt = (nettle_cipher_func *)des_encrypt;
> +        ctx->alg_decrypt = (nettle_cipher_func *)des_decrypt;
>
>          ctx->niv = DES_BLOCK_SIZE;
>          break;
> @@ -98,8 +102,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
>          aes_set_encrypt_key(ctx->ctx_encrypt, nkey, key);
>          aes_set_decrypt_key(ctx->ctx_decrypt, nkey, key);
>
> -        ctx->alg_encrypt = (nettle_crypt_func *)aes_encrypt;
> -        ctx->alg_decrypt = (nettle_crypt_func *)aes_decrypt;
> +        ctx->alg_encrypt = (nettle_cipher_func *)aes_encrypt;
> +        ctx->alg_decrypt = (nettle_cipher_func *)aes_decrypt;

Why do we need the casts here at all? If the functions
we're passing around don't have the right signature
anyway we're in big trouble and casting them is
just going to hide the problem until runtime...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] crypt: fix build with nettle >= 3.0.0

[Radim Krčmář]

2015-07-10 13:56+0100, Peter Maydell:
> On 10 July 2015 at 13:33, Radim Krčmář <rkrcmar@redhat.com> wrote:
>> @@ -83,8 +87,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
>> -        ctx->alg_encrypt = (nettle_crypt_func *)des_encrypt;
>> -        ctx->alg_decrypt = (nettle_crypt_func *)des_decrypt;
>> +        ctx->alg_encrypt = (nettle_cipher_func *)des_encrypt;
>> +        ctx->alg_decrypt = (nettle_cipher_func *)des_decrypt;
>> @@ -98,8 +102,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
>> -        ctx->alg_encrypt = (nettle_crypt_func *)aes_encrypt;
>> -        ctx->alg_decrypt = (nettle_crypt_func *)aes_decrypt;
>> +        ctx->alg_encrypt = (nettle_cipher_func *)aes_encrypt;
>> +        ctx->alg_decrypt = (nettle_cipher_func *)aes_decrypt;
> 
> Why do we need the casts here at all?  If the functions
> we're passing around don't have the right signature
> anyway we're in big trouble and casting them is
> just going to hide the problem until runtime...

Yes.

We pass 'ctx' as a 'void *' in the code, but these functions accept
specialized structures, which makes them incompatible:

  void nettle_cipher_func(const void *ctx, size_t length, [...])

  void aes_decrypt(const struct aes_ctx *ctx, size_t length, [...])
  void des_decrypt(const struct des_ctx *ctx, size_t length, [...])

We could take make struct QCryptoCipherNettle into an aes/des union
(it's already tagged by QCryptoCipher->mode) to make coding a bit safer,
but C types can't guarantee everything.  Also, cbc_encrypt() takes
nettle_cipher_func, so we need to directly cast at some point.

Re: [Qemu-devel] [PATCH] crypt: fix build with nettle >= 3.0.0

[Peter Maydell]

On 10 July 2015 at 14:31, Radim Krčmář <rkrcmar@redhat.com> wrote:
> 2015-07-10 13:56+0100, Peter Maydell:
>> On 10 July 2015 at 13:33, Radim Krčmář <rkrcmar@redhat.com> wrote:
>>> @@ -83,8 +87,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
>>> -        ctx->alg_encrypt = (nettle_crypt_func *)des_encrypt;
>>> -        ctx->alg_decrypt = (nettle_crypt_func *)des_decrypt;
>>> +        ctx->alg_encrypt = (nettle_cipher_func *)des_encrypt;
>>> +        ctx->alg_decrypt = (nettle_cipher_func *)des_decrypt;
>>> @@ -98,8 +102,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
>>> -        ctx->alg_encrypt = (nettle_crypt_func *)aes_encrypt;
>>> -        ctx->alg_decrypt = (nettle_crypt_func *)aes_decrypt;
>>> +        ctx->alg_encrypt = (nettle_cipher_func *)aes_encrypt;
>>> +        ctx->alg_decrypt = (nettle_cipher_func *)aes_decrypt;
>>
>> Why do we need the casts here at all?  If the functions
>> we're passing around don't have the right signature
>> anyway we're in big trouble and casting them is
>> just going to hide the problem until runtime...
>
> Yes.
>
> We pass 'ctx' as a 'void *' in the code, but these functions accept
> specialized structures, which makes them incompatible:
>
>   void nettle_cipher_func(const void *ctx, size_t length, [...])
>
>   void aes_decrypt(const struct aes_ctx *ctx, size_t length, [...])
>   void des_decrypt(const struct des_ctx *ctx, size_t length, [...])

But aren't both the typedef and the aes/des_decrypt functions
provided by the nettle library? Why is the library providing
functions whose prototypes don't match its own typedef?

-- PMM

Re: [Qemu-devel] [PATCH] crypt: fix build with nettle >= 3.0.0

[Peter Maydell]

On 10 July 2015 at 14:38, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 10 July 2015 at 14:31, Radim Krčmář <rkrcmar@redhat.com> wrote:
>> We pass 'ctx' as a 'void *' in the code, but these functions accept
>> specialized structures, which makes them incompatible:
>>
>>   void nettle_cipher_func(const void *ctx, size_t length, [...])
>>
>>   void aes_decrypt(const struct aes_ctx *ctx, size_t length, [...])
>>   void des_decrypt(const struct des_ctx *ctx, size_t length, [...])
>
> But aren't both the typedef and the aes/des_decrypt functions
> provided by the nettle library? Why is the library providing
> functions whose prototypes don't match its own typedef?

I had a suspicion that this was undefined behaviour,
and I was right:

http://stackoverflow.com/questions/559581/casting-a-function-pointer-to-another-type/14044244#14044244

If you have a function that takes "const struct foo *f"
but the library code is calling it using a function pointer
whose type says it's "const void *", then you can't just cast
the function pointer before handing it to the library.
You need to provide the obvious wrapper:

void aes_decrypt_wrapper(const void *ctx, size_t length, ...)
{
    aes_decrypt(ctx, length, ...);
}

-- PMM

Re: [Qemu-devel] [PATCH] crypt: fix build with nettle >= 3.0.0

[Radim Krčmář]

2015-07-10 14:38+0100, Peter Maydell:
> On 10 July 2015 at 14:31, Radim Krčmář <rkrcmar@redhat.com> wrote:
>> 2015-07-10 13:56+0100, Peter Maydell:
>>> On 10 July 2015 at 13:33, Radim Krčmář <rkrcmar@redhat.com> wrote:
>>>> @@ -83,8 +87,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
>>>> -        ctx->alg_encrypt = (nettle_crypt_func *)des_encrypt;
>>>> -        ctx->alg_decrypt = (nettle_crypt_func *)des_decrypt;
>>>> +        ctx->alg_encrypt = (nettle_cipher_func *)des_encrypt;
>>>> +        ctx->alg_decrypt = (nettle_cipher_func *)des_decrypt;
>>>> @@ -98,8 +102,8 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
>>>> -        ctx->alg_encrypt = (nettle_crypt_func *)aes_encrypt;
>>>> -        ctx->alg_decrypt = (nettle_crypt_func *)aes_decrypt;
>>>> +        ctx->alg_encrypt = (nettle_cipher_func *)aes_encrypt;
>>>> +        ctx->alg_decrypt = (nettle_cipher_func *)aes_decrypt;
>>>
>>> Why do we need the casts here at all?  If the functions
>>> we're passing around don't have the right signature
>>> anyway we're in big trouble and casting them is
>>> just going to hide the problem until runtime...
>>
>> Yes.
>>
>> We pass 'ctx' as a 'void *' in the code, but these functions accept
>> specialized structures, which makes them incompatible:
>>
>>   void nettle_cipher_func(const void *ctx, size_t length, [...])
>>
>>   void aes_decrypt(const struct aes_ctx *ctx, size_t length, [...])
>>   void des_decrypt(const struct des_ctx *ctx, size_t length, [...])
> 
> But aren't both the typedef and the aes/des_decrypt functions
> provided by the nettle library? Why is the library providing
> functions whose prototypes don't match its own typedef?

They are.  Authors needed to sacrifice something to fit into the type
system and I think they valued safety when using just a single cipher
above safety when mixing them ... (The decision was probably biased by
existing unabstracted code, if I were to guess how the library started.)

Re: [Qemu-devel] [PATCH] crypt: fix build with nettle >= 3.0.0

[Radim Krčmář]

2015-07-10 14:56+0100, Peter Maydell:
> On 10 July 2015 at 14:38, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 10 July 2015 at 14:31, Radim Krčmář <rkrcmar@redhat.com> wrote:
>>> We pass 'ctx' as a 'void *' in the code, but these functions accept
>>> specialized structures, which makes them incompatible:
>>>
>>>   void nettle_cipher_func(const void *ctx, size_t length, [...])
>>>
>>>   void aes_decrypt(const struct aes_ctx *ctx, size_t length, [...])
>>>   void des_decrypt(const struct des_ctx *ctx, size_t length, [...])
>>
>> But aren't both the typedef and the aes/des_decrypt functions
>> provided by the nettle library? Why is the library providing
>> functions whose prototypes don't match its own typedef?
> 
> I had a suspicion that this was undefined behaviour,
> and I was right:
> 
> http://stackoverflow.com/questions/559581/casting-a-function-pointer-to-another-type/14044244#14044244
> 
> If you have a function that takes "const struct foo *f"
> but the library code is calling it using a function pointer
> whose type says it's "const void *", then you can't just cast
> the function pointer before handing it to the library.
> You need to provide the obvious wrapper:
> 
> void aes_decrypt_wrapper(const void *ctx, size_t length, ...)
> {
>     aes_decrypt(ctx, length, ...);
> }

Makes sense, thanks.  'void *' shenanigans got me on this one.

(Btw. the library isn't casting it back to the original type before
 calling and its test cases don't use wrappers.)

I've posted v2 that addresses this problem.