Re: [PATCH v3 1/2] [FIXED] test_firmware: Fix some racing conditions in test_fw_config locking.

From: Mirsad Goran Todorovac
Date: Sat Apr 08 2023 - 12:22:59 EST

On 08. 04. 2023. 11:33, Mirsad Goran Todorovac wrote:
> On 07. 04. 2023. 23:08, Mirsad Goran Todorovac wrote:
>> On 07. 04. 2023. 11:03, Dan Carpenter wrote:
>>> On Fri, Apr 07, 2023 at 10:24:24AM +0200, Mirsad Goran Todorovac wrote:
>>>>
>>>> Hi Mr. Carpenter,
>>>>
>>>> Thank you for your review.
>>>>
>>>> I will proceed according to your guidelines and issue the next version of the
>>>> patch set.
>>>>
>>>> But I cannot promise it will be before the holidays - I do not want to make
>>>> the gods angry either ;-)
>>>>
>>>
>>> There is never a rush.
>>>
>>>> I cannot promise to try smart macros or inline functions with smart function
>>>> parameters just yet.
>>>>
>>>
>>> Don't worry about that. It just seemed like you were working towards
>>> a more general purpose infrastructure. It's just a clean up.
>>>
>>>> I would consider the real success if I hunt down the remaining leak and races
>>>> in this driver. Despite being considered a less important one.
>>>>
>>>> As you have previously asserted, it is not a real security issue with a CVE,
>>>> however, for completeness sake I would like to see these problems fixed.
>>>
>>> That's great. If you get bored and feel like giving up then just send
>>> PATCH 2/2 by itself because that one could be merged as is.
>>
>> Hi Mr. Carpenter,
>>
>> Actually, I do not like to give up easily :-)
>>
>> I seem to be onto something:
>>
>> In drivers/base/firmware_loader/main.c:
>>
>> 202 static void __free_fw_priv(struct kref *ref)
>> 203 __releases(&fwc->lock)
>> 204 {
>> 205 struct fw_priv *fw_priv = to_fw_priv(ref);
>> 206 struct firmware_cache *fwc = fw_priv->fwc;
>> 207
>> 208 pr_debug("%s: fw-%s fw_priv=%p data=%p size=%u\n",
>> 209 __func__, fw_priv->fw_name, fw_priv, fw_priv->data,
>> 210 (unsigned int)fw_priv->size);
>> 211
>> 212 list_del(&fw_priv->list);
>> 213 spin_unlock(&fwc->lock);
>> 214
>> 215 if (fw_is_paged_buf(fw_priv))
>> 216 fw_free_paged_buf(fw_priv);
>> 217 else if (!fw_priv->allocated_size)
>> 218 vfree(fw_priv->data);
>> 219
>> 220 kfree_const(fw_priv->fw_name);
>> 221 kfree(fw_priv);
>> 222 }
>>
>> This deallocates fw_priv->data only if fpriv->allocated_size == 0
>>
>> 217 else if (!fw_priv->allocated_size)
>> 218 vfree(fw_priv->data);
>>
>> However, this function:
>>
>> 112 static struct fw_priv *__allocate_fw_priv(const char *fw_name,
>> 113 struct firmware_cache *fwc,
>> 114 void *dbuf,
>> 115 size_t size,
>> 116 size_t offset,
>> 117 u32 opt_flags)
>> 118 {
>> 119 struct fw_priv *fw_priv;
>> 120
>> 121 /* For a partial read, the buffer must be preallocated. */
>> 122 if ((opt_flags & FW_OPT_PARTIAL) && !dbuf)
>> 123 return NULL;
>> 124
>> 125 /* Only partial reads are allowed to use an offset. */
>> 126 if (offset != 0 && !(opt_flags & FW_OPT_PARTIAL))
>> 127 return NULL;
>> 128
>> 129 fw_priv = kzalloc(sizeof(*fw_priv), GFP_ATOMIC);
>> 130 if (!fw_priv)
>> 131 return NULL;
>> 132
>> 133 fw_priv->fw_name = kstrdup_const(fw_name, GFP_ATOMIC);
>> 134 if (!fw_priv->fw_name) {
>> 135 kfree(fw_priv);
>> 136 return NULL;
>> 137 }
>> 138
>> 139 kref_init(&fw_priv->ref);
>> 140 fw_priv->fwc = fwc;
>> 141 fw_priv->data = dbuf;
>> 142 fw_priv->allocated_size = size;
>> 143 fw_priv->offset = offset;
>> 144 fw_priv->opt_flags = opt_flags;
>> 145 fw_state_init(fw_priv);
>> 146 #ifdef CONFIG_FW_LOADER_USER_HELPER
>> 147 INIT_LIST_HEAD(&fw_priv->pending_list);
>> 148 #endif
>> 149
>> 150 pr_debug("%s: fw-%s fw_priv=%p\n", __func__, fw_name, fw_priv);
>> 151
>> 152 return fw_priv;
>> 153 }
>>
>> Has both set:
>>
>> 141 fw_priv->data = dbuf;
>> 142 fw_priv->allocated_size = size;
>>
>> I suspect this could be the source of the leak?
>>
>> size in passed all the way down from
>>
>> request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
>> struct device *device, void *buf, size_t size)
>>
>> It is sized test_buf = kzalloc(TEST_FIRMWARE_BUF_SIZE, GFP_KERNEL); which is
>>
>> #define TEST_FIRMWARE_BUF_SIZE SZ_1K
>>
>> (the exact size of the leak: 1024 bytes)
>>
>> I did not dare to fix this, because in other contexts as xz_uncompress this
>> test allocated_size is used with different semantics, and I am not sure what
>> is the right way to fix this:
>>
>> 357 if (!fw_priv->allocated_size)
>> 358 fw_priv->data = out_buf;
>>
>> so I would break this case.
>>
>> Possibly, the way of allocation and allocated_size could be separated?
>>
>> I did not expect the fix to go that deep into the kernel proper?
>>
>> Just to give you a progress report.
>>
>> I might even come up with a fix attempt, but not yet a formal patch I suppose.
>
> P.S.
>
> Apparently, AFAICS, in this context:
>
> lib/test_firmware.c:lines 842-858:
> if (test_fw_config->partial)
> req->rc = request_partial_firmware_into_buf
> (&req->fw,
> req->name,
> req->dev,
> test_buf,
> test_fw_config->buf_size,
> test_fw_config->file_offset);
> else
> req->rc = request_firmware_into_buf
> (&req->fw,
> req->name,
> req->dev,
> test_buf,
> test_fw_config->buf_size);
> if (!req->fw)
> kfree(test_buf);
>
> we call
>
> request_firmware_into_buf(&req->fw, req->name, req->dev, test_buf, test_fw_config->buf_size);
>
> that calls drivers/base/firmware_loader/main.c:1035-1036:
>
> ret = _request_firmware(firmware_p, name, device, buf, size, 0,
> FW_OPT_UEVENT | FW_OPT_NOCACHE);
>
> which calls line 814-815:
>
> ret = _request_firmware_prepare(&fw, name, device, buf, size,
> offset, opt_flags);
>
> which calls line 748-749:
>
> ret = alloc_lookup_fw_priv(name, &fw_cache, &fw_priv, dbuf, size,
> offset, opt_flags);
>
> which calls line 189:
>
> tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, offset, opt_flags);
>
> which does line 112-153:
>
> static struct fw_priv *__allocate_fw_priv(const char *fw_name,
> struct firmware_cache *fwc,
> void *dbuf,
> size_t size,
> size_t offset,
> u32 opt_flags)
> {
> struct fw_priv *fw_priv;
>
> /* For a partial read, the buffer must be preallocated. */
> if ((opt_flags & FW_OPT_PARTIAL) && !dbuf)
> return NULL;
>
> /* Only partial reads are allowed to use an offset. */
> if (offset != 0 && !(opt_flags & FW_OPT_PARTIAL))
> return NULL;
>
> fw_priv = kzalloc(sizeof(*fw_priv), GFP_ATOMIC);
> if (!fw_priv)
> return NULL;
>
> fw_priv->fw_name = kstrdup_const(fw_name, GFP_ATOMIC);
> if (!fw_priv->fw_name) {
> kfree(fw_priv);
> return NULL;
> }
>
> kref_init(&fw_priv->ref);
> fw_priv->fwc = fwc;
> fw_priv->data = dbuf;
> fw_priv->allocated_size = size;
> fw_priv->offset = offset;
> fw_priv->opt_flags = opt_flags;
> fw_state_init(fw_priv);
> #ifdef CONFIG_FW_LOADER_USER_HELPER
> INIT_LIST_HEAD(&fw_priv->pending_list);
> #endif
>
> pr_debug("%s: fw-%s fw_priv=%p\n", __func__, fw_name, fw_priv);
>
> return fw_priv;
> }
>
> Now, fw_priv->data is set to test_buf, fw_priv->allocated_size is set to test_fw_config->buf_size.
>
> When release_firmware(fw) is called, it calls in line drivers/base/firmware_loader/main.c:1081:
>
> firmware_free_data(fw) which calls lines 582-591:
>
> /* firmware holds the ownership of pages */
> static void firmware_free_data(const struct firmware *fw)
> {
> /* Loaded directly? */
> if (!fw->priv) {
> vfree(fw->data);
> return;
> }
> free_fw_priv(fw->priv);
> }
>
> fw_priv is allocated in line 129 of drivers/base/firmware_loader/main.c:
>
> fw_priv = kzalloc(sizeof(*fw_priv), GFP_ATOMIC);
>
> so vfree(fw->data) is not called.
>
> free_fw_priv(fw->priv) is in lines 224-230:
>
> void free_fw_priv(struct fw_priv *fw_priv)
> {
> struct firmware_cache *fwc = fw_priv->fwc;
> spin_lock(&fwc->lock);
> if (!kref_put(&fw_priv->ref, __free_fw_priv))
> spin_unlock(&fwc->lock);
> }
>
> which calls __free_fw_priv(struct kref *ref), lines 202-222:
>
> static void __free_fw_priv(struct kref *ref)
> __releases(&fwc->lock)
> {
> struct fw_priv *fw_priv = to_fw_priv(ref);
> struct firmware_cache *fwc = fw_priv->fwc;
>
> pr_debug("%s: fw-%s fw_priv=%p data=%p size=%u\n",
> __func__, fw_priv->fw_name, fw_priv, fw_priv->data,
> (unsigned int)fw_priv->size);
>
> list_del(&fw_priv->list);
> spin_unlock(&fwc->lock);
>
> if (fw_is_paged_buf(fw_priv))
> fw_free_paged_buf(fw_priv);
> else if (!fw_priv->allocated_size)
> vfree(fw_priv->data);
>
> kfree_const(fw_priv->fw_name);
> kfree(fw_priv);
> }
>
> This has this construct:
>
> 215 if (fw_is_paged_buf(fw_priv))
> 216 fw_free_paged_buf(fw_priv);
> 217 else if (!fw_priv->allocated_size)
> 218 vfree(fw_priv->data);
>
> but as fw->priv was set to test_fw_config->buf_size with the line
>
> 141 fw_priv->data = dbuf;
> 142 fw_priv->allocated_size = size;
>
> apparently vfree(fw_priv->data) is never being called for the firmware loaded
> with
>
> req-rc = request_firmware_into_buf(&req->fw, req->name, req->dev, test_buf,
> test_fw_config->buf_size);
>
> so IMHO we need to release it on the side of the caller, for this is what causes
> the leaks sized test_fw_config->buf_size = TEST_FIRMWARE_BUF_SIZE where
>
> TEST_FIRMWARE_BUF_SIZE is
>
> #define TEST_FIRMWARE_BUF_SIZE SZ_1K
>
> that is, the actual size of the memleaks:
>
> unreferenced object 0xffff9e011c39f000 (size 1024):
> comm "test_firmware-2", pid 27646, jiffies 4302559254 (age 466.216s)
> hex dump (first 32 bytes):
> 45 46 47 48 34 35 36 37 0a 00 00 00 00 00 00 00 EFGH4567........
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<ffffffff9390d59c>] slab_post_alloc_hook+0x8c/0x4f0
> [<ffffffff93914a69>] __kmem_cache_alloc_node+0x1d9/0x2a0
> [<ffffffff93883a9e>] kmalloc_trace+0x2e/0xc0
> [<ffffffff93cd1760>] test_fw_run_batch_request+0x90/0x170
> [<ffffffff935d901a>] kthread+0x11a/0x150
> [<ffffffff93402fc9>] ret_from_fork+0x29/0x50
>
> (71-73 of them per run of tools/testing/selftest/firmware/fw_filesystem.sh)
>
> I hope this helps.

