linux-kernel - Re: [PATCH v2 3/5] spmi: mtk-pmif: Keep spinlock until read is fully done

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] [day] [month] [year] [list]

Message-ID: <CAGXv+5G2dVyKzrpofTu02CcX7inpa0OWw7Bhvv-5a3kEepNnQw@mail.gmail.com>
Date: Tue, 22 Jul 2025 15:53:55 +0800
From: Chen-Yu Tsai <wenst@...omium.org>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: sboyd@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, 
	matthias.bgg@...il.com, hsin-hsiung.wang@...iatek.com, 
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org, 
	kernel@...labora.com, Nícolas F. R. A. Prado <nfraprado@...labora.com>
Subject: Re: [PATCH v2 3/5] spmi: mtk-pmif: Keep spinlock until read is fully done

On Thu, Jul 3, 2025 at 9:07 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com> wrote:
>
> Move the spin unlocking to after reading the contents of the
> PMIF_SWINF_(x)_RDATA_31_0 register in pmif_spmi_read_cmd():
> since this is the only register that we can read to get the
> data from all of the arbitered busses, a concurrent request
> for reading (especially on a busy arbiter) will show a race
> condition and a unexpected or corrupted value may be read.
>
> Doing the entire read sequence while spin locked guarantees
> that concurrent access to the arbiter doesn't happen.

I think the driver should have implemented this from the beginning, so

Fixes: b45b3ccef8c0 ("spmi: mediatek: Add support for MT6873/8192")
Fixes: f200fff8d019 ("spmi: mtk-pmif: Serialize PMIF status check and
command submission")
Reviewed-by: Chen-Yu Tsai <wenst@...omium.org>

> Reviewed-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
> ---
>  drivers/spmi/spmi-mtk-pmif.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/spmi/spmi-mtk-pmif.c b/drivers/spmi/spmi-mtk-pmif.c
> index 68f458587c67..9f416b231ab8 100644
> --- a/drivers/spmi/spmi-mtk-pmif.c
> +++ b/drivers/spmi/spmi-mtk-pmif.c
> @@ -22,7 +22,7 @@
>  #define PMIF_CMD_EXT_REG       2
>  #define PMIF_CMD_EXT_REG_LONG  3
>
> -#define PMIF_DELAY_US   10
> +#define PMIF_DELAY_US   2
>  #define PMIF_TIMEOUT_US (10 * 1000)
>
>  #define PMIF_CHAN_OFFSET 0x5
> @@ -372,7 +372,6 @@ static int pmif_spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>         /* Send the command. */
>         cmd = (opc << 30) | (sid << 24) | ((len - 1) << 16) | addr;
>         pmif_writel(arb, pbus, cmd, inf_reg->ch_send);
> -       raw_spin_unlock_irqrestore(&pbus->lock, flags);
>
>         /*
>          * Wait for Software Interface FSM state to be WFVLDCLR,
> @@ -382,13 +381,16 @@ static int pmif_spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>                                         data, GET_SWINF(data) == SWINF_WFVLDCLR,
>                                         PMIF_DELAY_US, PMIF_TIMEOUT_US);
>         if (ret < 0) {
> +               raw_spin_unlock_irqrestore(&pbus->lock, flags);
>                 dev_err(&ctrl->dev, "failed to wait for SWINF_WFVLDCLR\n");
>                 return ret;
>         }
>
>         data = pmif_readl(arb, pbus, inf_reg->rdata);
> -       memcpy(buf, &data, len);
>         pmif_writel(arb, pbus, 1, inf_reg->ch_rdy);
> +       raw_spin_unlock_irqrestore(&pbus->lock, flags);
> +
> +       memcpy(buf, &data, len);
>
>         return 0;
>  }
> --
> 2.49.0
>
>