[PATCH 1/2] Revert "ecryptfs: forbid opening files without mmap handler"

[PATCH 1/2] Revert "ecryptfs: forbid opening files without mmap handler"

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

This reverts commit 2f36db71009304b3f0b95afacd8eba1f9f046b87.

It fixed a local root exploit but also introduced a dependency on
the lower file system implementing an mmap operation just to open a file,
which is a bit of a heavy hammer.  The right fix is to have mmap depend
on the existence of the mmap handler instead.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 fs/ecryptfs/kthread.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/fs/ecryptfs/kthread.c b/fs/ecryptfs/kthread.c
index e818f5a..866bb18 100644
--- a/fs/ecryptfs/kthread.c
+++ b/fs/ecryptfs/kthread.c
@@ -25,7 +25,6 @@
 #include <linux/slab.h>
 #include <linux/wait.h>
 #include <linux/mount.h>
-#include <linux/file.h>
 #include "ecryptfs_kernel.h"
 
 struct ecryptfs_open_req {
@@ -148,7 +147,7 @@ int ecryptfs_privileged_open(struct file **lower_file,
 	flags |= IS_RDONLY(d_inode(lower_dentry)) ? O_RDONLY : O_RDWR;
 	(*lower_file) = dentry_open(&req.path, flags, cred);
 	if (!IS_ERR(*lower_file))
-		goto have_file;
+		goto out;
 	if ((flags & O_ACCMODE) == O_RDONLY) {
 		rc = PTR_ERR((*lower_file));
 		goto out;
@@ -166,16 +165,8 @@ int ecryptfs_privileged_open(struct file **lower_file,
 	mutex_unlock(&ecryptfs_kthread_ctl.mux);
 	wake_up(&ecryptfs_kthread_ctl.wait);
 	wait_for_completion(&req.done);
-	if (IS_ERR(*lower_file)) {
+	if (IS_ERR(*lower_file))
 		rc = PTR_ERR(*lower_file);
-		goto out;
-	}
-have_file:
-	if ((*lower_file)->f_op->mmap == NULL) {
-		fput(*lower_file);
-		*lower_file = NULL;
-		rc = -EMEDIUMTYPE;
-	}
 out:
 	return rc;
 }
-- 
2.7.1

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

[jeffm]

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.
+	 */
+	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

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

[Jann Horn]

[-- Attachment #1: Type: text/plain, Size: 1907 bytes --]

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. :/


> +	 */
> +	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: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Tyler Hicks]

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

On 07/05/2016 04:32 PM, 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));

This should be:

	struct file *lower_file = ecryptfs_file_to_lower(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.
> +	 */
> +	if (!d_inode(dentry)->i_fop->mmap)

and then:

	if (!lower_file->f_op->mmap)


I'll make these simple changes, add stable to cc in the patch tags, and
push to Linus.

Thanks again!

Tyler

> +		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,
> 



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

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

[Tyler Hicks]

[-- 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 --]

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

[Jeff Mahoney]

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

On 7/7/16 7:20 PM, Tyler Hicks wrote:
> On 07/05/2016 04:32 PM, 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));
> 
> This should be:
> 
> 	struct file *lower_file = ecryptfs_file_to_lower(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.
>> +	 */
>> +	if (!d_inode(dentry)->i_fop->mmap)
> 
> and then:
> 
> 	if (!lower_file->f_op->mmap)
> 
> 
> I'll make these simple changes, add stable to cc in the patch tags, and
> push to Linus.

Oh, whoops. Yeah, that's better.

Thanks,

-Jeff

> Thanks again!
> 
> Tyler
> 
>> +		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,
>>
> 
> 


-- 
Jeff Mahoney
SUSE Labs


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

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

[Henry Jensen]

Hello,

is there any new status? We are meanwhile at kernel 4.4.16 LTS and
ecryptfs is still broken there.

Regards,

Henry


On Thu, 7 Jul 2016 20:46:34 -0400
Jeff Mahoney <jeffm@suse.com> wrote:

> On 7/7/16 7:20 PM, Tyler Hicks wrote:
> > On 07/05/2016 04:32 PM, 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));  
> > 
> > This should be:
> > 
> > 	struct file *lower_file = ecryptfs_file_to_lower(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.
> >> +	 */
> >> +	if (!d_inode(dentry)->i_fop->mmap)  
> > 
> > and then:
> > 
> > 	if (!lower_file->f_op->mmap)
> > 
> > 
> > I'll make these simple changes, add stable to cc in the patch tags, and
> > push to Linus.  
> 
> Oh, whoops. Yeah, that's better.
> 
> Thanks,
> 
> -Jeff
> 
> > Thanks again!
> > 
> > Tyler
> >   
> >> +		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,
> >>  
> > 
> >   
> 
> 
> -- 
> Jeff Mahoney
> SUSE Labs
> 

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

[Jeff Mahoney]

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

On 8/2/16 8:50 AM, Henry Jensen wrote:
> Hello,
> 
> is there any new status? We are meanwhile at kernel 4.4.16 LTS and
> ecryptfs is still broken there.

The revert and the fix below, plus Tyler's cleanup, landed in 4.7.  Both
were tagged for inclusion in -stable but haven't landed there yet.  I'm
afraid that's the limit of my tracking of it.

-Jeff

> Regards,
> 
> Henry
> 
> 
> On Thu, 7 Jul 2016 20:46:34 -0400
> Jeff Mahoney <jeffm@suse.com> wrote:
> 
>> On 7/7/16 7:20 PM, Tyler Hicks wrote:
>>> On 07/05/2016 04:32 PM, 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));  
>>>
>>> This should be:
>>>
>>> 	struct file *lower_file = ecryptfs_file_to_lower(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.
>>>> +	 */
>>>> +	if (!d_inode(dentry)->i_fop->mmap)  
>>>
>>> and then:
>>>
>>> 	if (!lower_file->f_op->mmap)
>>>
>>>
>>> I'll make these simple changes, add stable to cc in the patch tags, and
>>> push to Linus.  
>>
>> Oh, whoops. Yeah, that's better.
>>
>> Thanks,
>>
>> -Jeff
>>
>>> Thanks again!
>>>
>>> Tyler
>>>   
>>>> +		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,
>>>>  
>>>
>>>   
>>
>>
>> -- 
>> Jeff Mahoney
>> SUSE Labs
>>
> 


-- 
Jeff Mahoney
SUSE Labs


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

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

[Greg KH]

On Tue, Aug 02, 2016 at 09:05:04AM -0400, Jeff Mahoney wrote:
> On 8/2/16 8:50 AM, Henry Jensen wrote:
> > Hello,
> > 
> > is there any new status? We are meanwhile at kernel 4.4.16 LTS and
> > ecryptfs is still broken there.
> 
> The revert and the fix below, plus Tyler's cleanup, landed in 4.7.  Both
> were tagged for inclusion in -stable but haven't landed there yet.  I'm
> afraid that's the limit of my tracking of it.

Yes, that's all you need to do.  I'll be queueing these up soon, I have
200+ stable patches to process at this point in time.

Henry, there's nothing preventing you from adding these to your local
trees if you need them.

thanks,

greg k-h