linux-kernel - RE: [PATCH] firmware: arm_scmi: Check Mailbox/SMT channel for consistency

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <PUZPR06MB54988A8170D462FC8EE43A05F095A@PUZPR06MB5498.apcprd06.prod.outlook.com>
Date: Thu, 21 Dec 2023 10:31:29 +0000
From: Xinglong Yang <xinglong.yang@...tech.com>
To: Cristian Marussi <cristian.marussi@....com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>
CC: "sudeep.holla@....com" <sudeep.holla@....com>,
	"vincent.guittot@...aro.org" <vincent.guittot@...aro.org>,
	"stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH] firmware: arm_scmi: Check Mailbox/SMT channel for
 consistency

Hi, Cristian

This patch successfully solves the bug.

From: Cristian Marussi <cristian.marussi@....com> Sent: Thursday, December 21, 2023 1:21 AM
> On reception of a completion interrupt the SMT memory area is accessed to
> retrieve the message header at first and then, if the message sequence
> number identifies a transaction which is still pending, the related
> payload is fetched too.
>
> When an SCMI command times out the channel ownership remains with the
> platform until eventually a late reply is received and, as a consequence,
> any further transmission attempt remains pending, waiting for the channel
> to be relinquished by the platform.
>
> Once that late reply is received the channel ownership is given back
> to the agent and any pending request is then allowed to proceed and
> overwrite the SMT area of the just delivered late reply; then the wait for
> the reply to the new request starts.
>
> It has been observed that the spurious IRQ related to the late reply can
> be wrongly associated with the freshly enqueued request: when that
> happens
> the SCMI stack in-flight lookup procedure is fooled by the fact that the
> message header now present in the SMT area is related to the new pending
> transaction, even though the real reply has still to arrive.
>
> This race-condition on the A2P channel can be detected by looking at the
> channel status bits: a genuine reply from the platform will have set the
> channel free bit before triggering the completion IRQ.
>
> Add a consistency check to validate such condition in the A2P ISR.
>
> Reported-by: Xinglong Yang <xinglong.yang@...tech.com>
> Closes:
> https://lore.k/
> ernel.org%2Fall%2FPUZPR06MB54981E6FA00D82BFDBB864FBF08DA%40PUZP
> R06MB5498.apcprd06.prod.outlook.com%2F&data=05%7C02%7Cxinglong.ya
> ng%40cixtech.com%7C669e9ff5e6764a77791208dc01801b8e%7C0409f77ae53
> d4d23943eccade7cb4811%7C1%7C0%7C638386896955072826%7CUnknown%
> 7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW
> wiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=T1DOD7KfY%2FJNJHacHtX
> d5wcfde%2Fd5UDqGvyW4vuYwYU%3D&reserved=0
> Fixes: 5c8a47a5a91d ("firmware: arm_scmi: Make scmi core independent of
> the transport type")
> CC: stable@...r.kernel.org # 5.15+
> Signed-off-by: Cristian Marussi <cristian.marussi@....com>
> ---
>  drivers/firmware/arm_scmi/common.h  |  1 +
>  drivers/firmware/arm_scmi/mailbox.c | 14 ++++++++++++++
>  drivers/firmware/arm_scmi/shmem.c   |  6 ++++++
>  3 files changed, 21 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/common.h
> b/drivers/firmware/arm_scmi/common.h
> index 3b7c68a11fd0..0956c2443840 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -329,6 +329,7 @@ void shmem_fetch_notification(struct
> scmi_shared_mem __iomem *shmem,
>  void shmem_clear_channel(struct scmi_shared_mem __iomem *shmem);
>  bool shmem_poll_done(struct scmi_shared_mem __iomem *shmem,
>                      struct scmi_xfer *xfer);
> +bool shmem_channel_free(struct scmi_shared_mem __iomem *shmem);
>
>  /* declarations for message passing transports */
>  struct scmi_msg_payld;
> diff --git a/drivers/firmware/arm_scmi/mailbox.c
> b/drivers/firmware/arm_scmi/mailbox.c
> index 19246ed1f01f..b8d470417e8f 100644
> --- a/drivers/firmware/arm_scmi/mailbox.c
> +++ b/drivers/firmware/arm_scmi/mailbox.c
> @@ -45,6 +45,20 @@ static void rx_callback(struct mbox_client *cl, void *m)
>  {
>         struct scmi_mailbox *smbox = client_to_scmi_mailbox(cl);
>
> +       /*
> +        * An A2P IRQ is NOT valid when received while the platform still has
> +        * the ownership of the channel, because the platform at first releases
> +        * the SMT channel and then sends the completion interrupt.
> +        *
> +        * This addresses a possible race condition in which a spurious IRQ from
> +        * a previous timed-out reply which arrived late could be wrongly
> +        * associated with the next pending transaction.
> +        */
> +       if (cl->knows_txdone && !shmem_channel_free(smbox->shmem)) {
> +               dev_warn(smbox->cinfo->dev, "Ignoring spurious A2P IRQ !\n");
> +               return;
> +       }
> +
>         scmi_rx_callback(smbox->cinfo, shmem_read_header(smbox->shmem),
> NULL);
>  }
>
> diff --git a/drivers/firmware/arm_scmi/shmem.c
> b/drivers/firmware/arm_scmi/shmem.c
> index 87b4f4d35f06..517d52fb3bcb 100644
> --- a/drivers/firmware/arm_scmi/shmem.c
> +++ b/drivers/firmware/arm_scmi/shmem.c
> @@ -122,3 +122,9 @@ bool shmem_poll_done(struct scmi_shared_mem
> __iomem *shmem,
>                 (SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR |
>                  SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE);
>  }
> +
> +bool shmem_channel_free(struct scmi_shared_mem __iomem *shmem)
> +{
> +       return (ioread32(&shmem->channel_status) &
> +                       SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE);
> +}
> --
> 2.34.1

Thanks,
Xinglong