RE: [RFC] odd check in ceph_encode_encrypted_dname()

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

On Mon, 2025-02-17 at 18:48 +0000, Luis Henriques wrote:
> 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@igalia.com>
> > > 
> > > 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.
> 

As far as I can see, we have issue in the patchset. I was able to reproduce the
issue two times. 

sudo ./check -g encrypt
FSTYP         -- ceph
PLATFORM      -- Linux/x86_64 ceph-testing-0001 6.14.0-rc3+ #174 SMP
PREEMPT_DYNAMIC Mon Feb 17 21:02:23 UTC 2025
MKFS_OPTIONS  -- 127.0.0.1:40364:/scratch
MOUNT_OPTIONS -- -o name=fs,secret=<secret>,ms_mode=crc,nowsync,copyfrom
127.0.0.1:40364:/scratch /mnt/scratch

generic/395 10s ...  10s
generic/396 9s ...  9s
generic/397 10s ... _check_dmesg: something found in dmesg (see
/home/slavad/XFSTESTS/xfstests-dev/results//generic/397.dmesg)

generic/398 1s ... [not run] kernel doesn't support renameat2 syscall
generic/399 28s ... [not run] Filesystem ceph not supported in
_scratch_mkfs_sized_encrypted
generic/419 1s ... [not run] kernel doesn't support renameat2 syscall
generic/421 14s ...  13s
generic/429 22s ...  22s
generic/435 891s ...  848s
generic/440 14s ...  13s
generic/548 2s ... [not run] xfs_io fiemap  failed (old kernel/wrong fs?)
generic/549 2s ... [not run] encryption policy '-c 5 -n 6 -f 0' is unusable;
probably missing kernel crypto API support
generic/550 4s ... [not run] encryption policy '-c 9 -n 9 -f 0' is unusable;
probably missing kernel crypto API support
generic/576       [not run] fsverity utility required, skipped this test
generic/580 15s ...  14s
generic/581 22s ...  21s
generic/582 2s ... [not run] xfs_io fiemap  failed (old kernel/wrong fs?)
generic/583 2s ... [not run] encryption policy '-c 5 -n 6 -v 2 -f 0' is
unusable; probably missing kernel crypto API support
generic/584 3s ... [not run] encryption policy '-c 9 -n 9 -v 2 -f 0' is
unusable; probably missing kernel crypto API support
generic/592 3s ... [not run] kernel does not support encryption policy: '-c 1 -n
4 -v 2 -f 8'
generic/593 15s ...  16s
generic/595 19s ...  18s
generic/602 2s ... [not run] kernel does not support encryption policy: '-c 1 -n
4 -v 2 -f 16'
generic/613 5s ... [not run] _get_encryption_nonce() isn't implemented on ceph
generic/621 6s ... [not run] kernel doesn't support renameat2 syscall
generic/693 6s ... [not run] encryption policy '-c 1 -n 10 -v 2 -f 0' is
unusable; probably missing kernel crypto API support
generic/739       [not run] xfs_io set_encpolicy doesn't support -s
Ran: generic/395 generic/396 generic/397 generic/398 generic/399 generic/419
generic/421 generic/429 generic/435 generic/440 generic/548 generic/549
generic/550 generic/576 generic/580 generic/581 generic/582 generic/583
generic/584 generic/592 generic/593 generic/595 generic/602 generic/613
generic/621 generic/693 generic/739
Not run: generic/398 generic/399 generic/419 generic/548 generic/549 generic/550
generic/576 generic/582 generic/583 generic/584 generic/592 generic/602
generic/613 generic/621 generic/693 generic/739
Failures: generic/397
Failed 1 of 27 tests

[  324.470491] run fstests generic/397 at 2025-02-17 12:18:13
[  325.099452] libceph: mon0 (2)127.0.0.1:40489 session established
[  325.100845] libceph: client4422 fsid 5b237794-984b-474e-a140-606ec05b28a3
[  325.597065] libceph: mon0 (2)127.0.0.1:40489 session established
[  325.598444] libceph: client4425 fsid 5b237794-984b-474e-a140-606ec05b28a3
[  325.679627] libceph: mon0 (2)127.0.0.1:40489 session established
[  325.687399] libceph: client4428 fsid 5b237794-984b-474e-a140-606ec05b28a3
[  325.803731] xfs_io (pid 8916) is setting deprecated v1 encryption policy;
recommend upgrading to v2.
[  325.883128] libceph: mon0 (2)127.0.0.1:40489 session established
[  325.884512] libceph: client4431 fsid 5b237794-984b-474e-a140-606ec05b28a3
[  325.946397] libceph: mon0 (2)127.0.0.1:40489 session established
[  325.947903] libceph: client4434 fsid 5b237794-984b-474e-a140-606ec05b28a3
[  326.321358] fscrypt: AES-256-CBC-CTS using implementation "cts(cbc(ecb(aes-
generic)))"
[  326.354897] fscrypt: AES-256-XTS using implementation "xts(ecb(aes-generic))"
[  326.439844]
==================================================================
[  326.440593] BUG: KASAN: stack-out-of-bounds in
ceph_encode_encrypted_dname+0x4a5/0x560
[  326.441355] Write of size 1 at addr ffff8881b6c5f27f by task xfs_io/9048

