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

[Luc Van Oostenryck <luc.vanoostenryck@gmail.com>]

On Thu, Sep 06, 2018 at 03:13:16PM +0100, Vincenzo Frascino wrote:
> On 05/09/18 20:03, Luc Van Oostenryck wrote:
> > I think that at this point, it would be nice to have a clear description
> > of the problem and what sort of checks are wanted.
> >
> 
> 
> The problem we are trying to address here is to identify when the user pointers
> are cast to integer types and to sanitize (when required) the kernel, when this
> happens.
> 
> The way on which we are trying to address this problem based on what Andrey
> proposed in his patch-set is to use the Top Byte Ignore feature (which is a 64 bit
> specific feature).
> 
> Based on what I said I think that we require 2 'modifiers':
> - __compat (or __compat_ptr) used when the kernel is dealing with user compat 
> pointers (32 bit, they can not be tagged). It should behave like force
> (silence warnings), but having something separate IMO makes more clear the
> intention of what we are trying to do.
> - __tagged (or __tagged_ptr) used when the kernel is dealing with user normal
> pointers (which can be tagged). In this case sparse should still be able to trigger
> a warning (that can be disabled by default as I was proposing in my previous email).
> When we put a tagged identifier we declare that we analyzed the code impacted by
> the conversion and eventually sanitized it. Having the warning still there allows us
> or whoever is looking at the code to always go back to the identified issue.  

OK. Thanks for the explanation.

So, the way I see things from a type checking perspective, is that
'being (potentially) tagged' is a new property of values, othogonal
the the concept of address space. Leaving the other address spaces
(__iomem, __percpu & __rcu) aside, it should be possible to have
__user & __kernel tagged pointers as well as tagged ulongs:
	__user __tagged *
	__kernel __tagged *
	ulong __tagged
in addition of the usuals:
	__user *
	__kernel *
	ulong
But some of them are banished or meaningless:
	__user *            (all __user pointers are potentially tagged)
	__kernel __tagged * (tags are only for user space)
	ulong __tagged      (pointers need to be untagged during conversion)
So, only the followings remain:
	__user __tagged *
	__kernel *
	ulong
and the property '__tagged' becomes equivalent to '__user'.
Thus '__tagged' can be implicit and this would have the advantage
of not needing to change any annotations.

Since the conversion '__user *' to '__kernel *' is already covered
by the default sparse warnings, only the conversion '__user' to
'ulong' need to be covered (and this is already covered by the new
option -Wcast-from-as) but that is only fine for detection. After
detection and auditing, several solution are possible:
1) simply add '__force' in the cast (this is very bad)
2) moving this '__force' inside a macro '__untag_ptr(x)' would already
   more acceptable but is fundamentaly the same as 1)
3) a weaker form of '__force', '__force_as', will do the trick nicely
   as long as __user is equated to __tagged (and could be useful on
   its own but could also hide real AS conversion problems).
4) a more specific solution would be to effectively add a new attribute,
   '__tagged', to define '__user' like:
	#define __user attribute((noderef,address_space(1),tagged))
   and have something like '__untag', a weaker form of __force meaning:
   "I know what I'm doing regarding conversion from 'tagged'". 

Neither 3) nor 4) should be much work but while I firmly think that
4) is the most correct solution, I'm not sure it's worth the added
complexity, certainly if KHWASAN is not meant to be upstreamed.

For the compat pointers, I'm less sure to understand the situation:
even if they can't be tagged, treating them as the other __user
pointers will still be OK (but I understand that it could be
interesting to be able to track them, it's just that it's independent
from the __tagged property).


-- Luc