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

[Vincenzo Frascino]

On 05/09/18 20:03, Luc Van Oostenryck wrote:
> On Tue, Sep 04, 2018 at 12:27:23PM +0100, Vincenzo Frascino wrote:
>> On 03/09/18 16:10, Luc Van Oostenryck wrote:
>>> On Mon, Sep 03, 2018 at 02:49:38PM +0100, Vincenzo Frascino wrote:
>>>> On 03/09/18 13:34, Andrey Konovalov wrote:
>>>>> On Fri, Aug 31, 2018 at 3:42 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>>>>>> On Fri, Aug 31, 2018 at 10:11:24AM +0200, Luc Van Oostenryck wrote:
>>>>>>> On Thu, Aug 30, 2018 at 01:41:16PM +0200, Andrey Konovalov wrote:
>>>>>>>> This patch adds __force annotations for __user pointers casts detected by
>>>>>>>> sparse with the -Wcast-from-as flag enabled (added in [1]).
>>>>>>>>
>>>>>>>> [1] https://github.com/lucvoo/sparse-dev/commit/5f960cb10f56ec2017c128ef9d16060e0145f292
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> It would be nice to have some explanation for why these added __force
>>>>>>> are useful.
>>>>>
>>>>> I'll add this in the next version, thanks!
>>>>>
>>>>>>         It would be even more useful if that series would either deal with
>>>>>> the noise for real ("that's what we intend here, that's what we intend there,
>>>>>> here's a primitive for such-and-such kind of cases, here we actually
>>>>>> ought to pass __user pointer instead of unsigned long", etc.) or left it
>>>>>> unmasked.
>>>>>>
>>>>>>         As it is, __force says only one thing: "I know the code is doing
>>>>>> the right thing here".  That belongs in primitives, and I do *not* mean the
>>>>>> #define cast_to_ulong(x) ((__force unsigned long)(x))
>>>>>> kind.
>>>>>>
>>>>>>         Folks, if you don't want to deal with that - leave the warnings be.
>>>>>> They do carry more information than "someone has slapped __force in that place".
>>>>>>
>>>>>> Al, very annoyed by that kind of information-hiding crap...
>>>>>
>>>>> This patch only adds __force to hide the reports I've looked at and
>>>>> decided that the code does the right thing. The cases where this is
>>>>> not the case are handled by the previous patches in the patchset. I'll
>>>>> this to the patch description as well. Is that OK?
>>>>>
>>>> I think as well that we should make explicit the information that
>>>> __force is hiding.
>>>> A possible solution could be defining some new address spaces and use
>>>> them where it is relevant in the kernel. Something like:
>>>>
>>>> # define __compat_ptr __attribute__((noderef, address_space(5)))
>>>> # define __tagged_ptr __attribute__((noderef, address_space(6)))
>>>>
>>>> In this way sparse can still identify the casting and trigger a warning.
>>>>
>>>> We could at that point modify sparse to ignore these conversions when a
>>>> specific flag is passed (i.e. -Wignore-compat-ptr, -Wignore-tagged-ptr)
>>>> to exclude from the generated warnings the ones we have already dealt
>>>> with.
>>>>
>>>> What do you think about this approach?
>>>
>>> I'll be happy to add such warnings to sparse if it is useful to detect
>>> (and correct!) problems. I'm also thinking to other possiblities, like
>>> having some weaker form of __force (maybe simply __force_as (which will
>>> 'only' force the address space) or even __force_as(TO, FROM) (with TO
>>> and FROM being a mask of the address space allowed).I believe we need something here to address this type of problems and I like
>> your proposal of adding a weaker force in the form of __force_as(TO, FROM)
>> because I think it provides the right level information. 
>>
>>> However, for the specific situation here, I'm not sure that using
>>> address spaces is the right choice because I suspect that the concept
>>> of tagged pointer is orthogonal to the one of (the usual) address space
>>> (it won't be possible for a pointer to be __tagged_ptr *and* __user).
>> I was thinking to address spaces because the information seems easily accessible
>> in sparse [1], but I am certainly open to any solution that can be semantically
>> more correct.
> 
> Yes, adding a new address space is easy (and doesn't need any modification
> to sparse). Here, I think adding a new 'modifier' __tagged (much like
> __nocast, __noderef, ...) would be much more appropriate.
> 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.  
  
>>>
>>> OTOH, when I see already the tons of warnings for concepts established
>>> since many years (I'm thinking especially at __bitwise, see [1]) I'm a
>>> bit affraid of adding new, more specialized ones that people will
>>> understand even less how/when they need to use them.
>> Thanks for providing this statistic, it is very interesting. I understand your
>> concern, but I think that in this case we need a more specialized option not only
>> to find potential problems but even to provide the right amount of information
>> to who reads the code. 
>>
>> A solution could be to let __force_as(TO, FROM) behave like __force and silence
>> the warning by default, but have an option in sparse to re-enable it 
>> (i.e. -Wshow-force-as). 
> 
> That would be, indeed, a simple solution but IMO even more dangerous than
> __force itself (since by readingthe code with this annotation  people would 
> naturally think it only involves the AS will in fact it would be the same
> as __force). I prefer to directly implement a plain __force_as, forcing
> only the AS).
>  

Agreed, even if we can't stop people from overlooking at things, I believe that
we will all benefit if they are clearer.

>> [1]
>> ---
>> commit ee7985f0c2b29c96aefe78df4139209eb4e719d8
>> Author: Vincenzo Frascino <vincenzo.frascino@xxxxxxx>
>> Date:   Wed Aug 15 10:55:44 2018 +0100
>>
>>     print address space number for explicit cast to ulong
>>     
>>     This patch build on top of commit b34880d ("stricter warning
>>     for explicit cast to ulong") and prints the address space
>>     number when a "warning: cast removes address space of expression"
>>     is triggered.
>>     
>>     This makes easier to discriminate in between different address
>>     spaces.
>>     
>>     A validation example is provided as well as part of this patch.
>>     
>>     Signed-off-by: Vincenzo Frascino <vincenzo.frascino@xxxxxxx>
>>
>> diff --git a/evaluate.c b/evaluate.c
>> index 6d5d479..2fc0ebc 100644
>> --- a/evaluate.c
>> +++ b/evaluate.c
>> @@ -3017,8 +3017,12 @@ static struct symbol *evaluate_cast(struct expression *expr)
>>  		sas = stype->ctype.as;
>>  	}
>>  
>> -	if (!tas && sas > 0)
>> -		warning(expr->pos, "cast removes address space of expression");
>> +	if (!tas && sas > 0) {
>> +		if (Wcast_from_as)
>> +			warning(expr->pos, "cast removes address space of expression (<asn:%d>)", sas);
>> +		else
>> +			warning(expr->pos, "cast removes address space of expression");
>> +	}
> 
> I think that the if (Wcast_from_as) is unneeded, the <asn:%d> can be added
> even if Wcast_from_as is false. Woukd it be OK for you?
> 

Yes, it is OK for me (I put it there because I did not know if we wanted to preserve the
original behavior). Feel free to hack my patch if you want to put it on your tree.
Thanks.

> -- Luc
> 

-- 
Regards,
Vincenzo