linux-ext4 - Re: [PATCH] misc: copy extended attributes in populate

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: <CAJTo0LZL4JMsza7XWkTjT0rPXvbhiYwbgXXJj9AWdohP6TiK7w@mail.gmail.com>
Date:	Mon, 30 Jun 2014 19:59:03 +0100
From:	"Burton, Ross" <ross.burton@...el.com>
To:	"Darrick J. Wong" <darrick.wong@...cle.com>
Cc:	linux-ext4@...r.kernel.org, Darren Hart <dvhart@...ux.intel.com>,
	tytso <tytso@....edu>, Robert Yang <liezhi.yang@...driver.com>
Subject: Re: [PATCH] misc: copy extended attributes in populate_fs

On 30 June 2014 19:43, Darrick J. Wong <darrick.wong@...cle.com> wrote:
>> +     retval = ext2fs_xattrs_open(fs, ino, &handle);
>
> If the FS does not have at least one of the inline_data or ext_attr features
> turned on, the ext2fs_xattrs_open call returns EXT2_ET_MISSING_EA_FEATURE,
> which aborts the whole operation.  Is that ok?

Good point.

I'm currently dithering over whether this should silently do nothing,
or warn per file, or have an already-warned boolean to avoid spamming
the user.

>> +     if (retval) {
>> +             com_err(__func__, errno, "while opening inode %u", ino);
>
> retval, not errno.

Literally just squashing some commits for these error paths which went wrong.

>> +             return errno;
>> +     }
>> +
>> +     list = malloc(size);
>
> What happens if malloc fails?

You're doomed?

What's e2fsprog's policy on OOM - I see some instances of checked
malloc() that exit() promptly, others that simply return 0 and hope
for the best, and some that use fputs() and gettext() - but if we're
OOM then that's even more likely to fail.

I'll follow the official line, but what that line is isn't clear from
a sample of malloc() calls in the source.

>> +     size = llistxattr(filename, list, size);
>
> What if this second call should fail for some reason?  Shouldn't we stop?

Yes, also fixed locally.

>> +
>> +     for (i = 0; i < size; i += strlen(&list[i]) + 1) {
>> +             const char *name = &list[i];
>> +             char *value;
>> +
>> +             value_size = getxattr(filename, name, NULL, 0);
>
> What if getxattr returns -1?

Should bail, will fix.

>> +             value = malloc(value_size);
>> +             value_size = getxattr(filename, name, value, value_size);
>
> Same complaints about not checking malloc/getxattr return values.

Agreed.

>> +     retval = ext2fs_xattrs_close(&handle);
>> +     if (retval)
>> +             com_err(__func__, errno, "while closing inode %u", ino);
>
> retval, not errno.

[sags head in shame]

Next time I'm patching at 10pm I'll remember to review the patch the
next morning before sending...

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