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@xxxxxxxxxx> >> >> 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
