Re: [RFC] odd check in ceph_encode_encrypted_dname()

[Luis Henriques <luis@igalia.com>]

On Mon, Feb 17 2025, Viacheslav Dubeyko wrote:

> On Sat, 2025-02-15 at 15:39 +0000, Luis Henriques wrote:
>> On Sat, Feb 15 2025, Al Viro wrote:
>> 
>> > On Fri, Feb 14, 2025 at 04:05:42PM +0000, Luis Henriques wrote:
>> > 
>> > > So, IIRC, when encrypting the snapshot name (the "my-snapshot" string),
>> > > you'll use key from the original inode.  That's why we need to handle
>> > > snapshot names starting with '_' differently.  And why we have a
>> > > customized base64 encoding function.
>> > 
>> > OK...  The reason I went looking at that thing was the race with rename()
>> > that can end up with UAF in ceph_mdsc_build_path().
>> > 
>> > We copy the plaintext name under ->d_lock, but then we call
>> > ceph_encode_encrypted_fname() which passes dentry->d_name to
>> > ceph_encode_encrypted_dname() with no locking whatsoever.
>> > 
>> > Have it race with rename and you've got a lot of unpleasantness.
>> > 
>> > The thing is, we can have all ceph_encode_encrypted_dname() put the
>> > plaintext name into buf; that eliminates the need to have a separate
>> > qstr (or dentry, in case of ceph_encode_encrypted_fname()) argument and
>> > simplifies ceph_encode_encrypted_dname() while we are at it.
>> > 
>> > Proposed fix in git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #d_name
>> > 
>> > WARNING: it's completely untested and needs review.  It's split in two commits
>> > (massage of ceph_encode_encrypted_dname(), then changing the calling conventions);
>> > both patches in followups.
>> > 
>> > Please, review.
>> 
>> I've reviewed both patches and they seem to be OK, so feel free to add my
>> 
>> Reviewed-by: Luis Henriques <luis@igalia.com>
>> 
>> But as I said, I don't have a test environment at the moment.  I'm adding
>> Slava to CC with the hope that he may be able to run some fscrypt-specific
>> tests (including snapshots creation) against these patches.
>> 
>> 
>
> Let me apply the patches and test it. I'll share the testing results ASAP.

Awesome!  Thanks a lot.

Cheers,
-- 
Luís