linux-ext4 - Re: [PATCH 1/2] ext4: Calculate and verify inode checksums

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: <C1228346-6174-4AD9-8384-E60197C7C85E@dilger.ca>
Date:	Fri, 8 Apr 2011 16:49:54 -0600
From:	Andreas Dilger <adilger.kernel@...ger.ca>
To:	djwong@...ibm.com
Cc:	Theodore Ts'o <tytso@....edu>,
	linux-ext4 <linux-ext4@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] ext4: Calculate and verify inode checksums

On 2011-04-08, at 1:12 PM, Darrick J. Wong wrote:
> On Fri, Apr 08, 2011 at 02:58:27AM -0600, Andreas Dilger wrote:
>> On 2011-04-06, at 4:45 PM, Darrick J. Wong wrote:
>>> @@ -617,7 +617,7 @@ struct ext4_inode {
>>> 		} masix2;
>>> 	} osd2;				/* OS dependent 2 */
>>> 	__le16	i_extra_isize;
>>> -	__le16	i_pad1;
>>> +	__le16  i_checksum;		/* crc16(sb_uuid+inodenum+inode) */
>> 
>> In previous discussions, we thought about using part/all of the l_i_reserved2
>> field to store the inode checksum.  The inode checksum is important enough to
>> warrant a field in the core inode, so that it also works on upgraded
>> filesystems.
> 
> Hmm, yes, I better like using that field too, it'll simplify the code.  Does
> anyone know which of the s_creator_os codes map to the Linux format?  I'm going
> to guess 1 and 2 don't... for now I suppose the mount code can check for a
> Linux creator code if the inode checksum feature is set, and fail the mount if
> it finds 1 or 2.  I'm not sure which version of the osd2 union FreeBSD or
> "Lites" use.

Actually, the e2fsprogs code has already removed the "masix" part of the union years ago, and I'm not even sure why the "hurd" part of the union remains in the kernel code.  I think only Hurd is using the "h_i_author" field, so I suspect all that is needed is to refuse tune2fs to enable EXT4_FEATURE_RO_COMPAT_INODE_CSUM if s_creator_os == EXT2_OS_HURD (1).

The kernel shouldn't really care about this field - if the checksum feature isn't enabled, then this field will be ignored, and if the checksum feature IS enabled it could only have been done by mke2fs, or tune2fs which should check if it is allowed based on s_creator_os.

>>> +static __le16 ext4_inode_csum(struct inode *inode, struct ext4_inode *raw)
>>> +{
>>> +	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
>>> +	struct ext4_inode_info *ei = EXT4_I(inode);
>>> +	int offset = offsetof(struct ext4_inode, i_checksum);
>>> +	__le32 inum = cpu_to_le32(inode->i_ino);
>>> +	__u16 crc = 0;
>>> +
>>> +	if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
>>> +		EXT4_FEATURE_RO_COMPAT_INODE_CSUM) &&
>>> +	    le16_to_cpu(raw->i_extra_isize) >= 4) {
>> 
>> If this field remains in the large part of the inode, then this check is
>> incorrect.  i_extra_isize is itself only valid if (s_inode_size is >
>> EXT4_GOOD_OLD_INODE_SIZE), otherwise it points at the next inode's i_mode.
>> 
>> Also, instead of hard-coding ">= 4" here, it would be better to use
>> EXT4_FITS_IN_INODE(raw, EXT4_I(inode), i_checksum).  Probably no reason to
>> put "ei" on the stack at all.
> 
> As always, thank you for the feedback!
> 
> --D


Cheers, Andreas





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