Re: [LTP] [PATCH v2 1/1] tst_af_alg: Another fix for disabled weak cipher

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> e.g. md5 on enabled FIPS.
> Similar fix to 4fa302ef9d. It fixes:
> 
> ./af_alg01
> tst_af_alg.c:84: TBROK: unexpected error binding AF_ALG socket to hash algorithm 'md5': ELIBBAD (80)
> become
> tst_fips.c:22: TINFO: FIPS: on
> tst_af_alg.c:82: TCONF: FIPS enabled => hash algorithm 'md5' disabled
> tst_fips.c:22: TINFO: FIPS: on
> tst_af_alg.c:82: TCONF: FIPS enabled => hash algorithm 'md5-generic' disabled
> 
> ./af_alg02
> tst_af_alg.c:37: TBROK: unexpected error binding AF_ALG socket to skcipher algorithm 'salsa20': ELIBBAD (80)
> become
> tst_fips.c:22: TINFO: FIPS: on
> tst_af_alg.c:36: TCONF: FIPS enabled => skcipher algorithm 'salsa20' disabled
> 
> ./af_alg04
> tst_af_alg.c:81: TBROK: unexpected error binding AF_ALG socket to hash algorithm 'vmac64(aes)': ELIBBAD (80)
> become
> tst_fips.c:22: TINFO: FIPS: on
> tst_af_alg.c:82: TCONF: FIPS enabled => hash algorithm 'vmac64(aes)' disabled
> af_alg04.c:32: TCONF: kernel doesn't have hash algorithm 'vmac(aes)'
> af_alg04.c:32: TCONF: kernel doesn't have hash algorithm 'vmac(sm4)'
> af_alg04.c:32: TCONF: kernel doesn't have hash algorithm 'vmac(sm4-generic)'
> 
> af_alg01.c adjusted not to print TCONF twice.
> 
> Tested on Debian stable bullseye and SLES 15-SP4.
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> Hi,
> 
> I was wrong, although SUSE has some custom patches for FIPS to disable
> ciphers in drivers/crypto, patch is for mainline, because it returns
> ELIBBAD for algorithms it considers non-FIPS-approved.
> 
> Also, while it's not that easy to run fips=1 on current openSUSE
> Tumbleweed or Fedora 34 (there are probably some restricted ciphers
> boot (systemd?) depends on), at least Debian stable boots and restrict
> ciphers as expected.
> 
> NOTE: do we want to optimize repeated fips detection or repeated output?
> (didn't see any elegant solution).
> 
> Kind regards,
> Petr
> 
>  include/tst_af_alg.h               |  3 ++-
>  lib/tst_af_alg.c                   | 16 +++++++++++++++-
>  testcases/kernel/crypto/af_alg01.c |  6 ++++--
>  3 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/include/tst_af_alg.h b/include/tst_af_alg.h
> index fd2ff06478..264e226a2c 100644
> --- a/include/tst_af_alg.h
> +++ b/include/tst_af_alg.h
> @@ -61,7 +61,8 @@ void tst_alg_bind(int algfd, const char *algtype, const char *algname);
>   * @param algname The name of the algorithm, such as "sha256" or "xts(aes)"
>   *
>   * Return true if the algorithm is available, or false if unavailable.
> - * If another error occurs, tst_brk() is called with TBROK.
> + * If another error occurs, tst_brk() is called with TBROK,
> + * unless algorithm enabled due FIPS mode (errno ELIBBAD).
                      ^
		      is disabled
>   */
>  bool tst_have_alg(const char *algtype, const char *algname);
>  
> diff --git a/lib/tst_af_alg.c b/lib/tst_af_alg.c
> index 05caa63016..9325a98432 100644
> --- a/lib/tst_af_alg.c
> +++ b/lib/tst_af_alg.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * Copyright 2019 Google LLC
> + * Copyright (c) Linux Test Project, 2019-2021
>   */
>  
>  #include <errno.h>
> @@ -30,10 +31,18 @@ void tst_alg_bind_addr(int algfd, const struct sockaddr_alg *addr)
>  
>  	if (ret == 0)
>  		return;
> +
> +	if (errno == ELIBBAD && tst_fips_enabled()) {
> +		tst_brk(TCONF,
> +			"FIPS enabled => %s algorithm '%s' disabled",
> +			addr->salg_type, addr->salg_name);
> +	}
> +
>  	if (errno == ENOENT) {
>  		tst_brk(TCONF, "kernel doesn't support %s algorithm '%s'",
>  			addr->salg_type, addr->salg_name);
>  	}
> +
>  	tst_brk(TBROK | TERRNO,
>  		"unexpected error binding AF_ALG socket to %s algorithm '%s'",
>  		addr->salg_type, addr->salg_name);
> @@ -77,11 +86,16 @@ bool tst_have_alg(const char *algtype, const char *algname)
>  
>  	ret = bind(algfd, (const struct sockaddr *)&addr, sizeof(addr));
>  	if (ret != 0) {
> -		if (errno != ENOENT) {
> +		if (errno == ELIBBAD && tst_fips_enabled()) {
> +			tst_res(TCONF,
> +				"FIPS enabled => %s algorithm '%s' disabled",
> +				algtype, algname);
> +		} else if (errno != ENOENT) {
>  			tst_brk(TBROK | TERRNO,
>  				"unexpected error binding AF_ALG socket to %s algorithm '%s'",
>  				algtype, algname);
>  		}
> +
>  		have_alg = false;
>  	}
>  
> diff --git a/testcases/kernel/crypto/af_alg01.c b/testcases/kernel/crypto/af_alg01.c
> index 47292ee328..e31126fe01 100644
> --- a/testcases/kernel/crypto/af_alg01.c
> +++ b/testcases/kernel/crypto/af_alg01.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * Copyright 2019 Google LLC
> + * Copyright (c) Linux Test Project, 2019-2021
>   */
>  
>  /*
> @@ -22,8 +23,9 @@ static void test_with_hash_alg(const char *hash_algname)
>  	char key[4096] = { 0 };
>  
>  	if (!tst_have_alg("hash", hash_algname)) {
> -		tst_res(TCONF, "kernel doesn't have hash algorithm '%s'",
> -			hash_algname);
> +		if (errno != ELIBBAD)
> +			tst_res(TCONF, "kernel doesn't have hash algorithm '%s'",
> +				hash_algname);

What about moving the tst_res(TCONF, ...) in the case of ENOENT to the
tst_have_alg() function?

That way we would have here just

	if (!tst_have_alg("hash", hash_algname))
		return;

Other than these two minor things this version does look good:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

>  		return;
>  	}
>  	sprintf(hmac_algname, "hmac(%s)", hash_algname);
> -- 
> 2.34.1
> 

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp