Input: evdev - add event-mask API
diff mbox

Message ID 20151025001709.GA27533@dtor-pixel
State Accepted
Headers show

Commit Message

Dmitry Torokhov Oct. 25, 2015, 12:17 a.m. UTC
Hi David,

On Thu, Sep 03, 2015 at 06:14:01PM +0200, David Herrmann wrote:
> +static int bits_from_user(unsigned long *bits, unsigned int maxbit,
> +			  unsigned int maxlen, const void __user *p, int compat)
> +{
> +	int len;
> +
> +#if IS_ENABLED(CONFIG_COMPAT)
> +	if (compat) {
> +		if (maxlen % sizeof(compat_long_t))
> +			return -EINVAL;
> +		len = BITS_TO_LONGS_COMPAT(maxbit) * sizeof(compat_long_t);
> +	} else
> +#endif
> +	{
> +		if (maxlen % sizeof(long))
> +			return -EINVAL;
> +		len = BITS_TO_LONGS(maxbit) * sizeof(long);
> +	}
> +
> +	if (len > maxlen)
> +		len = maxlen;
> +
> +#if IS_ENABLED(CONFIG_COMPAT) && defined(__BIG_ENDIAN)
> +	if (compat) {
> +		int i;
> +
> +		for (i = 0; i < len / sizeof(compat_long_t); i++)
> +			if (copy_from_user((compat_long_t *) bits +
> +						i + 1 - ((i % 2) << 1),
> +					   (compat_long_t __user *) p + i,
> +					   sizeof(compat_long_t)))
> +				return -EFAULT;
> +		if (i % 2)
> +			*((compat_long_t *) bits + i - 1) = 0;
> +	} else
> +#endif
> +		if (copy_from_user(bits, p, len))
> +			return -EFAULT;
> +
> +	return len;
> +}

I do not quite like how we sprinkle ifdefs throughout, I prefer the way
we have bits_to_user defined, even if it is more verbose.


