linux-kernel - Re: [RFC/RFT 1/5] Input: implement proper locking in input core

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <46A58FBA.5010505@garzik.org>
Date:	Tue, 24 Jul 2007 01:35:54 -0400
From:	Jeff Garzik <jeff@...zik.org>
To:	Dmitry Torokhov <dtor@...ightbb.com>
CC:	linux-input@...ey.karlin.mff.cuni.cz, linux-kernel@...r.kernel.org
Subject: Re: [RFC/RFT 1/5] Input: implement proper locking in input core

Dmitry Torokhov wrote:
> +static void input_repeat_key(unsigned long data)
> +{
> +	struct input_dev *dev = (void *) data;
>  
> -			change_bit(code, dev->key);
> +	spin_lock_irq(&dev->event_lock);
[...]
> +void input_inject_event(struct input_handle *handle,
> +			unsigned int type, unsigned int code, int value)
>  {
> -	struct input_dev *dev = (void *) data;
> +	struct input_dev *dev = handle->dev;
> +	struct input_handle *grab;
>  
> -	if (!test_bit(dev->repeat_key, dev->key))
> -		return;
> +	if (is_event_supported(type, dev->evbit, EV_MAX)) {
> +		spin_lock_irq(&dev->event_lock);
>  
> -	input_event(dev, EV_KEY, dev->repeat_key, 2);
> -	input_sync(dev);
> +		grab = rcu_dereference(dev->grab);
> +		if (!grab || grab == handle)
> +			input_handle_event(dev, type, code, value);
>  
> -	if (dev->rep[REP_PERIOD])
> -		mod_timer(&dev->timer, jiffies + msecs_to_jiffies(dev->rep[REP_PERIOD]));
> +		spin_unlock_irq(&dev->event_lock);
> +	}
>  }
> +EXPORT_SYMBOL(input_inject_event);
[...]
> +	spin_lock_irq(&dev->event_lock);
> +
> +	/*
> +	 * Simulate keyup events for all pressed keys so that handlers
> +	 * are not left with "stuck" keys. The driver may continue
> +	 * generate events even after we done here but they will not
> +	 * reach any handlers.
> +	 */
> +	if (is_event_supported(EV_KEY, dev->evbit, EV_MAX)) {
> +		for (code = 0; code <= KEY_MAX; code++) {
> +			if (is_event_supported(code, dev->keybit, KEY_MAX) &&
> +			    test_bit(code, dev->key)) {
> +				input_pass_event(dev, EV_KEY, code, 0);
> +			}
> +		}
> +		input_pass_event(dev, EV_SYN, SYN_REPORT, 1);
> +	}
> +
> +	list_for_each_entry(handle, &dev->h_list, d_node)
> +		handle->open = 0;
> +
> +	spin_unlock_irq(&dev->event_lock);


spin_lock_irq() should generally be avoided.

In cases like the first case -- input_repeat_key() -- you are making 
incorrect assumptions about the state of interrupts.  The other cases 
are probably ok, but in general spin_lock_irq() has a long history of 
being very fragile and quite often wrong.

Use spin_lock_irqsave() to be safe.  Definitely in input_repeat_key(), 
but I strongly recommend removing spin_lock_irq() from all your patches 
here.

	Jeff


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/