[PATCH] hid: hid-picolcd: fix possible sleep-in-atomic-context bug

[PATCH] hid: hid-picolcd: fix possible sleep-in-atomic-context bug

[Jia-Ju Bai]

The driver may sleep while holding a read lock.
The function call path (from bottom to top) in Linux 4.19 is:

drivers/hid/hid-core.c, 1459: 
	hid_alloc_report_buf(GFP_KERNEL) in __hid_request
./include/linux/hid.h, 1051: 
	__hid_request in hid_hw_request
drivers/hid/hid-picolcd_leds.c, 56:
	hid_hw_request in picolcd_leds_set
drivers/hid/hid-picolcd_leds.c, 53:
	_raw_spin_lock_irqsave in picolcd_leds_set

drivers/hid/hid-core.c, 1459: 
	hid_alloc_report_buf(GFP_KERNEL) in __hid_request
./include/linux/hid.h, 1051: 
	__hid_request in hid_hw_request
drivers/hid/hid-picolcd_lcd.c, 49: 
	hid_hw_request in picolcd_set_contrast
drivers/hid/hid-picolcd_lcd.c, 46: 
	_raw_spin_lock_irqsave in picolcd_set_contrast

drivers/hid/hid-core.c, 1459: 
	hid_alloc_report_buf(GFP_KERNEL) in __hid_request
./include/linux/hid.h, 1051: 
	__hid_request in hid_hw_request
drivers/hid/hid-picolcd_core.c, 245: 
	hid_hw_request in picolcd_reset
drivers/hid/hid-picolcd_core.c, 235: 
	_raw_spin_lock_irqsave in picolcd_reset

drivers/hid/hid-core.c, 1459: 
	hid_alloc_report_buf(GFP_KERNEL) in __hid_request
./include/linux/hid.h, 1051: 
	__hid_request in hid_hw_request
drivers/hid/hid-picolcd_core.c, 111: 
	hid_hw_request in picolcd_send_and_wait
drivers/hid/hid-picolcd_core.c, 100: 
	_raw_spin_lock_irqsave in picolcd_send_and_wait

hid_alloc_report_buf(GFP_KERNEL) can sleep at runtime.

To fix these bugs, hid_hw_request() is called without holding the
spinlock.

These bugs are found by a static analysis tool STCheck written by myself.

Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
 drivers/hid/hid-picolcd_core.c | 4 ++--
 drivers/hid/hid-picolcd_lcd.c  | 6 ++++--
 drivers/hid/hid-picolcd_leds.c | 6 ++++--
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
index 1b5c63241af0..55d1892daa15 100644
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -99,8 +99,8 @@ struct picolcd_pending *picolcd_send_and_wait(struct hid_device *hdev,
 		work = NULL;
 	} else {
 		data->pending = work;
-		hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
 		spin_unlock_irqrestore(&data->lock, flags);
+		hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
 		wait_for_completion_interruptible_timeout(&work->ready, HZ*2);
 		spin_lock_irqsave(&data->lock, flags);
 		data->pending = NULL;
@@ -233,8 +233,8 @@ int picolcd_reset(struct hid_device *hdev)
 		spin_unlock_irqrestore(&data->lock, flags);
 		return -ENODEV;
 	}
-	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
 	spin_unlock_irqrestore(&data->lock, flags);
+	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
 
 	error = picolcd_check_version(hdev);
 	if (error)
diff --git a/drivers/hid/hid-picolcd_lcd.c b/drivers/hid/hid-picolcd_lcd.c
index 0c4b76de8ae5..1fd291674ffe 100644
--- a/drivers/hid/hid-picolcd_lcd.c
+++ b/drivers/hid/hid-picolcd_lcd.c
@@ -26,6 +26,7 @@ static int picolcd_get_contrast(struct lcd_device *ldev)
 static int picolcd_set_contrast(struct lcd_device *ldev, int contrast)
 {
 	struct picolcd_data *data = lcd_get_data(ldev);
+	int status;
 	struct hid_report *report = picolcd_out_report(REPORT_CONTRAST, data->hdev);
 	unsigned long flags;
 
@@ -35,9 +36,10 @@ static int picolcd_set_contrast(struct lcd_device *ldev, int contrast)
 	data->lcd_contrast = contrast & 0x0ff;
 	spin_lock_irqsave(&data->lock, flags);
 	hid_set_field(report->field[0], 0, data->lcd_contrast);
-	if (!(data->status & PICOLCD_FAILED))
-		hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
+	status = data->status;
 	spin_unlock_irqrestore(&data->lock, flags);
+	if (!(status & PICOLCD_FAILED))
+		hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
 	return 0;
 }
 
