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

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

On Tue, Mar 31, 2026 at 03:55:01PM +0300, Kostiantyn Kostiuk wrote:
> Best Regards,
> Kostiantyn Kostiuk.
> 
> 
> On Tue, Mar 31, 2026 at 3:08 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > 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.
> >
> 
> In MSFT sample
> https://github.com/microsoft/Windows-universal-samples/blob/main/Samples/CustomCapability/Service/Client/uefi.cpp
> the reverse logic is used.
> But from our testing on Windows, this comparison is correct.
> 
> 
> >
> >
> >
> > > +}
> > > +
> > > +/*
> > > + * 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 ?
> >
> 
> There is no any existing enum in the Win32 API, as I know.
> If we introduce it, we should care about any updates from the MSFT side.
> Explanation of these numbers
> https://learn.microsoft.com/en-us/windows/security/hardware-security/enable-virtualization-based-protection-of-code-integrity?tabs=security
> 
> looks complicated, and I am not sure whether it is better to return
> "HardwareEnforcedStackProtectionInAuditMode" is better than 6.
> 
> Currently, QGA returns data in the same format as MSFT provides it.
> I think the management system should make a decision based on this.
> 
> 
> >
> > > +
> > > +##
> > > +# @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',
> >
> 
> Any real use cases for all other fields?
> Or just get it because all of them are related to SB?

The security characteristics of a system with secure boot are more
granular than just a boolean on/off. These other variables give you
the full insight into the security of the system.

Further details are in the spec

  http://www.uefi.org/sites/default/files/resources/UEFI%20Spec%202_7_A%20Sept%206.pdf

Specifically the section "31.3 Firmware/OS Key Exchange: creating
trust relationships" is a large diagram showing the different modes
that are expressed by these variables. The exception mok-sb-state
which is something exposed by Shim, not a core UEFI variable, but
still important to overall system security.

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 :|