Re: [PATCH v2] qga: add security info to guest-get-osinfo

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Mar 30, 2026 at 06:19:41PM +0300, Elizabeth Ashurov wrote:
> Extend guest-get-osinfo to include security features status
> (VBS, Secure Boot, TPM) in a nested 'security' field.
> OS-specific data (e.g. Windows DeviceGuard) is separated
> using a union to allow future per-OS extensions.
> 
> The implementation queries Win32_DeviceGuard and Win32_Tpm via
> WMI, and reads the SecureBoot UEFI variable through
> GetFirmwareEnvironmentVariable().
> 
> Signed-off-by: Elizabeth Ashurov <eashurov@redhat.com>
> ---
>  qga/commands-win32.c | 421 +++++++++++++++++++++++++++++++++++++++++++

This should also provide a Linux impl for the TPM and secureboot info as
that is platform portable and easily available.

>  qga/qapi-schema.json |  91 +++++++++-
>  2 files changed, 511 insertions(+), 1 deletion(-)

> +
> +/*
> + * Read the SecureBoot UEFI variable.  On legacy BIOS systems the field
> + * is omitted (not applicable).
> + */
> +static void get_secure_boot_status(GuestSecurityInfo *info,
> +                                   Error **errp)
> +{
> +    Error *local_err = NULL;
> +    BYTE value = 0;
> +    DWORD ret;
> +
> +    acquire_privilege(SE_SYSTEM_ENVIRONMENT_NAME, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    ret = GetFirmwareEnvironmentVariableA("SecureBoot",
> +        "{8be4df61-93ca-11d2-aa0d-00e098032b8c}", &value, sizeof(value));
> +
> +    if (ret == 0) {
> +        DWORD err = GetLastError();
> +        if (err == ERROR_INVALID_FUNCTION) {
> +            return;
> +        }
> +        if (err == ERROR_ENVVAR_NOT_FOUND) {
> +            info->has_secure_boot = true;
> +            info->secure_boot = false;
> +            return;
> +        }
> +        error_setg_win32(errp, err,
> +                         "failed to read SecureBoot UEFI variable");
> +        return;
> +    }
> +
> +    info->has_secure_boot = true;
> +    info->secure_boot = (value == 1);

Is that comparison of 'value == 1' definitely correct ?  My
understanding was that secure boot state is indicated by the 5th byte
in that EFI variable.



> +}
> +
> +/*
> + * Query Win32_Tpm WMI class for TPM presence and version.
> + */
> +static void get_tpm_info(GuestSecurityInfo *info, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    IWbemServices *services = NULL;
> +    IEnumWbemClassObject *enumerator = NULL;
> +    IWbemClassObject *obj = NULL;
> +    ULONG count = 0;
> +    HRESULT hr;
> +    VARIANT var;
> +
> +    services = wmi_connect_to_namespace(
> +        L"ROOT\\CIMV2\\Security\\MicrosoftTpm", &local_err);
> +    if (!services) {
> +        /* TPM namespace may not exist -- field omitted (unknown) */
> +        error_free(local_err);
> +        return;
> +    }
> +
> +    enumerator = wmi_exec_query(services,
> +        L"SELECT * FROM Win32_Tpm", &local_err);
> +    if (!enumerator) {
> +        error_free(local_err);
> +        goto out;
> +    }
> +
> +    hr = enumerator->lpVtbl->Next(enumerator, WBEM_INFINITE, 1,
> +                                   &obj, &count);
> +    if (FAILED(hr) || count == 0) {
> +        info->has_tpm_present = true;
> +        info->tpm_present = false;
> +        goto out;
> +    }
> +
> +    info->has_tpm_present = true;
> +    info->tpm_present = true;
> +
> +    VariantInit(&var);
> +    if (SUCCEEDED(wmi_get_property(obj, L"SpecVersion", &var)) &&
> +        V_VT(&var) == VT_BSTR && V_BSTR(&var)) {
> +        info->tpm_version = g_utf16_to_utf8(
> +            (const gunichar2 *)V_BSTR(&var), -1, NULL, NULL, NULL);
> +        if (info->tpm_version) {
> +            /* keep only the part before the first comma */
> +            char *comma = strchr(info->tpm_version, ',');

It would be helpful if the comment included a full example string
from "SpecVersion", so we can see what we're parsing here.

> +            if (comma) {
> +                *comma = '\0';
> +            }
> +        }
> +    }
> +    VariantClear(&var);


> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index c57bc9a02f..2247f77cff 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1490,6 +1490,8 @@
>  #     * POSIX: as defined by os-release(5)
>  #     * Windows: contains string "server" or "client"
>  #
> +# @security: Security features status (since 10.3)
> +#
>  # .. note:: On POSIX systems the fields @id, @name, @pretty-name,
>  #    @version, @version-id, @variant and @variant-id follow the
>  #    definition specified in os-release(5).  Refer to the manual page
> @@ -1508,7 +1510,8 @@
>        '*kernel-release': 'str', '*kernel-version': 'str',
>        '*machine': 'str', '*id': 'str', '*name': 'str',
>        '*pretty-name': 'str', '*version': 'str', '*version-id': 'str',
> -      '*variant': 'str', '*variant-id': 'str' } }
> +      '*variant': 'str', '*variant-id': 'str',
> +      '*security': 'GuestSecurityInfo' } }
>  
>  ##
>  # @guest-get-osinfo:
> @@ -1952,3 +1955,89 @@
>    'returns': ['GuestNetworkRoute'],
>    'if': { 'any': ['CONFIG_LINUX', 'CONFIG_WIN32'] }
>  }
> +
> +##
> +# @GuestSecurityInfoWindows:
> +#
> +# Windows-specific security features from Win32_DeviceGuard.
> +#
> +# @vbs-status: VirtualizationBasedSecurityStatus
> +#
> +# @available-security-properties:
> +#     AvailableSecurityProperties
> +#
> +# @code-integrity-policy-enforcement-status:
> +#     CodeIntegrityPolicyEnforcementStatus
> +#
> +# @required-security-properties: RequiredSecurityProperties
> +#
> +# @security-services-configured:
> +#     SecurityServicesConfigured
> +#
> +# @security-services-running: SecurityServicesRunning
> +#
> +# @usr-cfg-code-integrity-policy-enforcement-status:
> +#     UsermodeCodeIntegrityPolicyEnforcementStatus