diff --git a/drivers/hid/hid-picolcd_leds.c b/drivers/hid/hid-picolcd_leds.c
index 6b505a753511..6652aa6b98dd 100644
--- a/drivers/hid/hid-picolcd_leds.c
+++ b/drivers/hid/hid-picolcd_leds.c
@@ -32,6 +32,7 @@
 void picolcd_leds_set(struct picolcd_data *data)
 {
 	struct hid_report *report;
+	int status;
 	unsigned long flags;
 
 	if (!data->led[0])
@@ -42,9 +43,10 @@ void picolcd_leds_set(struct picolcd_data *data)
 
 	spin_lock_irqsave(&data->lock, flags);
 	hid_set_field(report->field[0], 0, data->led_state);
-	if (!(data->status & PICOLCD_FAILED))
-		hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
+	status = data->status;
 	spin_unlock_irqrestore(&data->lock, flags);
+	if (!(status & PICOLCD_FAILED))
+		hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
 }
 
 static void picolcd_led_set_brightness(struct led_classdev *led_cdev,
-- 
2.17.1

Re: [PATCH] hid: hid-picolcd: fix possible sleep-in-atomic-context bug

[Bruno Prémont]

Hi Jia-Ju,

Your checker has been looking at fallback implementation for
the might-sleep hid_alloc_report_buf(GFP_KERNEL).

Did you have a look at the low-lever bus-driver implementations:
  hdev->ll_driver->request
                   ^^^^^^^

Are those all sleeping as well or maybe they don't sleep?


I will have a look over the coming days.

Best regards,
Bruno Prémont

On Wed, 18 Dec 2019 16:02:01 +0800 Jia-Ju Bai wrote:
> The driver may sleep while holding a read lock.
> The function call path (from bottom to top) in Linux 4.19 is:
> 
> drivers/hid/hid-core.c, 1459: 
> 	hid_alloc_report_buf(GFP_KERNEL) in __hid_request
> ./include/linux/hid.h, 1051: 
> 	__hid_request in hid_hw_request
> drivers/hid/hid-picolcd_leds.c, 56:
> 	hid_hw_request in picolcd_leds_set
> drivers/hid/hid-picolcd_leds.c, 53:
> 	_raw_spin_lock_irqsave in picolcd_leds_set
> 
> drivers/hid/hid-core.c, 1459: 
> 	hid_alloc_report_buf(GFP_KERNEL) in __hid_request
> ./include/linux/hid.h, 1051: 
> 	__hid_request in hid_hw_request
> drivers/hid/hid-picolcd_lcd.c, 49: 
> 	hid_hw_request in picolcd_set_contrast
> drivers/hid/hid-picolcd_lcd.c, 46: 
> 	_raw_spin_lock_irqsave in picolcd_set_contrast
> 
> drivers/hid/hid-core.c, 1459: 
> 	hid_alloc_report_buf(GFP_KERNEL) in __hid_request
> ./include/linux/hid.h, 1051: 
> 	__hid_request in hid_hw_request
> drivers/hid/hid-picolcd_core.c, 245: 
> 	hid_hw_request in picolcd_reset
> drivers/hid/hid-picolcd_core.c, 235: 
> 	_raw_spin_lock_irqsave in picolcd_reset
> 
> drivers/hid/hid-core.c, 1459: 
> 	hid_alloc_report_buf(GFP_KERNEL) in __hid_request
> ./include/linux/hid.h, 1051: 
> 	__hid_request in hid_hw_request
> drivers/hid/hid-picolcd_core.c, 111: 
> 	hid_hw_request in picolcd_send_and_wait
> drivers/hid/hid-picolcd_core.c, 100: 
> 	_raw_spin_lock_irqsave in picolcd_send_and_wait
> 
> hid_alloc_report_buf(GFP_KERNEL) can sleep at runtime.
> 
> To fix these bugs, hid_hw_request() is called without holding the
> spinlock.
> 
> These bugs are found by a static analysis tool STCheck written by myself.
> 
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> ---
>  drivers/hid/hid-picolcd_core.c | 4 ++--
>  drivers/hid/hid-picolcd_lcd.c  | 6 ++++--
>  drivers/hid/hid-picolcd_leds.c | 6 ++++--
>  3 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> index 1b5c63241af0..55d1892daa15 100644
> --- a/drivers/hid/hid-picolcd_core.c
> +++ b/drivers/hid/hid-picolcd_core.c
> @@ -99,8 +99,8 @@ struct picolcd_pending *picolcd_send_and_wait(struct hid_device *hdev,
>  		work = NULL;
>  	} else {
>  		data->pending = work;
> -		hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
>  		spin_unlock_irqrestore(&data->lock, flags);
> +		hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
>  		wait_for_completion_interruptible_timeout(&work->ready, HZ*2);
>  		spin_lock_irqsave(&data->lock, flags);
>  		data->pending = NULL;
> @@ -233,8 +233,8 @@ int picolcd_reset(struct hid_device *hdev)
>  		spin_unlock_irqrestore(&data->lock, flags);
>  		return -ENODEV;
>  	}
> -	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
>  	spin_unlock_irqrestore(&data->lock, flags);
> +	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
>  
>  	error = picolcd_check_version(hdev);
>  	if (error)
> diff --git a/drivers/hid/hid-picolcd_lcd.c b/drivers/hid/hid-picolcd_lcd.c
> index 0c4b76de8ae5..1fd291674ffe 100644
> --- a/drivers/hid/hid-picolcd_lcd.c
> +++ b/drivers/hid/hid-picolcd_lcd.c
> @@ -26,6 +26,7 @@ static int picolcd_get_contrast(struct lcd_device *ldev)
>  static int picolcd_set_contrast(struct lcd_device *ldev, int contrast)
>  {
>  	struct picolcd_data *data = lcd_get_data(ldev);
> +	int status;
>  	struct hid_report *report = picolcd_out_report(REPORT_CONTRAST, data->hdev);
>  	unsigned long flags;
>  
> @@ -35,9 +36,10 @@ static int picolcd_set_contrast(struct lcd_device *ldev, int contrast)
>  	data->lcd_contrast = contrast & 0x0ff;
>  	spin_lock_irqsave(&data->lock, flags);
>  	hid_set_field(report->field[0], 0, data->lcd_contrast);
> -	if (!(data->status & PICOLCD_FAILED))
> -		hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
> +	status = data->status;
>  	spin_unlock_irqrestore(&data->lock, flags);
> +	if (!(status & PICOLCD_FAILED))
> +		hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
>  	return 0;
>  }
>  
> diff --git a/drivers/hid/hid-picolcd_leds.c b/drivers/hid/hid-picolcd_leds.c
> index 6b505a753511..6652aa6b98dd 100644
> --- a/drivers/hid/hid-picolcd_leds.c
> +++ b/drivers/hid/hid-picolcd_leds.c
> @@ -32,6 +32,7 @@
>  void picolcd_leds_set(struct picolcd_data *data)
>  {
>  	struct hid_report *report;
> +	int status;
>  	unsigned long flags;
>  
>  	if (!data->led[0])
> @@ -42,9 +43,10 @@ void picolcd_leds_set(struct picolcd_data *data)
>  
>  	spin_lock_irqsave(&data->lock, flags);
>  	hid_set_field(report->field[0], 0, data->led_state);
> -	if (!(data->status & PICOLCD_FAILED))
> -		hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
> +	status = data->status;
>  	spin_unlock_irqrestore(&data->lock, flags);
> +	if (!(status & PICOLCD_FAILED))
> +		hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
>  }
>  
>  static void picolcd_led_set_brightness(struct led_classdev *led_cdev,

Re: [PATCH] hid: hid-picolcd: fix possible sleep-in-atomic-context bug

[Jia-Ju Bai]

Thanks for the reply.

On 2019/12/18 16:41, Bruno Prémont wrote:
> Hi Jia-Ju,
>
> Your checker has been looking at fallback implementation for
> the might-sleep hid_alloc_report_buf(GFP_KERNEL).
>
> Did you have a look at the low-lever bus-driver implementations:
>    hdev->ll_driver->request
>                     ^^^^^^^
>
> Are those all sleeping as well or maybe they don't sleep?\

In fact, I find that a function possibly-related to this function 
pointer can sleep:

drivers/hid/intel-ish-hid/ishtp-hid.c, 97:
     kzalloc(GFP_KERNEL) in ishtp_hid_request

But I am not quite sure whether this function is really referenced by 
the function pointer, so I did not report it.


Best wishes,
Jia-Ju Bai

Re: [PATCH] hid: hid-picolcd: fix possible sleep-in-atomic-context bug

[Bruno Prémont]

Hi Jia-Ju,

I've had a deeper look at the code (possibly also applies to hid-lg4ff).


The hdev->ll_driver->request (at least on USB bus which is the only one
that matters for hid-picolcd) points to:
  usbhid_request() from drivers/hid/usbhid/hid-core.c

This one directly calls usbhid_submit_report() which then directly calls
__usbhid_submit_report() under spinlock.

Thus for USB bus there is no possible sleep left.


Just moving the hid_hw_request() calls out of the spinlock is
incorrect though as it would introduce the possibility of unexpected
concurrent initialization/submissions of reports from the distinct
sub-drivers. The report pointer used is not call-private but comes from
feature description and is filled with data on each call within the
spinlock.


The question could be whether the generic fallback in hid_hw_request()
should be adjusted to be non-sleeping.
It has been introduced rather more recently than both drivers you
detected.


Best regards,
Bruno Prémont

On Wed, 18 Dec 2019 20:11:47 Jia-Ju Bai wrote:
> Thanks for the reply.
> 
> On 2019/12/18 16:41, Bruno Prémont wrote:
> > Hi Jia-Ju,
> >
> > Your checker has been looking at fallback implementation for
> > the might-sleep hid_alloc_report_buf(GFP_KERNEL).
> >
> > Did you have a look at the low-lever bus-driver implementations:
> >    hdev->ll_driver->request
> >                     ^^^^^^^
> >
> > Are those all sleeping as well or maybe they don't sleep?\  
> 
> In fact, I find that a function possibly-related to this function 
> pointer can sleep:
> 
> drivers/hid/intel-ish-hid/ishtp-hid.c, 97:
>      kzalloc(GFP_KERNEL) in ishtp_hid_request
> 
> But I am not quite sure whether this function is really referenced by 
> the function pointer, so I did not report it.
> 
> 
> Best wishes,
> Jia-Ju Bai