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

[Weidong Han <weidong.han@xxxxxxxxx>]

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