Re: [PULL 02/17] crypto: Introduce SM4 symmetric cipher algorithm

[Peter Maydell]

On Fri, 9 Feb 2024 at 14:07, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> From: Hyman Huang <yong.huang@smartx.com>
>
> 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.
>
> Detect the SM4 cipher algorithms and enable the feature silently
> if it is available.
>
> Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Hi; Coverity points out an issue in this code (CID 1546884):


> @@ -701,6 +731,25 @@ static QCryptoCipher 
> *qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
>
>              return &ctx->base;
>          }
> +#ifdef CONFIG_CRYPTO_SM4
> +    case QCRYPTO_CIPHER_ALG_SM4:
> +        {
> +            QCryptoNettleSm4 *ctx = g_new0(QCryptoNettleSm4, 1);

Here we allocate memory...

> +
> +            switch (mode) {
> +            case QCRYPTO_CIPHER_MODE_ECB:
> +                ctx->base.driver = &qcrypto_nettle_sm4_driver_ecb;
> +                break;
> +            default:
> +                goto bad_cipher_mode;

...but here we may jump to bad_cipher_mode, resulting in
the memory being leaked, because we never free it.

> +            }
> +

This could be fixed by not doing the g_new0() until this
point when we know we aren't going to fail. This is what we
do for the other cipher cases in this file that have a switch
where the default for mode is a jump to bad_cipher_mode.

(PS: is it intentional that for some of these ciphers
an unknown mode value is an error returned to the caller
via bad_cipher_mode, and for others it is a g_assert_not_reached()?)

> +            sm4_set_encrypt_key(&ctx->key[0], key);
> +            sm4_set_decrypt_key(&ctx->key[1], key);
> +
> +            return &ctx->base;
> +        }
> +#endif

thanks
-- PMM