Re: [Xen-devel] [PATCH] VT-d: improve RMRR validity checking

[Noboru Iwamatsu <n_iwamatsu@xxxxxxxxxxxxxx>]

Hi,

> No, we cannot ignore it if iommu=force. The invisible device may be
> disabled, not really non-existent. it is possibly that it is re-enabled
> by malfunctional s/w. So when iommu=force, we should not ignore any
> DRHD. We ignores it just to workaround the BIOS issue you encountered.

OK, I return to the same question as Pasi asked.
You mean even ignoring the DRHD that has no existent devices is
insecure, right?
In other word, iommu=1 might be insecure while working with workaround.

We might have to consider security and BIOS workaround separately.
I believe default action must be secure and enabled with strictly
checked values.
If "force" or some workaround options (e.g ignore_bogus_rmrr,
ignore_bogus_drhd, force_enable_with_bogus_drhd, ...)
specified, VT-d enabled with some special workaround, with
uncertain values, but these mode should be considered
"not always secure".

What do you think?

Regards,
Noboru.

> Noboru Iwamatsu wrote:
> > Weidong,
> >
> > I read the patch and the following thread.
> >
> > I understood what you mean, but I think it's better to
> > limit the scope of "force_iommu".
> > And I believe RMRR should be checked as same as DRHD.
> >
> > What I thought about DRHD is:
> > If all devices under the scope of the DRHD are non-existent,
> > this DRHD is invalid but safely ignorable, so ignore it.
> No, we cannot ignore it if iommu=force. The invisible device may be
> disabled, not really non-existent. it is possibly that it is re-enabled
> by malfunctional s/w. So when iommu=force, we should not ignore any
> DRHD. We ignores it just to workaround the BIOS issue you encountered.
> > If some devices under the scope of the DRHD are non-existent,
> > this DRHD is invalid, so disable VT-d unless "iommu=force"
> > option is specified.
> > When "iommu=force" option is specified, even the invalid DRHD
> > will be registered, because DRHD that has some existent devices
> > must not be ignored due to security reasons.
> >
> > About the RMRR:
> > If all devices under the scope of the RMRR are non-existent,
> > this RMMR is invalid but ignorable, so ignore it.
> > If some devices under the scope of the RMRR are non-existent,
> > this RMRR is invalid, so disable VT-d unless "iommu=force"
> RMRR is much different from DRHD, it's just reversed memories for
> specific devices (now only Intel IGD and USB contollers need RMRR), it's
> no security issue like described above.
> if "all" devices under the scope of the RMRR are non-existent, we can
> ignore the RMRR because no devices will use it.
> if some" devices under the scope of the RMRR are non-existent, we cannot
> ignore the RMRR, because there are still some devices want to use it. I
> think we needn't to disable VT-d because it won't cause any issues. Of
> course, we also can disable VT-d for this case strictly.
> > option is specified. When "iommu=force" option is specified,
> > the invalid RMRR is ignored (it's safe).
> >
> > I attach the patch.
> >
> > What do you think?
> Noboru,
>
> I think it need not to change current code. BTW, your patch is not based
> on latest Xen.
>
> Regards,
> Weidong
>
>
> > Regards,
> > Noboru.
> >
> > > I implemented a patch and attached.
> > >
> > > patch description:
> > > In order to make Xen more defensive to VT-d related BIOS issue, this
> > > patch ignores a DRHD if all devices under its scope are not pci
> > > discoverable, and regards a DRHD as invalid and then disable whole VT-d
> > > if some devices under its scope are not pci discoverable. But if
> > > iommu=force is set, it will enable all DRHDs reported by BIOS, to avoid
> > > any security vulnerability with malicious s/s re-enabling "supposed
> > > disabled" devices. Pls note that we don't know the devices under the
> > > "Include_all" DRHD are existent or not, because the scope of
> > > "Include_all" DRHD won't enumerate common pci device, it only enumerates
> > > I/OxAPIC and HPET devices.
> > >
> > > Signed-off-by: Noboru Iwamatsu <n_iwamatsu@xxxxxxxxxxxxxx>
> > > Signed-off-by: Weidong Han <weidong.han@xxxxxxxxx>
> > >
> > >
> > > Noboru, pls test the patch on your machine?
> > >
> > > Joe, could you review the patch? and pls ACK it if it's fine for you.
> > >
> > > Regards,
> > > Weidong
> > >
> > > Noboru Iwamatsu wrote:
> > > > Thanks,
> > > >
> > > > I understood.
> > > >
> > > > > Noboru Iwamatsu wrote:
> > > > > > Hi Weidong,
> > > > > >
> > > > > > I'm not sure why the security problem is caused by ignoring
> > > > > > the DRHD that has only non-existent devices.
> > > > > >
> > > > > > Could you explain details or where to read the spec?
> > > > > It's requested from security experts. The device that is not pci
> > > > > discoverable may be re-enabled by malicious software. If its DRHD
> > > > > is not
> > > > > enabled, the re-enabled device is not protected by VT-d. It will cause
> > > > > security issue.
> > > > >
> > > > > > As you saying, security is the top-priority.
> > > > > > However, when iommu=force is specified, we should enable vt-d
> > > > > > if there are some potential issues.
> > > > > > Because users want to "force" anyway.
> > > > > iommu=force was introduced to enable VT-d anyway for security
> > > > > purpose. I
> > > > > plan to still enable those DRHDs that includes non-existed device when
> > > > > iommu=force, otherwise ignore them.
> > > > >
> > > > > Regards,
> > > > > Weidong
> > > > > > Regards,
> > > > > > Noboru.
> > > > > >
> > > > > > > Keir Fraser wrote:
> > > > > > > > If we want to keep iommu=1 as default, then it is unacceptable to
> > > > > > > > fail to
> > > > > > > > boot on a fairly wide range of modern systems. We have to
> > > > > > > > warn-and-disable,
> > > > > > > > partially or completely, unless iommu=force is specified. Or we
> > > > > > > > need to
> > > > > > > > revert to iommu=0 as the default.
> > > > > > > >
> > > > > > > > What do you think, Weidong?
> > > > > > > Yes. I agree to warn-and-disable for these BIOS issues, and consider
> > > > > > > security more when iommu=force. Therefore I will implement a patch
> > > > > > > based
> > > > > > > on Nororu's patch.
> > > > > > >
> > > > > > > Regards,
> > > > > > > Weidong
> > > > > > >
> > > > > > > > -- Keir
> > > > > > > >
> > > > > > > > On 21/01/2010 14:17, "Sander Eikelenboom" <linux@xxxxxxxxxxxxxx>
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > Hello Weidong,
> > > > > > > > >
> > > > > > > > > The problem is most vendor's just don't fix it and ignore the
> > > > > > > > > problem
> > > > > > > > > completely.
> > > > > > > > > Most often hiding them selves behind: come back when it's a
> > > > > > > > > problem
> > > > > > > > > with
> > > > > > > > > Microsoft Windows, that the only single thing we support (and no
> > > > > > > > > other
> > > > > > > > > software, so no vmware, no xen, no linux, perhaps even no
> > > > > > > > > hypervisor)
> > > > > > > > > Well I don't know if the virtual pc in windows 7 supports an iommu
> > > > > > > > > now, but it
> > > > > > > > > didn't in the past as far as i know, so any complain bounces off,
> > > > > > > > > and
> > > > > > > > > there it
> > > > > > > > > all seems to end for them.
> > > > > > > > >
> > > > > > > > > Besides that i don't know if they do know what the problems with
> > > > > > > > > there
> > > > > > > > > implementation in BIOS is when someone reports it.
> > > > > > > > > I think some behind the scenes pressure from Intel to vendors
> > > > > > > > > might
> > > > > > > > > help to
> > > > > > > > > solve some of them.
> > > > > > > > > (my Q35 chipset, "Intel V-PRO" marketed motherboard (so much for
> > > > > > > > > that) also
> > > > > > > > > suffers RMRR problem when another graphics card is inserted which
> > > > > > > > > switches off
> > > > > > > > > the IGD).
> > > > > > > > >
> > > > > > > > > Although i think in my case your patch will work around that
> > > > > > > > > for me.
> > > > > > > > > Perhaps a
> > > > > > > > > third option is needed, which does all the workarounds possible
> > > > > > > > > and
> > > > > > > > > warns
> > > > > > > > > about potential security problem when requested ?
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Sander
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Thursday, January 21, 2010, 1:46:39 PM, you wrote:
> > > > > > > > >
> > > > > > > > > > Noboru Iwamatsu wrote:
> > > > > > > > > > > Hi Weidong,
> > > > > > > > > > >
> > > > > > > > > > > I re-send the DRHD-fix patch.
> > > > > > > > > > >
> > > > > > > > > > > If DRHD does not have existent devices, ignore it.
> > > > > > > > > > > If DRHD has both existent and non-existent devices, consider it
> > > > > > > > > > > invalid
> > > > > > > > > > > and not register.
> > > > > > > > > > Although you patch workarounds your buggy BIOS, but we still
> > > > > > > > > > need to
> > > > > > > > > > enable it for security purpose as I mentioned in previous
> > > > > > > > > > mail. We
> > > > > > > > > > needn't workaround / fix all BIOS issues in software. I think
> > > > > > > > > > security
> > > > > > > > > > is more important for this specific BIOS issue. Did you report
> > > > > > > > > > the
> > > > > > > > > > BIOS
> > > > > > > > > > issue to your OEM vendor? maybe it's better to get it fixed in
> > > > > > > > > > BIOS.
> > > > > > > > > > Regards,
> > > > > > > > > > Weidong
> > > > > > > > > > > According to this patch and yours, my machine successfully
> > > > > > > > > > > booted
> > > > > > > > > > > with vt-d enabled.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Noboru Iwamatsu <n_iwamatsu@xxxxxxxxxxxxxx>
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > Keir Fraser wrote:
> > > > > > > > > > > > > On 21/01/2010 10:19, "Weidong Han" <weidong.han@xxxxxxxxx>
> > > > > > > > > > > > > wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > > > Sorry this is typo.
> > > > > > > > > > > > > > > I mean:
> > > > > > > > > > > > > > > So, I think RMRR that has no-existent device is "invalid"
> > > > > > > > > > > > > > > and whole RMRR should be ignored.
> > > > > > > > > > > > > > looks reasonable.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Keir, I Acks Noboru's rmrr patch. Or do you want us to merge
> > > > > > > > > > > > > > them to one
> > > > > > > > > > > > > > patch?
> > > > > > > > > > > > > Merge them up, re-send with both sign-off and acked-by all in
> > > > > > > > > > > > > one
> > > > > > > > > > > > > email.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Thanks,
> > > > > > > > > > > > > Keir
> > > > > > > > > > > > Sorry, I disagree with Noboru after thinking it again. If the
> > > > > > > > > > > > RMRR
> > > > > > > > > > > > has
> > > > > > > > > > > > both no-existent device and also has existent devices in its
> > > > > > > > > > > > scope, we
> > > > > > > > > > > > should not ignore it because the existent devices under its
> > > > > > > > > > > > scope
> > > > > > > > > > > > will
> > > > > > > > > > > > be impacted without the RMRR. so I suggest to print a warning
> > > > > > > > > > > > instead of
> > > > > > > > > > > > ignore it. Attached a patch for it.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Weidong Han <weidong.han@xxxxxxxxx>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel