linux-kernel - Re: [PATCH] dmaengine: qcom: bam_dma: Avoid accessing BAM

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: <Z6m8btwhJ9q4RjB6@linaro.org>
Date: Mon, 10 Feb 2025 09:44:30 +0100
From: Stephan Gerhold <stephan.gerhold@...aro.org>
To: Bjorn Andersson <bjorn.andersson@....qualcomm.com>
Cc: Vinod Koul <vkoul@...nel.org>, Md Sadre Alam <quic_mdalam@...cinc.com>,
	linux-arm-msm@...r.kernel.org, dmaengine@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
	Georgi Djakov <djakov@...nel.org>
Subject: Re: [PATCH] dmaengine: qcom: bam_dma: Avoid accessing BAM_REVISION
 on remote BAM

On Fri, Feb 07, 2025 at 12:17:33PM -0800, Bjorn Andersson wrote:
> Commit '57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing
> unavailable register")' made this read unconditional, in order to
> identify if the instance is BAM-NDP or BAM-Lite.
> But the BAM_REVISION register is not accessible on remotely managed BAM
> instances and attempts to access it causes the system to crash.
> 
> Move the access back to be conditional and expand the checks that was
> introduced to restore the old behavior when no revision information is
> available.
> 
> Fixes: 57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing unavailable register")
> Reported-by: Georgi Djakov <djakov@...nel.org>
> Closes: https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@kernel.org/
> Signed-off-by: Bjorn Andersson <bjorn.andersson@....qualcomm.com>

This patch fixes the most critical regression (the bus hang), but the
in_range(..., BAM_NDP) checks are also wrong. They do not consider the
plain "BAM" type where the register is apparently also available. The
check should be !in_range(..., BAM_LITE) instead to fix this.

I mentioned this twice to Md Sadre Alam [1, 2], but a fix was
unfortunately never sent for that part of the regression.

I think we should take Caleb's patch and revert the entire patch for the
6.14 cycle. There are several incorrect assumptions in the original
patch, it will be easier to review a fixed version with the full diff,
rather than looking at incremental fixups.

On a somewhat related note, I'm working on a patch series for bam_dma to
clean up the handling of remotely controlled BAMs. It will make it more
clear when it's safe to access BAM registers and when not, and should
allow reading the revision also for remotely controlled BAMs. This would
avoid the need for all these if (!bdev->bam_revision) checks.

Thanks,
Stephan

[1]: https://lore.kernel.org/linux-arm-msm/Z4D2jQNNW94qGIlv@linaro.org/
[2]: https://lore.kernel.org/linux-arm-msm/Z4_U19_QyH2RJvKW@linaro.org/

> ---
>  drivers/dma/qcom/bam_dma.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index c14557efd577..d42d913492a8 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -445,8 +445,8 @@ static void bam_reset(struct bam_device *bdev)
>  	writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL));
>  
>  	/* set descriptor threshold, start with 4 bytes */
> -	if (in_range(bdev->bam_revision, BAM_NDP_REVISION_START,
> -		     BAM_NDP_REVISION_END))
> +	if (!bdev->bam_revision ||
> +	    in_range(bdev->bam_revision, BAM_NDP_REVISION_START, BAM_NDP_REVISION_END))
>  		writel_relaxed(DEFAULT_CNT_THRSHLD,
>  			       bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD));
>  
> @@ -1006,8 +1006,8 @@ static void bam_apply_new_config(struct bam_chan *bchan,
>  			maxburst = bchan->slave.src_maxburst;
>  		else
>  			maxburst = bchan->slave.dst_maxburst;
> -		if (in_range(bdev->bam_revision, BAM_NDP_REVISION_START,
> -			     BAM_NDP_REVISION_END))
> +		if (!bdev->bam_revision ||
> +		    in_range(bdev->bam_revision, BAM_NDP_REVISION_START, BAM_NDP_REVISION_END))
>  			writel_relaxed(maxburst,
>  				       bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD));
>  	}
> @@ -1199,11 +1199,12 @@ static int bam_init(struct bam_device *bdev)
>  	u32 val;
>  
>  	/* read revision and configuration information */
> -	val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
> -	if (!bdev->num_ees)
> +	if (!bdev->num_ees) {
> +		val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
>  		bdev->num_ees = (val >> NUM_EES_SHIFT) & NUM_EES_MASK;
>  
> -	bdev->bam_revision = val & REVISION_MASK;
> +		bdev->bam_revision = val & REVISION_MASK;
> +	}
>  
>  	/* check that configured EE is within range */
>  	if (bdev->ee >= bdev->num_ees)
> 
> ---
> base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
> change-id: 20250207-bam-read-fix-2b31297d3fa1
> 
> Best regards,
> -- 
> Bjorn Andersson <bjorn.andersson@....qualcomm.com>
>