git.ipfire.org Git - thirdparty/kernel/stable-queue.git/blob

   1 From 4acfe3dfde685a5a9eaec5555351918e2d7266a1 Mon Sep 17 00:00:00 2001
   2 From: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
   3 Date: Tue, 9 May 2023 10:47:45 +0200
   4 Subject: test_firmware: prevent race conditions by a correct implementation of locking
   5
   6 From: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
   7
   8 commit 4acfe3dfde685a5a9eaec5555351918e2d7266a1 upstream.
   9
  10 Dan Carpenter spotted a race condition in a couple of situations like
  11 these in the test_firmware driver:
  12
  13 static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
  14 {
  15         u8 val;
  16         int ret;
  17
  18         ret = kstrtou8(buf, 10, &val);
  19         if (ret)
  20                 return ret;
  21
  22         mutex_lock(&test_fw_mutex);
  23         *(u8 *)cfg = val;
  24         mutex_unlock(&test_fw_mutex);
  25
  26         /* Always return full write size even if we didn't consume all */
  27         return size;
  28 }
  29
  30 static ssize_t config_num_requests_store(struct device *dev,
  31                                          struct device_attribute *attr,
  32                                          const char *buf, size_t count)
  33 {
  34         int rc;
  35
  36         mutex_lock(&test_fw_mutex);
  37         if (test_fw_config->reqs) {
  38                 pr_err("Must call release_all_firmware prior to changing config\n");
  39                 rc = -EINVAL;
  40                 mutex_unlock(&test_fw_mutex);
  41                 goto out;
  42         }
  43         mutex_unlock(&test_fw_mutex);
  44
  45         rc = test_dev_config_update_u8(buf, count,
  46                                        &test_fw_config->num_requests);
  47
  48 out:
  49         return rc;
  50 }
  51
  52 static ssize_t config_read_fw_idx_store(struct device *dev,
  53                                         struct device_attribute *attr,
  54                                         const char *buf, size_t count)
  55 {
  56         return test_dev_config_update_u8(buf, count,
  57                                          &test_fw_config->read_fw_idx);
  58 }
  59
  60 The function test_dev_config_update_u8() is called from both the locked
  61 and the unlocked context, function config_num_requests_store() and
  62 config_read_fw_idx_store() which can both be called asynchronously as
  63 they are driver's methods, while test_dev_config_update_u8() and siblings
  64 change their argument pointed to by u8 *cfg or similar pointer.
  65
  66 To avoid deadlock on test_fw_mutex, the lock is dropped before calling
  67 test_dev_config_update_u8() and re-acquired within test_dev_config_update_u8()
  68 itself, but alas this creates a race condition.
  69
  70 Having two locks wouldn't assure a race-proof mutual exclusion.
  71
  72 This situation is best avoided by the introduction of a new, unlocked
  73 function __test_dev_config_update_u8() which can be called from the locked
  74 context and reducing test_dev_config_update_u8() to:
  75
  76 static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
  77 {
  78         int ret;
  79
  80         mutex_lock(&test_fw_mutex);
  81         ret = __test_dev_config_update_u8(buf, size, cfg);
  82         mutex_unlock(&test_fw_mutex);
  83
  84         return ret;
  85 }
  86
  87 doing the locking and calling the unlocked primitive, which enables both
  88 locked and unlocked versions without duplication of code.
  89
  90 The similar approach was applied to all functions called from the locked
  91 and the unlocked context, which safely mitigates both deadlocks and race
  92 conditions in the driver.
  93
  94 __test_dev_config_update_bool(), __test_dev_config_update_u8() and
  95 __test_dev_config_update_size_t() unlocked versions of the functions
  96 were introduced to be called from the locked contexts as a workaround
  97 without releasing the main driver's lock and thereof causing a race
  98 condition.
  99
 100 The test_dev_config_update_bool(), test_dev_config_update_u8() and
 101 test_dev_config_update_size_t() locked versions of the functions
 102 are being called from driver methods without the unnecessary multiplying
 103 of the locking and unlocking code for each method, and complicating
 104 the code with saving of the return value across lock.
 105
 106 Fixes: 7feebfa487b92 ("test_firmware: add support for request_firmware_into_buf")
 107 Cc: Luis Chamberlain <mcgrof@kernel.org>
 108 Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
 109 Cc: Russ Weight <russell.h.weight@intel.com>
 110 Cc: Takashi Iwai <tiwai@suse.de>
 111 Cc: Tianfei Zhang <tianfei.zhang@intel.com>
 112 Cc: Shuah Khan <shuah@kernel.org>
 113 Cc: Colin Ian King <colin.i.king@gmail.com>
 114 Cc: Randy Dunlap <rdunlap@infradead.org>
 115 Cc: linux-kselftest@vger.kernel.org
 116 Cc: stable@vger.kernel.org # v5.4
 117 Suggested-by: Dan Carpenter <error27@gmail.com>
 118 Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
 119 Link: https://lore.kernel.org/r/20230509084746.48259-1-mirsad.todorovac@alu.unizg.hr
 120 Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
 121 ---
 122  lib/test_firmware.c |   37 ++++++++++++++++++++++++++++---------
 123  1 file changed, 28 insertions(+), 9 deletions(-)
 124
 125 --- a/lib/test_firmware.c
 126 +++ b/lib/test_firmware.c
 127 @@ -301,16 +301,26 @@ static ssize_t config_test_show_str(char
 128         return len;
 129  }
 130
 131 -static int test_dev_config_update_bool(const char *buf, size_t size,
 132 -                                      bool *cfg)
 133 +static inline int __test_dev_config_update_bool(const char *buf, size_t size,
 134 +                                               bool *cfg)
 135  {
 136         int ret;
 137
 138 -       mutex_lock(&test_fw_mutex);
 139         if (strtobool(buf, cfg) < 0)
 140                 ret = -EINVAL;
 141         else
 142                 ret = size;
 143 +
 144 +       return ret;
 145 +}
 146 +
 147 +static int test_dev_config_update_bool(const char *buf, size_t size,
 148 +                                      bool *cfg)
 149 +{
 150 +       int ret;
 151 +
 152 +       mutex_lock(&test_fw_mutex);
 153 +       ret = __test_dev_config_update_bool(buf, size, cfg);
 154         mutex_unlock(&test_fw_mutex);
 155
 156         return ret;
 157 @@ -340,7 +350,7 @@ static ssize_t test_dev_config_show_int(
 158         return snprintf(buf, PAGE_SIZE, "%d\n", val);
 159  }
 160
 161 -static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
 162 +static inline int __test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
 163  {
 164         int ret;
 165         long new;
 166 @@ -352,14 +362,23 @@ static int test_dev_config_update_u8(con
 167         if (new > U8_MAX)
 168                 return -EINVAL;
 169
 170 -       mutex_lock(&test_fw_mutex);
 171         *(u8 *)cfg = new;
 172 -       mutex_unlock(&test_fw_mutex);
 173
 174         /* Always return full write size even if we didn't consume all */
 175         return size;
 176  }
 177
 178 +static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
 179 +{
 180 +       int ret;
 181 +
 182 +       mutex_lock(&test_fw_mutex);
 183 +       ret = __test_dev_config_update_u8(buf, size, cfg);
 184 +       mutex_unlock(&test_fw_mutex);
 185 +
 186 +       return ret;
 187 +}
 188 +
 189  static ssize_t test_dev_config_show_u8(char *buf, u8 cfg)
 190  {
 191         u8 val;
 192 @@ -392,10 +411,10 @@ static ssize_t config_num_requests_store
 193                 mutex_unlock(&test_fw_mutex);
 194                 goto out;
 195         }
 196 -       mutex_unlock(&test_fw_mutex);
 197
 198 -       rc = test_dev_config_update_u8(buf, count,
 199 -                                      &test_fw_config->num_requests);
 200 +       rc = __test_dev_config_update_u8(buf, count,
 201 +                                        &test_fw_config->num_requests);
 202 +       mutex_unlock(&test_fw_mutex);
 203
 204  out:
 205         return rc;