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

[Tyler Hicks <tyhicks@canonical.com>]

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

On 07/07/2016 01:19 PM, Jeff Mahoney wrote:
> On 7/7/16 2:02 PM, Tyler Hicks wrote:
>> 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).
> 
> I didn't see that.  Strange that it linked my profile pic but didn't
> send a notification. :)
> 
>>> 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.
> 
> It doesn't have anything to do with the regression -- but it can be
> backported directly to stable without regressions if the original fix is
> removed and stable fixes should match the upstream ones.

After reviewing and testing, I've changed my mind. I agree that your
patches are the better fix for trunk and stable. I do have some small
changes to make to the second patch. I'll reply directly to that patch
with those changes.

Tyler

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