Re: [PATCH v6 11/11] arm64: annotate user pointers casts detected by sparse

[Catalin Marinas]

On Thu, Sep 06, 2018 at 02:16:19PM -0700, Linus Torvalds wrote:
> On Thu, Sep 6, 2018 at 2:13 PM Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > So for example:
> >
> > >  static inline compat_uptr_t ptr_to_compat(void __user *uptr)
> > >  {
> > > -       return (u32)(unsigned long)uptr;
> > > +       return (u32)(__force unsigned long)uptr;
> > >  }
> >
> > this actually looks correct.
> 
> Side note: I do think that while the above is correct, the rest of the
> patch shows that we might be better off simply not havign the warning
> for address space changes at all for the "cast a pointer to an integer
> type" case.
> 
> When you cast to a non-pointer type, the address space issue simply
> doesn't exist at all, so the warning makes less sense.

That's actually a new (potential) issue introduced by these patches. The
arm64 architecture has a feature called Top Byte Ignore (TBI, a.k.a.
tagged pointers) where the top 8-bit of a 64-bit pointer can be set to
anything and the hardware automatically ignores it when dereferencing.
The arm64 user/kernel ABI currently mandates that any pointer passed
from user space to the kernel must have the top byte 0.

This patchset is proposing to relax the ABI so that user pointers with a
non-zero top byte can be actually passed via kernel syscalls. It
basically moves the responsibility to remove the pointer tag (where
needed) from user to the kernel (and for some good reasons, user space
can't always do it given the way hwasan is implemented in LLVM).

The downside is that now a tagged user pointer may not represent just a
virtual address but address|tag, so things like access_ok() or
find_extended_vma() need to untag (clear the top byte of) the pointer
before use. Note that copy_from_user() etc. can safely dereference a
tagged user pointer as the tag is automatically ignored by the hardware.

The arm64 maintainers asked for a more reliable approach to identifying
existing and new cases where such explicit untagging is required and one
of the proposals was a sparse option. Based on some observations, it
seems that untagging is needed when a pointer is cast to a long and the
pointer tag information can be dropped. With the sparse patch, there are
lots of warnings where we actually can preserve the tag (e.g. compat
user pointers should be ignored since the top 32-bit are always 0), so
Andrey is trying to mask such warnings out so that we can detect new
potential issues as the kernel evolves.

So it's not about casting to another pointer; it's rather about no
longer using the value as a user pointer but as an actual (untyped,
untagged) virtual address.

There may be better options to address this but I haven't seen any
concrete proposal so far. Or we could simply consider that we've found
all places where it matters and not bother with any static analysis
tools (but for the time being it's still worth investigating whether we
can do better than this).

> It's really just he "pointer to one address space" being cast to
> "pointer to another address space" that should really warn, and that
> might need that "__force" thing.

I think sparse already warns if changing the address space of a pointer.

-- 
Catalin