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;
From:   =?utf-8?Q?Lu=C3=ADs_Henriques?= <lhenriques@suse.de>
To:     Xiubo Li <xiubli@redhat.com>
Cc:     Jeff Layton <jlayton@kernel.org>,
        Ilya Dryomov <idryomov@gmail.com>, ceph-devel@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v3 4/4] ceph: replace base64url by the encoding used
 for mailbox names
References: <20220317154521.6615-1-lhenriques@suse.de>
        <20220317154521.6615-5-lhenriques@suse.de>
        <6ac92645-dede-b87c-3731-2280d59f8d8e@redhat.com>
Date:   Fri, 18 Mar 2022 11:01:01 +0000
In-Reply-To: <6ac92645-dede-b87c-3731-2280d59f8d8e@redhat.com> (Xiubo Li's
        message of "Fri, 18 Mar 2022 16:32:15 +0800")
Message-ID: <8735jfh6du.fsf@brahms.olymp>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Xiubo Li <xiubli@redhat.com> writes:

> On 3/17/22 11:45 PM, Lu=C3=ADs Henriques wrote:
>> The base64url encoding includes the '_' character, which may cause probl=
ems
>> in snapshot names (if the name starts with '_').  Thus, use the base64
>> encoding defined for IMAP mailbox names (RFC 3501), which uses '+' and '=
,'
>> instead of '-' and '_'.
>>
>> Signed-off-by: Lu=C3=ADs Henriques <lhenriques@suse.de>
>> ---
>>   fs/ceph/crypto.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++--
>>   fs/ceph/crypto.h |  3 +++
>>   fs/ceph/dir.c    |  2 +-
>>   fs/ceph/inode.c  |  2 +-
>>   4 files changed, 54 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>> index caa9863dee93..d6f1c444ce91 100644
>> --- a/fs/ceph/crypto.c
>> +++ b/fs/ceph/crypto.c
>> @@ -7,6 +7,53 @@
>>   #include "mds_client.h"
>>   #include "crypto.h"
>>   +static const char base64_table[65] =3D
>> +        "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789=
+,";
>> +
>> +int ceph_base64_encode(const u8 *src, int srclen, char *dst)
>> +{
>> +	u32 ac =3D 0;
>> +	int bits =3D 0;
>> +	int i;
>> +	char *cp =3D dst;
>> +
>> +	for (i =3D 0; i < srclen; i++) {
>> +		ac =3D (ac << 8) | src[i];
>> +		bits +=3D 8;
>> +		do {
>> +			bits -=3D 6;
>> +			*cp++ =3D base64_table[(ac >> bits) & 0x3f];
>> +		} while (bits >=3D 6);
>> +	}
>> +	if (bits)
>> +		*cp++ =3D base64_table[(ac << (6 - bits)) & 0x3f];
>> +	return cp - dst;
>> +}
>> +
>> +int ceph_base64_decode(const char *src, int srclen, u8 *dst)
>> +{
>> +	u32 ac =3D 0;
>> +	int bits =3D 0;
>> +	int i;
>> +	u8 *bp =3D dst;
>> +
>> +	for (i =3D 0; i < srclen; i++) {
>> +		const char *p =3D strchr(base64_table, src[i]);
>> +
>> +		if (p =3D=3D NULL || src[i] =3D=3D 0)
>> +			return -1;
>> +		ac =3D (ac << 6) | (p - base64_table);
>> +		bits +=3D 6;
>> +		if (bits >=3D 8) {
>> +			bits -=3D 8;
>> +			*bp++ =3D (u8)(ac >> bits);
>> +		}
>> +	}
>> +	if (ac & ((1 << bits) - 1))
>> +		return -1;
>> +	return bp - dst;
>> +}
>
> Maybe this should be in fs/crypto.c ?

Yeah, if the solution is to modify the base64 encoding, that's my
preference too (in the series cover-letter this would correspond to
alternative #3).

[ In fact, I've probably done something very wrong here, as I didn't even
  mentioned that this patch is basically a copy of someone else's code; I
  simply modified the table used.  So, please do *not* consider merging
  this patch. ]

Cheers,
--=20
Lu=C3=ADs

>
> -- Xiubo
>
>> +
>>   static int ceph_crypt_get_context(struct inode *inode, void *ctx, size=
_t len)
>>   {
>>   	struct ceph_inode_info *ci =3D ceph_inode(inode);
>> @@ -260,7 +307,7 @@ int ceph_encode_encrypted_dname(struct inode *parent=
, struct qstr *d_name, char
>>   	}
>>     	/* base64 encode the encrypted name */
>> -	elen =3D fscrypt_base64url_encode(cryptbuf, len, buf);
>> +	elen =3D ceph_base64_encode(cryptbuf, len, buf);
>>   	dout("base64-encoded ciphertext name =3D %.*s\n", elen, buf);
>>     	WARN_ON(elen > (CEPH_NOHASH_NAME_MAX + SHA256_DIGEST_SIZE));
>> @@ -365,7 +412,7 @@ int ceph_fname_to_usr(const struct ceph_fname *fname=
, struct fscrypt_str *tname,
>>   			tname =3D &_tname;
>>   		}
>>   -		declen =3D fscrypt_base64url_decode(name, name_len, tname->name);
>> +		declen =3D ceph_base64_decode(name, name_len, tname->name);
>>   		if (declen <=3D 0) {
>>   			ret =3D -EIO;
>>   			goto out;
>> diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
>> index 3273d076a9e5..d22316011810 100644
>> --- a/fs/ceph/crypto.h
>> +++ b/fs/ceph/crypto.h
>> @@ -93,6 +93,9 @@ static inline u32 ceph_fscrypt_auth_len(struct ceph_fs=
crypt_auth *fa)
>>    */
>>   #define CEPH_NOHASH_NAME_MAX (180 - SHA256_DIGEST_SIZE)
>>   +int ceph_base64_encode(const u8 *src, int srclen, char *dst);
>> +int ceph_base64_decode(const char *src, int srclen, u8 *dst);
>> +
>>   void ceph_fscrypt_set_ops(struct super_block *sb);
>>     void ceph_fscrypt_free_dummy_policy(struct ceph_fs_client *fsc);
>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>> index 5ae5cb778389..417d8c3a7edd 100644
>> --- a/fs/ceph/dir.c
>> +++ b/fs/ceph/dir.c
>> @@ -960,7 +960,7 @@ static int prep_encrypted_symlink_target(struct ceph=
_mds_request *req, const cha
>>   		goto out;
>>   	}
>>   -	len =3D fscrypt_base64url_encode(osd_link.name, osd_link.len,
>> req->r_path2);
>> +	len =3D ceph_base64_encode(osd_link.name, osd_link.len, req->r_path2);
>>   	req->r_path2[len] =3D '\0';
>>   out:
>>   	fscrypt_fname_free_buffer(&osd_link);
>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>> index 359e29896f16..8fd493257e0b 100644
>> --- a/fs/ceph/inode.c
>> +++ b/fs/ceph/inode.c
>> @@ -875,7 +875,7 @@ static int decode_encrypted_symlink(const char *encs=
ym, int enclen, u8 **decsym)
>>   	if (!sym)
>>   		return -ENOMEM;
>>   -	declen =3D fscrypt_base64url_decode(encsym, enclen, sym);
>> +	declen =3D ceph_base64_decode(encsym, enclen, sym);
>>   	if (declen < 0) {
>>   		pr_err("%s: can't decode symlink (%d). Content: %.*s\n", __func__, d=
eclen, enclen, encsym);
>>   		kfree(sym);
>>
>