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-krasi...@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 :|