> +
>  static int str_to_user(const char *str, unsigned int maxlen, void __user *p)
>  {
>  	int len;
> @@ -854,6 +953,86 @@ static int evdev_revoke(struct evdev *evdev, struct evdev_client *client,
>  	return 0;
>  }
>  
> +/* must be called with evdev-mutex held */
> +static int evdev_set_mask(struct evdev_client *client,
> +			  unsigned int type,
> +			  const void __user *codes,
> +			  u32 codes_size,
> +			  int compat)
> +{
> +	unsigned long flags, *mask, *oldmask;
> +	size_t cnt, size, min;
> +	int error;
> +
> +	/* we allow unknown types and 'codes_size > size' for forward-compat */
> +	cnt = evdev_get_mask_cnt(type);
> +	if (!cnt)
> +		return 0;
> +
> +	size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
> +	min = min_t(size_t, codes_size, size);
> +
> +	mask = kzalloc(size, GFP_KERNEL);
> +	if (!mask)
> +		return -ENOMEM;
> +
> +	error = bits_from_user(mask, cnt - 1, min, codes, compat);

I do not think we need to calculate and pass min here: bits_from_user()
will limit the output for us already.

Does it still work if you apply the patch below?

Thanks!

Comments

David Herrmann Oct. 25, 2015, 9 a.m. UTC | #1
Hi

On Sun, Oct 25, 2015 at 2:17 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi David,
>
> On Thu, Sep 03, 2015 at 06:14:01PM +0200, David Herrmann wrote:
>> +static int bits_from_user(unsigned long *bits, unsigned int maxbit,
>> +                       unsigned int maxlen, const void __user *p, int compat)
>> +{
>> +     int len;
>> +
>> +#if IS_ENABLED(CONFIG_COMPAT)
>> +     if (compat) {
>> +             if (maxlen % sizeof(compat_long_t))
>> +                     return -EINVAL;
>> +             len = BITS_TO_LONGS_COMPAT(maxbit) * sizeof(compat_long_t);
>> +     } else
>> +#endif
>> +     {
>> +             if (maxlen % sizeof(long))
>> +                     return -EINVAL;
>> +             len = BITS_TO_LONGS(maxbit) * sizeof(long);
>> +     }
>> +
>> +     if (len > maxlen)
>> +             len = maxlen;
>> +
>> +#if IS_ENABLED(CONFIG_COMPAT) && defined(__BIG_ENDIAN)
>> +     if (compat) {
>> +             int i;
>> +
>> +             for (i = 0; i < len / sizeof(compat_long_t); i++)
>> +                     if (copy_from_user((compat_long_t *) bits +
>> +                                             i + 1 - ((i % 2) << 1),
>> +                                        (compat_long_t __user *) p + i,
>> +                                        sizeof(compat_long_t)))
>> +                             return -EFAULT;
>> +             if (i % 2)
>> +                     *((compat_long_t *) bits + i - 1) = 0;
>> +     } else
>> +#endif
>> +             if (copy_from_user(bits, p, len))
>> +                     return -EFAULT;
>> +
>> +     return len;
>> +}
>
> I do not quite like how we sprinkle ifdefs throughout, I prefer the way
> we have bits_to_user defined, even if it is more verbose.

Makes sense.

>> +
>>  static int str_to_user(const char *str, unsigned int maxlen, void __user *p)
>>  {
>>       int len;
>> @@ -854,6 +953,86 @@ static int evdev_revoke(struct evdev *evdev, struct evdev_client *client,
>>       return 0;
>>  }
>>
>> +/* must be called with evdev-mutex held */
>> +static int evdev_set_mask(struct evdev_client *client,
>> +                       unsigned int type,
>> +                       const void __user *codes,
>> +                       u32 codes_size,
>> +                       int compat)
>> +{
>> +     unsigned long flags, *mask, *oldmask;
>> +     size_t cnt, size, min;
>> +     int error;
>> +
>> +     /* we allow unknown types and 'codes_size > size' for forward-compat */
>> +     cnt = evdev_get_mask_cnt(type);
>> +     if (!cnt)
>> +             return 0;
>> +
>> +     size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
>> +     min = min_t(size_t, codes_size, size);
>> +
>> +     mask = kzalloc(size, GFP_KERNEL);
>> +     if (!mask)
>> +             return -ENOMEM;
>> +
>> +     error = bits_from_user(mask, cnt - 1, min, codes, compat);
>
> I do not think we need to calculate and pass min here: bits_from_user()
> will limit the output for us already.
>
> Does it still work if you apply the patch below?

One comment on void*-arithmetic below. Otherwise, this is reviewed and
tested by me.

Thanks a lot!
David

> ---
>  drivers/input/evdev.c |  148 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 88 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index 83d699f..ce35ea3 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -685,7 +685,46 @@ static int bits_to_user(unsigned long *bits, unsigned int maxbit,
>
>         return len;
>  }
> +
> +static int bits_from_user(unsigned long *bits, unsigned int maxbit,
> +                         unsigned int maxlen, const void __user *p, int compat)
> +{
> +       int len, i;
> +
> +       if (compat) {
> +               if (maxlen % sizeof(compat_long_t))
> +                       return -EINVAL;
> +
> +               len = BITS_TO_LONGS_COMPAT(maxbit) * sizeof(compat_long_t);
> +               if (len > maxlen)
> +                       len = maxlen;
> +
> +               for (i = 0; i < len / sizeof(compat_long_t); i++)
> +                       if (copy_from_user((compat_long_t *) bits +
> +                                               i + 1 - ((i % 2) << 1),
> +                                          (compat_long_t __user *) p + i,
> +                                          sizeof(compat_long_t)))
> +                               return -EFAULT;
> +               if (i % 2)
> +                       *((compat_long_t *) bits + i - 1) = 0;
> +
> +       } else {
> +               if (maxlen % sizeof(long))
> +                       return -EINVAL;
> +
> +               len = BITS_TO_LONGS(maxbit) * sizeof(long);
> +               if (len > maxlen)
> +                       len = maxlen;
> +
> +               if (copy_from_user(p, bits, len))
> +                       return -EFAULT;
> +       }
> +
> +       return len;
> +}
> +
>  #else
> +
>  static int bits_to_user(unsigned long *bits, unsigned int maxbit,
>                         unsigned int maxlen, void __user *p, int compat)
>  {
> @@ -698,6 +737,24 @@ static int bits_to_user(unsigned long *bits, unsigned int maxbit,
>
>         return copy_to_user(p, bits, len) ? -EFAULT : len;
>  }
> +
> +static int bits_from_user(unsigned long *bits, unsigned int maxbit,
> +                         unsigned int maxlen, const void __user *p, int compat)
> +{
> +       size_t chunk_size = compat ? sizeof(compat_long_t) : sizeof(long);
> +       int len;
> +
> +       if (maxlen % chunk_size)
> +               return -EINVAL;
> +
> +       len = compat ? BITS_TO_LONGS_COMPAT(maxbit) : BITS_TO_LONGS(maxbit);
> +       len *= chunk_size;
> +       if (len > maxlen)
> +               len = maxlen;
> +
> +       return copy_from_user(bits, p, len) ? -EFAULT : len;
> +}
> +
>  #endif /* __BIG_ENDIAN */
>
>  #else
> @@ -713,49 +770,23 @@ static int bits_to_user(unsigned long *bits, unsigned int maxbit,
>         return copy_to_user(p, bits, len) ? -EFAULT : len;
>  }
>
> -#endif /* CONFIG_COMPAT */
> -
>  static int bits_from_user(unsigned long *bits, unsigned int maxbit,
>                           unsigned int maxlen, const void __user *p, int compat)
>  {
>         int len;
>
> -#if IS_ENABLED(CONFIG_COMPAT)
> -       if (compat) {
> -               if (maxlen % sizeof(compat_long_t))
> -                       return -EINVAL;
> -               len = BITS_TO_LONGS_COMPAT(maxbit) * sizeof(compat_long_t);
> -       } else
> -#endif
> -       {
> -               if (maxlen % sizeof(long))
> -                       return -EINVAL;
> -               len = BITS_TO_LONGS(maxbit) * sizeof(long);
> -       }
> +       if (maxlen % sizeof(long))
> +               return -EINVAL;
>
> +       len = BITS_TO_LONGS(maxbit) * sizeof(long);
>         if (len > maxlen)
>                 len = maxlen;
>
> -#if IS_ENABLED(CONFIG_COMPAT) && defined(__BIG_ENDIAN)
> -       if (compat) {
> -               int i;
> -
> -               for (i = 0; i < len / sizeof(compat_long_t); i++)
> -                       if (copy_from_user((compat_long_t *) bits +
> -                                               i + 1 - ((i % 2) << 1),
> -                                          (compat_long_t __user *) p + i,
> -                                          sizeof(compat_long_t)))
> -                               return -EFAULT;
> -               if (i % 2)
> -                       *((compat_long_t *) bits + i - 1) = 0;
> -       } else
> -#endif
> -               if (copy_from_user(bits, p, len))
> -                       return -EFAULT;
> -
> -       return len;
> +       return copy_from_user(bits, p, len) ? -EFAULT : len;
>  }
>
> +#endif /* CONFIG_COMPAT */
> +
>  static int str_to_user(const char *str, unsigned int maxlen, void __user *p)
>  {
>         int len;
> @@ -956,7 +987,7 @@ static int evdev_set_mask(struct evdev_client *client,
>                           int compat)
>  {
>         unsigned long flags, *mask, *oldmask;
> -       size_t cnt, size, min;
> +       size_t cnt;
>         int error;
>
>         /* we allow unknown types and 'codes_size > size' for forward-compat */
> @@ -964,14 +995,11 @@ static int evdev_set_mask(struct evdev_client *client,
>         if (!cnt)
>                 return 0;
>
> -       size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
> -       min = min_t(size_t, codes_size, size);
> -
> -       mask = kzalloc(size, GFP_KERNEL);
> +       mask = kcalloc(sizeof(unsigned long), BITS_TO_LONGS(cnt), GFP_KERNEL);
>         if (!mask)
>                 return -ENOMEM;
>
> -       error = bits_from_user(mask, cnt - 1, min, codes, compat);
> +       error = bits_from_user(mask, cnt - 1, codes_size, codes, compat);
>         if (error < 0) {
>                 kfree(mask);
>                 return error;
> @@ -995,35 +1023,33 @@ static int evdev_get_mask(struct evdev_client *client,
>                           int compat)
>  {
>         unsigned long *mask;
> -       size_t cnt, size, min, i;
> -       u8 __user *out;
> +       size_t cnt, size, xfer_size;
> +       int i;
>         int error;
>
>         /* we allow unknown types and 'codes_size > size' for forward-compat */
>         cnt = evdev_get_mask_cnt(type);
>         size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
> -       min = min_t(size_t, codes_size, size);
> +       xfer_size = min_t(size_t, codes_size, size);
>
>         if (cnt > 0) {
>                 mask = client->evmasks[type];
>                 if (mask) {
> -                       error = bits_to_user(mask, cnt - 1, min, codes, compat);
> +                       error = bits_to_user(mask, cnt - 1,
> +                                            xfer_size, codes, compat);
>                         if (error < 0)
>                                 return error;
>                 } else {
>                         /* fake mask with all bits set */
> -                       out = (u8 __user*)codes;
> -                       for (i = 0; i < min; ++i)
> -                               if (put_user((u8)0xff,  out + i))
> +                       for (i = 0; i < xfer_size; i++)
> +                               if (put_user(0xffU, (u8 __user *)codes + i))
>                                         return -EFAULT;
>                 }
>         }
>
> -       codes = (u8*)codes + min;
> -       codes_size -= min;
> -
> -       if (codes_size > 0 && clear_user(codes, codes_size))
> -               return -EFAULT;
> +       if (xfer_size < codes_size)
> +               if (clear_user(codes + xfer_size, codes_size - xfer_size))

'codes' is void*. Want to cast it to u8 first? void* arithmetic is a
gnu extension, iirc. But maybe kernel code doesn't care, not sure.

> +                       return -EFAULT;
>
>         return 0;
>  }
> @@ -1097,27 +1123,29 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>                 else
>                         return evdev_revoke(evdev, client, file);
>
> -       case EVIOCGMASK:
> +       case EVIOCGMASK: {
> +               void __user *codes_ptr;
> +
>                 if (copy_from_user(&mask, p, sizeof(mask)))
>                         return -EFAULT;
>
> +               codes_ptr = (void __user *)(unsigned long)mask.codes_ptr;
>                 return evdev_get_mask(client,
> -                                     mask.type,
> -                                     (void __user*)
> -                                     (unsigned long)mask.codes_ptr,
> -                                     mask.codes_size,
> +                                     mask.type, codes_ptr, mask.codes_size,
>                                       compat_mode);
> +       }
> +
> +       case EVIOCSMASK: {
> +               const void __user *codes_ptr;
>
> -       case EVIOCSMASK:
>                 if (copy_from_user(&mask, p, sizeof(mask)))
>                         return -EFAULT;
>
> +               codes_ptr = (const void __user *)(unsigned long)mask.codes_ptr;
>                 return evdev_set_mask(client,
> -                                     mask.type,
> -                                     (const void __user*)
> -                                     (unsigned long)mask.codes_ptr,
> -                                     mask.codes_size,
> +                                     mask.type, codes_ptr, mask.codes_size,
>                                       compat_mode);
> +       }
>
>         case EVIOCSCLOCKID:
>                 if (copy_from_user(&i, p, sizeof(unsigned int)))
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov Oct. 25, 2015, 9:01 p.m. UTC | #2
On Sun, Oct 25, 2015 at 10:00:17AM +0100, David Herrmann wrote:
> Hi
> 
> On Sun, Oct 25, 2015 at 2:17 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Hi David,
> >
> > On Thu, Sep 03, 2015 at 06:14:01PM +0200, David Herrmann wrote:
> >> +static int bits_from_user(unsigned long *bits, unsigned int maxbit,
> >> +                       unsigned int maxlen, const void __user *p, int compat)
> >> +{
> >> +     int len;
> >> +
> >> +#if IS_ENABLED(CONFIG_COMPAT)
> >> +     if (compat) {
> >> +             if (maxlen % sizeof(compat_long_t))
> >> +                     return -EINVAL;
> >> +             len = BITS_TO_LONGS_COMPAT(maxbit) * sizeof(compat_long_t);
> >> +     } else
> >> +#endif
> >> +     {
> >> +             if (maxlen % sizeof(long))
> >> +                     return -EINVAL;
> >> +             len = BITS_TO_LONGS(maxbit) * sizeof(long);
> >> +     }
> >> +
> >> +     if (len > maxlen)
> >> +             len = maxlen;
> >> +
> >> +#if IS_ENABLED(CONFIG_COMPAT) && defined(__BIG_ENDIAN)
> >> +     if (compat) {
> >> +             int i;
> >> +
> >> +             for (i = 0; i < len / sizeof(compat_long_t); i++)
> >> +                     if (copy_from_user((compat_long_t *) bits +
> >> +                                             i + 1 - ((i % 2) << 1),
> >> +                                        (compat_long_t __user *) p + i,
> >> +                                        sizeof(compat_long_t)))
> >> +                             return -EFAULT;
> >> +             if (i % 2)
> >> +                     *((compat_long_t *) bits + i - 1) = 0;
> >> +     } else
> >> +#endif
> >> +             if (copy_from_user(bits, p, len))
> >> +                     return -EFAULT;
> >> +
> >> +     return len;
> >> +}
> >
> > I do not quite like how we sprinkle ifdefs throughout, I prefer the way
> > we have bits_to_user defined, even if it is more verbose.
> 
> Makes sense.
> 
> >> +
> >>  static int str_to_user(const char *str, unsigned int maxlen, void __user *p)
> >>  {
> >>       int len;
> >> @@ -854,6 +953,86 @@ static int evdev_revoke(struct evdev *evdev, struct evdev_client *client,
> >>       return 0;
> >>  }
> >>
> >> +/* must be called with evdev-mutex held */
> >> +static int evdev_set_mask(struct evdev_client *client,
> >> +                       unsigned int type,
> >> +                       const void __user *codes,
> >> +                       u32 codes_size,
> >> +                       int compat)
> >> +{
> >> +     unsigned long flags, *mask, *oldmask;
> >> +     size_t cnt, size, min;
> >> +     int error;
> >> +
> >> +     /* we allow unknown types and 'codes_size > size' for forward-compat */
> >> +     cnt = evdev_get_mask_cnt(type);
> >> +     if (!cnt)
> >> +             return 0;
> >> +
> >> +     size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
> >> +     min = min_t(size_t, codes_size, size);
> >> +
> >> +     mask = kzalloc(size, GFP_KERNEL);
> >> +     if (!mask)
> >> +             return -ENOMEM;
> >> +
> >> +     error = bits_from_user(mask, cnt - 1, min, codes, compat);
> >
> > I do not think we need to calculate and pass min here: bits_from_user()
> > will limit the output for us already.
> >
> > Does it still work if you apply the patch below?
> 
> One comment on void*-arithmetic below. Otherwise, this is reviewed and
> tested by me.

Yeah, I am not concerned about using this extension - it is used in
kernel quite often I think and it actually makes sense to me.

Thank you for giving it a spin, I'll fold into original patch and apply.

Patch
diff mbox

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 83d699f..ce35ea3 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -685,7 +685,46 @@  static int bits_to_user(unsigned long *bits, unsigned int maxbit,
 
 	return len;
 }
+
+static int bits_from_user(unsigned long *bits, unsigned int maxbit,
+			  unsigned int maxlen, const void __user *p, int compat)
+{
+	int len, i;
+
+	if (compat) {
+		if (maxlen % sizeof(compat_long_t))
+			return -EINVAL;
+
+		len = BITS_TO_LONGS_COMPAT(maxbit) * sizeof(compat_long_t);
+		if (len > maxlen)
+			len = maxlen;
+
+		for (i = 0; i < len / sizeof(compat_long_t); i++)
+			if (copy_from_user((compat_long_t *) bits +
+						i + 1 - ((i % 2) << 1),
+					   (compat_long_t __user *) p + i,
+					   sizeof(compat_long_t)))
+				return -EFAULT;
+		if (i % 2)
+			*((compat_long_t *) bits + i - 1) = 0;
+
+	} else {
+		if (maxlen % sizeof(long))
+			return -EINVAL;
+
+		len = BITS_TO_LONGS(maxbit) * sizeof(long);
+		if (len > maxlen)
+			len = maxlen;
+
+		if (copy_from_user(p, bits, len))
+			return -EFAULT;
+	}
+
+	return len;
+}
+
 #else
+
 static int bits_to_user(unsigned long *bits, unsigned int maxbit,
 			unsigned int maxlen, void __user *p, int compat)
 {
@@ -698,6 +737,24 @@  static int bits_to_user(unsigned long *bits, unsigned int maxbit,
 
 	return copy_to_user(p, bits, len) ? -EFAULT : len;
 }
+
+static int bits_from_user(unsigned long *bits, unsigned int maxbit,
+			  unsigned int maxlen, const void __user *p, int compat)
+{
+	size_t chunk_size = compat ? sizeof(compat_long_t) : sizeof(long);
+	int len;
+
+	if (maxlen % chunk_size)
+		return -EINVAL;
+
+	len = compat ? BITS_TO_LONGS_COMPAT(maxbit) : BITS_TO_LONGS(maxbit);
+	len *= chunk_size;
+	if (len > maxlen)
+		len = maxlen;
+
+	return copy_from_user(bits, p, len) ? -EFAULT : len;
+}
+
 #endif /* __BIG_ENDIAN */
 
 #else
@@ -713,49 +770,23 @@  static int bits_to_user(unsigned long *bits, unsigned int maxbit,
 	return copy_to_user(p, bits, len) ? -EFAULT : len;
 }
 
-#endif /* CONFIG_COMPAT */
-
 static int bits_from_user(unsigned long *bits, unsigned int maxbit,
 			  unsigned int maxlen, const void __user *p, int compat)
 {
 	int len;
 
-#if IS_ENABLED(CONFIG_COMPAT)
-	if (compat) {
-		if (maxlen % sizeof(compat_long_t))
-			return -EINVAL;
-		len = BITS_TO_LONGS_COMPAT(maxbit) * sizeof(compat_long_t);
-	} else
-#endif
-	{
-		if (maxlen % sizeof(long))
-			return -EINVAL;
-		len = BITS_TO_LONGS(maxbit) * sizeof(long);
-	}
+	if (maxlen % sizeof(long))
+		return -EINVAL;
 
+	len = BITS_TO_LONGS(maxbit) * sizeof(long);
 	if (len > maxlen)
 		len = maxlen;
 
-#if IS_ENABLED(CONFIG_COMPAT) && defined(__BIG_ENDIAN)
-	if (compat) {
-		int i;
-
-		for (i = 0; i < len / sizeof(compat_long_t); i++)
-			if (copy_from_user((compat_long_t *) bits +
-						i + 1 - ((i % 2) << 1),
-					   (compat_long_t __user *) p + i,
-					   sizeof(compat_long_t)))
-				return -EFAULT;
-		if (i % 2)
-			*((compat_long_t *) bits + i - 1) = 0;
-	} else
-#endif
-		if (copy_from_user(bits, p, len))
-			return -EFAULT;
-
-	return len;
+	return copy_from_user(bits, p, len) ? -EFAULT : len;
 }
 
+#endif /* CONFIG_COMPAT */
+
 static int str_to_user(const char *str, unsigned int maxlen, void __user *p)
 {
 	int len;
@@ -956,7 +987,7 @@  static int evdev_set_mask(struct evdev_client *client,
 			  int compat)
 {
 	unsigned long flags, *mask, *oldmask;
-	size_t cnt, size, min;
+	size_t cnt;
 	int error;
 
 	/* we allow unknown types and 'codes_size > size' for forward-compat */
@@ -964,14 +995,11 @@  static int evdev_set_mask(struct evdev_client *client,
 	if (!cnt)
 		return 0;
 
-	size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
-	min = min_t(size_t, codes_size, size);
-
-	mask = kzalloc(size, GFP_KERNEL);
+	mask = kcalloc(sizeof(unsigned long), BITS_TO_LONGS(cnt), GFP_KERNEL);
 	if (!mask)
 		return -ENOMEM;
 
-	error = bits_from_user(mask, cnt - 1, min, codes, compat);
+	error = bits_from_user(mask, cnt - 1, codes_size, codes, compat);
 	if (error < 0) {
 		kfree(mask);
 		return error;
@@ -995,35 +1023,33 @@  static int evdev_get_mask(struct evdev_client *client,
 			  int compat)
 {
 	unsigned long *mask;
-	size_t cnt, size, min, i;
-	u8 __user *out;
+	size_t cnt, size, xfer_size;
+	int i;
 	int error;
 
 	/* we allow unknown types and 'codes_size > size' for forward-compat */
 	cnt = evdev_get_mask_cnt(type);
 	size = sizeof(unsigned long) * BITS_TO_LONGS(cnt);
-	min = min_t(size_t, codes_size, size);
+	xfer_size = min_t(size_t, codes_size, size);
 
 	if (cnt > 0) {
 		mask = client->evmasks[type];
 		if (mask) {
-			error = bits_to_user(mask, cnt - 1, min, codes, compat);
+			error = bits_to_user(mask, cnt - 1,
+					     xfer_size, codes, compat);
 			if (error < 0)
 				return error;
 		} else {
 			/* fake mask with all bits set */
-			out = (u8 __user*)codes;
-			for (i = 0; i < min; ++i)
-				if (put_user((u8)0xff,  out + i))
+			for (i = 0; i < xfer_size; i++)
+				if (put_user(0xffU, (u8 __user *)codes + i))
 					return -EFAULT;
 		}
 	}
 
-	codes = (u8*)codes + min;
-	codes_size -= min;
-
-	if (codes_size > 0 && clear_user(codes, codes_size))
-		return -EFAULT;
+	if (xfer_size < codes_size)
+		if (clear_user(codes + xfer_size, codes_size - xfer_size))
+			return -EFAULT;
 
 	return 0;
 }
@@ -1097,27 +1123,29 @@  static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		else
 			return evdev_revoke(evdev, client, file);
 
-	case EVIOCGMASK:
+	case EVIOCGMASK: {
+		void __user *codes_ptr;
+
 		if (copy_from_user(&mask, p, sizeof(mask)))
 			return -EFAULT;
 
+		codes_ptr = (void __user *)(unsigned long)mask.codes_ptr;
 		return evdev_get_mask(client,
-				      mask.type,
-				      (void __user*)
-				      (unsigned long)mask.codes_ptr,
-				      mask.codes_size,
+				      mask.type, codes_ptr, mask.codes_size,
 				      compat_mode);
+	}
+
+	case EVIOCSMASK: {
+		const void __user *codes_ptr;
 
-	case EVIOCSMASK:
 		if (copy_from_user(&mask, p, sizeof(mask)))
 			return -EFAULT;
 
+		codes_ptr = (const void __user *)(unsigned long)mask.codes_ptr;
 		return evdev_set_mask(client,
-				      mask.type,
-				      (const void __user*)
-				      (unsigned long)mask.codes_ptr,
-				      mask.codes_size,
+				      mask.type, codes_ptr, mask.codes_size,
 				      compat_mode);
+	}
 
 	case EVIOCSCLOCKID:
 		if (copy_from_user(&i, p, sizeof(unsigned int)))