[libcamera-devel] [PATCH 1/2] libcamera: Make IPA module signing optional
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Apr 16 14:16:01 CEST 2020
Hi Laurent,
On 15/04/2020 20:37, Laurent Pinchart wrote:
> The IPA module signing mechanism relies on openssl to generate keys and
> sign the module. If openssl is not found on the system, the build will
> fail. Make the dependency optional by detecting openssl, and skip
> generation of signatures if openssl isn't found.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
If I had more time I might play around with different options here - but
given this works ... lets get it in and move on.
Some small notes and possible options below but:
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> src/ipa/meson.build | 4 +++-
> src/ipa/rkisp1/meson.build | 14 ++++++++------
> src/ipa/vimc/meson.build | 14 ++++++++------
> src/libcamera/include/ipa_manager.h | 2 ++
> src/libcamera/ipa_manager.cpp | 4 ++++
> src/libcamera/ipa_pub_key.cpp.in | 4 +++-
> src/libcamera/meson.build | 16 +++++++++-------
> src/meson.build | 15 +++++++++++----
> 8 files changed, 48 insertions(+), 25 deletions(-)
>
> diff --git a/src/ipa/meson.build b/src/ipa/meson.build
> index cb4e3ab3388f..96d3ce79ffe9 100644
> --- a/src/ipa/meson.build
> +++ b/src/ipa/meson.build
> @@ -10,7 +10,9 @@ config_h.set('IPA_MODULE_DIR',
>
> subdir('libipa')
>
> -ipa_sign = find_program('ipa-sign.sh')
> +if ipa_sign_module
> + ipa_sign = find_program('ipa-sign.sh')
> +endif
This file will exist regardless, so making this conditional on
ipa_sign_module feels like clutter.
Sure it might not be used though ... Does meson complain if a variable
is defined but not used?
> ipas = ['rkisp1', 'vimc']
>
> diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build
> index 6ccadcfbbe64..247d0429b49c 100644
> --- a/src/ipa/rkisp1/meson.build
> +++ b/src/ipa/rkisp1/meson.build
> @@ -9,9 +9,11 @@ mod = shared_module(ipa_name,
> install : true,
> install_dir : ipa_install_dir)
>
> -custom_target(ipa_name + '.so.sign',
> - input : mod,
> - output : ipa_name + '.so.sign',
> - command : [ ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@' ],
> - install : true,
> - install_dir : ipa_install_dir)
> +if ipa_sign_module
> + custom_target(ipa_name + '.so.sign',
> + input : mod,
> + output : ipa_name + '.so.sign',
> + command : [ ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@' ],
> + install : true,
> + install_dir : ipa_install_dir)
> +endif
> diff --git a/src/ipa/vimc/meson.build b/src/ipa/vimc/meson.build
> index 3c932aa7aaad..a354096d8496 100644
> --- a/src/ipa/vimc/meson.build
> +++ b/src/ipa/vimc/meson.build
> @@ -9,9 +9,11 @@ mod = shared_module(ipa_name,
> install : true,
> install_dir : ipa_install_dir)
>
> -custom_target(ipa_name + '.so.sign',
> - input : mod,
> - output : ipa_name + '.so.sign',
> - command : [ ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@' ],
> - install : true,
> - install_dir : ipa_install_dir)
> +if ipa_sign_module
> + custom_target(ipa_name + '.so.sign',
> + input : mod,
> + output : ipa_name + '.so.sign',
> + command : [ ipa_sign, ipa_priv_key, '@INPUT@', '@OUTPUT@' ],
> + install : true,
> + install_dir : ipa_install_dir)
> +endif
> diff --git a/src/libcamera/include/ipa_manager.h b/src/libcamera/include/ipa_manager.h
> index 0b5fd2ac1f12..6165efb8b145 100644
> --- a/src/libcamera/include/ipa_manager.h
> +++ b/src/libcamera/include/ipa_manager.h
> @@ -40,8 +40,10 @@ private:
>
> bool isSignatureValid(IPAModule *ipa) const;
>
> +#if HAVE_IPA_PUBKEY
I wonder if this could be 'cleaner' using the linker and weak symbols or
such ... but that would take time we don't have to play around with...
so this should be ok.
It's all internal, so it's not going to affect any ABI I don't believe.
> static const uint8_t publicKeyData_[];
> static const PubKey pubKey_;
> +#endif
> };
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index 7de1404eebdd..50b6792d6cce 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -304,6 +304,7 @@ std::unique_ptr<IPAInterface> IPAManager::createIPA(PipelineHandler *pipe,
>
> bool IPAManager::isSignatureValid(IPAModule *ipa) const
> {
> +#if HAVE_IPA_PUBKEY
> File file{ ipa->path() };
> if (!file.open(File::ReadOnly))
> return false;
> @@ -319,6 +320,9 @@ bool IPAManager::isSignatureValid(IPAModule *ipa) const
> << (valid ? "valid" : "not valid");
>
> return valid;
> +#else
> + return false;
> +#endif
> }
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/ipa_pub_key.cpp.in b/src/libcamera/ipa_pub_key.cpp.in
> index e1fe287c160e..7ffc1e24d67b 100644
> --- a/src/libcamera/ipa_pub_key.cpp.in
> +++ b/src/libcamera/ipa_pub_key.cpp.in
> @@ -2,7 +2,7 @@
> /*
> * Copyright (C) 2020, Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> *
> - * ipa_key.cpp - IPA module signing public key
> + * ipa_pub_key.cpp - IPA module signing public key
Unrelated change, but keep it.
> *
> * This file is auto-generated. Do not edit.
> */
> @@ -11,10 +11,12 @@
>
> namespace libcamera {
>
> +#if HAVE_IPA_PUBKEY
> const uint8_t IPAManager::publicKeyData_[] = {
> ${ipa_key}
> };
>
> const PubKey IPAManager::pubKey_{ { IPAManager::publicKeyData_ } };
> +#endif
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index c502450c4b2d..dcd2fb4900e6 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -101,13 +101,15 @@ version_cpp = vcs_tag(command : [gen_version, meson.build_root()],
>
> libcamera_sources += version_cpp
>
> -gen_ipa_pub_key = files('gen-ipa-pub-key.py')
> -ipa_pub_key_cpp = custom_target('ipa_pub_key_cpp',
> - input : [ ipa_priv_key, 'ipa_pub_key.cpp.in' ],
> - output : 'ipa_pub_key.cpp',
> - command : [ gen_ipa_pub_key, '@INPUT@', '@OUTPUT@' ])
> -
> -libcamera_sources += ipa_pub_key_cpp
> +if ipa_sign_module
> + gen_ipa_pub_key = files('gen-ipa-pub-key.py')
> + ipa_pub_key_cpp = custom_target('ipa_pub_key_cpp',
> + input : [ ipa_priv_key, 'ipa_pub_key.cpp.in' ],
> + output : 'ipa_pub_key.cpp',
> + command : [ gen_ipa_pub_key, '@INPUT@', '@OUTPUT@' ])
> +
> + libcamera_sources += ipa_pub_key_cpp
> +endif
>
> libcamera_deps = [
> libatomic,
> diff --git a/src/meson.build b/src/meson.build
> index dc0e0c82b900..296682758613 100644
> --- a/src/meson.build
> +++ b/src/meson.build
> @@ -2,10 +2,17 @@ if get_option('android')
> subdir('android')
> endif
>
> -ipa_gen_priv_key = find_program('ipa/gen-ipa-priv-key.sh')
> -ipa_priv_key = custom_target('ipa-priv-key',
> - output : [ 'ipa-priv-key.pem' ],
> - command : [ ipa_gen_priv_key, '@OUTPUT@' ])
> +openssl = find_program('openssl', required : false)
> +if openssl.found()
I'd be tempted to make this also dependant upon whether the gnutls was
found or not, as if we can't validate the key - there's not much point
in signing it ...
> + ipa_gen_priv_key = find_program('ipa/gen-ipa-priv-key.sh')
> + ipa_priv_key = custom_target('ipa-priv-key',
> + output : [ 'ipa-priv-key.pem' ],
> + command : [ ipa_gen_priv_key, '@OUTPUT@' ])
> + config_h.set('HAVE_IPA_PUBKEY', 1)
> + ipa_sign_module = true
> +else
> + ipa_sign_module = false
> +endif
>
> subdir('libcamera')
> subdir('ipa')
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list