On Fri, Aug 12 2016, Michael S. Tsirkin wrote:

> On Fri, Aug 12, 2016 at 03:23:28PM +1000, NeilBrown wrote:
>> On Fri, Aug 12 2016, Michael S. Tsirkin wrote:
>> 
>> > On Tue, Jul 19, 2016 at 10:32:51AM -0500, Eric W. Biederman wrote:
>> >> I would really like to get a feel among kernel maintainers and
>> >> developers if this is something that is interesting, and what kind of
>> >> constraints they think something like this would need to be usable for
>> >> the kernel?
>> >> 
>> >> Eric
>> >
>> > Surprised that no one mentioned this yet - I think tagging
>> > integers/structs as coming from userspace could be useful,
>> > if we can teach e.g. smatch that access to a kernel
>> > pointer through this offset might fault.
>> 
>> We already have that.
>> Sparse recognizes
>>     __attribute__((noderef, address_space(1)))
>>  to mean "this is a pointer to a different address space which
>>  cannot be dereferened" and linux has
>> 
>> # define __user                __attribute__((noderef, address_space(1)))
>> 
>> so if you mark a pointer as "__user", then sparse will complain
>> if you dereference it.
>> 
>> We've had this for over a decade :-)
>> 
>>   https://lwn.net/Articles/87538/
>> 
>> NeilBrown
>
>
> Of course, everyone uses these.  But what I mean is tagging index types:
>
> 	int data[256];
>
> 	int foo(u32 __user *ptr)
> 	{
> 		u32 i;
> 		if (get_user(i, ptr))
> 			return -EFAULT;
>
> 		data[i] = 0;
> 			^^^ security vulnerability
>
> 	}
>
> Above, i is coming from userspace and so must always be range-checked
> before it's used as an index.

Ahhh, I see.  Thanks spelling it out for me.


>
> Maybe we could change get_user return a tagged result: __from_user int.
> And have above warn because __from_user can not be assigned to plain
> int.
>
> Then rework the code along the following lines:
>
>
> 	int data[256];
>
> 	int force_range(__unsafe u32 value, unsigned idx)
> 	{
> 		return ((__force int)value) % idx;
> 	}
>
> 	int foo(u32 __user *ptr)
> 	{
> 		__unsafe u32 i;
> 		int ichecked;
> 		if (get_user(i, ptr))
> 			return -EFAULT;
>
> 		ichecked = force_range(i, sizeof data);
> 		data[ichecked] = 0;
> 			^^^ ok now
>
> 	}

You could probably do this today using __attribute__((bitwise))

typedef int __attribute__((bitwise)) unsafe32;

Then use "unsafe32" wherever you have "__unsafe u32".

When you try

  data[i] = 0;

sparse says "warning: restricted int degrades to integer"

Of course, changing all the code would be a pain.
I guess you introduce "get_safe_user" then gradually transition code
over.

NeilBrown