[PATCH] firmware_loader: use kernel credentials when reading firmware

[PATCH] firmware_loader: use kernel credentials when reading firmware

[Thiébaud Weksteen]

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>
---
 drivers/base/firmware_loader/main.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 94d1789a233e..416ee3cc6584 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,13 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	if (ret <= 0) /* error or already assigned */
 		goto out;
 
+	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 +785,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.35.1.1094.g7c7d902a7c-goog

Re: [PATCH] firmware_loader: use kernel credentials when reading firmware

[Greg Kroah-Hartman]

On Mon, Apr 04, 2022 at 03:46:42PM +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.
> 
> Signed-off-by: Thiébaud Weksteen <tweek@google.com>
> ---
>  drivers/base/firmware_loader/main.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Is this a bugfix?  if so, what commit does this fix?  If not, how has
this never been a problem in the past (i.e. what changed to cause
problems?)

thanks,

greg k-h

Re: [PATCH] firmware_loader: use kernel credentials when reading firmware

[Thiébaud Weksteen]

On Mon, Apr 4, 2022 at 6:33 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Apr 04, 2022 at 03:46:42PM +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.
> >
> > Signed-off-by: Thiébaud Weksteen <tweek@google.com>
> > ---
> >  drivers/base/firmware_loader/main.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
>
> Is this a bugfix?  if so, what commit does this fix?  If not, how has
> this never been a problem in the past (i.e. what changed to cause
> problems?)

Not a bug fix. I think this is the combination of 3 factors that
surfaces the issue:

1. As mentioned in the last paragraph of the commit, previously, the
userspace fallback was used (that is, udev/ueventd would read the
firmware file so only this process would need the correct permission
to access the files). More devices are using firmware_class.path=
which causes the kernel to try to load the firmware directly[1].

2. Drivers are calling request_firmware when handling a userspace
request (such as open() on a device file). Historically,
request_firmware seems to have been called when probe() was invoked by
the kernel.

3. The precise MAC policy on Android (as opposed to DAC where the
firmware files are own by root and a root process accesses the device
file).

Thanks,

[1] https://www.kernel.org/doc/html/v5.17/driver-api/firmware/fw_search_path.html

> thanks,
>
> greg k-h

Re: [PATCH] firmware_loader: use kernel credentials when reading firmware

[Greg Kroah-Hartman]

On Mon, Apr 04, 2022 at 03:46:42PM +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.
> 
> Signed-off-by: Thiébaud Weksteen <tweek@google.com>
> ---
>  drivers/base/firmware_loader/main.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index 94d1789a233e..416ee3cc6584 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,13 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>  	if (ret <= 0) /* error or already assigned */
>  		goto out;
>  
> +	kern_cred = prepare_kernel_cred(NULL);
> +	if (!kern_cred) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	old_cred = override_creds(kern_cred);

Can you add a comment here before the call to prepare_kernel_cred() to
say why you are doing this and what it is for?  Otherwise it is not
obvious at all.

thanks,

greg k-h

Re: [PATCH] firmware_loader: use kernel credentials when reading firmware

[John Stultz]

On Mon, Apr 04, 2022 at 03:46:42PM +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.
> 
> Signed-off-by: Thiébaud Weksteen <tweek@google.com>

I recently hit a problem in AOSP: Normally on the device gralloc is the
first processes to open the dri device, which then trips firmware loading.
This worked ok because the firmware is tagged as vendor_firmware, and
gralloc runs in vendor context and the sepolicy allows that.

I had made a change to gralloc, so it no longer opened the dri device,
making SurfaceFlinger the first processes to open the dri device.
Unfortunately, this caused firmware loading to fail due to the sepolicy
blocking SurfaceFlinger from accessing vendor_firmware tagged files.

My initial workaround was to add two silly lines to open and close the
dri device in the gralloc initialization, just to ensure the first
access was in the right context and the firmware would load properly.

But Greg pointed me to this patch, which exactly described the problem and
resolved it (much more cleanly then my workaround). I can't comment on the
change itself, but wanted to say thanks!

Tested-by: John Stultz <jstultz@google.com>

-john

Re: [PATCH] firmware_loader: use kernel credentials when reading firmware

[Greg Kroah-Hartman]

On Mon, May 23, 2022 at 03:24:44PM +1000, Thiébaud Weksteen wrote:
> commit 581dd69830341d299b0c097fc366097ab497d679 upstream.
> 
> 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).
> 
> Previously, Android configurations were not setting up the
> firmware_class.path command line argument and were relying on the
> userspace fallback mechanism. In this case, the security context of the
> userspace daemon (i.e. ueventd) was consistently used to read firmware
> files. More Android devices are now found to set firmware_class.path
> which gives the kernel the opportunity to read the firmware directly
> (via kernel_read_file_from_path_initns). In this scenario, the current
> process credentials were used, even if unrelated to the loading of the
> firmware file.
> 
> Signed-off-by: Thiébaud Weksteen <tweek@google.com>
> Cc: <stable@vger.kernel.org> # 5.4
> Reviewed-by: Paul Moore <paul@paul-moore.com>
> Acked-by: Luis Chamberlain <mcgrof@kernel.org>
> Link: https://lore.kernel.org/r/20220502004952.3970800-1-tweek@google.com
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/base/firmware_loader/main.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)

Now queued up, thanks.

greg k-h

[PATCH] firmware_loader: use kernel credentials when reading firmware

[Thiébaud Weksteen]

commit 581dd69830341d299b0c097fc366097ab497d679 upstream.

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).

Previously, Android configurations were not setting up the
firmware_class.path command line argument and were relying on the
userspace fallback mechanism. In this case, the security context of the
userspace daemon (i.e. ueventd) was consistently used to read firmware
files. More Android devices are now found to set firmware_class.path
which gives the kernel the opportunity to read the firmware directly
(via kernel_read_file_from_path_initns). In this scenario, the current
process credentials were used, even if unrelated to the loading of the
firmware file.

Signed-off-by: Thiébaud Weksteen <tweek@google.com>
Cc: <stable@vger.kernel.org> # 5.4
Reviewed-by: Paul Moore <paul@paul-moore.com>
Acked-by: Luis Chamberlain <mcgrof@kernel.org>
Link: https://lore.kernel.org/r/20220502004952.3970800-1-tweek@google.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/firmware_loader/main.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 4f6b76bd957e..12ab50d29548 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -761,6 +761,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		  enum fw_opt opt_flags)
 {
 	struct firmware *fw = NULL;
+	struct cred *kern_cred = NULL;
+	const struct cred *old_cred;
 	int ret;
 
 	if (!firmware_p)
@@ -776,6 +778,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);
 #ifdef CONFIG_FW_LOADER_COMPRESS
 	if (ret == -ENOENT)
@@ -792,6 +806,9 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	} else
 		ret = assign_fw(fw, device, opt_flags);
 
+	revert_creds(old_cred);
+	put_cred(kern_cred);
+
  out:
 	if (ret < 0) {
 		fw_abort_batch_reqs(fw);
-- 
2.36.1.124.g0e6072fb45-goog