Re: [PATCH] crypto: ccp - Fix use-after-free in backlog cmd advancement

[Herbert Xu]

Fan Wu <fanwu01@xxxxxxxxxx> wrote:
> CCP_CMD_MAY_BACKLOG commands are removed from ccp->backlog by a
> queue kthread and advanced asynchronously by ccp_do_cmd_backlog().
> The promoted command is no longer on either command list.
> 
> During device removal, ccp*_destroy() stops the queue kthreads and
> flushes the lists, but does not wait for a promoted work on system_wq.
> That work can subsequently access the devm-allocated ccp_device after
> it has been released, and wake a queue kthread whose task_struct
> kthread_stop() has already released.
> 
> Use a per-device workqueue for backlog advancement and destroy it after
> stopping all queue kthreads, so every promoted work has completed before
> the command lists are flushed. The queue kthreads are the only source
> of backlog works, so no new work can be queued once they are stopped.
> Individual cancellation is not possible because a promoted command is
> no longer reachable from either list.
> 
> Mark the device as halting before teardown. A draining backlog work
> then completes its command with -ENODEV instead of re-queuing it.
> Keep the halting check and the kthread wake under cmd_lock so teardown
> cannot stop the selected kthread between them.
> 
> This issue was found by an in-house static analysis tool.
> 
> Fixes: 63b945091a07 ("crypto: ccp - CCP device driver and interface support")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: Codex:gpt-5.6
> Co-developed-by: Song Li <songl@xxxxxxxxxx>
> Signed-off-by: Song Li <songl@xxxxxxxxxx>
> Signed-off-by: Fan Wu <fanwu01@xxxxxxxxxx>
> ---
> drivers/crypto/ccp/ccp-dev-v3.c | 19 +++++++++++++++++++
> drivers/crypto/ccp/ccp-dev-v5.c | 18 ++++++++++++++++++
> drivers/crypto/ccp/ccp-dev.c    | 33 ++++++++++++++++++++++++++++-----
> drivers/crypto/ccp/ccp-dev.h    |  4 ++++
> 4 files changed, 69 insertions(+), 5 deletions(-)

Thanks for catching this!

The backlog handling still looks a bit wrong though.  When you detach
a backlogged request after processing a normal request, there is
a gap before the backlogged request is added to the normal queue.

In that gap new requests could have come in and taken up the space
meant for the backlogged request.

I think this should be done atomically so that there is no gap
between the two operations, to ensure that the space is actually
reserved for the backlogged entry.

Once this is done, there should be no need to have a per-backlog
work struct, which means that you don't need to add a separate
work queue.  In fact, there may not be a need for a work struct
at all since this is all running out of process context anyway.

On a higher level, this driver should probably switch over to
using crypto_engine rather than doing backlog handling manually.

Cheers,
-- 
Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt