Re: [RFC PATCH v2 4/5] crypto/linux_keyring: add 'syskey' secret object.

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Apr 16, 2020 at 01:25:24AM +0300, Alexey Krasikov wrote:
> * Add the ability for the secret object to obtain secret data from the
>   Linux in-kernel key managment and retention facility, as an extra option
>   to the existing ones: reading from a file or passing directly as a
>   string.
> 
>   The secret is identified by the key serial number.  The upper layers
>   need to instantiate the key and make sure the QEMU process has access
>   rights to read it.
> 
> Signed-off-by: Alexey Krasikov <alex-krasikov@yandex-team.ru>
> ---
>  crypto/Makefile.objs           |   1 +
>  crypto/linux_keyring.c         | 140 +++++++++++++++++++++++++++++++++
>  include/crypto/linux_keyring.h |  38 +++++++++
>  3 files changed, 179 insertions(+)
>  create mode 100644 crypto/linux_keyring.c
>  create mode 100644 include/crypto/linux_keyring.h

Can we call these  secret_keyring.{c,h}

> diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
> index 3ae0dfd1a4..7fc354a8d5 100644
> --- a/crypto/Makefile.objs
> +++ b/crypto/Makefile.objs
> @@ -19,6 +19,7 @@ crypto-obj-y += tlscredspsk.o
>  crypto-obj-y += tlscredsx509.o
>  crypto-obj-y += tlssession.o
>  crypto-obj-y += secret_interface.o
> +crypto-obj-y += linux_keyring.o

I'd expect to see a configure check for deciding whether or not
to build the keyring code, and then have $(CONFIG_SECRET_KEYRING)
variable used here.

