Enzo Matsumiya <ematsumiya@xxxxxxx> writes:
> The SHA-512 shash TFM is used only briefly during Session Setup stage,
> when computing SMB 3.1.1 preauth hash.
>
> There's no need to keep it allocated in servers' secmech the whole time,
> so keep its lifetime inside smb311_update_preauth_hash().
>
> This also makes smb311_crypto_shash_allocate() redundant, so expose
> smb3_crypto_shash_allocate() and use that.
>
> Signed-off-by: Enzo Matsumiya <ematsumiya@xxxxxxx>
> ---
> fs/smb/client/cifsencrypt.c | 1 -
> fs/smb/client/cifsglob.h | 1 -
> fs/smb/client/sess.c | 2 +-
> fs/smb/client/smb2misc.c | 28 ++++++++++++++--------------
> fs/smb/client/smb2proto.h | 2 +-
> fs/smb/client/smb2transport.c | 30 +-----------------------------
> 6 files changed, 17 insertions(+), 47 deletions(-)
This commit leads to the following NULL ptr deref when mounting a share
with 'vers=2.1'. Reproduced it against Windows Server 2022 and samba
4.21.
$ mount.cifs //srv/share /mnt -o ...,vers=2.1
BUG: KASAN: null-ptr-deref in smb2_calc_signature+0x195/0x4f0 [cifs]
Read of size 8 at addr 0000000000000000 by task mount.cifs/693
CPU: 2 UID: 0 PID: 693 Comm: mount.cifs Not tainted 6.11.0 #3
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-2.fc40
04/01/2014
Call Trace:
<TASK>
dump_stack_lvl+0x5d/0x80
? smb2_calc_signature+0x195/0x4f0 [cifs]
kasan_report+0xda/0x110
? smb2_calc_signature+0x195/0x4f0 [cifs]
smb2_calc_signature+0x195/0x4f0 [cifs]
? __pfx_smb2_calc_signature+0x10/0x10 [cifs]
? do_raw_spin_trylock+0xc6/0x120
? do_raw_spin_unlock+0x9a/0xf0
? _raw_spin_unlock+0x23/0x40
? smb2_sign_rqst+0x10c/0x160 [cifs]
smb2_setup_request+0x225/0x3a0 [cifs]
compound_send_recv+0x417/0x1140 [cifs]
? __pfx_compound_send_recv+0x10/0x10 [cifs]
? __create_object+0x5e/0x90
? hlock_class+0x32/0xb0
? do_raw_spin_unlock+0x9a/0xf0
cifs_send_recv+0x23/0x30 [cifs]
SMB2_tcon+0x3ec/0xb30 [cifs]
? __pfx_SMB2_tcon+0x10/0x10 [cifs]
? ___ratelimit+0x133/0x1f0
? __pfx____ratelimit+0x10/0x10
? __pfx_SMB2_tcon+0x10/0x10 [cifs]
? cifs_get_smb_ses+0xcaf/0x1070 [cifs]
cifs_get_smb_ses+0xcaf/0x1070 [cifs]
? __pfx_cifs_get_smb_ses+0x10/0x10 [cifs]
? cifs_get_tcp_session+0xaa0/0xca0 [cifs]
cifs_mount_get_session+0x8a/0x210 [cifs]
dfs_mount_share+0x1b0/0x11d0 [cifs]
? __pfx___lock_acquire+0x10/0x10
? __pfx_dfs_mount_share+0x10/0x10 [cifs]
? lock_acquire+0x14b/0x3e0
? find_held_lock+0x8a/0xa0
? hlock_class+0x32/0xb0
? lock_release+0x203/0x5d0
cifs_mount+0xb3/0x3d0 [cifs]
? do_raw_spin_trylock+0xc6/0x120
? __pfx_cifs_mount+0x10/0x10 [cifs]
? ___ratelimit+0x133/0x1f0
? smb3_update_mnt_flags+0x372/0x3b0 [cifs]
cifs_smb3_do_mount+0x1e2/0xc80 [cifs]
? __pfx___mutex_lock+0x10/0x10
? __pfx_cifs_smb3_do_mount+0x10/0x10 [cifs]
smb3_get_tree+0x1bf/0x330 [cifs]
vfs_get_tree+0x4a/0x160
path_mount+0x3c1/0xfb0
? kasan_quarantine_put+0xc7/0x1d0
? __pfx_path_mount+0x10/0x10
? kmem_cache_free+0x118/0x3e0
? user_path_at+0x74/0xa0
__x64_sys_mount+0x1a6/0x1e0
? __pfx___x64_sys_mount+0x10/0x10
? mark_held_locks+0x1a/0x90
do_syscall_64+0xbb/0x1d0
entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7fd23a9a88fe
Code: 48 8b 0d 1d a5 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84
00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01
f0 ff ff 73 01 c3 48 8b 0d ea a4 0c 00 f7 d8 64 89 01 48
RSP: 002b:00007ffdeef79388 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 0000562a96057eb0 RCX: 00007fd23a9a88fe
RDX: 0000562a6d33347e RSI: 0000562a6d3334e4 RDI: 00007ffdeef7a4a2
RBP: 00007ffdeef79440 R08: 0000562a96057eb0 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000000a
R13: 00007ffdeef7a4a2 R14: 0000562a96058f00 R15: 00007fd23aa96000
</TASK>
$ ./scripts/faddr2line --list fs/smb/client/cifs.o smb2_calc_signature+0x195
smb2_calc_signature+0x195/0x4f0:
smb2_calc_signature at /home/pc/g/linux/fs/smb/client/smb2transport.c:235
230 }
231 } else {
232 shash = server->secmech.hmacsha256;
233 }
234
>235< rc = crypto_shash_setkey(shash->tfm, ses->auth_key.response,
236 SMB2_NTLMV2_SESSKEY_SIZE);
237 if (rc) {
238 cifs_server_dbg(VFS,
239 "%s: Could not update with response\n",
240 __func__);
What's the problem with having it allocated the whole time? Reconnect
path will need it to re-establish SMB sessions, so this means we'll now
have to allocate it every time we attempt to reconnect a session? This
could be worse when smb2_reconnect_server() is periodically attemping to
reconnect a session.
