linux-kernel - Re: [PATCH] random: Initialize vsprintf's pointer hash once the random core is ready.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 29 Jul 2022 12:34:46 +0200
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Petr Mladek <pmladek@...e.com>
Cc:     linux-kernel@...r.kernel.org,
        "Jason A. Donenfeld" <Jason@...c4.com>,
        Theodore Ts'o <tytso@....edu>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        John Ogness <john.ogness@...utronix.de>,
        Mike Galbraith <efault@....de>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] random: Initialize vsprintf's pointer hash once the
 random core is ready.

On 2022-07-29 12:22:08 [+0200], Petr Mladek wrote:
> > --- a/drivers/char/random.c
> > +++ b/drivers/char/random.c
> > @@ -221,10 +222,15 @@ static void crng_reseed(void)
> >  		++next_gen;
> >  	WRITE_ONCE(base_crng.generation, next_gen);
> >  	WRITE_ONCE(base_crng.birth, jiffies);
> > -	if (!static_branch_likely(&crng_is_ready))
> > +	if (!static_branch_likely(&crng_is_ready)) {
> >  		crng_init = CRNG_READY;
> > +		init_hash_pointer = true;
> 
> I am not familiar with the crng code. I wonder if the following would work:
> 
> 	if (!static_branch_likely(&crng_is_ready) && crng_init != CRNG_READY) {
> 		crng_init = CRNG_READY;
> 		init_hash_pointer = true;
> 	}
> 
> The point is that vsprintf_init_hash_pointer() will be called only by
> the first caller. It would allow to remove the @filling spin lock.

Not sure about the resulting code gen but crng_is_ready is swapped with
"crng_init == CRNG_READY" and this patch already put init_hash_pointer
in an unlikely path so it might already what you suggest.

> > +	}
> >  	spin_unlock_irqrestore(&base_crng.lock, flags);
> >  	memzero_explicit(key, sizeof(key));
> > +
> > +	if (init_hash_pointer)
> > +		vsprintf_init_hash_pointer();
> >  }
> >  
> >  /*
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 3c1853a9d1c09..6fa2ebb9f9b9e 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -751,36 +751,30 @@ static int __init debug_boot_weak_hash_enable(char *str)
> >  early_param("debug_boot_weak_hash", debug_boot_weak_hash_enable);
> >  
> >  static DEFINE_STATIC_KEY_FALSE(filled_random_ptr_key);
> > +static siphash_key_t ptr_key __read_mostly;
> >  
> > -static void enable_ptr_key_workfn(struct work_struct *work)
> > +void vsprintf_init_hash_pointer(void)
> >  {
> > -	static_branch_enable(&filled_random_ptr_key);
> > +	static DEFINE_SPINLOCK(filling);
> > +	unsigned long flags;
> > +	static bool filled;
> > +
> > +	spin_lock_irqsave(&filling, flags);
> > +	if (!filled) {
> > +		get_random_bytes(&ptr_key, sizeof(ptr_key));
> > +		filled = true;
> > +		static_branch_enable(&filled_random_ptr_key);
> 
> This can't be called in an atomic context. Is crng_reseed() always
> called in a non-atomic context?

Since a "recent" change, get_random_bytes() and friends can't be called
from an explicit IRQ-off/preempt-off region. There was a little bit of
fallout on the RT side with this change including lockdep and something
else I don't recall. So two users that invoked that from an IRQ-off
region on RT which is in general not the rule and the code was altered.

This (vsprintf) popped up recently since there isn't much that uses %p.
And since printk can (and should) be invoked everywhere I would like to
keep that working.

> That said, the static branch is an overkill. vsprintf() is a slow
> path. It should be enough to use a simple boolean. It might require
> a simple memory barrier to serialize @ptr_key and the new boolean
> read&write.

I could do that if it is preferred. It was already there, this isn't
something I just added…

> > +	}
> > +	spin_unlock_irqrestore(&filling, flags);
> >  }
> 
> Best Regards,
> Petr

Sebastian