* [PATCH] firmware: Fix security issue with request_firmware_into_buf()
@ 2018-08-31 15:43 Luis R. Rodriguez
0 siblings, 0 replies; 3+ messages in thread
From: Luis R. Rodriguez @ 2018-08-31 15:43 UTC (permalink / raw)
To: gregkh
Cc: akpm, josh, rishabhb, kubakici, maco, andy.gross, david.brown,
bjorn.andersson, linux-wireless, keescook, shuah, mfuzzey, zohar,
dhowells, pali.rohar, tiwai, arend.vanspriel, zajec5, nbroeking,
markivx, broonie, dmitry.torokhov, dwmw2, torvalds,
Abhay_Salunke, jewalt, cantabile.desu, ast, andresx7,
linux-kernel, linux-fsdevel, stable, Luis Chamberlain
From: Rishabh Bhatnagar <rishabhb@codeaurora.org>
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")
[mcgrof: broken since feature introduction on v4.8]
Cc: stable@vger.kernel.org # v4.8+
Signed-off-by: Vikram Mulukutla <markivx@codeaurora.org>
Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
This has been tested with the self test firmware scrip and found no
regressions.
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 0943e7065e0e..b3c0498ee433 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 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
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 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
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;
--
2.18.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] firmware: Fix security issue with request_firmware_into_buf()
[not found] ` <CAB=NE6XfwMOMgZK9YrE=kq0++wT109bNQQ0QYcu=0vfuuDyF3g@mail.gmail.com>
@ 2018-08-07 22:41 ` rishabhb
0 siblings, 0 replies; 3+ messages in thread
From: rishabhb @ 2018-08-07 22:41 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Mimi Zohar, Bjorn Andersson, ard.biesheuvel, vbabka, riel, akpm,
linux-kernel, ckadabi, tsoni, psodagud, Vikram Mulukutla
On 2018-08-02 14:58, Luis Chamberlain wrote:
> On Wed, Aug 1, 2018, 4:26 PM Rishabh Bhatnagar
> <rishabhb@codeaurora.org> 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 <markivx@codeaurora.org>
>> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>> ---
>
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] firmware: Fix security issue with request_firmware_into_buf()
@ 2018-08-01 23:25 Rishabh Bhatnagar
[not found] ` <CAB=NE6XfwMOMgZK9YrE=kq0++wT109bNQQ0QYcu=0vfuuDyF3g@mail.gmail.com>
0 siblings, 1 reply; 3+ messages in thread
From: Rishabh Bhatnagar @ 2018-08-01 23:25 UTC (permalink / raw)
To: mcgrof, zohar, bjorn.andersson, ard.biesheuvel, vbabka, riel,
akpm, linux-kernel
Cc: ckadabi, tsoni, psodagud, Rishabh Bhatnagar, Vikram Mulukutla
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 <markivx@codeaurora.org>
Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-08-31 15:43 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-31 15:43 [PATCH] firmware: Fix security issue with request_firmware_into_buf() Luis R. Rodriguez
-- strict thread matches above, loose matches on Subject: below --
2018-08-01 23:25 Rishabh Bhatnagar
[not found] ` <CAB=NE6XfwMOMgZK9YrE=kq0++wT109bNQQ0QYcu=0vfuuDyF3g@mail.gmail.com>
2018-08-07 22:41 ` rishabhb
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).