linux-kernel - Re: [PATCH 1/7] asm-generic/io.h: add io{read,write}64 accessors

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]

Date:	Thu, 05 May 2016 00:35:50 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Horia Geantă <horia.geanta@....com>
Cc:	Herbert Xu <herbert@...dor.apana.org.au>,
	linux-crypto@...r.kernel.org, linux-arch@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	Scott Wood <scott.wood@....com>,
	Alexandru Porosanu <alexandru.porosanu@....com>,
	Tudor Ambarus <tudor-dan.ambarus@....com>,
	Cristian Stoica <cristian.stoica@....com>,
	Vineet Gupta <Vineet.Gupta1@...opsys.com>
Subject: Re: [PATCH 1/7] asm-generic/io.h: add io{read,write}64 accessors

On Wednesday 04 May 2016 20:16:19 Horia Geantă wrote:
> @@ -625,6 +645,16 @@ static inline u32 ioread32be(const volatile void __iomem *addr)
>  }
>  #endif
>  
> +#ifdef CONFIG_64BIT
> +#ifndef ioread64be
> +#define ioread64be ioread64be
> +static inline u64 ioread64be(const volatile void __iomem *addr)
> +{
> +       return __be64_to_cpu(__raw_readq(addr));
> +}
> +#endif
> +#endif /* CONFIG_64BIT */
> +
>  #ifndef iowrite16be
>  #define iowrite16be iowrite16be
>  static inline void iowrite16be(u16 value, void volatile __iomem *addr)
> @@ -641,6 +671,16 @@ static inline void iowrite32be(u32 value, volatile void __iomem *addr)
>  }
>  #endif
>  
> +#ifdef CONFIG_64BIT
> +#ifndef iowrite64be
> +#define iowrite64be iowrite64be
> +static inline void iowrite64be(u64 value, volatile void __iomem *addr)
> +{
> +       __raw_writeq(__cpu_to_be64(value), addr);
> +}
> +#endif
> +#endif /* CONFIG_64BIT */
> +
> 

I just noticed that these two are both a bit wrong, but they copy the
mistake that already exists in the 16 and 32 bit versions: If an
architecture overrides readq/writeq to have barriers but does not override
ioread64be/iowrite64be, this will lack the barriers and behave differently
from the little-endian version. I think the only affected architecture
is ARC, since ARM and ARM64 both override the big-endian accessors to
have the correct barriers, and all others don't use barriers at all.

Maybe you can add a patch before this one to replace the 16/32-bit accessors
with ones that do a

static inline void iowrite32be(u32 value, volatile void __iomem *addr)
{
	writel(swab32(value), addr);
}

This will lead to a double-swap on architectures that don't override it,
but it will work correctly on all architectures without them having
to override the big-endian accessors.

Aside from that, the patch looks fine.

	Arnd