When calling request_firmware_into_buf() with the FW_OPT_NOCACHE flag
it is expected that firmware is loaded into buffer from memory.
But inside alloc_lookup_fw_priv every new firmware that is loaded is
added to the firmware cache (fwc) list head. So if any driver requests
a firmware that is already loaded the code iterates over the above
mentioned list and it can end up giving a pointer to other device driver's
firmware buffer.
Also the existing copy may either be modified by drivers, remote processors
or even freed. This causes a potential security issue with batched requests
when using request_firmware_into_buf.
Fix alloc_lookup_fw_priv to not add to the fwc head list if FW_OPT_NOCACHE
is set, and also don't do the lookup in the list.
Fixes: 0e742e9275 ("firmware: provide infrastructure to make fw caching optional")
Signed-off-by: Vikram Mulukutla <[email protected]>
Signed-off-by: Rishabh Bhatnagar <[email protected]>
---
drivers/base/firmware_loader/main.c | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 0943e70..b3c0498 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -209,21 +209,24 @@ static struct fw_priv *__lookup_fw_priv(const char *fw_name)
static int alloc_lookup_fw_priv(const char *fw_name,
struct firmware_cache *fwc,
struct fw_priv **fw_priv, void *dbuf,
- size_t size)
+ size_t size, enum fw_opt opt_flags)
{
struct fw_priv *tmp;
spin_lock(&fwc->lock);
- tmp = __lookup_fw_priv(fw_name);
- if (tmp) {
- kref_get(&tmp->ref);
- spin_unlock(&fwc->lock);
- *fw_priv = tmp;
- pr_debug("batched request - sharing the same struct fw_priv and lookup for multiple requests\n");
- return 1;
+ if (!(opt_flags & FW_OPT_NOCACHE)) {
+ tmp = __lookup_fw_priv(fw_name);
+ if (tmp) {
+ kref_get(&tmp->ref);
+ spin_unlock(&fwc->lock);
+ *fw_priv = tmp;
+ pr_debug("batched request - sharing the same struct fw_priv and lookup for multiple requests\n");
+ return 1;
+ }
}
+
tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size);
- if (tmp)
+ if (tmp && !(opt_flags & FW_OPT_NOCACHE))
list_add(&tmp->list, &fwc->head);
spin_unlock(&fwc->lock);
@@ -493,7 +496,8 @@ int assign_fw(struct firmware *fw, struct device *device,
*/
static int
_request_firmware_prepare(struct firmware **firmware_p, const char *name,
- struct device *device, void *dbuf, size_t size)
+ struct device *device, void *dbuf, size_t size,
+ enum fw_opt opt_flags)
{
struct firmware *firmware;
struct fw_priv *fw_priv;
@@ -511,7 +515,8 @@ int assign_fw(struct firmware *fw, struct device *device,
return 0; /* assigned */
}
- ret = alloc_lookup_fw_priv(name, &fw_cache, &fw_priv, dbuf, size);
+ ret = alloc_lookup_fw_priv(name, &fw_cache, &fw_priv, dbuf, size,
+ opt_flags);
/*
* bind with 'priv' now to avoid warning in failure path
@@ -571,7 +576,8 @@ static void fw_abort_batch_reqs(struct firmware *fw)
goto out;
}
- ret = _request_firmware_prepare(&fw, name, device, buf, size);
+ ret = _request_firmware_prepare(&fw, name, device, buf, size,
+ opt_flags);
if (ret <= 0) /* error or already assigned */
goto out;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On Wed, Aug 1, 2018, 4:26 PM Rishabh Bhatnagar <[email protected]>
wrote:
> When calling request_firmware_into_buf() with the FW_OPT_NOCACHE flag
> it is expected that firmware is loaded into buffer from memory.
> But inside alloc_lookup_fw_priv every new firmware that is loaded is
> added to the firmware cache (fwc) list head. So if any driver requests
> a firmware that is already loaded the code iterates over the above
> mentioned list and it can end up giving a pointer to other device driver's
> firmware buffer.
> Also the existing copy may either be modified by drivers, remote processors
> or even freed. This causes a potential security issue with batched requests
> when using request_firmware_into_buf.
>
> Fix alloc_lookup_fw_priv to not add to the fwc head list if FW_OPT_NOCACHE
> is set, and also don't do the lookup in the list.
>
> Fixes: 0e742e9275 ("firmware: provide infrastructure to make fw caching
> optional")
>
> Signed-off-by: Vikram Mulukutla <[email protected]>
> Signed-off-by: Rishabh Bhatnagar <[email protected]>
> ---
Did you test with the tools/testing/selftests/firmware/ scripts? If not
please do so and report back and confirm no regressions are found.
Brownie points for you to add a test case to show the issue highlighted in
this patch, and which it fixes. I believe this fix should be pushed to
stable, so I'll do that after you confirm no regressions were found.
The new selftests changed you'd make would not go to stable, however there
are Linux distributions and 0day that test the latest tools directory
against older kernels. So this test would help capture gaps later.
Luis
On 2018-08-02 14:58, Luis Chamberlain wrote:
> On Wed, Aug 1, 2018, 4:26 PM Rishabh Bhatnagar
> <[email protected]> wrote:
>
>> When calling request_firmware_into_buf() with the FW_OPT_NOCACHE
>> flag
>> it is expected that firmware is loaded into buffer from memory.
>> But inside alloc_lookup_fw_priv every new firmware that is loaded is
>> added to the firmware cache (fwc) list head. So if any driver
>> requests
>> a firmware that is already loaded the code iterates over the above
>> mentioned list and it can end up giving a pointer to other device
>> driver's
>> firmware buffer.
>> Also the existing copy may either be modified by drivers, remote
>> processors
>> or even freed. This causes a potential security issue with batched
>> requests
>> when using request_firmware_into_buf.
>>
>> Fix alloc_lookup_fw_priv to not add to the fwc head list if
>> FW_OPT_NOCACHE
>> is set, and also don't do the lookup in the list.
>>
>> Fixes: 0e742e9275 ("firmware: provide infrastructure to make fw
>> caching optional")
>>
>> Signed-off-by: Vikram Mulukutla <[email protected]>
>> Signed-off-by: Rishabh Bhatnagar <[email protected]>
>> ---
>
> Did you test with the tools/testing/selftests/firmware/ scripts? If
> not please do so and report back and confirm no regressions are found.
>
> Brownie points for you to add a test case to show the issue
> highlighted in this patch, and which it fixes. I believe this fix
> should be pushed to stable, so I'll do that after you confirm no
> regressions were found.
>
> The new selftests changed you'd make would not go to stable, however
> there are Linux distributions and 0day that test the latest tools
> directory against older kernels. So this test would help capture gaps
> later.
>
> Luis
I ran the selftests and observed no regressions with this change.
I'm still working on adding a test case though.
-Rishabh