Re: [PATCH 08/19] smbios: don't check type4 structures in legacy mode

[Ani Sinha <anisinha@redhat.com>]

On Tue, 27 Feb 2024, Igor Mammedov wrote:

> legacy mode doesn't support structures of type 2 and more,
> and CLI has a check for '-smbios type' option, however it's
> still possible to sneak in type4 as a blob with '-smbios file'
> option. However doing the later makes SMBIOS tables broken
> since SeaBIOS doesn't expect that.
>
> Rather than trying to add support for type4 to legacy code
> (both QEMU and SeaBIOS), simplify smbios_get_table_legacy()
> by dropping not relevant check in legacy code and error out
> on type4 blob.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Ani Sinha <anisinha@redhat.com>

> ---
>  * The issue affects 'isapc' and pc-i440fx-2.0. the later is
>    in deprecated state and to be dropped in near future
>  * possibly the same issue applies to other SMBIOS types above type 1
>    but I haven't tested that, and well tables that aren't
>    generated by SeaBIOS can get be added just fine
>    (tested type11 blob). So I went with a minimal change
>    to fixup type4 only that I'm touching. Leaving the rest
>    for other time or when someone complains about it, which is
>    very unlikely given it's really only remaining isapc machine.
>
>    I'd very much prefer to deprecate 'isapc' and then drop
>    all legacy related code (it will benefit not only SMBIOS
>    but other code as well).
>    BTW: 'isapc' is in semi-dead, I cna't boot RHEL6 on it
>    with KVM enabled anymore (RHEL9 host), TCG still boots though.
>    One more reason to get deprecate it.
> ---
>  include/hw/firmware/smbios.h |  2 +-
>  hw/i386/fw_cfg.c             |  3 +--
>  hw/smbios/smbios.c           | 18 ++++++++++++++----
>  3 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h
> index 0818184834..1fbff3c55f 100644
> --- a/include/hw/firmware/smbios.h
> +++ b/include/hw/firmware/smbios.h
> @@ -296,7 +296,7 @@ void smbios_set_defaults(const char *manufacturer, const char *product,
>                           const char *version,
>                           bool uuid_encoded, SmbiosEntryPointType ep_type);
>  void smbios_set_default_processor_family(uint16_t processor_family);
> -uint8_t *smbios_get_table_legacy(uint32_t expected_t4_count, size_t *length);
> +uint8_t *smbios_get_table_legacy(size_t *length);
>  void smbios_get_tables(MachineState *ms,
>                         const struct smbios_phys_mem_area *mem_array,
>                         const unsigned int mem_array_size,
> diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
> index c1e9c0fd9c..d1281066f4 100644
> --- a/hw/i386/fw_cfg.c
> +++ b/hw/i386/fw_cfg.c
> @@ -71,8 +71,7 @@ void fw_cfg_build_smbios(PCMachineState *pcms, FWCfgState *fw_cfg)
>      smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
>
>      if (pcmc->smbios_legacy_mode) {
> -        smbios_tables = smbios_get_table_legacy(ms->smp.cpus,
> -                                                &smbios_tables_len);
> +        smbios_tables = smbios_get_table_legacy(&smbios_tables_len);
>          fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
>                           smbios_tables, smbios_tables_len);
>          return;
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index aa2cc5bdbd..97cf762228 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -482,14 +482,17 @@ opts_init(smbios_register_config);
>   */
>  #define SMBIOS_21_MAX_TABLES_LEN 0xffff
>
> -static void smbios_validate_table(uint32_t expected_t4_count)
> +static void smbios_check_type4_count(uint32_t expected_t4_count)
>  {
>      if (smbios_type4_count && smbios_type4_count != expected_t4_count) {
>          error_report("Expected %d SMBIOS Type 4 tables, got %d instead",
>                       expected_t4_count, smbios_type4_count);
>          exit(1);
>      }
> +}
>
> +static void smbios_validate_table(void)
> +{
>      if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_32 &&
>          smbios_tables_len > SMBIOS_21_MAX_TABLES_LEN) {
>          error_report("SMBIOS 2.1 table length %zu exceeds %d",
> @@ -574,7 +577,7 @@ static void smbios_build_type_1_fields(void)
>      }
>  }
>
> -uint8_t *smbios_get_table_legacy(uint32_t expected_t4_count, size_t *length)
> +uint8_t *smbios_get_table_legacy(size_t *length)
>  {
>      int i;
>      size_t usr_offset;
> @@ -587,6 +590,12 @@ uint8_t *smbios_get_table_legacy(uint32_t expected_t4_count, size_t *length)
>          exit(1);
>      }
>
> +    if (test_bit(4, have_binfile_bitmap)) {
> +        error_report("can't process table for smbios "
> +                     "type 4 on machine versions < 2.1!");
> +        exit(1);
> +    }
> +
>      g_free(smbios_entries);
>      smbios_entries_len = sizeof(uint16_t);
>      smbios_entries = g_malloc0(smbios_entries_len);
> @@ -613,7 +622,7 @@ uint8_t *smbios_get_table_legacy(uint32_t expected_t4_count, size_t *length)
>
>      smbios_build_type_0_fields();
>      smbios_build_type_1_fields();
> -    smbios_validate_table(expected_t4_count);
> +    smbios_validate_table();
>      *length = smbios_entries_len;
>      return smbios_entries;
>  }
> @@ -1180,7 +1189,8 @@ void smbios_get_tables(MachineState *ms,
>      smbios_build_type_41_table(errp);
>      smbios_build_type_127_table();
>
> -    smbios_validate_table(ms->smp.sockets);
> +    smbios_check_type4_count(ms->smp.sockets);
> +    smbios_validate_table();
>      smbios_entry_point_setup();
>
>      /* return tables blob and entry point (anchor), and their sizes */
> --
> 2.39.3
>
>