[  326.442149] CPU: 0 UID: 0 PID: 9048 Comm: xfs_io Not tainted 6.14.0-rc3+ #172
[  326.442156] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-
1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
[  326.442162] Call Trace:
[  326.442166]  <TASK>
[  326.442171]  dump_stack_lvl+0x76/0xa0
[  326.442182]  print_report+0xce/0x5f0
[  326.442187]  ? ceph_encode_encrypted_dname+0x4a5/0x560
[  326.442191]  ? kasan_addr_to_slab+0xd/0xb0
[  326.442198]  ? ceph_encode_encrypted_dname+0x4a5/0x560
[  326.442203]  kasan_report+0xbe/0x110
[  326.442206]  ? ceph_encode_encrypted_dname+0x4a5/0x560
[  326.442212]  __asan_report_store1_noabort+0x17/0x30
[  326.442217]  ceph_encode_encrypted_dname+0x4a5/0x560
[  326.442221]  ? lockref_get_not_zero+0xd5/0x1d0
[  326.442227]  ? __pfx_ceph_encode_encrypted_dname+0x10/0x10
[  326.442232]  ? __kasan_check_write+0x14/0x30
[  326.442237]  ? _raw_spin_lock+0x82/0xf0
[  326.442242]  ceph_mdsc_build_path+0x710/0xb00
[  326.442249]  ? __pfx_ceph_mdsc_build_path+0x10/0x10
[  326.442258]  set_request_path_attr.constprop.0+0x286/0xa50
[  326.442262]  ? __kernel_text_address+0x12/0x50
[  326.442269]  __prepare_send_request+0x6ad/0x49f0
[  326.442277]  ? __pfx___prepare_send_request+0x10/0x10
[  326.442282]  ? __kasan_check_write+0x14/0x30
[  326.442286]  ? _raw_spin_lock_irqsave+0x96/0x110
[  326.442293]  ? ceph_get_mds_session+0xd7/0x160
[  326.442298]  ? __pfx_ceph_get_mds_session+0x10/0x10
[  326.442302]  ? _atomic_dec_and_lock+0x19/0xc0
[  326.442307]  ? iput+0x104/0x5c0
[  326.442312]  __do_request+0x172b/0x4590
[  326.442319]  ? __pfx___do_request+0x10/0x10
[  326.442324]  ? __kasan_check_write+0x14/0x30
[  326.442328]  ? _raw_spin_lock+0x82/0xf0
[  326.442331]  ? __pfx__raw_spin_lock+0x10/0x10
[  326.442335]  ceph_mdsc_submit_request+0x6a8/0xe70
[  326.442341]  ceph_mdsc_do_request+0x6f/0x200
[  326.442346]  ceph_atomic_open+0xa18/0x24c0
[  326.442354]  ? __pfx_ceph_atomic_open+0x10/0x10
[  326.442358]  ? __pfx_make_vfsuid+0x10/0x10
[  326.442363]  ? __pfx_map_id_up+0x10/0x10
[  326.442367]  ? generic_permission+0x149/0x670
[  326.442373]  ? ceph_permission+0x54/0x80
[  326.442379]  ? inode_permission+0x130/0x4f0
[  326.442383]  path_openat+0x2262/0x4430
[  326.442389]  ? __pfx_path_openat+0x10/0x10
[  326.442392]  ? stack_depot_save_flags+0x2d/0x800
[  326.442397]  ? local_clock_noinstr+0xe/0xd0
[  326.442403]  ? stack_depot_save+0xe/0x20
[  326.442407]  do_filp_open+0x1d4/0x430
[  326.442411]  ? _raw_spin_lock_irqsave+0x96/0x110
[  326.442415]  ? __pfx_do_filp_open+0x10/0x10
[  326.442421]  ? _raw_spin_unlock+0xe/0x40
[  326.442426]  do_sys_openat2+0x136/0x180
[  326.442431]  ? __pfx_do_sys_openat2+0x10/0x10
[  326.442436]  ? __count_memcg_events+0x19e/0x490
[  326.442440]  __x64_sys_openat+0x128/0x220
[  326.442445]  ? __pfx___x64_sys_openat+0x10/0x10
[  326.442451]  x64_sys_call+0x1998/0x26f0
[  326.442457]  do_syscall_64+0x7c/0x170
[  326.442463]  ? irqentry_exit+0x43/0x50
[  326.442466]  ? clear_bhb_loop+0x15/0x70
[  326.442472]  ? clear_bhb_loop+0x15/0x70
[  326.442476]  ? clear_bhb_loop+0x15/0x70
[  326.442481]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[  326.442485] RIP: 0033:0x748c12b1453b
[  326.442495] Code: 25 00 00 41 00 3d 00 00 41 00 74 4b 64 8b 04 25 18 00 00 00
85 c0 75 67 44 89 e2 48 89 ee bf 9c ff ff ff b8 01 01 00 00 0f 05 <48> 3d 00 f0
ff ff 0f 87 91 00 00 00 48 8b 54 24 28 64 48 2b 14 25
[  326.442499] RSP: 002b:00007fffdf0d0730 EFLAGS: 00000246 ORIG_RAX:
0000000000000101
[  326.442504] RAX: ffffffffffffffda RBX: 0000000000000242 RCX: 0000748c12b1453b
[  326.442508] RDX: 0000000000000242 RSI: 00007fffdf0d1599 RDI: 00000000ffffff9c
[  326.442510] RBP: 00007fffdf0d1599 R08: 00007fffdf0d09e0 R09: 0000000000000000
[  326.442513] R10: 0000000000000180 R11: 0000000000000246 R12: 0000000000000242
[  326.442515] R13: 0000748c12c1a42c R14: 0000000000000180 R15: 0000000000000060
[  326.442521]  </TASK>

