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@xxxxxxxxxx>
> > >
> > > 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.
