Hi
On Wed, Nov 12, 2014 at 8:40 AM, Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:
> 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)
BITS_TO_LONGS_COMPAT is only defined if CONFIG_COMPAT is. We'd rely on
compiler-optimizations (dead code elimination) if we'd do that change.
I'm not really fond of making -O0 compiles fail, but there're several
places in the kernel that do. Your call ;)
>> + 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.
Right, I relied on the caller of bits_from_user() to initialize it to
zero and make it long-aligned. I can move it into that helper.
>> + } 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.
I'm actually not very familiar with the details of copy_to_user().
That's why I went with put_user(). I can look into it, but if you know
it well, I am open for suggestions.
Thanks
David
--
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
