linux-kernel - Re: [PATCH] ceph: make sure directories aren't complete after setting crypt context

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: <41710b3d-b37f-8c65-d55d-c4137a366efd@redhat.com>
Date:   Thu, 17 Nov 2022 19:08:02 +0800
From:   Xiubo Li <xiubli@...hat.com>
To:     Luís Henriques <lhenriques@...e.de>,
        Ilya Dryomov <idryomov@...il.com>,
        Jeff Layton <jlayton@...nel.org>
Cc:     ceph-devel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ceph: make sure directories aren't complete after setting
 crypt context


On 17/11/2022 16:03, Xiubo Li wrote:
>
> On 16/11/2022 23:37, Luís Henriques wrote:
>> When setting a directory's crypt context, __ceph_dir_clear_complete() 
>> needs
>> to be used otherwise, if it was complete before, any old dentry 
>> that's still
>> around will be valid.
>>
>> Signed-off-by: Luís Henriques <lhenriques@...e.de>
>> ---
>> Hi!
>>
>> Here's a simple way to trigger the bug this patch is fixing:
>>
>> # cd /cephfs
>> # ls mydir
>> nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0
>> # ls mydir/nKRhofOAVNsAwVLvDw7a0c9ypsjbZfK3n0Npnmni6j0/
>> Cyuer5xT+kBlEPgtwAqSj0WK2taEljP5vHZ,D8VXCJ8 
>> u+46b2XVCt7Obpz0gznZyNLRj79Q2l4KmkwbKOzdQKw
>> # fscrypt unlock mydir
>> # touch /mnt/test/mydir/mysubdir/file
>> touch: cannot touch '/mnt/test/mydir/mysubdir/file': No such file or 
>> directory
>>
>>   fs/ceph/crypto.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>> index 35a2ccfe6899..dc1557967032 100644
>> --- a/fs/ceph/crypto.c
>> +++ b/fs/ceph/crypto.c
>> @@ -87,6 +87,10 @@ static int ceph_crypt_get_context(struct inode 
>> *inode, void *ctx, size_t len)
>>           return -ERANGE;
>>         memcpy(ctx, cfa->cfa_blob, ctxlen);
>> +
>> +    /* Directory isn't complete anymore */
>> +    if (S_ISDIR(inode->i_mode) && __ceph_dir_is_complete(ci))
>> +        __ceph_dir_clear_complete(ci);
>
> Hi Luis,
>
> Good catch!
>
> BTW, why do this in the ceph_crypt_get_context() ? As my understanding 
> is that we should mark 'mydir' as incomplete when unlocking it. While 
> as I remembered the unlock operation will do:
>
>
> Step1: get_encpolicy via 'mydir' as ctx
> Step2: rm_enckey of ctx from the superblock
>
Sorry, it should be add_enckey.
>
> Since I am still running the test cases for the file lock patches, so 
> I didn't catch logs to confirm the above steps yet.
>
> If I am right IMO then we should mark the dir as incomplete in the 
> Step2 instead, because for non-unlock operations they may also do the 
> Step1.
>
Your patch will work. But probably we should do this just around 
__fscrypt_prepare_readdir() or fscrypt_prepare_readdir() instead ? We 
need to detect that once the 'inode->i_crypt_info' changed then mark the 
dir as incomplete.

For now for the lock operation it will evict the inode, which will help 
do this for us already. But for unlock case, we need to handle it by 
ourself.

Thanks!

- Xiubo


> Thanks!
>
> - Xiubo
>
>>       return ctxlen;
>>   }
>>