[libcamera-devel] [PATCH 2/2] media-libs/libcamera: Do not strip IPA binaries

[Tomasz Figa]

On Tue, Dec 8, 2020 at 11:32 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Tomasz,
>
> On Tue, Dec 08, 2020 at 11:28:20AM +0900, Tomasz Figa wrote:
> > On Sat, Dec 5, 2020 at 8:43 AM Laurent Pinchart wrote:
> > > On Fri, Nov 27, 2020 at 01:39:40AM +0900, Tomasz Figa wrote:
> > > > On Fri, Nov 27, 2020 at 1:33 AM Laurent Pinchart wrote:
> > > > > On Tue, Nov 10, 2020 at 07:08:53PM +0900, Tomasz Figa wrote:
> > > > > > On Mon, Nov 9, 2020 at 10:17 AM Niklas Söderlund wrote:
> > > > > > >
> > > > > > > Libcamera signs its IPA modules (.so files) after they are built. The
> > > > > > > signature is later verified when loading the IPA modules and if they do
> > > > > > > not match the IPA is treated as a untrusted module. The CrOS build
> > > > > > > system by default strips all binaries after the build step and modify
> > > > > > > the IPA .so files in so they fail the signature check.
> > > > > > >
> > > > > > > The build system inject hooks after the post_src_install hook that
> > > > > > > strips binaries and creates the packet that is installed on target. It
> > > > > > > is therefor not possible to to generate the IPA module signature for the
> > > > > > > stripped modules without also packeting the private key and doing so in
> > > > > > > pre_pkg_preinst. Stripping and generating signatures for the IPA .so
> > > > > > > files in src_install is not possible as the exact method for stripping
> > > > > > > them may differ between the ebuild and the build system hook.
> > > > > > >
> > > > > > > Safest route is to never stripp the IPA modules. Instead of restricting
> > > > > > > stripping of all libcamera binaries use dostrip to only disable
> > > > > > > stripping of the IPA modules. The EAPI needs to be increased to version
> > > > > > > 7 to support dostrip.
> > > > > >
> > > > > > Could we just disable the extra signing and signature verification on
> > > > > > Chrome OS? We have integrity enforced for the whole file system by
> > > > > > dm-verity, so there is no need to verify anything in particular
> > > > > > components of the stack anymore.
> > > > >
> > > > > The signature mechanism is how we decide if an IPA module has to be
> > > > > isolated. Once Paul's IPA IPC series gets merged, we could disable it
> > > > > indeed, which would force isolation of all IPA modules, even the
> > > > > open-source ones. Is that desired though ?
> > > >
> > > > Could you elaborate a bit more how we decide whether to isolate or not
> > > > based on this? I'd assume there would be integrators willing to run
> > > > out of tree IPAs (which could be still open source) without isolation.
> > >
> > > At the moment, we isolate all IPA modules that don't provide a valid
> > > signature. In-tree modules are signed during the build process, and are
> > > thus run without isolation (but in a separate thread, to replicate the
> > > asynchronous communication mechanism of the isolated case, in order to
> > > avoid too many differences between the two cases). Out-of-tree modules
> > > are not signed, and are thus isolated.
> > >
> > > We expect this mechanism to be extended with some or all of the
> > > following:
> > >
> > > - A flag in the module information structure to force isolated
> > >   execution. This would be set, for instance, by the wrapper module for
> > >   the IPU3 that loads the Intel binaries, even if the module is in-tree
> > >   (we haven't decided on whether that will be the case though) and thus
> > >   gets signed.
> > >
> > > - A similar mechanism that forces isolation of modules listed in a
> > >   configuration file.
> > >
> > > - A method to save the private key at build time, to sign modules built
> > >   out of tree. This could be used by Linux distributions to update IPA
> > >   modules without having to update libcamera itself. We may also update
> > >   the build process to import a public/private key pair instead of
> > >   generating one.
> > >
> > > It will ultimately be an integrator decision, as integrators will in any
> > > case have the option of carrying local modifications to libcamera that
> > > changes the IPA module loading and isolation mechanism. Changes that
> > > make sense upstream should of course be merged in our tree.
> > >
> > > Note that we also foresee changes in the isolation mechanism, at least
> > > for Chrome OS, but possibly globally, to use an algorithm daemon. I
> > > would however prefer not implementing this right now as we have more
> > > urgent tasks to focus on.
> > >
> >
> > While I understand this mechanism for the general purpose usage, my
> > point is that the extra signing just adds build-time complexity (and
> > time spent on the extra steps), while not serving any purpose on
> > Chrome OS, because we enforce the integrity and authenticity of all
> > the files with a higher level mechanism (dm-verity).
> >
> > Could we perhaps add a build option to just bypass that signing step
> > and always enable the isolation?
>
> Yes, but not yet, as the isolated code path is currently incomplete.
> Paul's work will fix that, and we expect to merge it soon, so maybe we
> can delay this change. On the other hand, this fixes operation with IPA
> modules right now, which is needed for development, so we all have to
> carry this patch in our local trees. Could this be merged as a temporary
> workaround ?
>

I'm perfectly fine with this as a temporary workaround. Let's file a
bug to track the implementation of the bypass and have a TODO comment
added in the workaround, so that we don't forget about it.

> > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > > > > > ---
> > > > > > >  media-libs/libcamera/libcamera-9999.ebuild | 4 +++-
> > > > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/media-libs/libcamera/libcamera-9999.ebuild b/media-libs/libcamera/libcamera-9999.ebuild
> > > > > > > index 57ff00337309f30c..ce4183a89ef095de 100644
> > > > > > > --- a/media-libs/libcamera/libcamera-9999.ebuild
> > > > > > > +++ b/media-libs/libcamera/libcamera-9999.ebuild
> > > > > > > @@ -1,7 +1,7 @@
> > > > > > >  # Copyright 2019 The Chromium OS Authors. All rights reserved.
> > > > > > >  # Distributed under the terms of the GNU General Public License v2
> > > > > > >
> > > > > > > -EAPI=6
> > > > > > > +EAPI=7
> > > > > > >
> > > > > > >  CROS_WORKON_PROJECT="chromiumos/third_party/libcamera"
> > > > > > >  CROS_WORKON_INCREMENTAL_BUILD="1"
> > > > > > > @@ -49,4 +49,6 @@ src_install() {
> > > > > > >         meson_src_install
> > > > > > >
> > > > > > >         dosym ../libcamera.so "/usr/$(get_libdir)/camera_hal/libcamera.so"
> > > > > > > +
> > > > > > > +       dostrip -x /usr/$(get_libdir)/libcamera/
> > > > > > >  }
>
> --
> Regards,
>
> Laurent Pinchart