[PATCH 4.19 0/2] fix divide zero error in padata_do_parallel()

[PATCH 4.19 0/2] fix divide zero error in padata_do_parallel()

[Yang Yingliang]

It can reproduced by the following commands:

  # modprobe pcrypt
  # echo 2 > /sys/kernel/pcrypt/pencrypt/parallel_cpumask
  # echo 0 > /sys/devices/system/cpu/cpu1/online
  # modprobe tcrypt alg="pcrypt(rfc4106(gcm(aes)))" type=3

[  229.549005] divide error: 0000 [#1] SMP PTI
[  229.549130] CPU: 32 PID: 9565 Comm: cryptomgr_test Kdump: loaded Not tainted 4.19.200 #3
[  229.549381] Hardware name: Huawei 2288H V5/BC11SPSCB0, BIOS 0.68 05/03/2018
[  229.549607] RIP: 0010:padata_do_parallel+0x96/0x150
[  229.549750] Code: 5e 10 89 56 18 f0 0f c1 6b 20 8b 35 78 b1 24 01 48 8b 7b 28 e8 eb d6 20 00 89 c1 8d 45 01 31 d2 8b 35 62 b1 24 01 48 8b 7b 28 <f7> f1 41 89 d7 e8 d0 45 21 00 45 85 ff 41 89 c4 7e 19 31 ed 48 8b
[  229.550335] RSP: 0018:ffffa48b8e1cbbc8 EFLAGS: 00010246
[  229.550498] RAX: 0000000000000000 RBX: ffff964940883bc0 RCX: 0000000000000000
[  229.550720] RDX: 0000000000000000 RSI: 0000000000000038 RDI: ffff9687b6e45650
[  229.550943] RBP: 00000000ffffffff R08: 0000000000000000 R09: ffff96c7af6f6200
[  229.551165] R10: ffff9687b6e45650 R11: ffff968839629000 R12: 0000000000000010
[  229.551388] R13: ffff9687b6997f50 R14: ffff96c7ad84d700 R15: ffffffff9287a220
[  229.551610] FS:  0000000000000000(0000) GS:ffff9687bfc80000(0000) knlGS:0000000000000000
[  229.551863] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  229.552044] CR2: 00007fad13d47564 CR3: 000000759480a004 CR4: 00000000007606e0
[  229.552265] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  229.552488] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  229.552710] PKRU: 55555554
[  229.552796] Call Trace:
[  229.552878]  pcrypt_aead_encrypt+0xbb/0xc7 [pcrypt]
[  229.553028]  __test_aead+0x654/0x15d0
[  229.553139]  ? _cond_resched+0x15/0x40
[  229.553257]  ? crypto_create_tfm+0x4e/0xe0
[  229.553385]  ? crypto_spawn_tfm2+0x2e/0x50
[  229.553513]  ? _cond_resched+0x15/0x40
[  229.553632]  ? crypto_acomp_scomp_free_ctx+0x30/0x30
[  229.553786]  test_aead+0x21/0xa0
[  229.553889]  alg_test_aead+0x3f/0xa0
[  229.554001]  alg_test.part.15+0x178/0x380
[  229.554127]  ? __switch_to+0x8c/0x400
[  229.554239]  ? __switch_to_asm+0x41/0x70
[  229.554362]  ? __switch_to_asm+0x35/0x70
[  229.554486]  ? __schedule+0x25d/0x850
[  229.554602]  ? __wake_up_common+0x76/0x170
[  229.554727]  ? crypto_acomp_scomp_free_ctx+0x30/0x30
[  229.554884]  cryptomgr_test+0x40/0x50
[  229.554999]  kthread+0x113/0x130
[  229.555099]  ? kthread_create_worker_on_cpu+0x70/0x70
[  229.555255]  ret_from_fork+0x35/0x40


Daniel Jordan (2):
  padata: validate cpumask without removed CPU during offline
  padata: add separate cpuhp node for CPUHP_PADATA_DEAD

 include/linux/cpuhotplug.h |  1 +
 include/linux/padata.h     |  6 ++++--
 kernel/padata.c            | 28 ++++++++++++++++++++--------
 3 files changed, 25 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH 4.19 1/2] padata: validate cpumask without removed CPU during offline

[Yang Yingliang]

From: Daniel Jordan <daniel.m.jordan@oracle.com>

[ Upstream commit 894c9ef9780c5cf2f143415e867ee39a33ecb75d ]

Configuring an instance's parallel mask without any online CPUs...

  echo 2 > /sys/kernel/pcrypt/pencrypt/parallel_cpumask
  echo 0 > /sys/devices/system/cpu/cpu1/online

