[RFC] mips ptrace32.c and compat_ptr()

[RFC] mips ptrace32.c and compat_ptr()

[Al Viro]

	There's something odd going on:

1)
static inline void __user *compat_ptr(compat_uptr_t uptr)
{
        /* cast to a __user pointer via "unsigned long" makes sparse happy */
        return (void __user *)(unsigned long)(long)uptr;
}

Huh?  The first impression is that it wants to sign-extend
the argument, but... compat_uptr_t is and always had been
unsigned.  Initially it went
+typedef u32            compat_uptr_t;
+
+static inline void *compat_ptr(compat_uptr_t uptr)
+{
+       return (void *)(long)uptr;
+}
so there never had been any sign-extension in that thing.
So what is that cast to long in the current version about?

2)
long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
                        compat_ulong_t caddr, compat_ulong_t cdata)
{
        int addr = caddr;
        int data = cdata;
...
                ret = put_user(tmp, (u32 __user *) (unsigned long) data);
                break;
and quite a few similar in the same function.  Here we _do_ get sign
extension.  Which is bloody odd, seeing that all other compat syscalls
end up using compat_ptr(), either explicitly or in the syscall glue.

Shouldn't it be doing
                ret = put_user(tmp, (u32 __user *)compat_ptr(cdata));
                break;
instead?

Re: [RFC] mips ptrace32.c and compat_ptr()

[Paul Burton]

Hi Al,

On Mon, Apr 01, 2019 at 01:01:04AM +0100, Al Viro wrote:
> 	There's something odd going on:
> 
> 1)
> static inline void __user *compat_ptr(compat_uptr_t uptr)
> {
>         /* cast to a __user pointer via "unsigned long" makes sparse happy */
>         return (void __user *)(unsigned long)(long)uptr;
> }
> 
> Huh?  The first impression is that it wants to sign-extend
> the argument, but... compat_uptr_t is and always had been
> unsigned.  Initially it went
> +typedef u32            compat_uptr_t;
> +
> +static inline void *compat_ptr(compat_uptr_t uptr)
> +{
> +       return (void *)(long)uptr;
> +}
> so there never had been any sign-extension in that thing.
> So what is that cast to long in the current version about?

Good question. It looks like it's been there ever since it was added in
the separate arch/mips64 [1] & then got carried forwards into arch/mips.

The only thing I can think up is that perhaps Ralf added the cast to
long in order to avoid warnings from -Wint-to-pointer-cast, and ought to
have used unsigned long but didn't & got away with it. Later sparse
warned about the cast & we got the band-aid of the extra cast from long
to unsigned long [2], which ought to have replaced rather than
supplemented the cast to long. All speculative of course, the git
history isn't really descriptive enough to say anything for sure...

> 2)
> long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
>                         compat_ulong_t caddr, compat_ulong_t cdata)
> {
>         int addr = caddr;
>         int data = cdata;
> ...
>                 ret = put_user(tmp, (u32 __user *) (unsigned long) data);
>                 break;
> and quite a few similar in the same function.  Here we _do_ get sign
> extension.  Which is bloody odd, seeing that all other compat syscalls
> end up using compat_ptr(), either explicitly or in the syscall glue.
> 
> Shouldn't it be doing
>                 ret = put_user(tmp, (u32 __user *)compat_ptr(cdata));
>                 break;
> instead?

This is weird, and I need to go wrap my head around it. Worrying &
confusing me further is the comment above compat_ptr:

> A pointer passed in from user mode. This should not
> be used for syscall parameters, just declare them
> as pointers because the syscall entry code will have
> appropriately converted them already.

I initially thought it was trying to get at the fact that 32b user code
ought to be providing sign extended addresses anyway since MIPS32
instructions generally sign extend when run on a MIPS64 CPU, but
actually arch/mips/kernel/scall64-o32.S implicitly sign extends
arguments (using sll for the first 4 args in a0-a3, and the
sign-extending lw instruction for any further arguments on the stack).

So perhaps this is what that comment is referring to, and it's saying
that syscall entry points should just declare the arguments as pointers
since scall64-o32.S will have sign extended addresses already anyway
which makes explicitly trying to sign extend using compat_ptr() a no-op
(even if it did some sign extension).

That implicit sign extension is not done for n32, only o32. And this is
despite the fact that n32 is where I'd expect us to be more likely to
have non-sign-extended addresses since we'll have MIPS64 instructions in
the mix that could more easily generate such addresses.

And of course that only accounts for pointers passed directly as
arguments to syscalls - a pointer in a struct will still need sign
extension & we seem to have various bits of code scattered around doing
that sort of thing that probably ought to use compat_ptr() if it worked
properly.

I suppose we're getting away with this purely because of the typical
2GB/2GB user/kernel split of the MIPS32 address space. If users never
have pointers with bit 31 set then sign extending or zero extending is
all the same thing, and compat_ptr() doing zero extension becomes
harmless.

I need to give this some further thought though & it feels like
something we can clean up for the sake of sanity if nothing else.

Thanks,
    Paul

[1] https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/include/asm-mips64/compat.h?id=f19e2d9e9eb4
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/asm-mips/compat.h?id=01bebc66793f