None of these docs are actually useful. Can you explain in plain
language what they each mean.

> +#
> +# Since: 10.3
> +##
> +{ 'struct': 'GuestSecurityInfoWindows',
> +  'data': {
> +      '*vbs-status': 'int',
> +      '*available-security-properties': ['int'],
> +      '*code-integrity-policy-enforcement-status': 'int',
> +      '*required-security-properties': ['int'],
> +      '*security-services-configured': ['int'],
> +      '*security-services-running': ['int'],
> +      '*usr-cfg-code-integrity-policy-enforcement-status':
> +          'int' } }

I'm guessing there that the are all largely reflecting enumerated
values from the Windows API. I'm wondering how they will be
consumed by whatever calls this command. Would they be better off
as enums, instead of plain ints, or arrays of ints ?

> +
> +##
> +# @GuestSecurityInfoType:
> +#
> +# Guest operating system type for security info.
> +#
> +# @windows: Microsoft Windows
> +#
> +# Since: 10.3
> +##
> +{ 'enum': 'GuestSecurityInfoType',
> +  'data': ['windows'] }
> +
> +##
> +# @GuestSecurityInfoOs:
> +#
> +# OS-specific security information.
> +#
> +# @type: guest operating system type
> +#
> +# Since: 10.3
> +##
> +{ 'union': 'GuestSecurityInfoOs',
> +  'base': { 'type': 'GuestSecurityInfoType' },
> +  'discriminator': 'type',
> +  'data': {
> +      'windows': 'GuestSecurityInfoWindows' } }
> +
> +##
> +# @GuestSecurityInfo:
> +#
> +# Guest security features status.  Fields are optional; a missing
> +# field means the information is not available on this platform.
> +#
> +# @tpm-present: Whether a TPM device is present
> +#
> +# @tpm-version: TPM specification version (e.g. "2.0")
> +#
> +# @secure-boot: Whether UEFI Secure Boot is enabled
> +#
> +# @os: OS-specific security information
> +#
> +# Since: 10.3
> +##
> +{ 'struct': 'GuestSecurityInfo',
> +  'data': {
> +      '*tpm-present': 'bool',
> +      '*tpm-version': 'str',

Should we have a GuestSecurityTPMInfo struct ?

    '*tpm': 'GuestSecurityTPMInfo'

We would not need a "present" value, as the mere existence of the struct
data would indicate presence. Meanwhile this struct would contain a
mandatory version field:

   { 'struct': 'GuestSecurityTPMInfo',
     'data': {
         'major-version': 'int'
     }
   }

I suggest only reporting major version, as an int, not an arbitrary
string as that's ambiguous IMHO.  We could later add other TPM info
to this struct if needed.

For Linux you can provide this TPM info by checking whether
/sys/class/tpm/tpm0 exists, and reading /sys/class/tpm/tpm0/tpm_version_major

> +      '*secure-boot': 'bool',

For Linux you can provide this secureboot info by reading
/sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c

This bool only tells us whether secureboot is enabled or not. I wonder
if we should allow for other info here too. Systemd's bootctl reads
various other secureboot fields "AuditMode", "DeployedMode", 'SetupMode'
and 'MokSBStateRT'.

https://github.com/systemd/systemd/blob/main/src/basic/efivars.c#L381

We could represent each as bools within a struct, eg

  '*secure-boot': 'GuestSecuritySecureBootInfo'

which would have several mandatory fields

   { 'struct': 'GuestSecuritySecureBootInfo',
     'data': {
         'enabled': 'bool',
	 'audit-mode': 'bool',
	 'deployed-mode': 'bool',
	 'setup-mode': 'bool',
	 'mok-sb-state': 'bool',
     }
   }

These are again all available from EFI variables in sysfs, or the
equivalent API you have already used on Windows.

Any missing variable can be assumed "false", or we would make all
the variables except 'enabled' be optional.

> +      '*os': 'GuestSecurityInfoOs' } }

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|