Linux Virtualization - Jul 2014 - [PATCH 1/2 v2] hwrng: Allow drivers to disable reading during probe

On Wed, Jul 02, 2014 at 06:56:35PM +0530, Amit Shah
wrote:
> Hi Jason,
> 
> On (Wed) 02 Jul 2014 [13:00:19], Jason Cooper wrote:
> > Commit d9e7972619334 "hwrng: add randomness to system from rng
sources"
> > added a call to rng_get_data() from the hwrng_register() function.
> > However, some rng devices need initialization before data can be read
> > from them.
> > 
> > Also, the virtio-rng device does not behave properly when this call is
> > made in its probe() routine - the virtio core sets the DRIVER_OK
status
> > bit only on a successful probe, which means the host ignores all
> > communication from the guest, and the guest insmod or boot process
just
> > sits there doing nothing.
> > 
> > [ jac: Modify the API to allow drivers to disable reading at probe,
new
> > patch, copied Amit's commit message. ]
> > 
> > CC: Kees Cook <keescook at chromium.org>
> > CC: Jason Cooper <jason at lakedaemon.net>
> > CC: Herbert Xu <herbert at gondor.apana.org.au>
> > CC: <stable at vger.kernel.org> # v3.15+
> > Signed-off-by: Amit Shah <amit.shah at redhat.com>
> > Signed-off-by: Jason Cooper <jason at lakedaemon.net>
> > ---
> >  drivers/char/hw_random/core.c | 8 +++++---
> >  include/linux/hw_random.h     | 4 ++++
> >  2 files changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/char/hw_random/core.c
b/drivers/char/hw_random/core.c
> > index 334601cc81cf..b7b6c48ca682 100644
> > --- a/drivers/char/hw_random/core.c
> > +++ b/drivers/char/hw_random/core.c
> > @@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)
> >  	INIT_LIST_HEAD(&rng->list);
> >  	list_add_tail(&rng->list, &rng_list);
> >  
> > -	bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
> > -	if (bytes_read > 0)
> > -		add_device_randomness(bytes, bytes_read);
> > +	if (!(rng->flags & HWRNG_NO_READ_AT_PROBE)) {
> > +		bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
> > +		if (bytes_read > 0)
> > +			add_device_randomness(bytes, bytes_read);
> > +	}
> 
> But this has the inverse problem: if there are two hwrngs in the
> system, one will be initialized and probed.  The other will not be
> initialized, but still be probed.
That's a problem outside the scope of this patch.  You're basically
saying the ->init() should be called unconditionally for each hwrng.  If
that's what driver authors assumed, that's not what is happening if
there is more than one driver in the system.

I think you should be changing the code a few lines up to make sure
hwrng_init() is called once for each driver.
> My version was more conservative while this one keeps the bug from the
> current kernels.
Huh?  What do you mean by "keeps the bug from the current kernels." ?

Besides, you're second patch isn't actually doing any ->init to get
the
hwrng ready for reading...  If you had a real ->init function, and it
was always called, would rng_get_data() work at probe time for your
driver?

confused,

Jason.

On Wed, Jul 2, 2014 at 6:41 AM, Jason Cooper <jason at lakedaemon.net>
wrote:
> On Wed, Jul 02, 2014 at 06:56:35PM +0530, Amit Shah wrote:
>> Hi Jason,
>>
>> On (Wed) 02 Jul 2014 [13:00:19], Jason Cooper wrote:
>> > Commit d9e7972619334 "hwrng: add randomness to system from
rng sources"
>> > added a call to rng_get_data() from the hwrng_register() function.
>> > However, some rng devices need initialization before data can be
read
>> > from them.
>> >
>> > Also, the virtio-rng device does not behave properly when this
call is
>> > made in its probe() routine - the virtio core sets the DRIVER_OK
status
>> > bit only on a successful probe, which means the host ignores all
>> > communication from the guest, and the guest insmod or boot process
just
>> > sits there doing nothing.
>> >
>> > [ jac: Modify the API to allow drivers to disable reading at
probe, new
>> > patch, copied Amit's commit message. ]
>> >
>> > CC: Kees Cook <keescook at chromium.org>
>> > CC: Jason Cooper <jason at lakedaemon.net>
>> > CC: Herbert Xu <herbert at gondor.apana.org.au>
>> > CC: <stable at vger.kernel.org> # v3.15+
>> > Signed-off-by: Amit Shah <amit.shah at redhat.com>
>> > Signed-off-by: Jason Cooper <jason at lakedaemon.net>
>> > ---
>> >  drivers/char/hw_random/core.c | 8 +++++---
>> >  include/linux/hw_random.h     | 4 ++++
>> >  2 files changed, 9 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/char/hw_random/core.c
b/drivers/char/hw_random/core.c
>> > index 334601cc81cf..b7b6c48ca682 100644
>> > --- a/drivers/char/hw_random/core.c
>> > +++ b/drivers/char/hw_random/core.c
>> > @@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)
>> >     INIT_LIST_HEAD(&rng->list);
>> >     list_add_tail(&rng->list, &rng_list);
>> >
>> > -   bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
>> > -   if (bytes_read > 0)
>> > -           add_device_randomness(bytes, bytes_read);
>> > +   if (!(rng->flags & HWRNG_NO_READ_AT_PROBE)) {
>> > +           bytes_read = rng_get_data(rng, bytes, sizeof(bytes),
1);
>> > +           if (bytes_read > 0)
>> > +                   add_device_randomness(bytes, bytes_read);
>> > +   }
>>
>> But this has the inverse problem: if there are two hwrngs in the
>> system, one will be initialized and probed.  The other will not be
>> initialized, but still be probed.
>
> That's a problem outside the scope of this patch.  You're basically
> saying the ->init() should be called unconditionally for each hwrng.  If
> that's what driver authors assumed, that's not what is happening if
> there is more than one driver in the system.
My intent with the patch was to get randomness added when a hwrng was
available. Perhaps instead of skipping this call, it can be done
either during probe (for devices that support it or lack an init
function), or done after initialization?
> I think you should be changing the code a few lines up to make sure
> hwrng_init() is called once for each driver.
Is that really how it should work? I'd be for the "always-init"
approach since it gains us access to the entropy, but I have to play
devil's advocate: would one expect "probe" to just probe, not
initialize? That seems more like a function of enabling the device.

