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
