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

[Laurent Pinchart]

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 ?

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