-Kees

-- 
Kees Cook
Chrome OS Security

On (Wed) 02 Jul 2014 [09:41:56], Jason Cooper wrote:
> On Wed, Jul 02, 2014 at 06:56:35PM +0530, Amit Shah wrote:
> > Hi Jason,
> > 
> > On (Wed) 02 Jul 2014 [13:00:19], Jason Cooper wrote:
> > > Commit d9e7972619334 "hwrng: add randomness to system from
rng sources"
> > > added a call to rng_get_data() from the hwrng_register()
function.
> > > However, some rng devices need initialization before data can be
read
> > > from them.
> > > 
> > > Also, the virtio-rng device does not behave properly when this
call is
> > > made in its probe() routine - the virtio core sets the DRIVER_OK
status
> > > bit only on a successful probe, which means the host ignores all
> > > communication from the guest, and the guest insmod or boot
process just
> > > sits there doing nothing.
> > > 
> > > [ jac: Modify the API to allow drivers to disable reading at
probe, new
> > > patch, copied Amit's commit message. ]
> > > 
> > > CC: Kees Cook <keescook at chromium.org>
> > > CC: Jason Cooper <jason at lakedaemon.net>
> > > CC: Herbert Xu <herbert at gondor.apana.org.au>
> > > CC: <stable at vger.kernel.org> # v3.15+
> > > Signed-off-by: Amit Shah <amit.shah at redhat.com>
> > > Signed-off-by: Jason Cooper <jason at lakedaemon.net>
> > > ---
> > >  drivers/char/hw_random/core.c | 8 +++++---
> > >  include/linux/hw_random.h     | 4 ++++
> > >  2 files changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/char/hw_random/core.c
b/drivers/char/hw_random/core.c
> > > index 334601cc81cf..b7b6c48ca682 100644
> > > --- a/drivers/char/hw_random/core.c
> > > +++ b/drivers/char/hw_random/core.c
> > > @@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)
> > >  	INIT_LIST_HEAD(&rng->list);
> > >  	list_add_tail(&rng->list, &rng_list);
> > >  
> > > -	bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
> > > -	if (bytes_read > 0)
> > > -		add_device_randomness(bytes, bytes_read);
> > > +	if (!(rng->flags & HWRNG_NO_READ_AT_PROBE)) {
> > > +		bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
> > > +		if (bytes_read > 0)
> > > +			add_device_randomness(bytes, bytes_read);
> > > +	}
> > 
> > But this has the inverse problem: if there are two hwrngs in the
> > system, one will be initialized and probed.  The other will not be
> > initialized, but still be probed.
> 
> That's a problem outside the scope of this patch.
No, not really.  What will happen in a 2 hwrng scenario is that the
first one that calls hwrng_register() will get its init() routine
called, and probed for data properly.

