Messages in this thread | | | Subject | Re: Intel MIC host driver: possible signed underflow (undefined behavior) in userspace API | From | Sudeep Dutt <> | Date | Fri, 10 Jan 2014 10:22:55 -0800 |
| |
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
| |