Skip to content

Commit 4acfe3d

Browse files
Mirsad Goran Todorovacgregkh
Mirsad Goran Todorovac
authored and
gregkh
committed
test_firmware: prevent race conditions by a correct implementation of locking
Dan Carpenter spotted a race condition in a couple of situations like these in the test_firmware driver: static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg) { u8 val; int ret; ret = kstrtou8(buf, 10, &val); if (ret) return ret; mutex_lock(&test_fw_mutex); *(u8 *)cfg = val; mutex_unlock(&test_fw_mutex); /* Always return full write size even if we didn't consume all */ return size; } static ssize_t config_num_requests_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { int rc; mutex_lock(&test_fw_mutex); if (test_fw_config->reqs) { pr_err("Must call release_all_firmware prior to changing config\n"); rc = -EINVAL; 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); out: return rc; } static ssize_t config_read_fw_idx_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { return test_dev_config_update_u8(buf, count, &test_fw_config->read_fw_idx); } The function test_dev_config_update_u8() is called from both the locked and the unlocked context, function config_num_requests_store() and config_read_fw_idx_store() which can both be called asynchronously as they are driver's methods, while test_dev_config_update_u8() and siblings change their argument pointed to by u8 *cfg or similar pointer. To avoid deadlock on test_fw_mutex, the lock is dropped before calling test_dev_config_update_u8() and re-acquired within test_dev_config_update_u8() itself, but alas this creates a race condition. Having two locks wouldn't assure a race-proof mutual exclusion. This situation is best avoided by the introduction of a new, unlocked function __test_dev_config_update_u8() which can be called from the locked context and reducing test_dev_config_update_u8() to: static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg) { int ret; mutex_lock(&test_fw_mutex); ret = __test_dev_config_update_u8(buf, size, cfg); mutex_unlock(&test_fw_mutex); return ret; } doing the locking and calling the unlocked primitive, which enables both locked and unlocked versions without duplication of code. The similar approach was applied to all functions called from the locked and the unlocked context, which safely mitigates both deadlocks and race conditions in the driver. __test_dev_config_update_bool(), __test_dev_config_update_u8() 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 thereof causing a race condition. The test_dev_config_update_bool(), test_dev_config_update_u8() and test_dev_config_update_size_t() locked versions of the functions are being called from driver methods without the unnecessary multiplying of the locking and unlocking code for each method, and complicating the code with saving of the return value across lock. Fixes: 7feebfa ("test_firmware: add support for request_firmware_into_buf") Cc: Luis Chamberlain <mcgrof@kernel.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Russ Weight <russell.h.weight@intel.com> Cc: Takashi Iwai <tiwai@suse.de> Cc: Tianfei Zhang <tianfei.zhang@intel.com> Cc: Shuah Khan <shuah@kernel.org> Cc: Colin Ian King <colin.i.king@gmail.com> Cc: Randy Dunlap <rdunlap@infradead.org> Cc: linux-kselftest@vger.kernel.org Cc: stable@vger.kernel.org # v5.4 Suggested-by: Dan Carpenter <error27@gmail.com> Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr> Link: https://lore.kernel.org/r/20230509084746.48259-1-mirsad.todorovac@alu.unizg.hr Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
1 parent ffa2831 commit 4acfe3d

File tree

1 file changed

+35
-17
lines changed

1 file changed

+35
-17
lines changed

‎lib/test_firmware.c

Lines changed: 35 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -353,16 +353,26 @@ static ssize_t config_test_show_str(char *dst,
353353
return len;
354354
}
355355

