| Message ID | 20220422013215.2301793-1-tweek@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v2] firmware_loader: use kernel credentials when reading firmware | expand |
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 <mcgrof@kernel.org> > Signed-off-by: Thiébaud Weksteen <tweek@google.com> > --- > 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.
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);
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 <tweek@google.com> --- v2: Add comment drivers/base/firmware_loader/main.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)