| 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
| ||
|
Message-ID: <3bc5797e-f23e-6416-4626-161e34c409ff@redhat.com>
Date: Sat, 26 Feb 2022 14:52:12 +0800
From: Xiubo Li <xiubli@...hat.com>
To: Luís Henriques <lhenriques@...e.de>
Cc: Jeff Layton <jlayton@...nel.org>,
Ilya Dryomov <idryomov@...il.com>, ceph-devel@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] ceph: add support for encrypted snapshot names
On 2/25/22 5:48 PM, Luís Henriques wrote:
> Xiubo Li <xiubli@...hat.com> writes:
>
>> On 2/24/22 7:21 PM, 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 | 15 +++++++++++++++
>>> fs/ceph/inode.c | 10 +++++++++-
>>> 2 files changed, 24 insertions(+), 1 deletion(-)
>>>
>>> Support on the MDS for names that'll be > MAX_NAME when base64 encoded is
>>> still TBD. I thought it would be something easy to do, but snapshots
>>> don't seem to make use of the CDir/CDentry (which is where alternate_name
>>> is stored on the MDS). I'm still looking into this, but I may need some
>>> help there :-(
>>>
>>> Cheers,
>>> --
>>> Luís
>>>
>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>> index a449f4a07c07..20ae600ee7cd 100644
>>> --- a/fs/ceph/dir.c
>>> +++ b/fs/ceph/dir.c
>>> @@ -1065,6 +1065,13 @@ 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);
>>> + /* XXX missing support for alternate_name in snapshots */
>>> + if (IS_ENCRYPTED(dir) && (dentry->d_name.len >= 189)) {
>>> + dout("encrypted snapshot name too long: %pd len: %d\n",
>>> + dentry, dentry->d_name.len);
>>> + err = -ENAMETOOLONG;
>>> + goto out;
>>> + }
>>> } 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;
>>> @@ -1109,6 +1116,14 @@ static int ceph_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>> !req->r_reply_info.head->is_target &&
>>> !req->r_reply_info.head->is_dentry)
>>> err = ceph_handle_notrace_create(dir, dentry);
>>> +
>>> + /*
>>> + * If we have created a snapshot we need to clear the cache, otherwise
>>> + * snapshot will show encrypted filenames in readdir.
>>> + */
>> Do you mean dencrypted filenames ?
> What I see without this d_drop() is that, if I run an 'ls' in a snapshot
> directory immediately after creating it, the filenames in that snapshot
> will be encrypted. Maybe there's a bug somewhere else and this d_drop()
> isn't the right fix...?
BTW, how did you reproduce this ?
The snapshot name hasn't encrypted yet ? Did you add one patch to do this ?
Locally I have one patch to support this, but not finished yet:
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 914a6e68bb56..bc9b2b0c9c66 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -2464,7 +2464,7 @@ static u8 *get_fscrypt_altname(const struct
ceph_mds_request *req, u32 *plen)
char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64
*pbase, int for_wire)
{
struct dentry *cur;
- struct inode *inode;
+ struct inode *inode, *pinode = NULL;
char *path;
int pos;
unsigned seq;
@@ -2485,18 +2485,29 @@ char *ceph_mdsc_build_path(struct dentry
*dentry, int *plen, u64 *pbase, int for
for (;;) {
struct dentry *parent;
+ parent = dget_parent(cur);
spin_lock(&cur->d_lock);
+ pinode = d_inode(parent);
+ ihold(pinode);
+ if (ceph_snap(pinode) == CEPH_SNAPDIR) {
+ struct ceph_vino vino = {
+ .ino = ceph_ino(pinode),
+ .snap = CEPH_NOSNAP,
+ };
+ pinode = ceph_find_inode(pinode->i_sb, vino);
+ BUG_ON(!pinode);
+ }
+
inode = d_inode(cur);
if (inode && ceph_snap(inode) == CEPH_SNAPDIR) {
dout("build_path path+%d: %p SNAPDIR\n",
pos, cur);
spin_unlock(&cur->d_lock);
- parent = dget_parent(cur);
} else if (for_wire && inode && dentry != cur &&
ceph_snap(inode) == CEPH_NOSNAP) {
spin_unlock(&cur->d_lock);
pos++; /* get rid of any prepended '/' */
break;
- } else if (!for_wire ||
!IS_ENCRYPTED(d_inode(cur->d_parent))) {
+ } else if (!for_wire || !IS_ENCRYPTED(pinode)) {
pos -= cur->d_name.len;
if (pos < 0) {
spin_unlock(&cur->d_lock);
@@ -2504,7 +2515,6 @@ char *ceph_mdsc_build_path(struct dentry *dentry,
int *plen, u64 *pbase, int for
}
memcpy(path + pos, cur->d_name.name,
cur->d_name.len);
spin_unlock(&cur->d_lock);
- parent = dget_parent(cur);
} else {
int len, ret;
char buf[FSCRYPT_BASE64URL_CHARS(NAME_MAX)];
@@ -2516,20 +2526,21 @@ char *ceph_mdsc_build_path(struct dentry
*dentry, int *plen, u64 *pbase, int for
memcpy(buf, cur->d_name.name, cur->d_name.len);
len = cur->d_name.len;
spin_unlock(&cur->d_lock);
- parent = dget_parent(cur);
- ret = __fscrypt_prepare_readdir(d_inode(parent));
+ ret = __fscrypt_prepare_readdir(pinode);
if (ret < 0) {
dput(parent);
dput(cur);
+ iput(pinode);
return ERR_PTR(ret);
}
- if (fscrypt_has_encryption_key(d_inode(parent))) {
- len =
ceph_encode_encrypted_fname(d_inode(parent), cur, buf);
+ if (fscrypt_has_encryption_key(pinode)) {
+ len =
ceph_encode_encrypted_fname(pinode, cur, buf);
if (len < 0) {
dput(parent);
dput(cur);
+ iput(pinode);
return ERR_PTR(len);
}
}
@@ -2552,7 +2563,11 @@ char *ceph_mdsc_build_path(struct dentry *dentry,
int *plen, u64 *pbase, int for
break;
path[pos] = '/';
+ iput(pinode);
+ pinode = NULL;
}
+ if (pinode)
+ iput(pinode);
inode = d_inode(cur);
base = inode ? ceph_ino(inode) : 0;
dput(cur);
Are you doing the same thing too ? If so I will leave this to you, or I
can send one patch to support it.
Thanks
- Xiubo
> Cheers,
Powered by blists - more mailing lists