linux-ext4 - Re: [PATCH 20/20] ext4: Implement encoding-aware dcache hooks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87zhyxo6q1.fsf@collabora.co.uk>
Date:   Wed, 11 Jul 2018 16:30:46 -0400
From:   Gabriel Krisman Bertazi <krisman@...labora.co.uk>
To:     "Theodore Y. Ts'o" <tytso@....edu>
Cc:     linux-ext4@...r.kernel.org, darrick.wong@...cle.com,
        kernel@...labora.com
Subject: Re: [PATCH 20/20] ext4: Implement encoding-aware dcache hooks

"Theodore Y. Ts'o" <tytso@....edu> writes:

> On Tue, Jul 03, 2018 at 01:07:00PM -0400, Gabriel Krisman Bertazi wrote:
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 53db9b6c7e33..f292cc5bacda 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -4358,6 +4358,10 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>>  		iput(root);
>>  		goto failed_mount4;
>>  	}
>> +
>> +	if (sbi->encoding)
>> +		sb->s_d_op = &ext4_dentry_ops;
>> +

Hello,

Thanks for the review.

>
> This is going to be potentially problematic for fscrypt (e.g., when
> ext4's encryption is enabled, as will be the case in Android's File
> Based Encryption).  See the call to d_set_d_op() in
> __fscrypt_prepare_lookup in fs/crypto/hooks.c, since d_set_d_op is
> going to overwrite sb->s_d_op.
>
> This probably means that the fscrypt code is going to have to be
> case-sensitivity aware.  Or we would have to make case folding and fs
> encryption to be mutually exclusive, which would be unfortunate, since
> Android is going to be a potential user of case folding.

Yes, I am aware of that issue with fscrypt's d_revalidate, the smoke
tests helped me catch it beforehand.  I worked around it on a different
branch by implementing a wrapper around my own d_revalidate, and
removing it from fscrypt.  I didn't send it along with this patchset
because, as you mentioned, fscrypt needs to become case-sensitive aware
(in fact, encoding-aware), which means we need to either also store the
normalized version of the encrypted file name in the dentry, or do a
much more expensive search in the directory, by decrypting each name
before comparison.  I'm doing the second option now, (decrypting the
filename at comparison time), but I considered this non-trivial change
to be a future step, which is not ready for submission.

My approach in the current patchset is make these features mutually
exclusive for now, as you can see in patch 17.  I'm refusing to mount
with encoding any superblock that has the encryption feature enabled.  I
know it is not a final solution for Android and Valve usecases, but I
think this support can be integrated at a later moment without breaking
the abi.

-- 
9Gabriel Krisman Bertazi