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]

Date:   Tue, 4 May 2021 09:46:19 -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 09:49:15AM -0400, Theodore Ts'o wrote:
> On Tue, May 04, 2021 at 02:40:08AM -0700, harshad shirwadkar wrote:
> > Hi Ted,
> > 
> > Thanks for the patch. While I now see that these accesses are safe,
> > ubsan still complains about it the dereferences not being aligned.
> > With your changes, the way we read journal_block_tag_t is now safe.
> > But IIUC, ubsan still complains mainly because we still pass the
> > pointer as "&tag->t_flags" and at which point ubsan thinks that we are
> > accessing member t_flags in an aligned way. Is there a way to silence
> > these errors?
> 
> Yeah, I had noticed that.  I was thinking perhaps of doing something
> like casting the pointer to void * or char *, and then adding offsetof
> to work around the UBSAN warning.  Or maybe asking the compiler folks
> if they can make the UBSAN warning smarter, since what we're doing
> should be perfectly safe. 

This does seem to be an UBSAN bug, although both gcc and clang report this same
error, which is odd...  Dereferencing a misaligned field would be undefined
behavior, but just taking its address isn't (AFAIK).

> 
> > 
> > I was wondering if it makes sense to do something like this for known
> > unaligned structures:
> > 
> > journal_block_tag_t local, *unaligned;
> > ...
> > memcpy(&local, unaligned, sizeof(&local));
> 
> I guess that would work too.  The extra memory copy is unfortunate,
> although I suspect the performance hit isn't measurable, and journal
> replay isn't really a hot path in either the kernel or e2fsprogs.
> (Note that want to keep recovery.c in sync between the kernel and
> e2fsprogs, so whatever we do needs to be something we're happy with in
> both places.)
> 

Modern compilers will optimize out the memcpy().

However, wouldn't it be easier to just add __attribute__((packed)) to the
definition of struct journal_block_tag_t?

- Eric