linux-kernel - [PATCH v3 1/2] test_firmware: Fix some racing conditions in test_fw

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: <20230406015315.31505-1-mirsad.todorovac@alu.unizg.hr>
Date:   Thu,  6 Apr 2023 03:53:17 +0200
From:   Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Russ Weight <russell.h.weight@...el.com>,
        linux-kernel@...r.kernel.org
Cc:     Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Tianfei zhang <tianfei.zhang@...el.com>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Zhengchao Shao <shaozhengchao@...wei.com>,
        Colin Ian King <colin.i.king@...il.com>,
        Takashi Iwai <tiwai@...e.de>, Dan Carpenter <error27@...il.com>
Subject: [PATCH v3 1/2] test_firmware: Fix some racing conditions in test_fw_config locking.

Some functions were called both from locked and unlocked context, so the lock
was dropped prematurely, introducing a race condition when deadlock was avoided.

Having two locks wouldn't assure a race-proof mutual exclusion.

test_dev_config_update_bool_unlocked(), test_dev_config_update_u8_unlocked()
and test_dev_config_update_size_t_unlocked() versions of the functions were
introduced to be called from the locked contexts as a workaround without
releasing the main driver's lock and causing a race condition, much like putc()
and putc_unlocked() in stdio glibc library.

This should guarantee mutual exclusion and prevent any race conditions.

Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org> 
Cc: Luis Chamberlain <mcgrof@...nel.org> 
Cc: Russ Weight <russell.h.weight@...el.com> 
Cc: Tianfei zhang <tianfei.zhang@...el.com> 
Cc: Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr> 
Cc: Christophe JAILLET <christophe.jaillet@...adoo.fr> 
Cc: Zhengchao Shao <shaozhengchao@...wei.com> 
Cc: Colin Ian King <colin.i.king@...il.com> 
Cc: linux-kernel@...r.kernel.org 
Cc: Takashi Iwai <tiwai@...e.de>
Suggested-by: Dan Carpenter <error27@...il.com>
Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>
---
 lib/test_firmware.c | 52 +++++++++++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 14 deletions(-)

diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 05ed84c2fc4c..272af8dc54b0 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -353,16 +353,26 @@ static ssize_t config_test_show_str(char *dst,
 	return len;
 }
 
-static int test_dev_config_update_bool(const char *buf, size_t size,
+static inline int test_dev_config_update_bool_unlocked(const char *buf, size_t size,
 				       bool *cfg)
 {
 	int ret;
 
-	mutex_lock(&test_fw_mutex);
 	if (kstrtobool(buf, cfg) < 0)
 		ret = -EINVAL;
 	else
 		ret = size;
+
+	return ret;
+}
+
+static int test_dev_config_update_bool(const char *buf, size_t size,
+				       bool *cfg)
+{
+	int ret;
+
+	mutex_lock(&test_fw_mutex);
+	ret = test_dev_config_update_bool_unlocked(buf, size, cfg);
 	mutex_unlock(&test_fw_mutex);
 
 	return ret;
@@ -373,7 +383,8 @@ static ssize_t test_dev_config_show_bool(char *buf, bool val)
 	return snprintf(buf, PAGE_SIZE, "%d\n", val);
 }
 
-static int test_dev_config_update_size_t(const char *buf,
+static int test_dev_config_update_size_t_unlocked(
+					 const char *buf,
 					 size_t size,
 					 size_t *cfg)
 {
@@ -384,9 +395,7 @@ static int test_dev_config_update_size_t(const char *buf,
 	if (ret)
 		return ret;
 
-	mutex_lock(&test_fw_mutex);
 	*(size_t *)cfg = new;
-	mutex_unlock(&test_fw_mutex);
 
 	/* Always return full write size even if we didn't consume all */
 	return size;
@@ -402,6 +411,21 @@ static ssize_t test_dev_config_show_int(char *buf, int val)
 	return snprintf(buf, PAGE_SIZE, "%d\n", val);
 }
 
+static int test_dev_config_update_u8_unlocked(const char *buf, size_t size, u8 *cfg)
+{
+	u8 val;
+	int ret;
+
+	ret = kstrtou8(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	*(u8 *)cfg = val;
+
+	/* Always return full write size even if we didn't consume all */
+	return size;
+}
+
 static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
 {
 	u8 val;
@@ -471,10 +495,10 @@ static ssize_t config_num_requests_store(struct device *dev,
 		mutex_unlock(&test_fw_mutex);
 		goto out;
 	}
-	mutex_unlock(&test_fw_mutex);
 
-	rc = test_dev_config_update_u8(buf, count,
-				       &test_fw_config->num_requests);
+	rc = test_dev_config_update_u8_unlocked(buf, count,
+						&test_fw_config->num_requests);
+	mutex_unlock(&test_fw_mutex);
 
 out:
 	return rc;
@@ -518,10 +542,10 @@ static ssize_t config_buf_size_store(struct device *dev,
 		mutex_unlock(&test_fw_mutex);
 		goto out;
 	}
-	mutex_unlock(&test_fw_mutex);
 
-	rc = test_dev_config_update_size_t(buf, count,
-					   &test_fw_config->buf_size);
+	rc = test_dev_config_update_size_t_unlocked(buf, count,
+						    &test_fw_config->buf_size);
+	mutex_unlock(&test_fw_mutex);
 
 out:
 	return rc;
@@ -548,10 +572,10 @@ static ssize_t config_file_offset_store(struct device *dev,
 		mutex_unlock(&test_fw_mutex);
 		goto out;
 	}
-	mutex_unlock(&test_fw_mutex);
 
-	rc = test_dev_config_update_size_t(buf, count,
-					   &test_fw_config->file_offset);
+	rc = test_dev_config_update_size_t_unlocked(buf, count,
+						    &test_fw_config->file_offset);
+	mutex_unlock(&test_fw_mutex);
 
 out:
 	return rc;
-- 
2.30.2