Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Thu, 21 Apr 2022 19:26:18 +0000
From:   John Stultz <jstultz@google.com>
To:     =?iso-8859-1?Q?Thi=E9baud?= Weksteen <tweek@google.com>
Cc:     Luis Chamberlain <mcgrof@kernel.org>,
        Jeffrey Vander Stoep <jeffv@google.com>,
        Saravana Kannan <saravanak@google.com>,
        Alistair Delva <adelva@google.com>,
        Adam Shih <adamshih@google.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        selinux@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] firmware_loader: use kernel credentials when reading
 firmware
Message-ID: <YmGv2lY65NniqWzk@google.com>
Reply-To: 20220404054642.3095732-1-tweek@google.com
References: <20220404054642.3095732-1-tweek@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220404054642.3095732-1-tweek@google.com>
Precedence: bulk

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