356-
static int test_dev_config_update_bool(const char *buf, size_t size,
356+
static inline int __test_dev_config_update_bool(const char *buf, size_t size,
357357
bool *cfg)
358358
{
359359
int ret;
360360

361-
mutex_lock(&test_fw_mutex);
362361
if (kstrtobool(buf, cfg) < 0)
363362
ret = -EINVAL;
364363
else
365364
ret = size;
365+
366+
return ret;
367+
}
368+
369+
static int test_dev_config_update_bool(const char *buf, size_t size,
370+
bool *cfg)
371+
{
372+
int ret;
373+
374+
mutex_lock(&test_fw_mutex);
375+
ret = __test_dev_config_update_bool(buf, size, cfg);
366376
mutex_unlock(&test_fw_mutex);
367377

368378
return ret;
@@ -373,7 +383,8 @@ static ssize_t test_dev_config_show_bool(char *buf, bool val)
373383
return snprintf(buf, PAGE_SIZE, "%d\n", val);
374384
}
375385

376-
static int test_dev_config_update_size_t(const char *buf,
386+
static int __test_dev_config_update_size_t(
387+
const char *buf,
377388
size_t size,
378389
size_t *cfg)
379390
{
@@ -384,9 +395,7 @@ static int test_dev_config_update_size_t(const char *buf,
384395
if (ret)
385396
return ret;
386397

387-
mutex_lock(&test_fw_mutex);
388398
*(size_t *)cfg = new;
389-
mutex_unlock(&test_fw_mutex);
390399

391400
/* Always return full write size even if we didn't consume all */
392401
return size;
@@ -402,7 +411,7 @@ static ssize_t test_dev_config_show_int(char *buf, int val)
402411
return snprintf(buf, PAGE_SIZE, "%d\n", val);
403412
}
404413

405-
static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
414+
static int __test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
406415
{
407416
u8 val;
408417
int ret;
@@ -411,14 +420,23 @@ static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
411420
if (ret)
412421
return ret;
413422

414-
mutex_lock(&test_fw_mutex);
415423
*(u8 *)cfg = val;
416-
mutex_unlock(&test_fw_mutex);
417424

418425
/* Always return full write size even if we didn't consume all */
419426
return size;
420427
}
421428

429+
static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
430+
{
431+
int ret;
432+
433+
mutex_lock(&test_fw_mutex);
434+
ret = __test_dev_config_update_u8(buf, size, cfg);
435+
mutex_unlock(&test_fw_mutex);
436+
437+
return ret;
438+
}
439+
422440
static ssize_t test_dev_config_show_u8(char *buf, u8 val)
423441
{
424442
return snprintf(buf, PAGE_SIZE, "%u\n", val);
@@ -471,10 +489,10 @@ static ssize_t config_num_requests_store(struct device *dev,
471489
mutex_unlock(&test_fw_mutex);
472490
goto out;
473491
}
474-
mutex_unlock(&test_fw_mutex);
475492

476-
rc = test_dev_config_update_u8(buf, count,
477-
&test_fw_config->num_requests);
493+
rc = __test_dev_config_update_u8(buf, count,
494+
&test_fw_config->num_requests);
495+
mutex_unlock(&test_fw_mutex);
478496

479497
out:
480498
return rc;
@@ -518,10 +536,10 @@ static ssize_t config_buf_size_store(struct device *dev,
518536
mutex_unlock(&test_fw_mutex);
519537
goto out;
520538
}
521-
mutex_unlock(&test_fw_mutex);
522539

523-
rc = test_dev_config_update_size_t(buf, count,
524-
&test_fw_config->buf_size);
540+
rc = __test_dev_config_update_size_t(buf, count,
541+
&test_fw_config->buf_size);
542+
mutex_unlock(&test_fw_mutex);
525543

526544
out:
527545
return rc;
@@ -548,10 +566,10 @@ static ssize_t config_file_offset_store(struct device *dev,
548566
mutex_unlock(&test_fw_mutex);
549567
goto out;
550568
}
551-
mutex_unlock(&test_fw_mutex);
552569

553-
rc = test_dev_config_update_size_t(buf, count,
554-
&test_fw_config->file_offset);
570+
rc = __test_dev_config_update_size_t(buf, count,
571+
&test_fw_config->file_offset);
572+
mutex_unlock(&test_fw_mutex);
555573

556574
out:
557575
return rc;

0 commit comments

Comments
 (0)