linux-ext4 - Re: [PATCH e2fsprogs-next] Fix extent flag validity tests in pass1 on big endian boxes.

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]

Date:	Mon, 24 Mar 2008 19:56:23 -0500
From:	Eric Sandeen <sandeen@...hat.com>
To:	Andreas Dilger <adilger@....com>
CC:	ext4 development <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH e2fsprogs-next] Fix extent flag validity tests in pass1
 on big endian boxes.

Andreas Dilger wrote:
> On Mar 24, 2008  17:13 -0500, Eric Sandeen wrote:
>> Extent data is shared with the i_block[] space in the inode,
>> but it is always swapped on access, not when the inode is read.
>>
>> In e2fsck/pass1.c we must be careful when checking validity
>> of the extents flag on the inode.  If the flag was set when
>> the inode was read & swapped, then the extents data itself
>> (in ->i_block[]) was NOT swapped, so testing for a valid
>> extent header requires some swapping first.  Then, if we
>> ultimately set the extents flag, all of i_block[] must be
>> re/un-swapped.
> 
> This seems pretty awkward for any other users of the library.  Having the
> i_block[] array NOT be swabbed if it is an extent file means that every
> place in the code which is accessing this array also needs to do the
> swabbing itself.  This would break the abstraction that the in-memory
> inode is in host-endian order, and also forces every application to
> understand the difference between extent- and non-extent-mapped inodes,
> and the on-disk byte order.  Ugh.

Well, I tend to agree, but I thought that's how it was implemented
throughout the library... as well as in the kernel?

        if (unlikely(le16_to_cpu(eh->eh_depth) != depth)) {
                error_msg = "unexpected eh_depth";
                goto corrupted;
        }

...

static inline unsigned short ext_depth(struct inode *inode)
{
        return le16_to_cpu(ext_inode_hdr(inode)->eh_depth);
}

etc... or am I missing something silly...

-Eric

> IMHO, it would be better to swab the i_block[] array in
> ext2fs_swap_inode_full() if the EXTENTS_FL is set, and in the rare
> case of e2fsck needing to clear that flag then it should un-swap (if
> needed), clear the flag, and re-swap (if needed).  This will very rarely
> happen, I think.  Note that the Lustre extents patches did NOT do the
> swap+clear_swap operation when clearing the extents flags because we
> don't have any big-endian server systems (our PPC testing is limited to
> userspace "make check"), though I think that is the right thing to do.
> 
> 
> Cheers, Andreas
> --
> Andreas Dilger
> Sr. Staff Engineer, Lustre Group
> Sun Microsystems of Canada, Inc.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html