[  326.475514] The buggy address belongs to stack of task xfs_io/9048
[  326.476100]  and is located at offset 287 in frame:
[  326.476555]  ceph_mdsc_build_path+0x0/0xb00

[  326.477120] This frame has 1 object:
[  326.477466]  [32, 287) 'buf'

[  326.477899] The buggy address belongs to the physical page:
[  326.478428] page: refcount:0 mapcount:0 mapping:0000000000000000
index:0x10af98 pfn:0x1b6c5f
[  326.478437] flags: 0x17ffffc0000000(node=0|zone=2|lastcpupid=0x1fffff)
[  326.478445] raw: 0017ffffc0000000 0000000000000000 dead000000000122
0000000000000000
[  326.478449] raw: 000000000010af98 0000000000000000 00000000ffffffff
0000000000000000
[  326.478452] page dumped because: kasan: bad access detected

[  326.478619] Memory state around the buggy address:
[  326.479079]  ffff8881b6c5f100: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1
f1
[  326.479749]  ffff8881b6c5f180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00
[  326.480431] >ffff8881b6c5f200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
07
[  326.481106]                                                                 ^
[  326.481770]  ffff8881b6c5f280: f3 f3 f3 f3 f3 f3 f3 f3 00 00 00 00 00 00 00
00
[  326.482455]  ffff8881b6c5f300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00
[  326.483128]
==================================================================
[  326.484102] Disabling lock debugging due to kernel taint
[  326.752698] libceph: mon0 (2)127.0.0.1:40489 session established
[  326.755111] libceph: client4437 fsid 5b237794-984b-474e-a140-606ec05b28a3
[  327.128197] libceph: mon0 (2)127.0.0.1:40489 session established
[  327.129890] libceph: client4440 fsid 5b237794-984b-474e-a140-606ec05b28a3

(gdb) l *ceph_encode_encrypted_dname+0x4a5
0xffffffff829d6605 is in ceph_encode_encrypted_dname (fs/ceph/crypto.c:273).
268		u32 len;
269		int name_len = elen;
270		int ret;
271		u8 *cryptbuf = NULL;
272	
273		buf[elen] = '\0';
274		/* Handle the special case of snapshot names that start with
'_' */
275		if (ceph_snap(dir) == CEPH_SNAPDIR && *p == '_') {
276			dir = parse_longname(parent, p, &name_len);
277			if (IS_ERR(dir))
(gdb) l *ceph_mdsc_build_path+0x710
0xffffffff829a4c70 is in ceph_mdsc_build_path (fs/ceph/mds_client.c:2769).
2764					dput(cur);
2765					return ERR_PTR(ret);
2766				}
2767	
2768				if
(fscrypt_has_encryption_key(d_inode(parent))) {
2769					len =
ceph_encode_encrypted_dname(d_inode(parent),
2770									  buf,
len);
2771					if (len < 0) {
2772						dput(parent);
2773						dput(cur);

Thanks,
Slava.