Re: [Qemu-devel] [PATCH for-1.7 v2 0/8] fix address space size issue

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Nov 07, 2013 at 03:08:35PM +0100, Paolo Bonzini wrote:
> Il 07/11/2013 11:41, Marcel Apfelbaum ha scritto:
> > A bug reported by Luiz Capitulino let us to find
> > several bugs in memory address space setup.
> > 
> > One issue is that gdb stub can give us arbitrary addresses
> > and we'll try to access them.
> > Since our lookup ignored high bits in the address,
> > we hit a wrong section and got a crash.
> > In fact, PCI devices can access arbitrary addresses too,
> > so we should just make lookup robust against this case.
> > 
> > Another issue has to do with size of regions.
> > memory API uses UINT64_MAX so say "all 64 bit" but
> > some devices mistakenly used INT64_MAX.
> > 
> > It should not affect most systems in practice as
> > everything should be limited by address space size,
> > but it's an API misuse that we should not keep around,
> > and it will become a problem if a system with 64 bit
> > target address hits this path.
> > 
> > Patch 1 introduces TARGET_PHYS_ADDR_SPACE_MAX that is
> >         the max size for memory regions rendered by exec. 
> > Patches 2-3 limits the size of memory regions used by exec.c.
> > Patch 4 fixes an actual bug.
> > The rest of patches make code cleaner and more robust.
> 
> I think this is wrong.
> 
> There is no reason why boards should be using TARGET_PHYS_ADDR_SPACE_MAX
> if the PCI address space is 64-bit.  Many files in hw/ do not even have
> access to TARGET_PHYS_ADDR_SPACE_MAX, which is only available to files
> that are compiled per-target.
> 
> The fact that you have to reduce the IOMMU address spaces from 64 bits
> (UINT64_MAX, not the buggy INT64_MAX) to something else is a red flag.
> If the IOMMU supports 64-bit addressing, the IOMMU regions should have
> 2^64 bits as their length.  Yes, it works by chance now, but it _does_
> work for 2^64-bit size regions: you can do DMA to a high address (say
> 0xf << 60), the IOMMU tables will convert to a low address that fits
> within TARGET_PHYS_ADDR_SPACE_BITS, and the everything will be fine.
> Patches 4 and 6 change that.
> 
> So, ack for patch 5-7-8, which should also be enough to fix the problem
> that Luiz reported.

Not at all. As long as exec.c ignores high bits, any access
there will end up in the wrong region.
We might not get a crash but we'll get guest memory corruption.
Not good.

>  For everything else the solution is to make memory
> dispatch use the whole 64 bits, as in
> http://permalink.gmane.org/gmane.comp.emulators.qemu/240229.  If you
> want to delay that to 1.8 that's fine.
> 
> Paolo
> 
> > 
> > Marcel Apfelbaum (3):
> >   exec: declare TARGET_PHYS_ADDR_SPACE_MAX to limit memory regions
> >     rendered by exec
> >   hw/alpha: limit iommu-typhoon memory size
> >   hw/ppc: limit iommu-spapr memory size
> > 
> > Michael S. Tsirkin (4):
> >   exec: don't ignore high address bits on lookup
> >   pci: fix address space size for bridge
> >   exec: don't ignore high address bits on set
> >   spapr_pci: s/INT64_MAX/UINT64_MAX/
> > 
> > Paolo Bonzini (1):
> >   pc: s/INT64_MAX/UINT64_MAX/
> > 
> >  exec.c                        | 18 ++++++++++++++++++
> >  hw/alpha/typhoon.c            |  2 +-
> >  hw/i386/pc_piix.c             |  2 +-
> >  hw/i386/pc_q35.c              |  2 +-
> >  hw/pci/pci_bridge.c           |  2 +-
> >  hw/ppc/spapr_iommu.c          |  2 +-
> >  hw/ppc/spapr_pci.c            |  2 +-
> >  include/exec/address-spaces.h |  4 ++++
> >  8 files changed, 28 insertions(+), 6 deletions(-)
> >