All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jan Kiszka <jan.kiszka@siemens.com>
To: Andy Lutomirski <luto@amacapital.net>
Cc: "linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	xen-devel <Xen-devel@lists.xen.org>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	"linux390@de.ibm.com" <linux390@de.ibm.com>,
	Linux Virtualization <virtualization@lists.linux-foundation.org>
Subject: Re: [PATCH v4 0/4] virtio: Clean up scatterlists and use the DMA API
Date: Tue, 28 Jul 2015 21:06:17 +0200	[thread overview]
Message-ID: <55B7D2A9.6040700@siemens.com> (raw)
In-Reply-To: <CALCETrWUVQWQ=mydbdiSJnA0uLSFarVYCnWWKpcgYD0A8hpfXQ@mail.gmail.com>

On 2015-07-28 20:22, Andy Lutomirski wrote:
> On Tue, Jul 28, 2015 at 10:17 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2015-07-28 19:10, Andy Lutomirski wrote:
>>> The trouble is that this is really a property of the bus and not of
>>> the device.  If you build a virtio device that physically plugs into a
>>> PCIe slot, the device has no concept of an IOMMU in the first place.
>>
>> If one would build a real virtio device today, it would be broken
>> because every IOMMU would start to translate its requests. Already from
>> that POV, we really need to introduce a feature flag "I will be
>> IOMMU-translated" so that a potential physical implementation can carry
>> it unconditionally.
>>
> 
> Except that, with my patches, it would work correctly.  ISTM the thing

I haven't looked at your patches yet - they make the virtio PCI driver
in Linux IOMMU-compatible? Perfect - except for a compatibility check,
right?

> that's broken right now is QEMU and the virtio_pci driver.  My patches
> fix the driver.  Last year that would have been the end of the story
> except for PPC.  Now we have to deal with QEMU.
> 
>>> Similarly, if you take an L0-provided IOMMU-supporting device and pass
>>> it through to L2 using current QEMU on L1 (with Q35 emulation and
>>> iommu enabled), then, from L2's perspective, the device is 1:1 no
>>> matter what the device thinks.
>>>
>>> IOW, I think the original design was wrong and now we have to deal
>>> with it.  I think the best solution would be to teach QEMU to fix its
>>> ACPI tables so that 1:1 virtio devices are actually exposed as 1:1.
>>
>> Only the current drivers are broken. And we can easily tell them apart
>> from newer ones via feature flags. Sorry, don't get the problem.
> 
> I still don't see how feature flags solve the problem.  Suppose we
> added a feature flag meaning "respects IOMMU".
> 
> Bad case 1:  Build a malicious device that advertises
> non-IOMMU-respecting virtio.  Plug it in behind an IOMMU.  Host starts
> leaking physical addresses to the device (and the device doesn't work,
> of course).  Maybe that's only barely a security problem, but still...

I don't see right now how critical such a hypothetical case could be.
But the OS / its drivers could still decide to refuse talking to such a
device.

> 
> Bad case 2:  Use current QEMU w/ IOMMU enabled.  Assign a virtio
> device provided by L0 QEMU to L2.  L1 crashes.  I consider *that* to
> be a security problem, although in practice no one will configure
> their system that way because it has zero chance of actually working.
> Nonetheless, the device does work if L1 accesses it directly?  The
> issue is vfio doesn't notice that the device doesn't respect the IOMMU
> because "respects-IOMMU" is a property of the PCI bus and the platform
> IOMMU, and vfio assumes it works correctly.

I would have no problem with rejecting configurations in future QEMU
that try to expose unconfined virtio devices in the presence of IOMMU
emulation. Once we can do better, it's just about letting the guest know
about the difference.

The current situation is indeed just broken, we don't need to discuss
this as we can't change history to prevent this.

> 
> Bad case 2: Some hypothetical well-behaved new QEMU provides a virtio
> device that *does* respect the IOMMU and sets the feature flag.  They
> emulate Q35 with an IOMMU.  They boot Linux 4.1.  Data corruption in
> the guest.

No. In that case, the feature negotiation of "virtio-with-iommu-support"
would have failed for older drivers, and the device would have never
been used by the guest.

