Re: Intel MIC host driver: possible signed underflow (undefined behavior) in userspace API

[Sudeep Dutt <sudeep.dutt@intel.com>]

On Fri, 2014-01-10 at 06:21 -0800, Greg Kroah-Hartman wrote:
> On Fri, Jan 10, 2014 at 05:56:25AM +0000, Mathieu Desnoyers wrote:
> > Hi,
> > 
> > Looking at this commit:
> > 
> > commit f69bcbf3b4c4b333dcd7a48eaf868bf0c88edab5
> > Author: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > Date:   Thu Sep 5 16:42:18 2013 -0700
> > 
> >     Intel MIC Host Driver Changes for Virtio Devices.
> > 
> > Especially at:
> > 
> > +struct mic_copy_desc {
> > +#ifdef __KERNEL__
> > +       struct iovec __user *iov;
> > +#else
> > +       struct iovec *iov;
> > +#endif
> > +       int iovcnt;
> > +       __u8 vr_idx;
> > +       __u8 update_used;
> > +       __u32 out_len;
> > +};
> > 
> > Seeing iovcnt being declared as a signed integer seems strange. The
> > first question would be: why is it signed rather than unsigned ?
> > 
> > Then, looking further into 
> > 
> > drivers/misc/mic/host/mic_virtio.c:_mic_virtio_copy()
> > 
> > We can see that the while() loop iterates until the local variable
> > iovcnt reaches the value 0 (and iovcnt is also a signed integer). If
> > user-space passes e.g. INT_MIN as iovcnt field, this loop then appears
> > to depend on an undefined behavior (signed underflow) to complete.
> > Wouldn't it be better to use an unsigned integers both in the
> > userspace API and for the local variable ?
> 
> Better yet, it should be a "__" type variable, as "int" doesn't mean
> much when crossing the user/kernel boundry...
> 

We had designed the interface to be similar to readv(..)/writev(..)
which takes an integer iovcnt. However, the suggestion to use __u32
works nicely since it avoids adding the missing integer parameter
validation in the driver. We will post a patch incorporating this
feedback for the next kernel release.

Thanks to Mathieu for reporting this issue.

Sudeep Dutt