Re: [PATCH v3] Input: evdev - add event-mask API

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi David,

On Tue, Nov 04, 2014 at 06:12:23PM +0100, 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)

I think this can be written as:
	if (IS_ENABLED(CONFIG_COMPAT) && compat)

> +		len = BITS_TO_LONGS_COMPAT(maxbit) * sizeof(compat_long_t);
> +	else
> +#endif
> +		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)))

Unfortunately this is not quite enough, you need to also zero out the
upper bits of the last partial if userspace did not supply multiple of
64 bits.

Frankly, bitmask APIs that we have in input are god-awful on big endian:
they do not work if usespace uses quantities less than long (or compat
long). I think we'll have to enforce it in the code.

> +		} else {
> +			/* fake mask with all bits set */
> +			out = (u8 __user*)codes;
> +			for (i = 0; i < min; ++i) {
> +				if (put_user((u8)0xff,  out + i))
> +					return -EFAULT;

I wonder if we should not replace this with access_ok() + straight
memset instead of doing byte-by-byte copy.

Thanks.

-- 
Dmitry