Input: synaptics-rmi4 - Avoid processing unknown IRQs
diff mbox series

Message ID 20191008223657.163366-1-evgreen@chromium.org
State Mainlined
Commit 363c53875aef8fce69d4a2d0873919ccc7d9e2ad
Headers show
Series
  • Input: synaptics-rmi4 - Avoid processing unknown IRQs
Related show

Commit Message

Evan Green Oct. 8, 2019, 10:36 p.m. UTC
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.

Signed-off-by: Evan Green <evgreen@chromium.org>
---

 drivers/input/rmi4/rmi_driver.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Andrew Duggan Oct. 11, 2019, 9:46 p.m. UTC | #1
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@chromium.org>
Reviewed-by: Andrew Duggan <aduggan@synaptics.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);
>
Dmitry Torokhov Oct. 17, 2019, 6:04 p.m. UTC | #2
On Fri, Oct 11, 2019 at 09:46:56PM +0000, Andrew Duggan wrote:
> 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@chromium.org>
> Reviewed-by: Andrew Duggan <aduggan@synaptics.com>

Applied, thank you.

> > ---
> >
> >   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);
> >

Patch
diff mbox series

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);