This analysis really helped, and while waiting for the reply I have actually came upon
a fix:

(This v4 probably needs some style changes, as much as I tried to blend in the present
code ...).

---
diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 05ed84c2fc4c..1d7d480b8eeb 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -45,6 +45,7 @@ struct test_batched_req {
bool sent;
const struct firmware *fw;
const char *name;
+ const char *fw_buf;
struct completion completion;
struct task_struct *task;
struct device *dev;
@@ -175,8 +176,14 @@ static void __test_release_all_firmware(void)

for (i = 0; i < test_fw_config->num_requests; i++) {
req = &test_fw_config->reqs[i];
- if (req->fw)
+ if (req->fw) {
+ if (req->fw_buf) {
+ kfree_const(req->fw_buf);
+ req->fw_buf = NULL;
+ }
release_firmware(req->fw);
+ req->fw = NULL;
+ }
}

vfree(test_fw_config->reqs);
@@ -353,16 +360,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(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(buf, size, cfg);
mutex_unlock(&test_fw_mutex);

return ret;
@@ -373,7 +390,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(
+ const char *buf,
size_t size,
size_t *cfg)
{
@@ -384,9 +402,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,7 +418,7 @@ 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(const char *buf, size_t size, u8 *cfg)
+static int __test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
{
u8 val;
int ret;
@@ -411,14 +427,23 @@ static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
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 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;
+}
+
static ssize_t test_dev_config_show_u8(char *buf, u8 val)
{
return snprintf(buf, PAGE_SIZE, "%u\n", val);
@@ -471,10 +496,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(buf, count,
+ &test_fw_config->num_requests);
+ mutex_unlock(&test_fw_mutex);

out:
return rc;
@@ -518,10 +543,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(buf, count,
+ &test_fw_config->buf_size);
+ mutex_unlock(&test_fw_mutex);

out:
return rc;
@@ -548,10 +573,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(buf, count,
+ &test_fw_config->file_offset);
+ mutex_unlock(&test_fw_mutex);

out:
return rc;
@@ -652,6 +677,8 @@ static ssize_t trigger_request_store(struct device *dev,

mutex_lock(&test_fw_mutex);
release_firmware(test_firmware);
+ if (test_fw_config->reqs)
+ __test_release_all_firmware();
test_firmware = NULL;
rc = request_firmware(&test_firmware, name, dev);
if (rc) {
@@ -752,6 +779,8 @@ static ssize_t trigger_async_request_store(struct device *dev,
mutex_lock(&test_fw_mutex);
release_firmware(test_firmware);
test_firmware = NULL;
+ if (test_fw_config->reqs)
+ __test_release_all_firmware();
rc = request_firmware_nowait(THIS_MODULE, 1, name, dev, GFP_KERNEL,
NULL, trigger_async_request_cb);
if (rc) {
@@ -794,6 +823,8 @@ static ssize_t trigger_custom_fallback_store(struct device *dev,

mutex_lock(&test_fw_mutex);
release_firmware(test_firmware);
+ if (test_fw_config->reqs)
+ __test_release_all_firmware();
test_firmware = NULL;
rc = request_firmware_nowait(THIS_MODULE, FW_ACTION_NOUEVENT, name,
dev, GFP_KERNEL, NULL,
@@ -856,6 +887,8 @@ static int test_fw_run_batch_request(void *data)
test_fw_config->buf_size);
if (!req->fw)
kfree(test_buf);
+ else
+ req->fw_buf = test_buf;
} else {
req->rc = test_fw_config->req_firmware(&req->fw,
req->name,
@@ -895,6 +928,11 @@ static ssize_t trigger_batched_requests_store(struct device *dev,

mutex_lock(&test_fw_mutex);

+ if (test_fw_config->reqs) {
+ rc = -EBUSY;
+ goto out_bail;
+ }
+
test_fw_config->reqs =
vzalloc(array3_size(sizeof(struct test_batched_req),
test_fw_config->num_requests, 2));
@@ -911,6 +949,7 @@ static ssize_t trigger_batched_requests_store(struct device *dev,
req->fw = NULL;
req->idx = i;
req->name = test_fw_config->name;
+ req->fw_buf = NULL;
req->dev = dev;
init_completion(&req->completion);
req->task = kthread_run(test_fw_run_batch_request, req,
@@ -993,6 +1032,11 @@ ssize_t trigger_batched_requests_async_store(struct device *dev,

mutex_lock(&test_fw_mutex);

+ if (test_fw_config->reqs) {
+ rc = -EBUSY;
+ goto out_bail;
+ }
+
test_fw_config->reqs =
vzalloc(array3_size(sizeof(struct test_batched_req),
test_fw_config->num_requests, 2));
@@ -1010,6 +1054,7 @@ ssize_t trigger_batched_requests_async_store(struct device *dev,
for (i = 0; i < test_fw_config->num_requests; i++) {
req = &test_fw_config->reqs[i];
req->name = test_fw_config->name;
+ req->fw_buf = NULL;
req->fw = NULL;
req->idx = i;
init_completion(&req->completion);
--

Best regards,
Mirsad

--
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu

System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia
The European Union

"I see something approaching fast ... Will it be friends with me?"