Re: [PATCH v2 6/6] tools: make virt-host-validate check CPU vulnerabilities

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Sep 30, 2019 at 10:55:00AM +0200, Martin Kletzander wrote:
> On Fri, Sep 27, 2019 at 01:52:25PM +0100, Daniel P. Berrangé wrote:
> > Add a check reporting if any CPU vulnerabilities have not been mitigated
> > by the kernel. It further reports whether it is safe to use Intel SMT
> > for KVM guests or not, as several of the vulnerabilities are dangerous
> > when combined with SMT and KVM, even if mitigations are in effect.
> > 
> > eg on a host with mitigations, but unsafe SMT still enabled:
> > 
> >  Checking CPU hardware vulnerability mitigation...PASS
> >  Checking CPU hardware vulnerability SMT safety...FAIL
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > ---
> > libvirt.spec.in                               |   1 +
> > tools/Makefile.am                             |   1 +
> > .../rules/linux-cpu-hardware-flaws.yaml       | 165 ++++++++++++++++++
> > 3 files changed, 167 insertions(+)
> > create mode 100644 tools/host-validate/rules/linux-cpu-hardware-flaws.yaml
> > 
> > diff --git a/libvirt.spec.in b/libvirt.spec.in
> > index f336296a08..8aa226798a 100644
> > --- a/libvirt.spec.in
> > +++ b/libvirt.spec.in
> > @@ -1901,6 +1901,7 @@ exit 0
> > %{_datadir}/libvirt/host-validate/linux-acpi.yaml
> > %{_datadir}/libvirt/host-validate/linux-cgroups.yaml
> > %{_datadir}/libvirt/host-validate/linux-cpu.yaml
> > +%{_datadir}/libvirt/host-validate/linux-cpu-hardware-flaws.yaml
> > %{_datadir}/libvirt/host-validate/linux-devices.yaml
> > %{_datadir}/libvirt/host-validate/linux-iommu.yaml
> > %{_datadir}/libvirt/host-validate/linux-namespaces.yaml
> > diff --git a/tools/Makefile.am b/tools/Makefile.am
> > index 728de475a2..907b0195c2 100644
> > --- a/tools/Makefile.am
> > +++ b/tools/Makefile.am
> > @@ -173,6 +173,7 @@ virt_host_validate_rules_DATA = \
> > 	$(srcdir)/host-validate/rules/linux-acpi.yaml \
> > 	$(srcdir)/host-validate/rules/linux-cgroups.yaml \
> > 	$(srcdir)/host-validate/rules/linux-cpu.yaml \
> > +	$(srcdir)/host-validate/rules/linux-cpu-hardware-flaws.yaml \
> > 	$(srcdir)/host-validate/rules/linux-devices.yaml \
> > 	$(srcdir)/host-validate/rules/linux-iommu.yaml \
> > 	$(srcdir)/host-validate/rules/linux-namespaces.yaml \
> > diff --git a/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml b/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml
> > new file mode 100644
> > index 0000000000..6a243df96d
> > --- /dev/null
> > +++ b/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml
> > @@ -0,0 +1,165 @@
> > +#
> > +# Define facts related to CPU hardware vulnerabilities
> > +#
> > +
> > +facts:
> > +- name: cpu.vulnerability.meltdown
> > +  filter:
> > +    fact:
> > +      name: os.kernel
> > +      value: Linux
> > +  value:
> > +    file:
> > +      path: /sys/devices/system/cpu/vulnerabilities/meltdown
> > +      ignoreMissing: true
> > +      parse:
> > +        scalar:
> > +          regex: (\w+)
> > +          match: 1
> > +- name: cpu.vulnerability.spectre_v1
> > +  filter:
> > +    fact:
> > +      name: os.kernel
> > +      value: Linux
> > +  value:
> > +    file:
> > +      path: /sys/devices/system/cpu/vulnerabilities/spectre_v1
> > +      ignoreMissing: true
> > +      parse:
> > +        scalar:
> > +          regex: (\w+)
> > +          match: 1
> > +- name: cpu.vulnerability.spectre_v2
> > +  filter:
> > +    fact:
> > +      name: os.kernel
> > +      value: Linux
> > +  value:
> > +    file:
> > +      path: /sys/devices/system/cpu/vulnerabilities/spectre_v2
> > +      ignoreMissing: true
> > +      parse:
> > +        scalar:
> > +          regex: (\w+)
> > +          match: 1
> > +- name: cpu.vulnerability.spec_store_bypass
> > +  filter:
> > +    fact:
> > +      name: os.kernel
> > +      value: Linux
> > +  value:
> > +    file:
> > +      path: /sys/devices/system/cpu/vulnerabilities/spec_store_bypass
> > +      ignoreMissing: true
> > +      parse:
> > +        scalar:
> > +          regex: (\w+)
> > +          match: 1
> > +- name: cpu.vulnerability.mds
> > +  filter:
> > +    fact:
> > +      name: os.kernel
> > +      value: Linux
> > +  value:
> > +    file:
> > +      path: /sys/devices/system/cpu/vulnerabilities/mds
> > +      ignoreMissing: true
> > +      parse:
> > +        scalar:
> > +          regex: (\w+)
> > +          match: 1
> > +- name: cpu.vulnerability.mds_smt
> > +  filter:
> > +    fact:
> > +      name: os.kernel
> > +      value: Linux
> > +  value:
> > +    file:
> > +      path: /sys/devices/system/cpu/vulnerabilities/mds
> > +      ignoreMissing: true
> > +      parse:
> > +        scalar:
> > +          regex: SMT (\w+)
> > +          match: 1
> > +- name: cpu.vulnerability.l1tf
> > +  filter:
> > +    fact:
> > +      name: os.kernel
> > +      value: Linux
> > +  value:
> > +    file:
> > +      path: /sys/devices/system/cpu/vulnerabilities/l1tf
> > +      ignoreMissing: true
> > +      parse:
> > +        scalar:
> > +          regex: (\w+)
> > +          match: 1
> > +- name: cpu.vulnerability.l1tf_smt
> > +  filter:
> > +    fact:
> > +      name: os.kernel
> > +      value: Linux
> > +  value:
> > +    file:
> > +      path: /sys/devices/system/cpu/vulnerabilities/l1tf
> > +      ignoreMissing: true
> > +      parse:
> > +        scalar:
> > +          regex: SMT (\w+)
> > +          match: 1
> 
> Given the fact that most of these could just be virFileReadValueUint() it does
> not even make it easier to read or write the code.

