Bluetooth: hci_qca: Fix an error pointer dereference

PATCH In review Dan Carpenter 2 participants 2 messages May 29, 2020 – Jun 3, 2020

Subsystems (3)

Bluetooth drivers Kernel janitors Qualcomm Bluetooth driver

lore

Share & tools

lore

Dan Carpenter

author

2020-05-29 09:59 UTC #0 lore raw

PATCH 1/1 Bluetooth: hci_qca: Fix an error pointer dereference

When a function like devm_clk_get_optional() function returns both error pointers on error and NULL then the NULL return means that the optional feature is deliberately disabled. It is a special sort of success and should not trigger an error message. The surrounding code should be written to check for NULL and not crash.

On the other hand, if we encounter an error, then the probe from should clean up and return a failure.

In this code, if devm_clk_get_optional() returns an error pointer then the kernel will crash inside the call to:

	clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);

The error handling must be updated to prevent that.

Fixes 77131dfec6af ("Bluetooth: hci_qca: Replace devm_gpiod_get() with devm_gpiod_get_optional()")

Signed-off-by Dan Carpenter <[email protected]>

---
Commit 77131dfec6af ("Bluetooth: hci_qca: Replace devm_gpiod_get() with devm_gpiod_get_optional()") changed how qcadev->bt_en is handled as well. From a very strict perspective the new code is buggy because the warnings should only be printed for error pointers, but currently errors are ignored and warnings are printed for NULL.

However this bug does not lead to a crash so I have left it as is.

 drivers/bluetooth/hci_qca.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

drivers/bluetooth/hci_qca.c +9 -9

index e4a68238fcb93..5738ab062a733 100644
@@ -1926,17 +1926,17 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 		}
 
 		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
-		if (!qcadev->susclk) {
+		if (IS_ERR(qcadev->susclk)) {
 			dev_warn(&serdev->dev, "failed to acquire clk\n");
-		} else {
-			err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
-			if (err)
-				return err;
-
-			err = clk_prepare_enable(qcadev->susclk);
-			if (err)
-				return err;
+			return PTR_ERR(qcadev->susclk);
 		}
+		err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
+		if (err)
+			return err;
+
+		err = clk_prepare_enable(qcadev->susclk);
+		if (err)
+			return err;
 
 		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
 		if (err) {

Signature · 1 line

2.26.2

Marcel Holtmann

maintainer

2020-06-03 17:56 UTC #1 lore raw

Hi Dan,

Quoted 25 lines

When a function like devm_clk_get_optional() function returns both error pointers on error and NULL then the NULL return means that the optional feature is deliberately disabled. It is a special sort of success and should not trigger an error message. The surrounding code should be written to check for NULL and not crash.
On the other hand, if we encounter an error, then the probe from should clean up and return a failure.
In this code, if devm_clk_get_optional() returns an error pointer then the kernel will crash inside the call to:
	clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
The error handling must be updated to prevent that.
Fixes 77131dfec6af ("Bluetooth: hci_qca: Replace devm_gpiod_get() with devm_gpiod_get_optional()")
Signed-off-by Dan Carpenter <[email protected]>
---
Commit 77131dfec6af ("Bluetooth: hci_qca: Replace devm_gpiod_get() with devm_gpiod_get_optional()") changed how qcadev->bt_en is handled as well. From a very strict perspective the new code is buggy because the warnings should only be printed for error pointers, but currently errors are ignored and warnings are printed for NULL.

Quoted 4 lines

However this bug does not lead to a crash so I have left it as is.
drivers/bluetooth/hci_qca.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel