linux-kernel - Re: [PATCH 1/4] dmaengine: dw: Add peripheral bus width verification

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: <Zh680h4h6hURIb82@smile.fi.intel.com>
Date: Tue, 16 Apr 2024 21:00:50 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Serge Semin <fancer.lancer@...il.com>
Cc: Viresh Kumar <vireshk@...nel.org>, Vinod Koul <vkoul@...nel.org>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>, dmaengine@...r.kernel.org,
	linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] dmaengine: dw: Add peripheral bus width verification

On Tue, Apr 16, 2024 at 07:28:55PM +0300, Serge Semin wrote:
> Currently the src_addr_width and dst_addr_width fields of the
> dma_slave_config structure are mapped to the CTLx.SRC_TR_WIDTH and
> CTLx.DST_TR_WIDTH fields of the peripheral bus side in order to have the
> properly aligned data passed to the target device. It's done just by
> converting the passed peripheral bus width to the encoded value using the
> __ffs() function. This implementation has several problematic sides:
> 
> 1. __ffs() is undefined if no bit exist in the passed value. Thus if the
> specified addr-width is DMA_SLAVE_BUSWIDTH_UNDEFINED, __ffs() may return
> unexpected value depending on the platform-specific implementation.
> 
> 2. DW AHB DMA-engine permits having the power-of-2 transfer width limited
> by the DMAH_Mk_HDATA_WIDTH IP-core synthesize parameter. Specifying
> bus-width out of that constraints scope will definitely cause unexpected
> result since the destination reg will be only partly touched than the
> client driver implied.
> 
> Let's fix all of that by adding the peripheral bus width verification
> method which would make sure that the passed source or destination address
> width is valid and if undefined then the driver will just fallback to the
> 1-byte width transfer.

Please, add a word that you apply the check in the dwc_config() which is
supposed to be called before preparing any transfer?

..

> +static int dwc_verify_p_buswidth(struct dma_chan *chan)
> +{
> +	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
> +	struct dw_dma *dw = to_dw_dma(chan->device);
> +	u32 reg_width, max_width;
> +
> +	if (dwc->dma_sconfig.direction == DMA_MEM_TO_DEV)
> +		reg_width = dwc->dma_sconfig.dst_addr_width;
> +	else if (dwc->dma_sconfig.direction == DMA_DEV_TO_MEM)
> +		reg_width = dwc->dma_sconfig.src_addr_width;

> +	else /* DMA_MEM_TO_MEM */

Actually not only this direction, but TBH I do not see value in these comments.

> +		return 0;
> +
> +	max_width = dw->pdata->data_width[dwc->dws.p_master];
> +
> +	/* Fall-back to 1byte transfer width if undefined */

1-byte
(as you even used in the commit message correctly)

> +	if (reg_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
> +		reg_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> +	else if (!is_power_of_2(reg_width) || reg_width > max_width)
> +		return -EINVAL;
> +	else /* bus width is valid */
> +		return 0;
> +
> +	/* Update undefined addr width value */
> +	if (dwc->dma_sconfig.direction == DMA_MEM_TO_DEV)
> +		dwc->dma_sconfig.dst_addr_width = reg_width;
> +	else /* DMA_DEV_TO_MEM */
> +		dwc->dma_sconfig.src_addr_width = reg_width;

So, can't you simply call clamp() for both fields in dwc_config()?

> +	return 0;
> +}

..

> +	int err;

Hmm... we have two functions one of which is using different name for this.
Can we have a patch to convert to err the other one?

-- 
With Best Regards,
Andy Shevchenko