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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD+ocbx9STMGrE0xkHtR8J_c_TgMEz1A6MmNOQyrQtakoZjq3Q@mail.gmail.com>
Date:   Tue, 4 May 2021 02:40:08 -0700
From:   harshad shirwadkar <harshadshirwadkar@...il.com>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     "Theodore Ts'o" <tytso@....edu>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>,
        Harshad Shirwadkar <harshads@...gle.com>
Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses

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?

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

// access local.t_flags instead of unaligned

Here are the failures ubsan that I still see:

recovery.c:243:24: runtime error: member access within misaligned
address 0x000001c18fae for type 'journal_block_tag_t' (aka 'struct
journal_block_tag_s'), which requires 4 byte alignment
0x000001c18fae: note: pointer points here
 00 00 00 00 00 01  e0 01 ac 26 00 02 00 00  00 00 01 03 ac 26 00 02
00 00 00 01 e0 02 ac 26  00 02
             ^
Thanks,
Harshad

On Mon, May 3, 2021 at 11:33 PM Andreas Dilger <adilger@...ger.ca> wrote:
>
> On May 3, 2021, at 9:10 PM, Theodore Ts'o <tytso@....edu> wrote:
> >
> > The on-disk format for the ext4 journal can have unaigned 32-bit
> > integers.  This can happen when replaying a journal using a obsolete
> > checksum format (which was never popularly used, since the v3 format
> > replaced v2 while the metadata checksum feature was being stablized),
> > and in the fast commit feature (which landed in the 5.10 kernel,
> > although it is not enabled by default).
> >
> > This commit fixes the following regression tests on some platforms
> > (such as running 32-bit arm architectures on a 64-bit arm kernel):
> > j_recover_csum2_32bit, j_recover_csum2_64bit, j_recover_fast_commit.
> >
> > https://github.com/tytso/e2fsprogs/issues/65
>
> Minor style comments inline.
>
> > Addresses-Debian-Bug: #987641
> > Signed-off-by: Theodore Ts'o <tytso@....edu>
> > ---
> > e2fsck/journal.c                   | 41 ++++++++++++++++++++---------
> > e2fsck/recovery.c                  | 42 +++++++++++++++++++++++++-----
> > tests/j_recover_fast_commit/script |  1 -
> > 3 files changed, 65 insertions(+), 19 deletions(-)
> >
> > diff --git a/e2fsck/journal.c b/e2fsck/journal.c
> > index a425bbd1..2231b811 100644
> > --- a/e2fsck/journal.c
> > +++ b/e2fsck/journal.c
> > @@ -344,10 +361,10 @@ static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh,
> >                                               offsetof(struct ext4_fc_tail,
> >                                               fc_crc));
> >                       jbd_debug(1, "tail tid %d, expected %d\n",
> > -                                     le32_to_cpu(tail->fc_tid),
> > +                                     get_le32(&tail->fc_tid),
> >                                       expected_tid);
> > -                     if (le32_to_cpu(tail->fc_tid) == expected_tid &&
> > -                             le32_to_cpu(tail->fc_crc) == state->fc_crc) {
> > +                     if (get_le32(&tail->fc_tid) == expected_tid &&
> > +                             get_le32(&tail->fc_crc) == state->fc_crc) {
>
> (style) better to align continued line after '(' on previous line?  That way
> it can be distinguished from the next (body) line more easily
>
> >                               state->fc_replay_num_tags = state->fc_cur_tag;
> >                       } else {
> >                               ret = state->fc_replay_num_tags ?
> > @@ -357,12 +374,12 @@ static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh,
> >                       break;
> >               case EXT4_FC_TAG_HEAD:
> >                       head = (struct ext4_fc_head *)ext4_fc_tag_val(tl);
> > -                     if (le32_to_cpu(head->fc_features) &
> > +                     if (get_le32(&head->fc_features) &
> >                               ~EXT4_FC_SUPPORTED_FEATURES) {
>
> (style) same
>
> >                               ret = -EOPNOTSUPP;
> >                               break;
> >                       }
> > -                     if (le32_to_cpu(head->fc_tid) != expected_tid) {
> > +                     if (get_le32(&head->fc_tid) != expected_tid) {
> >                               ret = -EINVAL;
> >                               break;
> >                       }
>
>
> Cheers, Andreas
>
>
>
>
>