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@xxxxxxxxxx>
> Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> ---
> 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);
