| Message ID | 5087131.2PHHu6SUIE@positron.chronox.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | /dev/random - a new approach with full SP800-90B | expand |
Hi "Stephan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on cryptodev/master crypto/master v5.5-rc5 next-20200110]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Stephan-M-ller/dev-random-a-new-approach-with-full-SP800-90B/20200110-084934
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 68faa679b8be1a74e6663c21c3a9d25d32f1c079
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-129-g341daf20-dirty
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/char/lrng/lrng_internal.h:239:39: sparse: sparse: context imbalance in 'lrng_drng_switch' - unexpected unlock
vim +/lrng_drng_switch +239 drivers/char/lrng/lrng_internal.h
58c283819a1e87 Stephan Müller 2020-01-09 233
58c283819a1e87 Stephan Müller 2020-01-09 234 /* Unlock the DRNG */
58c283819a1e87 Stephan Müller 2020-01-09 235 static __always_inline void lrng_drng_unlock(struct lrng_drng *drng,
58c283819a1e87 Stephan Müller 2020-01-09 236 unsigned long *flags)
58c283819a1e87 Stephan Müller 2020-01-09 237 {
58c283819a1e87 Stephan Müller 2020-01-09 238 if (lrng_drng_is_atomic(drng))
58c283819a1e87 Stephan Müller 2020-01-09 @239 spin_unlock_irqrestore(&drng->spin_lock, *flags);
58c283819a1e87 Stephan Müller 2020-01-09 240 else
58c283819a1e87 Stephan Müller 2020-01-09 241 mutex_unlock(&drng->lock);
58c283819a1e87 Stephan Müller 2020-01-09 242 }
58c283819a1e87 Stephan Müller 2020-01-09 243
:::::: The code at line 239 was first introduced by commit
:::::: 58c283819a1e879bc2e30d05720285f9709f7f6d Linux Random Number Generator
:::::: TO: Stephan Müller <smueller@chronox.de>
:::::: CC: 0day robot <lkp@intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
Am Samstag, 11. Januar 2020, 08:09:50 CET schrieb kbuild test robot: Hi, > Hi "Stephan, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on char-misc/char-misc-testing] > [also build test WARNING on cryptodev/master crypto/master v5.5-rc5 > next-20200110] [if your patch is applied to the wrong git tree, please drop > us a note to help improve the system. BTW, we also suggest to use '--base' > option to specify the base tree in git format-patch, please see > https://stackoverflow.com/a/37406982] > > url: > https://github.com/0day-ci/linux/commits/Stephan-M-ller/dev-random-a-new-ap > proach-with-full-SP800-90B/20200110-084934 base: > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git > 68faa679b8be1a74e6663c21c3a9d25d32f1c079 reproduce: > # apt-get install sparse > # sparse version: v0.6.1-129-g341daf20-dirty > make ARCH=x86_64 allmodconfig > make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' > > If you fix the issue, kindly add following tag > Reported-by: kbuild test robot <lkp@intel.com> > > > sparse warnings: (new ones prefixed by >>) > > >> drivers/char/lrng/lrng_internal.h:239:39: sparse: sparse: context > >> imbalance in 'lrng_drng_switch' - unexpected unlock > vim +/lrng_drng_switch +239 drivers/char/lrng/lrng_internal.h > > 58c283819a1e87 Stephan Müller 2020-01-09 233 > 58c283819a1e87 Stephan Müller 2020-01-09 234 /* Unlock the DRNG */ > 58c283819a1e87 Stephan Müller 2020-01-09 235 static __always_inline void > lrng_drng_unlock(struct lrng_drng *drng, 58c283819a1e87 Stephan Müller > 2020-01-09 236 unsigned long *flags) 58c283819a1e87 Stephan > Müller 2020-01-09 237 { > 58c283819a1e87 Stephan Müller 2020-01-09 238 if > (lrng_drng_is_atomic(drng)) 58c283819a1e87 Stephan Müller 2020-01-09 @239 > spin_unlock_irqrestore(&drng->spin_lock, *flags); 58c283819a1e87 Stephan > Müller 2020-01-09 240 else > 58c283819a1e87 Stephan Müller 2020-01-09 241 mutex_unlock(&drng- >lock); > 58c283819a1e87 Stephan Müller 2020-01-09 242 } > 58c283819a1e87 Stephan Müller 2020-01-09 243 > > :::::: The code at line 239 was first introduced by commit > :::::: 58c283819a1e879bc2e30d05720285f9709f7f6d Linux Random Number > :::::: Generator > :::::: > :::::: TO: Stephan Müller <smueller@chronox.de> > :::::: CC: 0day robot <lkp@intel.com> > > --- > 0-DAY kernel test infrastructure Open Source Technology > Center https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel > Corporation After analyzing the issue a bit more, it seems that I have to remove "unlikely" from lrng_drng_lock which seems to cause additional grief with sparse. Note, sparse will still report a lock context imbalance as it used to since we indeed have two lock context as documented in lrng_drng_switch. Ciao Stephan
diff --git a/drivers/char/lrng/Kconfig b/drivers/char/lrng/Kconfig index 56f13efd3592..cb701bb0b8b6 100644 --- a/drivers/char/lrng/Kconfig +++ b/drivers/char/lrng/Kconfig @@ -64,4 +64,11 @@ config LRNG_POOL_SIZE default 7 if LRNG_POOL_SIZE_65536 default 8 if LRNG_POOL_SIZE_131072 +menuconfig LRNG_DRNG_SWITCH + bool "Support DRNG runtime switching" + help + The Linux RNG per default uses a ChaCha20 DRNG that is + accessible via the external interfaces. With this configuration + option other DRNGs can be selected and loaded at runtime. + endif # LRNG diff --git a/drivers/char/lrng/Makefile b/drivers/char/lrng/Makefile index e69c176f0161..31cfe87c999e 100644 --- a/drivers/char/lrng/Makefile +++ b/drivers/char/lrng/Makefile @@ -10,3 +10,4 @@ obj-y += lrng_pool.o lrng_aux.o \ obj-$(CONFIG_NUMA) += lrng_numa.o obj-$(CONFIG_SYSCTL) += lrng_proc.o +obj-$(CONFIG_LRNG_DRNG_SWITCH) += lrng_switch.o diff --git a/drivers/char/lrng/lrng_switch.c b/drivers/char/lrng/lrng_switch.c new file mode 100644 index 000000000000..2c7468d8de09 --- /dev/null +++ b/drivers/char/lrng/lrng_switch.c @@ -0,0 +1,179 @@ +// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause +/* + * LRNG DRNG switching support + * + * Copyright (C) 2016 - 2020, Stephan Mueller <smueller@chronox.de> + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/lrng.h> + +#include "lrng_internal.h" + +static int lrng_drng_switch(struct lrng_drng *drng_store, + const struct lrng_crypto_cb *cb, int node) +{ + const struct lrng_crypto_cb *old_cb; + unsigned long flags = 0; + int ret; + u8 seed[LRNG_DRNG_SECURITY_STRENGTH_BYTES]; + void *new_drng = cb->lrng_drng_alloc(LRNG_DRNG_SECURITY_STRENGTH_BYTES); + void *old_drng, *new_hash, *old_hash; + bool sl = false, reset_drng = !lrng_get_available(); + + if (IS_ERR(new_drng)) { + pr_warn("could not allocate new DRNG for NUMA node %d (%ld)\n", + node, PTR_ERR(new_drng)); + return PTR_ERR(new_drng); + } + + new_hash = cb->lrng_hash_alloc(seed, sizeof(seed)); + if (IS_ERR(new_hash)) { + pr_warn("could not allocate new LRNG pool hash (%ld)\n", + PTR_ERR(new_hash)); + cb->lrng_drng_dealloc(new_drng); + return PTR_ERR(new_hash); + } + + lrng_drng_lock(drng_store, &flags); + + /* + * Pull from existing DRNG to seed new DRNG regardless of seed status + * of old DRNG -- the entropy state for the DRNG is left unchanged which + * implies that als the new DRNG is reseeded when deemed necessary. This + * seeding of the new DRNG shall only ensure that the new DRNG has the + * same entropy as the old DRNG. + */ + ret = drng_store->crypto_cb->lrng_drng_generate_helper( + drng_store->drng, seed, sizeof(seed)); + lrng_drng_unlock(drng_store, &flags); + + if (ret < 0) { + reset_drng = true; + pr_warn("getting random data from DRNG failed for NUMA node %d " + "(%d)\n", node, ret); + } else { + /* seed new DRNG with data */ + ret = cb->lrng_drng_seed_helper(new_drng, seed, ret); + if (ret < 0) { + reset_drng = true; + pr_warn("seeding of new DRNG failed for NUMA node %d " + "(%d)\n", node, ret); + } else { + pr_debug("seeded new DRNG of NUMA node %d instance " + "from old DRNG instance\n", node); + } + } + + mutex_lock(&drng_store->lock); + /* + * If we switch the DRNG from the initial ChaCha20 DRNG to something + * else, there is a lock transition from spin lock to mutex (see + * lrng_drng_is_atomic and how the lock is taken in lrng_drng_lock). + * Thus, we need to take both locks during the transition phase. + */ + if (lrng_drng_is_atomic(drng_store)) { + spin_lock_irqsave(&drng_store->spin_lock, flags); + sl = true; + } + + if (reset_drng) + lrng_drng_reset(drng_store); + + old_drng = drng_store->drng; + old_cb = drng_store->crypto_cb; + drng_store->drng = new_drng; + drng_store->crypto_cb = cb; + + old_hash = drng_store->hash; + drng_store->hash = new_hash; + pr_info("Entropy pool read-hash allocated for DRNG for NUMA node %d\n", + node); + + if (sl) + spin_unlock_irqrestore(&drng_store->spin_lock, flags); + mutex_unlock(&drng_store->lock); + + /* ChaCha20 serves as atomic instance left untouched. */ + if (old_drng != &chacha20) { + old_cb->lrng_drng_dealloc(old_drng); + old_cb->lrng_hash_dealloc(old_hash); + } + + pr_info("DRNG of NUMA node %d switched\n", node); + + return 0; +} + +/** + * Switch the existing DRNG instances with new using the new crypto callbacks. + * The caller must hold the lrng_crypto_cb_update lock. + */ +static int lrng_drngs_switch(const struct lrng_crypto_cb *cb) +{ + struct lrng_drng **lrng_drng = lrng_drng_instances(); + struct lrng_drng *lrng_drng_init = lrng_drng_init_instance(); + int ret = 0; + + /* Update DRNG */ + if (lrng_drng) { + u32 node; + + for_each_online_node(node) { + if (lrng_drng[node]) + ret = lrng_drng_switch(lrng_drng[node], cb, + node); + } + } else { + ret = lrng_drng_switch(lrng_drng_init, cb, 0); + } + + if (!ret) + lrng_set_available(); + + return 0; +} + +/** + * lrng_set_drng_cb - Register new cryptographic callback functions for DRNG + * The registering implies that all old DRNG states are replaced with new + * DRNG states. + * @cb: Callback functions to be registered -- if NULL, use the default + * callbacks pointing to the ChaCha20 DRNG. + * @return: 0 on success, < 0 on error + */ +int lrng_set_drng_cb(const struct lrng_crypto_cb *cb) +{ + struct lrng_drng *lrng_drng_init = lrng_drng_init_instance(); + int ret; + + if (!cb) + cb = &lrng_cc20_crypto_cb; + + mutex_lock(&lrng_crypto_cb_update); + + /* + * If a callback other than the default is set, allow it only to be + * set back to the default callback. This ensures that multiple + * different callbacks can be registered at the same time. If a + * callback different from the current callback and the default + * callback shall be set, the current callback must be deregistered + * (e.g. the kernel module providing it must be unloaded) and the new + * implementation can be registered. + */ + if ((cb != &lrng_cc20_crypto_cb) && + (lrng_drng_init->crypto_cb != &lrng_cc20_crypto_cb)) { + pr_warn("disallow setting new cipher callbacks, unload the old " + "callbacks first!\n"); + ret = -EINVAL; + goto out; + } + + ret = lrng_drngs_switch(cb); + +out: + mutex_unlock(&lrng_crypto_cb_update); + return ret; +} +EXPORT_SYMBOL(lrng_set_drng_cb);