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

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

[Mathieu Desnoyers]

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 ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

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

[Greg Kroah-Hartman]

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

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

[Sudeep Dutt]

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

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

[Greg Kroah-Hartman]

On Fri, Jan 10, 2014 at 10:22:55AM -0800, Sudeep Dutt wrote:
> 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;
> > > +};

Oh, there's also the obvious 32/64 bit userspace/kernel issue here as
well, but I'm sure you all know how to handle that properly.  It would
be nice to fix that up, if you can change the ABI still.

thanks,

greg k-h