linux-kernel - Re: [PATCH RESEND v4 1/6] 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: <ZugZ9NcnPMNTH_ZQ@smile.fi.intel.com>
Date: Mon, 16 Sep 2024 14:43:48 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Serge Semin <fancer.lancer@...il.com>
Cc: Ferry Toth <ftoth@...londelft.nl>, 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 RESEND v4 1/6] dmaengine: dw: Add peripheral bus width
 verification

On Sat, Sep 14, 2024 at 10:22:22PM +0300, Serge Semin wrote:
> On Sat, Sep 14, 2024 at 10:12:35PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 02, 2024 at 10:50:46AM +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 and calling it in dwc_config() which is supposed to be executed
> > > before preparing any transfer. The new method will 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.
> > 
> > This patch broke Intel Merrifield iDMA32 + SPI PXA2xx configuration to
> > me. Since it's first in the series and most likely the rest is
> > dependent and we are almost at the release date I propose to roll back
> > and start again after v6.12-rc1 will be out. Vinod, can we revert the
> > entire series, please?
> 
> I guess it's not the best option, since the patch has already been
> backported to the stable kernels anyway. Rolling back it from all of
> them seems tiresome. Let's at least try to fix the just discovered
> problem?

Please, provide one we can test!

> Could you please provide more details about what exactly happening?

Sure. AFAICT the only problematic line is this:

        else if (!is_power_of_2(reg_width) || reg_width > max_width)

in your patch, and it may trigger, for example, when max_width == 0.
This, in accordance with my brief investigation, happens due to the following.

The DMA slave configuration is being copied twice in DW DMA code:
1) when respective filter function triggers (see acpi/of glue code);
2) when the channel is about to be allocated.

The iDMA32 has only a single master, and hence m_master == p_master,
BUT the filter function in the acpi code is universal and it copies
the wrong (from the iDMA32 perspective) value to p_master.
As the result, when you retrieve the max_width, it takes the value from
p_master, which is defined to 1 (sic!), and hence assigns it to 0.

I don't know how to quickfix this as the proper fix seems to provide
the correct data in the first place.

Any ideas, patches we may test?

-- 
With Best Regards,
Andy Shevchenko