> 
> We could make the rule that *all* virtio-pci devices (except on PPC)
> respect the bus rules.  We'd have to fix QEMU so that virtio devices
> on Q35 iommu=on systems set up a PCI topology where the devices
> *aren't* behind the IOMMU or are protected by RMRRs or whatever.  Then
> old kernels would work correctly on new hosts, new kernels would work
> correctly except on old iommu-providing hosts, and Xen would work.

I don't see a point in doing anything about old QEMU with IOMMU enabled
and virtio devices plugged except declaring such setups broken. No one
should have configured this for production purposes, only for test
setups (like we, with the knowledge about the limitations).

> 
> In fact, on Xen, it's impossible without colossal hacks to support
> non-IOMMU-respecting virtio devices because Xen acts as an
> intermediate IOMMU between the Linux dom0 guest and the actual host.
> The QEMU host doesn't even know that Xen is involved.  This is why Xen
> and virtio don't currently work together (without my patches): the
> device thinks it doesn't respect the IOMMU, the driver thinks the
> device doesn't respect the IOMMU, and they're both wrong.
> 
> TL;DR: I think there are only two cases.  Either a device respects the
> IOMMU or a device doesn't know whether it respects the IOMMU.  The
> latter case is problematic.

See above, the latter is only problematic on setups that actually use an
IOMMU. If that includes Xen, then no one should use it until virtio can
declare itself IOMMU compatible, and drivers exist that process this.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

  parent reply	other threads:[~2015-07-28 19:06 UTC|newest]

Thread overview: 104+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-28  1:08 [PATCH v4 0/4] virtio: Clean up scatterlists and use the DMA API Andy Lutomirski
2015-07-28  7:05 ` Christian Borntraeger
2015-07-28  7:05 ` Christian Borntraeger
2015-07-28  8:16 ` Paolo Bonzini
2015-07-28  8:16 ` Paolo Bonzini
2015-07-28 10:12   ` Benjamin Herrenschmidt
2015-07-28 10:12   ` Benjamin Herrenschmidt
2015-07-28 12:46     ` Paolo Bonzini
2015-07-28 13:06       ` Michael S. Tsirkin
2015-07-28 13:06       ` Michael S. Tsirkin
2015-07-28 13:11         ` Jan Kiszka
2015-07-28 16:11           ` Andy Lutomirski
2015-07-28 16:44             ` Jan Kiszka
2015-07-28 16:44             ` Jan Kiszka
2015-07-28 17:10               ` Andy Lutomirski
2015-07-28 17:10               ` Andy Lutomirski
2015-07-28 17:17                 ` Jan Kiszka
2015-07-28 17:17                 ` Jan Kiszka
2015-07-28 18:22                   ` Andy Lutomirski
2015-07-28 18:22                   ` Andy Lutomirski
2015-07-28 19:06                     ` Jan Kiszka
2015-07-28 19:06                     ` Jan Kiszka [this message]
2015-07-28 19:24                       ` Andy Lutomirski
2015-07-28 19:24                       ` Andy Lutomirski
2015-07-28 19:33                         ` Jan Kiszka
2015-07-28 21:16                           ` Andy Lutomirski
2015-07-28 21:16                           ` Andy Lutomirski
2015-07-28 22:43                             ` Andy Lutomirski
2015-07-28 22:43                             ` Andy Lutomirski
2015-07-28 23:21                               ` Benjamin Herrenschmidt
2015-07-28 23:33                                 ` Andy Lutomirski
2015-07-28 23:33                                 ` Andy Lutomirski
2015-07-29  0:36                                   ` Benjamin Herrenschmidt
2015-07-29  0:36                                   ` Benjamin Herrenschmidt
2015-07-29  0:47                                     ` Andy Lutomirski
2015-07-29  0:47                                     ` Andy Lutomirski
2015-07-29  0:54                                       ` Benjamin Herrenschmidt
2015-07-29  0:54                                       ` Benjamin Herrenschmidt
2015-07-29  8:17                                       ` Paolo Bonzini
2015-07-29  8:20                                         ` Jan Kiszka
2015-07-29  8:20                                         ` Jan Kiszka
2015-07-29  9:21                                         ` Benjamin Herrenschmidt
2015-07-29  9:21                                         ` Benjamin Herrenschmidt
2015-07-29  8:17                                       ` Paolo Bonzini
2015-07-29  8:07                                 ` Jan Kiszka
2015-07-29  8:07                                 ` Jan Kiszka
2015-07-28 23:21                               ` Benjamin Herrenschmidt
2015-07-28 19:33                         ` Jan Kiszka
2015-07-28 16:11           ` Andy Lutomirski
2015-07-28 16:36           ` Paolo Bonzini
2015-07-28 16:36           ` Paolo Bonzini
2015-07-28 16:42             ` Jan Kiszka
2015-07-28 17:15               ` Paolo Bonzini
2015-07-28 17:15               ` Paolo Bonzini
2015-07-28 17:19                 ` Jan Kiszka
2015-07-28 17:19                 ` Jan Kiszka
2015-07-28 17:31                   ` Paolo Bonzini
2015-07-28 17:31                   ` Paolo Bonzini
2015-07-28 16:42             ` Jan Kiszka
2015-07-28 13:11         ` Jan Kiszka
2015-07-28 12:46     ` Paolo Bonzini
2015-07-28 13:08 ` Michael S. Tsirkin
2015-07-28 13:08 ` Michael S. Tsirkin
  -- strict thread matches above, loose matches on Subject: below --
