[libcamera-devel] libcamera: Regenerate IPA module signatures at install time
diff mbox series

Message ID 20200429013349.2389-1-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel] libcamera: Regenerate IPA module signatures at install time
Related show

Commit Message

Laurent Pinchart April 29, 2020, 1:33 a.m. UTC 
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.

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.

Signed-off-by: Laurent Pinchart <laurent.pinchart@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

Comments

Kieran Bingham April 29, 2020, 8:43 a.m. UTC | #1 
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@ideasonboard.com>

Small points below:...


> Signed-off-by: Laurent Pinchart <laurent.pinchart@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@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.
Laurent Pinchart April 29, 2020, 12:17 p.m. UTC | #2 
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@ideasonboard.com>
> 
> Small points below:...
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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@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.

Patch
diff mbox series 

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