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@xxxxxxxxxxxxx>
>>>> Tested-by: Tyler Hicks <tyhicks@xxxxxxxxxxxxx> # 4.4.y, 3.18.y
>>>> Cc: <stable@xxxxxxxxxxxxxxx> # 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:
signature.asc
Description: OpenPGP digital signature