* [PATCH v7 1/5] hw/nvram/fw_cfg: Add the FW_CFG_DATA_GENERATOR interface
2020-05-28 17:31 [PATCH v7 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites Philippe Mathieu-Daudé
@ 2020-05-28 17:31 ` Philippe Mathieu-Daudé
2020-05-29 9:09 ` Laszlo Ersek
2020-05-28 17:31 ` [PATCH v7 2/5] softmmu/vl: Let -fw_cfg option take a 'gen_id' argument Philippe Mathieu-Daudé
` (3 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-28 17:31 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Philippe Mathieu-Daudé,
Daniel P. Berrangé,
Laszlo Ersek, Gerd Hoffmann
The FW_CFG_DATA_GENERATOR allows any object to produce
blob of data consumable by the fw_cfg device.
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v7: addressed Laszlo's comments
- fixed typos in description
- return size_t instead of ssize_t; 0 for error
- do not use 1-letter variable names
- do not open-code 'fw_cfg-data-generator'
- cast g_memdup() size argument as 'guint'
- improved documentation
---
docs/specs/fw_cfg.txt | 9 ++++++-
include/hw/nvram/fw_cfg.h | 52 +++++++++++++++++++++++++++++++++++++++
hw/nvram/fw_cfg.c | 31 +++++++++++++++++++++++
3 files changed, 91 insertions(+), 1 deletion(-)
diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index 8f1ebc66fa..bc16daa38a 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -219,7 +219,7 @@ To check the result, read the "control" field:
= Externally Provided Items =
-As of v2.4, "file" fw_cfg items (i.e., items with selector keys above
+Since v2.4, "file" fw_cfg items (i.e., items with selector keys above
FW_CFG_FILE_FIRST, and with a corresponding entry in the fw_cfg file
directory structure) may be inserted via the QEMU command line, using
the following syntax:
@@ -230,6 +230,13 @@ Or
-fw_cfg [name=]<item_name>,string=<string>
+Since v5.1, QEMU allows some objects to generate fw_cfg-specific content,
+the content is then associated with a "file" item using the 'gen_id' option
+in the command line, using the following syntax:
+
+ -object <generator-type>,id=<generated_id>,[generator-specific-options] \
+ -fw_cfg [name=]<item_name>,gen_id=<generated_id>
+
See QEMU man page for more documentation.
Using item_name with plain ASCII characters only is recommended.
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 25d9307018..8fbf2446c1 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -9,11 +9,43 @@
#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
+ *
+ * The returned pointer is a QObject weak reference, @obj owns
+ * the reference and may free it at any time in the future.
+ */
+ 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 +295,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
+ * @gen_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
+ * @gen_id object. The data generated by the @gen_id object/ 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 device tree image on success, or 0 on errors.
+ */
+size_t fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
+ const char *gen_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..6d2fa13042 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -1032,6 +1032,31 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
return NULL;
}
+size_t fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
+ const char *gen_id, Error **errp)
+{
+ FWCfgDataGeneratorClass *klass;
+ Object *obj;
+ size_t size;
+
+ obj = object_resolve_path_component(object_get_objects_root(), gen_id);
+ if (!obj) {
+ error_setg(errp, "Cannot find object ID %s", gen_id);
+ return 0;
+ }
+ if (!object_dynamic_cast(obj, TYPE_FW_CFG_DATA_GENERATOR_INTERFACE)) {
+ error_setg(errp, "Object '%s' is not a '%s' subclass",
+ TYPE_FW_CFG_DATA_GENERATOR_INTERFACE, gen_id);
+ return 0;
+ }
+ klass = FW_CFG_DATA_GENERATOR_GET_CLASS(obj);
+ size = klass->get_length(obj);
+ fw_cfg_add_file(s, filename, g_memdup(klass->get_data(obj), (guint)size),
+ size);
+
+ return size;
+}
+
static void fw_cfg_machine_reset(void *opaque)
{
MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
@@ -1333,12 +1358,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] 22+ messages in thread* Re: [PATCH v7 1/5] hw/nvram/fw_cfg: Add the FW_CFG_DATA_GENERATOR interface
2020-05-28 17:31 ` [PATCH v7 1/5] hw/nvram/fw_cfg: Add the FW_CFG_DATA_GENERATOR interface Philippe Mathieu-Daudé
@ 2020-05-29 9:09 ` Laszlo Ersek
2020-05-29 9:21 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2020-05-29 9:09 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé, Gerd Hoffmann
On 05/28/20 19:31, Philippe Mathieu-Daudé wrote:
> The FW_CFG_DATA_GENERATOR allows any object to produce
> blob of data consumable by the fw_cfg device.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v7: addressed Laszlo's comments
> - fixed typos in description
> - return size_t instead of ssize_t; 0 for error
> - do not use 1-letter variable names
> - do not open-code 'fw_cfg-data-generator'
> - cast g_memdup() size argument as 'guint'
> - improved documentation
> ---
> docs/specs/fw_cfg.txt | 9 ++++++-
> include/hw/nvram/fw_cfg.h | 52 +++++++++++++++++++++++++++++++++++++++
> hw/nvram/fw_cfg.c | 31 +++++++++++++++++++++++
> 3 files changed, 91 insertions(+), 1 deletion(-)
>
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index 8f1ebc66fa..bc16daa38a 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -219,7 +219,7 @@ To check the result, read the "control" field:
>
> = Externally Provided Items =
>
> -As of v2.4, "file" fw_cfg items (i.e., items with selector keys above
> +Since v2.4, "file" fw_cfg items (i.e., items with selector keys above
> FW_CFG_FILE_FIRST, and with a corresponding entry in the fw_cfg file
> directory structure) may be inserted via the QEMU command line, using
> the following syntax:
> @@ -230,6 +230,13 @@ Or
>
> -fw_cfg [name=]<item_name>,string=<string>
>
> +Since v5.1, QEMU allows some objects to generate fw_cfg-specific content,
> +the content is then associated with a "file" item using the 'gen_id' option
> +in the command line, using the following syntax:
> +
> + -object <generator-type>,id=<generated_id>,[generator-specific-options] \
> + -fw_cfg [name=]<item_name>,gen_id=<generated_id>
> +
> See QEMU man page for more documentation.
>
> Using item_name with plain ASCII characters only is recommended.
I've looked at this hunk with a larger context, and I think it's really
good.
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 25d9307018..8fbf2446c1 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -9,11 +9,43 @@
> #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
> + *
> + * The returned pointer is a QObject weak reference, @obj owns
> + * the reference and may free it at any time in the future.
> + */
> + 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 +295,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
> + * @gen_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
> + * @gen_id object. The data generated by the @gen_id object/ is copied
(1) typo: "object/" (possibly a copy-paste error from my v6 review)
> + * 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 device tree image on success, or 0 on errors.
(2) typo (probably another copy-paste error):
s/device tree image/generated item data/
> + */
> +size_t fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
> + const char *gen_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..6d2fa13042 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -1032,6 +1032,31 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
> return NULL;
> }
>
> +size_t fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
> + const char *gen_id, Error **errp)
> +{
> + FWCfgDataGeneratorClass *klass;
> + Object *obj;
> + size_t size;
> +
> + obj = object_resolve_path_component(object_get_objects_root(), gen_id);
> + if (!obj) {
> + error_setg(errp, "Cannot find object ID %s", gen_id);
> + return 0;
> + }
> + if (!object_dynamic_cast(obj, TYPE_FW_CFG_DATA_GENERATOR_INTERFACE)) {
> + error_setg(errp, "Object '%s' is not a '%s' subclass",
> + TYPE_FW_CFG_DATA_GENERATOR_INTERFACE, gen_id);
(3) the order of the last two arguments is wrong; please reverse it.
> + return 0;
> + }
> + klass = FW_CFG_DATA_GENERATOR_GET_CLASS(obj);
> + size = klass->get_length(obj);
(4) I think we should not call fw_cfg_add_file() if get_length() returns
zero.
AFAICS, fw_cfg_add_file() will happily create a useless zero-size fw_cfg
entry, but then the same zero size that we return from
fw_cfg_add_from_generator() will indicate failure to the caller (and we
will also not have set "errp").
I think we need something like:
if (size == 0) {
error_setg(errp, "Object '%s' failed to generate fw_cfg data",
gen_id);
return 0;
}
That will also prevent an awkward call to g_memdup() with size==0.
Now, looking at patches #4 and #5, I think if no suitable cipher suite
is found, then qcrypto_tls_cipher_suites_get_data() will return NULL,
and qcrypto_tls_cipher_suites_get_length() will return zero. In turn, my
suggestion (4) here will prevent QEMU from starting up.
And I think that is just fine. The corresponding firmware function is
SetCipherSuites() in
"OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c", and if it sees a
zero-sized "etc/edk2/https/ciphers" fw_cfg file, it will hang the
firmware, *on purpose*. (See the reason explained in comments there.)
Thus, it's much better if we prevent QEMU from starting up in the first
place.
> + fw_cfg_add_file(s, filename, g_memdup(klass->get_data(obj), (guint)size),
> + size);
> +
> + return size;
> +}
> +
> static void fw_cfg_machine_reset(void *opaque)
> {
> MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> @@ -1333,12 +1358,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)
>
With (1) through (4) fixed:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v7 1/5] hw/nvram/fw_cfg: Add the FW_CFG_DATA_GENERATOR interface
2020-05-29 9:09 ` Laszlo Ersek
@ 2020-05-29 9:21 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-29 9:21 UTC (permalink / raw)
To: Laszlo Ersek, qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé, Gerd Hoffmann
On 5/29/20 11:09 AM, Laszlo Ersek wrote:
> On 05/28/20 19:31, Philippe Mathieu-Daudé wrote:
>> The FW_CFG_DATA_GENERATOR allows any object to produce
>> blob of data consumable by the fw_cfg device.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> v7: addressed Laszlo's comments
>> - fixed typos in description
>> - return size_t instead of ssize_t; 0 for error
>> - do not use 1-letter variable names
>> - do not open-code 'fw_cfg-data-generator'
>> - cast g_memdup() size argument as 'guint'
>> - improved documentation
>> ---
>> docs/specs/fw_cfg.txt | 9 ++++++-
>> include/hw/nvram/fw_cfg.h | 52 +++++++++++++++++++++++++++++++++++++++
>> hw/nvram/fw_cfg.c | 31 +++++++++++++++++++++++
>> 3 files changed, 91 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
>> index 8f1ebc66fa..bc16daa38a 100644
>> --- a/docs/specs/fw_cfg.txt
>> +++ b/docs/specs/fw_cfg.txt
>> @@ -219,7 +219,7 @@ To check the result, read the "control" field:
>>
>> = Externally Provided Items =
>>
>> -As of v2.4, "file" fw_cfg items (i.e., items with selector keys above
>> +Since v2.4, "file" fw_cfg items (i.e., items with selector keys above
>> FW_CFG_FILE_FIRST, and with a corresponding entry in the fw_cfg file
>> directory structure) may be inserted via the QEMU command line, using
>> the following syntax:
>> @@ -230,6 +230,13 @@ Or
>>
>> -fw_cfg [name=]<item_name>,string=<string>
>>
>> +Since v5.1, QEMU allows some objects to generate fw_cfg-specific content,
>> +the content is then associated with a "file" item using the 'gen_id' option
>> +in the command line, using the following syntax:
>> +
>> + -object <generator-type>,id=<generated_id>,[generator-specific-options] \
>> + -fw_cfg [name=]<item_name>,gen_id=<generated_id>
>> +
>> See QEMU man page for more documentation.
>>
>> Using item_name with plain ASCII characters only is recommended.
>
> I've looked at this hunk with a larger context, and I think it's really
> good.
>
>> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
>> index 25d9307018..8fbf2446c1 100644
>> --- a/include/hw/nvram/fw_cfg.h
>> +++ b/include/hw/nvram/fw_cfg.h
>> @@ -9,11 +9,43 @@
>> #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
>> + *
>> + * The returned pointer is a QObject weak reference, @obj owns
>> + * the reference and may free it at any time in the future.
>> + */
>> + 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 +295,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
>> + * @gen_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
>> + * @gen_id object. The data generated by the @gen_id object/ is copied
>
> (1) typo: "object/" (possibly a copy-paste error from my v6 review)
>
>> + * 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 device tree image on success, or 0 on errors.
>
> (2) typo (probably another copy-paste error):
>
> s/device tree image/generated item data/
>
>> + */
>> +size_t fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
>> + const char *gen_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..6d2fa13042 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -1032,6 +1032,31 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
>> return NULL;
>> }
>>
>> +size_t fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
>> + const char *gen_id, Error **errp)
>> +{
>> + FWCfgDataGeneratorClass *klass;
>> + Object *obj;
>> + size_t size;
>> +
>> + obj = object_resolve_path_component(object_get_objects_root(), gen_id);
>> + if (!obj) {
>> + error_setg(errp, "Cannot find object ID %s", gen_id);
>> + return 0;
>> + }
>> + if (!object_dynamic_cast(obj, TYPE_FW_CFG_DATA_GENERATOR_INTERFACE)) {
>> + error_setg(errp, "Object '%s' is not a '%s' subclass",
>> + TYPE_FW_CFG_DATA_GENERATOR_INTERFACE, gen_id);
>
> (3) the order of the last two arguments is wrong; please reverse it.
Oops...
>
>> + return 0;
>> + }
>> + klass = FW_CFG_DATA_GENERATOR_GET_CLASS(obj);
>> + size = klass->get_length(obj);
>
> (4) I think we should not call fw_cfg_add_file() if get_length() returns
> zero.
>
> AFAICS, fw_cfg_add_file() will happily create a useless zero-size fw_cfg
> entry, but then the same zero size that we return from
> fw_cfg_add_from_generator() will indicate failure to the caller (and we
> will also not have set "errp").
Good catch.
>
> I think we need something like:
>
> if (size == 0) {
> error_setg(errp, "Object '%s' failed to generate fw_cfg data",
> gen_id);
> return 0;
> }
>
> That will also prevent an awkward call to g_memdup() with size==0.
>
>
> Now, looking at patches #4 and #5, I think if no suitable cipher suite
> is found, then qcrypto_tls_cipher_suites_get_data() will return NULL,
> and qcrypto_tls_cipher_suites_get_length() will return zero. In turn, my
> suggestion (4) here will prevent QEMU from starting up.
>
> And I think that is just fine. The corresponding firmware function is
> SetCipherSuites() in
> "OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c", and if it sees a
> zero-sized "etc/edk2/https/ciphers" fw_cfg file, it will hang the
> firmware, *on purpose*. (See the reason explained in comments there.)
>
> Thus, it's much better if we prevent QEMU from starting up in the first
> place.
>
>> + fw_cfg_add_file(s, filename, g_memdup(klass->get_data(obj), (guint)size),
>> + size);
>> +
>> + return size;
>> +}
>> +
>> static void fw_cfg_machine_reset(void *opaque)
>> {
>> MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>> @@ -1333,12 +1358,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)
>>
>
> With (1) through (4) fixed:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Will fix, thanks!
>
> Thanks
> Laszlo
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v7 2/5] softmmu/vl: Let -fw_cfg option take a 'gen_id' argument
2020-05-28 17:31 [PATCH v7 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites Philippe Mathieu-Daudé
2020-05-28 17:31 ` [PATCH v7 1/5] hw/nvram/fw_cfg: Add the FW_CFG_DATA_GENERATOR interface Philippe Mathieu-Daudé
@ 2020-05-28 17:31 ` Philippe Mathieu-Daudé
2020-05-29 9:50 ` Laszlo Ersek
2020-05-28 17:31 ` [RFC PATCH v7 3/5] softmmu/vl: Allow -fw_cfg 'gen_id' option to use the 'etc/' namespace Philippe Mathieu-Daudé
` (2 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-28 17:31 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Philippe Mathieu-Daudé,
Daniel P. Berrangé,
Laszlo Ersek, Gerd Hoffmann
The 'gen_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>
---
v7:
- renamed 'blob_id' -> 'gen_id' (danpb)
- update comment in code (lersek)
- fixed CODING_STYLE (lersek)
- use Laszlo's if (SUM(options)) != 1 { error } form
---
softmmu/vl.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/softmmu/vl.c b/softmmu/vl.c
index ae5451bc23..cdb1d187ed 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -489,6 +489,11 @@ 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 = "gen_id",
+ .type = QEMU_OPT_STRING,
+ .help = "Sets id of the object generating the fw_cfg blob "
+ "to be inserted",
},
{ /* end of list */ }
},
@@ -2020,7 +2025,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, *gen_id;
FWCfgState *fw_cfg = (FWCfgState *) opaque;
if (fw_cfg == NULL) {
@@ -2030,14 +2035,13 @@ 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");
+ gen_id = qemu_opt_get(opts, "gen_id");
- /* we need name and either a file or the content string */
- if (!(nonempty_str(name) && (nonempty_str(file) || nonempty_str(str)))) {
- error_setg(errp, "invalid argument(s)");
- return -1;
- }
- if (nonempty_str(file) && nonempty_str(str)) {
- error_setg(errp, "file and string are mutually exclusive");
+ /* we need the name, and exactly one of: file, content string, gen_id */
+ if (!nonempty_str(name) ||
+ nonempty_str(file) + nonempty_str(str) + nonempty_str(gen_id) != 1) {
+ error_setg(errp, "name, plus exactly one of file,"
+ " string and gen_id, are needed");
return -1;
}
if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) {
@@ -2052,6 +2056,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(gen_id)) {
+ return fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
} else {
GError *err = NULL;
if (!g_file_get_contents(file, &buf, &size, &err)) {
--
2.21.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v7 2/5] softmmu/vl: Let -fw_cfg option take a 'gen_id' argument
2020-05-28 17:31 ` [PATCH v7 2/5] softmmu/vl: Let -fw_cfg option take a 'gen_id' argument Philippe Mathieu-Daudé
@ 2020-05-29 9:50 ` Laszlo Ersek
2020-06-09 14:12 ` Philippe Mathieu-Daudé
2020-06-09 15:50 ` Corey Minyard
0 siblings, 2 replies; 22+ messages in thread
From: Laszlo Ersek @ 2020-05-29 9:50 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel, Gerd Hoffmann, Corey Minyard
Cc: Paolo Bonzini, Daniel P. Berrangé
Gerd, Corey: there's a question for you near the end, please.
On 05/28/20 19:31, Philippe Mathieu-Daudé wrote:
> The 'gen_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>
> ---
> v7:
> - renamed 'blob_id' -> 'gen_id' (danpb)
> - update comment in code (lersek)
> - fixed CODING_STYLE (lersek)
> - use Laszlo's if (SUM(options)) != 1 { error } form
> ---
> softmmu/vl.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index ae5451bc23..cdb1d187ed 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -489,6 +489,11 @@ 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 = "gen_id",
> + .type = QEMU_OPT_STRING,
> + .help = "Sets id of the object generating the fw_cfg blob "
> + "to be inserted",
> },
> { /* end of list */ }
> },
> @@ -2020,7 +2025,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, *gen_id;
> FWCfgState *fw_cfg = (FWCfgState *) opaque;
>
> if (fw_cfg == NULL) {
> @@ -2030,14 +2035,13 @@ 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");
> + gen_id = qemu_opt_get(opts, "gen_id");
>
> - /* we need name and either a file or the content string */
> - if (!(nonempty_str(name) && (nonempty_str(file) || nonempty_str(str)))) {
> - error_setg(errp, "invalid argument(s)");
> - return -1;
> - }
> - if (nonempty_str(file) && nonempty_str(str)) {
> - error_setg(errp, "file and string are mutually exclusive");
> + /* we need the name, and exactly one of: file, content string, gen_id */
> + if (!nonempty_str(name) ||
> + nonempty_str(file) + nonempty_str(str) + nonempty_str(gen_id) != 1) {
(1) I believe the indentation of this line is not correct. I think it
should be out-dented by 2 spaces.
> + error_setg(errp, "name, plus exactly one of file,"
> + " string and gen_id, are needed");
> return -1;
> }
> if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) {
> @@ -2052,6 +2056,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(gen_id)) {
> + return fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
(2) This is no longer correct: fw_cfg_add_from_generator() now returns 0
on failure, but parse_fw_cfg() is supposed to return nonzer on failure.
See the comment on qemu_opts_foreach() -- "parse_fw_cfg" is passed as
the loop callback to qemu_opts_foreach().
Technically, we could simply write
return !fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
but that wouldn't be consistent with the -1 error codes returned
elsewhere from parse_fw_cfg(). So how about:
size_t fw_cfg_size;
fw_cfg_size = fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
return (fw_cfg_size > 0) ? 0 : -1;
I think your testing may have missed this because the problem is only
visible if you have *another* -fw_cfg option on the QEMU command line.
Returning the wrong status code from here terminates the
qemu_opts_foreach() loop, without attempting to set "error_fatal".
Therefore the loop is silently terminated, thus the only symptom would
be that -fw_cfg options beyond the "gen_id" one wouldn't take effect.
(3) I've noticed another *potential* issue, from looking at the larger
context. I apologize for missing it in v6.
See commit bab47d9a75a3 ("Sort the fw_cfg file list", 2016-04-07). (I'm
copying Corey; Gerd is already copied.) From that commit, we have, at
the end of this function:
/* For legacy, keep user files in a specific global order. */
fw_cfg_set_order_override(fw_cfg, FW_CFG_ORDER_OVERRIDE_USER);
fw_cfg_add_file(fw_cfg, name, buf, size);
fw_cfg_reset_order_override(fw_cfg);
This takes effect for "file" and "string", but not for "gen_id". Should
we apply it to "gen_id" as well? (Sorry, I really don't understand what
commit bab47d9a75a3 is about!)
*IF* we want to apply the same logic to "gen_id", then we should
*perhaps* do, on the "nonempty_str(gen_id)" branch:
size_t fw_cfg_size;
fw_cfg_set_order_override(fw_cfg, FW_CFG_ORDER_OVERRIDE_USER);
fw_cfg_size = fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
fw_cfg_reset_order_override(fw_cfg);
return (fw_cfg_size > 0) ? 0 : -1;
I think???
Or maybe even use FW_CFG_ORDER_OVERRIDE_DEVICE rather than
FW_CFG_ORDER_OVERRIDE_USER? I don't have the slightest clue.
(I guess if I understood what commit bab47d9a75a3 was about, I'd be less
in doubt now. But that commit only hints at "avoid[ing] any future
issues of moving the file creation" -- I don't know what those issues
were in the first place!)
With (1) optionally fixed, and (2) fixed, I'd be willing to R-b this
patch; but I'm really thrown off by (3).
Thanks,
Laszlo
> } else {
> GError *err = NULL;
> if (!g_file_get_contents(file, &buf, &size, &err)) {
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v7 2/5] softmmu/vl: Let -fw_cfg option take a 'gen_id' argument
2020-05-29 9:50 ` Laszlo Ersek
@ 2020-06-09 14:12 ` Philippe Mathieu-Daudé
2020-06-09 15:50 ` Corey Minyard
1 sibling, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-09 14:12 UTC (permalink / raw)
To: Laszlo Ersek, qemu-devel, Gerd Hoffmann, Corey Minyard
Cc: Paolo Bonzini, Daniel P. Berrangé
On 5/29/20 11:50 AM, Laszlo Ersek wrote:
> Gerd, Corey: there's a question for you near the end, please.
>
> On 05/28/20 19:31, Philippe Mathieu-Daudé wrote:
>> The 'gen_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>
>> ---
>> v7:
>> - renamed 'blob_id' -> 'gen_id' (danpb)
>> - update comment in code (lersek)
>> - fixed CODING_STYLE (lersek)
>> - use Laszlo's if (SUM(options)) != 1 { error } form
>> ---
>> softmmu/vl.c | 22 ++++++++++++++--------
>> 1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index ae5451bc23..cdb1d187ed 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -489,6 +489,11 @@ 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 = "gen_id",
>> + .type = QEMU_OPT_STRING,
>> + .help = "Sets id of the object generating the fw_cfg blob "
>> + "to be inserted",
>> },
>> { /* end of list */ }
>> },
>> @@ -2020,7 +2025,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, *gen_id;
>> FWCfgState *fw_cfg = (FWCfgState *) opaque;
>>
>> if (fw_cfg == NULL) {
>> @@ -2030,14 +2035,13 @@ 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");
>> + gen_id = qemu_opt_get(opts, "gen_id");
>>
>> - /* we need name and either a file or the content string */
>> - if (!(nonempty_str(name) && (nonempty_str(file) || nonempty_str(str)))) {
>> - error_setg(errp, "invalid argument(s)");
>> - return -1;
>> - }
>> - if (nonempty_str(file) && nonempty_str(str)) {
>> - error_setg(errp, "file and string are mutually exclusive");
>> + /* we need the name, and exactly one of: file, content string, gen_id */
>> + if (!nonempty_str(name) ||
>> + nonempty_str(file) + nonempty_str(str) + nonempty_str(gen_id) != 1) {
>
> (1) I believe the indentation of this line is not correct. I think it
> should be out-dented by 2 spaces.
>
>> + error_setg(errp, "name, plus exactly one of file,"
>> + " string and gen_id, are needed");
>> return -1;
>> }
>> if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) {
>> @@ -2052,6 +2056,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(gen_id)) {
>> + return fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
>
> (2) This is no longer correct: fw_cfg_add_from_generator() now returns 0
> on failure, but parse_fw_cfg() is supposed to return nonzer on failure.
> See the comment on qemu_opts_foreach() -- "parse_fw_cfg" is passed as
> the loop callback to qemu_opts_foreach().
>
> Technically, we could simply write
>
> return !fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
>
> but that wouldn't be consistent with the -1 error codes returned
> elsewhere from parse_fw_cfg(). So how about:
>
> size_t fw_cfg_size;
>
> fw_cfg_size = fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
> return (fw_cfg_size > 0) ? 0 : -1;
>
> I think your testing may have missed this because the problem is only
> visible if you have *another* -fw_cfg option on the QEMU command line.
> Returning the wrong status code from here terminates the
> qemu_opts_foreach() loop, without attempting to set "error_fatal".
> Therefore the loop is silently terminated, thus the only symptom would
> be that -fw_cfg options beyond the "gen_id" one wouldn't take effect.
>
>
> (3) I've noticed another *potential* issue, from looking at the larger
> context. I apologize for missing it in v6.
>
> See commit bab47d9a75a3 ("Sort the fw_cfg file list", 2016-04-07). (I'm
> copying Corey; Gerd is already copied.) From that commit, we have, at
> the end of this function:
>
> /* For legacy, keep user files in a specific global order. */
> fw_cfg_set_order_override(fw_cfg, FW_CFG_ORDER_OVERRIDE_USER);
> fw_cfg_add_file(fw_cfg, name, buf, size);
> fw_cfg_reset_order_override(fw_cfg);
>
> This takes effect for "file" and "string", but not for "gen_id". Should
> we apply it to "gen_id" as well? (Sorry, I really don't understand what
> commit bab47d9a75a3 is about!)
>
> *IF* we want to apply the same logic to "gen_id", then we should
> *perhaps* do, on the "nonempty_str(gen_id)" branch:
>
> size_t fw_cfg_size;
>
> fw_cfg_set_order_override(fw_cfg, FW_CFG_ORDER_OVERRIDE_USER);
> fw_cfg_size = fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
> fw_cfg_reset_order_override(fw_cfg);
> return (fw_cfg_size > 0) ? 0 : -1;
>
> I think???
>
> Or maybe even use FW_CFG_ORDER_OVERRIDE_DEVICE rather than
> FW_CFG_ORDER_OVERRIDE_USER? I don't have the slightest clue.
>
> (I guess if I understood what commit bab47d9a75a3 was about, I'd be less
> in doubt now. But that commit only hints at "avoid[ing] any future
> issues of moving the file creation" -- I don't know what those issues
> were in the first place!)
Since the filename is not listed in fw_cfg_order[], it falls to
the "unknown stuff at the end":
/* Stick unknown stuff at the end. */
error_report("warning: Unknown firmware file in legacy mode: %s\n",
name);
return FW_CFG_ORDER_OVERRIDE_LAST;
Which seems safe (we do not mess with firmware specific DEVICE/USER
entries).
Gerd?
>
> With (1) optionally fixed, and (2) fixed, I'd be willing to R-b this
> patch; but I'm really thrown off by (3).
I addressed (1) and (2), thanks for your review :)
>
> Thanks,
> Laszlo
>
>
>> } else {
>> GError *err = NULL;
>> if (!g_file_get_contents(file, &buf, &size, &err)) {
>>
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v7 2/5] softmmu/vl: Let -fw_cfg option take a 'gen_id' argument
2020-05-29 9:50 ` Laszlo Ersek
2020-06-09 14:12 ` Philippe Mathieu-Daudé
@ 2020-06-09 15:50 ` Corey Minyard
2020-06-11 11:31 ` Laszlo Ersek
1 sibling, 1 reply; 22+ messages in thread
From: Corey Minyard @ 2020-06-09 15:50 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Paolo Bonzini, Daniel P. Berrangé,
Philippe Mathieu-Daudé,
qemu-devel, Gerd Hoffmann
On Fri, May 29, 2020 at 11:50:24AM +0200, Laszlo Ersek wrote:
> Gerd, Corey: there's a question for you near the end, please.
>
> On 05/28/20 19:31, Philippe Mathieu-Daudé wrote:
snip...
>
>
> (3) I've noticed another *potential* issue, from looking at the larger
> context. I apologize for missing it in v6.
>
> See commit bab47d9a75a3 ("Sort the fw_cfg file list", 2016-04-07). (I'm
> copying Corey; Gerd is already copied.) From that commit, we have, at
> the end of this function:
>
> /* For legacy, keep user files in a specific global order. */
> fw_cfg_set_order_override(fw_cfg, FW_CFG_ORDER_OVERRIDE_USER);
> fw_cfg_add_file(fw_cfg, name, buf, size);
> fw_cfg_reset_order_override(fw_cfg);
>
> This takes effect for "file" and "string", but not for "gen_id". Should
> we apply it to "gen_id" as well? (Sorry, I really don't understand what
> commit bab47d9a75a3 is about!)
I can explain the rationale for that change, but I'm not sure of the
answer to your question. That changes makes sure that the fw_cfg data
remains exactly the same even on newer versions of qemu if the machine
is set the same. This way you can do migrations to newer qemu versions
and anything using fw_cfg won't get confused because the data changes.
The reason that change was so complex was preserving the order for
migrating from older versions.
This is only about migration. I'm not sure what gen_id is, but if it's
migrated, it better be future proof.
-corey
>
> *IF* we want to apply the same logic to "gen_id", then we should
> *perhaps* do, on the "nonempty_str(gen_id)" branch:
>
> size_t fw_cfg_size;
>
> fw_cfg_set_order_override(fw_cfg, FW_CFG_ORDER_OVERRIDE_USER);
> fw_cfg_size = fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
> fw_cfg_reset_order_override(fw_cfg);
> return (fw_cfg_size > 0) ? 0 : -1;
>
> I think???
>
> Or maybe even use FW_CFG_ORDER_OVERRIDE_DEVICE rather than
> FW_CFG_ORDER_OVERRIDE_USER? I don't have the slightest clue.
>
> (I guess if I understood what commit bab47d9a75a3 was about, I'd be less
> in doubt now. But that commit only hints at "avoid[ing] any future
> issues of moving the file creation" -- I don't know what those issues
> were in the first place!)
>
> With (1) optionally fixed, and (2) fixed, I'd be willing to R-b this
> patch; but I'm really thrown off by (3).
>
> Thanks,
> Laszlo
>
>
> > } else {
> > GError *err = NULL;
> > if (!g_file_get_contents(file, &buf, &size, &err)) {
> >
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v7 2/5] softmmu/vl: Let -fw_cfg option take a 'gen_id' argument
2020-06-09 15:50 ` Corey Minyard
@ 2020-06-11 11:31 ` Laszlo Ersek
2020-06-11 11:49 ` Philippe Mathieu-Daudé
2020-06-15 14:45 ` Gerd Hoffmann
0 siblings, 2 replies; 22+ messages in thread
From: Laszlo Ersek @ 2020-06-11 11:31 UTC (permalink / raw)
To: minyard
Cc: Paolo Bonzini, Daniel P. Berrangé,
Philippe Mathieu-Daudé,
qemu-devel, Gerd Hoffmann
On 06/09/20 17:50, Corey Minyard wrote:
> On Fri, May 29, 2020 at 11:50:24AM +0200, Laszlo Ersek wrote:
>> Gerd, Corey: there's a question for you near the end, please.
>>
>> On 05/28/20 19:31, Philippe Mathieu-Daudé wrote:
>
> snip...
>
>>
>>
>> (3) I've noticed another *potential* issue, from looking at the larger
>> context. I apologize for missing it in v6.
>>
>> See commit bab47d9a75a3 ("Sort the fw_cfg file list", 2016-04-07). (I'm
>> copying Corey; Gerd is already copied.) From that commit, we have, at
>> the end of this function:
>>
>> /* For legacy, keep user files in a specific global order. */
>> fw_cfg_set_order_override(fw_cfg, FW_CFG_ORDER_OVERRIDE_USER);
>> fw_cfg_add_file(fw_cfg, name, buf, size);
>> fw_cfg_reset_order_override(fw_cfg);
>>
>> This takes effect for "file" and "string", but not for "gen_id". Should
>> we apply it to "gen_id" as well? (Sorry, I really don't understand what
>> commit bab47d9a75a3 is about!)
>
> I can explain the rationale for that change, but I'm not sure of the
> answer to your question. That changes makes sure that the fw_cfg data
> remains exactly the same even on newer versions of qemu if the machine
> is set the same. This way you can do migrations to newer qemu versions
> and anything using fw_cfg won't get confused because the data changes.
>
> The reason that change was so complex was preserving the order for
> migrating from older versions.
>
> This is only about migration. I'm not sure what gen_id is, but if it's
> migrated, it better be future proof.
Whenever introducing a new fw_cfg file (*any* new named file), how do we
decide whether we need fw_cfg_set_order_override()?
Thanks
Laszlo
>
> -corey
>
>>
>> *IF* we want to apply the same logic to "gen_id", then we should
>> *perhaps* do, on the "nonempty_str(gen_id)" branch:
>>
>> size_t fw_cfg_size;
>>
>> fw_cfg_set_order_override(fw_cfg, FW_CFG_ORDER_OVERRIDE_USER);
>> fw_cfg_size = fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
>> fw_cfg_reset_order_override(fw_cfg);
>> return (fw_cfg_size > 0) ? 0 : -1;
>>
>> I think???
>>
>> Or maybe even use FW_CFG_ORDER_OVERRIDE_DEVICE rather than
>> FW_CFG_ORDER_OVERRIDE_USER? I don't have the slightest clue.
>>
>> (I guess if I understood what commit bab47d9a75a3 was about, I'd be less
>> in doubt now. But that commit only hints at "avoid[ing] any future
>> issues of moving the file creation" -- I don't know what those issues
>> were in the first place!)
>>
>> With (1) optionally fixed, and (2) fixed, I'd be willing to R-b this
>> patch; but I'm really thrown off by (3).
>>
>> Thanks,
>> Laszlo
>>
>>
>>> } else {
>>> GError *err = NULL;
>>> if (!g_file_get_contents(file, &buf, &size, &err)) {
>>>
>>
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v7 2/5] softmmu/vl: Let -fw_cfg option take a 'gen_id' argument
2020-06-11 11:31 ` Laszlo Ersek
@ 2020-06-11 11:49 ` Philippe Mathieu-Daudé
2020-06-11 17:54 ` Corey Minyard
2020-06-15 14:45 ` Gerd Hoffmann
1 sibling, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-11 11:49 UTC (permalink / raw)
To: Laszlo Ersek, minyard, Gerd Hoffmann
Cc: Paolo Bonzini, Daniel P. Berrangé, qemu-devel
On 6/11/20 1:31 PM, Laszlo Ersek wrote:
> On 06/09/20 17:50, Corey Minyard wrote:
>> On Fri, May 29, 2020 at 11:50:24AM +0200, Laszlo Ersek wrote:
>>> Gerd, Corey: there's a question for you near the end, please.
>>>
>>> On 05/28/20 19:31, Philippe Mathieu-Daudé wrote:
>>
>> snip...
>>
>>>
>>>
>>> (3) I've noticed another *potential* issue, from looking at the larger
>>> context. I apologize for missing it in v6.
>>>
>>> See commit bab47d9a75a3 ("Sort the fw_cfg file list", 2016-04-07). (I'm
>>> copying Corey; Gerd is already copied.) From that commit, we have, at
>>> the end of this function:
>>>
>>> /* For legacy, keep user files in a specific global order. */
>>> fw_cfg_set_order_override(fw_cfg, FW_CFG_ORDER_OVERRIDE_USER);
>>> fw_cfg_add_file(fw_cfg, name, buf, size);
>>> fw_cfg_reset_order_override(fw_cfg);
>>>
>>> This takes effect for "file" and "string", but not for "gen_id". Should
>>> we apply it to "gen_id" as well? (Sorry, I really don't understand what
>>> commit bab47d9a75a3 is about!)
>>
>> I can explain the rationale for that change, but I'm not sure of the
>> answer to your question. That changes makes sure that the fw_cfg data
>> remains exactly the same even on newer versions of qemu if the machine
>> is set the same. This way you can do migrations to newer qemu versions
>> and anything using fw_cfg won't get confused because the data changes.
>>
>> The reason that change was so complex was preserving the order for
>> migrating from older versions.
>>
>> This is only about migration. I'm not sure what gen_id is, but if it's
>> migrated, it better be future proof.
>
> Whenever introducing a new fw_cfg file (*any* new named file), how do we
> decide whether we need fw_cfg_set_order_override()?
Good idea to ask, so we can document the answer in the fw_cfg API doc.
>
> Thanks
> Laszlo
>
>
>>
>> -corey
>>
>>>
>>> *IF* we want to apply the same logic to "gen_id", then we should
>>> *perhaps* do, on the "nonempty_str(gen_id)" branch:
>>>
>>> size_t fw_cfg_size;
>>>
>>> fw_cfg_set_order_override(fw_cfg, FW_CFG_ORDER_OVERRIDE_USER);
>>> fw_cfg_size = fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
>>> fw_cfg_reset_order_override(fw_cfg);
>>> return (fw_cfg_size > 0) ? 0 : -1;
>>>
>>> I think???
>>>
>>> Or maybe even use FW_CFG_ORDER_OVERRIDE_DEVICE rather than
>>> FW_CFG_ORDER_OVERRIDE_USER? I don't have the slightest clue.
>>>
>>> (I guess if I understood what commit bab47d9a75a3 was about, I'd be less
>>> in doubt now. But that commit only hints at "avoid[ing] any future
>>> issues of moving the file creation" -- I don't know what those issues
>>> were in the first place!)
>>>
>>> With (1) optionally fixed, and (2) fixed, I'd be willing to R-b this
>>> patch; but I'm really thrown off by (3).
>>>
>>> Thanks,
>>> Laszlo
>>>
>>>
>>>> } else {
>>>> GError *err = NULL;
>>>> if (!g_file_get_contents(file, &buf, &size, &err)) {
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v7 2/5] softmmu/vl: Let -fw_cfg option take a 'gen_id' argument
2020-06-11 11:49 ` Philippe Mathieu-Daudé
@ 2020-06-11 17:54 ` Corey Minyard
0 siblings, 0 replies; 22+ messages in thread
From: Corey Minyard @ 2020-06-11 17:54 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Paolo Bonzini, Daniel P. Berrangé,
Laszlo Ersek, Gerd Hoffmann, qemu-devel
On Thu, Jun 11, 2020 at 01:49:31PM +0200, Philippe Mathieu-Daudé wrote:
> On 6/11/20 1:31 PM, Laszlo Ersek wrote:
> > On 06/09/20 17:50, Corey Minyard wrote:
> >> On Fri, May 29, 2020 at 11:50:24AM +0200, Laszlo Ersek wrote:
> >>> Gerd, Corey: there's a question for you near the end, please.
> >>>
> >>> On 05/28/20 19:31, Philippe Mathieu-Daudé wrote:
> >>
> >> snip...
> >>
> >>>
> >>>
> >>> (3) I've noticed another *potential* issue, from looking at the larger
> >>> context. I apologize for missing it in v6.
> >>>
> >>> See commit bab47d9a75a3 ("Sort the fw_cfg file list", 2016-04-07). (I'm
> >>> copying Corey; Gerd is already copied.) From that commit, we have, at
> >>> the end of this function:
> >>>
> >>> /* For legacy, keep user files in a specific global order. */
> >>> fw_cfg_set_order_override(fw_cfg, FW_CFG_ORDER_OVERRIDE_USER);
> >>> fw_cfg_add_file(fw_cfg, name, buf, size);
> >>> fw_cfg_reset_order_override(fw_cfg);
> >>>
> >>> This takes effect for "file" and "string", but not for "gen_id". Should
> >>> we apply it to "gen_id" as well? (Sorry, I really don't understand what
> >>> commit bab47d9a75a3 is about!)
> >>
> >> I can explain the rationale for that change, but I'm not sure of the
> >> answer to your question. That changes makes sure that the fw_cfg data
> >> remains exactly the same even on newer versions of qemu if the machine
> >> is set the same. This way you can do migrations to newer qemu versions
> >> and anything using fw_cfg won't get confused because the data changes.
> >>
> >> The reason that change was so complex was preserving the order for
> >> migrating from older versions.
> >>
> >> This is only about migration. I'm not sure what gen_id is, but if it's
> >> migrated, it better be future proof.
> >
> > Whenever introducing a new fw_cfg file (*any* new named file), how do we
> > decide whether we need fw_cfg_set_order_override()?
>
> Good idea to ask, so we can document the answer in the fw_cfg API doc.
fw_cfg_set_order_override() is only needed in cases where you are loading
data for devices (VGA, NICs, and other devices) and when loading a
user-specified file. So basically, anything that is not a named entry
in fw_config_order[]. If it has a name in fw_config_order[], then you
shouldn't use an override. Otherwise you should.
I'm not aware of anything that wouldn't fall into the existing cases,
so I don't see a reason to add a new call. Assuming that device
initialization order and such all stays the same, order should be
preserved, and I don't know of another category you would add. Am I
missing something?
-corey
>
> >
> > Thanks
> > Laszlo
> >
> >
> >>
> >> -corey
> >>
> >>>
> >>> *IF* we want to apply the same logic to "gen_id", then we should
> >>> *perhaps* do, on the "nonempty_str(gen_id)" branch:
> >>>
> >>> size_t fw_cfg_size;
> >>>
> >>> fw_cfg_set_order_override(fw_cfg, FW_CFG_ORDER_OVERRIDE_USER);
> >>> fw_cfg_size = fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
> >>> fw_cfg_reset_order_override(fw_cfg);
> >>> return (fw_cfg_size > 0) ? 0 : -1;
> >>>
> >>> I think???
> >>>
> >>> Or maybe even use FW_CFG_ORDER_OVERRIDE_DEVICE rather than
> >>> FW_CFG_ORDER_OVERRIDE_USER? I don't have the slightest clue.
> >>>
> >>> (I guess if I understood what commit bab47d9a75a3 was about, I'd be less
> >>> in doubt now. But that commit only hints at "avoid[ing] any future
> >>> issues of moving the file creation" -- I don't know what those issues
> >>> were in the first place!)
> >>>
> >>> With (1) optionally fixed, and (2) fixed, I'd be willing to R-b this
> >>> patch; but I'm really thrown off by (3).
> >>>
> >>> Thanks,
> >>> Laszlo
> >>>
> >>>
> >>>> } else {
> >>>> GError *err = NULL;
> >>>> if (!g_file_get_contents(file, &buf, &size, &err)) {
> >>>>
> >>>
> >>
> >
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/5] softmmu/vl: Let -fw_cfg option take a 'gen_id' argument
2020-06-11 11:31 ` Laszlo Ersek
2020-06-11 11:49 ` Philippe Mathieu-Daudé
@ 2020-06-15 14:45 ` Gerd Hoffmann
2020-06-15 15:02 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 22+ messages in thread
From: Gerd Hoffmann @ 2020-06-15 14:45 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Paolo Bonzini, Daniel P. Berrangé,
qemu-devel, minyard, Philippe Mathieu-Daudé
Hi,
> > I can explain the rationale for that change, but I'm not sure of the
> > answer to your question. That changes makes sure that the fw_cfg data
> > remains exactly the same even on newer versions of qemu if the machine
> > is set the same. This way you can do migrations to newer qemu versions
> > and anything using fw_cfg won't get confused because the data changes.
> >
> > The reason that change was so complex was preserving the order for
> > migrating from older versions.
> >
> > This is only about migration. I'm not sure what gen_id is, but if it's
> > migrated, it better be future proof.
>
> Whenever introducing a new fw_cfg file (*any* new named file), how do we
> decide whether we need fw_cfg_set_order_override()?
The whole point of the sorting is to make sure the fw_cfg directory
listing entry (FW_CFG_FILE_DIR) is stable and doesn't change underneath
the guest when it gets live-migrated.
That sorting was added in qemu 2.6, to make sure things don't chance by
accident in case the initialization order changes. Now you've got a
problem when you migrate from qemu 2.5 (+older) to qemu 2.6 (+newer),
because 2.5 has the entries in initialization order and 2.6 has the
entries in alphabetical order. To deal with that machine types for 2.5
& older keep the old sort order. This is the reason why
legacy_fw_cfg_order exists.
For new features and files you can completely ignore the whole legacy
sorting mess. cross-version live migration works only for features
supported by both qemu versions, therefore the legacy sorting is only
relevant for features & files already supported by qemu 2.5.
HTH,
Gerd
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/5] softmmu/vl: Let -fw_cfg option take a 'gen_id' argument
2020-06-15 14:45 ` Gerd Hoffmann
@ 2020-06-15 15:02 ` Philippe Mathieu-Daudé
2020-06-16 15:23 ` Laszlo Ersek
0 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-15 15:02 UTC (permalink / raw)
To: Gerd Hoffmann, Laszlo Ersek
Cc: Paolo Bonzini, Daniel P. Berrangé, qemu-devel, minyard
On 6/15/20 4:45 PM, Gerd Hoffmann wrote:
> Hi,
>
>>> I can explain the rationale for that change, but I'm not sure of the
>>> answer to your question. That changes makes sure that the fw_cfg data
>>> remains exactly the same even on newer versions of qemu if the machine
>>> is set the same. This way you can do migrations to newer qemu versions
>>> and anything using fw_cfg won't get confused because the data changes.
>>>
>>> The reason that change was so complex was preserving the order for
>>> migrating from older versions.
>>>
>>> This is only about migration. I'm not sure what gen_id is, but if it's
>>> migrated, it better be future proof.
>>
>> Whenever introducing a new fw_cfg file (*any* new named file), how do we
>> decide whether we need fw_cfg_set_order_override()?
>
> The whole point of the sorting is to make sure the fw_cfg directory
> listing entry (FW_CFG_FILE_DIR) is stable and doesn't change underneath
> the guest when it gets live-migrated.
>
> That sorting was added in qemu 2.6, to make sure things don't chance by
> accident in case the initialization order changes. Now you've got a
> problem when you migrate from qemu 2.5 (+older) to qemu 2.6 (+newer),
> because 2.5 has the entries in initialization order and 2.6 has the
> entries in alphabetical order. To deal with that machine types for 2.5
> & older keep the old sort order. This is the reason why
> legacy_fw_cfg_order exists.
>
> For new features and files you can completely ignore the whole legacy
> sorting mess. cross-version live migration works only for features
> supported by both qemu versions, therefore the legacy sorting is only
> relevant for features & files already supported by qemu 2.5.
Thanks you Gerd for the whole explanation. I added an entry to
my TODO list to document this, based on your comment (and Corey's).
I'll address it later, as you confirmed it doesn't impact this
series.
Regards,
Phil.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/5] softmmu/vl: Let -fw_cfg option take a 'gen_id' argument
2020-06-15 15:02 ` Philippe Mathieu-Daudé
@ 2020-06-16 15:23 ` Laszlo Ersek
0 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2020-06-16 15:23 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Gerd Hoffmann
Cc: Paolo Bonzini, Daniel P. Berrangé, qemu-devel, minyard
On 06/15/20 17:02, Philippe Mathieu-Daudé wrote:
> On 6/15/20 4:45 PM, Gerd Hoffmann wrote:
>> Hi,
>>
>>>> I can explain the rationale for that change, but I'm not sure of the
>>>> answer to your question. That changes makes sure that the fw_cfg data
>>>> remains exactly the same even on newer versions of qemu if the machine
>>>> is set the same. This way you can do migrations to newer qemu versions
>>>> and anything using fw_cfg won't get confused because the data changes.
>>>>
>>>> The reason that change was so complex was preserving the order for
>>>> migrating from older versions.
>>>>
>>>> This is only about migration. I'm not sure what gen_id is, but if it's
>>>> migrated, it better be future proof.
>>>
>>> Whenever introducing a new fw_cfg file (*any* new named file), how do we
>>> decide whether we need fw_cfg_set_order_override()?
>>
>> The whole point of the sorting is to make sure the fw_cfg directory
>> listing entry (FW_CFG_FILE_DIR) is stable and doesn't change underneath
>> the guest when it gets live-migrated.
>>
>> That sorting was added in qemu 2.6, to make sure things don't chance by
>> accident in case the initialization order changes. Now you've got a
>> problem when you migrate from qemu 2.5 (+older) to qemu 2.6 (+newer),
>> because 2.5 has the entries in initialization order and 2.6 has the
>> entries in alphabetical order. To deal with that machine types for 2.5
>> & older keep the old sort order. This is the reason why
>> legacy_fw_cfg_order exists.
>>
>> For new features and files you can completely ignore the whole legacy
>> sorting mess. cross-version live migration works only for features
>> supported by both qemu versions, therefore the legacy sorting is only
>> relevant for features & files already supported by qemu 2.5.
>
> Thanks you Gerd for the whole explanation. I added an entry to
> my TODO list to document this, based on your comment (and Corey's).
Yes, please!
Apparently, I've been confused by commit bab47d9a75a3 ("Sort the fw_cfg
file list", 2016-04-07) before (in January 2018):
http://mid.mail-archive.com/5367c8a4-91bd-7712-525d-0a1ed6e6acab@redhat.com
(See in particular my question which I believe remains relevant:
"is the idea that the same machine type on a new QEMU release may only
reorder the additions of the preexistent fw_cfg files across the source
code, but must not expose *new* fw_cfg files?"
And I think Gerd just answered that above (in the positive), namely,
"cross-version live migration works only for features supported by both
qemu versions". So indeed we must not have a new fw_cfg file appear in
an old machine type on a new QEMU release, without the user explicitly
asking for it on the command line.)
> I'll address it later, as you confirmed it doesn't impact this
> series.
That's my understanding too. Thanks for explaining, Gerd!
Laszlo
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC PATCH v7 3/5] softmmu/vl: Allow -fw_cfg 'gen_id' option to use the 'etc/' namespace
2020-05-28 17:31 [PATCH v7 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites Philippe Mathieu-Daudé
2020-05-28 17:31 ` [PATCH v7 1/5] hw/nvram/fw_cfg: Add the FW_CFG_DATA_GENERATOR interface Philippe Mathieu-Daudé
2020-05-28 17:31 ` [PATCH v7 2/5] softmmu/vl: Let -fw_cfg option take a 'gen_id' argument Philippe Mathieu-Daudé
@ 2020-05-28 17:31 ` Philippe Mathieu-Daudé
2020-05-29 10:10 ` Laszlo Ersek
2020-05-28 17:31 ` [PATCH v7 4/5] crypto: Add tls-cipher-suites object Philippe Mathieu-Daudé
2020-05-28 17:31 ` [PATCH v7 5/5] crypto/tls-cipher-suites: Produce fw_cfg consumable blob Philippe Mathieu-Daudé
4 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-28 17:31 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Philippe Mathieu-Daudé,
Daniel P. Berrangé,
Laszlo Ersek, Gerd Hoffmann
User-generated fw_cfg keys should be prefixed with "opt/".
However FW_CFG_DATA_GENERATOR keys are generated by QEMU,
so allow the "etc/" namespace in this specific case.
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v7: reword commit description and added comment in code
---
softmmu/vl.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/softmmu/vl.c b/softmmu/vl.c
index cdb1d187ed..d5423eaf2b 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2049,7 +2049,13 @@ 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(gen_id)) {
+ /*
+ * In this particular case where the content is populated
+ * internally, the "etc/" namespace protection is relaxed,
+ * so do not emit a warning.
+ */
+ } else if (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] 22+ messages in thread* Re: [RFC PATCH v7 3/5] softmmu/vl: Allow -fw_cfg 'gen_id' option to use the 'etc/' namespace
2020-05-28 17:31 ` [RFC PATCH v7 3/5] softmmu/vl: Allow -fw_cfg 'gen_id' option to use the 'etc/' namespace Philippe Mathieu-Daudé
@ 2020-05-29 10:10 ` Laszlo Ersek
0 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2020-05-29 10:10 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé, Gerd Hoffmann
On 05/28/20 19:31, Philippe Mathieu-Daudé wrote:
> User-generated fw_cfg keys should be prefixed with "opt/".
(1) Please formulate this as follows:
'Names of user-provided fw_cfg items are supposed to start with "opt/".'
(Because we're really not "prefixing keys".)
> However FW_CFG_DATA_GENERATOR keys are generated by QEMU,
(2) s/keys/items/
> so allow the "etc/" namespace in this specific case.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v7: reword commit description and added comment in code
> ---
> softmmu/vl.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index cdb1d187ed..d5423eaf2b 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2049,7 +2049,13 @@ 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(gen_id)) {
(3) I think this condition should be inverted. We'd like to suppress the
warning when "gen_id" is specified. In that case, nonempty_str(gen_id)
returns "true".
In other words, please drop the "!" operator.
> + /*
> + * In this particular case where the content is populated
> + * internally, the "etc/" namespace protection is relaxed,
> + * so do not emit a warning.
> + */
> + } else if (strncmp(name, "opt/", 4) != 0) {
> warn_report("externally provided fw_cfg item names "
> "should be prefixed with \"opt/\"");
> }
>
(4) I think having this in a separate patch is nice; I agree we should
do this. But I'd like to request a small update to
"docs/specs/fw_cfg.txt" as well, in the same patch.
Namely, where the document says:
"""
Use of names not beginning with "opt/" is potentially dangerous and
entirely unsupported. QEMU will warn if you try.
"""
Please append:
"""
Use of names not beginning with "opt/" is tolerated with 'gen_id' (that
is, the warning is suppressed), but you must know exactly what you're
doing.
"""
Because this highlights that the user (or the management tool) *actively
participates* in connecting the content generated by QEMU with the
fw_cfg filename expected by the firmware.
With (1) through (4) fixed:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v7 4/5] crypto: Add tls-cipher-suites object
2020-05-28 17:31 [PATCH v7 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites Philippe Mathieu-Daudé
` (2 preceding siblings ...)
2020-05-28 17:31 ` [RFC PATCH v7 3/5] softmmu/vl: Allow -fw_cfg 'gen_id' option to use the 'etc/' namespace Philippe Mathieu-Daudé
@ 2020-05-28 17:31 ` Philippe Mathieu-Daudé
2020-05-29 11:09 ` Laszlo Ersek
2020-05-28 17:31 ` [PATCH v7 5/5] crypto/tls-cipher-suites: Produce fw_cfg consumable blob Philippe Mathieu-Daudé
4 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-28 17:31 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Philippe Mathieu-Daudé,
Daniel P. Berrangé,
Laszlo Ersek, Gerd Hoffmann
Example of use to dump:
$ qemu-system-x86_64 -S \
-object tls-cipher-suites,id=mysuite,priority=@SYSTEM \
-trace qcrypto\*
1590664444.197123:qcrypto_tls_cipher_suite_priority priority: @SYSTEM
1590664444.197219:qcrypto_tls_cipher_suite_info data:[0x13, 0x02] version:TLS1.3 name:TLS_AES_256_GCM_SHA384
1590664444.197228:qcrypto_tls_cipher_suite_info data:[0x13, 0x03] version:TLS1.3 name:TLS_CHACHA20_POLY1305_SHA256
1590664444.197233:qcrypto_tls_cipher_suite_info data:[0x13, 0x01] version:TLS1.3 name:TLS_AES_128_GCM_SHA256
1590664444.197236:qcrypto_tls_cipher_suite_info data:[0x13, 0x04] version:TLS1.3 name:TLS_AES_128_CCM_SHA256
1590664444.197240:qcrypto_tls_cipher_suite_info data:[0xc0, 0x30] version:TLS1.2 name:TLS_ECDHE_RSA_AES_256_GCM_SHA384
1590664444.197245:qcrypto_tls_cipher_suite_info data:[0xcc, 0xa8] version:TLS1.2 name:TLS_ECDHE_RSA_CHACHA20_POLY1305
1590664444.197250:qcrypto_tls_cipher_suite_info data:[0xc0, 0x14] version:TLS1.0 name:TLS_ECDHE_RSA_AES_256_CBC_SHA1
1590664444.197254:qcrypto_tls_cipher_suite_info data:[0xc0, 0x2f] version:TLS1.2 name:TLS_ECDHE_RSA_AES_128_GCM_SHA256
1590664444.197258:qcrypto_tls_cipher_suite_info data:[0xc0, 0x13] version:TLS1.0 name:TLS_ECDHE_RSA_AES_128_CBC_SHA1
1590664444.197261:qcrypto_tls_cipher_suite_info data:[0xc0, 0x2c] version:TLS1.2 name:TLS_ECDHE_ECDSA_AES_256_GCM_SHA384
1590664444.197266:qcrypto_tls_cipher_suite_info data:[0xcc, 0xa9] version:TLS1.2 name:TLS_ECDHE_ECDSA_CHACHA20_POLY1305
1590664444.197270:qcrypto_tls_cipher_suite_info data:[0xc0, 0xad] version:TLS1.2 name:TLS_ECDHE_ECDSA_AES_256_CCM
1590664444.197274:qcrypto_tls_cipher_suite_info data:[0xc0, 0x0a] version:TLS1.0 name:TLS_ECDHE_ECDSA_AES_256_CBC_SHA1
1590664444.197278:qcrypto_tls_cipher_suite_info data:[0xc0, 0x2b] version:TLS1.2 name:TLS_ECDHE_ECDSA_AES_128_GCM_SHA256
1590664444.197283:qcrypto_tls_cipher_suite_info data:[0xc0, 0xac] version:TLS1.2 name:TLS_ECDHE_ECDSA_AES_128_CCM
1590664444.197287:qcrypto_tls_cipher_suite_info data:[0xc0, 0x09] version:TLS1.0 name:TLS_ECDHE_ECDSA_AES_128_CBC_SHA1
1590664444.197291:qcrypto_tls_cipher_suite_info data:[0x00, 0x9d] version:TLS1.2 name:TLS_RSA_AES_256_GCM_SHA384
1590664444.197296:qcrypto_tls_cipher_suite_info data:[0xc0, 0x9d] version:TLS1.2 name:TLS_RSA_AES_256_CCM
1590664444.197300:qcrypto_tls_cipher_suite_info data:[0x00, 0x35] version:TLS1.0 name:TLS_RSA_AES_256_CBC_SHA1
1590664444.197304:qcrypto_tls_cipher_suite_info data:[0x00, 0x9c] version:TLS1.2 name:TLS_RSA_AES_128_GCM_SHA256
1590664444.197308:qcrypto_tls_cipher_suite_info data:[0xc0, 0x9c] version:TLS1.2 name:TLS_RSA_AES_128_CCM
1590664444.197312:qcrypto_tls_cipher_suite_info data:[0x00, 0x2f] version:TLS1.0 name:TLS_RSA_AES_128_CBC_SHA1
1590664444.197316:qcrypto_tls_cipher_suite_info data:[0x00, 0x9f] version:TLS1.2 name:TLS_DHE_RSA_AES_256_GCM_SHA384
1590664444.197320:qcrypto_tls_cipher_suite_info data:[0xcc, 0xaa] version:TLS1.2 name:TLS_DHE_RSA_CHACHA20_POLY1305
1590664444.197325:qcrypto_tls_cipher_suite_info data:[0xc0, 0x9f] version:TLS1.2 name:TLS_DHE_RSA_AES_256_CCM
1590664444.197329:qcrypto_tls_cipher_suite_info data:[0x00, 0x39] version:TLS1.0 name:TLS_DHE_RSA_AES_256_CBC_SHA1
1590664444.197333:qcrypto_tls_cipher_suite_info data:[0x00, 0x9e] version:TLS1.2 name:TLS_DHE_RSA_AES_128_GCM_SHA256
1590664444.197337:qcrypto_tls_cipher_suite_info data:[0xc0, 0x9e] version:TLS1.2 name:TLS_DHE_RSA_AES_128_CCM
1590664444.197341:qcrypto_tls_cipher_suite_info data:[0x00, 0x33] version:TLS1.0 name:TLS_DHE_RSA_AES_128_CBC_SHA1
1590664444.197345:qcrypto_tls_cipher_suite_count count: 29
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v7:
- Use Laszlo's loop with enum mode (lersek)
- Convert debug printf to trace events (danpb)
- Use buildsys CONFIG_GNUTLS instead of C ifdef'ry (danpb)
---
include/crypto/tls-cipher-suites.h | 38 +++++++++
crypto/tls-cipher-suites.c | 127 +++++++++++++++++++++++++++++
crypto/Makefile.objs | 1 +
crypto/trace-events | 5 ++
4 files changed, 171 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..20a7c74edf
--- /dev/null
+++ b/include/crypto/tls-cipher-suites.h
@@ -0,0 +1,38 @@
+/*
+ * 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];
+} QEMU_PACKED IANA_TLS_CIPHER;
+
+typedef struct QCryptoTLSCipherSuites {
+ /* <private> */
+ QCryptoTLSCreds parent_obj;
+
+ /* <public> */
+ 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..f02a041f9a
--- /dev/null
+++ b/crypto/tls-cipher-suites.c
@@ -0,0 +1,127 @@
+/*
+ * 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"
+#include "trace.h"
+
+static void parse_cipher_suites(QCryptoTLSCipherSuites *s,
+ const char *priority_name, Error **errp)
+{
+ int ret;
+ const char *err;
+ gnutls_priority_t pcache;
+ enum { M_ENUMERATE, M_GENERATE, M_DONE } mode;
+
+ assert(priority_name);
+ trace_qcrypto_tls_cipher_suite_priority(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;
+ }
+
+ for (mode = M_ENUMERATE; mode < M_DONE; mode++) {
+ size_t i;
+
+ for (i = 0;; i++) {
+ int ret;
+ unsigned idx;
+ const char *name;
+ IANA_TLS_CIPHER cipher;
+ gnutls_protocol_t protocol;
+
+ 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, (unsigned char *)&cipher,
+ NULL, NULL, NULL, &protocol);
+ if (name == NULL) {
+ continue;
+ }
+
+ if (mode == M_GENERATE) {
+ const char *version;
+
+ version = gnutls_protocol_get_name(protocol);
+ trace_qcrypto_tls_cipher_suite_info(cipher.data[0],
+ cipher.data[1],
+ version, name);
+ s->cipher_list[s->cipher_count] = cipher;
+ }
+ s->cipher_count++;
+ }
+
+ if (mode == M_ENUMERATE) {
+ if (s->cipher_count == 0) {
+ break;
+ }
+ s->cipher_list = g_new(IANA_TLS_CIPHER, s->cipher_count);
+ s->cipher_count = 0;
+ }
+ }
+ trace_qcrypto_tls_cipher_suite_count(s->cipher_count);
+ gnutls_priority_deinit(pcache);
+}
+
+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_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;
+}
+
+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..1c1b5e21ff 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-$(CONFIG_GNUTLS) += tls-cipher-suites.o
crypto-obj-y += tlscreds.o
crypto-obj-y += tlscredsanon.o
crypto-obj-y += tlscredspsk.o
diff --git a/crypto/trace-events b/crypto/trace-events
index 9e594d30e8..c07a752b50 100644
--- a/crypto/trace-events
+++ b/crypto/trace-events
@@ -21,3 +21,8 @@ qcrypto_tls_creds_x509_load_cert_list(void *creds, const char *file) "TLS creds
# tlssession.c
qcrypto_tls_session_new(void *session, void *creds, const char *hostname, const char *authzid, int endpoint) "TLS session new session=%p creds=%p hostname=%s authzid=%s endpoint=%d"
qcrypto_tls_session_check_creds(void *session, const char *status) "TLS session check creds session=%p status=%s"
+
+# tls-cipher-suites.c
+qcrypto_tls_cipher_suite_priority(const char *name) "priority: %s"
+qcrypto_tls_cipher_suite_info(uint8_t data0, uint8_t data1, const char *version, const char *name) "data:[0x%02x, 0x%02x] version:%s name:%s"
+qcrypto_tls_cipher_suite_count(unsigned count) "count: %u"
--
2.21.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v7 4/5] crypto: Add tls-cipher-suites object
2020-05-28 17:31 ` [PATCH v7 4/5] crypto: Add tls-cipher-suites object Philippe Mathieu-Daudé
@ 2020-05-29 11:09 ` Laszlo Ersek
2020-05-29 11:17 ` Laszlo Ersek
2020-05-29 11:18 ` Daniel P. Berrangé
0 siblings, 2 replies; 22+ messages in thread
From: Laszlo Ersek @ 2020-05-29 11:09 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel, Daniel P. Berrangé
Cc: Paolo Bonzini, Gerd Hoffmann
On 05/28/20 19:31, Philippe Mathieu-Daudé wrote:
> Example of use to dump:
>
> $ qemu-system-x86_64 -S \
> -object tls-cipher-suites,id=mysuite,priority=@SYSTEM \
> -trace qcrypto\*
> 1590664444.197123:qcrypto_tls_cipher_suite_priority priority: @SYSTEM
> 1590664444.197219:qcrypto_tls_cipher_suite_info data:[0x13, 0x02] version:TLS1.3 name:TLS_AES_256_GCM_SHA384
> 1590664444.197228:qcrypto_tls_cipher_suite_info data:[0x13, 0x03] version:TLS1.3 name:TLS_CHACHA20_POLY1305_SHA256
> 1590664444.197233:qcrypto_tls_cipher_suite_info data:[0x13, 0x01] version:TLS1.3 name:TLS_AES_128_GCM_SHA256
> 1590664444.197236:qcrypto_tls_cipher_suite_info data:[0x13, 0x04] version:TLS1.3 name:TLS_AES_128_CCM_SHA256
> 1590664444.197240:qcrypto_tls_cipher_suite_info data:[0xc0, 0x30] version:TLS1.2 name:TLS_ECDHE_RSA_AES_256_GCM_SHA384
> 1590664444.197245:qcrypto_tls_cipher_suite_info data:[0xcc, 0xa8] version:TLS1.2 name:TLS_ECDHE_RSA_CHACHA20_POLY1305
> 1590664444.197250:qcrypto_tls_cipher_suite_info data:[0xc0, 0x14] version:TLS1.0 name:TLS_ECDHE_RSA_AES_256_CBC_SHA1
> 1590664444.197254:qcrypto_tls_cipher_suite_info data:[0xc0, 0x2f] version:TLS1.2 name:TLS_ECDHE_RSA_AES_128_GCM_SHA256
> 1590664444.197258:qcrypto_tls_cipher_suite_info data:[0xc0, 0x13] version:TLS1.0 name:TLS_ECDHE_RSA_AES_128_CBC_SHA1
> 1590664444.197261:qcrypto_tls_cipher_suite_info data:[0xc0, 0x2c] version:TLS1.2 name:TLS_ECDHE_ECDSA_AES_256_GCM_SHA384
> 1590664444.197266:qcrypto_tls_cipher_suite_info data:[0xcc, 0xa9] version:TLS1.2 name:TLS_ECDHE_ECDSA_CHACHA20_POLY1305
> 1590664444.197270:qcrypto_tls_cipher_suite_info data:[0xc0, 0xad] version:TLS1.2 name:TLS_ECDHE_ECDSA_AES_256_CCM
> 1590664444.197274:qcrypto_tls_cipher_suite_info data:[0xc0, 0x0a] version:TLS1.0 name:TLS_ECDHE_ECDSA_AES_256_CBC_SHA1
> 1590664444.197278:qcrypto_tls_cipher_suite_info data:[0xc0, 0x2b] version:TLS1.2 name:TLS_ECDHE_ECDSA_AES_128_GCM_SHA256
> 1590664444.197283:qcrypto_tls_cipher_suite_info data:[0xc0, 0xac] version:TLS1.2 name:TLS_ECDHE_ECDSA_AES_128_CCM
> 1590664444.197287:qcrypto_tls_cipher_suite_info data:[0xc0, 0x09] version:TLS1.0 name:TLS_ECDHE_ECDSA_AES_128_CBC_SHA1
> 1590664444.197291:qcrypto_tls_cipher_suite_info data:[0x00, 0x9d] version:TLS1.2 name:TLS_RSA_AES_256_GCM_SHA384
> 1590664444.197296:qcrypto_tls_cipher_suite_info data:[0xc0, 0x9d] version:TLS1.2 name:TLS_RSA_AES_256_CCM
> 1590664444.197300:qcrypto_tls_cipher_suite_info data:[0x00, 0x35] version:TLS1.0 name:TLS_RSA_AES_256_CBC_SHA1
> 1590664444.197304:qcrypto_tls_cipher_suite_info data:[0x00, 0x9c] version:TLS1.2 name:TLS_RSA_AES_128_GCM_SHA256
> 1590664444.197308:qcrypto_tls_cipher_suite_info data:[0xc0, 0x9c] version:TLS1.2 name:TLS_RSA_AES_128_CCM
> 1590664444.197312:qcrypto_tls_cipher_suite_info data:[0x00, 0x2f] version:TLS1.0 name:TLS_RSA_AES_128_CBC_SHA1
> 1590664444.197316:qcrypto_tls_cipher_suite_info data:[0x00, 0x9f] version:TLS1.2 name:TLS_DHE_RSA_AES_256_GCM_SHA384
> 1590664444.197320:qcrypto_tls_cipher_suite_info data:[0xcc, 0xaa] version:TLS1.2 name:TLS_DHE_RSA_CHACHA20_POLY1305
> 1590664444.197325:qcrypto_tls_cipher_suite_info data:[0xc0, 0x9f] version:TLS1.2 name:TLS_DHE_RSA_AES_256_CCM
> 1590664444.197329:qcrypto_tls_cipher_suite_info data:[0x00, 0x39] version:TLS1.0 name:TLS_DHE_RSA_AES_256_CBC_SHA1
> 1590664444.197333:qcrypto_tls_cipher_suite_info data:[0x00, 0x9e] version:TLS1.2 name:TLS_DHE_RSA_AES_128_GCM_SHA256
> 1590664444.197337:qcrypto_tls_cipher_suite_info data:[0xc0, 0x9e] version:TLS1.2 name:TLS_DHE_RSA_AES_128_CCM
> 1590664444.197341:qcrypto_tls_cipher_suite_info data:[0x00, 0x33] version:TLS1.0 name:TLS_DHE_RSA_AES_128_CBC_SHA1
> 1590664444.197345:qcrypto_tls_cipher_suite_count count: 29
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v7:
> - Use Laszlo's loop with enum mode (lersek)
Nice improvement with the enum, thanks!
> - Convert debug printf to trace events (danpb)
> - Use buildsys CONFIG_GNUTLS instead of C ifdef'ry (danpb)
> ---
> include/crypto/tls-cipher-suites.h | 38 +++++++++
> crypto/tls-cipher-suites.c | 127 +++++++++++++++++++++++++++++
> crypto/Makefile.objs | 1 +
> crypto/trace-events | 5 ++
> 4 files changed, 171 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..20a7c74edf
> --- /dev/null
> +++ b/include/crypto/tls-cipher-suites.h
> @@ -0,0 +1,38 @@
> +/*
> + * 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];
> +} QEMU_PACKED IANA_TLS_CIPHER;
> +
> +typedef struct QCryptoTLSCipherSuites {
> + /* <private> */
> + QCryptoTLSCreds parent_obj;
> +
> + /* <public> */
> + 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..f02a041f9a
> --- /dev/null
> +++ b/crypto/tls-cipher-suites.c
> @@ -0,0 +1,127 @@
> +/*
> + * 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"
> +#include "trace.h"
> +
> +static void parse_cipher_suites(QCryptoTLSCipherSuites *s,
> + const char *priority_name, Error **errp)
> +{
> + int ret;
> + const char *err;
> + gnutls_priority_t pcache;
> + enum { M_ENUMERATE, M_GENERATE, M_DONE } mode;
> +
> + assert(priority_name);
> + trace_qcrypto_tls_cipher_suite_priority(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;
> + }
> +
> + for (mode = M_ENUMERATE; mode < M_DONE; mode++) {
> + size_t i;
> +
> + for (i = 0;; i++) {
> + int ret;
> + unsigned idx;
> + const char *name;
> + IANA_TLS_CIPHER cipher;
> + gnutls_protocol_t protocol;
> +
> + 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, (unsigned char *)&cipher,
> + NULL, NULL, NULL, &protocol);
> + if (name == NULL) {
> + continue;
> + }
> +
> + if (mode == M_GENERATE) {
> + const char *version;
> +
> + version = gnutls_protocol_get_name(protocol);
> + trace_qcrypto_tls_cipher_suite_info(cipher.data[0],
> + cipher.data[1],
> + version, name);
> + s->cipher_list[s->cipher_count] = cipher;
> + }
> + s->cipher_count++;
> + }
> +
> + if (mode == M_ENUMERATE) {
> + if (s->cipher_count == 0) {
> + break;
> + }
> + s->cipher_list = g_new(IANA_TLS_CIPHER, s->cipher_count);
> + s->cipher_count = 0;
> + }
> + }
> + trace_qcrypto_tls_cipher_suite_count(s->cipher_count);
> + gnutls_priority_deinit(pcache);
> +}
> +
> +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_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;
> +}
> +
> +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..1c1b5e21ff 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-$(CONFIG_GNUTLS) += tls-cipher-suites.o
> crypto-obj-y += tlscreds.o
> crypto-obj-y += tlscredsanon.o
> crypto-obj-y += tlscredspsk.o
> diff --git a/crypto/trace-events b/crypto/trace-events
> index 9e594d30e8..c07a752b50 100644
> --- a/crypto/trace-events
> +++ b/crypto/trace-events
> @@ -21,3 +21,8 @@ qcrypto_tls_creds_x509_load_cert_list(void *creds, const char *file) "TLS creds
> # tlssession.c
> qcrypto_tls_session_new(void *session, void *creds, const char *hostname, const char *authzid, int endpoint) "TLS session new session=%p creds=%p hostname=%s authzid=%s endpoint=%d"
> qcrypto_tls_session_check_creds(void *session, const char *status) "TLS session check creds session=%p status=%s"
> +
> +# tls-cipher-suites.c
> +qcrypto_tls_cipher_suite_priority(const char *name) "priority: %s"
> +qcrypto_tls_cipher_suite_info(uint8_t data0, uint8_t data1, const char *version, const char *name) "data:[0x%02x, 0x%02x] version:%s name:%s"
> +qcrypto_tls_cipher_suite_count(unsigned count) "count: %u"
>
(1) It feels like we should insert one space character right after
"data:", and another space character right after "version:". I think
that makes things easier to read and possibly to parse. It also seems a
bit more idiomatic with the rest of the trace messages.
Anyway, I don't insist, up to you.
(2) We need an actual commit message for this patch. How about the
following -- I have liberally stolen and edited comments that Daniel
made earlier in the Red Hat Bugzilla:
---v--- ---v--- ---v--- ---v---
On the host OS, various aspects of TLS operation are configurable. In
particular it is possible for the sysadmin to control the TLS
cipher/protocol algorithms that applications are permitted to use.
* Any given crypto library has a built-in default priority list defined by
the distro maintainer of the libary package (or by upstream).
* The "crypto-policies" RPM (or equivalent host OS package) provides a
config file such as "/etc/crypto-policies/config", where the sysadmin
can set a high level (library-independent) policy.
The "update-crypto-policies --set" command (or equivalent) is used to
translate the global policy to individual library representations,
producing files such as "/etc/crypto-policies/back-ends/*.config". The
generated files, if present, are loaded by the various crypto libraries
to override their own built-in defaults.
For example, the GNUTLS library may read
"/etc/crypto-policies/back-ends/gnutls.config".
* A management application (or the QEMU user) may overide the system-wide
crypto-policies config via their own config, if they need to diverge
from the former.
Thus the priority order is "QEMU user config" > "crypto-policies system
config" > "library built-in config".
Introduce the "tls-cipher-suites" object for exposing the ordered list of
permitted TLS cipher suites from the host side to the firmware, via
fw_cfg. The list is represented as an array of IANA_TLS_CIPHER objects.
The firmware uses the IANA_TLS_CIPHER array for configuring guest-side
TLS, for example in UEFI HTTPS Boot.
The priority at which the host-side policy is retrieved is given by the
"priority" property of the new object type. For example,
"priority=@SYSTEM" may be used to refer to
"/etc/crypto-policies/back-ends/gnutls.config" (given that QEMU uses
GNUTLS).
---^--- ---^--- ---^--- ---^---
(3) I think I have now at least formed an idea about where we should
document -fw_cfg / "gen_id" in the *manual*.
The various -object types are already documented extensively; namely in
section "Generic object creation". Thus, I think we should document
"tls-cipher-suites" there -- near the already existent "-object tls-*"
ones.
I suggest including a manual update to that effect. I think we can mostly
copy the suggested commit message into the manual as well.
And then, we can include the new "-fw_cfg" command line option (with
"gen_id") *right there*. Consequently, we won't need to modify the
existent "-fw_cfg" documentation bits (about "file" and "string") under
section "Debug/Expert options".
Dan: please comment!
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v7 4/5] crypto: Add tls-cipher-suites object
2020-05-29 11:09 ` Laszlo Ersek
@ 2020-05-29 11:17 ` Laszlo Ersek
2020-05-29 11:18 ` Daniel P. Berrangé
1 sibling, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2020-05-29 11:17 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel, Daniel P. Berrangé
Cc: Paolo Bonzini, Gerd Hoffmann
On 05/29/20 13:09, Laszlo Ersek wrote:
> On 05/28/20 19:31, Philippe Mathieu-Daudé wrote:
>> Example of use to dump:
>>
>> $ qemu-system-x86_64 -S \
>> -object tls-cipher-suites,id=mysuite,priority=@SYSTEM \
>> -trace qcrypto\*
>> 1590664444.197123:qcrypto_tls_cipher_suite_priority priority: @SYSTEM
>> 1590664444.197219:qcrypto_tls_cipher_suite_info data:[0x13, 0x02] version:TLS1.3 name:TLS_AES_256_GCM_SHA384
>> 1590664444.197228:qcrypto_tls_cipher_suite_info data:[0x13, 0x03] version:TLS1.3 name:TLS_CHACHA20_POLY1305_SHA256
>> 1590664444.197233:qcrypto_tls_cipher_suite_info data:[0x13, 0x01] version:TLS1.3 name:TLS_AES_128_GCM_SHA256
>> 1590664444.197236:qcrypto_tls_cipher_suite_info data:[0x13, 0x04] version:TLS1.3 name:TLS_AES_128_CCM_SHA256
>> 1590664444.197240:qcrypto_tls_cipher_suite_info data:[0xc0, 0x30] version:TLS1.2 name:TLS_ECDHE_RSA_AES_256_GCM_SHA384
>> 1590664444.197245:qcrypto_tls_cipher_suite_info data:[0xcc, 0xa8] version:TLS1.2 name:TLS_ECDHE_RSA_CHACHA20_POLY1305
>> 1590664444.197250:qcrypto_tls_cipher_suite_info data:[0xc0, 0x14] version:TLS1.0 name:TLS_ECDHE_RSA_AES_256_CBC_SHA1
>> 1590664444.197254:qcrypto_tls_cipher_suite_info data:[0xc0, 0x2f] version:TLS1.2 name:TLS_ECDHE_RSA_AES_128_GCM_SHA256
>> 1590664444.197258:qcrypto_tls_cipher_suite_info data:[0xc0, 0x13] version:TLS1.0 name:TLS_ECDHE_RSA_AES_128_CBC_SHA1
>> 1590664444.197261:qcrypto_tls_cipher_suite_info data:[0xc0, 0x2c] version:TLS1.2 name:TLS_ECDHE_ECDSA_AES_256_GCM_SHA384
>> 1590664444.197266:qcrypto_tls_cipher_suite_info data:[0xcc, 0xa9] version:TLS1.2 name:TLS_ECDHE_ECDSA_CHACHA20_POLY1305
>> 1590664444.197270:qcrypto_tls_cipher_suite_info data:[0xc0, 0xad] version:TLS1.2 name:TLS_ECDHE_ECDSA_AES_256_CCM
>> 1590664444.197274:qcrypto_tls_cipher_suite_info data:[0xc0, 0x0a] version:TLS1.0 name:TLS_ECDHE_ECDSA_AES_256_CBC_SHA1
>> 1590664444.197278:qcrypto_tls_cipher_suite_info data:[0xc0, 0x2b] version:TLS1.2 name:TLS_ECDHE_ECDSA_AES_128_GCM_SHA256
>> 1590664444.197283:qcrypto_tls_cipher_suite_info data:[0xc0, 0xac] version:TLS1.2 name:TLS_ECDHE_ECDSA_AES_128_CCM
>> 1590664444.197287:qcrypto_tls_cipher_suite_info data:[0xc0, 0x09] version:TLS1.0 name:TLS_ECDHE_ECDSA_AES_128_CBC_SHA1
>> 1590664444.197291:qcrypto_tls_cipher_suite_info data:[0x00, 0x9d] version:TLS1.2 name:TLS_RSA_AES_256_GCM_SHA384
>> 1590664444.197296:qcrypto_tls_cipher_suite_info data:[0xc0, 0x9d] version:TLS1.2 name:TLS_RSA_AES_256_CCM
>> 1590664444.197300:qcrypto_tls_cipher_suite_info data:[0x00, 0x35] version:TLS1.0 name:TLS_RSA_AES_256_CBC_SHA1
>> 1590664444.197304:qcrypto_tls_cipher_suite_info data:[0x00, 0x9c] version:TLS1.2 name:TLS_RSA_AES_128_GCM_SHA256
>> 1590664444.197308:qcrypto_tls_cipher_suite_info data:[0xc0, 0x9c] version:TLS1.2 name:TLS_RSA_AES_128_CCM
>> 1590664444.197312:qcrypto_tls_cipher_suite_info data:[0x00, 0x2f] version:TLS1.0 name:TLS_RSA_AES_128_CBC_SHA1
>> 1590664444.197316:qcrypto_tls_cipher_suite_info data:[0x00, 0x9f] version:TLS1.2 name:TLS_DHE_RSA_AES_256_GCM_SHA384
>> 1590664444.197320:qcrypto_tls_cipher_suite_info data:[0xcc, 0xaa] version:TLS1.2 name:TLS_DHE_RSA_CHACHA20_POLY1305
>> 1590664444.197325:qcrypto_tls_cipher_suite_info data:[0xc0, 0x9f] version:TLS1.2 name:TLS_DHE_RSA_AES_256_CCM
>> 1590664444.197329:qcrypto_tls_cipher_suite_info data:[0x00, 0x39] version:TLS1.0 name:TLS_DHE_RSA_AES_256_CBC_SHA1
>> 1590664444.197333:qcrypto_tls_cipher_suite_info data:[0x00, 0x9e] version:TLS1.2 name:TLS_DHE_RSA_AES_128_GCM_SHA256
>> 1590664444.197337:qcrypto_tls_cipher_suite_info data:[0xc0, 0x9e] version:TLS1.2 name:TLS_DHE_RSA_AES_128_CCM
>> 1590664444.197341:qcrypto_tls_cipher_suite_info data:[0x00, 0x33] version:TLS1.0 name:TLS_DHE_RSA_AES_128_CBC_SHA1
>> 1590664444.197345:qcrypto_tls_cipher_suite_count count: 29
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> v7:
>> - Use Laszlo's loop with enum mode (lersek)
>
> Nice improvement with the enum, thanks!
>
>> - Convert debug printf to trace events (danpb)
>> - Use buildsys CONFIG_GNUTLS instead of C ifdef'ry (danpb)
>> ---
>> include/crypto/tls-cipher-suites.h | 38 +++++++++
>> crypto/tls-cipher-suites.c | 127 +++++++++++++++++++++++++++++
>> crypto/Makefile.objs | 1 +
>> crypto/trace-events | 5 ++
>> 4 files changed, 171 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..20a7c74edf
>> --- /dev/null
>> +++ b/include/crypto/tls-cipher-suites.h
>> @@ -0,0 +1,38 @@
>> +/*
>> + * 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];
>> +} QEMU_PACKED IANA_TLS_CIPHER;
>> +
>> +typedef struct QCryptoTLSCipherSuites {
>> + /* <private> */
>> + QCryptoTLSCreds parent_obj;
>> +
>> + /* <public> */
>> + 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..f02a041f9a
>> --- /dev/null
>> +++ b/crypto/tls-cipher-suites.c
>> @@ -0,0 +1,127 @@
>> +/*
>> + * 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"
>> +#include "trace.h"
>> +
>> +static void parse_cipher_suites(QCryptoTLSCipherSuites *s,
>> + const char *priority_name, Error **errp)
>> +{
>> + int ret;
>> + const char *err;
>> + gnutls_priority_t pcache;
>> + enum { M_ENUMERATE, M_GENERATE, M_DONE } mode;
>> +
>> + assert(priority_name);
>> + trace_qcrypto_tls_cipher_suite_priority(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;
>> + }
>> +
>> + for (mode = M_ENUMERATE; mode < M_DONE; mode++) {
>> + size_t i;
>> +
>> + for (i = 0;; i++) {
>> + int ret;
>> + unsigned idx;
>> + const char *name;
>> + IANA_TLS_CIPHER cipher;
>> + gnutls_protocol_t protocol;
>> +
>> + 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, (unsigned char *)&cipher,
>> + NULL, NULL, NULL, &protocol);
>> + if (name == NULL) {
>> + continue;
>> + }
>> +
>> + if (mode == M_GENERATE) {
>> + const char *version;
>> +
>> + version = gnutls_protocol_get_name(protocol);
>> + trace_qcrypto_tls_cipher_suite_info(cipher.data[0],
>> + cipher.data[1],
>> + version, name);
>> + s->cipher_list[s->cipher_count] = cipher;
>> + }
>> + s->cipher_count++;
>> + }
>> +
>> + if (mode == M_ENUMERATE) {
>> + if (s->cipher_count == 0) {
>> + break;
>> + }
>> + s->cipher_list = g_new(IANA_TLS_CIPHER, s->cipher_count);
>> + s->cipher_count = 0;
>> + }
>> + }
>> + trace_qcrypto_tls_cipher_suite_count(s->cipher_count);
>> + gnutls_priority_deinit(pcache);
>> +}
>> +
>> +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_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;
>> +}
>> +
>> +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..1c1b5e21ff 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-$(CONFIG_GNUTLS) += tls-cipher-suites.o
>> crypto-obj-y += tlscreds.o
>> crypto-obj-y += tlscredsanon.o
>> crypto-obj-y += tlscredspsk.o
>> diff --git a/crypto/trace-events b/crypto/trace-events
>> index 9e594d30e8..c07a752b50 100644
>> --- a/crypto/trace-events
>> +++ b/crypto/trace-events
>> @@ -21,3 +21,8 @@ qcrypto_tls_creds_x509_load_cert_list(void *creds, const char *file) "TLS creds
>> # tlssession.c
>> qcrypto_tls_session_new(void *session, void *creds, const char *hostname, const char *authzid, int endpoint) "TLS session new session=%p creds=%p hostname=%s authzid=%s endpoint=%d"
>> qcrypto_tls_session_check_creds(void *session, const char *status) "TLS session check creds session=%p status=%s"
>> +
>> +# tls-cipher-suites.c
>> +qcrypto_tls_cipher_suite_priority(const char *name) "priority: %s"
>> +qcrypto_tls_cipher_suite_info(uint8_t data0, uint8_t data1, const char *version, const char *name) "data:[0x%02x, 0x%02x] version:%s name:%s"
>> +qcrypto_tls_cipher_suite_count(unsigned count) "count: %u"
>>
>
> (1) It feels like we should insert one space character right after
> "data:", and another space character right after "version:". I think
> that makes things easier to read and possibly to parse. It also seems a
> bit more idiomatic with the rest of the trace messages.
>
> Anyway, I don't insist, up to you.
>
> (2) We need an actual commit message for this patch. How about the
> following -- I have liberally stolen and edited comments that Daniel
> made earlier in the Red Hat Bugzilla:
>
> ---v--- ---v--- ---v--- ---v---
> On the host OS, various aspects of TLS operation are configurable. In
> particular it is possible for the sysadmin to control the TLS
> cipher/protocol algorithms that applications are permitted to use.
>
> * Any given crypto library has a built-in default priority list defined by
> the distro maintainer of the libary package (or by upstream).
>
> * The "crypto-policies" RPM (or equivalent host OS package) provides a
> config file such as "/etc/crypto-policies/config", where the sysadmin
> can set a high level (library-independent) policy.
>
> The "update-crypto-policies --set" command (or equivalent) is used to
> translate the global policy to individual library representations,
> producing files such as "/etc/crypto-policies/back-ends/*.config". The
> generated files, if present, are loaded by the various crypto libraries
> to override their own built-in defaults.
>
> For example, the GNUTLS library may read
> "/etc/crypto-policies/back-ends/gnutls.config".
>
> * A management application (or the QEMU user) may overide the system-wide
> crypto-policies config via their own config, if they need to diverge
> from the former.
>
> Thus the priority order is "QEMU user config" > "crypto-policies system
> config" > "library built-in config".
>
> Introduce the "tls-cipher-suites" object for exposing the ordered list of
> permitted TLS cipher suites from the host side to the firmware, via
*guest* firmware -- sorry about the omission!
> fw_cfg. The list is represented as an array of IANA_TLS_CIPHER objects.
> The firmware uses the IANA_TLS_CIPHER array for configuring guest-side
Ditto.
Thanks,
Laszlo
> TLS, for example in UEFI HTTPS Boot.
>
> The priority at which the host-side policy is retrieved is given by the
> "priority" property of the new object type. For example,
> "priority=@SYSTEM" may be used to refer to
> "/etc/crypto-policies/back-ends/gnutls.config" (given that QEMU uses
> GNUTLS).
> ---^--- ---^--- ---^--- ---^---
>
> (3) I think I have now at least formed an idea about where we should
> document -fw_cfg / "gen_id" in the *manual*.
>
> The various -object types are already documented extensively; namely in
> section "Generic object creation". Thus, I think we should document
> "tls-cipher-suites" there -- near the already existent "-object tls-*"
> ones.
>
> I suggest including a manual update to that effect. I think we can mostly
> copy the suggested commit message into the manual as well.
>
> And then, we can include the new "-fw_cfg" command line option (with
> "gen_id") *right there*. Consequently, we won't need to modify the
> existent "-fw_cfg" documentation bits (about "file" and "string") under
> section "Debug/Expert options".
>
> Dan: please comment!
>
> Thanks,
> Laszlo
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v7 4/5] crypto: Add tls-cipher-suites object
2020-05-29 11:09 ` Laszlo Ersek
2020-05-29 11:17 ` Laszlo Ersek
@ 2020-05-29 11:18 ` Daniel P. Berrangé
2020-05-29 12:08 ` Laszlo Ersek
1 sibling, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2020-05-29 11:18 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Paolo Bonzini, Philippe Mathieu-Daudé, qemu-devel, Gerd Hoffmann
On Fri, May 29, 2020 at 01:09:22PM +0200, Laszlo Ersek wrote:
> On 05/28/20 19:31, Philippe Mathieu-Daudé wrote:
> (2) We need an actual commit message for this patch. How about the
> following -- I have liberally stolen and edited comments that Daniel
> made earlier in the Red Hat Bugzilla:
>
> ---v--- ---v--- ---v--- ---v---
> On the host OS, various aspects of TLS operation are configurable. In
> particular it is possible for the sysadmin to control the TLS
> cipher/protocol algorithms that applications are permitted to use.
>
> * Any given crypto library has a built-in default priority list defined by
> the distro maintainer of the libary package (or by upstream).
>
> * The "crypto-policies" RPM (or equivalent host OS package) provides a
> config file such as "/etc/crypto-policies/config", where the sysadmin
> can set a high level (library-independent) policy.
>
> The "update-crypto-policies --set" command (or equivalent) is used to
> translate the global policy to individual library representations,
> producing files such as "/etc/crypto-policies/back-ends/*.config". The
> generated files, if present, are loaded by the various crypto libraries
> to override their own built-in defaults.
>
> For example, the GNUTLS library may read
> "/etc/crypto-policies/back-ends/gnutls.config".
>
> * A management application (or the QEMU user) may overide the system-wide
> crypto-policies config via their own config, if they need to diverge
> from the former.
>
> Thus the priority order is "QEMU user config" > "crypto-policies system
> config" > "library built-in config".
>
> Introduce the "tls-cipher-suites" object for exposing the ordered list of
> permitted TLS cipher suites from the host side to the firmware, via
> fw_cfg. The list is represented as an array of IANA_TLS_CIPHER objects.
> The firmware uses the IANA_TLS_CIPHER array for configuring guest-side
> TLS, for example in UEFI HTTPS Boot.
>
> The priority at which the host-side policy is retrieved is given by the
> "priority" property of the new object type. For example,
> "priority=@SYSTEM" may be used to refer to
> "/etc/crypto-policies/back-ends/gnutls.config" (given that QEMU uses
> GNUTLS).
> ---^--- ---^--- ---^--- ---^---
>
> (3) I think I have now at least formed an idea about where we should
> document -fw_cfg / "gen_id" in the *manual*.
>
> The various -object types are already documented extensively; namely in
> section "Generic object creation". Thus, I think we should document
> "tls-cipher-suites" there -- near the already existent "-object tls-*"
> ones.
>
> I suggest including a manual update to that effect. I think we can mostly
> copy the suggested commit message into the manual as well.
>
> And then, we can include the new "-fw_cfg" command line option (with
> "gen_id") *right there*. Consequently, we won't need to modify the
> existent "-fw_cfg" documentation bits (about "file" and "string") under
> section "Debug/Expert options".
>
> Dan: please comment!
I don't really have anything else to say. More docs == better
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] 22+ messages in thread
* Re: [PATCH v7 4/5] crypto: Add tls-cipher-suites object
2020-05-29 11:18 ` Daniel P. Berrangé
@ 2020-05-29 12:08 ` Laszlo Ersek
0 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2020-05-29 12:08 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Paolo Bonzini, Philippe Mathieu-Daudé, qemu-devel, Gerd Hoffmann
On 05/29/20 13:18, Daniel P. Berrangé wrote:
> On Fri, May 29, 2020 at 01:09:22PM +0200, Laszlo Ersek wrote:
>> On 05/28/20 19:31, Philippe Mathieu-Daudé wrote:
>> (2) We need an actual commit message for this patch. How about the
>> following -- I have liberally stolen and edited comments that Daniel
>> made earlier in the Red Hat Bugzilla:
>>
>> ---v--- ---v--- ---v--- ---v---
>> On the host OS, various aspects of TLS operation are configurable. In
>> particular it is possible for the sysadmin to control the TLS
>> cipher/protocol algorithms that applications are permitted to use.
>>
>> * Any given crypto library has a built-in default priority list defined by
>> the distro maintainer of the libary package (or by upstream).
>>
>> * The "crypto-policies" RPM (or equivalent host OS package) provides a
>> config file such as "/etc/crypto-policies/config", where the sysadmin
>> can set a high level (library-independent) policy.
>>
>> The "update-crypto-policies --set" command (or equivalent) is used to
>> translate the global policy to individual library representations,
>> producing files such as "/etc/crypto-policies/back-ends/*.config". The
>> generated files, if present, are loaded by the various crypto libraries
>> to override their own built-in defaults.
>>
>> For example, the GNUTLS library may read
>> "/etc/crypto-policies/back-ends/gnutls.config".
>>
>> * A management application (or the QEMU user) may overide the system-wide
>> crypto-policies config via their own config, if they need to diverge
>> from the former.
>>
>> Thus the priority order is "QEMU user config" > "crypto-policies system
>> config" > "library built-in config".
>>
>> Introduce the "tls-cipher-suites" object for exposing the ordered list of
>> permitted TLS cipher suites from the host side to the firmware, via
>> fw_cfg. The list is represented as an array of IANA_TLS_CIPHER objects.
>> The firmware uses the IANA_TLS_CIPHER array for configuring guest-side
>> TLS, for example in UEFI HTTPS Boot.
>>
>> The priority at which the host-side policy is retrieved is given by the
>> "priority" property of the new object type. For example,
>> "priority=@SYSTEM" may be used to refer to
>> "/etc/crypto-policies/back-ends/gnutls.config" (given that QEMU uses
>> GNUTLS).
>> ---^--- ---^--- ---^--- ---^---
>>
>> (3) I think I have now at least formed an idea about where we should
>> document -fw_cfg / "gen_id" in the *manual*.
>>
>> The various -object types are already documented extensively; namely in
>> section "Generic object creation". Thus, I think we should document
>> "tls-cipher-suites" there -- near the already existent "-object tls-*"
>> ones.
>>
>> I suggest including a manual update to that effect. I think we can mostly
>> copy the suggested commit message into the manual as well.
>>
>> And then, we can include the new "-fw_cfg" command line option (with
>> "gen_id") *right there*. Consequently, we won't need to modify the
>> existent "-fw_cfg" documentation bits (about "file" and "string") under
>> section "Debug/Expert options".
>>
>> Dan: please comment!
>
> I don't really have anything else to say. More docs == better
Thanks! Just wanted to be sure I wasn't suggesting something egregious.
Laszlo
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v7 5/5] crypto/tls-cipher-suites: Produce fw_cfg consumable blob
2020-05-28 17:31 [PATCH v7 0/5] fw_cfg: Add FW_CFG_DATA_GENERATOR; crypto: Add tls-cipher-suites Philippe Mathieu-Daudé
` (3 preceding siblings ...)
2020-05-28 17:31 ` [PATCH v7 4/5] crypto: Add tls-cipher-suites object Philippe Mathieu-Daudé
@ 2020-05-28 17:31 ` Philippe Mathieu-Daudé
4 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-28 17:31 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Philippe Mathieu-Daudé,
Daniel P. Berrangé,
Laszlo Ersek, Gerd Hoffmann
Since our format is consumable by the fw_cfg device,
we can implement the FW_CFG_DATA_GENERATOR interface.
Acked-by: Laszlo Ersek <lersek@redhat.com>
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 f02a041f9a..d6ea0ed190 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"
#include "trace.h"
static void parse_cipher_suites(QCryptoTLSCipherSuites *s,
@@ -99,11 +100,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;
}
static const TypeInfo qcrypto_tls_cipher_suites_info = {
@@ -115,6 +133,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] 22+ messages in thread