Re: [PATCH 4.16 163/196] random: fix crng_ready() test

[Dan Rue]

On Sat, Apr 28, 2018 at 08:00:03AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Apr 27, 2018 at 11:34:43AM -0500, Dan Rue wrote:
> > On Sun, Apr 22, 2018 at 03:53:03PM +0200, Greg Kroah-Hartman wrote:
> > > 4.16-stable review patch.  If anyone has any objections, please let me 
> > > know.
> > > 
> > > --
> > > 
> > > From: Theodore Ts'o 
> > > 
> > > commit 43838a23a05fbd13e47d750d3dfd77001536dd33 upstream.
> > > 
> > > The crng_init variable has three states:
> > > 
> > > 0: The CRNG is not initialized at all
> > > 1: The CRNG has a small amount of entropy, hopefully good enough for
> > >early-boot, non-cryptographical use cases
> > > 2: The CRNG is fully initialized and we are sure it is safe for
> > >cryptographic use cases.
> > > 
> > > The crng_ready() function should only return true once we are in the
> > > last state.  This addresses CVE-2018-1108.
> > > 
> > > Reported-by: Jann Horn 
> > > Fixes: e192be9d9a30 ("random: replace non-blocking pool...")
> > > Cc: sta...@kernel.org # 4.8+
> > > Signed-off-by: Theodore Ts'o 
> > > Reviewed-by: Jann Horn 
> > > Signed-off-by: Greg Kroah-Hartman 
> > 
> > This patch has caused a regression on 4.16 using kselftest lib/printf.sh
> > - specifically, when it runs "/sbin/modprobe test_printf". This
> > regression has been detected on arm64 dragonboard 410c (not seen on
> > other arm64 or x86_64 devices).
> > 
> > /sbin/modprobe test_printf
> > [   22.725551] test_printf: hashing plain 'p' has unexpected format
> > [   22.726031] test_printf: failed 1 out of 236 tests
> > modprobe: ERROR: could not insert 'test_printf': Invalid argument
> > 
> > This problem has not been seen on 4.9 or 4.14 under the same
> > conditions.
> 
> Does 4.17-rc2 also fail like this?

No, but I did see it in v4.17-rc2-102-g3442097b765c, and it was fixed in
the subsequent build:

good: v4.17-rc2-64-g26ed24e429d8
bad: v4.17-rc2-102-g3442097b765c
good: v4.17-rc2-104-g69bfd470f462

I haven't investigated deeper though, and I do not see any changes to
random in those above revisions. This test passes on all the other
mainline builds, including v4.17-rc2.

> 
> You all are testing the -rc releases, right?  :)

We test every push to mainline and we test -next daily.

> 
> I think the random changes that will be in 4.17-rc3 should fix this, and
> if so, I'll suck them in here too.  But testing that would be good to
> see happen...

Waiting is fine with me.

Thanks,
Dan

Re: [PATCH 4.16 163/196] random: fix crng_ready() test

[Dan Rue]

On Sat, Apr 28, 2018 at 08:00:03AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Apr 27, 2018 at 11:34:43AM -0500, Dan Rue wrote:
> > On Sun, Apr 22, 2018 at 03:53:03PM +0200, Greg Kroah-Hartman wrote:
> > > 4.16-stable review patch.  If anyone has any objections, please let me 
> > > know.
> > > 
> > > --
> > > 
> > > From: Theodore Ts'o 
> > > 
> > > commit 43838a23a05fbd13e47d750d3dfd77001536dd33 upstream.
> > > 
> > > The crng_init variable has three states:
> > > 
> > > 0: The CRNG is not initialized at all
> > > 1: The CRNG has a small amount of entropy, hopefully good enough for
> > >early-boot, non-cryptographical use cases
> > > 2: The CRNG is fully initialized and we are sure it is safe for
> > >cryptographic use cases.
> > > 
> > > The crng_ready() function should only return true once we are in the
> > > last state.  This addresses CVE-2018-1108.
> > > 
> > > Reported-by: Jann Horn 
> > > Fixes: e192be9d9a30 ("random: replace non-blocking pool...")
> > > Cc: sta...@kernel.org # 4.8+
> > > Signed-off-by: Theodore Ts'o 
> > > Reviewed-by: Jann Horn 
> > > Signed-off-by: Greg Kroah-Hartman 
> > 
> > This patch has caused a regression on 4.16 using kselftest lib/printf.sh
> > - specifically, when it runs "/sbin/modprobe test_printf". This
> > regression has been detected on arm64 dragonboard 410c (not seen on
> > other arm64 or x86_64 devices).
> > 
> > /sbin/modprobe test_printf
> > [   22.725551] test_printf: hashing plain 'p' has unexpected format
> > [   22.726031] test_printf: failed 1 out of 236 tests
> > modprobe: ERROR: could not insert 'test_printf': Invalid argument
> > 
> > This problem has not been seen on 4.9 or 4.14 under the same
> > conditions.
> 
> Does 4.17-rc2 also fail like this?

