linux-kernel - Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

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: <20210107221446.GS1551@shell.armlinux.org.uk>
Date:   Thu, 7 Jan 2021 22:14:46 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Arnd Bergmann <arnd@...nel.org>
Cc:     Theodore Ts'o <tytso@....edu>, Will Deacon <will@...nel.org>,
        linux-toolchains@...r.kernel.org,
        Mark Rutland <mark.rutland@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory
 ordering issues

On Thu, Jan 07, 2021 at 10:48:05PM +0100, Arnd Bergmann wrote:
> On Thu, Jan 7, 2021 at 5:27 PM Theodore Ts'o <tytso@....edu> wrote:
> >
> > On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:
> > > > The gcc bugzilla mentions backports into gcc-linaro, but I do not see
> > > > them in my git history.
> > >
> > > So, do we raise the minimum gcc version for the kernel as a whole to 5.1
> > > or just for aarch64?
> >
> > Russell, Arnd, thanks so much for tracking down the root cause of the
> > bug!
> 
> There is one more thing that I wondered about when looking through
> the ext4 code: Should it just call the crc32c_le() function directly
> instead of going through the crypto layer? It seems that with Ard's
> rework from 2018, that can just call the underlying architecture specific
> implementation anyway.

Yes, I've been wondering about that too. To me, it looks like the
ext4 code performs a layering violation by going "under the covers"
- there are accessor functions to set the CRC and retrieve it. ext4
instead just makes the assumption that the CRC value is stored after
struct shash_desc. Especially as the crypto/crc32c code references
the value using:

	struct chksum_desc_ctx *ctx = shash_desc_ctx(desc);

Not even crypto drivers are allowed to assume that desc+1 is where
the CRC is stored.

However, struct shash_desc is already 128 bytes in size on aarch64,
and the proper way of doing it via SHASH_DESC_ON_STACK() is overkill,
being strangely 2 * sizeof(struct shash_desc) + 360 (which looks like
another bug to me!)

#define HASH_MAX_DESCSIZE       (sizeof(struct shash_desc) + 360)
                                 ^^^^^^^^^^^^^^^^^^^^^^^^^
#define SHASH_DESC_ON_STACK(shash, ctx)                           \
        char __##shash##_desc[sizeof(struct shash_desc) +         \
                              ^^^^^^^^^^^^^^^^^^^^^^^^^
                HASH_MAX_DESCSIZE] CRYPTO_MINALIGN_ATTR; \
        struct shash_desc *shash = (struct shash_desc *)__##shash##_desc

So, I agree with you wrt crc32c_le(), especially as it would be more
efficient, and as the use of crc32c is already hard coded in the ext4
code - not only with crypto_alloc_shash("crc32c", 0, 0) but also with
the fixed-size structure in ext4_chksum().

However, it's ultimately up to the ext4 maintainers to decide.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!