Hi
On Wed, Nov 12, 2014 at 1:41 PM, David Herrmann <dh.herrmann@xxxxxxxxx> wrote:
> 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.
I now added a memset(bits, 0, len); before "if (len > maxlen)". This
just clears the buffer before copying data. Due to the cross-over in
the for()-loop, I'm not entirely sure how to only clear the unused
parts, as the tail "long"s might be only set partially. Quite
confusing..
I didn't change anything else. Lemme know if that's fine and I will
resend the patch.
Thanks
David
>>> + } 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