No, but I did see it in v4.17-rc2-102-g3442097b765c, and it was fixed in
the subsequent build:

good: v4.17-rc2-64-g26ed24e429d8
bad: v4.17-rc2-102-g3442097b765c
good: v4.17-rc2-104-g69bfd470f462

I haven't investigated deeper though, and I do not see any changes to
random in those above revisions. This test passes on all the other
mainline builds, including v4.17-rc2.

> 
> You all are testing the -rc releases, right?  :)

We test every push to mainline and we test -next daily.

> 
> I think the random changes that will be in 4.17-rc3 should fix this, and
> if so, I'll suck them in here too.  But testing that would be good to
> see happen...

Waiting is fine with me.

Thanks,
Dan

Re: [PATCH 4.16 163/196] random: fix crng_ready() test

[Greg Kroah-Hartman]

On Fri, Apr 27, 2018 at 11:34:43AM -0500, Dan Rue wrote:
> On Sun, Apr 22, 2018 at 03:53:03PM +0200, Greg Kroah-Hartman wrote:
> > 4.16-stable review patch.  If anyone has any objections, please let me know.
> > 
> > --
> > 
> > From: Theodore Ts'o 
> > 
> > commit 43838a23a05fbd13e47d750d3dfd77001536dd33 upstream.
> > 
> > The crng_init variable has three states:
> > 
> > 0: The CRNG is not initialized at all
> > 1: The CRNG has a small amount of entropy, hopefully good enough for
> >early-boot, non-cryptographical use cases
> > 2: The CRNG is fully initialized and we are sure it is safe for
> >cryptographic use cases.
> > 
> > The crng_ready() function should only return true once we are in the
> > last state.  This addresses CVE-2018-1108.
> > 
> > Reported-by: Jann Horn 
> > Fixes: e192be9d9a30 ("random: replace non-blocking pool...")
> > Cc: sta...@kernel.org # 4.8+
> > Signed-off-by: Theodore Ts'o 
> > Reviewed-by: Jann Horn 
> > Signed-off-by: Greg Kroah-Hartman 
> 
> This patch has caused a regression on 4.16 using kselftest lib/printf.sh
> - specifically, when it runs "/sbin/modprobe test_printf". This
> regression has been detected on arm64 dragonboard 410c (not seen on
> other arm64 or x86_64 devices).
> 
> /sbin/modprobe test_printf
> [   22.725551] test_printf: hashing plain 'p' has unexpected format
> [   22.726031] test_printf: failed 1 out of 236 tests
> modprobe: ERROR: could not insert 'test_printf': Invalid argument
> 
> This problem has not been seen on 4.9 or 4.14 under the same
> conditions.

Does 4.17-rc2 also fail like this?

You all are testing the -rc releases, right?  :)

I think the random changes that will be in 4.17-rc3 should fix this, and
if so, I'll suck them in here too.  But testing that would be good to
see happen...

thanks,

greg k-h

Re: [PATCH 4.16 163/196] random: fix crng_ready() test

[Greg Kroah-Hartman]

