Re: [PATCH v2] crypto: shash - avoid comparing pointers to exported functions under CFI

[Eric Biggers <ebiggers@kernel.org>]

On Sat, Jun 05, 2021 at 08:59:02AM +0200, Ard Biesheuvel wrote:
> crypto_shash_alg_has_setkey() is implemented by testing whether the
> .setkey() member of a struct shash_alg points to the default version,
> called shash_no_setkey(). As crypto_shash_alg_has_setkey() is a static
> inline, this requires shash_no_setkey() to be exported to modules.
> 
> Unfortunately, when building with CFI, function pointers are routed
> via CFI stubs which are private to each module (or to the kernel proper)
> and so this function pointer comparison may fail spuriously.
> 
> Let's fix this by turning crypto_shash_alg_has_setkey() into an out of
> line function, which makes the problem go away.
> 
> Cc: Sami Tolvanen <samitolvanen@google.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> v2: add code comment to explain why the function needs to remain out of
> line
> 
>  crypto/shash.c                 | 20 +++++++++++++++++---
>  include/crypto/internal/hash.h |  8 +-------
>  2 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/crypto/shash.c b/crypto/shash.c
> index 2e3433ad9762..36579c37e27d 100644
> --- a/crypto/shash.c
> +++ b/crypto/shash.c
> @@ -20,12 +20,26 @@
>  
>  static const struct crypto_type crypto_shash_type;
>  
> -int shash_no_setkey(struct crypto_shash *tfm, const u8 *key,
> -		    unsigned int keylen)
> +static int shash_no_setkey(struct crypto_shash *tfm, const u8 *key,
> +			   unsigned int keylen)
>  {
>  	return -ENOSYS;
>  }
> -EXPORT_SYMBOL_GPL(shash_no_setkey);
> +
> +bool crypto_shash_alg_has_setkey(struct shash_alg *alg)
> +{
> +	/*
> +	 * Function pointer comparisons such as the one below will not work as
> +	 * expected when CFI is enabled, and the comparison involves an
> +	 * exported symbol: as indirect function calls are routed via CFI stubs
> +	 * that are private to each module, the pointer values may be different
> +	 * even if they refer to the same function.
> +	 *
> +	 * Therefore, this function must remain out of line.
> +	 */
> +	return alg->setkey != shash_no_setkey;
> +}
> +EXPORT_SYMBOL_GPL(crypto_shash_alg_has_setkey);

Looks okay, but the comment isn't great since it doesn't get to the point until
the last sentence.  Also it talks about an exported symbol, which is confusing
because shash_no_setkey won't actually be exported anymore.  I'd prefer
something like the following:

/*
 * Check whether an shash algorithm has a setkey function.
 *
 * For CFI compatibility, this must not be an inline function.  This is because
 * when CFI is enabled, modules won't get the same address for shash_no_setkey
 * (if it were exported, which inlining would require) as the core kernel will.
 */
bool crypto_shash_alg_has_setkey(struct shash_alg *alg)
{
        return alg->setkey != shash_no_setkey;
}
EXPORT_SYMBOL_GPL(crypto_shash_alg_has_setkey);