Messages in this thread | | | Date | Fri, 10 Jan 2014 06:21:16 -0800 | From | Greg Kroah-Hartman <> | Subject | Re: Intel MIC host driver: possible signed underflow (undefined behavior) in userspace API |
| |
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...
thanks,
greg k-h
| |