[PATCH] drivers: input: input: Fix possible sleep-in-atomic bug in input_alloc_absinfo()

[PATCH] drivers: input: input: Fix possible sleep-in-atomic bug in input_alloc_absinfo()

[starmiku1207184332]

From: Teng Qi <starmiku1207184332@gmail.com>

input_alloc_absinfo() may cause the kernel to sleep by calling kcalloc() with
the GFP_KERNEL flag. It may be called by input_event() holding spinlock through
the following call path: 

input_event(), 433
input_handle_event(), 399
input_get_disposition(), 294
input_handle_abs_event(), 232
input_abs_set_val(), Clang suggests it calls input_alloc_absinfo()
	unconditionally
input_alloc_absinfo(), 483
kcalloc(..., GFP_KERNEL)

This call path can potentially trigger a sleep-in-atomic bug. To prevent this
potential bug, we modify input_alloc_absinfo() to use the GFP_ATOMIC flag
instead of the GFP_KERNEL flag.
The possible bug is detected by a static code analysis tool.

Signed-off-by: Teng Qi <starmiku1207184332@gmail.com>
---
 drivers/input/input.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 37e876d45eb9..eefc4c85a693 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -480,7 +480,7 @@ void input_alloc_absinfo(struct input_dev *dev)
 	if (dev->absinfo)
 		return;
 
-	dev->absinfo = kcalloc(ABS_CNT, sizeof(*dev->absinfo), GFP_KERNEL);
+	dev->absinfo = kcalloc(ABS_CNT, sizeof(*dev->absinfo), GFP_ATOMIC);
 	if (!dev->absinfo) {
 		dev_err(dev->dev.parent ?: &dev->dev,
 			"%s: unable to allocate memory\n", __func__);
-- 
2.25.1

Re: [PATCH] drivers: input: input: Fix possible sleep-in-atomic bug in input_alloc_absinfo()

[Dmitry Torokhov]

Hi Teng Qi,

On Tue, Apr 25, 2023 at 07:49:51AM +0000, starmiku1207184332@gmail.com wrote:
> From: Teng Qi <starmiku1207184332@gmail.com>
> 
> input_alloc_absinfo() may cause the kernel to sleep by calling kcalloc() with
> the GFP_KERNEL flag. It may be called by input_event() holding spinlock through
> the following call path: 
> 
> input_event(), 433
> input_handle_event(), 399
> input_get_disposition(), 294
> input_handle_abs_event(), 232
> input_abs_set_val(), Clang suggests it calls input_alloc_absinfo()
> 	unconditionally
> input_alloc_absinfo(), 483
> kcalloc(..., GFP_KERNEL)
> 
> This call path can potentially trigger a sleep-in-atomic bug. To prevent this
> potential bug, we modify input_alloc_absinfo() to use the GFP_ATOMIC flag
> instead of the GFP_KERNEL flag.

This can not happen in practice, and changing the allocation type to
GFP_ATOMIC hurts the other code. We can avoid the issue by not using the
helper and have input core access the slot info directly. I CCed you on
a patch doing so.

> The possible bug is detected by a static code analysis tool.
> 
> Signed-off-by: Teng Qi <starmiku1207184332@gmail.com>
> ---
>  drivers/input/input.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 37e876d45eb9..eefc4c85a693 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -480,7 +480,7 @@ void input_alloc_absinfo(struct input_dev *dev)
>  	if (dev->absinfo)
>  		return;
>  
> -	dev->absinfo = kcalloc(ABS_CNT, sizeof(*dev->absinfo), GFP_KERNEL);
> +	dev->absinfo = kcalloc(ABS_CNT, sizeof(*dev->absinfo), GFP_ATOMIC);
>  	if (!dev->absinfo) {
>  		dev_err(dev->dev.parent ?: &dev->dev,
>  			"%s: unable to allocate memory\n", __func__);
> -- 
> 2.25.1
> 

Thanks.

-- 
Dmitry

Re: [PATCH] drivers: input: input: Fix possible sleep-in-atomic bug in input_alloc_absinfo()

[Dmitry Torokhov]

On Mon, May 08, 2023 at 03:13:19PM +0800, Teng Qi wrote:
> Thank you for an alternative solution. I will take a closer look at the
> patch
> you CCed me on.
> However, I am still confused about why this cannot happen in practice.
> Could you provide more information?

The only call to input_abs_set_val() in the event processing code that
is running in an atomic context (under spinlock) is done for
ABS_MT_SLOT, and the call is protected by checks ensuring that
multitouch was set up properly for the device. This includes allocating
absinfo. Therefore while the code may trip up static analyzers the
problem will not happen in practice with the current code.

Thanks.

-- 
Dmitry