linux-kernel - [PATCH] asm/io: Correct output operand specification of the MMIO write* routines

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-next>] [day] [month] [year] [list]

Message-ID: <20190417085008.GB20492@zn.tnic>
Date:   Wed, 17 Apr 2019 10:50:08 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Michael Matz <matz@...e.de>, x86-ml <x86@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>
Subject: [PATCH] asm/io: Correct output operand specification of the MMIO
 write* routines

Hi Linus,

I'm looking at

  c1f64a58003f ("x86: MMIO and gcc re-ordering issue")

and trying to figure out was there any particular reason the address to
the MMIO write routines had to be an input operand?

Because if not, please have a look at the patch below. It boots here and
from the couple of resulting asm I looked at, it doesn't change. But
there might be some other aspect I'm missing so...

Thx.

---
From: Borislav Petkov <bp@...e.de>

Currently, all the MMIO write operations specify the output @addr
operand as an input operand to the extended asm statement. This works
because the asm statement is marked volatile and "memory" is on the
clobbered list, preventing gcc from optimizing around an inline asm
without output operands.

However, the more correct way of writing this is to make the target MMIO
write address an input *and* output operand so that gcc is aware that
modifications have happened through it.

Now, one could speculate further and say, the memory clobber could be
dropped:

  *P;   // (1)
  mmio_write("..." : "+m" (whatever));  // no memory clobber
  *P;   // (2)

but then gcc would at -O2 optimize the access in (2) by replacing it
with its value from (1), which would be wrong.

The solution would be sticking a memory barrier after (1) but then that
puts the onus on the programmer to make sure it doesn't get forgotten,
and that is the wrong approach: generic interfaces like that should
JustWork(tm) so let's leave the "memory" clobber.

Signed-off-by: Borislav Petkov <bp@...e.de>
Cc: Michael Matz <matz@...e.de>
---
 arch/x86/include/asm/io.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 686247db3106..33c4d8776b47 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -49,10 +49,11 @@ static inline type name(const volatile void __iomem *addr) \
 { type ret; asm volatile("mov" size " %1,%0":reg (ret) \
 :"m" (*(volatile type __force *)addr) barrier); return ret; }

-#define build_mmio_write(name, size, type, reg, barrier) \
-static inline void name(type val, volatile void __iomem *addr) \
-{ asm volatile("mov" size " %0,%1": :reg (val), \
-"m" (*(volatile type __force *)addr) barrier); }
+#define build_mmio_write(name, size, type, reg, barrier)	\
+static inline void name(type val, volatile void __iomem *mem)	\
+{ asm volatile("mov" size " %1,%0"				\
+		: "+m" (*(volatile type __force *)mem)		\
+		: reg (val) barrier); }

 build_mmio_read(readb, "b", unsigned char, "=q", :"memory")
 build_mmio_read(readw, "w", unsigned short, "=r", :"memory")
-- 
2.21.0

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.