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

[Kieran Bingham]

Hi Laurent,

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?


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

-- 
Regards
--
Kieran