| Message ID | 20180614200811.76401d95@wiggum (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
On 14.06.2018 20:08, Michael Büsch wrote: > enable_best_rng() is used in hwrng_unregister() to switch away from the > currently active RNG, if that is the one currently being removed. > However enable_best_rng() might fail, if the next RNG's init routine > fails. In that case enable_best_rng() will return an error code and > the currently active RNG will remain active. > After unregistering this might lead to crashes due to use-after-free. > > Fix this by dropping the currently active RNG, if enable_best_rng() > failed. This will result in no RNG to be active, if the next-best > one failed to initialize. > > This problem was introduced by 142a27f0a731ddcf467546960a5585970ca98e21 > > > Reported-by: Wirz <spam@lukas-wirz.de> > Tested-by: Wirz <spam@lukas-wirz.de> > Signed-off-by: Michael Büsch <m@bues.ch> > Cc: stable@vger.kernel.org > > --- > > See this discussion for a crash in b43's hwrng caused by this problem: > https://www.spinics.net/lists/linux-wireless/msg173089.html > > > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c > index 91bb98c42a1c..aaf9e5afaad4 100644 > --- a/drivers/char/hw_random/core.c > +++ b/drivers/char/hw_random/core.c > @@ -516,11 +516,18 @@ EXPORT_SYMBOL_GPL(hwrng_register); > > void hwrng_unregister(struct hwrng *rng) > { > + int err; > + > mutex_lock(&rng_mutex); > > list_del(&rng->list); > - if (current_rng == rng) > - enable_best_rng(); > + if (current_rng == rng) { > + err = enable_best_rng(); > + if (err) { > + drop_current_rng(); > + cur_rng_set_by_user = 0; > + } > + } > > if (list_empty(&rng_list)) { > mutex_unlock(&rng_mutex); > > > Yes, if the hw_init() of the newly chosen rng fails, enable_best_rng() comes back with an error and did not drop the current rng. The patch handles this case. I did not test it, but the code looks fine. reviewed-by Harald Freudenberger <freude@linux.ibm.com>
Michael Büsch <m@bues.ch> wrote: > [-- text/plain, encoding quoted-printable, charset: UTF-8, 57 lines --] > > enable_best_rng() is used in hwrng_unregister() to switch away from the > currently active RNG, if that is the one currently being removed. > However enable_best_rng() might fail, if the next RNG's init routine > fails. In that case enable_best_rng() will return an error code and > the currently active RNG will remain active. > After unregistering this might lead to crashes due to use-after-free. > > Fix this by dropping the currently active RNG, if enable_best_rng() > failed. This will result in no RNG to be active, if the next-best > one failed to initialize. > > This problem was introduced by 142a27f0a731ddcf467546960a5585970ca98e21 > > > Reported-by: Wirz <spam@lukas-wirz.de> > Tested-by: Wirz <spam@lukas-wirz.de> > Signed-off-by: Michael Büsch <m@bues.ch> > Cc: stable@vger.kernel.org Patch applied. Thanks.
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c index 91bb98c42a1c..aaf9e5afaad4 100644 --- a/drivers/char/hw_random/core.c +++ b/drivers/char/hw_random/core.c @@ -516,11 +516,18 @@ EXPORT_SYMBOL_GPL(hwrng_register); void hwrng_unregister(struct hwrng *rng) { + int err; + mutex_lock(&rng_mutex); list_del(&rng->list); - if (current_rng == rng) - enable_best_rng(); + if (current_rng == rng) { + err = enable_best_rng(); + if (err) { + drop_current_rng(); + cur_rng_set_by_user = 0; + } + } if (list_empty(&rng_list)) { mutex_unlock(&rng_mutex);