Re: [PATCH v2 1/2] accel/amdxdna: clear the mailbox channel pointer when starting it fails

[Lizhi Hou]

Applied to drm-misc-next

On 9/15/26 09:36, Lizhi Hou wrote:
> On 9/13/26 14:31, Eva Crystal wrote:
> > aie2_create_context() allocates a mailbox channel, starts it, and on a
> > failed start frees it again:
> >
> >     hwctx->priv->mbox_chann = xdna_mailbox_alloc_channel(ndev->mbox);
> >     ...
> >     ret = xdna_mailbox_start_channel(hwctx->priv->mbox_chann, ...);
> >     if (ret)
> >         goto free_channel;
> >     ...
> > free_channel:
> >     xdna_mailbox_free_channel(hwctx->priv->mbox_chann);
> >
> > hwctx->priv->mbox_chann keeps pointing at the freed channel. That
> > pointer is the driver's own test for whether a hardware context has a
> > usable channel: aie2_destroy_context() returns early on NULL,
> > aie2_sched_job_run() refuses to run a job on NULL, and the message
> > helpers return -ENODEV on NULL. A stale pointer passes all of them.
> >
> > It matters on the restart path. aie2_hwctx_restart() calls
> > aie2_create_context() again on a hardware context that is already live,
> > after aie2_hwctx_stop() has torn its channel down, and the context
> > survives a failure there: aie2_sched_job_timedout() discards the return
> > value entirely, and aie2_hwctx_resume() only propagates it. What is
> > left is a live hardware context holding a freed channel, and every
> > later user of it takes the non-NULL branch:
> >
> >    - the next job submitted reaches xdna_mailbox_send_msg() on the freed
> >      channel, via the !mbox_chann guard in aie2_sched_job_run(),
> >    - a second command timeout reaches aie2_hwctx_stop() ->
> >      aie2_destroy_context(),
> >    - closing the device reaches aie2_hwctx_fini() ->
> >      aie2_release_resource() -> xrs_release_resource() ->
> >      aie2_xrs_unload() -> aie2_destroy_context(),
> >
> > and aie2_destroy_context() then calls xdna_mailbox_stop_channel()
> > followed by xdna_mailbox_free_channel() on memory that was already
> > freed: a use-after-free, and a second free of the same channel.
> >
> > The AIE4 management channel already does this correctly -
> > aie4_mailbox_start() clears ndev->aie.mgmt_chann right after freeing
> > it. Do the same here.
> >
> > The create path itself is not affected: when aie2_xrs_load() fails,
> > xrs_allocate_resource() removes the solver node without calling
> > ->unload, and aie2_hwctx_init() frees hwctx->priv, so the stale pointer
> > never outlives the structure holding it.
> >
> > Starting a channel can fail today without this patch: the ring buffer
> > sizes firmware reports are rejected unless both are powers of two, and
> > request_irq() can fail.
> >
> > Fixes: d5b8b0347fa8 ("accel/amdxdna: Split mailbox channel create 
> > function")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Eva Crystal <0xiviel@xxxxxxxxx>
> > ---
> >   drivers/accel/amdxdna/aie2_message.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/accel/amdxdna/aie2_message.c 
> > b/drivers/accel/amdxdna/aie2_message.c
> > index b4c49259a1a2..f658760c3d48 100644
> > --- a/drivers/accel/amdxdna/aie2_message.c
> > +++ b/drivers/accel/amdxdna/aie2_message.c
> > @@ -277,6 +277,7 @@ int aie2_create_context(struct amdxdna_dev_hdl 
> > *ndev, struct amdxdna_hwctx *hwct
> >     free_channel:
> >       xdna_mailbox_free_channel(hwctx->priv->mbox_chann);
> > +    hwctx->priv->mbox_chann = NULL;
> Reviewed-by: Lizhi Hou <lizhi.hou@xxxxxxx>
> >   del_ctx_req:
> >       aie2_destroy_context_req(ndev, hwctx->fw_ctx_id);
> >       return ret;