On Fri, Apr 27, 2018 at 11:34:43AM -0500, Dan Rue wrote:
> On Sun, Apr 22, 2018 at 03:53:03PM +0200, Greg Kroah-Hartman wrote:
> > 4.16-stable review patch.  If anyone has any objections, please let me know.
> > 
> > --
> > 
> > From: Theodore Ts'o 
> > 
> > commit 43838a23a05fbd13e47d750d3dfd77001536dd33 upstream.
> > 
> > The crng_init variable has three states:
> > 
> > 0: The CRNG is not initialized at all
> > 1: The CRNG has a small amount of entropy, hopefully good enough for
> >early-boot, non-cryptographical use cases
> > 2: The CRNG is fully initialized and we are sure it is safe for
> >cryptographic use cases.
> > 
> > The crng_ready() function should only return true once we are in the
> > last state.  This addresses CVE-2018-1108.
> > 
> > Reported-by: Jann Horn 
> > Fixes: e192be9d9a30 ("random: replace non-blocking pool...")
> > Cc: sta...@kernel.org # 4.8+
> > Signed-off-by: Theodore Ts'o 
> > Reviewed-by: Jann Horn 
> > Signed-off-by: Greg Kroah-Hartman 
> 
> This patch has caused a regression on 4.16 using kselftest lib/printf.sh
> - specifically, when it runs "/sbin/modprobe test_printf". This
> regression has been detected on arm64 dragonboard 410c (not seen on
> other arm64 or x86_64 devices).
> 
> /sbin/modprobe test_printf
> [   22.725551] test_printf: hashing plain 'p' has unexpected format
> [   22.726031] test_printf: failed 1 out of 236 tests
> modprobe: ERROR: could not insert 'test_printf': Invalid argument
> 
> This problem has not been seen on 4.9 or 4.14 under the same
> conditions.

Does 4.17-rc2 also fail like this?

You all are testing the -rc releases, right?  :)

I think the random changes that will be in 4.17-rc3 should fix this, and
if so, I'll suck them in here too.  But testing that would be good to
see happen...

thanks,

greg k-h

Re: [PATCH 4.16 163/196] random: fix crng_ready() test

[Dan Rue]

On Sun, Apr 22, 2018 at 03:53:03PM +0200, Greg Kroah-Hartman wrote:
> 4.16-stable review patch.  If anyone has any objections, please let me know.
> 
> --
> 
> From: Theodore Ts'o 
> 
> commit 43838a23a05fbd13e47d750d3dfd77001536dd33 upstream.
> 
> The crng_init variable has three states:
> 
> 0: The CRNG is not initialized at all
> 1: The CRNG has a small amount of entropy, hopefully good enough for
>early-boot, non-cryptographical use cases
> 2: The CRNG is fully initialized and we are sure it is safe for
>cryptographic use cases.
> 
> The crng_ready() function should only return true once we are in the
> last state.  This addresses CVE-2018-1108.
> 
> Reported-by: Jann Horn 
> Fixes: e192be9d9a30 ("random: replace non-blocking pool...")
> Cc: sta...@kernel.org # 4.8+
> Signed-off-by: Theodore Ts'o 
> Reviewed-by: Jann Horn 
> Signed-off-by: Greg Kroah-Hartman 

This patch has caused a regression on 4.16 using kselftest lib/printf.sh
- specifically, when it runs "/sbin/modprobe test_printf". This
regression has been detected on arm64 dragonboard 410c (not seen on
other arm64 or x86_64 devices).

/sbin/modprobe test_printf
[   22.725551] test_printf: hashing plain 'p' has unexpected format
[   22.726031] test_printf: failed 1 out of 236 tests
modprobe: ERROR: could not insert 'test_printf': Invalid argument

This problem has not been seen on 4.9 or 4.14 under the same
conditions.

Dan

> 
> ---
>  drivers/char/random.c |   10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -427,7 +427,7 @@ struct crng_state primary_crng = {
>   * its value (from 0->1->2).
>   */
>  static int crng_init = 0;
> -#define crng_ready() (likely(crng_init > 0))
> +#define crng_ready() (likely(crng_init > 1))
>  static int crng_init_cnt = 0;
>  #define CRNG_INIT_CNT_THRESH (2*CHACHA20_KEY_SIZE)
>  static void _extract_crng(struct crng_state *crng,
> @@ -793,7 +793,7 @@ static int crng_fast_load(const char *cp
>  
>   if (!spin_trylock_irqsave(_crng.lock, flags))
>   return 0;
> - if (crng_ready()) {
> + if (crng_init != 0) {
>   spin_unlock_irqrestore(_crng.lock, flags);
>   return 0;
>   }
> @@ -855,7 +855,7 @@ static void _extract_crng(struct crng_st
>  {
>   unsigned long v, flags;
>  
> - if (crng_init > 1 &&
> + if (crng_ready() &&
>   time_after(jiffies, crng->init_time + CRNG_RESEED_INTERVAL))
>   crng_reseed(crng, crng == _crng ? _pool : NULL);
>   spin_lock_irqsave(>lock, flags);
> @@ -1141,7 +1141,7 @@ void add_interrupt_randomness(int irq, i
>   fast_mix(fast_pool);
>   add_interrupt_bench(cycles);
>  
> - if (!crng_ready()) {
> + if (unlikely(crng_init == 0)) {
>   if ((fast_pool->count >= 64) &&
>   crng_fast_load((char *) fast_pool->pool,
>  sizeof(fast_pool->pool))) {
> @@ -2214,7 +2214,7 @@ void add_hwgenerator_randomness(const ch
>  {
>   struct entropy_store *poolp = _pool;
>  
> - if (!crng_ready()) {
> + if (unlikely(crng_init == 0)) {
>   crng_fast_load(buffer, count);
>   return;
>   }
> 
> 

Re: [PATCH 4.16 163/196] random: fix crng_ready() test

[Dan Rue]

On Sun, Apr 22, 2018 at 03:53:03PM +0200, Greg Kroah-Hartman wrote:
> 4.16-stable review patch.  If anyone has any objections, please let me know.
> 
> --
> 
> From: Theodore Ts'o 
> 
> commit 43838a23a05fbd13e47d750d3dfd77001536dd33 upstream.
> 
> The crng_init variable has three states:
> 
> 0: The CRNG is not initialized at all
> 1: The CRNG has a small amount of entropy, hopefully good enough for
>early-boot, non-cryptographical use cases
> 2: The CRNG is fully initialized and we are sure it is safe for
>cryptographic use cases.
> 
> The crng_ready() function should only return true once we are in the
> last state.  This addresses CVE-2018-1108.
> 
> Reported-by: Jann Horn 
> Fixes: e192be9d9a30 ("random: replace non-blocking pool...")
> Cc: sta...@kernel.org # 4.8+
> Signed-off-by: Theodore Ts'o 
> Reviewed-by: Jann Horn 
> Signed-off-by: Greg Kroah-Hartman 

This patch has caused a regression on 4.16 using kselftest lib/printf.sh
- specifically, when it runs "/sbin/modprobe test_printf". This
regression has been detected on arm64 dragonboard 410c (not seen on
other arm64 or x86_64 devices).

/sbin/modprobe test_printf
[   22.725551] test_printf: hashing plain 'p' has unexpected format
[   22.726031] test_printf: failed 1 out of 236 tests
modprobe: ERROR: could not insert 'test_printf': Invalid argument

This problem has not been seen on 4.9 or 4.14 under the same
conditions.

Dan

> 
> ---
>  drivers/char/random.c |   10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -427,7 +427,7 @@ struct crng_state primary_crng = {
>   * its value (from 0->1->2).
>   */
>  static int crng_init = 0;
> -#define crng_ready() (likely(crng_init > 0))
> +#define crng_ready() (likely(crng_init > 1))
>  static int crng_init_cnt = 0;
>  #define CRNG_INIT_CNT_THRESH (2*CHACHA20_KEY_SIZE)
>  static void _extract_crng(struct crng_state *crng,
> @@ -793,7 +793,7 @@ static int crng_fast_load(const char *cp
>  
>   if (!spin_trylock_irqsave(_crng.lock, flags))
>   return 0;
> - if (crng_ready()) {
> + if (crng_init != 0) {
>   spin_unlock_irqrestore(_crng.lock, flags);
>   return 0;
>   }
> @@ -855,7 +855,7 @@ static void _extract_crng(struct crng_st
>  {
>   unsigned long v, flags;
>  
> - if (crng_init > 1 &&
> + if (crng_ready() &&
>   time_after(jiffies, crng->init_time + CRNG_RESEED_INTERVAL))
>   crng_reseed(crng, crng == _crng ? _pool : NULL);
>   spin_lock_irqsave(>lock, flags);
> @@ -1141,7 +1141,7 @@ void add_interrupt_randomness(int irq, i
>   fast_mix(fast_pool);
>   add_interrupt_bench(cycles);
>  
> - if (!crng_ready()) {
> + if (unlikely(crng_init == 0)) {
>   if ((fast_pool->count >= 64) &&
>   crng_fast_load((char *) fast_pool->pool,
>  sizeof(fast_pool->pool))) {
> @@ -2214,7 +2214,7 @@ void add_hwgenerator_randomness(const ch
>  {
>   struct entropy_store *poolp = _pool;
>  
> - if (!crng_ready()) {
> + if (unlikely(crng_init == 0)) {
>   crng_fast_load(buffer, count);
>   return;
>   }
> 
>