Re: [PATCH] intel_iommu: Disable vfio and kvm interrupt assignment when unsafe

[Joerg Roedel <joro@8bytes.org>]

On Thu, Feb 07, 2013 at 09:53:45AM -0800, Andy Lutomirski wrote:
> On Thu, Feb 7, 2013 at 9:27 AM, Joerg Roedel <joro@8bytes.org> wrote:
> > Hmm, looking into the intel_irq_remapping.c version in the tip tree
> > makes me wonder even more.
> 
> Is this the version I'm based on (intel_irq_remapping: Clean up x2apic
> optout security warning mess), or something else?

The current tip/master branch with your previous patches included. Btw.
commit af8d102f99 (which introduces the CFIS check) is also bogus. It
claims to cleanup warning messages but does more, like explicitly
clearing DMA_GCMD_CFI in the global command register. This should have
been a seperate patch with a seperate commit-msg.

Now to the two warnings. First of:

        if (sts & DMA_GSTS_CFIS)
                WARN(1, KERN_WARNING
                        "Compatibility-format IRQs enabled despite intr remapping;\n"
                        "you are vulnerable to IRQ injection.\n");

This one can be removed completly as it will never trigger. The CFIS bit
you check here (according to the VT-d spec) has the same value as what
you write into GCMD.CFI (which you set to 0 earlier in the function).

The other warning:

        if (x2apic_present)
                WARN(1, KERN_WARNING
                        "Failed to enable irq remapping.  You are vulnerable to irq-injection attacks.\n");

Here you should remove the x2apic_present check as this is the
error-path for xapic and x2apic. The vulnerability exists with xapic
too.

> What's the general rule here?  If this warning hits, then my
> understanding of the Intel VT-d spec is wrong, and I don't think that
> firmware can cause it.  A buggy hypervisor could, I suppose.

A buggy hypervisor can not cause the CFIS-check warning. In my
understanding only broken hardware can cause it, but that would be known
already in the form of a hardware erratum. As I said above, after some
reading in the VT-d spec I think the warning can go away (Even clearing
GCMD.CFI can be removed as the value is never set in the VT-d driver, so
the bit is always written as 0 by Linux).

With this in mind, I also think that the patch this thread is about does
not make much sense. It just adds more code to handle a case that could
never happen.

Regards,

	Joerg