linux-kernel - Re: [PATCH] iomap: Fix -Wmissing-prototypes on UM

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: <96829b49-62a9-435b-9e35-fe3ef01d1c67@app.fastmail.com>
Date: Mon, 03 Feb 2025 14:08:35 +0100
From: "Arnd Bergmann" <arnd@...db.de>
To: "Andy Shevchenko" <andriy.shevchenko@...ux.intel.com>
Cc: Thomas Weißschuh <thomas.weissschuh@...utronix.de>,
 "Kees Cook" <kees@...nel.org>, "Palmer Dabbelt" <palmer@...osinc.com>,
 "Andrew Morton" <akpm@...ux-foundation.org>,
 Linux-Arch <linux-arch@...r.kernel.org>, linux-kernel@...r.kernel.org,
 "Richard Cochran" <richardcochran@...il.com>
Subject: Re: [PATCH] iomap: Fix -Wmissing-prototypes on UM

On Mon, Feb 3, 2025, at 13:07, Andy Shevchenko wrote:
> On Mon, Feb 03, 2025 at 12:43:01PM +0100, Arnd Bergmann wrote:
>> In addition, these seem to be timer registers that may overrun
>> from the lo into the hi field between the two accesses, so
>> technically a 32-bit host needs to do an extra read to
>> check for overflow and possibly repeat the access.
>
> Yes, precisely why hi_lo is used to minimize the error when it races like this.
>
> But IIRC *_pch drivers are for 32-bit platform, the code, if so, was made
> to be compiled on 64-bit but never used IRL, just for test coverage.
>
> (I believe the PCH stands for EG20 PCH, I have [32-bit] boards with it.)

Ok, so we don't even know how that hardware block would read to
a 64-bit bus transaction, it might give a race-free result, might
have the same race as 32-bit or might just cause data corruption
(e.g. ignoring half the bits).

I think the usual way to access a timestamp in two registers works
like this

u64 read_double_reg(u32 __iomem *reg)
{
       u32 hi, lo;

       /* check for overflow race by re-reading upper bits */
       do {
               hi = readl(reg + 1);
               lo = readl(reg);
       } while (hi != readl(reg + 1);

       return (u64)hi << 32 | lo;
}

void write_double_reg(u32 __iomem *reg, u64 val)
{
       /* ensure the low bits don't overflow right now, assumes
          low word is ticking up */
       writel(reg, 0);

       writel(reg + 1, upper_32_bits(val));
       writel(reg,     lower_32_bits(val));
}

[If there might be concurrent read/write accesses, it gets
much more complicated than this.]

Do you know why the driver doesn't do it like that?

> I like the lib/* and include/* cleanup but PTP probably should stay as is.
> OTOH WWAN case most likely had been tested on 64-bit platforms only and
> proves that readq()/writeq() works there, so it's okay to unify.

Ok, I'll try to split it up into sensible patches then. For ptp
(both ixp46x and pch), these are the options I see:

- leave it unchanged since nobody cares any more
- add some comments about being racy and possibly broken on 64-bit
- revert your pch patch d09adf61002f/8664d49a815e3 to make it have 32-
  bit accesses again and fix the theoretical 64-bit issue but not the
  race
- use helper functions like the ones I showed above and test it
  properly

I also added Richard Cochran to cc, as he wrote the original
ixp46x driver and may know of other ptp drivers that have
this issue. One potential candidate I see is
https://elixir.bootlin.com/linux/v6.13.1/source/drivers/ptp/ptp_dfl_tod.c#L226
and other functions in that file.

       Arnd