>  crypto-obj-y += secret.o
>  crypto-obj-y += pbkdf.o
>  crypto-obj-$(CONFIG_NETTLE) += pbkdf-nettle.o
> diff --git a/crypto/linux_keyring.c b/crypto/linux_keyring.c
> new file mode 100644
> index 0000000000..7950d4c12d
> --- /dev/null
> +++ b/crypto/linux_keyring.c
> @@ -0,0 +1,140 @@
> +#ifdef __NR_keyctl
> +
> +#include "qemu/osdep.h"
> +#include <asm/unistd.h>
> +#include <linux/keyctl.h>
> +#include "qapi/error.h"
> +#include "qom/object_interfaces.h"
> +#include "trace.h"
> +#include "crypto/linux_keyring.h"
> +
> +
> +static inline
> +long keyctl_read(key_serial_t key, uint8_t *buffer, size_t buflen)
> +{
> +    return syscall(__NR_keyctl, KEYCTL_READ, key, buffer, buflen, 0);
> +}
> +
> +
> +static
> +long keyctl_read_alloc(key_serial_t key, uint8_t **buffer)
> +{
> +    uint8_t *loc_buf;
> +    long retcode = keyctl_read(key, NULL, 0);
> +    if (retcode <= 0) {
> +        return retcode;
> +    }
> +    loc_buf = g_malloc(retcode);

We generally prefer  g_new0 to guarantee zero-initialization

> +    retcode = keyctl_read(key, loc_buf, retcode);
> +
> +    if (retcode >= 0) {
> +        *buffer = loc_buf;
> +    } else {
> +        g_free(loc_buf);
> +    }
> +    return retcode;
> +}
> +
> +
> +static void
> +qcrypto_secret_linux_load_data(Object   *obj,
> +                               uint8_t  **output,
> +                               size_t   *outputlen,
> +                               Error    **errp)
> +{
> +    QCryptoSecretLinuxKeyring *secret = QCRYPTO_SECRET_LINUX_KEYRING(obj);
> +    uint8_t  *buffer = NULL;
> +    long     retcode;
> +
> +    *output    = NULL;
> +    *outputlen = 0;
> +
> +    if (secret->serial) {
> +        retcode = keyctl_read_alloc(secret->serial, &buffer);
> +        if (retcode < 0) {
> +          error_setg_errno(errp, errno,
> +                     "Unable to read serial key %08x",
> +                     secret->serial);
> +          return;
> +        } else {
> +          *outputlen = retcode;
> +          *output    = buffer;
> +        }

IMHO, we should just be passing outputlen & output straight
into keyctl_read_alloc, except then I think keyctl_read_alloc
is pointless. So just inline its logic into this method.

> +    } else {
> +      error_setg(errp, "Either 'serial' must be provided");

Indent is a bit off, and the error message text doesn't make sense

Just use

 "'serial' parameter must be provided"

> +    }
> +}
> +
> +
> +static void
> +qcrypto_secret_prop_set_key(Object     *obj,   Visitor *v,
> +                            const char *name,  void    *opaque,
> +                            Error      **errp)
> +{
> +    QCryptoSecretLinuxKeyring *secret = QCRYPTO_SECRET_LINUX_KEYRING(obj);
> +    int32_t value;
> +    visit_type_int32(v, name, &value, errp);
> +    if (!value) {
> +        error_setg(errp, "The 'serial' should be not equal 0");
> +    }
> +    secret->serial = value;
> +}
> +
> +
> +static void
> +qcrypto_secret_prop_get_key(Object     *obj,   Visitor *v,
> +                            const char *name,  void    *opaque,
> +                            Error      **errp)
> +{
> +    QCryptoSecretLinuxKeyring *secret = QCRYPTO_SECRET_LINUX_KEYRING(obj);
> +    int32_t value = secret->serial;
> +    visit_type_int32(v, name, &value, errp);
> +}
> +
> +
> +static void
> +qcrypto_secret_linux_complete(UserCreatable *uc, Error **errp)
> +{
> +    object_property_set_bool(OBJECT(uc), true, "loaded", errp);
> +}
> +
> +
> +static void
> +qcrypto_secret_linux_class_init(ObjectClass *oc, void *data)
> +{
> +    QCryptoSecretCommonClass *sic = QCRYPTO_SECRET_COMMON_CLASS(oc);
> +    sic->load_data = qcrypto_secret_linux_load_data;
> +
> +    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> +    ucc->complete = qcrypto_secret_linux_complete;
> +
> +    object_class_property_add(oc, "serial", "key_serial_t",
> +                                  qcrypto_secret_prop_get_key,
> +                                  qcrypto_secret_prop_set_key,
> +                                  NULL, NULL, NULL);
> +}
> +
> +
> +static const TypeInfo qcrypto_secret_info = {
> +    .parent        = TYPE_QCRYPTO_SECRET_COMMON,
> +    .name          = TYPE_QCRYPTO_SECRET_LINUX_KEYRING,
> +    .instance_size = sizeof(QCryptoSecretLinuxKeyring),
> +    .class_size    = sizeof(QCryptoSecretLinuxKeyringClass),
> +    .class_init    = qcrypto_secret_linux_class_init,
> +    .interfaces    = (InterfaceInfo[]) {
> +        { TYPE_USER_CREATABLE },
> +        { }
> +    }
> +};
> +
> +
> +static void
> +qcrypto_secret_register_types(void)
> +{
> +    type_register_static(&qcrypto_secret_info);
> +}
> +
> +
> +type_init(qcrypto_secret_register_types);
> +
> +#endif /* __NR_keyctl */
> diff --git a/include/crypto/linux_keyring.h b/include/crypto/linux_keyring.h
> new file mode 100644
> index 0000000000..2618b34444
> --- /dev/null
> +++ b/include/crypto/linux_keyring.h
> @@ -0,0 +1,38 @@
> +#ifndef QCRYPTO_SECRET_LINUX_KEYRING_H
> +#define QCRYPTO_SECRET_LINUX_KEYRING_H
> +
> +#ifdef __NR_keyctl
> +
> +#include "qapi/qapi-types-crypto.h"
> +#include "qom/object.h"
> +#include "crypto/secret_interface.h"
> +
> +#define TYPE_QCRYPTO_SECRET_LINUX_KEYRING "syskey"
> +#define QCRYPTO_SECRET_LINUX_KEYRING(obj) \
> +    OBJECT_CHECK(QCryptoSecretLinuxKeyring, (obj), \
> +                 TYPE_QCRYPTO_SECRET_LINUX_KEYRING)
> +#define QCRYPTO_SECRET_LINUX_KEYRING_CLASS(class) \
> +    OBJECT_CLASS_CHECK(QCryptoSecretLinuxKeyringClass, \
> +                       (class), TYPE_QCRYPTO_SECRET_LINUX_KEYRING)
> +#define QCRYPTO_SECRET_LINUX_KEYRING_GET_CLASS(class) \
> +    OBJECT_GET_CLASS(QCryptoSecretLinuxKeyringClass, \
> +                     (class), TYPE_QCRYPTO_SECRET_LINUX_KEYRING)
> +
> +typedef struct QCryptoSecretLinux QCryptoSecretLinux;
> +typedef struct QCryptoSecretLinuxClass QCryptoSecretLinuxClass;
> +
> +typedef int32_t key_serial_t;

IMHO, just use the int32_t type inline throughout, and skip the
key_serial_t , as this typename clashes with typenames I see
in /usr/include

> +
> +typedef struct QCryptoSecretLinuxKeyring {
> +    QCryptoSecretCommon  parent;
> +    key_serial_t         serial;
> +} QCryptoSecretLinuxKeyring;
> +
> +
> +typedef struct QCryptoSecretLinuxKeyringClass {
> +    QCryptoSecretCommonClass  parent;
> +} QCryptoSecretLinuxKeyringClass;
> +
> +#endif /* __NR_keyctl */
> +
> +#endif /* QCRYPTO_SECRET_LINUX_KEYRING_H */

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|