Re: [Qemu-devel] Re: linux-user issues

[malc <av1474@comtv.ru>]

On Sun, 14 Mar 2010, malc wrote:

> On Sun, 14 Mar 2010, Paul Brook wrote:
> 
> > > On Sat, 13 Mar 2010, Paul Brook wrote:
> > > > > a) elfload.c:859
> > > > >
> > > > >    #define TARGET_ELF_PAGESTART(_v) ((_v) & ~(unsigned
> > > > >  long)(TARGET_ELF_EXEC_PAGESIZE-1))
> > > > >
> > > > >    This means that for 64bit guest on a 32bit host the _v's value is
> > > > >    silently reduced to 32bit, the cast should be abi_ulong.
> > > > >
> > > > > b) mmap.c:428
> > > > >
> > > > >    real_start = start & qemu_host_page_mask;
> > > > >
> > > > >    Same thing basically qemu_host_page_mask is unsigned long and so
> > > > >    the upper bits are sliently cleared. Again qemu_host_page_mask
> > > > >    should probably be abi_ulong.
> > > > >
> > > > > The above two make elf binary with one of the segments above 4G load
> > > > > and run on 32bit guest only to fail well into execution and without
> > > > > any indication that it shouldn't have been allowed to run in the first
> > > > > place.
> > > >
> > > > I'd be amazed if these are the only two issues, and expect 64-bit guests
> > > > on 32-bit hosts to be generally unsafe.
> > > 
> > > These are the two issues i've been bitten by while trying to solve
> > > x86_64's far jumps, if you are okay with code silently misbehaving
> > > fine by me.
> > 
> > I never said this was ok behavior. Only that you've probably only discovered 
> > the tip of the iceberg.
> 
> Even on this path (mmap one) it's indeed not, further bellow there are
> yet more truncations due to improperly sized and's. Furthermore the
> abi_ulong sized `start' argument is passed to mmap without ever
> checking if it fits 32bit, question is what to do about it.

How about following? In the future munmap and mprotect needs to be
addressed similarly.

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 71ed2d6..25064dc 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -856,7 +856,7 @@ struct exec
 
 /* Necessary parameters */
 #define TARGET_ELF_EXEC_PAGESIZE TARGET_PAGE_SIZE
-#define TARGET_ELF_PAGESTART(_v) ((_v) & ~(unsigned long)(TARGET_ELF_EXEC_PAGESIZE-1))
+#define TARGET_ELF_PAGESTART(_v) ((_v) & ~(abi_ulong)(TARGET_ELF_EXEC_PAGESIZE-1))
 #define TARGET_ELF_PAGEOFFSET(_v) ((_v) & (TARGET_ELF_EXEC_PAGESIZE-1))
 
 #define INTERPRETER_NONE 0
diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index ad00b6f..c436709 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -417,6 +417,11 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
     }
 #endif
 
+    if (start > ULONG_MAX) {
+        errno = EINVAL;
+        goto fail;
+    }
+
     if (offset & ~TARGET_PAGE_MASK) {
         errno = EINVAL;
         goto fail;



-- 
mailto:av1474@comtv.ru