Re: [PATCH v2] crypto: Introduce SM4 symmetric cipher algorithm

[Daniel P . Berrangé]

On Wed, Nov 29, 2023 at 09:40:29AM +0800, Yong Huang wrote:
> I'll try to understand the comment, if i misunderstood, please point out.
> 
> On Wed, Nov 29, 2023 at 12:20 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Tue, Nov 28, 2023 at 04:57:20PM +0100, Philippe Mathieu-Daudé wrote:
> > > Hi Hyman,
> > >
> > > On 28/11/23 16:24, Hyman Huang wrote:
> > > > Introduce the SM4 cipher algorithms (OSCCA GB/T 32907-2016).
> > > >
> > > > SM4 (GBT.32907-2016) is a cryptographic standard issued by the
> > > > Organization of State Commercial Administration of China (OSCCA)
> > > > as an authorized cryptographic algorithms for the use within China.
> > > >
> > > > Use the crypto-sm4 meson build option for enabling this feature.
> > > >
> > > > Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> > > > ---
> > > >   crypto/block-luks.c             | 11 ++++++++
> > > >   crypto/cipher-gcrypt.c.inc      |  8 ++++++
> > > >   crypto/cipher-nettle.c.inc      | 49
> > +++++++++++++++++++++++++++++++++
> > > >   crypto/cipher.c                 |  6 ++++
> > > >   meson.build                     | 23 ++++++++++++++++
> > > >   meson_options.txt               |  2 ++
> > > >   qapi/crypto.json                |  5 +++-
> > > >   scripts/meson-buildoptions.sh   |  3 ++
> > > >   tests/unit/test-crypto-cipher.c | 13 +++++++++
> > > >   9 files changed, 119 insertions(+), 1 deletion(-)
> > >
> > >
> > > > diff --git a/meson.build b/meson.build
> > > > index ec01f8b138..256d3257bb 100644
> > > > --- a/meson.build
> > > > +++ b/meson.build
> > > > @@ -1480,6 +1480,7 @@ endif
> > > >   gcrypt = not_found
> > > >   nettle = not_found
> > > >   hogweed = not_found
> > > > +crypto_sm4 = not_found
> > > >   xts = 'none'
> > > >   if get_option('nettle').enabled() and get_option('gcrypt').enabled()
> > > > @@ -1514,6 +1515,26 @@ if not gnutls_crypto.found()
> > > >         xts = 'private'
> > > >       endif
> > > >     endif
> > > > +  if get_option('crypto_sm4').enabled()
> > >
> > > We want to detect it by default (not only when explicitly enabled) ...
> > >
> > > > +    if get_option('gcrypt').enabled()
> > > > +      # SM4 ALG is available in libgcrypt >= 1.9
> > > > +      crypto_sm4 = dependency('libgcrypt', version: '>=1.9',
> > > > +                              method: 'config-tool',
> > > > +                              required: get_option('gcrypt'))
> > > > +      # SM4 ALG static compilation
> > > > +      if crypto_sm4.found() and get_option('prefer_static')
> > > > +        crypto_sm4 = declare_dependency(dependencies: [
> > > > +          crypto_sm4,
> > > > +          cc.find_library('gpg-error', required: true)],
> > > > +          version: crypto_sm4.version())
> > > > +      endif
> > > > +    else
> > > > +      # SM4 ALG is available in nettle >= 3.9
> > > > +      crypto_sm4 = dependency('nettle', version: '>=3.9',
> > > > +                              method: 'pkg-config',
> > > > +                              required: get_option('nettle'))
> > > > +    endif
> > >
> > > ... and if it was forced with --enable-crypto_sm4 AND not found,
> > > display an error.
> > >
> > > IIUC your config you try to find the best effort implementation then
> > > if not found, keep going silently.
> >
> > Yes, ignore the get_option() calls, and instead look at .found()
> > in the library we just detected
> >
> ie
> >
> >   if nettle.found()
> >       ....check sm4 in nettle
> >   endif
> >
> >   if gcrypt.found()
> >       ....check sm4 in crypt
> >   endif
> >
> > To detect if sm4 is supported, there may be two methods:
> One is to specify the version explicitly(ligcrypt >=1.9,nettle >= 3.9)
> as in patch
> 
> Another is to use the cc.link for a test. eg:
> 
> +      crypto_sm4 = gcrypt
> +      if gcrypt.found() and not cc.links('''
> +        #include <gcrypt.h>
> +        void main(void) {
> +          gcry_cipher_hd_t handler;
> +          gcry_cipher_open(&handler, GCRY_CIPHER_SM4,
> GCRY_CIPHER_MODE_ECB, 0);
> +        }''', dependencies: gcrypt)
> +        crypto_sm4 = not_found
> +      endif
> 
> Is the latter a better choice?

I'm fine with either a version check, or a link check. A
link check is robust against anyone backporting new features,
so a little better.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|