Errr, virFileReadValueUint reads a single integer from a file.

This is extracting a single *word* from a specific field in a
file.

Doing this in code would involve doing a regex match the same as
here.

> Every time someone will want to add a new check or a fact they will need to find
> a similar one, copy-paste it, change it and hope for the best.  This introduces
> yet another "language" on top of the two you are adding already.  I really do
> not see any benefit in this.

Taking this particular example.

  - name: cpu.vulnerability.l1tf_smt
    filter:
      fact:
        name: os.kernel
        value: Linux
    value:
      file:
        path: /sys/devices/system/cpu/vulnerabilities/l1tf
        ignoreMissing: true
        parse:
          scalar:
            regex: SMT (\w+)
            match: 1

It could be approximately equiv to code

        if runtime.GOOS == "Linux" {
	    return "", nil
	}

        data, err = ioutils.ReadFile("/sys/devices/system/cpu/vulnerabilities/l1tf")
	if err != nil {
	    if  os.IsNotExist(err) {
	        return "", nil
	    }
	    return "", err
	}

	match, err := regexp.Find(`SMT (\w+)`, data)
	if err != nil {
 	    return "", err
	}
	err = SetFact("cpu.vulnerability.l1tf_smt", match)
	if err != nil {
	    return "", err
	}

The code does look more familiar, but it is certainly not simpler.
Most notabley there are alot of error handling cases here that need
to be dealt with, even when written in Go. Doing this in C with its
string handling support involves even more error handling. This
is a significant maintainence burden that is not well handled in
a consistent manner across the code.

This is a fairly simple check example, case where we want to extract
data from a more more complex file formats result in much more code
complexity.

In the pure code approach, we have mixed up code related to reading
files, running commands, parsing data formats, setting facts, printing
messages, skipping execution when certain other conditions applied.
Hidden in this is the information about what feature we are actually
checking for.

A data driven approach to this problem means that we get clear separation
between distinct parts/jobs.

The engine written in code concerns itself with generic facilities for
reading files, running commands, parsing data, and all the error handling
and data processing.

Thereafter the interesting checks is simply a declarative expression of
what data we wish to extract for each fature, and declarative expression
of dependancies between features.

This engine code has greater complexity than what exists today for sure,
but the key thing is that this is mostly a one time upfront code, and is
the least interesting part of the tool. What is most interest is the set
of features being checked for and the relationships between the features.

Having the declarative part separated from the functional part is where
the value arises. 

 - Changes to the functional code apply consistently across all
   the checks made. For example, an improvement to error handling
   to regex processing was done once and applied to all the checks
   that used regex. Or consistently handling when reading files
   that are potentially missing on systems.

 - New checks can be introduced in a more reliable and consistent
   manner. These CPU vulnerability checks are an example of this
   in practice. We merely have to express what files we're interested
   in and how to match the relevant data in them. No need to worry
   about reading files, compiling & running regexs, making sure the
   error handling was done in the same way as other existing checks.

 - The declarative data can be processed algorithmically. For example
   given the depedancies expressed between the facts, I saw that it
   was not neccessary to define all the facts in the same data file.
   It was possible split them across many files, load those files in
   any order, and algorithmically determine exactly which subset of
   checks need to be executed & in what order.

 - Having separated data from the code it is obviously possible to
   extend this without introducing any code changes. This is possible
   to use from outside libvirt. For example there are usage scenarios
   which may not be supported by certain versions of QEMU. The QEMU
   package can drop in some facts which will be picked up by the
   virt-host-validate tool. Alternatively going up the stack, an
   app like OpenStack which uses libvirt may wish to restrict what
   they use, or wish to check extra virt host features interesting
   to their app's usage of libvirt. Again their package can now
   drop in extra facts.

 - New features can be built ontop of the declarative data. At first
   I just set out to replicate the existing tool's output as is. Once
   that was done, it was obvious that we could trivially add a new
   feature allowing us to print the raw informationm about each
   fact that was checked, as an alternative to the human targetted
   message strings. IOW we got a machine readable output format
   essentially for free by virtue of using declarative data.

 - The implementation can be rewritten again at will. If the original
   C code had this split of data vs processing logic, this conversion
   of langauge would have been even more compelling, as none of othe
   declarative data would have needed changing. I'm not expecting
   another rewrite, but this capability was already valuable, between
   the v1 and v2 posting of this patch I changed from XML to YAML for
   the data format. This conversion was entirely automated, because
   it could be algorithmically processed.

These are all typical & expected benefits that arise from separating
functional logic from declarative data. In the long term this is a
clear win, & as I mentioned I would have done this sooner had it not
been for our use of C. 

> If I was to pick a new feature we could benefit from, I would much rather prefer
> having an opt-in for report-home of HW features and usage for some very rough
> anonymous statistics.

I don't think we want to get involved in reporting user data back to any
server, as it is a data privacy minefield. This tool can be used by other
existing reporting tools though - eg Red Hat distros often include the
'sosreport' tool that bundles stuff up for reporting customer tickets.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux