Re: [PATCH v2 07/20] smbios: avoid mangling user provided tables

[Igor Mammedov]

On Thu, 7 Mar 2024 09:33:17 +0530
Ani Sinha <anisinha@redhat.com> wrote:

> > On 06-Mar-2024, at 12:11, Ani Sinha <anisinha@redhat.com> 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 <imammedo@redhat.com>
> >> Tested-by: Fiona Ebner <f.ebner@proxmox.com>  
> > 
> > Reviewed-by: Ani Sinha <anisinha@redhat.com>
> >   
> >> ---
> >> 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=<foo>' 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 machine) and just remove it.

> 
> >   
> >>     }
> >> +
> >> +    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 = usr_table_max;
> >> +    smbios_table_cnt = usr_table_cnt;
> >> 
> >> -    if (!smbios_immutable) {
> >> -        smbios_build_type_0_table();
> >> -        smbios_build_type_1_table();
> >> -        smbios_build_type_2_table();
> >> -        smbios_build_type_3_table();
> >> +    smbios_build_type_0_table();
> >> +    smbios_build_type_1_table();
> >> +    smbios_build_type_2_table();
> >> +    smbios_build_type_3_table();
> >> 
> >> -        assert(ms->smp.sockets >= 1);
> >> +    assert(ms->smp.sockets >= 1);
> >> 
> >> -        for (i = 0; i < ms->smp.sockets; i++) {
> >> -            smbios_build_type_4_table(ms, i);
> >> -        }
> >> +    for (i = 0; i < ms->smp.sockets; i++) {
> >> +        smbios_build_type_4_table(ms, i);
> >> +    }
> >> 
> >> -        smbios_build_type_8_table();
> >> -        smbios_build_type_11_table();
> >> +    smbios_build_type_8_table();
> >> +    smbios_build_type_11_table();
> >> 
> >> #define MAX_DIMM_SZ (16 * GiB)
> >> #define GET_DIMM_SZ ((i < dimm_cnt - 1) ? MAX_DIMM_SZ \
> >>                                         : ((current_machine->ram_size - 1) 
> >> % MAX_DIMM_SZ) + 1)
> >> 
> >> -        dimm_cnt = QEMU_ALIGN_UP(current_machine->ram_size, MAX_DIMM_SZ) 
> >> / MAX_DIMM_SZ;
> >> +    dimm_cnt = QEMU_ALIGN_UP(current_machine->ram_size, MAX_DIMM_SZ) /
> >> +               MAX_DIMM_SZ;
> >> 
> >> -        /*
> >> -         * The offset determines if we need to keep additional space 
> >> between
> >> -         * table 17 and table 19 header handle numbers so that they do
> >> -         * not overlap. For example, for a VM with larger than 8 TB guest
> >> -         * memory and DIMM like chunks of 16 GiB, the default space 
> >> between
> >> -         * the two tables (T19_BASE - T17_BASE = 512) is not enough.
> >> -         */
> >> -        offset = (dimm_cnt > (T19_BASE - T17_BASE)) ? \
> >> -                 dimm_cnt - (T19_BASE - T17_BASE) : 0;
> >> +    /*
> >> +     * The offset determines if we need to keep additional space between
> >> +     * table 17 and table 19 header handle numbers so that they do
> >> +     * not overlap. For example, for a VM with larger than 8 TB guest
> >> +     * memory and DIMM like chunks of 16 GiB, the default space between
> >> +     * the two tables (T19_BASE - T17_BASE = 512) is not enough.
> >> +     */
> >> +    offset = (dimm_cnt > (T19_BASE - T17_BASE)) ? \
> >> +             dimm_cnt - (T19_BASE - T17_BASE) : 0;
> >> 
> >> -        smbios_build_type_16_table(dimm_cnt);
> >> +    smbios_build_type_16_table(dimm_cnt);
> >> 
> >> -        for (i = 0; i < dimm_cnt; i++) {
> >> -            smbios_build_type_17_table(i, GET_DIMM_SZ);
> >> -        }
> >> +    for (i = 0; i < dimm_cnt; i++) {
> >> +        smbios_build_type_17_table(i, GET_DIMM_SZ);
> >> +    }
> >> 
> >> -        for (i = 0; i < mem_array_size; i++) {
> >> -            smbios_build_type_19_table(i, offset, mem_array[i].address,
> >> -                                       mem_array[i].length);
> >> -        }
> >> +    for (i = 0; i < mem_array_size; i++) {
> >> +        smbios_build_type_19_table(i, offset, mem_array[i].address,
> >> +                                   mem_array[i].length);
> >> +    }
> >> 
> >> -        /*
> >> -         * make sure 16 bit handle numbers in the headers of tables 19
> >> -         * and 32 do not overlap.
> >> -         */
> >> -        assert((mem_array_size + offset) < (T32_BASE - T19_BASE));
> >> +    /*
> >> +     * make sure 16 bit handle numbers in the headers of tables 19
> >> +     * and 32 do not overlap.
> >> +     */
> >> +    assert((mem_array_size + offset) < (T32_BASE - T19_BASE));
> >> 
> >> -        smbios_build_type_32_table();
> >> -        smbios_build_type_38_table();
> >> -        smbios_build_type_41_table(errp);
> >> -        smbios_build_type_127_table();
> >> +    smbios_build_type_32_table();
> >> +    smbios_build_type_38_table();
> >> +    smbios_build_type_41_table(errp);
> >> +    smbios_build_type_127_table();
> >> 
> >> -        smbios_validate_table(ms->smp.sockets);
> >> -        smbios_entry_point_setup();
> >> -        smbios_immutable = true;
> >> -    }
> >> +    smbios_validate_table(ms->smp.sockets);
> >> +    smbios_entry_point_setup();
> >> 
> >>     /* return tables blob and entry point (anchor), and their sizes */
> >>     *tables = smbios_tables;
> >> @@ -1254,13 +1281,10 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> >> {
> >>     const char *val;
> >> 
> >> -    assert(!smbios_immutable);
> >> -
> >>     val = qemu_opt_get(opts, "file");
> >>     if (val) {
> >>         struct smbios_structure_header *header;
> >> -        int size;
> >> -        struct smbios_table *table; /* legacy mode only */
> >> +        size_t size;
> >> 
> >>         if (!qemu_opts_validate(opts, qemu_smbios_file_opts, errp)) {
> >>             return;
> >> @@ -1277,9 +1301,9 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> >>          * (except in legacy mode, where the second '\0' is implicit and
> >>          *  will be inserted by the BIOS).
> >>          */
> >> -        smbios_tables = g_realloc(smbios_tables, smbios_tables_len + 
> >> size);
> >> -        header = (struct smbios_structure_header *)(smbios_tables +
> >> -                                                    smbios_tables_len);
> >> +        usr_blobs = g_realloc(usr_blobs, usr_blobs_len + size);
> >> +        header = (struct smbios_structure_header *)(usr_blobs +
> >> +                                                    usr_blobs_len);
> >> 
> >>         if (load_image_size(val, (uint8_t *)header, size) != size) {
> >>             error_setg(errp, "Failed to load SMBIOS file %s", val);
> >> @@ -1300,34 +1324,15 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> >>             smbios_type4_count++;
> >>         }
> >> 
> >> -        smbios_tables_len += size;
> >> -        if (size > smbios_table_max) {
> >> -            smbios_table_max = size;
> >> +        if (!usr_blobs_sizes) {
> >> +            usr_blobs_sizes = g_array_new(false, false, sizeof(size_t));
> >>         }
> >> -        smbios_table_cnt++;
> >> -
> >> -        /* add a copy of the newly loaded blob to legacy smbios_entries */
> >> -        /* NOTE: This code runs before smbios_set_defaults(), so we don't
> >> -         *       yet know which mode (legacy vs. aggregate-table) will be
> >> -         *       required. We therefore add the binary blob to both legacy
> >> -         *       (smbios_entries) and aggregate (smbios_tables) tables, 
> >> and
> >> -         *       delete the one we don't need from smbios_set_defaults(),
> >> -         *       once we know which machine version has been requested.
> >> -         */
> >> -        if (!smbios_entries) {
> >> -            smbios_entries_len = sizeof(uint16_t);
> >> -            smbios_entries = g_malloc0(smbios_entries_len);
> >> +        g_array_append_val(usr_blobs_sizes, size);
> >> +        usr_blobs_len += size;
> >> +        if (size > usr_table_max) {
> >> +            usr_table_max = size;
> >>         }
> >> -        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);
> >> -        /* end: add a copy of the newly loaded blob to legacy 
> >> smbios_entries */
> >> +        usr_table_cnt++;
> >> 
> >>         return;
> >>     }
> >> --
> >> 2.39.3  
> 
>