linux-ext4 - Re: problem in ext2fs_get_next_inode

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] [day] [month] [year] [list]

Message-ID: <20160219142431.GA20458@thunk.org>
Date:	Fri, 19 Feb 2016 09:24:31 -0500
From:	Theodore Ts'o <tytso@....edu>
To:	Andreas Dilger <adilger@...ger.ca>
Cc:	"linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>
Subject: Re: problem in ext2fs_get_next_inode_full() ?

On Thu, Feb 18, 2016 at 03:30:35PM -0700, Andreas Dilger wrote:
> > So if the inode is being swabbed then it handles the full inode size, but
> > if it is not being swabbed (the common case) it appears that it is only
> > copying the small inode into "*inode" using a struct assignment.  This
> > appears like it would be dropping the large inode data, but I'm not sure
> > if or when this "extra_bytes" case is hit.  The "else" clause appears to
> > copy the requested (full) inode size properly via "memcpy(..., bufsize)".
> > 
> > Should the struct assignment be changed similarly to use memcpy()?
> 
> To follow up on my own email - I also see struct ext2_inode_cache_ent is
> only caching the small inode, and not a large inode.  This would seem to
> potentially cause loss of the large inode data if the inode cache is
> used by tools like resize2fs or others that move around inodes?

Those are both bugs, and I'm guessing they were added when we added
metadata checksuming, as they aren't a problem in the maint branch.
The inode cache should *only* be used if we are reading the small
inode (which is the common case; we only need the full inode if we are
moving the inode around or if we need to access the xattrs).  And
indeed we do that in the maint branch:

	/* Check to see if it's in the inode cache */
	if (bufsize == sizeof(struct ext2_inode)) {
		/* only old good inode can be retrieved from the cache */
		for (i=0; i < fs->icache->cache_size; i++) {
			if (fs->icache->cache[i].ino == ino) {
				*inode = fs->icache->cache[i].inode;
				return 0;
			}
		}
	}

Unfortunately this check got removed in the next branch, and I missed
it in my code reviews.

We should probably have some unit tests to make sure we don't regress
here again, and probably make the comments a bit more explicit.

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