Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 2 Nov 2022 11:48:51 +0000
From:   =?iso-8859-1?Q?Lu=EDs?= Henriques <lhenriques@suse.de>
To:     Xiubo Li <xiubli@redhat.com>
Cc:     Ilya Dryomov <idryomov@gmail.com>,
        Jeff Layton <jlayton@kernel.org>, ceph-devel@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH] ceph: allow encrypting a directory while not having
 Ax caps
Message-ID: <Y2JZI1QOl3dXBVUb@suse.de>
References: <20221027112653.12122-1-lhenriques@suse.de>
 <a992d844-6d75-e134-60e1-acb8c8972ff3@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <a992d844-6d75-e134-60e1-acb8c8972ff3@redhat.com>
Precedence: bulk

On Mon, Oct 31, 2022 at 05:15:51PM +0800, Xiubo Li wrote:
> 
> On 27/10/2022 19:26, Lu?s Henriques wrote:
> > If a client doesn't have Fx caps on a directory, it will get errors while
> > trying encrypt it:
> > 
> > ceph: handle_cap_grant: cap grant attempt to change fscrypt_auth on non-I_NEW inode (old len 0 new len 48)
> > fscrypt (ceph, inode 1099511627812): Error -105 getting encryption context
> > 
> > A simple way to reproduce this is to use two clients:
> > 
> >      client1 # mkdir /mnt/mydir
> > 
> >      client2 # ls /mnt/mydir
> > 
> >      client1 # fscrypt encrypt /mnt/mydir
> >      client1 # echo hello > /mnt/mydir/world
> > 
> > This happens because, in __ceph_setattr(), we only initialize
> > ci->fscrypt_auth if we have Ax.  If we don't have, we'll need to do that
> > later, in handle_cap_grant().
> > 
> > Signed-off-by: Lu?s Henriques <lhenriques@suse.de>
> > ---
> > Hi!
> > 
> > To be honest, I'm not really sure about the conditions in the 'if': shall
> > I bother checking it's really a dir and that it is empty?
> > 
> > Cheers,
> > --
> > Lu?s
> > 
> >   fs/ceph/caps.c | 26 +++++++++++++++++++++++---
> >   1 file changed, 23 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > index 443fce066d42..e33b5c276cf3 100644
> > --- a/fs/ceph/caps.c
> > +++ b/fs/ceph/caps.c
> > @@ -3511,9 +3511,29 @@ static void handle_cap_grant(struct inode *inode,
> >   		     from_kuid(&init_user_ns, inode->i_uid),
> >   		     from_kgid(&init_user_ns, inode->i_gid));
> >   #if IS_ENABLED(CONFIG_FS_ENCRYPTION)
> > -		if (ci->fscrypt_auth_len != extra_info->fscrypt_auth_len ||
> > -		    memcmp(ci->fscrypt_auth, extra_info->fscrypt_auth,
> > -			   ci->fscrypt_auth_len))
> > +		if ((ci->fscrypt_auth_len == 0) &&
> > +		    (extra_info->fscrypt_auth_len > 0) &&
> > +		    S_ISDIR(inode->i_mode) &&
> > +		    (ci->i_rsubdirs + ci->i_rfiles == 1)) {
> > +			/*
> > +			 * We'll get here when setting up an encrypted directory
> > +			 * but we don't have Fx in that directory, i.e. other
> > +			 * clients have accessed this directory too.
> > +			 */
> > +			ci->fscrypt_auth = kmemdup(extra_info->fscrypt_auth,
> > +						   extra_info->fscrypt_auth_len,
> > +						   GFP_KERNEL);
> > +			if (ci->fscrypt_auth) {
> > +				inode->i_flags |= S_ENCRYPTED;
> > +				ci->fscrypt_auth_len = extra_info->fscrypt_auth_len;
> > +			} else {
> > +				pr_err("Failed to alloc memory for %llx.%llx fscrypt_auth\n",
> > +					ceph_vinop(inode));
> > +			}
> > +			dout("ino %llx.%llx is now encrypted\n", ceph_vinop(inode));
> > +		} else if (ci->fscrypt_auth_len != extra_info->fscrypt_auth_len ||
> > +			   memcmp(ci->fscrypt_auth, extra_info->fscrypt_auth,
> > +				  ci->fscrypt_auth_len))
> >   			pr_warn_ratelimited("%s: cap grant attempt to change fscrypt_auth on non-I_NEW inode (old len %d new len %d)\n",
> >   				__func__, ci->fscrypt_auth_len, extra_info->fscrypt_auth_len);
> >   #endif
> 
> Hi Luis,
> 
> Thanks for your time on this bug.
> 
> IMO we should fix this in ceph_fill_inode():
> 
> ?995 #ifdef CONFIG_FS_ENCRYPTION
> ?996???????? if (iinfo->fscrypt_auth_len && (inode->i_state & I_NEW)) {
> ?997???????????????? kfree(ci->fscrypt_auth);
> ?998???????????????? ci->fscrypt_auth_len = iinfo->fscrypt_auth_len;
> ?999???????????????? ci->fscrypt_auth = iinfo->fscrypt_auth;
> 1000???????????????? iinfo->fscrypt_auth = NULL;
> 1001???????????????? iinfo->fscrypt_auth_len = 0;
> 1002???????????????? inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED);
> 1003???????? }
> 1004 #endif
> 
> The setattr will get a reply from MDS including the fscrypt auth info, I
> think the kclient just drop it here.

I've done some testing and I don't really see this code kfree'ing a valid
fscrypt_auth here.  However, I guess it is possible to fix this issue here
too, but in a different way, by changing that 'if' condition to:

	if (iinfo->fscrypt_auth_len &&
	    ((inode->i_state & I_NEW) || (ci->fscrypt_auth_len == 0))) {
	...
	}

I'm not really sure if this is sane though.  When we loose the 'Ax' caps
(another client as accessed the directory we're encrypting), we also seem
to loose the I_NEW state.  Using the above code seems to work for the
testcase in my patch, but I'm not sure it won't break something else.

Cheers,
--
Lu?s

> If we fix it in handle_cap_grant() I am afraid this bug still exists. What
> if there is no any new caps will be issued or revoked recently and then
> access to the directory ?
> 
> Thanks
> 
> - Xiubo
> 
> > 
>