| 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
| ||
|
Message-ID:
<DM4PR12MB6109E009DAC953525093FE808C1CA@DM4PR12MB6109.namprd12.prod.outlook.com>
Date: Wed, 24 Sep 2025 12:22:48 +0000
From: "Guntupalli, Manikanta" <manikanta.guntupalli@....com>
To: Arnd Bergmann <arnd@...db.de>, "git (AMD-Xilinx)" <git@....com>, "Simek,
Michal" <michal.simek@....com>, Alexandre Belloni
<alexandre.belloni@...tlin.com>, Frank Li <Frank.Li@....com>, Rob Herring
<robh@...nel.org>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Przemysław Gaj <pgaj@...ence.com>,
Wolfram Sang <wsa+renesas@...g-engineering.com>,
"tommaso.merciai.xr@...renesas.com" <tommaso.merciai.xr@...renesas.com>,
"quic_msavaliy@...cinc.com" <quic_msavaliy@...cinc.com>, "S-k, Shyam-sundar"
<Shyam-sundar.S-k@....com>, Sakari Ailus <sakari.ailus@...ux.intel.com>,
"'billy_tsai@...eedtech.com'" <billy_tsai@...eedtech.com>, Kees Cook
<kees@...nel.org>, "Gustavo A. R. Silva" <gustavoars@...nel.org>, Jarkko
Nikula <jarkko.nikula@...ux.intel.com>, Jorge Marques
<jorge.marques@...log.com>, "linux-i3c@...ts.infradead.org"
<linux-i3c@...ts.infradead.org>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, Linux-Arch <linux-arch@...r.kernel.org>,
"linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org>
CC: "Pandey, Radhey Shyam" <radhey.shyam.pandey@....com>, "Goud, Srinivas"
<srinivas.goud@....com>, "Datta, Shubhrajyoti" <shubhrajyoti.datta@....com>,
"manion05gk@...il.com" <manion05gk@...il.com>
Subject: RE: [PATCH V7 3/4] i3c: master: Add endianness support for
i3c_readl_fifo() and i3c_writel_fifo()
[Public]
Hi,
> -----Original Message-----
> From: Arnd Bergmann <arnd@...db.de>
> Sent: Wednesday, September 24, 2025 3:30 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli@....com>; git (AMD-Xilinx)
> <git@....com>; Simek, Michal <michal.simek@....com>; Alexandre Belloni
> <alexandre.belloni@...tlin.com>; Frank Li <Frank.Li@....com>; Rob Herring
> <robh@...nel.org>; krzk+dt@...nel.org; Conor Dooley <conor+dt@...nel.org>;
> Przemysław Gaj <pgaj@...ence.com>; Wolfram Sang <wsa+renesas@...g-
> engineering.com>; tommaso.merciai.xr@...renesas.com;
> quic_msavaliy@...cinc.com; S-k, Shyam-sundar <Shyam-sundar.S-k@....com>;
> Sakari Ailus <sakari.ailus@...ux.intel.com>; 'billy_tsai@...eedtech.com'
> <billy_tsai@...eedtech.com>; Kees Cook <kees@...nel.org>; Gustavo A. R. Silva
> <gustavoars@...nel.org>; Jarkko Nikula <jarkko.nikula@...ux.intel.com>; Jorge
> Marques <jorge.marques@...log.com>; linux-i3c@...ts.infradead.org;
> devicetree@...r.kernel.org; linux-kernel@...r.kernel.org; Linux-Arch <linux-
> arch@...r.kernel.org>; linux-hardening@...r.kernel.org
> Cc: Pandey, Radhey Shyam <radhey.shyam.pandey@....com>; Goud, Srinivas
> <srinivas.goud@....com>; Datta, Shubhrajyoti <shubhrajyoti.datta@....com>;
> manion05gk@...il.com
> Subject: Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo()
> and i3c_writel_fifo()
>
> On Wed, Sep 24, 2025, at 11:00, Guntupalli, Manikanta wrote:
> >> > if (nbytes & 3) {
> >> > u32 tmp = 0;
> >> >
> >> > memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
> >> > - writel(tmp, addr);
> >> > +
> >> > + if (endian)
> >> > + writel_be(tmp, addr);
> >> > + else
> >> > + writel(tmp, addr);
> >>
> >> This bit however seems to fix a bug, but does so in a confusing way.
> >> The way the FIFO registers usually deal with excess bytes is to put
> >> them into the first bytes of the FIFO register, so this should just
> >> be a
> >>
> >> writesl(addr, &tmp, 1);
> >>
> >> to write one set of four bytes into the FIFO without endian-swapping.
> >>
> >> Could it be that you are just trying to use a normal i3c adapter with
> >> little-endian registers on a normal big-endian machine but ran into this bug?
> > The intention here is to enforce big-endian ordering for the trailing
> > bytes as well. By using __cpu_to_be32() for full words and writel_be()
> > for the leftover word, the FIFO is always accessed in big-endian
> > format, regardless of the CPU's native endianness. This ensures
> > consistent and correct data ordering from the device's perspective.
>
> No, this makes no sense: The 'else' portion is incorrect in the function, and prevents
> it from working on all big-endian CPUs because 'writel()' only works for little-endian
> 32-bit registers. If you just fix the bug as I described above, this will work correctly on
> any driver calling the current function. At that point, your hack becomes a nop for big-
> endian systems, while calling the function with 'endian = true' on little-endian kernels
> is still wrong.
>
> Please start by fixing the existing bug and test that again.
>
> I know that endianess with FIFO registers is hard to understand, and everyone has
> to spend the time once to actually wrap their head around this. Even if you don't
> believe me, please try the bugfix below (without your added argument) and think
> about how FIFO registers that transfer byte streams are different of fixed-endian 32-
> bit registers. Note that your driver uses little-endian readl() for normal registers, and
> this is portable to both LE and BE kernels.
>
> See also the explanation in 41739ee355ab ("asm-generic: io:
> don't perform swab during {in,out} string functions").
>
> Arnd
>
> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h index
> 0d857cc68cc5..0f8a25cb71e7 100644
> --- a/drivers/i3c/internals.h
> +++ b/drivers/i3c/internals.h
> @@ -38,7 +38,7 @@ static inline void i3c_writel_fifo(void __iomem *addr, const
> void *buf,
> u32 tmp = 0;
>
> memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
> - writel(tmp, addr);
> + writesl(addr, &buf, 1);
> }
> }
>
> @@ -55,7 +55,7 @@ static inline void i3c_readl_fifo(const void __iomem *addr, void
> *buf,
> if (nbytes & 3) {
> u32 tmp;
>
> - tmp = readl(addr);
> + readsl(addr, &tmp, 1);
> memcpy(buf + (nbytes & ~3), &tmp, nbytes & 3);
> }
> }
We have not observed any issue on little-endian systems in our testing so far (as I mentioned earlier in asm-generic/io.h: Add big-endian MMIO accessors).
That said, I understand your point about FIFO semantics being different from fixed-endian registers. To cover both cases, we considered using writesl() for little-endian and introducing a writesl_be() helper for big-endian, as shown below:
static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
int nbytes, enum i3c_fifo_endian endian)
{
if (endian)
writesl_be(addr, buf, nbytes / 4);
else
writesl(addr, buf, nbytes / 4);
if (nbytes & 3) {
u32 tmp = 0;
memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
if (endian)
writesl_be(addr, &tmp, 1);
else
writesl(addr, &tmp, 1);
}
}
With this approach, both little-endian and big-endian cases works as expected.
Thanks,
Manikanta.
Powered by blists - more mailing lists