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@xxxxxxxxx> 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.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
