Re: [PATCH v2 07/20] smbios: avoid mangling user provided tables
> On 08-Mar-2024, at 22:49, Igor Mammedov wrote:
>
> On Thu, 7 Mar 2024 09:33:17 +0530
> Ani Sinha wrote:
>
>>> On 06-Mar-2024, at 12:11, Ani Sinha wrote:
>>>
>>>
>>>
>>> On Tue, 5 Mar 2024, Igor Mammedov wrote:
>>>
currently smbios_entry_add() preserves internally '-smbios type='
options but tables provided with '-smbios file=' are stored directly
into blob that eventually will be exposed to VM. And then later
QEMU adds default/'-smbios type' entries on top into the same blob.
It makes impossible to generate tables more than once, hence
'immutable' guard was used.
Make it possible to regenerate final blob by storing user provided
blobs into a dedicated area (usr_blobs) and then copy it when
composing final blob. Which also makes handling of -smbios
options consistent.
As side effect of this and previous commits there is no need to
generate legacy smbios_entries at the time options are parsed.
Instead compose smbios_entries on demand from usr_blobs like
it is done for non-legacy SMBIOS tables.
Signed-off-by: Igor Mammedov
Tested-by: Fiona Ebner
>>>
>>> Reviewed-by: Ani Sinha
>>>
---
hw/smbios/smbios.c | 179 +++--
1 file changed, 92 insertions(+), 87 deletions(-)
diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index c46fc93357..aa2cc5bdbd 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -57,6 +57,14 @@ static size_t smbios_entries_len;
static bool smbios_uuid_encoded = true;
/* end: legacy structures & constants for <= 2.0 machines */
+/*
+ * SMBIOS tables provided by user with '-smbios file=' option
+ */
+uint8_t *usr_blobs;
+size_t usr_blobs_len;
+static GArray *usr_blobs_sizes;
+static unsigned usr_table_max;
+static unsigned usr_table_cnt;
uint8_t *smbios_tables;
size_t smbios_tables_len;
@@ -67,7 +75,6 @@ static SmbiosEntryPointType smbios_ep_type =
SMBIOS_ENTRY_POINT_TYPE_32;
static SmbiosEntryPoint ep;
static int smbios_type4_count = 0;
-static bool smbios_immutable;
static bool smbios_have_defaults;
static uint32_t smbios_cpuid_version, smbios_cpuid_features;
@@ -569,9 +576,8 @@ static void smbios_build_type_1_fields(void)
uint8_t *smbios_get_table_legacy(uint32_t expected_t4_count, size_t
*length)
{
-/* drop unwanted version of command-line file blob(s) */
-g_free(smbios_tables);
-smbios_tables = NULL;
+int i;
+size_t usr_offset;
/* also complain if fields were given for types > 1 */
if (find_next_bit(have_fields_bitmap,
@@ -581,12 +587,33 @@ uint8_t *smbios_get_table_legacy(uint32_t
expected_t4_count, size_t *length)
exit(1);
}
-if (!smbios_immutable) {
-smbios_build_type_0_fields();
-smbios_build_type_1_fields();
-smbios_validate_table(expected_t4_count);
-smbios_immutable = true;
+g_free(smbios_entries);
+smbios_entries_len = sizeof(uint16_t);
+smbios_entries = g_malloc0(smbios_entries_len);
+
+for (i = 0, usr_offset = 0; usr_blobs_sizes && i <
usr_blobs_sizes->len;
+ i++)
+{
+struct smbios_table *table;
+struct smbios_structure_header *header;
+size_t size = g_array_index(usr_blobs_sizes, size_t, i);
+
+header = (struct smbios_structure_header *)(usr_blobs +
usr_offset);
+smbios_entries = g_realloc(smbios_entries, smbios_entries_len +
+ size + sizeof(*table));
+table = (struct smbios_table *)(smbios_entries +
smbios_entries_len);
+table->header.type = SMBIOS_TABLE_ENTRY;
+table->header.length = cpu_to_le16(sizeof(*table) + size);
+memcpy(table->data, header, size);
+smbios_entries_len += sizeof(*table) + size;
+(*(uint16_t *)smbios_entries) =
+cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1);
>>>
>>> I know this comes from existing code but can you please explain why we add
>>> 1 to it? This is confusing and a comment here would be nice.
>>>
+usr_offset += size;
>>>
>>> It would be better if we could add a comment here describing a bit what
>>> this is all about.
>>>
>>> user blobs are an array of smbios_structure_header entries whereas legacy
>>> tables are an array of smbios_table structures where
>>> smbios_table->data represents the a single user provided table blob in
>>> smbios_structure_header.
>>
>> Igor, are you going to send a v3 for this with the comments added?
>
> I can add comments as a patch on top of series,
> though I'd rather
Re: [PATCH v2 07/20] smbios: avoid mangling user provided tables
On Thu, 7 Mar 2024 09:33:17 +0530
Ani Sinha wrote:
> > On 06-Mar-2024, at 12:11, Ani Sinha wrote:
> >
> >
> >
> > On Tue, 5 Mar 2024, Igor Mammedov wrote:
> >
> >> currently smbios_entry_add() preserves internally '-smbios type='
> >> options but tables provided with '-smbios file=' are stored directly
> >> into blob that eventually will be exposed to VM. And then later
> >> QEMU adds default/'-smbios type' entries on top into the same blob.
> >>
> >> It makes impossible to generate tables more than once, hence
> >> 'immutable' guard was used.
> >> Make it possible to regenerate final blob by storing user provided
> >> blobs into a dedicated area (usr_blobs) and then copy it when
> >> composing final blob. Which also makes handling of -smbios
> >> options consistent.
> >>
> >> As side effect of this and previous commits there is no need to
> >> generate legacy smbios_entries at the time options are parsed.
> >> Instead compose smbios_entries on demand from usr_blobs like
> >> it is done for non-legacy SMBIOS tables.
> >>
> >> Signed-off-by: Igor Mammedov
> >> Tested-by: Fiona Ebner
> >
> > Reviewed-by: Ani Sinha
> >
> >> ---
> >> hw/smbios/smbios.c | 179 +++--
> >> 1 file changed, 92 insertions(+), 87 deletions(-)
> >>
> >> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> >> index c46fc93357..aa2cc5bdbd 100644
> >> --- a/hw/smbios/smbios.c
> >> +++ b/hw/smbios/smbios.c
> >> @@ -57,6 +57,14 @@ static size_t smbios_entries_len;
> >> static bool smbios_uuid_encoded = true;
> >> /* end: legacy structures & constants for <= 2.0 machines */
> >>
> >> +/*
> >> + * SMBIOS tables provided by user with '-smbios file=' option
> >> + */
> >> +uint8_t *usr_blobs;
> >> +size_t usr_blobs_len;
> >> +static GArray *usr_blobs_sizes;
> >> +static unsigned usr_table_max;
> >> +static unsigned usr_table_cnt;
> >>
> >> uint8_t *smbios_tables;
> >> size_t smbios_tables_len;
> >> @@ -67,7 +75,6 @@ static SmbiosEntryPointType smbios_ep_type =
> >> SMBIOS_ENTRY_POINT_TYPE_32;
> >> static SmbiosEntryPoint ep;
> >>
> >> static int smbios_type4_count = 0;
> >> -static bool smbios_immutable;
> >> static bool smbios_have_defaults;
> >> static uint32_t smbios_cpuid_version, smbios_cpuid_features;
> >>
> >> @@ -569,9 +576,8 @@ static void smbios_build_type_1_fields(void)
> >>
> >> uint8_t *smbios_get_table_legacy(uint32_t expected_t4_count, size_t
> >> *length)
> >> {
> >> -/* drop unwanted version of command-line file blob(s) */
> >> -g_free(smbios_tables);
> >> -smbios_tables = NULL;
> >> +int i;
> >> +size_t usr_offset;
> >>
> >> /* also complain if fields were given for types > 1 */
> >> if (find_next_bit(have_fields_bitmap,
> >> @@ -581,12 +587,33 @@ uint8_t *smbios_get_table_legacy(uint32_t
> >> expected_t4_count, size_t *length)
> >> exit(1);
> >> }
> >>
> >> -if (!smbios_immutable) {
> >> -smbios_build_type_0_fields();
> >> -smbios_build_type_1_fields();
> >> -smbios_validate_table(expected_t4_count);
> >> -smbios_immutable = true;
> >> +g_free(smbios_entries);
> >> +smbios_entries_len = sizeof(uint16_t);
> >> +smbios_entries = g_malloc0(smbios_entries_len);
> >> +
> >> +for (i = 0, usr_offset = 0; usr_blobs_sizes && i <
> >> usr_blobs_sizes->len;
> >> + i++)
> >> +{
> >> +struct smbios_table *table;
> >> +struct smbios_structure_header *header;
> >> +size_t size = g_array_index(usr_blobs_sizes, size_t, i);
> >> +
> >> +header = (struct smbios_structure_header *)(usr_blobs +
> >> usr_offset);
> >> +smbios_entries = g_realloc(smbios_entries, smbios_entries_len +
> >> + size + sizeof(*table));
> >> +table = (struct smbios_table *)(smbios_entries +
> >> smbios_entries_len);
> >> +table->header.type = SMBIOS_TABLE_ENTRY;
> >> +table->header.length = cpu_to_le16(sizeof(*table) + size);
> >> +memcpy(table->data, header, size);
> >> +smbios_entries_len += sizeof(*table) + size;
> >> +(*(uint16_t *)smbios_entries) =
> >> +cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1);
> >
> > I know this comes from existing code but can you please explain why we add
> > 1 to it? This is confusing and a comment here would be nice.
> >
> >> +usr_offset += size;
> >
> > It would be better if we could add a comment here describing a bit what
> > this is all about.
> >
> > user blobs are an array of smbios_structure_header entries whereas legacy
> > tables are an array of smbios_table structures where
> > smbios_table->data represents the a single user provided table blob in
> > smbios_structure_header.
>
> Igor, are you going to send a v3 for this with the comments added?
I can add comments as a patch on top of series,
though I'd rather prefer to deprecate all this legacy code
(along with ISA
Re: [PATCH v2 07/20] smbios: avoid mangling user provided tables
> On 06-Mar-2024, at 12:11, Ani Sinha wrote:
>
>
>
> On Tue, 5 Mar 2024, Igor Mammedov wrote:
>
>> currently smbios_entry_add() preserves internally '-smbios type='
>> options but tables provided with '-smbios file=' are stored directly
>> into blob that eventually will be exposed to VM. And then later
>> QEMU adds default/'-smbios type' entries on top into the same blob.
>>
>> It makes impossible to generate tables more than once, hence
>> 'immutable' guard was used.
>> Make it possible to regenerate final blob by storing user provided
>> blobs into a dedicated area (usr_blobs) and then copy it when
>> composing final blob. Which also makes handling of -smbios
>> options consistent.
>>
>> As side effect of this and previous commits there is no need to
>> generate legacy smbios_entries at the time options are parsed.
>> Instead compose smbios_entries on demand from usr_blobs like
>> it is done for non-legacy SMBIOS tables.
>>
>> Signed-off-by: Igor Mammedov
>> Tested-by: Fiona Ebner
>
> Reviewed-by: Ani Sinha
>
>> ---
>> hw/smbios/smbios.c | 179 +++--
>> 1 file changed, 92 insertions(+), 87 deletions(-)
>>
>> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
>> index c46fc93357..aa2cc5bdbd 100644
>> --- a/hw/smbios/smbios.c
>> +++ b/hw/smbios/smbios.c
>> @@ -57,6 +57,14 @@ static size_t smbios_entries_len;
>> static bool smbios_uuid_encoded = true;
>> /* end: legacy structures & constants for <= 2.0 machines */
>>
>> +/*
>> + * SMBIOS tables provided by user with '-smbios file=' option
>> + */
>> +uint8_t *usr_blobs;
>> +size_t usr_blobs_len;
>> +static GArray *usr_blobs_sizes;
>> +static unsigned usr_table_max;
>> +static unsigned usr_table_cnt;
>>
>> uint8_t *smbios_tables;
>> size_t smbios_tables_len;
>> @@ -67,7 +75,6 @@ static SmbiosEntryPointType smbios_ep_type =
>> SMBIOS_ENTRY_POINT_TYPE_32;
>> static SmbiosEntryPoint ep;
>>
>> static int smbios_type4_count = 0;
>> -static bool smbios_immutable;
>> static bool smbios_have_defaults;
>> static uint32_t smbios_cpuid_version, smbios_cpuid_features;
>>
>> @@ -569,9 +576,8 @@ static void smbios_build_type_1_fields(void)
>>
>> uint8_t *smbios_get_table_legacy(uint32_t expected_t4_count, size_t *length)
>> {
>> -/* drop unwanted version of command-line file blob(s) */
>> -g_free(smbios_tables);
>> -smbios_tables = NULL;
>> +int i;
>> +size_t usr_offset;
>>
>> /* also complain if fields were given for types > 1 */
>> if (find_next_bit(have_fields_bitmap,
>> @@ -581,12 +587,33 @@ uint8_t *smbios_get_table_legacy(uint32_t
>> expected_t4_count, size_t *length)
>> exit(1);
>> }
>>
>> -if (!smbios_immutable) {
>> -smbios_build_type_0_fields();
>> -smbios_build_type_1_fields();
>> -smbios_validate_table(expected_t4_count);
>> -smbios_immutable = true;
>> +g_free(smbios_entries);
>> +smbios_entries_len = sizeof(uint16_t);
>> +smbios_entries = g_malloc0(smbios_entries_len);
>> +
>> +for (i = 0, usr_offset = 0; usr_blobs_sizes && i < usr_blobs_sizes->len;
>> + i++)
>> +{
>> +struct smbios_table *table;
>> +struct smbios_structure_header *header;
>> +size_t size = g_array_index(usr_blobs_sizes, size_t, i);
>> +
>> +header = (struct smbios_structure_header *)(usr_blobs + usr_offset);
>> +smbios_entries = g_realloc(smbios_entries, smbios_entries_len +
>> + size + sizeof(*table));
>> +table = (struct smbios_table *)(smbios_entries +
>> smbios_entries_len);
>> +table->header.type = SMBIOS_TABLE_ENTRY;
>> +table->header.length = cpu_to_le16(sizeof(*table) + size);
>> +memcpy(table->data, header, size);
>> +smbios_entries_len += sizeof(*table) + size;
>> +(*(uint16_t *)smbios_entries) =
>> +cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1);
>
> I know this comes from existing code but can you please explain why we add
> 1 to it? This is confusing and a comment here would be nice.
>
>> +usr_offset += size;
>
> It would be better if we could add a comment here describing a bit what
> this is all about.
>
> user blobs are an array of smbios_structure_header entries whereas legacy
> tables are an array of smbios_table structures where
> smbios_table->data represents the a single user provided table blob in
> smbios_structure_header.
Igor, are you going to send a v3 for this with the comments added?
>
>> }
>> +
>> +smbios_build_type_0_fields();
>> +smbios_build_type_1_fields();
>> +smbios_validate_table(expected_t4_count);
>> *length = smbios_entries_len;
>> return smbios_entries;
>> }
>> @@ -1094,67 +1121,67 @@ void smbios_get_tables(MachineState *ms,
>> {
>> unsigned i, dimm_cnt, offset;
>>
>> -/* drop unwanted (legacy) version of command-line file blob(s) */
>> -g_free(smbios_entries);
>> -
Re: [PATCH v2 07/20] smbios: avoid mangling user provided tables
On Tue, 5 Mar 2024, Igor Mammedov wrote:
> currently smbios_entry_add() preserves internally '-smbios type='
> options but tables provided with '-smbios file=' are stored directly
> into blob that eventually will be exposed to VM. And then later
> QEMU adds default/'-smbios type' entries on top into the same blob.
>
> It makes impossible to generate tables more than once, hence
> 'immutable' guard was used.
> Make it possible to regenerate final blob by storing user provided
> blobs into a dedicated area (usr_blobs) and then copy it when
> composing final blob. Which also makes handling of -smbios
> options consistent.
>
> As side effect of this and previous commits there is no need to
> generate legacy smbios_entries at the time options are parsed.
> Instead compose smbios_entries on demand from usr_blobs like
> it is done for non-legacy SMBIOS tables.
>
> Signed-off-by: Igor Mammedov
> Tested-by: Fiona Ebner
Reviewed-by: Ani Sinha
> ---
> hw/smbios/smbios.c | 179 +++--
> 1 file changed, 92 insertions(+), 87 deletions(-)
>
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index c46fc93357..aa2cc5bdbd 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -57,6 +57,14 @@ static size_t smbios_entries_len;
> static bool smbios_uuid_encoded = true;
> /* end: legacy structures & constants for <= 2.0 machines */
>
> +/*
> + * SMBIOS tables provided by user with '-smbios file=' option
> + */
> +uint8_t *usr_blobs;
> +size_t usr_blobs_len;
> +static GArray *usr_blobs_sizes;
> +static unsigned usr_table_max;
> +static unsigned usr_table_cnt;
>
> uint8_t *smbios_tables;
> size_t smbios_tables_len;
> @@ -67,7 +75,6 @@ static SmbiosEntryPointType smbios_ep_type =
> SMBIOS_ENTRY_POINT_TYPE_32;
> static SmbiosEntryPoint ep;
>
> static int smbios_type4_count = 0;
> -static bool smbios_immutable;
> static bool smbios_have_defaults;
> static uint32_t smbios_cpuid_version, smbios_cpuid_features;
>
> @@ -569,9 +576,8 @@ static void smbios_build_type_1_fields(void)
>
> uint8_t *smbios_get_table_legacy(uint32_t expected_t4_count, size_t *length)
> {
> -/* drop unwanted version of command-line file blob(s) */
> -g_free(smbios_tables);
> -smbios_tables = NULL;
> +int i;
> +size_t usr_offset;
>
> /* also complain if fields were given for types > 1 */
> if (find_next_bit(have_fields_bitmap,
> @@ -581,12 +587,33 @@ uint8_t *smbios_get_table_legacy(uint32_t
> expected_t4_count, size_t *length)
> exit(1);
> }
>
> -if (!smbios_immutable) {
> -smbios_build_type_0_fields();
> -smbios_build_type_1_fields();
> -smbios_validate_table(expected_t4_count);
> -smbios_immutable = true;
> +g_free(smbios_entries);
> +smbios_entries_len = sizeof(uint16_t);
> +smbios_entries = g_malloc0(smbios_entries_len);
> +
> +for (i = 0, usr_offset = 0; usr_blobs_sizes && i < usr_blobs_sizes->len;
> + i++)
> +{
> +struct smbios_table *table;
> +struct smbios_structure_header *header;
> +size_t size = g_array_index(usr_blobs_sizes, size_t, i);
> +
> +header = (struct smbios_structure_header *)(usr_blobs + usr_offset);
> +smbios_entries = g_realloc(smbios_entries, smbios_entries_len +
> + size + sizeof(*table));
> +table = (struct smbios_table *)(smbios_entries + smbios_entries_len);
> +table->header.type = SMBIOS_TABLE_ENTRY;
> +table->header.length = cpu_to_le16(sizeof(*table) + size);
> +memcpy(table->data, header, size);
> +smbios_entries_len += sizeof(*table) + size;
> +(*(uint16_t *)smbios_entries) =
> +cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1);
I know this comes from existing code but can you please explain why we add
1 to it? This is confusing and a comment here would be nice.
> +usr_offset += size;
It would be better if we could add a comment here describing a bit what
this is all about.
user blobs are an array of smbios_structure_header entries whereas legacy
tables are an array of smbios_table structures where
smbios_table->data represents the a single user provided table blob in
smbios_structure_header.
> }
> +
> +smbios_build_type_0_fields();
> +smbios_build_type_1_fields();
> +smbios_validate_table(expected_t4_count);
> *length = smbios_entries_len;
> return smbios_entries;
> }
> @@ -1094,67 +1121,67 @@ void smbios_get_tables(MachineState *ms,
> {
> unsigned i, dimm_cnt, offset;
>
> -/* drop unwanted (legacy) version of command-line file blob(s) */
> -g_free(smbios_entries);
> -smbios_entries = NULL;
> +g_free(smbios_tables);
> +smbios_tables = g_memdup2(usr_blobs, usr_blobs_len);
Again a comment describing here would be nice as to why memdup is ok.
> +smbios_tables_len = usr_blobs_len;
> +smbios_table_max =
