linux-kernel - Re: [PATCH] Input: synaptics-rmi4

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 for Android: free password hash cracker in your pocket

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

Message-ID: <a6e517a2-51ca-431a-88f8-8bfa0a867dc6@synaptics.com>
Date:   Fri, 11 Oct 2019 21:46:56 +0000
From:   Andrew Duggan <aduggan@...aptics.com>
To:     Evan Green <evgreen@...omium.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>
CC:     Pan Bian <bianpan2016@....com>,
        Allison Randal <allison@...utok.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Kate Stewart <kstewart@...uxfoundation.org>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Nick Dyer <nick@...anahar.org>,
        Christopher Heiny <Cheiny@...aptics.com>
Subject: Re: [PATCH] Input: synaptics-rmi4 - Avoid processing unknown IRQs

Hi Evan,

On 10/8/19 3:36 PM, Evan Green wrote:
> rmi_process_interrupt_requests() calls handle_nested_irq() for
> each interrupt status bit it finds. If the irq domain mapping for
> this bit had not yet been set up, then it ends up calling
> handle_nested_irq(0), which causes a NULL pointer dereference.
>
> There's already code that masks the irq_status bits coming out of the
> hardware with current_irq_mask, presumably to avoid this situation.
> However current_irq_mask seems to more reflect the actual mask set
> in the hardware rather than the IRQs software has set up and registered
> for. For example, in rmi_driver_reset_handler(), the current_irq_mask
> is initialized based on what is read from the hardware. If the reset
> value of this mask enables IRQs that Linux has not set up yet, then
> we end up in this situation.
>
> There appears to be a third unused bitmask that used to serve this
> purpose, fn_irq_bits. Use that bitmask instead of current_irq_mask
> to avoid calling handle_nested_irq() on IRQs that have not yet been
> set up.

Yes, it looks like the code which ensured that there was a function 
handler to handle the IRQ was removed when the driver switched to using 
an irq domain. Setting the fn_irq_bits and using them instead of 
current_irq_mask in rmi_process_interrupt_requests() makes sense to me.

Andrew

> Signed-off-by: Evan Green <evgreen@...omium.org>
Reviewed-by: Andrew Duggan <aduggan@...aptics.com>
> ---
>
>   drivers/input/rmi4/rmi_driver.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index 772493b1f665..190b9974526b 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -146,7 +146,7 @@ static int rmi_process_interrupt_requests(struct rmi_device *rmi_dev)
>   	}
>   
>   	mutex_lock(&data->irq_mutex);
> -	bitmap_and(data->irq_status, data->irq_status, data->current_irq_mask,
> +	bitmap_and(data->irq_status, data->irq_status, data->fn_irq_bits,
>   	       data->irq_count);
>   	/*
>   	 * At this point, irq_status has all bits that are set in the
> @@ -385,6 +385,8 @@ static int rmi_driver_set_irq_bits(struct rmi_device *rmi_dev,
>   	bitmap_copy(data->current_irq_mask, data->new_irq_mask,
>   		    data->num_of_irq_regs);
>   
> +	bitmap_or(data->fn_irq_bits, data->fn_irq_bits, mask, data->irq_count);
> +
>   error_unlock:
>   	mutex_unlock(&data->irq_mutex);
>   	return error;
> @@ -398,6 +400,8 @@ static int rmi_driver_clear_irq_bits(struct rmi_device *rmi_dev,
>   	struct device *dev = &rmi_dev->dev;
>   
>   	mutex_lock(&data->irq_mutex);
> +	bitmap_andnot(data->fn_irq_bits,
> +		      data->fn_irq_bits, mask, data->irq_count);
>   	bitmap_andnot(data->new_irq_mask,
>   		  data->current_irq_mask, mask, data->irq_count);
>