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 <imamm...@redhat.com>
Reviewed-by: Ani Sinha <anisi...@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
>
>
