crypto: ccp - Fix use-after-free in backlog cmd advancement
PATCH v2 1/1 crypto: ccp - Fix use-after-free in backlog cmd advancement
A CCP_CMD_MAY_BACKLOG command promoted by a queue kthread leaves the backlog list and is re-queued only later, by a work running on the system workqueue. Until that work runs, the command is on neither list, so the flush in ccp*_destroy() cannot reach it: the work then uses the ccp_device after it has been released and wakes a queue kthread whose task_struct kthread_stop() has already freed.
The gap between the promotion and the requeue also lets concurrent submissions claim the space meant for the backlogged command, so it is not actually reserved and cmd_count can grow past MAX_CMD_QLEN.
Promote the backlogged command in ccp_dequeue_cmd() itself: the slot freed by the dequeue is transferred to the backlog head under the same cmd_lock, so the space is reserved atomically, and the command is put on the normal queue only after its advancement callback (-EINPROGRESS) has run from the queue kthread, which provides process context and keeps the backlog completion ahead of execution. The work struct is no longer needed and the unused member is removed from ccp_cmd.
Mark the device halting before stopping its queues and suppress the promotion wake under cmd_lock once teardown starts, so it cannot wake a queue kthread that teardown has already stopped.
This issue was found by an in-house static analysis tool.
---
Link to v1: https://lore.kernel.org/linux-crypto/[email protected]/
- transfer the slot freed by the dequeue to the backlogged command under cmd_lock, as suggested by Herbert Xu, and drop the work struct and the per-device workqueue of v1.
- mark the device halting before stopping its queues and suppress the promotion wake under cmd_lock once teardown starts.
drivers/crypto/ccp/ccp-dev-v3.c | 3 ++ drivers/crypto/ccp/ccp-dev-v5.c | 3 ++ drivers/crypto/ccp/ccp-dev.c | 88 +++++++++++++++++++-------------- drivers/crypto/ccp/ccp-dev.h | 3 ++ include/linux/ccp.h | 5 +- 5 files changed, 62 insertions(+), 40 deletions(-)
drivers/crypto/ccp/ccp-dev-v3.c +3 -0
@@ -501,6 +501,8 @@ static int ccp_init(struct ccp_device *ccp)ccp_unregister_rng (ccp); e_kthread: +ccp_halt_cmds (ccp); +for (i =0 ; i < ccp->cmd_q_count; i++)if (ccp->cmd_q[i].kthread)kthread_stop (ccp->cmd_q[i].kthread);
@@ -542,6 +544,7 @@ static void ccp_destroy(struct ccp_device *ccp)iowrite32 (ccp->qim, ccp->io_regs +IRQ_STATUS_REG );/* Stop the queue kthreads */ +ccp_halt_cmds (ccp);for (i =0 ; i < ccp->cmd_q_count; i++)if (ccp->cmd_q[i].kthread)kthread_stop (ccp->cmd_q[i].kthread);
drivers/crypto/ccp/ccp-dev-v5.c +3 -0
@@ -989,6 +989,8 @@ static int ccp5_init(struct ccp_device *ccp)ccp_unregister_rng (ccp); e_kthread: +ccp_halt_cmds (ccp); +for (i =0 ; i < ccp->cmd_q_count; i++)if (ccp->cmd_q[i].kthread)kthread_stop (ccp->cmd_q[i].kthread);
@@ -1043,6 +1045,7 @@ static void ccp5_destroy(struct ccp_device *ccp) }/* Stop the queue kthreads */ +ccp_halt_cmds (ccp);for (i =0 ; i < ccp->cmd_q_count; i++)if (ccp->cmd_q[i].kthread)kthread_stop (ccp->cmd_q[i].kthread);
drivers/crypto/ccp/ccp-dev.c +52 -36
125 lines not on this page · raw message
drivers/crypto/ccp/ccp-dev.h +3 -0
@@ -374,6 +374,8 @@ struct ccp_device {struct list_head cmd;struct list_head backlog; +bool halting; +/* The command queues. These represent the queues available on the * CCP that are available for processing cmds */
@@ -630,6 +632,7 @@ struct ccp5_desc {void ccp_add_device (struct ccp_device *ccp);void ccp_del_device (struct ccp_device *ccp); +void ccp_halt_cmds (struct ccp_device *ccp);extern void ccp_log_error (struct ccp_device *,unsigned int );
include/linux/ccp.h +1 -4
@@ -12,7 +12,6 @@#define __CCP_H__#include <linux/scatterlist.h> -#include <linux/workqueue.h> #include <linux/list.h> #include <crypto/aes.h> #include <crypto/sha1.h>
@@ -637,7 +636,6 @@ enum ccp_engine {/** * struct ccp_cmd - CCP operation request * @entry: list element (ccp driver use only) -* @work: work element used for callbacks (ccp driver use only) * @ccp: CCP device to be run on * @ret: operation return code (ccp driver use only) * @flags: cmd processing flags
@@ -653,11 +651,10 @@ enum ccp_engine {* operation. */ struct ccp_cmd { -/* The list_head, work_struct, ccp and ret variables are for use +/* The list_head, ccp and ret variables are for use * by the CCP driver only. */ struct list_head entry; -struct work_struct work;struct ccp_device *ccp;int ret;
…
Quoted 4 lines
+++
b/drivers/crypto/ccp/ccp-dev.cdrivers/crypto/ccp/ccp-dev.c +0
-0@@ -177,6 +177,15 @@ void ccp_del_device(struct ccp_device *ccp)write_unlock_irqrestore (&ccp_unit_lock, flags); }
drivers/crypto/ccp/ccp-dev.c void ccp_del_device(struct ccp_device *ccp) · line 188
Quoted 10 lines
drivers/crypto/ccp/ccp-dev.c +9
-0+/* Prevent promotion from waking an idle queue during teardown. */ +void ccp_halt_cmds (struct ccp_device *ccp) +{ +unsigned long flags; + +spin_lock_irqsave (&ccp->cmd_lock, flags); + ccp->halting = true; +spin_unlock_irqrestore (&ccp->cmd_lock, flags); +}
…
May lock guards be applied in affected function implementations?
https://elixir.bootlin.com/linux/v7.3-rc5/source/include/linux/spinlock.h#L642-L645
Regards,
Markus
Quoted 2 lines · Markus Elfring
May lock guards be applied in affected function implementations?
https://elixir.bootlin.com/linux/v7.3-rc5/source/include/linux/spinlock.h#L642-L645
I considered using lock guards, but kept the manual locking here.
ccp_dequeue_cmd() deliberately has two separate cmd_lock sections, with the -EINPROGRESS callback between them. The callback must run outside cmd_lock, so the explicit unlock makes that boundary clear.
This is also a stable fix for code dating back to 2013. Stable branches such as v6.1 do not have the cleanup/guard helpers, while the manual form backports unchanged.
I can use a guard for the one-statement ccp_halt_cmds() helper if you prefer, but would keep the two promotion sections explicit.
Thanks,
Fan Wu
Herbert Xu
maintainerQuoted 26 lines · Fan Wu
drivers/crypto/ccp/ccp-dev.c +11
-5@@ -391,26 +372,61 @@ static struct ccp_cmd *ccp_dequeue_cmd(struct ccp_cmd_queue *cmd_q)return NULL ; } -if (ccp->cmd_count) { +if (!list_empty (&ccp->cmd)) { cmd_q->active =1 ; cmd = list_first_entry(&ccp->cmd, struct ccp_cmd, entry);list_del (&cmd->entry); - ccp->cmd_count--; - } - -if (!list_empty (&ccp->backlog)) { +if (!list_empty (&ccp->backlog)) { +/* Transfer the freed slot to the backlogged +* command, so that concurrent submissions cannot +* claim the space meant for it. +*/ + backlog =list_first_entry (&ccp->backlog, + struct ccp_cmd, entry); +list_del (&backlog->entry); + }else { + ccp->cmd_count--;
drivers/crypto/ccp/ccp-dev.c static struct ccp_cmd *ccp_dequeue_cmd(struct ccp_cmd_queue *cmd_q) · line 400
Quoted 10 lines
drivers/crypto/ccp/ccp-dev.c +7
-0+ } + }else if (!list_empty (&ccp->backlog)) { +/* No command is available for execution, but a backlogged +* command is stranded: reserve a slot for it. +*/ backlog = list_first_entry(&ccp->backlog, struct ccp_cmd, entry);list_del (&backlog->entry); + + ccp->cmd_count++;
This seems dangerous. Since you're keeping the cmd_count without actually moving the backlogged entry over to the active list. Wouldn't this increment potentially exceed the limit on the number of active entries?
I think rather than spending time on fixing this mess, we should just convert it over to crypto_engine and be done with it. The crypto_engine system is known to work without these subtle race conditions and all drivers should use it.
Thanks,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt