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

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-next>] [day] [month] [year] [list]

Message-Id: <20191218080201.2508-1-baijiaju1990@gmail.com>
Date:   Wed, 18 Dec 2019 16:02:01 +0800
From:   Jia-Ju Bai <baijiaju1990@...il.com>
To:     bonbons@...ux-vserver.org, jikos@...nel.org,
        benjamin.tissoires@...hat.com
Cc:     linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jia-Ju Bai <baijiaju1990@...il.com>
Subject: [PATCH] hid: hid-picolcd: fix possible sleep-in-atomic-context bug

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@...il.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