Device drivers may decide to not load firmware when probed to avoid
slowing down the boot process should the firmware filesystem not be
available yet. In this case, the firmware loading request may be done
when a device file associated with the driver is first accessed. The
credentials of the userspace process accessing the device file may be
used to validate access to the firmware files requested by the driver.
Ensure that the kernel assumes the responsibility of reading the
firmware.
This was observed on Android for a graphic driver loading their firmware
when the device file (e.g. /dev/mali0) was first opened by userspace
(i.e. surfaceflinger). The security context of surfaceflinger was used
to validate the access to the firmware file (e.g.
/vendor/firmware/mali.bin).
Because previous configurations were relying on the userspace fallback
mechanism, the security context of the userspace daemon (i.e. ueventd)
was consistently used to read firmware files. More devices are found to
use the command line argument firmware_class.path which gives the kernel
the opportunity to read the firmware directly, hence surfacing this
misattribution.
Signed-off-by: Thiébaud Weksteen <[email protected]>
---
v2: Add comment
drivers/base/firmware_loader/main.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 94d1789a233e..8f3c2b2cfc61 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -735,6 +735,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
size_t offset, u32 opt_flags)
{
struct firmware *fw = NULL;
+ struct cred *kern_cred = NULL;
+ const struct cred *old_cred;
bool nondirect = false;
int ret;
@@ -751,6 +753,18 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
if (ret <= 0) /* error or already assigned */
goto out;
+ /*
+ * We are about to try to access the firmware file. Because we may have been
+ * called by a driver when serving an unrelated request from userland, we use
+ * the kernel credentials to read the file.
+ */
+ kern_cred = prepare_kernel_cred(NULL);
+ if (!kern_cred) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ old_cred = override_creds(kern_cred);
+
ret = fw_get_filesystem_firmware(device, fw->priv, "", NULL);
/* Only full reads can support decompression, platform, and sysfs. */
@@ -776,6 +790,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
} else
ret = assign_fw(fw, device);
+ revert_creds(old_cred);
+
out:
if (ret < 0) {
fw_abort_batch_reqs(fw);
--
2.36.0.rc2.479.g8af0fa9b8e-goog
On Fri, Apr 22, 2022 at 11:32:15AM +1000, Thi?baud Weksteen wrote:
> Device drivers may decide to not load firmware when probed to avoid
> slowing down the boot process should the firmware filesystem not be
> available yet. In this case, the firmware loading request may be done
> when a device file associated with the driver is first accessed. The
> credentials of the userspace process accessing the device file may be
> used to validate access to the firmware files requested by the driver.
> Ensure that the kernel assumes the responsibility of reading the
> firmware.
>
> This was observed on Android for a graphic driver loading their firmware
> when the device file (e.g. /dev/mali0) was first opened by userspace
> (i.e. surfaceflinger). The security context of surfaceflinger was used
> to validate the access to the firmware file (e.g.
> /vendor/firmware/mali.bin).
>
> Because previous configurations were relying on the userspace fallback
> mechanism, the security context of the userspace daemon (i.e. ueventd)
> was consistently used to read firmware files. More devices are found to
> use the command line argument firmware_class.path which gives the kernel
> the opportunity to read the firmware directly, hence surfacing this
> misattribution.
Can you elaborate on the last sentence? It's unclear how what you
describe is used exactly to allow driver to use direct filesystem
firmware loading.
And, given the feedback from Android it would seem this is a fix
which likely may be desirable to backport to some stable kernels?
Otherwise looks good
Reviewed-by: Luis Chamberlain <[email protected]>
> Signed-off-by: Thi?baud Weksteen <[email protected]>
> ---
> v2: Add comment
>
> drivers/base/firmware_loader/main.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index 94d1789a233e..8f3c2b2cfc61 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -735,6 +735,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> size_t offset, u32 opt_flags)
> {
> struct firmware *fw = NULL;
> + struct cred *kern_cred = NULL;
> + const struct cred *old_cred;
> bool nondirect = false;
> int ret;
>
> @@ -751,6 +753,18 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> if (ret <= 0) /* error or already assigned */
> goto out;
>
> + /*
> + * We are about to try to access the firmware file. Because we may have been
> + * called by a driver when serving an unrelated request from userland, we use
> + * the kernel credentials to read the file.
> + */
> + kern_cred = prepare_kernel_cred(NULL);
> + if (!kern_cred) {
> + ret = -ENOMEM;
> + goto out;
> + }
> + old_cred = override_creds(kern_cred);
> +
> ret = fw_get_filesystem_firmware(device, fw->priv, "", NULL);
>
> /* Only full reads can support decompression, platform, and sysfs. */
> @@ -776,6 +790,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> } else
> ret = assign_fw(fw, device);
>
> + revert_creds(old_cred);
> +
> out:
> if (ret < 0) {
> fw_abort_batch_reqs(fw);
> --
> 2.36.0.rc2.479.g8af0fa9b8e-goog
>
> Can you elaborate on the last sentence? It's unclear how what you
> describe is used exactly to allow driver to use direct filesystem
> firmware loading.
I realize my use of the word "device" here was unfortunate. I meant devices as
Android devices/systems. This may have contributed to the confusion.
Previously, Android systems were not setting up the firmware_class.path
command line argument. It means that the userspace fallback was always
kicking-in when a driver called request_firmware. This was handled by the
ueventd process on Android, which is generally given access to all firmware
files.
Now that more devices are setting up firmware_class.path, the call to
request_firmware will end up using kernel_read_file_from_path_initns, which
would have used the current process credentials.
>
> And, given the feedback from Android it would seem this is a fix
> which likely may be desirable to backport to some stable kernels?
Yes, that's right.
Thanks
On Tue, Apr 26, 2022 at 02:18:59PM +1000, Thi?baud Weksteen wrote:
> > Can you elaborate on the last sentence? It's unclear how what you
> > describe is used exactly to allow driver to use direct filesystem
> > firmware loading.
>
> I realize my use of the word "device" here was unfortunate. I meant devices as
> Android devices/systems. This may have contributed to the confusion.
>
> Previously, Android systems were not setting up the firmware_class.path
> command line argument. It means that the userspace fallback was always
> kicking-in when a driver called request_firmware. This was handled by the
> ueventd process on Android, which is generally given access to all firmware
> files.
>
> Now that more devices are setting up firmware_class.path, the call to
> request_firmware will end up using kernel_read_file_from_path_initns, which
> would have used the current process credentials.
That makes it crystal clear. This would be useful in the commit log.
> > And, given the feedback from Android it would seem this is a fix
> > which likely may be desirable to backport to some stable kernels?
>
> Yes, that's right.
Especially in light of this.
Luis
On Fri, Apr 22, 2022 at 11:32:15AM +1000, Thi?baud Weksteen wrote:
> drivers/base/firmware_loader/main.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index 94d1789a233e..8f3c2b2cfc61 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -735,6 +735,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> size_t offset, u32 opt_flags)
> {
> struct firmware *fw = NULL;
> + struct cred *kern_cred = NULL;
> + const struct cred *old_cred;
> bool nondirect = false;
> int ret;
>
> @@ -751,6 +753,18 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> if (ret <= 0) /* error or already assigned */
> goto out;
>
> + /*
> + * We are about to try to access the firmware file. Because we may have been
> + * called by a driver when serving an unrelated request from userland, we use
> + * the kernel credentials to read the file.
> + */
> + kern_cred = prepare_kernel_cred(NULL);
This triggers quite some leak reports from kmemleak.
unreferenced object 0xffff0801e47690c0 (size 176):
comm "kworker/0:1", pid 14, jiffies 4294904047 (age 2208.624s)
hex dump (first 32 bytes):
01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
kmem_cache_alloc
prepare_kernel_cred
_request_firmware
firmware_request_nowarn
firmware_request_nowarn at drivers/base/firmware_loader/main.c:933
nvkm_firmware_get [nouveau]
nvkm_firmware_get at drivers/gpu/drm/nouveau/nvkm/core/firmware.c:92
nvkm_firmware_load_name [nouveau]
nvkm_acr_lsfw_load_bl_inst_data_sig [nouveau]
gm200_gr_load [nouveau]
gf100_gr_new_ [nouveau]
tu102_gr_new [nouveau]
nvkm_device_ctor [nouveau]
nvkm_device_pci_new [nouveau]
nouveau_drm_probe [nouveau]
local_pci_probe
work_for_cpu_fn
process_one_work
> + if (!kern_cred) {
> + ret = -ENOMEM;
> + goto out;
> + }
> + old_cred = override_creds(kern_cred);
> +
> ret = fw_get_filesystem_firmware(device, fw->priv, "", NULL);
>
> /* Only full reads can support decompression, platform, and sysfs. */
> @@ -776,6 +790,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> } else
> ret = assign_fw(fw, device);
>
> + revert_creds(old_cred);
> +
> out:
> if (ret < 0) {
> fw_abort_batch_reqs(fw);
> --
> 2.36.0.rc2.479.g8af0fa9b8e-goog
>
On Wed, Apr 27, 2022 at 09:58:23AM -0400, Qian Cai wrote:
> On Fri, Apr 22, 2022 at 11:32:15AM +1000, Thi?baud Weksteen wrote:
> > drivers/base/firmware_loader/main.c | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> > index 94d1789a233e..8f3c2b2cfc61 100644
> > --- a/drivers/base/firmware_loader/main.c
> > +++ b/drivers/base/firmware_loader/main.c
> > @@ -735,6 +735,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> > size_t offset, u32 opt_flags)
> > {
> > struct firmware *fw = NULL;
> > + struct cred *kern_cred = NULL;
> > + const struct cred *old_cred;
> > bool nondirect = false;
> > int ret;
> >
> > @@ -751,6 +753,18 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> > if (ret <= 0) /* error or already assigned */
> > goto out;
> >
> > + /*
> > + * We are about to try to access the firmware file. Because we may have been
> > + * called by a driver when serving an unrelated request from userland, we use
> > + * the kernel credentials to read the file.
> > + */
> > + kern_cred = prepare_kernel_cred(NULL);
>
> This triggers quite some leak reports from kmemleak.
>
> unreferenced object 0xffff0801e47690c0 (size 176):
> comm "kworker/0:1", pid 14, jiffies 4294904047 (age 2208.624s)
> hex dump (first 32 bytes):
> 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> kmem_cache_alloc
> prepare_kernel_cred
> _request_firmware
> firmware_request_nowarn
> firmware_request_nowarn at drivers/base/firmware_loader/main.c:933
> nvkm_firmware_get [nouveau]
> nvkm_firmware_get at drivers/gpu/drm/nouveau/nvkm/core/firmware.c:92
> nvkm_firmware_load_name [nouveau]
> nvkm_acr_lsfw_load_bl_inst_data_sig [nouveau]
> gm200_gr_load [nouveau]
> gf100_gr_new_ [nouveau]
> tu102_gr_new [nouveau]
> nvkm_device_ctor [nouveau]
> nvkm_device_pci_new [nouveau]
> nouveau_drm_probe [nouveau]
> local_pci_probe
> work_for_cpu_fn
> process_one_work
Ugh, yeah, a put_cred() is not called after this.
I'll go revert this commit for now as it needs more work.
thanks,
greg k-h
> Ugh, yeah, a put_cred() is not called after this.
Good catch, I wasn't aware that an extra call to put_cred was required here.
I'll send a new version for the patch. I'll update the commit log as
well, with the recommendation from Luis. Thanks.