[libcamera-devel] [PATCH] libcamera: Regenerate IPA module signatures at install time

[Laurent Pinchart]

Hi Kieran,

On Wed, Apr 29, 2020 at 09:43:19AM +0100, Kieran Bingham wrote:
> On 29/04/2020 02:33, Laurent Pinchart wrote:
> > When the IPA modules are installed, meson strips the DT_RPATH and
> > DT_RUNPATH from the binaries. This invalidates the signatures. Disable
> > installation of the .sign files and add an installation script to
> > regenerate them directly in the target directory. The .sign files still
> > need to be created at build time to support running IPA modules from the
> > build tree.
> 
> Indeed, I think this is the best approach.
> 
> > Two alternative approaches have been considered:
> > 
> > - meson could be taught a new target argument to preserve binary
> >   compatibility by skipping any operation that modifies files. This has
> >   been proposed in the #mesonbuild IRC channel. While this could be
> >   interesting in the longer term, we need to fix the issue now.>
> > - The module signatures could be computed on selected sections only.
> >   While skipping the .dynamic section when signing may not cause
> >   security issues, it would make signature generation and verification
> >   more complex, and wasn't deemed worth it.
> 
> Agreed,
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> Small points below:...
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/ipa/ipa-sign-install.sh | 18 ++++++++++++++++++
> >  src/ipa/meson.build         |  9 +++++++++
> >  src/ipa/rkisp1/meson.build  |  2 +-
> >  src/ipa/vimc/meson.build    |  2 +-
> >  4 files changed, 29 insertions(+), 2 deletions(-)
> >  create mode 100755 src/ipa/ipa-sign-install.sh
> > 
> > diff --git a/src/ipa/ipa-sign-install.sh b/src/ipa/ipa-sign-install.sh
> > new file mode 100755
> > index 000000000000..5317a8a2042b
> > --- /dev/null
> > +++ b/src/ipa/ipa-sign-install.sh
> > @@ -0,0 +1,18 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> > +# Copyright (C) 2020, Google Inc.
> > +#
> > +# Author: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > +#
> > +# ipa-sign-install.sh - Regenerate IPA module signatures when installing
> > +
> > +libdir=$1
> > +key=$2
> > +
> > +ipa_sign=$(dirname "$0")/ipa-sign.sh
> > +
> > +echo "Regenerating IPA modules signatures"
> > +
> > +for module in "${MESON_INSTALL_DESTDIR_PREFIX}/${libdir}"/*.so ; do
> > +	"${ipa_sign}" "${key}" "${module}" "${module}.sign"
> > +done
> > diff --git a/src/ipa/meson.build b/src/ipa/meson.build
> > index 145bf8105af7..56e65eaa7426 100644
> > --- a/src/ipa/meson.build
> > +++ b/src/ipa/meson.build
> > @@ -25,3 +25,12 @@ foreach pipeline : get_option('pipelines')
> >          subdir(pipeline)
> >      endif
> >  endforeach
> > +
> > +if ipa_sign_module
> > +    # Regenerate the signatures for all IPA modules. We can't simply install the
> > +    # .sign files, as meson strips the DT_RPATH and DT_RUNPATH from binaries at
> > +    # install time, which invalidates the signatures.
> > +    meson.add_install_script('ipa-sign-install.sh',
> > +                             ipa_install_dir,
> > +                             ipa_priv_key.full_path())
> > +endif
> > diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build
> > index 247d0429b49c..98dbbd632c12 100644
> > --- a/src/ipa/rkisp1/meson.build
> > +++ b/src/ipa/rkisp1/meson.build
> > @@ -14,6 +14,6 @@ if ipa_sign_module
> >                    input : mod,
> >                    output : ipa_name + '.so.sign',
> >                    command : [ ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@' ],
> > -                  install : true,
> > +                  install : false,
> >                    install_dir : ipa_install_dir)
> 
> Why are we providing an install_dir for something we won't install?

Oops :-) I'll fix that.

> >  endif
> > diff --git a/src/ipa/vimc/meson.build b/src/ipa/vimc/meson.build
> > index f8650ee80461..9d0760e57cc1 100644
> > --- a/src/ipa/vimc/meson.build
> > +++ b/src/ipa/vimc/meson.build
> > @@ -14,7 +14,7 @@ if ipa_sign_module
> >                    input : mod,
> >                    output : ipa_name + '.so.sign',
> >                    command : [ ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@' ],
> > -                  install : true,
> > +                  install : false,
> >                    install_dir : ipa_install_dir)
> >  endif
> 
> Hrm... that's going to be duplicated for every IPA. We can't do
> functions in meson, but we could perhaps handle signing at the top level
> src/ipa/meson.build in a single loop at the end. .. but then it feels
> like fighting the tooling to keep things DRY. ho hum... Not something to
> consider in this patch anyway.

meson has generators, but they don't support taking the object returned
by a custom_target() as input, unlike the custom_target(). I think
that's what we should use, once meson will support it.

-- 
Regards,

Laurent Pinchart