linux-ext4 - Re: xattr corruption issue on ext2fs generated filesystems

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:	Wed, 10 Feb 2016 10:20:52 -0800
From:	Darren Hart <dvhart@...ux.intel.com>
To:	Richard Purdie <richard.purdie@...uxfoundation.org>,
	linux-ext4@...r.kernel.org
Cc:	Ted Ts'o <tytso@....edu>,
	"Darrick J. Wong" <darrick.wong@...cle.com>
Subject: Re: xattr corruption issue on ext2fs generated filesystems

On Sat, 2016-02-06 at 11:23 +0000, Richard Purdie wrote:
> I'm using the -d option of mke2fs to construct a filesystem, I'm
> seeing
> that some xattrs are being corrupted. The filesystem builds with no
> errors but when mounted by the kernel, I see errors like
> "security.ima:
> No such attribute". The strace from such a failure is:


Interesting. +Ted and +Darrick who helped us merge the -d argument
originally.


> mmap(NULL, 26258, PROT_READ, MAP_SHARED, 3, 0) = 0x7fdb36a8c000
> close(3)                    = 0
> getrlimit(RLIMIT_NOFILE, {rlim_cur=1024, rlim_max=64*1024}) = 0
> lstat("mnt/foobar", {st_mode=S_IFREG|0755, st_size=1, ...}) = 0
> listxattr("mnt/foobar", NULL, 0) = 30
> listxattr("mnt/foobar", "security.SMACK64\0security.ima\0", 256) = 30
> getxattr("mnt/foobar", "security.SMACK64", 0x0, 0) = 1
> getxattr("mnt/foobar", "security.SMACK64", "_", 256) = 1
> fstat(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 13), ...}) = 0
> mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
> 0) = 0x7fdb36a8b000
> write(1, "# file: mnt/foobar\n", 19# file: mnt/foobar) = 19
> write(1, "security.SMACK64=\"_\"\n", 21security.SMACK64="_") = 21
> getxattr("mnt/foobar", "security.ima", 0x0, 0) = -1 ENODATA (No data
> available)
> write(2, "mnt/foobar: ", 12mnt/foobar: ) = 12
> write(2, "security.ima: No such attribute\n", 32security.ima: No such
> attribute) = 32= 32
> 
> so the attribute is there but the kernel gives ENODATA when trying
> to read it.
> 
> http://www.nongnu.org/ext2-doc/ext2.html#CONTRIB-EXTENDED-ATTRIBUTES
> co
> ntains the small snippet that " The entry descriptors are sorted by
> attribute name, so that two extended attribute blocks can be compared
> efficiently. ". It doesn't specify what kind of sort.
> 
> Looking at ext2fs, there is some sorting code through the qsort call
> using attr_compare() but it doesn't match what the kernel is doing in
>  ext4_xattr_find_entry().
>
> I put together this quick patch to test my theory that this causing
> the
> problem:
> 
> 
> This makes my filesystems work.
> 
> Is this a bug? I'm assuming ext2fs shouldn't generate filesystems the
> kernel can't read? Is the above the correct fix?
> 

Reviewing the kernel ext4_attr_find_entry():

...
		if (cmp <= 0 && (sorted || cmp == 0))
			break;
	}
	*pentry = entry;
	if (!cmp && ext4_xattr_check_entry(entry, size))
		return -EFSCORRUPTED;
	return cmp ? -ENODATA : 0;
...

It would seem that a different sorting algorithm would result in the
kernel interpreting the FS to be corrupted.


> Cheers,
> 
> Richard
> ---
> 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
> 
> Index: git/lib/ext2fs/ext_attr.c
> ===================================================================
> --- git.orig/lib/ext2fs/ext_attr.c
> +++ git/lib/ext2fs/ext_attr.c
> @@ -258,6 +258,7 @@ static struct ea_name_index ea_names[] =
>  static int attr_compare(const void *a, const void *b)
>  {
>  	const struct ext2_xattr *xa = a, *xb = b;
> +	size_t len;
>  
>  	if (xa->name == NULL)
>  		return +1;
> @@ -267,7 +268,11 @@ static int attr_compare(const void *a, c
>  		return -1;
>  	else if (!strcmp(xb->name, "system.data"))
>  		return +1;
> -	return 0;
> +	len = strlen(xa->name) - strlen(xb->name);
> +	if (len)
> +		return len;

I *think* the index and len comparisons in the kernel are simply
optimizations to avoid the memcmp, but to properly sort them here, I
think you can drop the len block above and just return the strcmp
below.

Ted, Darrick?

> +
> +	return strcmp(xa->name, xb->name);
>  }
>  
>  static const char *find_ea_prefix(int index)
--
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