linux-kernel - Re: [PATCH] [v9]Input: evdev: fix bug of dropping valid packet after syn

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] [day] [month] [year] [list]
Message-ID: <20161119185959.GE20446@dtor-ws>
Date:   Sat, 19 Nov 2016 10:59:59 -0800
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Aniroop Mathur <a.mathur@...sung.com>
Cc:     linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] [v9]Input: evdev: fix bug of dropping valid packet after
 syn_dropped event

Hi Anoroop,

On Wed, Oct 05, 2016 at 12:42:56AM +0530, Aniroop Mathur wrote:
> If last event dropped in the old queue was EV_SYN/SYN_REPORT, then lets
> generate EV_SYN/SYN_REPORT immediately after queing EV_SYN/SYN_DROPPED
> so that clients would not ignore next valid full packet events.
> 
> Signed-off-by: Aniroop Mathur <a.mathur@...sung.com>
> 
> Difference from v8:
> Added check for handling EVIOCG[type] ioctl for queue empty case
> ---
>  drivers/input/evdev.c | 52 ++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index e9ae3d5..69407ff 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -156,7 +156,12 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>  static void __evdev_queue_syn_dropped(struct evdev_client *client)
>  {
>  	struct input_event ev;
> +	struct input_event *last_ev;
>  	ktime_t time;
> +	unsigned int mask = client->bufsize - 1;
> +
> +	/* capture last event stored in the buffer */
> +	last_ev = &client->buffer[(client->head - 1) & mask];

I am uneasy with this "looking back" into the queue. How can we be sure
that we are looking at the right event? As far as I can tell we do not
know if that event has been consumed or not.

>  
>  	time = client->clk_type == EV_CLK_REAL ?
>  			ktime_get_real() :
> @@ -170,13 +175,28 @@ static void __evdev_queue_syn_dropped(struct evdev_client *client)
>  	ev.value = 0;
>  
>  	client->buffer[client->head++] = ev;
> -	client->head &= client->bufsize - 1;
> +	client->head &= mask;
>  
>  	if (unlikely(client->head == client->tail)) {
>  		/* drop queue but keep our SYN_DROPPED event */
> -		client->tail = (client->head - 1) & (client->bufsize - 1);
> +		client->tail = (client->head - 1) & mask;
>  		client->packet_head = client->tail;
>  	}
> +
> +	/*
> +	 * If last packet was completely stored, then queue SYN_REPORT
> +	 * so that clients would not ignore next valid full packet
> +	 */
> +	if (last_ev->type == EV_SYN && last_ev->code == SYN_REPORT) {
> +		last_ev->time = ev.time;
> +		client->buffer[client->head++] = *last_ev;
> +		client->head &= mask;
> +		client->packet_head = client->head;
> +
> +		/* drop queue but keep our SYN_DROPPED & SYN_REPORT event */
> +		if (unlikely(client->head == client->tail))
> +			client->tail = (client->head - 2) & mask;
> +	}
>  }
>  
>  static void evdev_queue_syn_dropped(struct evdev_client *client)
> @@ -218,7 +238,7 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
>  		spin_lock_irqsave(&client->buffer_lock, flags);
>  
>  		if (client->head != client->tail) {
> -			client->packet_head = client->head = client->tail;
> +			client->packet_head = client->tail = client->head;
>  			__evdev_queue_syn_dropped(client);
>  		}
>  
> @@ -231,22 +251,24 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
>  static void __pass_event(struct evdev_client *client,
>  			 const struct input_event *event)
>  {
> +	unsigned int mask = client->bufsize - 1;
> +
>  	client->buffer[client->head++] = *event;
> -	client->head &= client->bufsize - 1;
> +	client->head &= mask;
>  
>  	if (unlikely(client->head == client->tail)) {
>  		/*
>  		 * This effectively "drops" all unconsumed events, leaving
> -		 * EV_SYN/SYN_DROPPED plus the newest event in the queue.
> +		 * EV_SYN/SYN_DROPPED, EV_SYN/SYN_REPORT (if required) and
> +		 * newest event in the queue.
>  		 */
> -		client->tail = (client->head - 2) & (client->bufsize - 1);
> -
> -		client->buffer[client->tail].time = event->time;
> -		client->buffer[client->tail].type = EV_SYN;
> -		client->buffer[client->tail].code = SYN_DROPPED;
> -		client->buffer[client->tail].value = 0;
> +		client->head = (client->head - 1) & mask;
> +		client->packet_head = client->tail = client->head;
> +		__evdev_queue_syn_dropped(client);
>  
> -		client->packet_head = client->tail;
> +		client->buffer[client->head++] = *event;
> +		client->head &= mask;
> +		/* No need to check for buffer overflow as it just occurred */
>  	}
>  
>  	if (event->type == EV_SYN && event->code == SYN_REPORT) {
> @@ -920,6 +942,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
>  	int ret;
>  	unsigned long *mem;
>  	size_t len;
> +	bool is_queue_empty;
>  
>  	len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
>  	mem = kmalloc(len, GFP_KERNEL);
> @@ -933,12 +956,15 @@ static int evdev_handle_get_val(struct evdev_client *client,
>  
>  	spin_unlock(&dev->event_lock);
>  
> +	if (client->head == client->tail)
> +		is_queue_empty = true;
> +
>  	__evdev_flush_queue(client, type);
>  
>  	spin_unlock_irq(&client->buffer_lock);
>  
>  	ret = bits_to_user(mem, maxbit, maxlen, p, compat);
> -	if (ret < 0)
> +	if (ret < 0 && !is_queue_empty)
>  		evdev_queue_syn_dropped(client);
>  
>  	kfree(mem);
> -- 
> 2.6.2
> 

Thanks.

-- 
Dmitry