linux-kernel - Re: [PATCH 1/2] ecryptfs: Improve metatdata read failure logging

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: <4F0F0E10.1010107@canonical.com>
Date:	Thu, 12 Jan 2012 17:45:04 +0100
From:	Tim Gardner <rtg.canonical@...il.com>
To:	Tyler Hicks <tyler.hicks@...onical.com>
CC:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org, ecryptfs@...r.kernel.org
Subject: Re: [PATCH 1/2] ecryptfs: Improve metatdata read failure logging

On 01/12/2012 01:00 PM, Tyler Hicks wrote:
> On 2012-01-11 18:00:41, Tim Gardner wrote:
>> There are 3 read failure cases in ecryptfs_read_metadata(), but only 2
>> of them are uniquely noted by kernel log messages. This patch
>> identifies and logs each read failure case. It also correctly interprets
>> a negative return value from ecryptfs_read_lower().
>
> I'm not sure that the negative return value was incorrectly interpreted.
> See below.
>
>>
>> Removes unnecessary variable initialization.
>>
>> Cc: linux-kernel@...r.kernel.org
>> Cc: stable@...r.kernel.org
>> Cc: Tyler Hicks<tyler.hicks@...onical.com>
>> Signed-off-by: Tim Gardner<tim.gardner@...onical.com>
>> ---
>>   fs/ecryptfs/crypto.c |   17 +++++++++++------
>>   1 files changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
>> index d3c8776..ac063bd 100644
>> --- a/fs/ecryptfs/crypto.c
>> +++ b/fs/ecryptfs/crypto.c
>> @@ -1590,8 +1590,8 @@ int ecryptfs_read_and_validate_xattr_region(struct dentry *dentry,
>>    */
>>   int ecryptfs_read_metadata(struct dentry *ecryptfs_dentry)
>>   {
>> -	int rc = 0;
>> -	char *page_virt = NULL;
>> +	int rc;
>> +	char *page_virt;
>>   	struct inode *ecryptfs_inode = ecryptfs_dentry->d_inode;
>>   	struct ecryptfs_crypt_stat *crypt_stat =
>>   	&ecryptfs_inode_to_private(ecryptfs_inode)->crypt_stat;
>> @@ -1611,10 +1611,15 @@ int ecryptfs_read_metadata(struct dentry *ecryptfs_dentry)
>>   	}
>>   	rc = ecryptfs_read_lower(page_virt, 0, crypt_stat->extent_size,
>>   				 ecryptfs_inode);
>> -	if (rc>= 0)
>> -		rc = ecryptfs_read_headers_virt(page_virt, crypt_stat,
>> -						ecryptfs_dentry,
>> -						ECRYPTFS_VALIDATE_HEADER_SIZE);
>> +	if (rc<  0) {
>> +		printk(KERN_ERR "%s: Could not read %u bytes\n",
>> +			__func__, crypt_stat->extent_size);
>> +		goto out;
>> +	}
>
> This may break things a bit for users that use the
> ecryptfs_xattr_metadata mount option (which stores the metadata in an
> xattr, rather than the header of the file). A failure to read the lower
> file here doesn't matter because the metadata is likely stored in an
> xattr, which will be found with the ecryptfs_read_xattr_region() call
> below.
>
> I've never liked that ecryptfs potentially looks in both the header and
> the xattr for metadata, but that's how it has been since the very early
> days of eCryptfs. I've wanted to change this but there is the potential
> to break things for users who have a mixture of files with metadata in
> the header and in the header.
>
> Maybe we just go whole hog for 3.3 and only look in either the header or
> xattr for metadata, depending on whether or not ecryptfs_xattr_metadata
> is used?
>
> I could live with that, but I would definitely not want something like
> that to go to the stable kernel. Any thoughts?
>
> Tyler
>

I think you're right. Given the way the metadata checks are coded I 
think its impossible to disambiguate the 2 failure scenarios. However, 
its not really relevant that we know which one failed, only the inode of 
the metadata read that _did_ fail is important.

How about this much simpler patch that is also suitable for stable ? 
Tested on 3.2.

rtg
-- 
Tim Gardner tim.gardner@...onical.com

View attachment "0001-ecryptfs-Improve-metadata-read-failure-logging.patch" of type "text/x-patch" (2573 bytes)