Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1533165956-23727-1-git-send-email-rishabhb@codeaurora.org>
In-Reply-To: <1533165956-23727-1-git-send-email-rishabhb@codeaurora.org>
From:   Luis Chamberlain <mcgrof@kernel.org>
Date:   Thu, 2 Aug 2018 14:58:05 -0700
Message-ID: <CAB=NE6XfwMOMgZK9YrE=kq0++wT109bNQQ0QYcu=0vfuuDyF3g@mail.gmail.com>
Subject: Re: [PATCH] firmware: Fix security issue with request_firmware_into_buf()
To:     Rishabh Bhatnagar <rishabhb@codeaurora.org>
Cc:     Mimi Zohar <zohar@linux.vnet.ibm.com>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        ard.biesheuvel@linaro.org, vbabka@suse.cz, riel@surriel.com,
        akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
        ckadabi@codeaurora.org, tsoni@codeaurora.org,
        psodagud@codeaurora.org, Vikram Mulukutla <markivx@codeaurora.org>
Content-Type: multipart/alternative; boundary="000000000000a3f1fa05727ae85c"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

--000000000000a3f1fa05727ae85c
Content-Type: text/plain; charset="UTF-8"

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

--000000000000a3f1fa05727ae85c
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"auto"><div><br><br><div class=3D"gmail_quote"><div dir=3D"ltr">=
On Wed, Aug 1, 2018, 4:26 PM Rishabh Bhatnagar &lt;<a href=3D"mailto:rishab=
hb@codeaurora.org">rishabhb@codeaurora.org</a>&gt; wrote:<br></div><blockqu=
ote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc s=
olid;padding-left:1ex">When calling request_firmware_into_buf() with the FW=
_OPT_NOCACHE flag<br>
it is expected that firmware is loaded into buffer from memory.<br>
But inside alloc_lookup_fw_priv every new firmware that is loaded is<br>
added to the firmware cache (fwc) list head. So if any driver requests<br>
a firmware that is already loaded the code iterates over the above<br>
mentioned list and it can end up giving a pointer to other device driver=
9;s<br>
firmware buffer.<br>
Also the existing copy may either be modified by drivers, remote processors=
<br>
or even freed. This causes a potential security issue with batched requests=
<br>
when using request_firmware_into_buf.<br>
<br>
Fix alloc_lookup_fw_priv to not add to the fwc head list if FW_OPT_NOCACHE<=
br>
is set, and also don&#39;t do the lookup in the list.<br>
<br>
Fixes: 0e742e9275 (&quot;firmware: provide infrastructure to make fw cachin=
g optional&quot;)<br>
<br>
Signed-off-by: Vikram Mulukutla &lt;<a href=3D"mailto:markivx@codeaurora.or=
g" target=3D"_blank" rel=3D"noreferrer">markivx@codeaurora.org</a>&gt;<br>
Signed-off-by: Rishabh Bhatnagar &lt;<a href=3D"mailto:rishabhb@codeaurora.=
org" target=3D"_blank" rel=3D"noreferrer">rishabhb@codeaurora.org</a>&gt;<b=
r>
---</blockquote></div></div><div dir=3D"auto"><br></div><div dir=3D"auto">D=
id you test with the tools/testing/selftests/firmware/ scripts? If not plea=
se do so and report back and confirm no regressions are found.</div><div di=
r=3D"auto"><br></div><div dir=3D"auto">Brownie points for you to add a test=
 case to show the issue highlighted in this patch, and which it fixes. I be=
lieve this fix should be pushed to stable, so I&#39;ll do that after you co=
nfirm no regressions were found.</div><div dir=3D"auto"><br></div><div dir=
=3D"auto">The new selftests changed you&#39;d make would not go to stable, =
however there are Linux distributions and 0day that test the latest tools d=
irectory against older kernels. So this test would help capture gaps later.=
</div><div dir=3D"auto"><br></div><div dir=3D"auto">=C2=A0 Luis</div><div d=
ir=3D"auto"></div></div>

--000000000000a3f1fa05727ae85c--