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

[Laurent Pinchart]

Hi Niklas,

On Sat, Dec 05, 2020 at 10:20:19AM +0100, Niklas Söderlund wrote:
> On 2020-12-05 01:43:13 +0200, 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.
> 
> Quick note: Storing the generated key and signing modules at packet 
> install time is one of the possible solutions for CrOS that could be 
> added with little effort. I was reluctant to it as it feels a bit 
> redundant to ship the things that should be signed together with the 
> signing key ;-) It would however allow us to sign the IPA's after the 
> CrOS striping of the binaries at packet creation time as we could resign 
> them when they are installed on target.

Shipping the private key on the target would indeed make this all a bit
useless :-) When I mentioned saving the private key, I meant saving it
in a secure location on the integrator's side, the same way a secureboot
or kernel module signing key would be saved.

If there's no hook we can use post-stripping and pre-packaging, one
option would be to strip the IPA modules manually in the src_install
hook, resign them, and mark them with dostrip -x.

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