hw_random: move add_early_randomness() out of rng_mutex
diff mbox series

Message ID 20190912133022.14870-1-lvivier@redhat.com
State Accepted
Commit daae28debcb03eee837fadfd20788107b325c5a2
Headers show
Series
  • hw_random: move add_early_randomness() out of rng_mutex
Related show

Commit Message

Laurent Vivier Sept. 12, 2019, 1:30 p.m. UTC
add_early_randomness() is called every time a new rng backend is added
and every time it is set as the current rng provider.

add_early_randomness() is called from functions locking rng_mutex,
and if it hangs all the hw_random framework hangs: we can't read sysfs,
add or remove a backend.

This patch move add_early_randomness() out of the rng_mutex zone.
It only needs the reading_mutex.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 drivers/char/hw_random/core.c | 60 +++++++++++++++++++++++++----------
 1 file changed, 44 insertions(+), 16 deletions(-)

Comments

Herbert Xu Oct. 4, 2019, 2:26 p.m. UTC | #1
On Thu, Sep 12, 2019 at 03:30:22PM +0200, Laurent Vivier wrote:
>
> @@ -496,19 +510,24 @@ int hwrng_register(struct hwrng *rng)
>  			goto out_unlock;
>  	}
>  
> -	if (old_rng && !rng->init) {
> +	new_rng = rng;
> +	kref_get(&new_rng->ref);
> +out_unlock:
> +	mutex_unlock(&rng_mutex);
> +
> +	if (new_rng) {
> +		if (new_rng != old_rng || !rng->init) {

Is this really supposed to be || instead of &&?

>  		/*
>  		 * Use a new device's input to add some randomness to
>  		 * the system.  If this rng device isn't going to be
>  		 * used right away, its init function hasn't been
> -		 * called yet; so only use the randomness from devices
> -		 * that don't need an init callback.
> +		 * called yet by set_current_rng(); so only use the
> +		 * randomness from devices that don't need an init callback
>  		 */
> -		add_early_randomness(rng);
> +			add_early_randomness(new_rng);
> +		}
> +		put_rng(new_rng);
>  	}

Cheers,
Laurent Vivier Oct. 4, 2019, 3:32 p.m. UTC | #2
On 04/10/2019 16:26, Herbert Xu wrote:
> On Thu, Sep 12, 2019 at 03:30:22PM +0200, Laurent Vivier wrote:
>>
>> @@ -496,19 +510,24 @@ int hwrng_register(struct hwrng *rng)
>>  			goto out_unlock;
>>  	}
>>  
>> -	if (old_rng && !rng->init) {
>> +	new_rng = rng;
>> +	kref_get(&new_rng->ref);
>> +out_unlock:
>> +	mutex_unlock(&rng_mutex);
>> +
>> +	if (new_rng) {
>> +		if (new_rng != old_rng || !rng->init) {
> 
> Is this really supposed to be || instead of &&?

original code calls add_early_randomness():

1- everytime a new device is plugged in except if there is an init
  function (because if there is an init function it has not been
  called and it is needed to be able to use the device)

2- everytime current device is changed, unconditionally
   because in this case the init function has already been called.
   (in hwrng_init() in set_current_rng())

"new_rng != old_rng" is for 2-, and "!rng->init" is for 1-.

So, yes, it's supposed to be "||".

Thanks,
Laurent
Herbert Xu Oct. 10, 2019, 12:53 p.m. UTC | #3
On Thu, Sep 12, 2019 at 03:30:22PM +0200, Laurent Vivier wrote:
> add_early_randomness() is called every time a new rng backend is added
> and every time it is set as the current rng provider.
> 
> add_early_randomness() is called from functions locking rng_mutex,
> and if it hangs all the hw_random framework hangs: we can't read sysfs,
> add or remove a backend.
> 
> This patch move add_early_randomness() out of the rng_mutex zone.
> It only needs the reading_mutex.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  drivers/char/hw_random/core.c | 60 +++++++++++++++++++++++++----------
>  1 file changed, 44 insertions(+), 16 deletions(-)

Patch applied.  Thanks.
Marek Szyprowski Oct. 11, 2019, 8:45 a.m. UTC | #4
Hi Laurent,

On 12.09.2019 15:30, Laurent Vivier wrote:
> add_early_randomness() is called every time a new rng backend is added
> and every time it is set as the current rng provider.
>
> add_early_randomness() is called from functions locking rng_mutex,
> and if it hangs all the hw_random framework hangs: we can't read sysfs,
> add or remove a backend.
>
> This patch move add_early_randomness() out of the rng_mutex zone.
> It only needs the reading_mutex.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

This patch landed in today's linux-next and causes the following warning 
on ARM 32bit Exynos5420-based Chromebook Peach-Pit board:

tpm_i2c_infineon 9-0020: 1.2 TPM (device-id 0x1A)
------------[ cut here ]------------
WARNING: CPU: 3 PID: 1 at lib/refcount.c:156 hwrng_register+0x13c/0x1b4
refcount_t: increment on 0; use-after-free.
Modules linked in:
CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.4.0-rc1-00061-gdaae28debcb0 
#6714
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[<c01124c8>] (unwind_backtrace) from [<c010dfb8>] (show_stack+0x10/0x14)
[<c010dfb8>] (show_stack) from [<c0ae86d8>] (dump_stack+0xa8/0xd4)
[<c0ae86d8>] (dump_stack) from [<c0127428>] (__warn+0xf4/0x10c)
[<c0127428>] (__warn) from [<c01274b4>] (warn_slowpath_fmt+0x74/0xb8)
[<c01274b4>] (warn_slowpath_fmt) from [<c054729c>] 
(hwrng_register+0x13c/0x1b4)
[<c054729c>] (hwrng_register) from [<c0547e54>] 
(tpm_chip_register+0xc4/0x274)
[<c0547e54>] (tpm_chip_register) from [<c055028c>] 
(tpm_tis_i2c_probe+0x138/0x1a0)
[<c055028c>] (tpm_tis_i2c_probe) from [<c07324b0>] 
(i2c_device_probe+0x230/0x2a4)
[<c07324b0>] (i2c_device_probe) from [<c05c1884>] (really_probe+0x1c4/0x48c)
[<c05c1884>] (really_probe) from [<c05c1d20>] 
(driver_probe_device+0x78/0x1f8)
[<c05c1d20>] (driver_probe_device) from [<c05bf7cc>] 
(bus_for_each_drv+0x74/0xb8)
[<c05bf7cc>] (bus_for_each_drv) from [<c05c1620>] 
(__device_attach+0xd4/0x16c)
[<c05c1620>] (__device_attach) from [<c05c0790>] 
(bus_probe_device+0x88/0x90)
[<c05c0790>] (bus_probe_device) from [<c05bdff8>] (device_add+0x478/0x62c)
[<c05bdff8>] (device_add) from [<c0734928>] 
(i2c_new_client_device+0x158/0x278)
[<c0734928>] (i2c_new_client_device) from [<c0734a50>] 
(i2c_new_device+0x8/0x14)
[<c0734a50>] (i2c_new_device) from [<c0738014>] 
(of_i2c_register_devices+0xf4/0x16c)
[<c0738014>] (of_i2c_register_devices) from [<c0734f34>] 
(i2c_register_adapter+0x180/0x458)
[<c0734f34>] (i2c_register_adapter) from [<c073b6c0>] 
(exynos5_i2c_probe+0x22c/0x28c)
[<c073b6c0>] (exynos5_i2c_probe) from [<c05c410c>] 
(platform_drv_probe+0x6c/0xa4)
[<c05c410c>] (platform_drv_probe) from [<c05c1884>] 
(really_probe+0x1c4/0x48c)
[<c05c1884>] (really_probe) from [<c05c1d20>] 
(driver_probe_device+0x78/0x1f8)
[<c05c1d20>] (driver_probe_device) from [<c05c2104>] 
(device_driver_attach+0x58/0x60)
[<c05c2104>] (device_driver_attach) from [<c05c21e8>] 
(__driver_attach+0xdc/0x174)
[<c05c21e8>] (__driver_attach) from [<c05bf6f8>] 
(bus_for_each_dev+0x68/0xb4)
[<c05bf6f8>] (bus_for_each_dev) from [<c05c0a2c>] 
(bus_add_driver+0x158/0x214)
[<c05c0a2c>] (bus_add_driver) from [<c05c30bc>] (driver_register+0x78/0x110)
[<c05c30bc>] (driver_register) from [<c0103214>] 
(do_one_initcall+0x8c/0x424)
[<c0103214>] (do_one_initcall) from [<c1001080>] 
(kernel_init_freeable+0x158/0x200)
[<c1001080>] (kernel_init_freeable) from [<c0b036a8>] 
(kernel_init+0x8/0x114)
[<c0b036a8>] (kernel_init) from [<c01010b4>] (ret_from_fork+0x14/0x20)
Exception stack(0xe88e1fb0 to 0xe88e1ff8)
1fa0:                                     00000000 00000000 00000000 
00000000
1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 
00000000
1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
irq event stamp: 296027
hardirqs last  enabled at (296157): [<c0b0bce8>] 
_raw_spin_unlock_irq+0x24/0x5c
hardirqs last disabled at (296180): [<c0b04030>] __schedule+0xd8/0x7b8
softirqs last  enabled at (296176): [<c01026bc>] __do_softirq+0x4fc/0x5fc
softirqs last disabled at (296197): [<c012fff4>] irq_exit+0x16c/0x170
---[ end trace d219e40773096999 ]---

If you need any help testing a fix for this issue, let me know.


> ---
>   drivers/char/hw_random/core.c | 60 +++++++++++++++++++++++++----------
>   1 file changed, 44 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 9044d31ab1a1..745ace6fffd7 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -111,6 +111,14 @@ static void drop_current_rng(void)
>   }
>   
>   /* Returns ERR_PTR(), NULL or refcounted hwrng */
> +static struct hwrng *get_current_rng_nolock(void)
> +{
> +	if (current_rng)
> +		kref_get(&current_rng->ref);
> +
> +	return current_rng;
> +}
> +
>   static struct hwrng *get_current_rng(void)
>   {
>   	struct hwrng *rng;
> @@ -118,9 +126,7 @@ static struct hwrng *get_current_rng(void)
>   	if (mutex_lock_interruptible(&rng_mutex))
>   		return ERR_PTR(-ERESTARTSYS);
>   
> -	rng = current_rng;
> -	if (rng)
> -		kref_get(&rng->ref);
> +	rng = get_current_rng_nolock();
>   
>   	mutex_unlock(&rng_mutex);
>   	return rng;
> @@ -155,8 +161,6 @@ static int hwrng_init(struct hwrng *rng)
>   	reinit_completion(&rng->cleanup_done);
>   
>   skip_init:
> -	add_early_randomness(rng);
> -
>   	current_quality = rng->quality ? : default_quality;
>   	if (current_quality > 1024)
>   		current_quality = 1024;
> @@ -320,12 +324,13 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
>   					const char *buf, size_t len)
>   {
>   	int err = -ENODEV;
> -	struct hwrng *rng;
> +	struct hwrng *rng, *old_rng, *new_rng;
>   
>   	err = mutex_lock_interruptible(&rng_mutex);
>   	if (err)
>   		return -ERESTARTSYS;
>   
> +	old_rng = current_rng;
>   	if (sysfs_streq(buf, "")) {
>   		err = enable_best_rng();
>   	} else {
> @@ -337,9 +342,15 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
>   			}
>   		}
>   	}
> -
> +	new_rng = get_current_rng_nolock();
>   	mutex_unlock(&rng_mutex);
>   
> +	if (new_rng) {
> +		if (new_rng != old_rng)
> +			add_early_randomness(new_rng);
> +		put_rng(new_rng);
> +	}
> +
>   	return err ? : len;
>   }
>   
> @@ -457,13 +468,17 @@ static void start_khwrngd(void)
>   int hwrng_register(struct hwrng *rng)
>   {
>   	int err = -EINVAL;
> -	struct hwrng *old_rng, *tmp;
> +	struct hwrng *old_rng, *new_rng, *tmp;
>   	struct list_head *rng_list_ptr;
>   
>   	if (!rng->name || (!rng->data_read && !rng->read))
>   		goto out;
>   
>   	mutex_lock(&rng_mutex);
> +
> +	old_rng = current_rng;
> +	new_rng = NULL;
> +
>   	/* Must not register two RNGs with the same name. */
>   	err = -EEXIST;
>   	list_for_each_entry(tmp, &rng_list, list) {
> @@ -482,7 +497,6 @@ int hwrng_register(struct hwrng *rng)
>   	}
>   	list_add_tail(&rng->list, rng_list_ptr);
>   
> -	old_rng = current_rng;
>   	err = 0;
>   	if (!old_rng ||
>   	    (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
> @@ -496,19 +510,24 @@ int hwrng_register(struct hwrng *rng)
>   			goto out_unlock;
>   	}
>   
> -	if (old_rng && !rng->init) {
> +	new_rng = rng;
> +	kref_get(&new_rng->ref);
> +out_unlock:
> +	mutex_unlock(&rng_mutex);
> +
> +	if (new_rng) {
> +		if (new_rng != old_rng || !rng->init) {
>   		/*
>   		 * Use a new device's input to add some randomness to
>   		 * the system.  If this rng device isn't going to be
>   		 * used right away, its init function hasn't been
> -		 * called yet; so only use the randomness from devices
> -		 * that don't need an init callback.
> +		 * called yet by set_current_rng(); so only use the
> +		 * randomness from devices that don't need an init callback
>   		 */
> -		add_early_randomness(rng);
> +			add_early_randomness(new_rng);
> +		}
> +		put_rng(new_rng);
>   	}
> -
> -out_unlock:
> -	mutex_unlock(&rng_mutex);
>   out:
>   	return err;
>   }
> @@ -516,10 +535,12 @@ EXPORT_SYMBOL_GPL(hwrng_register);
>   
>   void hwrng_unregister(struct hwrng *rng)
>   {
> +	struct hwrng *old_rng, *new_rng;
>   	int err;
>   
>   	mutex_lock(&rng_mutex);
>   
> +	old_rng = current_rng;
>   	list_del(&rng->list);
>   	if (current_rng == rng) {
>   		err = enable_best_rng();
> @@ -529,6 +550,7 @@ void hwrng_unregister(struct hwrng *rng)
>   		}
>   	}
>   
> +	new_rng = get_current_rng_nolock();
>   	if (list_empty(&rng_list)) {
>   		mutex_unlock(&rng_mutex);
>   		if (hwrng_fill)
> @@ -536,6 +558,12 @@ void hwrng_unregister(struct hwrng *rng)
>   	} else
>   		mutex_unlock(&rng_mutex);
>   
> +	if (new_rng) {
> +		if (old_rng != new_rng)
> +			add_early_randomness(new_rng);
> +		put_rng(new_rng);
> +	}
> +
>   	wait_for_completion(&rng->cleanup_done);
>   }
>   EXPORT_SYMBOL_GPL(hwrng_unregister);

Best regards
Laurent Vivier Oct. 11, 2019, 11 a.m. UTC | #5
On 11/10/2019 10:45, Marek Szyprowski wrote:
> Hi Laurent,
> 
> On 12.09.2019 15:30, Laurent Vivier wrote:
>> add_early_randomness() is called every time a new rng backend is added
>> and every time it is set as the current rng provider.
>>
>> add_early_randomness() is called from functions locking rng_mutex,
>> and if it hangs all the hw_random framework hangs: we can't read sysfs,
>> add or remove a backend.
>>
>> This patch move add_early_randomness() out of the rng_mutex zone.
>> It only needs the reading_mutex.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> This patch landed in today's linux-next and causes the following warning 
> on ARM 32bit Exynos5420-based Chromebook Peach-Pit board:
> 
> tpm_i2c_infineon 9-0020: 1.2 TPM (device-id 0x1A)
> ------------[ cut here ]------------
> WARNING: CPU: 3 PID: 1 at lib/refcount.c:156 hwrng_register+0x13c/0x1b4
> refcount_t: increment on 0; use-after-free.
> Modules linked in:
> CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.4.0-rc1-00061-gdaae28debcb0 
> #6714
> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [<c01124c8>] (unwind_backtrace) from [<c010dfb8>] (show_stack+0x10/0x14)
> [<c010dfb8>] (show_stack) from [<c0ae86d8>] (dump_stack+0xa8/0xd4)
> [<c0ae86d8>] (dump_stack) from [<c0127428>] (__warn+0xf4/0x10c)
> [<c0127428>] (__warn) from [<c01274b4>] (warn_slowpath_fmt+0x74/0xb8)
> [<c01274b4>] (warn_slowpath_fmt) from [<c054729c>] 
> (hwrng_register+0x13c/0x1b4)

This can happen if hwrng_init() has not been called for rng, that is
called by set_current_rng().

It appears with this patch because I have introduced the kref_get()
before the call of add_early_randomness() when the rng device is new but
is not set as the current one. So add_early_randomness() was called on
an unitialized device (it was already the case before)

I wanted to take the ref before releasing the mutex to avoid race
condition, but if the new rng device is not the new current_rng one, we
don't need that as the ref is only used with current_rng device.

I'm going to rework this patch.

Thanks,
Laurent

Patch
diff mbox series

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 9044d31ab1a1..745ace6fffd7 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -111,6 +111,14 @@  static void drop_current_rng(void)
 }
 
 /* Returns ERR_PTR(), NULL or refcounted hwrng */
