Re: [PATCH 2/2] ecryptfs: don't allow mmap when the lower fs doesn't support it

[Tyler Hicks <tyhicks@canonical.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2219 bytes --]

On 07/05/2016 04:57 PM, Jann Horn wrote:
> On Tue, Jul 05, 2016 at 05:32:30PM -0400, jeffm@suse.com wrote:
>> From: Jeff Mahoney <jeffm@suse.com>
>>
>> There are legitimate reasons to disallow mmap on certain files, notably
>> in sysfs or procfs.  We shouldn't emulate mmap support on file systems
>> that don't offer support natively.
>>
>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>> ---
>>  fs/ecryptfs/file.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
>> index 7000b96..4aaa656 100644
>> --- a/fs/ecryptfs/file.c
>> +++ b/fs/ecryptfs/file.c
>> @@ -169,6 +169,20 @@ out:
>>  	return rc;
>>  }
>>  
>> +
>> +static int ecryptfs_mmap(struct file *file, struct vm_area_struct *vma)
>> +{
>> +	struct dentry *dentry = ecryptfs_dentry_to_lower(file_dentry(file));
>> +	/*
>> +	 * Don't allow mmap on top of file systems that don't support it
>> +	 * natively.  If FILESYSTEM_MAX_STACK_DEPTH > 2 or ecryptfs
>> +	 * allows recursive mounting, this will need to be extended.
> 
> Or if another new stacking filesystem appears whose f_op->mmap just forwards
> to lower_f_op->mmap - but thinking about it, in that scenario, my patch
> would stop working, too.
> 
> At this point, I dislike both this patch and my own one because of their
> lack of robustness. Well, at least e54ad7f1ee263ffa5a2de9c609d58dfa27b21cd9
> should be solid. :/

I agree that neither approach provides long term protection from the
attack but they fix the immediate issue at hand. Therefore, I think we
need to move forward with what we have now.

Tyler

> 
> 
>> +	 */
>> +	if (!d_inode(dentry)->i_fop->mmap)
>> +		return -ENODEV;
>> +	return generic_file_mmap(file, vma);
>> +}
>> +
>>  /**
>>   * ecryptfs_open
>>   * @inode: inode speciying file to open
>> @@ -403,7 +417,7 @@ const struct file_operations ecryptfs_main_fops = {
>>  #ifdef CONFIG_COMPAT
>>  	.compat_ioctl = ecryptfs_compat_ioctl,
>>  #endif
>> -	.mmap = generic_file_mmap,
>> +	.mmap = ecryptfs_mmap,
>>  	.open = ecryptfs_open,
>>  	.flush = ecryptfs_flush,
>>  	.release = ecryptfs_release,
>> -- 
>> 2.7.1
>>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]