The second device to call hwrng_register() will not get its init
routine called, but still will be probed.  And that will result in
unpredictable behaviour, as the device wouldn't have been initialized
for reading of the data.  This bug was introduced by the original
patch, which was fixed by my series (by ensuring probe is only called
if init was called first.  But it missed one case where probe will not
be called even if init was called, as you pointed out).
>  You're basically
> saying the ->init() should be called unconditionally for each hwrng.  If
> that's what driver authors assumed, that's not what is happening if
> there is more than one driver in the system.
Either that, or that rng_get_data() should be called in hwrng_init(),
or not call rng_get_data() at all on that device.
> I think you should be changing the code a few lines up to make sure
> hwrng_init() is called once for each driver.
> 
> > My version was more conservative while this one keeps the bug from the
> > current kernels.
> 
> Huh?  What do you mean by "keeps the bug from the current
kernels." ?
Sorry; hope the explanation above is clearer.
> Besides, you're second patch isn't actually doing any ->init to
get the
> hwrng ready for reading...  If you had a real ->init function, and it
> was always called, would rng_get_data() work at probe time for your
> driver?
Right; that's the reason I didn't do it that way.

I think a combination of my patches and your flags approach will solve
the issues; I'll post a v2 tomorrow.


		Amit

On (Wed) 02 Jul 2014 [08:11:30], Kees Cook wrote:
> On Wed, Jul 2, 2014 at 6:41 AM, Jason Cooper <jason at
lakedaemon.net> wrote:
> > On Wed, Jul 02, 2014 at 06:56:35PM +0530, Amit Shah wrote:
> >> Hi Jason,
> >>
> >> On (Wed) 02 Jul 2014 [13:00:19], Jason Cooper wrote:
> >> > Commit d9e7972619334 "hwrng: add randomness to system
from rng sources"
> >> > added a call to rng_get_data() from the hwrng_register()
function.
> >> > However, some rng devices need initialization before data can
be read
> >> > from them.
> >> >
> >> > Also, the virtio-rng device does not behave properly when
this call is
> >> > made in its probe() routine - the virtio core sets the
DRIVER_OK status
> >> > bit only on a successful probe, which means the host ignores
all
> >> > communication from the guest, and the guest insmod or boot
process just
> >> > sits there doing nothing.
> >> >
> >> > [ jac: Modify the API to allow drivers to disable reading at
probe, new
> >> > patch, copied Amit's commit message. ]
> >> >
> >> > CC: Kees Cook <keescook at chromium.org>
> >> > CC: Jason Cooper <jason at lakedaemon.net>
> >> > CC: Herbert Xu <herbert at gondor.apana.org.au>
> >> > CC: <stable at vger.kernel.org> # v3.15+
> >> > Signed-off-by: Amit Shah <amit.shah at redhat.com>
> >> > Signed-off-by: Jason Cooper <jason at lakedaemon.net>
> >> > ---
> >> >  drivers/char/hw_random/core.c | 8 +++++---
> >> >  include/linux/hw_random.h     | 4 ++++
> >> >  2 files changed, 9 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/char/hw_random/core.c
b/drivers/char/hw_random/core.c
> >> > index 334601cc81cf..b7b6c48ca682 100644
> >> > --- a/drivers/char/hw_random/core.c
> >> > +++ b/drivers/char/hw_random/core.c
> >> > @@ -347,9 +347,11 @@ int hwrng_register(struct hwrng *rng)
> >> >     INIT_LIST_HEAD(&rng->list);
> >> >     list_add_tail(&rng->list, &rng_list);
> >> >
> >> > -   bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
> >> > -   if (bytes_read > 0)
> >> > -           add_device_randomness(bytes, bytes_read);
> >> > +   if (!(rng->flags & HWRNG_NO_READ_AT_PROBE)) {
> >> > +           bytes_read = rng_get_data(rng, bytes,
sizeof(bytes), 1);
> >> > +           if (bytes_read > 0)
> >> > +                   add_device_randomness(bytes, bytes_read);
> >> > +   }
> >>
> >> But this has the inverse problem: if there are two hwrngs in the
> >> system, one will be initialized and probed.  The other will not be
> >> initialized, but still be probed.
> >
> > That's a problem outside the scope of this patch.  You're
basically
> > saying the ->init() should be called unconditionally for each
hwrng.  If
> > that's what driver authors assumed, that's not what is
happening if
> > there is more than one driver in the system.
> 
> My intent with the patch was to get randomness added when a hwrng was
> available. Perhaps instead of skipping this call, it can be done
> either during probe (for devices that support it or lack an init
> function), or done after initialization?
I'll also look at just going ahead with ->init() in the hwrng_register
function itself, that's in line with the spirit of your patch.

However, that may lead to unwelcome problems.  The other approach is
to call rng_get_data() in hwrng_init().
> > I think you should be changing the code a few lines up to make sure
> > hwrng_init() is called once for each driver.
> 
> Is that really how it should work? I'd be for the
"always-init"
> approach since it gains us access to the entropy, but I have to play
> devil's advocate: would one expect "probe" to just probe, not
> initialize? That seems more like a function of enabling the device.
There are some other considerations: I found this problem while
looking at other things: especially that the hwrng core doesn't take a
reference on the driver that's the current_rng.  That has to be
fixed.  If we always call ->init(), we'll have to cleanup and possibly
take a reference.  I'll look at this more closely in a bit.

		Amit