+static struct hwrng *get_current_rng_nolock(void)
+{
+	if (current_rng)
+		kref_get(&current_rng->ref);
+
+	return current_rng;
+}
+
 static struct hwrng *get_current_rng(void)
 {
 	struct hwrng *rng;
@@ -118,9 +126,7 @@  static struct hwrng *get_current_rng(void)
 	if (mutex_lock_interruptible(&rng_mutex))
 		return ERR_PTR(-ERESTARTSYS);
 
-	rng = current_rng;
-	if (rng)
-		kref_get(&rng->ref);
+	rng = get_current_rng_nolock();
 
 	mutex_unlock(&rng_mutex);
 	return rng;
@@ -155,8 +161,6 @@  static int hwrng_init(struct hwrng *rng)
 	reinit_completion(&rng->cleanup_done);
 
 skip_init:
-	add_early_randomness(rng);
-
 	current_quality = rng->quality ? : default_quality;
 	if (current_quality > 1024)
 		current_quality = 1024;
@@ -320,12 +324,13 @@  static ssize_t hwrng_attr_current_store(struct device *dev,
 					const char *buf, size_t len)
 {
 	int err = -ENODEV;
-	struct hwrng *rng;
+	struct hwrng *rng, *old_rng, *new_rng;
 
 	err = mutex_lock_interruptible(&rng_mutex);
 	if (err)
 		return -ERESTARTSYS;
 
+	old_rng = current_rng;
 	if (sysfs_streq(buf, "")) {
 		err = enable_best_rng();
 	} else {
@@ -337,9 +342,15 @@  static ssize_t hwrng_attr_current_store(struct device *dev,
 			}
 		}
 	}
-
+	new_rng = get_current_rng_nolock();
 	mutex_unlock(&rng_mutex);
 
+	if (new_rng) {
+		if (new_rng != old_rng)
+			add_early_randomness(new_rng);
+		put_rng(new_rng);
+	}
+
 	return err ? : len;
 }
 
@@ -457,13 +468,17 @@  static void start_khwrngd(void)
 int hwrng_register(struct hwrng *rng)
 {
 	int err = -EINVAL;
-	struct hwrng *old_rng, *tmp;
+	struct hwrng *old_rng, *new_rng, *tmp;
 	struct list_head *rng_list_ptr;
 
 	if (!rng->name || (!rng->data_read && !rng->read))
 		goto out;
 
 	mutex_lock(&rng_mutex);
+
+	old_rng = current_rng;
+	new_rng = NULL;
+
 	/* Must not register two RNGs with the same name. */
 	err = -EEXIST;
 	list_for_each_entry(tmp, &rng_list, list) {
@@ -482,7 +497,6 @@  int hwrng_register(struct hwrng *rng)
 	}
 	list_add_tail(&rng->list, rng_list_ptr);
 
-	old_rng = current_rng;
 	err = 0;
 	if (!old_rng ||
 	    (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
@@ -496,19 +510,24 @@  int hwrng_register(struct hwrng *rng)
 			goto out_unlock;
 	}
 
-	if (old_rng && !rng->init) {
+	new_rng = rng;
+	kref_get(&new_rng->ref);
+out_unlock:
+	mutex_unlock(&rng_mutex);
+
+	if (new_rng) {
+		if (new_rng != old_rng || !rng->init) {
 		/*
 		 * Use a new device's input to add some randomness to
 		 * the system.  If this rng device isn't going to be
 		 * used right away, its init function hasn't been
-		 * called yet; so only use the randomness from devices
-		 * that don't need an init callback.
+		 * called yet by set_current_rng(); so only use the
+		 * randomness from devices that don't need an init callback
 		 */
-		add_early_randomness(rng);
+			add_early_randomness(new_rng);
+		}
+		put_rng(new_rng);
 	}
-
-out_unlock:
-	mutex_unlock(&rng_mutex);
 out:
 	return err;
 }
@@ -516,10 +535,12 @@  EXPORT_SYMBOL_GPL(hwrng_register);
 
 void hwrng_unregister(struct hwrng *rng)
 {
+	struct hwrng *old_rng, *new_rng;
 	int err;
 
 	mutex_lock(&rng_mutex);
 
+	old_rng = current_rng;
 	list_del(&rng->list);
 	if (current_rng == rng) {
 		err = enable_best_rng();
@@ -529,6 +550,7 @@  void hwrng_unregister(struct hwrng *rng)
 		}
 	}
 
+	new_rng = get_current_rng_nolock();
 	if (list_empty(&rng_list)) {
 		mutex_unlock(&rng_mutex);
 		if (hwrng_fill)
@@ -536,6 +558,12 @@  void hwrng_unregister(struct hwrng *rng)
 	} else
 		mutex_unlock(&rng_mutex);
 
+	if (new_rng) {
+		if (old_rng != new_rng)
+			add_early_randomness(new_rng);
+		put_rng(new_rng);
+	}
+
 	wait_for_completion(&rng->cleanup_done);
 }
 EXPORT_SYMBOL_GPL(hwrng_unregister);