Re: [Ksummit-discuss] [CORE TOPIC] More useful types in the linux kernel

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Aug 12, 2016 at 04:17:36PM +1000, NeilBrown wrote:
> 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"

Yes, but inability to do math on bitwise integers is rather
annoying.

E.g. if index is in bytes:

	int j = i / sizeof(*data);

will warn even though nothing bad is going on.


Maybe we could benefit from a separate
integer type, such that unsafe integers can interact
with safe (regular) integers, resulting in unsafe
integers.

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