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
| ||
|
Date: Mon, 14 Mar 2022 13:17:30 +0800 From: Xiubo Li <xiubli@...hat.com> To: Luís Henriques <lhenriques@...e.de>, Jeff Layton <jlayton@...nel.org>, Ilya Dryomov <idryomov@...il.com> Cc: ceph-devel@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [RFC PATCH 1/2] ceph: add support for encrypted snapshot names On 3/14/22 10:45 AM, Xiubo Li wrote: > > On 3/12/22 4:30 PM, Xiubo Li wrote: >> >> On 3/11/22 1:26 AM, Luís Henriques wrote: >>> Since filenames in encrypted directories are already encrypted and >>> shown >>> as a base64-encoded string when the directory is locked, snapshot names >>> should show a similar behaviour. >>> >>> Signed-off-by: Luís Henriques <lhenriques@...e.de> >>> --- >>> fs/ceph/dir.c | 9 +++++++++ >>> fs/ceph/inode.c | 13 +++++++++++++ >>> 2 files changed, 22 insertions(+) >>> >>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c >>> index 6df2a91af236..123e3b9c8161 100644 >>> --- a/fs/ceph/dir.c >>> +++ b/fs/ceph/dir.c >>> @@ -1075,6 +1075,15 @@ static int ceph_mkdir(struct user_namespace >>> *mnt_userns, struct inode *dir, >>> op = CEPH_MDS_OP_MKSNAP; >>> dout("mksnap dir %p snap '%pd' dn %p\n", dir, >>> dentry, dentry); >>> + /* >>> + * Encrypted snapshots require d_revalidate to force a >>> + * LOOKUPSNAP to cleanup dcache >>> + */ >>> + if (IS_ENCRYPTED(dir)) { >>> + spin_lock(&dentry->d_lock); >>> + dentry->d_flags |= DCACHE_NOKEY_NAME; >> >> I think this is not correct fix of this issue. >> >> Actually this dentry's name is a KEY NAME, which is human readable name. >> >> DCACHE_NOKEY_NAME means the base64_encoded names. This usually will >> be set when filling a new dentry if the directory is locked. If the >> directory is unlocked the directory inode will be set with the key. >> >> The root cause should be the snapshot's inode doesn't correctly set >> the encrypt stuff when you are reading from it. >> >> NOTE: when you are 'ls -l .snap/snapXXX' the snapXXX dentry name is >> correct, it's just corrupted for the file or directory names under >> snapXXX/. >> > When mksnap in ceph_mkdir() before sending the request out it will > create a new inode for the snapshot dentry and then will fill the > ci->fscrypt_auth from .snap's inode, please see > ceph_mkdir()->ceph_new_inode(). > > And in the mksnap request reply it will try to fill the > ci->fscrypt_auth again but failed because it was already filled. This > time the auth info is from .snap's parent dir from MDS side. In this > patch in theory they should be the same, but I am still not sure why > when decrypting the dentry names in snapXXX will fail. > > I just guess it possibly will depend on the inode number from the > related inode or something else. Before the request reply it seems the > inode isn't set the inode number ? > It should be the ci_nonce's problem. In the ceph_mkdir()->ceph_new_inode() it will generate a new random nonce and then setup the fscrypt context for the inode of .snap/snapXXX. But this context is not correct, because the context of .snap/snapXXX should always be inherit from .snap's parent, which will be sent from the MDS in the request reply. > - Xiubo > >> >>> + spin_unlock(&dentry->d_lock); >>> + } >>> } else if (ceph_snap(dir) == CEPH_NOSNAP) { >>> dout("mkdir dir %p dn %p mode 0%ho\n", dir, dentry, mode); >>> op = CEPH_MDS_OP_MKDIR; >>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c >>> index b573a0f33450..81d3d554d261 100644 >>> --- a/fs/ceph/inode.c >>> +++ b/fs/ceph/inode.c >>> @@ -182,6 +182,19 @@ struct inode *ceph_get_snapdir(struct inode >>> *parent) >>> ci->i_rbytes = 0; >>> ci->i_btime = ceph_inode(parent)->i_btime; >>> + /* if encrypted, just borrow fscrypt_auth from parent */ >>> + if (IS_ENCRYPTED(parent)) { >>> + struct ceph_inode_info *pci = ceph_inode(parent); >>> + >>> + ci->fscrypt_auth = kmemdup(pci->fscrypt_auth, >>> + pci->fscrypt_auth_len, >>> + GFP_KERNEL); >>> + if (ci->fscrypt_auth) { >>> + inode->i_flags |= S_ENCRYPTED; >>> + ci->fscrypt_auth_len = pci->fscrypt_auth_len; >>> + } else >>> + dout("Failed to alloc memory for fscrypt_auth in >>> snapdir\n"); >>> + } >> >> Here I think Jeff has already commented it in your last version, it >> should fail by returning NULL ? >> >> - Xiubo >> >>> if (inode->i_state & I_NEW) { >>> inode->i_op = &ceph_snapdir_iops; >>> inode->i_fop = &ceph_snapdir_fops; >>>
Powered by blists - more mailing lists