2015-07-28  1:08 Andy Lutomirski
2014-09-01 17:39 Andy Lutomirski
2014-09-01 22:16 ` Benjamin Herrenschmidt
2014-09-02  5:55   ` Andy Lutomirski
2014-09-02 20:53     ` Benjamin Herrenschmidt
2014-09-02 20:56       ` Konrad Rzeszutek Wilk
2014-09-02 21:08         ` Benjamin Herrenschmidt
2014-09-02 21:37       ` Andy Lutomirski
2014-09-02 22:10         ` Benjamin Herrenschmidt
2014-09-02 23:11           ` Andy Lutomirski
2014-09-02 23:20             ` Benjamin Herrenschmidt
2014-09-02 23:42               ` Andy Lutomirski
2014-09-03  0:25                 ` Benjamin Herrenschmidt
2014-09-03  0:32                   ` Andy Lutomirski
2014-09-03  0:43                     ` Benjamin Herrenschmidt
2014-09-04  2:03                       ` Andy Lutomirski
2014-09-03  7:47                   ` Paolo Bonzini
2014-09-03  7:52                     ` Andy Lutomirski
2014-09-03  8:01                       ` Paolo Bonzini
2014-09-03  8:05                     ` Benjamin Herrenschmidt
2014-09-03 12:11                       ` Paolo Bonzini
2014-09-03 15:07                         ` Andy Lutomirski
2014-09-03 15:11                           ` Paolo Bonzini
2014-09-03 16:39                           ` Michael S. Tsirkin
2014-09-03 20:38                             ` Andy Lutomirski
2014-09-03  7:43               ` Paolo Bonzini
2014-09-03  6:42         ` Rusty Russell
2014-09-03  7:50           ` Andy Lutomirski
2014-09-05  2:31             ` Rusty Russell
2014-09-05  2:57               ` Andy Lutomirski
2014-09-05  5:20                 ` Benjamin Herrenschmidt
2014-09-05  7:33                 ` Christian Borntraeger
2014-09-10 15:36                 ` Christopher Covington
2014-09-10 16:15                   ` Andy Lutomirski
2014-09-05  5:16               ` Benjamin Herrenschmidt
2014-09-14  8:58               ` Michael S. Tsirkin
2014-09-03 12:51           ` Michael S. Tsirkin
2014-09-05  2:32             ` Rusty Russell
2014-09-05  3:06               ` Andy Lutomirski
2014-09-02 21:10     ` Michael S. Tsirkin
2014-09-02 21:49       ` Andy Lutomirski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=55B7D2A9.6040700@siemens.com \
    --to=jan.kiszka@siemens.com \
    --cc=Xen-devel@lists.xen.org \
    --cc=benh@kernel.crashing.org \
    --cc=borntraeger@de.ibm.com \
    --cc=konrad.wilk@oracle.com \
    --cc=linux-s390@vger.kernel.org \
    --cc=linux390@de.ibm.com \
    --cc=luto@amacapital.net \
    --cc=mst@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=virtualization@lists.linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.