linux-ext4 - Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses

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: <YJG9CjVXKkha57RU@gmail.com>
Date:   Tue, 4 May 2021 14:30:50 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Theodore Ts'o <tytso@....edu>
Cc:     harshad shirwadkar <harshadshirwadkar@...il.com>,
        Andreas Dilger <adilger@...ger.ca>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>,
        Harshad Shirwadkar <harshads@...gle.com>
Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned
 accesses

On Tue, May 04, 2021 at 05:10:34PM -0400, Theodore Ts'o wrote:
> 
> Basically, what gcc (and presumably clang) is doing is it is special
> casing packed_ptr->field so that the compiled code will work
> regardless of the alignment of packed_ptr.  This isn't documented
> anywhere, but it apparently is the case.  (I had assumed that it would
> only generate unaligned access for those fields that are not aligned
> if the structure started on an aligned boundary.)

I don't think it's related to the pointer dereference per se, but rather the
compiler assigns an alignment of 1 to all fields in a packed struct (even the
field at the beginning of the struct).  If you had a packed struct as a global
variable and did 'packed_struct.field', the behavior would be the same.

> > If we really don't want to use __attribute__((packed)) that is fine, but then
> > we'll need to remember to use an unaligned accessor *every* field access (except
> > for bytes), which seems harder to me -- and the compiler won't warn when one of
> > these is missing.  (They can only be detected at runtime using UBSAN.)
> 
> One reason not to use the __packed__ attribute is that there are cases
> where people attempt to build e2fsprogs on non-gcc/non-clang binaries.
> At one point FreeBSD was trying to use pcc to build e2fsprogs IIRC.
> And certainly there are people who try to build e2fsprogs on MSVC on
> Windows.

Is that really true, given that e2fsprogs already uses a lot of gcc/clang
extensions, including __attribute((packed))__ already?

> So maybe the memcpy to a local copy is the better way to go, and
> hopefully the C compiler will optimize away the local copy on
> architectures where it is safe to do so.  And in the unlikely case
> that it is a performance bottleneck, we could add a -DUBSAN when
> configure --enable-ubsan is in force, which switches in the memcpy
> when only when ubsan is enabled.

These days the memcpy() approach does get optimized properly.  armv6 and armv7
with gcc used to be a notable exception, but it got fixed in gcc 6
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67366).

- Eric