Re: [Qemu-devel] RFC: x86_64 Best way to fix 'cast to pointer from integer of different size' problems?

[Thayne Harbaugh]

On Sat, 2007-11-03 at 18:52 +0100, Paul Brook wrote:
> On Saturday 03 November 2007, TJ wrote:
> > I'm building on x86_64 GNU/Linux. There are *lots* of (1053) compiler
> > warnings of the class:
> >
> > warning: cast to pointer from integer of different size
> 
> There are at due to the recent EFAULT/access_ok changes. There should be (and 
> used to be) a clear separation between host and target addresses. The EFAULT 
> changes have broken this. Before these ghanges it wa trivial to remap the 
> target address space (e.g. place it at a high address on a 64-bit host), or 
> even enabling softmmu. I'm fairly sure that wouldn't work if you tried it 
> now.

No, the EFAULT/access_ok() didn't cause it exactly.  The problem is that
access_ok() came from kernel code and was dummied-out to always be true.
While it was turned off it was used with arguments that weren't
appropriate.  Turning on access_ok() caused all of the incorrect usages
to suddenly show up.  As an asside, the access_ok() is usually out of
order with the lock_user() calls - access_ok() should come first.

There were also additional incorrect pointer usages that have nothing to
do with EFAULT/access_ok() but usually don't get noticed when building
on 32bit arch.

> > Fixing it looks to require a variety of fixes, from simple explicit
> > casts in-line, to some complicated review of multiple levels of macros
> > to decide where best to apply a fix.
> 
> Adding a cast is never the right thing to do. There should *always* be a 
> clear 
> distinction and explicit conversion between host pointers and target 
> addresses. It is never safe to cast from one to the other.

Agreed.

> I put quite a lot of effort into getting this separation right when I 
> implemented the lock_user interfaces. It was fairly trivial to implement 
> address space translation (even softmmu) using this inerface. I'm quite 
> annoyed that we seem to have regresses so badly in this area. AFAICS the 
> whole of syscalls.c needs re-auditing to figure out which values are host 
> pointers and which are target addresses.

I've actually done the audit and have all the fixes queued to submit to
the list.

> We should have either lock_user or {get,put}_user, not both.

Now that's a bit of a discussion.  It's possible to push everything into
{get,put}_user() but I don't think that's quite appropriate.  Let's look
at everything that's going on:

1) page in cache with proper permissions
2) address translation

"1" is performed by access_ok() which returns true/false.  "2" is
performed by lock_user() which internally uses g2h() to perform the
address translation and returns the translated address.  lock_user() can
also do memory replication/flushing to test that the memory calls are
coded correctly even when the implementation doesn't actually perform
address translation.

access_ok() and lock_user() perform essential functions.  lock_user(),
however, isn't directly comparable to how the kernel operates and should
therefore be encapsulated inside more typical kernel functions such as
{get,put}_user(), copy_{to,from}_user() and the like.  access_ok() and
lock_user() also have overhead and should therefore be used with the
largest memory hunks possible (e.g.: they should be used with an entire
structure - not with each individual data member of the structure).
That is why __{get,put}_user() exist: for copying the individual data
members of a structure once the *entire* structure has had access
checked and the address translation is performed.

The clean-ups that I am sending will follow the following guidelines:

* abi_long/abi_ulong will be passed as function arguments at the
high-level and all direct syscall wrappers, as well as do_*() functions
will only receive those types from user space as well as will only
return abi_long types.

* type casts will be removed.

* all target/host interactions will happen through {get,put}_user() and
copy_{to,from}_user() just like in the kernel.  These will accept a
target address in an abi_ulong, do access_ok(), do lock_user() and map
the target address to a host address and then perform the get/put or
copy_to/from with the appropriate unlock_user() afterwords.

  * {get,put}_user() will be used for individual data types that are
passed.

  * copy_{to,from}_user_<struct>() will be used for structures:

static inline abi_long copy_from_user_flock(struct flock *host_fl,
                                            abi_ulong target_fl_addr)
{
    struct target_flock *target_fl;

    if (!access_ok(VERIFY_READ, target_fl_addr, sizeof(*target_fl)))
        return -TARGET_EFAULT;

    lock_user_struct(target_fl, target_fl_addr, 1);

    __get_user(host_fl->l_type, &(target_fl->l_type));
    __get_user(host_fl->l_whence, &(target_fl->l_whence));
    __get_user(host_fl->l_start, &(target_fl->l_start));
    __get_user(host_fl->l_len, &(target_fl->l_len));
    __get_user(host_fl->l_pid, &(target_fl->l_pid));

    lock_user_struct(target_fl, target_fl_addr, 0);

    return 0;
}


This effectively eliminates all {lock,unlock}_user() calls and g2h/h2g()
calls from the main syscall wrappers and emulation - the
{lock,unlock}_user() calls are localized to wrappers that marshal
between target and user.

This is the way that my source tree is and it makes things very clean.
The code is much more comparable to kernel code, both access and locking
are managed and compiler warnings are almost non-existent (I still have
a few minor clean-ups to do).  I don't think there's an appropriate way
to eliminate either {lock,unlock}_user() or {get,put}_user() and keep
comparable coding semantics to the kernel.