Re: [PATCH] eCryptfs: Fix directory open regression in linux-stable

[Tyler Hicks <tyhicks@canonical.com>]

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

On 07/05/2016 04:14 PM, Jeff Mahoney wrote:
> On 6/28/16 11:39 PM, Tyler Hicks wrote:
>> Cherry-picking mainline commit 2f36db71009304b3f0b95afacd8eba1f9f046b87
>> introduces a regression in eCryptfs when mainline commit
>> 6a480a7842545ec520a91730209ec0bae41694c1 (4.6+) is not present. The
>> regression causes all attempts at opening directory files to fail with
>> EMEDIUMTYPE when the lower filesystem's file_operations for directory
>> files do not implement mmap.
>>
>> This is a simple fix that allows the check for the lower file's mmap
>> implementation to be ignored if the lower file is a directory.
> 
> I have a different fix that I believe is more correct for this.  I would
> have posted it in response to the original fix if it were ever actually
> posted for public discussion.

Hi Jeff - I'm glad that you are sending your fix out for review (not
sure if you noticed but I asked you to do so in a reply to your comment
on the project zero blog post).

> Denying open is the wrong place to fix this.  It's too heavy a hammer
> and, as we see here, a bit fragile.

I agree but I think that denying open() has little to do with this
regression in linux-stable. I'd prefer that we leave linux-stable as-is
(after applying my minimal regression fix patch) and take your fix trunk.

Tyler

> 
> The right fix is to deny the mmap call instead.
> 
> -Jeff
> 
>> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
>> Tested-by: Tyler Hicks <tyhicks@canonical.com> # 4.4.y, 3.18.y
>> Cc: <stable@vger.kernel.org> # 4.5-
>> ---
>>  fs/ecryptfs/kthread.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/ecryptfs/kthread.c b/fs/ecryptfs/kthread.c
>> index e818f5a..b9faeab 100644
>> --- a/fs/ecryptfs/kthread.c
>> +++ b/fs/ecryptfs/kthread.c
>> @@ -171,7 +171,7 @@ int ecryptfs_privileged_open(struct file **lower_file,
>>  		goto out;
>>  	}
>>  have_file:
>> -	if ((*lower_file)->f_op->mmap == NULL) {
>> +	if ((*lower_file)->f_op->mmap == NULL && !d_is_dir(lower_dentry)) {
>>  		fput(*lower_file);
>>  		*lower_file = NULL;
>>  		rc = -EMEDIUMTYPE;
>>
> 
> 



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