* [PATCH v6 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites
@ 2020-05-19 18:20 Philippe Mathieu-Daudé
2020-05-19 18:20 ` [PATCH v6 1/5] hw/nvram/fw_cfg: Add the FW_CFG_DATA_GENERATOR interface Philippe Mathieu-Daudé
` (5 more replies)
0 siblings, 6 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-19 18:20 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel P. Berrangé,
Eduardo Habkost, Laszlo Ersek, Gerd Hoffmann, Paolo Bonzini,
Philippe Mathieu-Daudé
Hi,
This series has two parts:
- First we add the ability to QOM objects to produce data
consumable by the fw_cfg device,
- Then we add the tls-cipher-suites object, and let it
implement the FW_CFG_DATA_GENERATOR interface.
This is required by EDK2 'HTTPS Boot' feature [*] to tell
the guest which TLS ciphers it can use.
[*]: https://github.com/tianocore/edk2/blob/master/OvmfPkg/README
Since v5:
- Complete rewrite after chatting with Daniel Berrangé
Since v4:
- Addressed Laszlo comments (see patch#1 description)
Since v3:
- Addressed Markus' comments (do not care about heap)
Since v2:
- Split of
Since v1:
- Addressed Michael and Laszlo comments.
Please review,
Phil.
v5: https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg04525.html
v4: https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg04300.html
v3: https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg02965.html
v2: https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg02522.html
v1: https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01598.html
Supersedes: <20190620122132.10075-1-philmd@redhat.com>
Philippe Mathieu-Daudé (5):
hw/nvram/fw_cfg: Add the FW_CFG_DATA_GENERATOR interface
softmmu/vl: Let -fw_cfg option take a 'blob_id' argument
softmmu/vl: Allow -fw_cfg 'blob_id' option to set any file pathname
crypto: Add tls-cipher-suites object
crypto/tls-cipher-suites: Product fw_cfg consumable blob
include/crypto/tls-cipher-suites.h | 39 ++++++++
include/hw/nvram/fw_cfg.h | 49 ++++++++++
crypto/tls-cipher-suites.c | 152 +++++++++++++++++++++++++++++
hw/nvram/fw_cfg.c | 30 ++++++
softmmu/vl.c | 19 +++-
crypto/Makefile.objs | 1 +
6 files changed, 285 insertions(+), 5 deletions(-)
create mode 100644 include/crypto/tls-cipher-suites.h
create mode 100644 crypto/tls-cipher-suites.c
--
2.21.3
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v6 1/5] hw/nvram/fw_cfg: Add the FW_CFG_DATA_GENERATOR interface
2020-05-19 18:20 [PATCH v6 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites Philippe Mathieu-Daudé
@ 2020-05-19 18:20 ` Philippe Mathieu-Daudé
2020-05-19 22:01 ` Laszlo Ersek
2020-05-19 18:20 ` [PATCH v6 2/5] softmmu/vl: Let -fw_cfg option take a 'blob_id' argument Philippe Mathieu-Daudé
` (4 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-19 18:20 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel P. Berrangé,
Eduardo Habkost, Laszlo Ersek, Gerd Hoffmann, Paolo Bonzini,
Philippe Mathieu-Daudé
The FW_CFG_DATA_GENERATOR allow any object to product
blob of data consumable by the fw_cfg device.
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
include/hw/nvram/fw_cfg.h | 49 +++++++++++++++++++++++++++++++++++++++
hw/nvram/fw_cfg.c | 30 ++++++++++++++++++++++++
2 files changed, 79 insertions(+)
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 25d9307018..74b4790fae 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -9,11 +9,40 @@
#define TYPE_FW_CFG "fw_cfg"
#define TYPE_FW_CFG_IO "fw_cfg_io"
#define TYPE_FW_CFG_MEM "fw_cfg_mem"
+#define TYPE_FW_CFG_DATA_GENERATOR_INTERFACE "fw_cfg-data-generator"
#define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG)
#define FW_CFG_IO(obj) OBJECT_CHECK(FWCfgIoState, (obj), TYPE_FW_CFG_IO)
#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
+#define FW_CFG_DATA_GENERATOR_CLASS(class) \
+ OBJECT_CLASS_CHECK(FWCfgDataGeneratorClass, (class), \
+ TYPE_FW_CFG_DATA_GENERATOR_INTERFACE)
+#define FW_CFG_DATA_GENERATOR_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(FWCfgDataGeneratorClass, (obj), \
+ TYPE_FW_CFG_DATA_GENERATOR_INTERFACE)
+
+typedef struct FWCfgDataGeneratorClass {
+ /*< private >*/
+ InterfaceClass parent_class;
+ /*< public >*/
+
+ /**
+ * get_data:
+ * @obj: the object implementing this interface
+ *
+ * Returns: pointer to start of the generated item data
+ */
+ const void *(*get_data)(Object *obj);
+ /**
+ * get_length:
+ * @obj: the object implementing this interface
+ *
+ * Returns: the size of the generated item data in bytes
+ */
+ size_t (*get_length)(Object *obj);
+} FWCfgDataGeneratorClass;
+
typedef struct fw_cfg_file FWCfgFile;
#define FW_CFG_ORDER_OVERRIDE_VGA 70
@@ -263,6 +292,26 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
size_t len);
+/**
+ * fw_cfg_add_from_generator:
+ * @s: fw_cfg device being modified
+ * @filename: name of new fw_cfg file item
+ * @generator_id: name of object implementing FW_CFG_DATA_GENERATOR interface
+ * @errp: pointer to a NULL initialized error object
+ *
+ * Add a new NAMED fw_cfg item with the content generated from the
+ * @generator_id object. The data referenced by the starting pointer is copied
+ * into the data structure of the fw_cfg device.
+ * The next available (unused) selector key starting at FW_CFG_FILE_FIRST
+ * will be used; also, a new entry will be added to the file directory
+ * structure residing at key value FW_CFG_FILE_DIR, containing the item name,
+ * data size, and assigned selector key value.
+ *
+ * Returns: the size of the generated item data on success, -1 otherwise.
+ */
+ssize_t fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
+ const char *generator_id, Error **errp);
+
FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
AddressSpace *dma_as);
FWCfgState *fw_cfg_init_io(uint32_t iobase);
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 8dd50c2c72..e18cb074df 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -1032,6 +1032,30 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
return NULL;
}
+ssize_t fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
+ const char *generator_id, Error **errp)
+{
+ FWCfgDataGeneratorClass *k;
+ Object *o;
+ size_t sz;
+
+ o = object_resolve_path_component(object_get_objects_root(), generator_id);
+ if (!o) {
+ error_setg(errp, "Cannot find object ID %s", generator_id);
+ return -1;
+ }
+ if (!object_dynamic_cast(o, TYPE_FW_CFG_DATA_GENERATOR_INTERFACE)) {
+ error_setg(errp, "Object '%s' is not a fw_cfg-data-generator subclass",
+ generator_id);
+ return -1;
+ }
+ k = FW_CFG_DATA_GENERATOR_GET_CLASS(o);
+ sz = k->get_length(o);
+ fw_cfg_add_file(s, filename, g_memdup(k->get_data(o), sz), sz);
+
+ return sz;
+}
+
static void fw_cfg_machine_reset(void *opaque)
{
MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
@@ -1333,12 +1357,18 @@ static const TypeInfo fw_cfg_mem_info = {
.class_init = fw_cfg_mem_class_init,
};
+static const TypeInfo fw_cfg_data_generator_interface_info = {
+ .name = TYPE_FW_CFG_DATA_GENERATOR_INTERFACE,
+ .parent = TYPE_INTERFACE,
+ .class_size = sizeof(FWCfgDataGeneratorClass),
+};
static void fw_cfg_register_types(void)
{
type_register_static(&fw_cfg_info);
type_register_static(&fw_cfg_io_info);
type_register_static(&fw_cfg_mem_info);
+ type_register_static(&fw_cfg_data_generator_interface_info);
}
type_init(fw_cfg_register_types)
--
2.21.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v6 2/5] softmmu/vl: Let -fw_cfg option take a 'blob_id' argument
2020-05-19 18:20 [PATCH v6 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites Philippe Mathieu-Daudé
2020-05-19 18:20 ` [PATCH v6 1/5] hw/nvram/fw_cfg: Add the FW_CFG_DATA_GENERATOR interface Philippe Mathieu-Daudé
@ 2020-05-19 18:20 ` Philippe Mathieu-Daudé
2020-05-19 22:34 ` Laszlo Ersek
2020-05-27 11:38 ` Daniel P. Berrangé
2020-05-19 18:20 ` [RFC PATCH v6 3/5] softmmu/vl: Allow -fw_cfg 'blob_id' option to set any file pathname Philippe Mathieu-Daudé
` (3 subsequent siblings)
5 siblings, 2 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-19 18:20 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel P. Berrangé,
Eduardo Habkost, Laszlo Ersek, Gerd Hoffmann, Paolo Bonzini,
Philippe Mathieu-Daudé
The 'blob_id' argument refers to a QOM object able to produce
data consumable by the fw_cfg device. The producer object must
implement the FW_CFG_DATA_GENERATOR interface.
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
softmmu/vl.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/softmmu/vl.c b/softmmu/vl.c
index ae5451bc23..f76c53ad2e 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -489,6 +489,10 @@ static QemuOptsList qemu_fw_cfg_opts = {
.name = "string",
.type = QEMU_OPT_STRING,
.help = "Sets content of the blob to be inserted from a string",
+ }, {
+ .name = "blob_id",
+ .type = QEMU_OPT_STRING,
+ .help = "Sets id of the object generating fw_cfg blob to be used",
},
{ /* end of list */ }
},
@@ -2020,7 +2024,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
{
gchar *buf;
size_t size;
- const char *name, *file, *str;
+ const char *name, *file, *str, *blob_id;
FWCfgState *fw_cfg = (FWCfgState *) opaque;
if (fw_cfg == NULL) {
@@ -2030,14 +2034,17 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
name = qemu_opt_get(opts, "name");
file = qemu_opt_get(opts, "file");
str = qemu_opt_get(opts, "string");
+ blob_id = qemu_opt_get(opts, "blob_id");
/* we need name and either a file or the content string */
- if (!(nonempty_str(name) && (nonempty_str(file) || nonempty_str(str)))) {
+ if (!(nonempty_str(name)
+ && (nonempty_str(file) || nonempty_str(str) || nonempty_str(blob_id)))
+ ) {
error_setg(errp, "invalid argument(s)");
return -1;
}
- if (nonempty_str(file) && nonempty_str(str)) {
- error_setg(errp, "file and string are mutually exclusive");
+ if (nonempty_str(file) && nonempty_str(str) && nonempty_str(blob_id)) {
+ error_setg(errp, "file, string and blob_id are mutually exclusive");
return -1;
}
if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) {
@@ -2052,6 +2059,8 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
if (nonempty_str(str)) {
size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
buf = g_memdup(str, size);
+ } else if (nonempty_str(blob_id)) {
+ return fw_cfg_add_from_generator(fw_cfg, name, blob_id, errp);
} else {
GError *err = NULL;
if (!g_file_get_contents(file, &buf, &size, &err)) {
--
2.21.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [RFC PATCH v6 3/5] softmmu/vl: Allow -fw_cfg 'blob_id' option to set any file pathname
2020-05-19 18:20 [PATCH v6 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites Philippe Mathieu-Daudé
2020-05-19 18:20 ` [PATCH v6 1/5] hw/nvram/fw_cfg: Add the FW_CFG_DATA_GENERATOR interface Philippe Mathieu-Daudé
2020-05-19 18:20 ` [PATCH v6 2/5] softmmu/vl: Let -fw_cfg option take a 'blob_id' argument Philippe Mathieu-Daudé
@ 2020-05-19 18:20 ` Philippe Mathieu-Daudé
2020-05-19 18:22 ` Philippe Mathieu-Daudé
2020-05-19 22:45 ` Laszlo Ersek
2020-05-19 18:20 ` [PATCH v6 4/5] crypto: Add tls-cipher-suites object Philippe Mathieu-Daudé
` (2 subsequent siblings)
5 siblings, 2 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-19 18:20 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel P. Berrangé,
Eduardo Habkost, Laszlo Ersek, Gerd Hoffmann, Paolo Bonzini,
Philippe Mathieu-Daudé
This is to silent:
$ qemu-system-x86_64 \
-object tls-cipher-suites,id=ciphersuite0,priority=@SYSTEM \
-fw_cfg name=etc/edk2/https/ciphers,blob_id=ciphersuite0
qemu-system-x86_64: -fw_cfg name=etc/edk2/https/ciphers,blob_id=ciphersuite0: warning: externally provided fw_cfg item names should be prefixed with "opt/"
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
softmmu/vl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/softmmu/vl.c b/softmmu/vl.c
index f76c53ad2e..3b77dcc00d 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2052,7 +2052,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
FW_CFG_MAX_FILE_PATH - 1);
return -1;
}
- if (strncmp(name, "opt/", 4) != 0) {
+ if (!nonempty_str(blob_id) && strncmp(name, "opt/", 4) != 0) {
warn_report("externally provided fw_cfg item names "
"should be prefixed with \"opt/\"");
}
--
2.21.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v6 4/5] crypto: Add tls-cipher-suites object
2020-05-19 18:20 [PATCH v6 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites Philippe Mathieu-Daudé
` (2 preceding siblings ...)
2020-05-19 18:20 ` [RFC PATCH v6 3/5] softmmu/vl: Allow -fw_cfg 'blob_id' option to set any file pathname Philippe Mathieu-Daudé
@ 2020-05-19 18:20 ` Philippe Mathieu-Daudé
2020-05-19 23:24 ` Laszlo Ersek
2020-05-27 11:36 ` Daniel P. Berrangé
2020-05-19 18:20 ` [PATCH v6 5/5] crypto/tls-cipher-suites: Product fw_cfg consumable blob Philippe Mathieu-Daudé
2020-05-27 11:29 ` [PATCH v6 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites Philippe Mathieu-Daudé
5 siblings, 2 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-19 18:20 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel P. Berrangé,
Eduardo Habkost, Laszlo Ersek, Gerd Hoffmann, Paolo Bonzini,
Philippe Mathieu-Daudé
Example of use to dump:
$ qemu-system-x86_64 -S \
-object tls-cipher-suites,id=mysuite,priority=@SYSTEM,verbose=yes
Cipher suites for @SYSTEM:
- TLS_AES_256_GCM_SHA384 0x13, 0x02 TLS1.3
- TLS_CHACHA20_POLY1305_SHA256 0x13, 0x03 TLS1.3
- TLS_AES_128_GCM_SHA256 0x13, 0x01 TLS1.3
- TLS_AES_128_CCM_SHA256 0x13, 0x04 TLS1.3
- TLS_ECDHE_RSA_AES_256_GCM_SHA384 0xc0, 0x30 TLS1.2
- TLS_ECDHE_RSA_CHACHA20_POLY1305 0xcc, 0xa8 TLS1.2
- TLS_ECDHE_RSA_AES_256_CBC_SHA1 0xc0, 0x14 TLS1.0
- TLS_ECDHE_RSA_AES_128_GCM_SHA256 0xc0, 0x2f TLS1.2
- TLS_ECDHE_RSA_AES_128_CBC_SHA1 0xc0, 0x13 TLS1.0
- TLS_ECDHE_ECDSA_AES_256_GCM_SHA384 0xc0, 0x2c TLS1.2
- TLS_ECDHE_ECDSA_CHACHA20_POLY1305 0xcc, 0xa9 TLS1.2
- TLS_ECDHE_ECDSA_AES_256_CCM 0xc0, 0xad TLS1.2
- TLS_ECDHE_ECDSA_AES_256_CBC_SHA1 0xc0, 0x0a TLS1.0
- TLS_ECDHE_ECDSA_AES_128_GCM_SHA256 0xc0, 0x2b TLS1.2
- TLS_ECDHE_ECDSA_AES_128_CCM 0xc0, 0xac TLS1.2
- TLS_ECDHE_ECDSA_AES_128_CBC_SHA1 0xc0, 0x09 TLS1.0
- TLS_RSA_AES_256_GCM_SHA384 0x00, 0x9d TLS1.2
- TLS_RSA_AES_256_CCM 0xc0, 0x9d TLS1.2
- TLS_RSA_AES_256_CBC_SHA1 0x00, 0x35 TLS1.0
- TLS_RSA_AES_128_GCM_SHA256 0x00, 0x9c TLS1.2
- TLS_RSA_AES_128_CCM 0xc0, 0x9c TLS1.2
- TLS_RSA_AES_128_CBC_SHA1 0x00, 0x2f TLS1.0
- TLS_DHE_RSA_AES_256_GCM_SHA384 0x00, 0x9f TLS1.2
- TLS_DHE_RSA_CHACHA20_POLY1305 0xcc, 0xaa TLS1.2
- TLS_DHE_RSA_AES_256_CCM 0xc0, 0x9f TLS1.2
- TLS_DHE_RSA_AES_256_CBC_SHA1 0x00, 0x39 TLS1.0
- TLS_DHE_RSA_AES_128_GCM_SHA256 0x00, 0x9e TLS1.2
- TLS_DHE_RSA_AES_128_CCM 0xc0, 0x9e TLS1.2
- TLS_DHE_RSA_AES_128_CBC_SHA1 0x00, 0x33 TLS1.0
total: 29 ciphers
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
include/crypto/tls-cipher-suites.h | 39 +++++++++
crypto/tls-cipher-suites.c | 133 +++++++++++++++++++++++++++++
crypto/Makefile.objs | 1 +
3 files changed, 173 insertions(+)
create mode 100644 include/crypto/tls-cipher-suites.h
create mode 100644 crypto/tls-cipher-suites.c
diff --git a/include/crypto/tls-cipher-suites.h b/include/crypto/tls-cipher-suites.h
new file mode 100644
index 0000000000..31e92916e1
--- /dev/null
+++ b/include/crypto/tls-cipher-suites.h
@@ -0,0 +1,39 @@
+/*
+ * QEMU TLS Cipher Suites
+ *
+ * Copyright (c) 2019 Red Hat, Inc.
+ *
+ * Author: Philippe Mathieu-Daudé <philmd@redhat.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef QCRYPTO_TLSCIPHERSUITES_H
+#define QCRYPTO_TLSCIPHERSUITES_H
+
+#include "qom/object.h"
+#include "crypto/tlscreds.h"
+
+#define TYPE_QCRYPTO_TLS_CIPHER_SUITES "tls-cipher-suites"
+#define QCRYPTO_TLS_CIPHER_SUITES(obj) \
+ OBJECT_CHECK(QCryptoTLSCipherSuites, (obj), TYPE_QCRYPTO_TLS_CIPHER_SUITES)
+
+/*
+ * IANA registered TLS ciphers:
+ * https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-4
+ */
+typedef struct {
+ uint8_t data[2];
+} IANA_TLS_CIPHER;
+
+typedef struct QCryptoTLSCipherSuites {
+ /* <private> */
+ QCryptoTLSCreds parent_obj;
+
+ /* <public> */
+ bool verbose;
+ IANA_TLS_CIPHER *cipher_list;
+ unsigned cipher_count;
+} QCryptoTLSCipherSuites;
+
+#endif /* QCRYPTO_TLSCIPHERSUITES_H */
diff --git a/crypto/tls-cipher-suites.c b/crypto/tls-cipher-suites.c
new file mode 100644
index 0000000000..c6c51359bd
--- /dev/null
+++ b/crypto/tls-cipher-suites.c
@@ -0,0 +1,133 @@
+/*
+ * QEMU TLS Cipher Suites
+ *
+ * Copyright (c) 2019 Red Hat, Inc.
+ *
+ * Author: Philippe Mathieu-Daudé <philmd@redhat.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qom/object_interfaces.h"
+#include "qemu/error-report.h"
+#include "crypto/tlscreds.h"
+#include "crypto/tls-cipher-suites.h"
+
+static void parse_cipher_suites(QCryptoTLSCipherSuites *s,
+ const char *priority_name, Error **errp)
+{
+#ifdef CONFIG_GNUTLS
+ int ret;
+ unsigned int idx;
+ const char *name;
+ const char *err;
+ gnutls_protocol_t version;
+ gnutls_priority_t pcache;
+
+ assert(priority_name);
+ ret = gnutls_priority_init(&pcache, priority_name, &err);
+ if (ret < 0) {
+ error_setg(errp, "Syntax error using priority '%s': %s",
+ priority_name, gnutls_strerror(ret));
+ return;
+ }
+
+ if (s->verbose) {
+ fprintf(stderr, "Cipher suites for %s:\n", priority_name);
+ }
+ for (size_t i = 0;; i++) {
+ ret = gnutls_priority_get_cipher_suite_index(pcache, i, &idx);
+ if (ret == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) {
+ break;
+ }
+ if (ret == GNUTLS_E_UNKNOWN_CIPHER_SUITE) {
+ continue;
+ }
+ s->cipher_list = g_renew(IANA_TLS_CIPHER,
+ s->cipher_list, s->cipher_count + 1);
+
+ name = gnutls_cipher_suite_info(idx,
+ s->cipher_list[s->cipher_count].data,
+ NULL, NULL, NULL, &version);
+ if (name != NULL) {
+ if (s->verbose) {
+ fprintf(stderr, "- %-50s\t0x%02x, 0x%02x\t%s\n", name,
+ s->cipher_list[s->cipher_count].data[0],
+ s->cipher_list[s->cipher_count].data[1],
+ gnutls_protocol_get_name(version));
+ }
+ s->cipher_count++;
+ }
+ }
+ if (s->verbose) {
+ fprintf(stderr, "total: %u ciphers\n", s->cipher_count);
+ }
+ gnutls_priority_deinit(pcache);
+#else
+ error_setg(errp, "GNU TLS not available");
+#endif /* CONFIG_GNUTLS */
+}
+
+static void qcrypto_tls_cipher_suites_complete(UserCreatable *uc, Error **errp)
+{
+ QCryptoTLSCreds *s = QCRYPTO_TLS_CREDS(uc);
+
+ if (!s->priority) {
+ error_setg(errp, "'priority' property is not set");
+ return;
+ }
+ parse_cipher_suites(QCRYPTO_TLS_CIPHER_SUITES(s), s->priority, errp);
+}
+
+static void qcrypto_tls_cipher_suites_set_verbose(Object *obj, bool value,
+ Error **errp G_GNUC_UNUSED)
+{
+ QCRYPTO_TLS_CIPHER_SUITES(obj)->verbose = value;
+}
+
+
+static bool qcrypto_tls_cipher_suites_get_verbose(Object *obj,
+ Error **errp G_GNUC_UNUSED)
+{
+ return QCRYPTO_TLS_CIPHER_SUITES(obj)->verbose;
+}
+
+static void qcrypto_tls_cipher_suites_finalize(Object *obj)
+{
+ QCryptoTLSCipherSuites *s = QCRYPTO_TLS_CIPHER_SUITES(obj);
+
+ g_free(s->cipher_list);
+}
+
+static void qcrypto_tls_cipher_suites_class_init(ObjectClass *oc, void *data)
+{
+ UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
+
+ ucc->complete = qcrypto_tls_cipher_suites_complete;
+
+ object_class_property_add_bool(oc, "verbose",
+ qcrypto_tls_cipher_suites_get_verbose,
+ qcrypto_tls_cipher_suites_set_verbose);
+}
+
+static const TypeInfo qcrypto_tls_cipher_suites_info = {
+ .parent = TYPE_QCRYPTO_TLS_CREDS,
+ .name = TYPE_QCRYPTO_TLS_CIPHER_SUITES,
+ .instance_size = sizeof(QCryptoTLSCipherSuites),
+ .instance_finalize = qcrypto_tls_cipher_suites_finalize,
+ .class_size = sizeof(QCryptoTLSCredsClass),
+ .class_init = qcrypto_tls_cipher_suites_class_init,
+ .interfaces = (InterfaceInfo[]) {
+ { TYPE_USER_CREATABLE },
+ { }
+ }
+};
+
+static void qcrypto_tls_cipher_suites_register_types(void)
+{
+ type_register_static(&qcrypto_tls_cipher_suites_info);
+}
+
+type_init(qcrypto_tls_cipher_suites_register_types);
diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
index c2a371b0b4..ce706d322a 100644
--- a/crypto/Makefile.objs
+++ b/crypto/Makefile.objs
@@ -13,6 +13,7 @@ crypto-obj-y += cipher.o
crypto-obj-$(CONFIG_AF_ALG) += afalg.o
crypto-obj-$(CONFIG_AF_ALG) += cipher-afalg.o
crypto-obj-$(CONFIG_AF_ALG) += hash-afalg.o
+crypto-obj-y += tls-cipher-suites.o
crypto-obj-y += tlscreds.o
crypto-obj-y += tlscredsanon.o
crypto-obj-y += tlscredspsk.o
--
2.21.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v6 5/5] crypto/tls-cipher-suites: Product fw_cfg consumable blob
2020-05-19 18:20 [PATCH v6 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites Philippe Mathieu-Daudé
` (3 preceding siblings ...)
2020-05-19 18:20 ` [PATCH v6 4/5] crypto: Add tls-cipher-suites object Philippe Mathieu-Daudé
@ 2020-05-19 18:20 ` Philippe Mathieu-Daudé
2020-05-19 22:49 ` Laszlo Ersek
2020-05-27 11:29 ` [PATCH v6 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites Philippe Mathieu-Daudé
5 siblings, 1 reply; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-19 18:20 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel P. Berrangé,
Eduardo Habkost, Laszlo Ersek, Gerd Hoffmann, Paolo Bonzini,
Philippe Mathieu-Daudé
Since our format is consumable by the fw_cfg device,
we can implement the FW_CFG_DATA_GENERATOR interface.
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
crypto/tls-cipher-suites.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/crypto/tls-cipher-suites.c b/crypto/tls-cipher-suites.c
index c6c51359bd..11872783eb 100644
--- a/crypto/tls-cipher-suites.c
+++ b/crypto/tls-cipher-suites.c
@@ -14,6 +14,7 @@
#include "qemu/error-report.h"
#include "crypto/tlscreds.h"
#include "crypto/tls-cipher-suites.h"
+#include "hw/nvram/fw_cfg.h"
static void parse_cipher_suites(QCryptoTLSCipherSuites *s,
const char *priority_name, Error **errp)
@@ -101,11 +102,28 @@ static void qcrypto_tls_cipher_suites_finalize(Object *obj)
g_free(s->cipher_list);
}
+static const void *qcrypto_tls_cipher_suites_get_data(Object *obj)
+{
+ QCryptoTLSCipherSuites *s = QCRYPTO_TLS_CIPHER_SUITES(obj);
+
+ return s->cipher_list;
+}
+
+static size_t qcrypto_tls_cipher_suites_get_length(Object *obj)
+{
+ QCryptoTLSCipherSuites *s = QCRYPTO_TLS_CIPHER_SUITES(obj);
+
+ return s->cipher_count * sizeof(IANA_TLS_CIPHER);
+}
+
static void qcrypto_tls_cipher_suites_class_init(ObjectClass *oc, void *data)
{
UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
+ FWCfgDataGeneratorClass *fwgc = FW_CFG_DATA_GENERATOR_CLASS(oc);
ucc->complete = qcrypto_tls_cipher_suites_complete;
+ fwgc->get_data = qcrypto_tls_cipher_suites_get_data;
+ fwgc->get_length = qcrypto_tls_cipher_suites_get_length;
object_class_property_add_bool(oc, "verbose",
qcrypto_tls_cipher_suites_get_verbose,
@@ -121,6 +139,7 @@ static const TypeInfo qcrypto_tls_cipher_suites_info = {
.class_init = qcrypto_tls_cipher_suites_class_init,
.interfaces = (InterfaceInfo[]) {
{ TYPE_USER_CREATABLE },
+ { TYPE_FW_CFG_DATA_GENERATOR_INTERFACE },
{ }
}
};
--
2.21.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [RFC PATCH v6 3/5] softmmu/vl: Allow -fw_cfg 'blob_id' option to set any file pathname
2020-05-19 18:20 ` [RFC PATCH v6 3/5] softmmu/vl: Allow -fw_cfg 'blob_id' option to set any file pathname Philippe Mathieu-Daudé
@ 2020-05-19 18:22 ` Philippe Mathieu-Daudé
2020-05-19 22:45 ` Laszlo Ersek
1 sibling, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-19 18:22 UTC (permalink / raw)
To: qemu-devel
Cc: Laszlo Ersek, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost, Gerd Hoffmann
On 5/19/20 8:20 PM, Philippe Mathieu-Daudé wrote:
> This is to silent:
>
> $ qemu-system-x86_64 \
> -object tls-cipher-suites,id=ciphersuite0,priority=@SYSTEM \
> -fw_cfg name=etc/edk2/https/ciphers,blob_id=ciphersuite0
> qemu-system-x86_64: -fw_cfg name=etc/edk2/https/ciphers,blob_id=ciphersuite0: warning: externally provided fw_cfg item names should be prefixed with "opt/"
Oops, this was supposed to be the last patch of the series. RFC anyway.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> softmmu/vl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index f76c53ad2e..3b77dcc00d 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2052,7 +2052,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
> FW_CFG_MAX_FILE_PATH - 1);
> return -1;
> }
> - if (strncmp(name, "opt/", 4) != 0) {
> + if (!nonempty_str(blob_id) && strncmp(name, "opt/", 4) != 0) {
> warn_report("externally provided fw_cfg item names "
> "should be prefixed with \"opt/\"");
> }
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 1/5] hw/nvram/fw_cfg: Add the FW_CFG_DATA_GENERATOR interface
2020-05-19 18:20 ` [PATCH v6 1/5] hw/nvram/fw_cfg: Add the FW_CFG_DATA_GENERATOR interface Philippe Mathieu-Daudé
@ 2020-05-19 22:01 ` Laszlo Ersek
2020-05-28 14:54 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2020-05-19 22:01 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost, Gerd Hoffmann
On 05/19/20 20:20, Philippe Mathieu-Daudé wrote:
> The FW_CFG_DATA_GENERATOR allow any object to product
(1) I suggest:
s/allow/allows/
s/product/produce/
> blob of data consumable by the fw_cfg device.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> include/hw/nvram/fw_cfg.h | 49 +++++++++++++++++++++++++++++++++++++++
> hw/nvram/fw_cfg.c | 30 ++++++++++++++++++++++++
> 2 files changed, 79 insertions(+)
>
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 25d9307018..74b4790fae 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -9,11 +9,40 @@
> #define TYPE_FW_CFG "fw_cfg"
> #define TYPE_FW_CFG_IO "fw_cfg_io"
> #define TYPE_FW_CFG_MEM "fw_cfg_mem"
> +#define TYPE_FW_CFG_DATA_GENERATOR_INTERFACE "fw_cfg-data-generator"
>
> #define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG)
> #define FW_CFG_IO(obj) OBJECT_CHECK(FWCfgIoState, (obj), TYPE_FW_CFG_IO)
> #define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
>
> +#define FW_CFG_DATA_GENERATOR_CLASS(class) \
> + OBJECT_CLASS_CHECK(FWCfgDataGeneratorClass, (class), \
> + TYPE_FW_CFG_DATA_GENERATOR_INTERFACE)
> +#define FW_CFG_DATA_GENERATOR_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(FWCfgDataGeneratorClass, (obj), \
> + TYPE_FW_CFG_DATA_GENERATOR_INTERFACE)
> +
> +typedef struct FWCfgDataGeneratorClass {
> + /*< private >*/
> + InterfaceClass parent_class;
> + /*< public >*/
> +
> + /**
> + * get_data:
> + * @obj: the object implementing this interface
> + *
> + * Returns: pointer to start of the generated item data
> + */
> + const void *(*get_data)(Object *obj);
I'm not familiar with QOM, so please excuse any dumb questions.
"const" suggests the blob returned remains owned by "obj"; that answers
the question whether the caller should attempt to free the blob. (The
answer is "no".)
(2) However, will this perhaps expose other functions, currently taking
non-const-qualified pointers, to which we'd like to pass the blob
returned by the above member function?
Because, then we'd have to cast away "const", and I find that much
uglier than removing the "const" from *here*, and adding a more verbose
comment as replacement.
Yes, this is clearly speculation -- IOW just a question. If all the
functions we're going to pass the return value to are fine with
pointer-to-const, then this interface should be OK.
(Obviously when I say "cast away const", I think of functions that do
not actually modify the object pointed-to by the non-const-qualified
pointer.)
> + /**
> + * get_length:
> + * @obj: the object implementing this interface
> + *
> + * Returns: the size of the generated item data in bytes
> + */
> + size_t (*get_length)(Object *obj);
> +} FWCfgDataGeneratorClass;
> +
> typedef struct fw_cfg_file FWCfgFile;
>
> #define FW_CFG_ORDER_OVERRIDE_VGA 70
> @@ -263,6 +292,26 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
> void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
> size_t len);
>
> +/**
> + * fw_cfg_add_from_generator:
> + * @s: fw_cfg device being modified
> + * @filename: name of new fw_cfg file item
> + * @generator_id: name of object implementing FW_CFG_DATA_GENERATOR interface
> + * @errp: pointer to a NULL initialized error object
> + *
> + * Add a new NAMED fw_cfg item with the content generated from the
> + * @generator_id object. The data referenced by the starting pointer is copied
(3) s/referenced by the starting pointer/generated by the @generator_id
object/
> + * into the data structure of the fw_cfg device.
> + * The next available (unused) selector key starting at FW_CFG_FILE_FIRST
> + * will be used; also, a new entry will be added to the file directory
> + * structure residing at key value FW_CFG_FILE_DIR, containing the item name,
> + * data size, and assigned selector key value.
> + *
> + * Returns: the size of the generated item data on success, -1 otherwise.
(4) I don't like ssize_t for a return value like this.
First, get_length() returns size_t, which may not be representable in an
ssize_t.
(Actually, it's worse than that; POSIX says, "the type ssize_t shall be
capable of storing values at least in the range [-1, {SSIZE_MAX}]" --
and if I run "getconf SSIZE_MAX", I get 32767. Indeed, _POSIX_SSIZE_MAX,
which is the minimum for any implementation's SSIZE_MAX, is 32767.)
Second, is a zero-sized blob useful in fw_cfg (from a generator)?
If it is not useful, then this function should return size_t, and use
retval=0 for signaling an error.
If a zero-sized blob is useful, then the function should return a bool
(in addition to producing "errp"), and output the blob size as a
separate parameter.
> + */
> +ssize_t fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
> + const char *generator_id, Error **errp);
> +
> FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
> AddressSpace *dma_as);
> FWCfgState *fw_cfg_init_io(uint32_t iobase);
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 8dd50c2c72..e18cb074df 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -1032,6 +1032,30 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
> return NULL;
> }
>
> +ssize_t fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
> + const char *generator_id, Error **errp)
> +{
> + FWCfgDataGeneratorClass *k;
> + Object *o;
(5) Not sure about QEMU coding standards, but the above single-char
variable names (especially "o") terrify me. Please use "klass" and "obj".
Do ignore my request if these variable names are just fine in QEMU.
> + size_t sz;
> +
> + o = object_resolve_path_component(object_get_objects_root(), generator_id);
> + if (!o) {
> + error_setg(errp, "Cannot find object ID %s", generator_id);
> + return -1;
> + }
> + if (!object_dynamic_cast(o, TYPE_FW_CFG_DATA_GENERATOR_INTERFACE)) {
> + error_setg(errp, "Object '%s' is not a fw_cfg-data-generator subclass",
> + generator_id);
(6) We should probably not open code
TYPE_FW_CFG_DATA_GENERATOR_INTERFACE as "fw_cfg-data-generator" even in
the error message.
(7) If this branch is taken, would that arguably merit an assertion
failure? I mean, can the dynamic cast fail without QEMU having a related
bug somewhere? (Maybe this is going to be answered in the rest of the
series.) Because I see those OBJECT_CHECK macros near the top of
"fw_cfg.h", and those boil down to object_dynamic_cast_assert().
> + return -1;
> + }
> + k = FW_CFG_DATA_GENERATOR_GET_CLASS(o);
> + sz = k->get_length(o);
> + fw_cfg_add_file(s, filename, g_memdup(k->get_data(o), sz), sz);
(g_memdup() takes a "guint" for "byte_size". Whether that matches
"size_t" is anyone's guess. I guess it can't be helped.)
> +
> + return sz;
Right, this is the size_t --> ssize_t conversion that makes me
uncomfortable.
I'm OK if you ignore all of my comments, these are simply the thoughts
that crossed my mind.
Thanks
Laszlo
> +}
> +
> static void fw_cfg_machine_reset(void *opaque)
> {
> MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> @@ -1333,12 +1357,18 @@ static const TypeInfo fw_cfg_mem_info = {
> .class_init = fw_cfg_mem_class_init,
> };
>
> +static const TypeInfo fw_cfg_data_generator_interface_info = {
> + .name = TYPE_FW_CFG_DATA_GENERATOR_INTERFACE,
> + .parent = TYPE_INTERFACE,
> + .class_size = sizeof(FWCfgDataGeneratorClass),
> +};
>
> static void fw_cfg_register_types(void)
> {
> type_register_static(&fw_cfg_info);
> type_register_static(&fw_cfg_io_info);
> type_register_static(&fw_cfg_mem_info);
> + type_register_static(&fw_cfg_data_generator_interface_info);
> }
>
> type_init(fw_cfg_register_types)
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 2/5] softmmu/vl: Let -fw_cfg option take a 'blob_id' argument
2020-05-19 18:20 ` [PATCH v6 2/5] softmmu/vl: Let -fw_cfg option take a 'blob_id' argument Philippe Mathieu-Daudé
@ 2020-05-19 22:34 ` Laszlo Ersek
2020-05-28 12:07 ` Philippe Mathieu-Daudé
2020-05-27 11:38 ` Daniel P. Berrangé
1 sibling, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2020-05-19 22:34 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost, Gerd Hoffmann
On 05/19/20 20:20, Philippe Mathieu-Daudé wrote:
> The 'blob_id' argument refers to a QOM object able to produce
> data consumable by the fw_cfg device. The producer object must
> implement the FW_CFG_DATA_GENERATOR interface.
OK, this answers my OBJECT_CHECK() question under patch #1 (in the
negative -- an assert would be wrong).
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> softmmu/vl.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index ae5451bc23..f76c53ad2e 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -489,6 +489,10 @@ static QemuOptsList qemu_fw_cfg_opts = {
> .name = "string",
> .type = QEMU_OPT_STRING,
> .help = "Sets content of the blob to be inserted from a string",
> + }, {
> + .name = "blob_id",
> + .type = QEMU_OPT_STRING,
> + .help = "Sets id of the object generating fw_cfg blob to be used",
> },
> { /* end of list */ }
> },
> @@ -2020,7 +2024,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
> {
> gchar *buf;
> size_t size;
> - const char *name, *file, *str;
> + const char *name, *file, *str, *blob_id;
> FWCfgState *fw_cfg = (FWCfgState *) opaque;
>
> if (fw_cfg == NULL) {
> @@ -2030,14 +2034,17 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
> name = qemu_opt_get(opts, "name");
> file = qemu_opt_get(opts, "file");
> str = qemu_opt_get(opts, "string");
> + blob_id = qemu_opt_get(opts, "blob_id");
>
> /* we need name and either a file or the content string */
(1) Please update this comment. If the option is given, we need the
name, and exactly one of: file, content string, blob_id.
> - if (!(nonempty_str(name) && (nonempty_str(file) || nonempty_str(str)))) {
> + if (!(nonempty_str(name)
> + && (nonempty_str(file) || nonempty_str(str) || nonempty_str(blob_id)))
> + ) {
> error_setg(errp, "invalid argument(s)");
> return -1;
> }
(2) Coding style: does QEMU keep operators on the left or on the right
when breaking subconditions to new lines? (I vaguely recall "to the
right", but I could be wrong... Well, "hw/nvram/fw_cfg.c" has at least 7
examples of the operator being on the right.)
> - if (nonempty_str(file) && nonempty_str(str)) {
> - error_setg(errp, "file and string are mutually exclusive");
> + if (nonempty_str(file) && nonempty_str(str) && nonempty_str(blob_id)) {
> + error_setg(errp, "file, string and blob_id are mutually exclusive");
> return -1;
> }
(3) I believe this catches only when all three of name/string/blob_id
are given. But we should continue catching "two given".
How about reworking both "if"s, *and* the comment at (1) at the same
time, into:
if (!nonempty_str(name) ||
nonempty_str(file) + nonempty_str(str) + nonempty_str(blob_id) != 1) {
error_setg(errp, "name, plus exactly one of file, string and blob_id, "
"are needed");
return -1;
}
(Regarding the addition, nonempty_str() returns a "bool", which is a
macro to _Bool, which is promoted to "int" or "unsigned int".)
> if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) {
> @@ -2052,6 +2059,8 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
> if (nonempty_str(str)) {
> size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
> buf = g_memdup(str, size);
> + } else if (nonempty_str(blob_id)) {
> + return fw_cfg_add_from_generator(fw_cfg, name, blob_id, errp);
> } else {
> GError *err = NULL;
> if (!g_file_get_contents(file, &buf, &size, &err)) {
>
(4) The "-fw_cfg" command line option is documented in both the qemu(1)
manual, and the "docs/specs/fw_cfg.txt" file.
I think we may have to update those. In particular I mean *where* the
option is documented (in both texts).
In the manual, "-fw_cfg" is currently under "Debug/Expert options", but
that will no longer apply (I think?) after this series.
Similarly, in "docs/specs/fw_cfg.txt", the section is called "Externally
Provided Items" -- but that might not be strictly true any more either.
Maybe leave the current "-fw_cfg" mentions in peace, and document
"-fw_cfg blob_id=..." separately (in different docs sections)? The
"fw_cfg generators" concept could deserve dedicated sections.
Sorry that I can't make a good concrete suggestion. :(
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH v6 3/5] softmmu/vl: Allow -fw_cfg 'blob_id' option to set any file pathname
2020-05-19 18:20 ` [RFC PATCH v6 3/5] softmmu/vl: Allow -fw_cfg 'blob_id' option to set any file pathname Philippe Mathieu-Daudé
2020-05-19 18:22 ` Philippe Mathieu-Daudé
@ 2020-05-19 22:45 ` Laszlo Ersek
2020-05-28 17:03 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2020-05-19 22:45 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost, Gerd Hoffmann
On 05/19/20 20:20, Philippe Mathieu-Daudé wrote:
> This is to silent:
>
> $ qemu-system-x86_64 \
> -object tls-cipher-suites,id=ciphersuite0,priority=@SYSTEM \
> -fw_cfg name=etc/edk2/https/ciphers,blob_id=ciphersuite0
> qemu-system-x86_64: -fw_cfg name=etc/edk2/https/ciphers,blob_id=ciphersuite0: warning: externally provided fw_cfg item names should be prefixed with "opt/"
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> softmmu/vl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index f76c53ad2e..3b77dcc00d 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2052,7 +2052,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
> FW_CFG_MAX_FILE_PATH - 1);
> return -1;
> }
> - if (strncmp(name, "opt/", 4) != 0) {
> + if (!nonempty_str(blob_id) && strncmp(name, "opt/", 4) != 0) {
> warn_report("externally provided fw_cfg item names "
> "should be prefixed with \"opt/\"");
> }
>
Hmmm, difficult question! Is "ciphersuite0" now externally provided or not?
Because, ciphersuite0 is populated internally to QEMU alright (and so we
can think it's internal), but its *association with the name* is external.
What if we keep the same "-object" switch, but use a different (bogus)
"name" with "-fw_cfg"? IMO that kind of invalidates "-object" too.
Should the fw_cfg generator interface dictate the fw_cfg filename too?
Because that would eliminate this problem. Put differently, we now have
a possibility to label the "ciphersuite0" object in the fw_cfg file
directory any way we want -- but is that freedom *useful* for anything?
I guess we might want multiple "tls-cipher-suites" objects one day, so
hard-coding the fw_cfg names on that level could cause conflicts. On the
other hand, I wouldn't like "blob_id" to generally circumvent the "etc/"
namespace protection.
I'm leaning towards agreeing with this patch, but I'd appreciate some
convincing arguments.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 5/5] crypto/tls-cipher-suites: Product fw_cfg consumable blob
2020-05-19 18:20 ` [PATCH v6 5/5] crypto/tls-cipher-suites: Product fw_cfg consumable blob Philippe Mathieu-Daudé
@ 2020-05-19 22:49 ` Laszlo Ersek
0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2020-05-19 22:49 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost, Gerd Hoffmann
On 05/19/20 20:20, Philippe Mathieu-Daudé wrote:
> Since our format is consumable by the fw_cfg device,
> we can implement the FW_CFG_DATA_GENERATOR interface.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> crypto/tls-cipher-suites.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
(1) s/product/produce/ in the subject line.
This patch looks OK to me otherwise (but I don't feel confident enough
to give an R-b):
Acked-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
>
> diff --git a/crypto/tls-cipher-suites.c b/crypto/tls-cipher-suites.c
> index c6c51359bd..11872783eb 100644
> --- a/crypto/tls-cipher-suites.c
> +++ b/crypto/tls-cipher-suites.c
> @@ -14,6 +14,7 @@
> #include "qemu/error-report.h"
> #include "crypto/tlscreds.h"
> #include "crypto/tls-cipher-suites.h"
> +#include "hw/nvram/fw_cfg.h"
>
> static void parse_cipher_suites(QCryptoTLSCipherSuites *s,
> const char *priority_name, Error **errp)
> @@ -101,11 +102,28 @@ static void qcrypto_tls_cipher_suites_finalize(Object *obj)
> g_free(s->cipher_list);
> }
>
> +static const void *qcrypto_tls_cipher_suites_get_data(Object *obj)
> +{
> + QCryptoTLSCipherSuites *s = QCRYPTO_TLS_CIPHER_SUITES(obj);
> +
> + return s->cipher_list;
> +}
> +
> +static size_t qcrypto_tls_cipher_suites_get_length(Object *obj)
> +{
> + QCryptoTLSCipherSuites *s = QCRYPTO_TLS_CIPHER_SUITES(obj);
> +
> + return s->cipher_count * sizeof(IANA_TLS_CIPHER);
> +}
> +
> static void qcrypto_tls_cipher_suites_class_init(ObjectClass *oc, void *data)
> {
> UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> + FWCfgDataGeneratorClass *fwgc = FW_CFG_DATA_GENERATOR_CLASS(oc);
>
> ucc->complete = qcrypto_tls_cipher_suites_complete;
> + fwgc->get_data = qcrypto_tls_cipher_suites_get_data;
> + fwgc->get_length = qcrypto_tls_cipher_suites_get_length;
>
> object_class_property_add_bool(oc, "verbose",
> qcrypto_tls_cipher_suites_get_verbose,
> @@ -121,6 +139,7 @@ static const TypeInfo qcrypto_tls_cipher_suites_info = {
> .class_init = qcrypto_tls_cipher_suites_class_init,
> .interfaces = (InterfaceInfo[]) {
> { TYPE_USER_CREATABLE },
> + { TYPE_FW_CFG_DATA_GENERATOR_INTERFACE },
> { }
> }
> };
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 4/5] crypto: Add tls-cipher-suites object
2020-05-19 18:20 ` [PATCH v6 4/5] crypto: Add tls-cipher-suites object Philippe Mathieu-Daudé
@ 2020-05-19 23:24 ` Laszlo Ersek
2020-05-27 11:36 ` Daniel P. Berrangé
1 sibling, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2020-05-19 23:24 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost, Gerd Hoffmann
On 05/19/20 20:20, Philippe Mathieu-Daudé wrote:
> Example of use to dump:
>
> $ qemu-system-x86_64 -S \
> -object tls-cipher-suites,id=mysuite,priority=@SYSTEM,verbose=yes
> Cipher suites for @SYSTEM:
> - TLS_AES_256_GCM_SHA384 0x13, 0x02 TLS1.3
> - TLS_CHACHA20_POLY1305_SHA256 0x13, 0x03 TLS1.3
> - TLS_AES_128_GCM_SHA256 0x13, 0x01 TLS1.3
> - TLS_AES_128_CCM_SHA256 0x13, 0x04 TLS1.3
> - TLS_ECDHE_RSA_AES_256_GCM_SHA384 0xc0, 0x30 TLS1.2
> - TLS_ECDHE_RSA_CHACHA20_POLY1305 0xcc, 0xa8 TLS1.2
> - TLS_ECDHE_RSA_AES_256_CBC_SHA1 0xc0, 0x14 TLS1.0
> - TLS_ECDHE_RSA_AES_128_GCM_SHA256 0xc0, 0x2f TLS1.2
> - TLS_ECDHE_RSA_AES_128_CBC_SHA1 0xc0, 0x13 TLS1.0
> - TLS_ECDHE_ECDSA_AES_256_GCM_SHA384 0xc0, 0x2c TLS1.2
> - TLS_ECDHE_ECDSA_CHACHA20_POLY1305 0xcc, 0xa9 TLS1.2
> - TLS_ECDHE_ECDSA_AES_256_CCM 0xc0, 0xad TLS1.2
> - TLS_ECDHE_ECDSA_AES_256_CBC_SHA1 0xc0, 0x0a TLS1.0
> - TLS_ECDHE_ECDSA_AES_128_GCM_SHA256 0xc0, 0x2b TLS1.2
> - TLS_ECDHE_ECDSA_AES_128_CCM 0xc0, 0xac TLS1.2
> - TLS_ECDHE_ECDSA_AES_128_CBC_SHA1 0xc0, 0x09 TLS1.0
> - TLS_RSA_AES_256_GCM_SHA384 0x00, 0x9d TLS1.2
> - TLS_RSA_AES_256_CCM 0xc0, 0x9d TLS1.2
> - TLS_RSA_AES_256_CBC_SHA1 0x00, 0x35 TLS1.0
> - TLS_RSA_AES_128_GCM_SHA256 0x00, 0x9c TLS1.2
> - TLS_RSA_AES_128_CCM 0xc0, 0x9c TLS1.2
> - TLS_RSA_AES_128_CBC_SHA1 0x00, 0x2f TLS1.0
> - TLS_DHE_RSA_AES_256_GCM_SHA384 0x00, 0x9f TLS1.2
> - TLS_DHE_RSA_CHACHA20_POLY1305 0xcc, 0xaa TLS1.2
> - TLS_DHE_RSA_AES_256_CCM 0xc0, 0x9f TLS1.2
> - TLS_DHE_RSA_AES_256_CBC_SHA1 0x00, 0x39 TLS1.0
> - TLS_DHE_RSA_AES_128_GCM_SHA256 0x00, 0x9e TLS1.2
> - TLS_DHE_RSA_AES_128_CCM 0xc0, 0x9e TLS1.2
> - TLS_DHE_RSA_AES_128_CBC_SHA1 0x00, 0x33 TLS1.0
> total: 29 ciphers
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> include/crypto/tls-cipher-suites.h | 39 +++++++++
> crypto/tls-cipher-suites.c | 133 +++++++++++++++++++++++++++++
> crypto/Makefile.objs | 1 +
> 3 files changed, 173 insertions(+)
> create mode 100644 include/crypto/tls-cipher-suites.h
> create mode 100644 crypto/tls-cipher-suites.c
>
> diff --git a/include/crypto/tls-cipher-suites.h b/include/crypto/tls-cipher-suites.h
> new file mode 100644
> index 0000000000..31e92916e1
> --- /dev/null
> +++ b/include/crypto/tls-cipher-suites.h
> @@ -0,0 +1,39 @@
> +/*
> + * QEMU TLS Cipher Suites
> + *
> + * Copyright (c) 2019 Red Hat, Inc.
> + *
> + * Author: Philippe Mathieu-Daudé <philmd@redhat.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef QCRYPTO_TLSCIPHERSUITES_H
> +#define QCRYPTO_TLSCIPHERSUITES_H
> +
> +#include "qom/object.h"
> +#include "crypto/tlscreds.h"
> +
> +#define TYPE_QCRYPTO_TLS_CIPHER_SUITES "tls-cipher-suites"
> +#define QCRYPTO_TLS_CIPHER_SUITES(obj) \
> + OBJECT_CHECK(QCryptoTLSCipherSuites, (obj), TYPE_QCRYPTO_TLS_CIPHER_SUITES)
> +
> +/*
> + * IANA registered TLS ciphers:
> + * https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-4
> + */
> +typedef struct {
> + uint8_t data[2];
> +} IANA_TLS_CIPHER;
(1) I propose marking this as QEMU_PACKED, even if only for
documentation purposes.
> +
> +typedef struct QCryptoTLSCipherSuites {
> + /* <private> */
> + QCryptoTLSCreds parent_obj;
> +
> + /* <public> */
> + bool verbose;
> + IANA_TLS_CIPHER *cipher_list;
> + unsigned cipher_count;
> +} QCryptoTLSCipherSuites;
> +
> +#endif /* QCRYPTO_TLSCIPHERSUITES_H */
> diff --git a/crypto/tls-cipher-suites.c b/crypto/tls-cipher-suites.c
> new file mode 100644
> index 0000000000..c6c51359bd
> --- /dev/null
> +++ b/crypto/tls-cipher-suites.c
> @@ -0,0 +1,133 @@
> +/*
> + * QEMU TLS Cipher Suites
> + *
> + * Copyright (c) 2019 Red Hat, Inc.
> + *
> + * Author: Philippe Mathieu-Daudé <philmd@redhat.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qom/object_interfaces.h"
> +#include "qemu/error-report.h"
> +#include "crypto/tlscreds.h"
> +#include "crypto/tls-cipher-suites.h"
> +
> +static void parse_cipher_suites(QCryptoTLSCipherSuites *s,
> + const char *priority_name, Error **errp)
> +{
> +#ifdef CONFIG_GNUTLS
> + int ret;
> + unsigned int idx;
> + const char *name;
> + const char *err;
> + gnutls_protocol_t version;
> + gnutls_priority_t pcache;
> +
> + assert(priority_name);
> + ret = gnutls_priority_init(&pcache, priority_name, &err);
> + if (ret < 0) {
> + error_setg(errp, "Syntax error using priority '%s': %s",
> + priority_name, gnutls_strerror(ret));
> + return;
> + }
> +
> + if (s->verbose) {
> + fprintf(stderr, "Cipher suites for %s:\n", priority_name);
> + }
> + for (size_t i = 0;; i++) {
> + ret = gnutls_priority_get_cipher_suite_index(pcache, i, &idx);
> + if (ret == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) {
> + break;
> + }
> + if (ret == GNUTLS_E_UNKNOWN_CIPHER_SUITE) {
> + continue;
> + }
> + s->cipher_list = g_renew(IANA_TLS_CIPHER,
> + s->cipher_list, s->cipher_count + 1);
> +
> + name = gnutls_cipher_suite_info(idx,
> + s->cipher_list[s->cipher_count].data,
> + NULL, NULL, NULL, &version);
> + if (name != NULL) {
> + if (s->verbose) {
> + fprintf(stderr, "- %-50s\t0x%02x, 0x%02x\t%s\n", name,
> + s->cipher_list[s->cipher_count].data[0],
> + s->cipher_list[s->cipher_count].data[1],
> + gnutls_protocol_get_name(version));
> + }
> + s->cipher_count++;
> + }
> + }
(2) I propose turning this into two loops (in sequence), so that we
don't have to call g_renew() in any loop body. The first loop would just
filter & count, then we'd allocate once, and the second loop would
filter and populate.
Alternatively, I sometimes use the following pattern:
unsigned mode;
for (mode = 0; mode < 2; mode++) {
size_t i;
for (i = 0;; i++) {
int ret;
unsigned idx;
const char *name;
IANA_TLS_CIPHER cipher;
gnutls_protocol_t version;
ret = gnutls_priority_get_cipher_suite_index(pcache, i, &idx);
if (ret == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) {
break;
}
if (ret == GNUTLS_E_UNKNOWN_CIPHER_SUITE) {
continue;
}
name = gnutls_cipher_suite_info(idx, &cipher, NULL, NULL, NULL,
&version);
if (name == NULL) {
continue;
}
if (mode == 1) {
if (s->verbose) {
/* ... log "name" and "cipher" ... */
}
s->cipher_list[s->cipher_count] = cipher;
}
s->cipher_count++;
}
if (mode == 0) {
if (s->cipher_count == 0) {
break;
}
s->cipher_list = g_new(IANA_TLS_CIPHER, s->cipher_count);
s->cipher_count = 0;
}
}
Feel free to ignore either point I've brought up.
No other comments from me for this patch.
Thanks,
Laszlo
> + if (s->verbose) {
> + fprintf(stderr, "total: %u ciphers\n", s->cipher_count);
> + }
> + gnutls_priority_deinit(pcache);
> +#else
> + error_setg(errp, "GNU TLS not available");
> +#endif /* CONFIG_GNUTLS */
> +}
> +
> +static void qcrypto_tls_cipher_suites_complete(UserCreatable *uc, Error **errp)
> +{
> + QCryptoTLSCreds *s = QCRYPTO_TLS_CREDS(uc);
> +
> + if (!s->priority) {
> + error_setg(errp, "'priority' property is not set");
> + return;
> + }
> + parse_cipher_suites(QCRYPTO_TLS_CIPHER_SUITES(s), s->priority, errp);
> +}
> +
> +static void qcrypto_tls_cipher_suites_set_verbose(Object *obj, bool value,
> + Error **errp G_GNUC_UNUSED)
> +{
> + QCRYPTO_TLS_CIPHER_SUITES(obj)->verbose = value;
> +}
> +
> +
> +static bool qcrypto_tls_cipher_suites_get_verbose(Object *obj,
> + Error **errp G_GNUC_UNUSED)
> +{
> + return QCRYPTO_TLS_CIPHER_SUITES(obj)->verbose;
> +}
> +
> +static void qcrypto_tls_cipher_suites_finalize(Object *obj)
> +{
> + QCryptoTLSCipherSuites *s = QCRYPTO_TLS_CIPHER_SUITES(obj);
> +
> + g_free(s->cipher_list);
> +}
> +
> +static void qcrypto_tls_cipher_suites_class_init(ObjectClass *oc, void *data)
> +{
> + UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> +
> + ucc->complete = qcrypto_tls_cipher_suites_complete;
> +
> + object_class_property_add_bool(oc, "verbose",
> + qcrypto_tls_cipher_suites_get_verbose,
> + qcrypto_tls_cipher_suites_set_verbose);
> +}
> +
> +static const TypeInfo qcrypto_tls_cipher_suites_info = {
> + .parent = TYPE_QCRYPTO_TLS_CREDS,
> + .name = TYPE_QCRYPTO_TLS_CIPHER_SUITES,
> + .instance_size = sizeof(QCryptoTLSCipherSuites),
> + .instance_finalize = qcrypto_tls_cipher_suites_finalize,
> + .class_size = sizeof(QCryptoTLSCredsClass),
> + .class_init = qcrypto_tls_cipher_suites_class_init,
> + .interfaces = (InterfaceInfo[]) {
> + { TYPE_USER_CREATABLE },
> + { }
> + }
> +};
> +
> +static void qcrypto_tls_cipher_suites_register_types(void)
> +{
> + type_register_static(&qcrypto_tls_cipher_suites_info);
> +}
> +
> +type_init(qcrypto_tls_cipher_suites_register_types);
> diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
> index c2a371b0b4..ce706d322a 100644
> --- a/crypto/Makefile.objs
> +++ b/crypto/Makefile.objs
> @@ -13,6 +13,7 @@ crypto-obj-y += cipher.o
> crypto-obj-$(CONFIG_AF_ALG) += afalg.o
> crypto-obj-$(CONFIG_AF_ALG) += cipher-afalg.o
> crypto-obj-$(CONFIG_AF_ALG) += hash-afalg.o
> +crypto-obj-y += tls-cipher-suites.o
> crypto-obj-y += tlscreds.o
> crypto-obj-y += tlscredsanon.o
> crypto-obj-y += tlscredspsk.o
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites
2020-05-19 18:20 [PATCH v6 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites Philippe Mathieu-Daudé
` (4 preceding siblings ...)
2020-05-19 18:20 ` [PATCH v6 5/5] crypto/tls-cipher-suites: Product fw_cfg consumable blob Philippe Mathieu-Daudé
@ 2020-05-27 11:29 ` Philippe Mathieu-Daudé
2020-05-27 11:33 ` Daniel P. Berrangé
5 siblings, 1 reply; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-27 11:29 UTC (permalink / raw)
To: qemu-devel, Daniel P. Berrangé
Cc: Paolo Bonzini, Laszlo Ersek, Eduardo Habkost, Gerd Hoffmann
Hi Daniel,
On 5/19/20 8:20 PM, Philippe Mathieu-Daudé wrote:
> Hi,
>
> This series has two parts:
>
> - First we add the ability to QOM objects to produce data
> consumable by the fw_cfg device,
>
> - Then we add the tls-cipher-suites object, and let it
> implement the FW_CFG_DATA_GENERATOR interface.
Can I have a quick feedback that you are not going to NAck this series
later before addressing all comments from Laszlo's reviews?
Thanks!
Phil.
>
> This is required by EDK2 'HTTPS Boot' feature [*] to tell
> the guest which TLS ciphers it can use.
>
> [*]: https://github.com/tianocore/edk2/blob/master/OvmfPkg/README
>
> Since v5:
> - Complete rewrite after chatting with Daniel Berrangé
> Since v4:
> - Addressed Laszlo comments (see patch#1 description)
> Since v3:
> - Addressed Markus' comments (do not care about heap)
> Since v2:
> - Split of
> Since v1:
> - Addressed Michael and Laszlo comments.
>
> Please review,
>
> Phil.
>
> v5: https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg04525.html
> v4: https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg04300.html
> v3: https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg02965.html
> v2: https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg02522.html
> v1: https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01598.html
> Supersedes: <20190620122132.10075-1-philmd@redhat.com>
>
> Philippe Mathieu-Daudé (5):
> hw/nvram/fw_cfg: Add the FW_CFG_DATA_GENERATOR interface
> softmmu/vl: Let -fw_cfg option take a 'blob_id' argument
> softmmu/vl: Allow -fw_cfg 'blob_id' option to set any file pathname
> crypto: Add tls-cipher-suites object
> crypto/tls-cipher-suites: Product fw_cfg consumable blob
>
> include/crypto/tls-cipher-suites.h | 39 ++++++++
> include/hw/nvram/fw_cfg.h | 49 ++++++++++
> crypto/tls-cipher-suites.c | 152 +++++++++++++++++++++++++++++
> hw/nvram/fw_cfg.c | 30 ++++++
> softmmu/vl.c | 19 +++-
> crypto/Makefile.objs | 1 +
> 6 files changed, 285 insertions(+), 5 deletions(-)
> create mode 100644 include/crypto/tls-cipher-suites.h
> create mode 100644 crypto/tls-cipher-suites.c
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites
2020-05-27 11:29 ` [PATCH v6 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites Philippe Mathieu-Daudé
@ 2020-05-27 11:33 ` Daniel P. Berrangé
2020-05-27 11:34 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 21+ messages in thread
From: Daniel P. Berrangé @ 2020-05-27 11:33 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Paolo Bonzini, Gerd Hoffmann, Laszlo Ersek, qemu-devel, Eduardo Habkost
On Wed, May 27, 2020 at 01:29:20PM +0200, Philippe Mathieu-Daudé wrote:
> Hi Daniel,
>
> On 5/19/20 8:20 PM, Philippe Mathieu-Daudé wrote:
> > Hi,
> >
> > This series has two parts:
> >
> > - First we add the ability to QOM objects to produce data
> > consumable by the fw_cfg device,
> >
> > - Then we add the tls-cipher-suites object, and let it
> > implement the FW_CFG_DATA_GENERATOR interface.
>
> Can I have a quick feedback that you are not going to NAck this series
> later before addressing all comments from Laszlo's reviews?
It looks reasonable.
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 :|
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites
2020-05-27 11:33 ` Daniel P. Berrangé
@ 2020-05-27 11:34 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-27 11:34 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Paolo Bonzini, Gerd Hoffmann, Laszlo Ersek, qemu-devel, Eduardo Habkost
On 5/27/20 1:33 PM, Daniel P. Berrangé wrote:
> On Wed, May 27, 2020 at 01:29:20PM +0200, Philippe Mathieu-Daudé wrote:
>> Hi Daniel,
>>
>> On 5/19/20 8:20 PM, Philippe Mathieu-Daudé wrote:
>>> Hi,
>>>
>>> This series has two parts:
>>>
>>> - First we add the ability to QOM objects to produce data
>>> consumable by the fw_cfg device,
>>>
>>> - Then we add the tls-cipher-suites object, and let it
>>> implement the FW_CFG_DATA_GENERATOR interface.
>>
>> Can I have a quick feedback that you are not going to NAck this series
>> later before addressing all comments from Laszlo's reviews?
>
> It looks reasonable.
Thanks for your quick answer!
Phil.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 4/5] crypto: Add tls-cipher-suites object
2020-05-19 18:20 ` [PATCH v6 4/5] crypto: Add tls-cipher-suites object Philippe Mathieu-Daudé
2020-05-19 23:24 ` Laszlo Ersek
@ 2020-05-27 11:36 ` Daniel P. Berrangé
2020-05-28 10:17 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 21+ messages in thread
From: Daniel P. Berrangé @ 2020-05-27 11:36 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Paolo Bonzini, Gerd Hoffmann, Laszlo Ersek, qemu-devel, Eduardo Habkost
On Tue, May 19, 2020 at 08:20:23PM +0200, Philippe Mathieu-Daudé wrote:
> Example of use to dump:
>
> $ qemu-system-x86_64 -S \
> -object tls-cipher-suites,id=mysuite,priority=@SYSTEM,verbose=yes
> Cipher suites for @SYSTEM:
> - TLS_AES_256_GCM_SHA384 0x13, 0x02 TLS1.3
> - TLS_CHACHA20_POLY1305_SHA256 0x13, 0x03 TLS1.3
> - TLS_AES_128_GCM_SHA256 0x13, 0x01 TLS1.3
> - TLS_AES_128_CCM_SHA256 0x13, 0x04 TLS1.3
> - TLS_ECDHE_RSA_AES_256_GCM_SHA384 0xc0, 0x30 TLS1.2
> - TLS_ECDHE_RSA_CHACHA20_POLY1305 0xcc, 0xa8 TLS1.2
> - TLS_ECDHE_RSA_AES_256_CBC_SHA1 0xc0, 0x14 TLS1.0
> - TLS_ECDHE_RSA_AES_128_GCM_SHA256 0xc0, 0x2f TLS1.2
> - TLS_ECDHE_RSA_AES_128_CBC_SHA1 0xc0, 0x13 TLS1.0
> - TLS_ECDHE_ECDSA_AES_256_GCM_SHA384 0xc0, 0x2c TLS1.2
> - TLS_ECDHE_ECDSA_CHACHA20_POLY1305 0xcc, 0xa9 TLS1.2
> - TLS_ECDHE_ECDSA_AES_256_CCM 0xc0, 0xad TLS1.2
> - TLS_ECDHE_ECDSA_AES_256_CBC_SHA1 0xc0, 0x0a TLS1.0
> - TLS_ECDHE_ECDSA_AES_128_GCM_SHA256 0xc0, 0x2b TLS1.2
> - TLS_ECDHE_ECDSA_AES_128_CCM 0xc0, 0xac TLS1.2
> - TLS_ECDHE_ECDSA_AES_128_CBC_SHA1 0xc0, 0x09 TLS1.0
> - TLS_RSA_AES_256_GCM_SHA384 0x00, 0x9d TLS1.2
> - TLS_RSA_AES_256_CCM 0xc0, 0x9d TLS1.2
> - TLS_RSA_AES_256_CBC_SHA1 0x00, 0x35 TLS1.0
> - TLS_RSA_AES_128_GCM_SHA256 0x00, 0x9c TLS1.2
> - TLS_RSA_AES_128_CCM 0xc0, 0x9c TLS1.2
> - TLS_RSA_AES_128_CBC_SHA1 0x00, 0x2f TLS1.0
> - TLS_DHE_RSA_AES_256_GCM_SHA384 0x00, 0x9f TLS1.2
> - TLS_DHE_RSA_CHACHA20_POLY1305 0xcc, 0xaa TLS1.2
> - TLS_DHE_RSA_AES_256_CCM 0xc0, 0x9f TLS1.2
> - TLS_DHE_RSA_AES_256_CBC_SHA1 0x00, 0x39 TLS1.0
> - TLS_DHE_RSA_AES_128_GCM_SHA256 0x00, 0x9e TLS1.2
> - TLS_DHE_RSA_AES_128_CCM 0xc0, 0x9e TLS1.2
> - TLS_DHE_RSA_AES_128_CBC_SHA1 0x00, 0x33 TLS1.0
> total: 29 ciphers
IMHO this "verbose" option shouldn't exist. Instead we should be
using the QEMU trace infrastructure to log this information. This
will make it possible to trace the info at runtime in production
deployments too
> +static void parse_cipher_suites(QCryptoTLSCipherSuites *s,
> + const char *priority_name, Error **errp)
> +{
> +#ifdef CONFIG_GNUTLS
Instead of doing this......
> diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
> index c2a371b0b4..ce706d322a 100644
> --- a/crypto/Makefile.objs
> +++ b/crypto/Makefile.objs
> @@ -13,6 +13,7 @@ crypto-obj-y += cipher.o
> crypto-obj-$(CONFIG_AF_ALG) += afalg.o
> crypto-obj-$(CONFIG_AF_ALG) += cipher-afalg.o
> crypto-obj-$(CONFIG_AF_ALG) += hash-afalg.o
> +crypto-obj-y += tls-cipher-suites.o
....Use crypto-obj-$(CONFIG_GNUTLS) += tls-cipher-suites.o
This lets the mgmt appliction introspect QEMU to discover whether the
TLS cipher suits object is present & usable.
> crypto-obj-y += tlscreds.o
> crypto-obj-y += tlscredsanon.o
> crypto-obj-y += tlscredspsk.o
> --
> 2.21.3
>
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 :|
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 2/5] softmmu/vl: Let -fw_cfg option take a 'blob_id' argument
2020-05-19 18:20 ` [PATCH v6 2/5] softmmu/vl: Let -fw_cfg option take a 'blob_id' argument Philippe Mathieu-Daudé
2020-05-19 22:34 ` Laszlo Ersek
@ 2020-05-27 11:38 ` Daniel P. Berrangé
1 sibling, 0 replies; 21+ messages in thread
From: Daniel P. Berrangé @ 2020-05-27 11:38 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Paolo Bonzini, Gerd Hoffmann, Laszlo Ersek, qemu-devel, Eduardo Habkost
On Tue, May 19, 2020 at 08:20:21PM +0200, Philippe Mathieu-Daudé wrote:
> The 'blob_id' argument refers to a QOM object able to produce
> data consumable by the fw_cfg device. The producer object must
> implement the FW_CFG_DATA_GENERATOR interface.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> softmmu/vl.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index ae5451bc23..f76c53ad2e 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -489,6 +489,10 @@ static QemuOptsList qemu_fw_cfg_opts = {
> .name = "string",
> .type = QEMU_OPT_STRING,
> .help = "Sets content of the blob to be inserted from a string",
> + }, {
> + .name = "blob_id",
> + .type = QEMU_OPT_STRING,
> + .help = "Sets id of the object generating fw_cfg blob to be used",
> },
<bikeshed>
I wonder if "generator_id" or "gen_id" is more appropriate as the property
name
</bikeshed>
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 :|
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 4/5] crypto: Add tls-cipher-suites object
2020-05-27 11:36 ` Daniel P. Berrangé
@ 2020-05-28 10:17 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-28 10:17 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Paolo Bonzini, Gerd Hoffmann, Laszlo Ersek, qemu-devel, Eduardo Habkost
On 5/27/20 1:36 PM, Daniel P. Berrangé wrote:
> On Tue, May 19, 2020 at 08:20:23PM +0200, Philippe Mathieu-Daudé wrote:
>> Example of use to dump:
>>
>> $ qemu-system-x86_64 -S \
>> -object tls-cipher-suites,id=mysuite,priority=@SYSTEM,verbose=yes
>> Cipher suites for @SYSTEM:
>> - TLS_AES_256_GCM_SHA384 0x13, 0x02 TLS1.3
>> - TLS_CHACHA20_POLY1305_SHA256 0x13, 0x03 TLS1.3
>> - TLS_AES_128_GCM_SHA256 0x13, 0x01 TLS1.3
>> - TLS_AES_128_CCM_SHA256 0x13, 0x04 TLS1.3
>> - TLS_ECDHE_RSA_AES_256_GCM_SHA384 0xc0, 0x30 TLS1.2
>> - TLS_ECDHE_RSA_CHACHA20_POLY1305 0xcc, 0xa8 TLS1.2
>> - TLS_ECDHE_RSA_AES_256_CBC_SHA1 0xc0, 0x14 TLS1.0
>> - TLS_ECDHE_RSA_AES_128_GCM_SHA256 0xc0, 0x2f TLS1.2
>> - TLS_ECDHE_RSA_AES_128_CBC_SHA1 0xc0, 0x13 TLS1.0
>> - TLS_ECDHE_ECDSA_AES_256_GCM_SHA384 0xc0, 0x2c TLS1.2
>> - TLS_ECDHE_ECDSA_CHACHA20_POLY1305 0xcc, 0xa9 TLS1.2
>> - TLS_ECDHE_ECDSA_AES_256_CCM 0xc0, 0xad TLS1.2
>> - TLS_ECDHE_ECDSA_AES_256_CBC_SHA1 0xc0, 0x0a TLS1.0
>> - TLS_ECDHE_ECDSA_AES_128_GCM_SHA256 0xc0, 0x2b TLS1.2
>> - TLS_ECDHE_ECDSA_AES_128_CCM 0xc0, 0xac TLS1.2
>> - TLS_ECDHE_ECDSA_AES_128_CBC_SHA1 0xc0, 0x09 TLS1.0
>> - TLS_RSA_AES_256_GCM_SHA384 0x00, 0x9d TLS1.2
>> - TLS_RSA_AES_256_CCM 0xc0, 0x9d TLS1.2
>> - TLS_RSA_AES_256_CBC_SHA1 0x00, 0x35 TLS1.0
>> - TLS_RSA_AES_128_GCM_SHA256 0x00, 0x9c TLS1.2
>> - TLS_RSA_AES_128_CCM 0xc0, 0x9c TLS1.2
>> - TLS_RSA_AES_128_CBC_SHA1 0x00, 0x2f TLS1.0
>> - TLS_DHE_RSA_AES_256_GCM_SHA384 0x00, 0x9f TLS1.2
>> - TLS_DHE_RSA_CHACHA20_POLY1305 0xcc, 0xaa TLS1.2
>> - TLS_DHE_RSA_AES_256_CCM 0xc0, 0x9f TLS1.2
>> - TLS_DHE_RSA_AES_256_CBC_SHA1 0x00, 0x39 TLS1.0
>> - TLS_DHE_RSA_AES_128_GCM_SHA256 0x00, 0x9e TLS1.2
>> - TLS_DHE_RSA_AES_128_CCM 0xc0, 0x9e TLS1.2
>> - TLS_DHE_RSA_AES_128_CBC_SHA1 0x00, 0x33 TLS1.0
>> total: 29 ciphers
>
> IMHO this "verbose" option shouldn't exist. Instead we should be
> using the QEMU trace infrastructure to log this information. This
> will make it possible to trace the info at runtime in production
> deployments too
OK, clever.
>
>> +static void parse_cipher_suites(QCryptoTLSCipherSuites *s,
>> + const char *priority_name, Error **errp)
>> +{
>> +#ifdef CONFIG_GNUTLS
>
> Instead of doing this......
>
>
>> diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
>> index c2a371b0b4..ce706d322a 100644
>> --- a/crypto/Makefile.objs
>> +++ b/crypto/Makefile.objs
>> @@ -13,6 +13,7 @@ crypto-obj-y += cipher.o
>> crypto-obj-$(CONFIG_AF_ALG) += afalg.o
>> crypto-obj-$(CONFIG_AF_ALG) += cipher-afalg.o
>> crypto-obj-$(CONFIG_AF_ALG) += hash-afalg.o
>> +crypto-obj-y += tls-cipher-suites.o
>
> ....Use crypto-obj-$(CONFIG_GNUTLS) += tls-cipher-suites.o
>
> This lets the mgmt appliction introspect QEMU to discover whether the
> TLS cipher suits object is present & usable.
OK, thanks!
>
>> crypto-obj-y += tlscreds.o
>> crypto-obj-y += tlscredsanon.o
>> crypto-obj-y += tlscredspsk.o
>> --
>> 2.21.3
>>
>
> Regards,
> Daniel
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 2/5] softmmu/vl: Let -fw_cfg option take a 'blob_id' argument
2020-05-19 22:34 ` Laszlo Ersek
@ 2020-05-28 12:07 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-28 12:07 UTC (permalink / raw)
To: Laszlo Ersek, qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost, Gerd Hoffmann
On 5/20/20 12:34 AM, Laszlo Ersek wrote:
> On 05/19/20 20:20, Philippe Mathieu-Daudé wrote:
>> The 'blob_id' argument refers to a QOM object able to produce
>> data consumable by the fw_cfg device. The producer object must
>> implement the FW_CFG_DATA_GENERATOR interface.
>
> OK, this answers my OBJECT_CHECK() question under patch #1 (in the
> negative -- an assert would be wrong).
>
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> softmmu/vl.c | 17 +++++++++++++----
>> 1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index ae5451bc23..f76c53ad2e 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -489,6 +489,10 @@ static QemuOptsList qemu_fw_cfg_opts = {
>> .name = "string",
>> .type = QEMU_OPT_STRING,
>> .help = "Sets content of the blob to be inserted from a string",
>> + }, {
>> + .name = "blob_id",
>> + .type = QEMU_OPT_STRING,
>> + .help = "Sets id of the object generating fw_cfg blob to be used",
>> },
>> { /* end of list */ }
>> },
>> @@ -2020,7 +2024,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
>> {
>> gchar *buf;
>> size_t size;
>> - const char *name, *file, *str;
>> + const char *name, *file, *str, *blob_id;
>> FWCfgState *fw_cfg = (FWCfgState *) opaque;
>>
>> if (fw_cfg == NULL) {
>> @@ -2030,14 +2034,17 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
>> name = qemu_opt_get(opts, "name");
>> file = qemu_opt_get(opts, "file");
>> str = qemu_opt_get(opts, "string");
>> + blob_id = qemu_opt_get(opts, "blob_id");
>>
>> /* we need name and either a file or the content string */
>
> (1) Please update this comment. If the option is given, we need the
> name, and exactly one of: file, content string, blob_id.
>
>> - if (!(nonempty_str(name) && (nonempty_str(file) || nonempty_str(str)))) {
>> + if (!(nonempty_str(name)
>> + && (nonempty_str(file) || nonempty_str(str) || nonempty_str(blob_id)))
>> + ) {
>> error_setg(errp, "invalid argument(s)");
>> return -1;
>> }
>
> (2) Coding style: does QEMU keep operators on the left or on the right
> when breaking subconditions to new lines? (I vaguely recall "to the
> right", but I could be wrong... Well, "hw/nvram/fw_cfg.c" has at least 7
> examples of the operator being on the right.)
You are right, I have been confused with a recommendation from Eric
Blake, but it was about shell script, not C.
>
>> - if (nonempty_str(file) && nonempty_str(str)) {
>> - error_setg(errp, "file and string are mutually exclusive");
>> + if (nonempty_str(file) && nonempty_str(str) && nonempty_str(blob_id)) {
>> + error_setg(errp, "file, string and blob_id are mutually exclusive");
>> return -1;
>> }
>
> (3) I believe this catches only when all three of name/string/blob_id
> are given. But we should continue catching "two given".
>
> How about reworking both "if"s, *and* the comment at (1) at the same
> time, into:
>
> if (!nonempty_str(name) ||
> nonempty_str(file) + nonempty_str(str) + nonempty_str(blob_id) != 1) {
> error_setg(errp, "name, plus exactly one of file, string and blob_id, "
> "are needed");
> return -1;
> }
>
> (Regarding the addition, nonempty_str() returns a "bool", which is a
> macro to _Bool, which is promoted to "int" or "unsigned int".)
>
>> if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) {
>> @@ -2052,6 +2059,8 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
>> if (nonempty_str(str)) {
>> size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
>> buf = g_memdup(str, size);
>> + } else if (nonempty_str(blob_id)) {
>> + return fw_cfg_add_from_generator(fw_cfg, name, blob_id, errp);
>> } else {
>> GError *err = NULL;
>> if (!g_file_get_contents(file, &buf, &size, &err)) {
>>
>
> (4) The "-fw_cfg" command line option is documented in both the qemu(1)
> manual, and the "docs/specs/fw_cfg.txt" file.
>
> I think we may have to update those. In particular I mean *where* the
> option is documented (in both texts).
Oh I forgot this part, thanks.
>
> In the manual, "-fw_cfg" is currently under "Debug/Expert options", but
> that will no longer apply (I think?) after this series.
Well I'm not sure, the intent is the same, targeting mostly libvirt as
management interface; other uses are for "experts".
>
> Similarly, in "docs/specs/fw_cfg.txt", the section is called "Externally
> Provided Items" -- but that might not be strictly true any more either.
>
> Maybe leave the current "-fw_cfg" mentions in peace, and document
> "-fw_cfg blob_id=..." separately (in different docs sections)? The
> "fw_cfg generators" concept could deserve dedicated sections.
>
> Sorry that I can't make a good concrete suggestion. :(
Thanks for the detailed review!
>
> Thanks,
> Laszlo
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 1/5] hw/nvram/fw_cfg: Add the FW_CFG_DATA_GENERATOR interface
2020-05-19 22:01 ` Laszlo Ersek
@ 2020-05-28 14:54 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-28 14:54 UTC (permalink / raw)
To: Laszlo Ersek, qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost, Gerd Hoffmann
On 5/20/20 12:01 AM, Laszlo Ersek wrote:
> On 05/19/20 20:20, Philippe Mathieu-Daudé wrote:
>> The FW_CFG_DATA_GENERATOR allow any object to product
>
> (1) I suggest:
>
> s/allow/allows/
> s/product/produce/
>
>> blob of data consumable by the fw_cfg device.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> include/hw/nvram/fw_cfg.h | 49 +++++++++++++++++++++++++++++++++++++++
>> hw/nvram/fw_cfg.c | 30 ++++++++++++++++++++++++
>> 2 files changed, 79 insertions(+)
>>
>> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
>> index 25d9307018..74b4790fae 100644
>> --- a/include/hw/nvram/fw_cfg.h
>> +++ b/include/hw/nvram/fw_cfg.h
>> @@ -9,11 +9,40 @@
>> #define TYPE_FW_CFG "fw_cfg"
>> #define TYPE_FW_CFG_IO "fw_cfg_io"
>> #define TYPE_FW_CFG_MEM "fw_cfg_mem"
>> +#define TYPE_FW_CFG_DATA_GENERATOR_INTERFACE "fw_cfg-data-generator"
>>
>> #define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG)
>> #define FW_CFG_IO(obj) OBJECT_CHECK(FWCfgIoState, (obj), TYPE_FW_CFG_IO)
>> #define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
>>
>> +#define FW_CFG_DATA_GENERATOR_CLASS(class) \
>> + OBJECT_CLASS_CHECK(FWCfgDataGeneratorClass, (class), \
>> + TYPE_FW_CFG_DATA_GENERATOR_INTERFACE)
>> +#define FW_CFG_DATA_GENERATOR_GET_CLASS(obj) \
>> + OBJECT_GET_CLASS(FWCfgDataGeneratorClass, (obj), \
>> + TYPE_FW_CFG_DATA_GENERATOR_INTERFACE)
>> +
>> +typedef struct FWCfgDataGeneratorClass {
>> + /*< private >*/
>> + InterfaceClass parent_class;
>> + /*< public >*/
>> +
>> + /**
>> + * get_data:
>> + * @obj: the object implementing this interface
>> + *
>> + * Returns: pointer to start of the generated item data
>> + */
>> + const void *(*get_data)(Object *obj);
>
> I'm not familiar with QOM, so please excuse any dumb questions.
>
> "const" suggests the blob returned remains owned by "obj";
Yes, it is owned by the generator object.
fw_cfg_add_from_generator() does a memdup().
> that answers
> the question whether the caller should attempt to free the blob. (The
> answer is "no".)
>
> (2) However, will this perhaps expose other functions, currently taking
> non-const-qualified pointers, to which we'd like to pass the blob
> returned by the above member function?
>
> Because, then we'd have to cast away "const",
It is illegal to cast away "const". This is why I choose to use it here,
if the consumer want to modify it, it is forced to make its own copy.
> and I find that much
> uglier than removing the "const" from *here*, and adding a more verbose
> comment as replacement.
>
> Yes, this is clearly speculation -- IOW just a question. If all the
> functions we're going to pass the return value to are fine with
> pointer-to-const, then this interface should be OK.
The only user so far uses memdup().
>
> (Obviously when I say "cast away const", I think of functions that do
> not actually modify the object pointed-to by the non-const-qualified
> pointer.)
>
>> + /**
>> + * get_length:
>> + * @obj: the object implementing this interface
>> + *
>> + * Returns: the size of the generated item data in bytes
>> + */
>> + size_t (*get_length)(Object *obj);
>> +} FWCfgDataGeneratorClass;
>> +
>> typedef struct fw_cfg_file FWCfgFile;
>>
>> #define FW_CFG_ORDER_OVERRIDE_VGA 70
>> @@ -263,6 +292,26 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
>> void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
>> size_t len);
>>
>> +/**
>> + * fw_cfg_add_from_generator:
>> + * @s: fw_cfg device being modified
>> + * @filename: name of new fw_cfg file item
>> + * @generator_id: name of object implementing FW_CFG_DATA_GENERATOR interface
>> + * @errp: pointer to a NULL initialized error object
>> + *
>> + * Add a new NAMED fw_cfg item with the content generated from the
>> + * @generator_id object. The data referenced by the starting pointer is copied
>
> (3) s/referenced by the starting pointer/generated by the @generator_id
> object/
>
>> + * into the data structure of the fw_cfg device.
>> + * The next available (unused) selector key starting at FW_CFG_FILE_FIRST
>> + * will be used; also, a new entry will be added to the file directory
>> + * structure residing at key value FW_CFG_FILE_DIR, containing the item name,
>> + * data size, and assigned selector key value.
>> + *
>> + * Returns: the size of the generated item data on success, -1 otherwise.
>
> (4) I don't like ssize_t for a return value like this.
>
> First, get_length() returns size_t, which may not be representable in an
> ssize_t.
>
> (Actually, it's worse than that; POSIX says, "the type ssize_t shall be
> capable of storing values at least in the range [-1, {SSIZE_MAX}]" --
> and if I run "getconf SSIZE_MAX", I get 32767. Indeed, _POSIX_SSIZE_MAX,
> which is the minimum for any implementation's SSIZE_MAX, is 32767.)
>
> Second, is a zero-sized blob useful in fw_cfg (from a generator)?
Not for now. We can add it later anyway.
>
> If it is not useful, then this function should return size_t, and use
> retval=0 for signaling an error.
OK.
>
> If a zero-sized blob is useful, then the function should return a bool
> (in addition to producing "errp"), and output the blob size as a
> separate parameter.
>
>> + */
>> +ssize_t fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
>> + const char *generator_id, Error **errp);
>> +
>> FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>> AddressSpace *dma_as);
>> FWCfgState *fw_cfg_init_io(uint32_t iobase);
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 8dd50c2c72..e18cb074df 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -1032,6 +1032,30 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
>> return NULL;
>> }
>>
>> +ssize_t fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
>> + const char *generator_id, Error **errp)
>> +{
>> + FWCfgDataGeneratorClass *k;
>> + Object *o;
>
> (5) Not sure about QEMU coding standards, but the above single-char
> variable names (especially "o") terrify me. Please use "klass" and "obj".
>
> Do ignore my request if these variable names are just fine in QEMU.
I don't want to terrify you :P
>
>> + size_t sz;
>> +
>> + o = object_resolve_path_component(object_get_objects_root(), generator_id);
>> + if (!o) {
>> + error_setg(errp, "Cannot find object ID %s", generator_id);
>> + return -1;
>> + }
>> + if (!object_dynamic_cast(o, TYPE_FW_CFG_DATA_GENERATOR_INTERFACE)) {
>> + error_setg(errp, "Object '%s' is not a fw_cfg-data-generator subclass",
>> + generator_id);
>
> (6) We should probably not open code
> TYPE_FW_CFG_DATA_GENERATOR_INTERFACE as "fw_cfg-data-generator" even in
> the error message.
>
> (7) If this branch is taken, would that arguably merit an assertion
> failure? I mean, can the dynamic cast fail without QEMU having a related
> bug somewhere? (Maybe this is going to be answered in the rest of the
> series.) Because I see those OBJECT_CHECK macros near the top of
> "fw_cfg.h", and those boil down to object_dynamic_cast_assert().
>
>> + return -1;
>> + }
>> + k = FW_CFG_DATA_GENERATOR_GET_CLASS(o);
>> + sz = k->get_length(o);
>> + fw_cfg_add_file(s, filename, g_memdup(k->get_data(o), sz), sz);
>
> (g_memdup() takes a "guint" for "byte_size". Whether that matches
> "size_t" is anyone's guess. I guess it can't be helped.)
Similarly it returns a gpointer...
>
>> +
>> + return sz;
>
> Right, this is the size_t --> ssize_t conversion that makes me
> uncomfortable.
>
> I'm OK if you ignore all of my comments, these are simply the thoughts
> that crossed my mind.
Thanks for them!
Regards,
Phil.
>
> Thanks
> Laszlo
>
>> +}
>> +
>> static void fw_cfg_machine_reset(void *opaque)
>> {
>> MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>> @@ -1333,12 +1357,18 @@ static const TypeInfo fw_cfg_mem_info = {
>> .class_init = fw_cfg_mem_class_init,
>> };
>>
>> +static const TypeInfo fw_cfg_data_generator_interface_info = {
>> + .name = TYPE_FW_CFG_DATA_GENERATOR_INTERFACE,
>> + .parent = TYPE_INTERFACE,
>> + .class_size = sizeof(FWCfgDataGeneratorClass),
>> +};
>>
>> static void fw_cfg_register_types(void)
>> {
>> type_register_static(&fw_cfg_info);
>> type_register_static(&fw_cfg_io_info);
>> type_register_static(&fw_cfg_mem_info);
>> + type_register_static(&fw_cfg_data_generator_interface_info);
>> }
>>
>> type_init(fw_cfg_register_types)
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH v6 3/5] softmmu/vl: Allow -fw_cfg 'blob_id' option to set any file pathname
2020-05-19 22:45 ` Laszlo Ersek
@ 2020-05-28 17:03 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-28 17:03 UTC (permalink / raw)
To: Laszlo Ersek, qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost, Gerd Hoffmann
On 5/20/20 12:45 AM, Laszlo Ersek wrote:
> On 05/19/20 20:20, Philippe Mathieu-Daudé wrote:
>> This is to silent:
>>
>> $ qemu-system-x86_64 \
>> -object tls-cipher-suites,id=ciphersuite0,priority=@SYSTEM \
>> -fw_cfg name=etc/edk2/https/ciphers,blob_id=ciphersuite0
>> qemu-system-x86_64: -fw_cfg name=etc/edk2/https/ciphers,blob_id=ciphersuite0: warning: externally provided fw_cfg item names should be prefixed with "opt/"
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> softmmu/vl.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index f76c53ad2e..3b77dcc00d 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -2052,7 +2052,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
>> FW_CFG_MAX_FILE_PATH - 1);
>> return -1;
>> }
>> - if (strncmp(name, "opt/", 4) != 0) {
>> + if (!nonempty_str(blob_id) && strncmp(name, "opt/", 4) != 0) {
>> warn_report("externally provided fw_cfg item names "
>> "should be prefixed with \"opt/\"");
>> }
>>
>
> Hmmm, difficult question! Is "ciphersuite0" now externally provided or not?
>
> Because, ciphersuite0 is populated internally to QEMU alright (and so we
> can think it's internal), but its *association with the name* is external.
>
> What if we keep the same "-object" switch, but use a different (bogus)
> "name" with "-fw_cfg"? IMO that kind of invalidates "-object" too.
>
> Should the fw_cfg generator interface dictate the fw_cfg filename too?
> Because that would eliminate this problem. Put differently, we now have
> a possibility to label the "ciphersuite0" object in the fw_cfg file
> directory any way we want -- but is that freedom *useful* for anything?
Good point. Not now.
>
> I guess we might want multiple "tls-cipher-suites" objects one day, so
> hard-coding the fw_cfg names on that level could cause conflicts.
I haven't thought of that. If we set a limit today, we can relax it
tomorrow. That won't break anything.
> On the
> other hand, I wouldn't like "blob_id" to generally circumvent the "etc/"
> namespace protection.
Eh, that was too easy.
>
> I'm leaning towards agreeing with this patch, but I'd appreciate some
> convincing arguments.
OK. I'll think about it. Maybe keep this as single-RFC patch & repost
the rest so review can go on until we settle on this.
>
> Thanks
> Laszlo
>
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2020-05-28 17:04 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-19 18:20 [PATCH v6 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites Philippe Mathieu-Daudé
2020-05-19 18:20 ` [PATCH v6 1/5] hw/nvram/fw_cfg: Add the FW_CFG_DATA_GENERATOR interface Philippe Mathieu-Daudé
2020-05-19 22:01 ` Laszlo Ersek
2020-05-28 14:54 ` Philippe Mathieu-Daudé
2020-05-19 18:20 ` [PATCH v6 2/5] softmmu/vl: Let -fw_cfg option take a 'blob_id' argument Philippe Mathieu-Daudé
2020-05-19 22:34 ` Laszlo Ersek
2020-05-28 12:07 ` Philippe Mathieu-Daudé
2020-05-27 11:38 ` Daniel P. Berrangé
2020-05-19 18:20 ` [RFC PATCH v6 3/5] softmmu/vl: Allow -fw_cfg 'blob_id' option to set any file pathname Philippe Mathieu-Daudé
2020-05-19 18:22 ` Philippe Mathieu-Daudé
2020-05-19 22:45 ` Laszlo Ersek
2020-05-28 17:03 ` Philippe Mathieu-Daudé
2020-05-19 18:20 ` [PATCH v6 4/5] crypto: Add tls-cipher-suites object Philippe Mathieu-Daudé
2020-05-19 23:24 ` Laszlo Ersek
2020-05-27 11:36 ` Daniel P. Berrangé
2020-05-28 10:17 ` Philippe Mathieu-Daudé
2020-05-19 18:20 ` [PATCH v6 5/5] crypto/tls-cipher-suites: Product fw_cfg consumable blob Philippe Mathieu-Daudé
2020-05-19 22:49 ` Laszlo Ersek
2020-05-27 11:29 ` [PATCH v6 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites Philippe Mathieu-Daudé
2020-05-27 11:33 ` Daniel P. Berrangé
2020-05-27 11:34 ` Philippe Mathieu-Daudé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.