Re: [XEN PATCH] tools/firmware/hvmloader/smbios.c: Add new SMBIOS tables (7,8,9,26,27,28)

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.01.2022 09:52, Anton Belousov wrote:
> SMBIOS tables like 7,8,9,26,27,28 are neccessary to prevent sandbox detection 
> by malware
> using WMI-queries.

This is a statement without any proof. It may seem obvious to you, but
could you please make us properly see the value of your addition as
well as allow justification towards the completeness of your changes
(i.e. that there aren't further types specifying of which may also
help)?

Furthermore, with the original intention of the machinery having been
to pass through host SMBIOS structures, you will also want to say a
word on the (intended) source of the data here. If it's again the host
table, then you will want to also discuss the security of doing so.

> New tables can be mapped to memory from binary file specified in
> "smbios_firmware" parameter of domain configuration. If particular table is 
> absent
> in binary file, then it will not be mapped to memory. This method works for 
> Windows
> domains as tables 7,8,9,26,27,28 are not critical for OS boot and runtime. 
> Also if "smbios_firmware"
> parameter is not provided, these tables will be skipped in 
> write_smbios_tables function.
> 
> Signed-off-by: Anton Belousov <blsvntn@xxxxxxxxxxx>
> ---
>  tools/firmware/hvmloader/smbios.c       | 146 ++++++++++++++++++++++++
>  tools/firmware/hvmloader/smbios_types.h |  77 +++++++++++++
>  2 files changed, 223 insertions(+)

Somewhere here please have a brief description of what has changed
between versions. Also please properly tag you patch with a version
(this one looks to be v2).

> @@ -700,6 +724,66 @@ smbios_type_4_init(
>      return start+1;
>  }
>  
> +/* Type 7 -- Cache Information */
> +static void *
> +smbios_type_7_init(void *start)
> +{
> +    struct smbios_type_7 *p = (struct smbios_type_7 *)start;

Please avoid introducing casts when none are necessary.

> +    void *pts;
> +    uint32_t length;
> +
> +    pts = get_smbios_pt_struct(7, &length);
> +    if ( (pts != NULL)&&(length > 0) )

In reply to v1 I did ask you to avoid copying existing style violations.
In the example here there are missing blanks on either side of &&.

I further wonder whether indeed any non-zero length is fine. IOW it may
be worthwhile to first harden the pre-existing cases a little before
further cloning them. Actually, seeing that the smbios_type_<N>_init()
functions are all largely identical, it may be worthwhile to introduce
a single function doing what is needed for all overridable types.

Jan