...makes tcrypt mode=215 crash like this:

  divide error: 0000 [#1] SMP PTI
  CPU: 4 PID: 283 Comm: modprobe Not tainted 5.4.0-rc8-padata-doc-v2+ #2
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191013_105130-anatol 04/01/2014
  RIP: 0010:padata_do_parallel+0x114/0x300
  Call Trace:
   pcrypt_aead_encrypt+0xc0/0xd0 [pcrypt]
   crypto_aead_encrypt+0x1f/0x30
   do_mult_aead_op+0x4e/0xdf [tcrypt]
   test_mb_aead_speed.constprop.0.cold+0x226/0x564 [tcrypt]
   do_test+0x28c2/0x4d49 [tcrypt]
   tcrypt_mod_init+0x55/0x1000 [tcrypt]
   ...

cpumask_weight() in padata_cpu_hash() returns 0 because the mask has no
CPUs.  The problem is __padata_remove_cpu() checks for valid masks too
early and so doesn't mark the instance PADATA_INVALID as expected, which
would have made padata_do_parallel() return error before doing the
division.

Fix by introducing a second padata CPU hotplug state before
CPUHP_BRINGUP_CPU so that __padata_remove_cpu() sees the online mask
without @cpu.  No need for the second argument to padata_replace() since
@cpu is now already missing from the online mask.

Fixes: 33e54450683c ("padata: Handle empty padata cpumasks")
Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Eric Biggers <ebiggers@kernel.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 include/linux/cpuhotplug.h |  1 +
 kernel/padata.c            | 18 ++++++++++++++----
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 3d323c6c85260..b51da879d7be0 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -59,6 +59,7 @@ enum cpuhp_state {
 	CPUHP_IOMMU_INTEL_DEAD,
 	CPUHP_LUSTRE_CFS_DEAD,
 	CPUHP_AP_ARM_CACHE_B15_RAC_DEAD,
+	CPUHP_PADATA_DEAD,
 	CPUHP_WORKQUEUE_PREP,
 	CPUHP_POWER_NUMA_PREPARE,
 	CPUHP_HRTIMERS_PREPARE,
diff --git a/kernel/padata.c b/kernel/padata.c
index 93e4fb2d9f2ee..4401b4f13d0be 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -682,7 +682,7 @@ static int __padata_remove_cpu(struct padata_instance *pinst, int cpu)
 {
 	struct parallel_data *pd = NULL;
 
-	if (cpumask_test_cpu(cpu, cpu_online_mask)) {
+	if (!cpumask_test_cpu(cpu, cpu_online_mask)) {
 
 		if (!padata_validate_cpumask(pinst, pinst->cpumask.pcpu) ||
 		    !padata_validate_cpumask(pinst, pinst->cpumask.cbcpu))
@@ -758,7 +758,7 @@ static int padata_cpu_online(unsigned int cpu, struct hlist_node *node)
 	return ret;
 }
 
-static int padata_cpu_prep_down(unsigned int cpu, struct hlist_node *node)
+static int padata_cpu_dead(unsigned int cpu, struct hlist_node *node)
 {
 	struct padata_instance *pinst;
 	int ret;
@@ -779,6 +779,7 @@ static enum cpuhp_state hp_online;
 static void __padata_free(struct padata_instance *pinst)
 {
 #ifdef CONFIG_HOTPLUG_CPU
+	cpuhp_state_remove_instance_nocalls(CPUHP_PADATA_DEAD, &pinst->node);
 	cpuhp_state_remove_instance_nocalls(hp_online, &pinst->node);
 #endif
 
@@ -964,6 +965,8 @@ static struct padata_instance *padata_alloc(struct workqueue_struct *wq,
 
 #ifdef CONFIG_HOTPLUG_CPU
 	cpuhp_state_add_instance_nocalls_cpuslocked(hp_online, &pinst->node);
+	cpuhp_state_add_instance_nocalls_cpuslocked(CPUHP_PADATA_DEAD,
+						    &pinst->node);
 #endif
 	return pinst;
 
@@ -1010,17 +1013,24 @@ static __init int padata_driver_init(void)
 	int ret;
 
 	ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, "padata:online",
-				      padata_cpu_online,
-				      padata_cpu_prep_down);
+				      padata_cpu_online, NULL);
 	if (ret < 0)
 		return ret;
 	hp_online = ret;
+
+	ret = cpuhp_setup_state_multi(CPUHP_PADATA_DEAD, "padata:dead",
+				      NULL, padata_cpu_dead);
+	if (ret < 0) {
+		cpuhp_remove_multi_state(hp_online);
+		return ret;
+	}
 	return 0;
 }
 module_init(padata_driver_init);
 
 static __exit void padata_driver_exit(void)
 {
+	cpuhp_remove_multi_state(CPUHP_PADATA_DEAD);
 	cpuhp_remove_multi_state(hp_online);
 }
 module_exit(padata_driver_exit);
-- 
2.25.1

[PATCH 4.19 2/2] padata: add separate cpuhp node for CPUHP_PADATA_DEAD

[Yang Yingliang]

From: Daniel Jordan <daniel.m.jordan@oracle.com>

[ Upstream commit 3c2214b6027ff37945799de717c417212e1a8c54 ]

Removing the pcrypt module triggers this:

  general protection fault, probably for non-canonical
    address 0xdead000000000122
  CPU: 5 PID: 264 Comm: modprobe Not tainted 5.6.0+ #2
  Hardware name: QEMU Standard PC
  RIP: 0010:__cpuhp_state_remove_instance+0xcc/0x120
  Call Trace:
   padata_sysfs_release+0x74/0xce
   kobject_put+0x81/0xd0
   padata_free+0x12/0x20
   pcrypt_exit+0x43/0x8ee [pcrypt]

padata instances wrongly use the same hlist node for the online and dead
states, so __padata_free()'s second cpuhp remove call chokes on the node
that the first poisoned.

cpuhp multi-instance callbacks only walk forward in cpuhp_step->list and
the same node is linked in both the online and dead lists, so the list
corruption that results from padata_alloc() adding the node to a second
list without removing it from the first doesn't cause problems as long
as no instances are freed.

Avoid the issue by giving each state its own node.

Fixes: 894c9ef9780c ("padata: validate cpumask without removed CPU during offline")
Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: stable@vger.kernel.org # v5.4+
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 include/linux/padata.h |  6 ++++--
 kernel/padata.c        | 14 ++++++++------
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/include/linux/padata.h b/include/linux/padata.h
index d803397a28f70..8c9827cc63747 100644
--- a/include/linux/padata.h
+++ b/include/linux/padata.h
@@ -138,7 +138,8 @@ struct parallel_data {
 /**
  * struct padata_instance - The overall control structure.
  *
- * @cpu_notifier: cpu hotplug notifier.
+ * @cpu_online_node: Linkage for CPU online callback.
+ * @cpu_dead_node: Linkage for CPU offline callback.
  * @wq: The workqueue in use.
  * @pd: The internal control structure.
  * @cpumask: User supplied cpumasks for parallel and serial works.
@@ -150,7 +151,8 @@ struct parallel_data {
  * @flags: padata flags.
  */
 struct padata_instance {
-	struct hlist_node		 node;
+	struct hlist_node		cpu_online_node;
+	struct hlist_node		cpu_dead_node;
 	struct workqueue_struct		*wq;
 	struct parallel_data		*pd;
 	struct padata_cpumask		cpumask;
diff --git a/kernel/padata.c b/kernel/padata.c
index 4401b4f13d0be..7f2b6d369fd47 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -748,7 +748,7 @@ static int padata_cpu_online(unsigned int cpu, struct hlist_node *node)
 	struct padata_instance *pinst;
 	int ret;
 
-	pinst = hlist_entry_safe(node, struct padata_instance, node);
+	pinst = hlist_entry_safe(node, struct padata_instance, cpu_online_node);
 	if (!pinst_has_cpu(pinst, cpu))
 		return 0;
 
@@ -763,7 +763,7 @@ static int padata_cpu_dead(unsigned int cpu, struct hlist_node *node)
 	struct padata_instance *pinst;
 	int ret;
 
-	pinst = hlist_entry_safe(node, struct padata_instance, node);
+	pinst = hlist_entry_safe(node, struct padata_instance, cpu_dead_node);
 	if (!pinst_has_cpu(pinst, cpu))
 		return 0;
 
@@ -779,8 +779,9 @@ static enum cpuhp_state hp_online;
 static void __padata_free(struct padata_instance *pinst)
 {
 #ifdef CONFIG_HOTPLUG_CPU
-	cpuhp_state_remove_instance_nocalls(CPUHP_PADATA_DEAD, &pinst->node);
-	cpuhp_state_remove_instance_nocalls(hp_online, &pinst->node);
+	cpuhp_state_remove_instance_nocalls(CPUHP_PADATA_DEAD,
+					    &pinst->cpu_dead_node);
+	cpuhp_state_remove_instance_nocalls(hp_online, &pinst->cpu_online_node);
 #endif
 
 	padata_stop(pinst);
@@ -964,9 +965,10 @@ static struct padata_instance *padata_alloc(struct workqueue_struct *wq,
 	mutex_init(&pinst->lock);
 
 #ifdef CONFIG_HOTPLUG_CPU
-	cpuhp_state_add_instance_nocalls_cpuslocked(hp_online, &pinst->node);
+	cpuhp_state_add_instance_nocalls_cpuslocked(hp_online,
+						    &pinst->cpu_online_node);
 	cpuhp_state_add_instance_nocalls_cpuslocked(CPUHP_PADATA_DEAD,
-						    &pinst->node);
+						    &pinst->cpu_dead_node);
 #endif
 	return pinst;
 
-- 
2.25.1

Re: [PATCH 4.19 0/2] fix divide zero error in padata_do_parallel()

[Greg KH]

On Tue, Aug 03, 2021 at 08:52:59PM +0800, Yang Yingliang wrote:
> It can reproduced by the following commands:
> 
>   # modprobe pcrypt
>   # echo 2 > /sys/kernel/pcrypt/pencrypt/parallel_cpumask
>   # echo 0 > /sys/devices/system/cpu/cpu1/online
>   # modprobe tcrypt alg="pcrypt(rfc4106(gcm(aes)))" type=3
> 
> [  229.549005] divide error: 0000 [#1] SMP PTI
> [  229.549130] CPU: 32 PID: 9565 Comm: cryptomgr_test Kdump: loaded Not tainted 4.19.200 #3
> [  229.549381] Hardware name: Huawei 2288H V5/BC11SPSCB0, BIOS 0.68 05/03/2018
> [  229.549607] RIP: 0010:padata_do_parallel+0x96/0x150
> [  229.549750] Code: 5e 10 89 56 18 f0 0f c1 6b 20 8b 35 78 b1 24 01 48 8b 7b 28 e8 eb d6 20 00 89 c1 8d 45 01 31 d2 8b 35 62 b1 24 01 48 8b 7b 28 <f7> f1 41 89 d7 e8 d0 45 21 00 45 85 ff 41 89 c4 7e 19 31 ed 48 8b
> [  229.550335] RSP: 0018:ffffa48b8e1cbbc8 EFLAGS: 00010246
> [  229.550498] RAX: 0000000000000000 RBX: ffff964940883bc0 RCX: 0000000000000000
> [  229.550720] RDX: 0000000000000000 RSI: 0000000000000038 RDI: ffff9687b6e45650
> [  229.550943] RBP: 00000000ffffffff R08: 0000000000000000 R09: ffff96c7af6f6200
> [  229.551165] R10: ffff9687b6e45650 R11: ffff968839629000 R12: 0000000000000010
> [  229.551388] R13: ffff9687b6997f50 R14: ffff96c7ad84d700 R15: ffffffff9287a220
> [  229.551610] FS:  0000000000000000(0000) GS:ffff9687bfc80000(0000) knlGS:0000000000000000
> [  229.551863] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  229.552044] CR2: 00007fad13d47564 CR3: 000000759480a004 CR4: 00000000007606e0
> [  229.552265] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [  229.552488] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [  229.552710] PKRU: 55555554
> [  229.552796] Call Trace:
> [  229.552878]  pcrypt_aead_encrypt+0xbb/0xc7 [pcrypt]
> [  229.553028]  __test_aead+0x654/0x15d0
> [  229.553139]  ? _cond_resched+0x15/0x40
> [  229.553257]  ? crypto_create_tfm+0x4e/0xe0
> [  229.553385]  ? crypto_spawn_tfm2+0x2e/0x50
> [  229.553513]  ? _cond_resched+0x15/0x40
> [  229.553632]  ? crypto_acomp_scomp_free_ctx+0x30/0x30
> [  229.553786]  test_aead+0x21/0xa0
> [  229.553889]  alg_test_aead+0x3f/0xa0
> [  229.554001]  alg_test.part.15+0x178/0x380
> [  229.554127]  ? __switch_to+0x8c/0x400
> [  229.554239]  ? __switch_to_asm+0x41/0x70
> [  229.554362]  ? __switch_to_asm+0x35/0x70
> [  229.554486]  ? __schedule+0x25d/0x850
> [  229.554602]  ? __wake_up_common+0x76/0x170
> [  229.554727]  ? crypto_acomp_scomp_free_ctx+0x30/0x30
> [  229.554884]  cryptomgr_test+0x40/0x50
> [  229.554999]  kthread+0x113/0x130
> [  229.555099]  ? kthread_create_worker_on_cpu+0x70/0x70
> [  229.555255]  ret_from_fork+0x35/0x40
> 
> 
> Daniel Jordan (2):
>   padata: validate cpumask without removed CPU during offline
>   padata: add separate cpuhp node for CPUHP_PADATA_DEAD
> 
>  include/linux/cpuhotplug.h |  1 +
>  include/linux/padata.h     |  6 ++++--
>  kernel/padata.c            | 28 ++++++++++++++++++++--------
>  3 files changed, 25 insertions(+), 10 deletions(-)

All now queued up, thanks!

greg k-h