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 <[email protected]>
Cc: Luis Chamberlain <[email protected]>
Cc: Russ Weight <[email protected]>
Cc: Tianfei zhang <[email protected]>
Cc: Mirsad Goran Todorovac <[email protected]>
Cc: Christophe JAILLET <[email protected]>
Cc: Zhengchao Shao <[email protected]>
Cc: Colin Ian King <[email protected]>
Cc: [email protected]
Cc: Takashi Iwai <[email protected]>
Suggested-by: Dan Carpenter <[email protected]>
Signed-off-by: Mirsad Goran Todorovac <[email protected]>
---
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
trigger_batched_requests_store() and trigger_batched_requests_async_store()
both caused test_fw_config->reqs ptr to be overwritten with the new call to
either function and the vzalloc() call, leaving the old memory object
unreferenced.
Semantically the most simple and prudent solution seemed to be returning the
-EBUSY errno in this case, rather than permitting a kernel memory leak.
However, this did fix closed only these obvious leaks, not all that are
present in the test firmware loader.
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Luis Chamberlain <[email protected]>
Cc: Russ Weight <[email protected]>
Cc: Tianfei zhang <[email protected]>
Cc: Mirsad Goran Todorovac <[email protected]>
Cc: Christophe JAILLET <[email protected]>
Cc: Zhengchao Shao <[email protected]>
Cc: Colin Ian King <[email protected]>
Cc: [email protected]
Suggested-by: Dan Carpenter <[email protected]>
Suggested-by: Takashi Iwai <[email protected]>
Signed-off-by: Mirsad Goran Todorovac <[email protected]>
---
lib/test_firmware.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 272af8dc54b0..b81f5621626e 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -919,6 +919,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));
@@ -1017,6 +1022,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));
--
2.30.2
On Thu, Apr 06, 2023 at 03:53:17AM +0200, Mirsad Goran Todorovac wrote:
> 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.
>
Thanks for figuring this out! It seems like a good approach to me.
However, I feel like PATCH 1/1 needs some style changes.
The question you seem to be dealing with is how consistent to be and how
much infrastructure to create. Don't think about that. Just fix the
bug in the most minimal way possible and don't worry about being
consistent.
(Probably the best way to make this consistent is to change the
test_dev_config_update_XXX functions into a single macro that calls the
correct kstroXXX function. Then create a second macro that takes the
lock and calls the first macro. But that is a clean up patch and
unrelated to this bug.)
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Luis Chamberlain <[email protected]>
> Cc: Russ Weight <[email protected]>
> Cc: Tianfei zhang <[email protected]>
> Cc: Mirsad Goran Todorovac <[email protected]>
> Cc: Christophe JAILLET <[email protected]>
> Cc: Zhengchao Shao <[email protected]>
> Cc: Colin Ian King <[email protected]>
> Cc: [email protected]
> Cc: Takashi Iwai <[email protected]>
> Suggested-by: Dan Carpenter <[email protected]>
> Signed-off-by: Mirsad Goran Todorovac <[email protected]>
> ---
> 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;
> +}
This change can be left out completely.
> +
> +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)
> {
Do not rename this function. Just add a comment that the mutext must be
held. Or a WARN_ONCE().
WARN_ON_ONCE(!mutex_is_locked(&test_fw_mutex));
> @@ -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;
> +}
> +
Just change the test_dev_config_update_u8() to not take the lock.
Add the comment that the lock must be held. Change both callers to take
the lock.
Otherwise we end up creating too much duplicate code.
> static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
> {
> u8 val;
regards,
dan carpenter
On 6.4.2023. 16:04, Dan Carpenter wrote:
> On Thu, Apr 06, 2023 at 03:53:17AM +0200, Mirsad Goran Todorovac wrote:
>> 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.
>>
>
> Thanks for figuring this out! It seems like a good approach to me.
> However, I feel like PATCH 1/1 needs some style changes.
>
> The question you seem to be dealing with is how consistent to be and how
> much infrastructure to create. Don't think about that. Just fix the
> bug in the most minimal way possible and don't worry about being
> consistent.
>
> (Probably the best way to make this consistent is to change the
> test_dev_config_update_XXX functions into a single macro that calls the
> correct kstroXXX function. Then create a second macro that takes the
> lock and calls the first macro. But that is a clean up patch and
> unrelated to this bug.)
>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: Luis Chamberlain <[email protected]>
>> Cc: Russ Weight <[email protected]>
>> Cc: Tianfei zhang <[email protected]>
>> Cc: Mirsad Goran Todorovac <[email protected]>
>> Cc: Christophe JAILLET <[email protected]>
>> Cc: Zhengchao Shao <[email protected]>
>> Cc: Colin Ian King <[email protected]>
>> Cc: [email protected]
>> Cc: Takashi Iwai <[email protected]>
>> Suggested-by: Dan Carpenter <[email protected]>
>> Signed-off-by: Mirsad Goran Todorovac <[email protected]>
>> ---
>> 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;
>> +}
>
> This change can be left out completely.
>
>> +
>> +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)
>> {
>
> Do not rename this function. Just add a comment that the mutext must be
> held. Or a WARN_ONCE().
>
> WARN_ON_ONCE(!mutex_is_locked(&test_fw_mutex));
>
>
>> @@ -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;
>> +}
>> +
>
> Just change the test_dev_config_update_u8() to not take the lock.
> Add the comment that the lock must be held. Change both callers to take
> the lock.
>
>
> Otherwise we end up creating too much duplicate code.
>
>> static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
>> {
>> u8 val;
>
> regards,
> dan carpenter
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 ;-)
I cannot promise to try smart macros or inline functions with smart function
parameters just yet.
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.
Best regards,
Mirsad
--
Mirsad Todorovac
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb
Republic of Croatia, the European Union
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu
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.
regards,
dan carpenter
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.
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?"
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.
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?"
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?"