linux-kernel - Re: [PATCH 07/10] crypto: Use ARCH_DMA_MINALIGN instead of ARCH_KMALLOC

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:   Sun, 17 Apr 2022 17:29:01 +0100
From:   Catalin Marinas <catalin.marinas@....com>
To:     Herbert Xu <herbert@...dor.apana.org.au>
Cc:     Ard Biesheuvel <ardb@...nel.org>, Will Deacon <will@...nel.org>,
        Marc Zyngier <maz@...nel.org>, Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Linux Memory Management List <linux-mm@...ck.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH 07/10] crypto: Use ARCH_DMA_MINALIGN instead of
 ARCH_KMALLOC_MINALIGN

On Sun, Apr 17, 2022 at 04:43:33PM +0800, Herbert Xu wrote:
> On Sun, Apr 17, 2022 at 09:38:40AM +0100, Catalin Marinas wrote:
> > I don't think we need to do anything here. A structure like:
> > 
> > struct x {
> > 	char y;
> > 	char z[] CRYPTO_MINALIGN_ATTR;
> > };
> > 
> > is already of size 128. Without CRYPTO_MINALIGN_ATTR, its size would be
> > 1 but otherwise the whole structure inherits the alignment of its
> > member and this translates into an aligned size.
> 
> No we should not lie to the compiler,

We won't if we ensure that a structure with sizeof() >= 128 is aligned
to 128.

> we have code elsewhere
> that uses the alignment to compute the amount of extra padding
> needed to create greater padding.  If CRYPTO_MINALIGN is misleading
> then that calculation will fall apart.

There is no direct CRYPTO_MINALIGN use for any extra padding AFAICT.
There is an indirect use via __alignof__(ctx) like in
crypto_tfm_ctx_alignment() but IIUC in that case CRYPTO_MINALIGN is a
statement of what you want rather than what you get from kmalloc(). So
if you want 128 alignment of tfm->__crt_ctx, you should say so by either
changing the attribute to __aligned(ARCH_DMA_MINALIGN) or keeping
CRYPTO_MINALIGN as 128.

There is the union padding that Ard suggested but I don't think it buys
us much, the __aligned(ARCH_DMA_MINALIGN) gives you the padding and the
kmalloc() rules the alignment (subject to removing kmalloc-192). The
code that allocates these would need to place the structure aligned
anyway, irrespective of whether we use the padding or the
__aligned(ARCH_DMA_MINALIGN).

> So keep CRYPTO_MINALIGN at whatever alignment you lower kmalloc
> to, and then add the padding you need to separate the Crypto API
> bits from the context.  In fact, that is exactly what cra_alignmask
> is supposed to do.

I disagree on the cra_alignmask intention here, though I may be wrong as
I did not write the code. Yes, you could make it ARCH_DMA_MINALIGN
everywhere but IMHO that's not what it is supposed to do. The driver
only knows about the bus master alignment requirements (typically
smaller than a cache line) while the architecture-defined
ARCH_DMA_MINALIGN cares about the non-coherent DMA requirements.

> Sure we currently limit the maximum alignment to 64 bytes because
> of stack usage but we can certainly look into increasing it as
> that's what you're doing here anyway.

I'm not actually increasing CRYPTO_MINALIGN, just trying to keep it as
the current value of 128 for arm64.

-- 
Catalin