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  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:   Tue, 4 May 2021 17:10:34 -0400
From:   "Theodore Ts'o" <>
To:     Eric Biggers <>
Cc:     harshad shirwadkar <>,
        Andreas Dilger <>,
        Ext4 Developers List <>,
        Harshad Shirwadkar <>
Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned

On Tue, May 04, 2021 at 01:45:02PM -0700, Eric Biggers wrote:
> No, 'flags = unaligned_ptr->t_flags' is fine, provided that unaligned_ptr is a
> pointer to a struct with the packed attribute.  What -Waddress-of-packed-member
> will warn about is if you do something like &unaligned_ptr->t_flags to get a
> pointer directly to the t_flags field, as such pointers can then be incorrectly
> used for misaligned accesses.

Yeah, the warnings appear if you apply my patch, and then *also* add
the __attribute__((__packed__)) annotation.  If you revert my patch,
and then only add the __packed__ annotation, there are no warnings,
because we're no longer taking the address of the field with a packed

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.)

However, if you take an address of a packed memory,

	short *p = &packed_ptr->field;

.. and then later derference that pointer, the C compiler can't know
that it needs to generate the magic unaligned derferencing code when
it dereferences that pointer.  So that's why that warning is there.

But if you just add __packed__ attribute, without my proposed patch,
we aren't taking the &packed_ptr->field anywhere in e2fsprogs, so
we're fine.

> 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

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.

					- Ted

Powered by blists - more mailing lists