ksmbd_decompress_request() bounds the decompressed size only against
the maximum request size. A compression transform can therefore
produce a buffer smaller than an SMB2 PDU and install it as
conn->request_buf.
The receive path subsequently calls ksmbd_smb_request(), which reads
the protocol ID before the normal SMB2 minimum-size check. If the
decompressed output is too short, that read can access beyond the
request allocation.
Require the decompressed output to contain at least a complete minimum
SMB2 PDU before allocating and installing the replacement request
buffer.
Fixes: a08de24c2b ("ksmbd: negotiate and decode SMB2 compression")
Cc: stable@vger.kernel.org
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
The receive path applies the minimum SMB2 PDU size check only when
ProtocolId is SMB2_PROTO_NUMBER. A packet carrying
SMB2_TRANSFORM_PROTO_NUM bypasses the check even when the negotiated
dialect does not provide transform handling.
On an SMB 2.1 connection, a short transform packet therefore reaches
init_smb2_rsp_hdr(), which interprets the request as a full SMB2 header
and reads beyond the request allocation. The copied fields can then be
returned to the unauthenticated client.
Compression transforms are converted to ordinary SMB2 messages before
protocol validation. After that conversion, validate ordinary SMB2
requests against SMB2_MIN_SUPPORTED_PDU_SIZE and require encryption
transform requests to contain both a transform header and an SMB2
header. This rejects truncated requests before work allocation.
Fixes: 368ba06881 ("ksmbd: check the validation of pdu_size in ksmbd_conn_handler_loop")
Cc: stable@vger.kernel.org
Reported-by: zdi-disclosures@trendmicro.com # ZDI-CAN-31063
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
In ntlm_authenticate(), destroy_previous_session() is called using a
user pointer resolved from the client-supplied NTLM blob username field
before the NTLMv2 response is validated. An authenticated attacker can
set the NTLM blob username to match a victim account and set
PreviousSessionId to the victim's session ID; destroy_previous_session()
destroys the victim's session while ksmbd_decode_ntlmssp_auth_blob()
subsequently rejects the request with -EPERM.
Move destroy_previous_session() and the prev_id assignment to after
ksmbd_decode_ntlmssp_auth_blob() returns success and use sess->user
rather than the pre-authentication lookup result. This matches the
ordering already used by krb5_authenticate(), where
destroy_previous_session() is called only after
ksmbd_krb5_authenticate() returns success.
Fixes: e2f34481b2 ("cifsd: add server-side procedures for SMB3")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/linux-cifs/20260702155449.3639773-1-james_montgomery@disroot.org/
Signed-off-by: James Montgomery <james_montgomery@disroot.org>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
set_ntacl_dacl() validates sid.num_subauth before copying an ACE, but
does not verify that the declared ACE size contains all sub-authorities
described by that field. An undersized ACE can therefore be copied
and later make the POSIX ACL deduplication walk inspect data beyond
the copied ACE boundary.
The existing initial bound check is also too small. It only ensures
that the ACE size field is accessible before set_ntacl_dacl() reads
sid.num_subauth farther into the input buffer.
Require enough input for the fixed SID header before accessing
num_subauth, reject ACEs smaller than that header, and skip ACEs
whose declared size cannot contain the complete SID. This makes the
validation consistent with the other ACE walk paths.
Reported-by: LocalHost <localhost.detect@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
check_add_overflow() unconditionally writes the truncated sum into *d
even on overflow, per its contract in include/linux/overflow.h.
The four check_add_overflow() guards in set_posix_acl_entries_dacl()
and set_ntacl_dacl() break out of the ACE-building loops on overflow,
but the truncated *size is then consumed downstream at the end of
set_ntacl_dacl():
pndacl->size = cpu_to_le16(le16_to_cpu(pndacl->size) + size);
This produces an on-wire NT ACL whose pndacl->size under-reports the
bytes actually written by the preceding fill_ace_for_sid()/memcpy()
calls, yielding a malformed ACL that can trigger out-of-bounds reads
when re-parsed by clients or ksmbd itself.
Restore *size to its pre-addition value on each overflow branch (via
`*size -= ace_sz` / `size -= nt_ace_size`) so that after the break,
*size once again holds the cumulative size of the successfully-written
ACEs. The committed ACL is then truncated-but-self-consistent rather
than malformed.
The ksmbd DACL builders are the only check_add_overflow() sites found
where an overflow path breaks out of a loop and the destination value
is consumed afterward. The other nearby break-style cases either
return -EINVAL on overflow (transport_ipc.c) or break without
consuming the overflowed destination value afterward (buildid.c).
Fixes: 299f962c0b ("ksmbd: use check_add_overflow() to prevent u16 DACL size overflow")
Assisted-by: atomcode:glm-5.2
Assisted-by: Codex:gpt-5.5
Cc: stable@vger.kernel.org
Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
set_ntacl_dacl() can stop copying ACEs before consuming the full input
DACL when size accounting overflows.
When that happens, num_aces reflects only the ACEs that were actually
copied into the output DACL, but set_posix_acl_entries_dacl() still
receives nt_num_aces and uses it to walk the existing ACE array during
dedup.
That makes the dedup walk scan past the copied ACE array and inspect
buffer tail that does not contain valid ACEs.
Split the two meanings currently carried by the NT ACE count. Pass the
number of copied NT ACEs to bound the dedup walk, and preserve the
original "input DACL had NT ACEs" state separately for the
Everyone/default ACL fallback.
This keeps the dedup walk aligned with the ACEs that are actually
present in the rebuilt DACL.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
SMB2_FLAGS_SIGNED is controlled by the incoming request and only indicates
that a signature accompanies that request. Do not use it to decide whether a
signing-required session must authenticate the request.
Reject an unsigned plaintext request before dispatch when the session
requires signing. Continue to validate signatures on signed requests,
including when signing is optional. Encrypted requests have already been
authenticated during decryption.
An OPLOCK_BREAK acknowledgment is a session request and is subject to the
same signing rule, so do not exclude it from signed-request detection.
Reported-by: Charles Vosburgh <trilobyte777@gmail.com>
Tested-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
Reviewed-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
The VFS initializes a child's POSIX ACL from the parent's default ACL and
the requested creation mode. Do not mutate the parent ACL or overwrite the
child's VFS-computed access and default ACLs afterwards.
This preserves restrictive ACL_MASK entries and prevents SMB object creation
from widening effective permissions.
Reported-by: Charles Vosburgh <trilobyte777@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
-----BEGIN PGP SIGNATURE-----
iQGzBAABCgAdFiEE6fsu8pdIjtWE/DpLiiy9cAdyT1EFAmpa3JoACgkQiiy9cAdy
T1EP7wv9FiM0oUOekxfydVFTMdpzkMpvBHcLoVSpnFjvgJZVh4gOmvyY0fRuXKRB
o40SAoBvZP5KRe6qjBxo2o+z4T6NpRY+K1ftv0xxFvgDZ3+wvyV6jEf80cLnoDAl
xqOji4PeeZfrKVwPYAFTVWhImQZ9IZuP3cBpZKqf5EWYovk43Ex/VQCqnVdAMY33
fvdUEY1u8HVVILOzHRWvo7QAlyw6SEBjdmTLqCB2mNDwBcfg1C/zuvbf8KYS4LXu
+C8F137JlSz6tplrTBGIq3FVwLxhGunZwanO3IVM8B0+FhSsedWT/qS5CbgijZxn
GpT9CBKWN/KU3YPHEFDKxcSkJAc0MA20cb9LRUdIy7uv+OnYVV2oY209dukj8RpI
05F8RC1frrdhY9vZZhjIw/x5jbrHb4cGEohGyB6pH8C1gJc1AOuFBEZ7X7dHgGqA
jjJ3bBD4f8mL7Pim37gubyCYvO/MtjEkKYWvoWxtjWewHxpJdvH+JefnOz8Cyp94
BXR4hd1r
=K5Ks
-----END PGP SIGNATURE-----
Merge tag 'v7.2-rc3-smb3-server-fixes' of git://git.samba.org/ksmbd
Pull smb server fixes from Steve French:
"ksmbd server fixes, mostly addressing malformed SMB request
handling and connection/session lifetime issues, including
two information-disclosure or memory-safety bugs in the SMB2
request/response paths.
- validate FILE_ALLOCATION_INFORMATION before block rounding to
prevent a client-controlled overflow from truncating a file.
- pin connections while asynchronous oplock and lease-break
notifications are pending.
- initialize compound SMB2 READ alignment padding, preventing
disclosure of uninitialized heap bytes.
- release the allocated alternate-stream xattr name after rename.
- size multichannel binding session-key buffers for the largest
permitted key, avoiding a stack buffer overflow.
- remove a disconnecting connection's channels from every session,
including channels whose binding state has since changed.
- serialize binding preauthentication-session lookup and update
against its teardown.
- check that every compound request element contains StructureSize2
before reading it"
* tag 'v7.2-rc3-smb3-server-fixes' of git://git.samba.org/ksmbd:
ksmbd: validate compound request size before reading StructureSize2
ksmbd: lock the binding preauth session in smb3_preauth_hash_rsp
ksmbd: remove stale channels from all sessions on teardown
ksmbd: fix stack buffer overflow in multichannel session-key copy
ksmbd: fix memory leak of xattr_stream_name in smb2_rename()
ksmbd: zero the smb2_read alignment tail to avoid an infoleak
ksmbd: pin conn during async oplock break notification
ksmbd: fix integer overflow in set_file_allocation_info()
When ksmbd validates a compound (chained) SMB2 request,
ksmbd_smb2_check_message() reads pdu->StructureSize2 without first
checking that the compound element is large enough to contain it.
StructureSize2 is a 2-byte field at offset 64
(__SMB2_HEADER_STRUCTURE_SIZE) from the start of each element.
The compound-walking logic only guarantees that a full 64-byte SMB2
header is present for the trailing element: when NextCommand is 0, len is
reduced to the number of bytes remaining after next_smb2_rcv_hdr_off. A
remote client can craft a compound request whose last element has exactly
64 bytes, so the 2-byte StructureSize2 read at offset 64 extends one byte
past the receive buffer, producing a slab-out-of-bounds read.
BUG: KASAN: slab-out-of-bounds in ksmbd_smb2_check_message (fs/smb/server/smb2misc.c:402)
Read of size 2 at addr ffff888012ae31ac by task kworker/0:1/14
The buggy address is located 172 bytes inside of allocated 173-byte region
Workqueue: ksmbd-io handle_ksmbd_work
Call Trace:
...
kasan_report (mm/kasan/report.c:595)
ksmbd_smb2_check_message (fs/smb/server/smb2misc.c:402)
handle_ksmbd_work (fs/smb/server/server.c:119)
process_one_work (kernel/workqueue.c:3314)
worker_thread (kernel/workqueue.c:3397)
kthread (kernel/kthread.c:436)
ret_from_fork (arch/x86/kernel/process.c:158)
ret_from_fork_asm (arch/x86/entry/entry_64.S:245)
Reject any compound element that is too small to hold StructureSize2
before dereferencing it.
Fixes: e2f34481b2 ("cifsd: add server-side procedures for SMB3")
Reported-by: AutonomousCodeSecurity@microsoft.com
Signed-off-by: Xiang Mei (Microsoft) <xmei5@asu.edu>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
smb3_preauth_hash_rsp() computes the SMB3.1.1 preauth integrity hash on
the response path. For a binding SESSION_SETUP it looks up the
per-connection preauth_session and reads its Preauth_HashValue.
smb2_sess_setup() frees that preauth_session under ksmbd_conn_lock().
Two SMB2 requests on one connection can run concurrently, so an unlocked
lookup and hash can use a preauth_session after another worker frees it.
Take ksmbd_conn_lock() before selecting conn->binding and hold it across
the selected preauth hash lookup and update. This preserves the existing
hash selection while preventing the lookup-to-use lifetime race.
Fixes: 1c5daa2ea9 ("ksmbd: handle channel binding with a different user")
Signed-off-by: Gil Portnoy <dddhkts1@gmail.com>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
ksmbd_sessions_deregister() removes a connection's channels from other
sessions' channel lists only while conn->binding is still set:
if (conn->binding) {
hash_for_each_safe(sessions_table, ...)
ksmbd_chann_del(conn, sess);
}
conn->binding is a transient flag: it is cleared once a binding
SESSION_SETUP completes, and also by a subsequent non-binding
SESSION_SETUP on the same connection (a reauthentication on a bound
channel, or a new SessionId==0 setup). A connection that has bound a
channel into another session's ksmbd_chann_list and then clears
conn->binding leaves that channel behind when it disconnects: the
channel, whose chann->conn points at the now freed struct ksmbd_conn,
stays on the owner session's list.
When the owning connection later tears down, the second loop
dereferences the stale channel:
xa_for_each(&sess->ksmbd_chann_list, chann_id, chann)
if (chann->conn != conn)
ksmbd_conn_set_exiting(chann->conn); /* freed */
which is a use-after-free write into the freed ksmbd_conn (the same
stale channel is also walked by show_proc_session() through /proc). The
session is leaked as well, because its channel list never empties.
Remove the conn->binding gate so a connection always removes its
channels from every session on teardown.
Fixes: faf8578c77 ("ksmbd: find bound sessions during reauthentication")
Signed-off-by: Gil Portnoy <dddhkts1@gmail.com>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
Commit 4b706360ff ("ksmbd: fix multichannel binding and enforce channel
limit") moved the binding-path session key out of the session-wide
sess->sess_key (CIFS_KEY_SIZE = 40) into a new per-channel buffer, and
sized both that buffer and the on-stack copy used during binding with
SMB2_NTLMV2_SESSKEY_SIZE (16):
struct channel {
char sess_key[SMB2_NTLMV2_SESSKEY_SIZE]; /* 16 */
...
};
ntlm_authenticate() / krb5_authenticate():
char channel_key[SMB2_NTLMV2_SESSKEY_SIZE] = {}; /* 16 */
char *auth_key = conn->binding ? channel_key : sess->sess_key;
The two writers that fill this destination still bound the copy length
against CIFS_KEY_SIZE (40), not against the 16-byte buffer:
ksmbd_decode_ntlmssp_auth_blob() (NTLM key exchange):
if (sess_key_len > CIFS_KEY_SIZE) /* 40 */
return -EINVAL;
arc4_crypt(ctx_arc4, sess_key,
(char *)authblob + sess_key_off, sess_key_len);
ksmbd_krb5_authenticate():
if (resp->session_key_len > sizeof(sess->sess_key)) /* 40 */
...
memcpy(sess_key, resp->payload, resp->session_key_len);
On a binding SESSION_SETUP, auth_key points at the 16-byte channel_key,
so a client that supplies an NTLM EncryptedRandomSessionKey of up to 40
bytes (with NTLMSSP_NEGOTIATE_KEY_EXCH), or a Kerberos ticket whose
session key is longer than 16 bytes (a normal AES256 key is 32), writes
past the 16-byte stack buffer -- up to a 24-byte kernel stack overflow.
KASAN reports it as a stack-out-of-bounds write in arc4_crypt() called
from ksmbd_decode_ntlmssp_auth_blob().
The destinations must be able to hold the full session key the length
checks already permit. Size the per-channel key buffer and the two
on-stack channel_key buffers with CIFS_KEY_SIZE, matching sess->sess_key.
Fixes: 4b706360ff ("ksmbd: fix multichannel binding and enforce channel limit")
Signed-off-by: Gil Portnoy <dddhkts1@gmail.com>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
On an SMB2 SET_INFO(FileRenameInformation) whose target names an alternate
data stream, smb2_rename() obtains a formatted stream-name string from
ksmbd_vfs_xattr_stream_name(), which allocates it with kasprintf() and
returns it through an out-param:
rc = ksmbd_vfs_xattr_stream_name(stream_name, &xattr_stream_name, ...);
if (rc)
goto out;
rc = ksmbd_vfs_setxattr(..., xattr_stream_name, ...);
if (rc < 0) {
...
goto out;
}
goto out;
xattr_stream_name is declared inside the alternate-data-stream block, but
the out: label is outside that block and frees only new_name, so it cannot
release xattr_stream_name. ksmbd_vfs_setxattr() takes a const char * and
only reads the name, so it does not take ownership either. Both the
setxattr-failure and the success path therefore leak the kasprintf()'d
string. An authenticated client with a writable share can leak kernel
memory on every stream rename, exhausting kernel memory over time.
Free xattr_stream_name after its use, before the block's goto out. The
two earlier goto out paths never assign the variable, so there is no
double-free.
Signed-off-by: Gil Portnoy <dddhkts1@gmail.com>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
Commit 6b9a2e09d4 ("ksmbd: avoid zeroing the read buffer in smb2_read()")
switched the SMB2 READ payload buffer from kvzalloc() to kvmalloc(), on the
premise that only the nbytes actually read are ever transmitted, so the
ALIGN(length, 8) tail need not be initialized.
That premise does not hold for a compound response. ksmbd_vfs_read() fills
only nbytes, leaving [nbytes, ALIGN(length, 8)) uninitialized. The aux
payload is pinned as the last response iov with iov_len == nbytes, but when
the READ is a member of a compound, init_chained_smb2_rsp() 8-byte-aligns
the previous member by extending that same iov:
new_len = ALIGN(len, 8);
work->iov[work->iov_idx].iov_len += (new_len - len);
inc_rfc1001_len(work->response_buf, new_len - len);
so up to 7 uninitialized bytes of the kvmalloc()'d slab tail are sent
to the client. When the read length is small the buffer is served from
a general kmalloc slab, so those bytes can be stale kernel-heap
contents, including pointer values -- an information leak usable to
defeat KASLR.
An authenticated client triggers it with a compound request containing a
READ whose returned nbytes is not 8-aligned (for example [READ, CLOSE] with
a 1-byte read).
Zero only the alignment tail after the read, preserving the bulk
no-zeroing optimization of 6b9a2e09d4.
Fixes: 6b9a2e09d4 ("ksmbd: avoid zeroing the read buffer in smb2_read()")
Cc: stable@vger.kernel.org
Signed-off-by: Gil Portnoy <dddhkts1@gmail.com>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
smb2_oplock_break_noti() and smb2_lease_break_noti() store a ksmbd_conn
pointer in an async ksmbd_work and then queue that work on ksmbd-io. The
work only increments conn->r_count, which prevents teardown from passing
the pending-request wait after the increment, but it does not pin the
struct ksmbd_conn object.
If connection teardown races with an oplock break notification, the last
conn reference can be dropped before the queued worker finishes. The
worker then uses the freed conn in ksmbd_conn_write() and
ksmbd_conn_r_count_dec().
Take a real conn reference when publishing the conn pointer to the async
work item, and drop it after the notification work has decremented
r_count. Apply the same lifetime rule to lease break notification, which
uses the same work->conn pattern.
Fixes: 3aa660c059 ("ksmbd: prevent connection release during oplock break notification")
Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
set_file_allocation_info() converts the client-supplied
FILE_ALLOCATION_INFORMATION::AllocationSize into a 512-byte block
count with:
alloc_blks = (le64_to_cpu(file_alloc_info->AllocationSize) + 511) >> 9;
AllocationSize is a fully client-controlled __le64 field; the only
validation performed by the caller (smb2_set_info_file(), case
FILE_ALLOCATION_INFORMATION) is that the fixed buffer is at least
sizeof(struct smb2_file_alloc_info) == 8 bytes. The value itself is
never range-checked before this arithmetic.
When AllocationSize is close to U64_MAX (e.g. 0xffffffffffffffff),
"AllocationSize + 511" wraps around mod 2^64 to a small number
(0xffffffffffffffff + 511 = 510), so alloc_blks becomes 0. Since any
existing regular file has stat.blocks > 0, the function then takes
the "shrink" branch and calls:
ksmbd_vfs_truncate(work, fp, alloc_blks * 512); /* == 0 */
silently truncating the file to size 0, even though the client asked
to grow the allocation to (what looks like) the maximum possible
size. The trailing "if (size < alloc_blks * 512) i_size_write(inode,
size);" restore is guarded by a comparison that is never true once
alloc_blks == 0, so the truncation is not undone. This lets an
authenticated SMB client that already holds an open handle with
FILE_WRITE_DATA on a file silently truncate that same file to size 0
via a single crafted SET_INFO(FILE_ALLOCATION_INFORMATION) request
advertising a near-U64_MAX AllocationSize, even though the request
asks to grow the file's allocation rather than shrink it. This is a
functional/data-loss bug, not a privilege-boundary
violation: the same client could already truncate the file via
FILE_END_OF_FILE_INFORMATION or a plain write.
Fix it by validating AllocationSize against MAX_LFS_FILESIZE, the
same upper bound the VFS itself uses to reject unrepresentable file
sizes, before doing the "+511" rounding, and rejecting oversized
values with -EINVAL. Bounding AllocationSize to
MAX_LFS_FILESIZE - 511 guarantees the "+511" addition cannot wrap,
and that the subsequent "alloc_blks * 512" values passed to
vfs_fallocate() and ksmbd_vfs_truncate() stay within a representable
loff_t as well.
No legitimate SMB client asks for an allocation size anywhere near
2^64 bytes, so this only rejects a value that was previously
silently misinterpreted as zero.
Runtime-verified on a v6.19 KASAN test stand: sending SET_INFO
(FILE_ALLOCATION_INFORMATION) with AllocationSize = 0xffffffffffffffff
against ksmbd now returns -EINVAL and leaves the target file's size
unchanged, where the unpatched kernel truncated it from 4096 to 0
bytes.
Fixes: e2f34481b2 ("cifsd: add server-side procedures for SMB3")
Cc: stable@vger.kernel.org
Signed-off-by: Ibrahim Hashimov <security@auditcode.ai>
Assisted-by: AuditCode-AI:2026.07
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
Before this change, xfstests generic/496 was not supported on ksmbd:
generic/496 ... [not run] fallocated swap not supported here
ksmbd handles SetEOF as truncate, so EOF extension alone does not
allocate backing blocks. A fallocated swapfile can therefore still
look sparse to swapon.
Request allocation for EOF-extending fallocate ranges that can be
represented by FILE_ALLOCATION_INFORMATION, and refresh the allocation
state afterwards.
With this change, xfstests generic/496 and generic/701 pass on ksmbd.
However, Samba "strict allocate = no" now exposes the real generic/701
failure: the old pass came from inflated local i_blocks, not from
server allocation. generic/213 also fails in that case because an
oversized allocation request may not return ENOSPC.
Signed-off-by: Huiwen He <hehuiwen@kylinos.cn>
Reviewed-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
Signed-off-by: Steve French <stfrench@microsoft.com>
When an SMB3 session is referenced by a binding request on an SMB2.1
connection, the request is signed with the existing session's SMB3 signing
algorithm. ksmbd instead verifies it with the new connection's SMB2.1 HMAC
algorithm, so verification fails and the client receives
STATUS_ACCESS_DENIED instead of STATUS_REQUEST_NOT_ACCEPTED.
Select the signing verifier from the referenced session dialect. Permit a
signed SESSION_SETUP without an established channel to use the SMB3 session
signing key for verification. This is limited to SESSION_SETUP so other
unbound requests remain rejected.
The rejected response must use the same existing session algorithm. When an
SMB3 session is referenced on an SMB2.1 connection, sign the SESSION_SETUP
response with the SMB3 signing path rather than the connection's SMB2.1
path.
This fixes smb2.session.bind_negative_smb3to2s.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
Binding an SMB 2.1 session to an SMB 3.x connection is invalid because the
dialects do not match. ksmbd returns STATUS_INVALID_PARAMETER. The check
fails before attaching the referenced session to the request, so the error
response lacks SMB2_FLAGS_SIGNED.
A client requiring signing checks this flag before handling the status and
reports STATUS_ACCESS_DENIED instead of STATUS_INVALID_PARAMETER. Preserve
the signed flag for a signed binding request rejected with
STATUS_INVALID_PARAMETER. The client can then apply the special error path
without attempting to validate a response using incompatible signing
algorithms.
This fixes smb2.session.bind_negative_smb2to3s.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
SMB2_SESSION_REQ_FLAG_BINDING is not supported before SMB 3.0. ksmbd maps
such a request to STATUS_REQUEST_NOT_ACCEPTED, but it rejects the request
without looking up the referenced session. The response is then sent
unsigned. A client requiring signing reports
STATUS_ACCESS_DENIED instead of the server status.
Look up the referenced session and verify the binding request with its
signing key. Keep the session reference only after successful verification
so the rejected response can be signed without providing a signing oracle.
A signed SESSION_SETUP without the binding flag can reference a session
that does not belong to the connection. Preserve SMB2_FLAGS_SIGNED on the
STATUS_USER_SESSION_DELETED response. Clients skip signature verification
for this status but still require the signed flag before propagating it.
Also restrict failed binding preauthentication cleanup to SMB 3.1.1, the
only dialect that initializes and uses that context.
This fixes smb2.session.bind_negative_smb210s.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
When an authenticated user tries to bind a channel to a session owned by a
different user, ksmbd returns STATUS_LOGON_FAILURE. Windows instead rejects
this attempt with STATUS_ACCESS_DENIED. The supplied credentials are valid
but cannot be used with the existing session.
Use a distinct internal error for a user mismatch in both NTLM and Kerberos
authentication and map it to STATUS_ACCESS_DENIED during SESSION_SETUP.
Keep ordinary authentication failures mapped to STATUS_LOGON_FAILURE.
A failed SMB 3.1.1 binding also leaves its preauthentication context on the
connection. A subsequent binding attempt for the same session reuses the
stale hash and derives an incorrect channel signing key. Remove the binding
preauthentication context on failure so a valid retry starts with a fresh
hash.
This fixes smb2.session.bind_different_user.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
A session bound to an additional connection is stored in the session
channel list, but it is not added to that connection's local session table.
After the binding exchange completes, conn->binding is cleared.
A later SESSION_SETUP reauthentication on the bound channel only searches
the local session table. It fails to find the session and returns
STATUS_USER_SESSION_DELETED instead of processing authentication and
returning STATUS_LOGON_FAILURE for invalid credentials.
If the local lookup fails, look up the session globally and accept it only
when the current connection is registered in its channel list. This keeps
unbound connections from using the session while allowing reauthentication
on an established channel.
This fixes smb2.session.bind_invalid_auth.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
When a signed request uses a session that is not registered on the
connection, ksmbd returns STATUS_USER_SESSION_DELETED before reaching the
normal response signing path. The response therefore lacks
SMB2_FLAGS_SIGNED.
Clients that require signing check this flag before handling
STATUS_USER_SESSION_DELETED and replace the server status with
STATUS_ACCESS_DENIED when it is absent. The protocol permits this error
response to skip signature verification because the connection has no
matching session key.
Preserve SMB2_FLAGS_SIGNED on the early error response when the request was
signed. This lets the client propagate STATUS_USER_SESSION_DELETED.
It fixes smb2.session.bind2.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
FILE_ALLOCATION_INFORMATION can call vfs_fallocate(). If the allocation
cannot be satisfied, vfs_fallocate() returns -ENOSPC.
smb2_set_info() did not map -ENOSPC, so ksmbd returned a generic SMB error
and the client reported EIO instead of ENOSPC. This makes the ENOSPC step
in xfstests generic/213 fail.
Map -ENOSPC and -EFBIG to STATUS_DISK_FULL in the SET_INFO error path.
Tested with xfstests generic/213 on ksmbd.
Signed-off-by: Huiwen He <hehuiwen@kylinos.cn>
Reviewed-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
Windows reports automatic write-time updates with a resolution of roughly
15 milliseconds. If a file is written and closed within that interval, a
close response requesting full information can report the write time from
the open rather than the filesystem's finer-grained mtime update.
ksmbd currently converts the filesystem mtime directly in SMB2 CLOSE, so
even a sub-millisecond write is visible to the client. This makes
smb2.timestamp_resolution.resolution1 fail because the immediate write
changes LastWriteTime.
Save the write time returned by SMB2 CREATE in the file handle. When CLOSE
requests post-query attributes, coalesce a positive mtime change smaller
than 15 milliseconds to that saved value. Larger changes remain visible,
including the test's write after a 20 millisecond delay.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
A signed multichannel SESSION_SETUP binding request can require multiple
authentication rounds. ksmbd excludes SESSION_SETUP from the signed
request check and tries to sign every binding response with the channel
signing key. The channel does not exist for
STATUS_MORE_PROCESSING_REQUIRED, so that response is sent unsigned.
Clients reject it with STATUS_ACCESS_DENIED.
The final channel signing key also needs the key exported by the binding
authentication context. Keep that key in the channel instead of
overwriting the established session key, and use the session signing key
for intermediate and failed binding responses. Retain the binding session
reference until an error response has been signed and sent.
Limit a session to 32 channels while holding the channel lock. Return
STATUS_INSUFFICIENT_RESOURCES for an additional binding, matching the
server limit expected by clients.
This fixes smb2.multichannel.generic.num_channels, which previously
failed the first binding with STATUS_ACCESS_DENIED and returned the same
status instead of STATUS_INSUFFICIENT_RESOURCES for channel 33.
Fixes: f5a544e3ba ("ksmbd: add support for SMB3 multichannel")
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
sid_to_id() currently treats the last subauthority of any owner or group
SID as a Unix uid or gid. For example, this maps Everyone (S-1-1-0) to
uid 0 and BUILTIN\Users (S-1-5-32-545) to gid 545.
When an SMB2 CREATE security descriptor contains those SIDs, ksmbd
attempts to change the newly created file to the bogus Unix ownership.
notify_change() then returns -EPERM, which makes smb2.create.aclfile fail
with NT_STATUS_SHARING_VIOLATION.
Validate the SID prefix before extracting its RID. Only server-domain
owner SIDs and S-1-22-2 Unix group SIDs have local ID representations.
Treat other valid Windows SIDs as unmapped so their original values can
still be preserved in the NT ACL xattr.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
ksmbd_close_fd_app_instance_id() looks up a prior durable handle by
AppInstanceId and closes it through opinfo->sess->file_table. This is
unsafe after the original session has been torn down. session_fd_check()
preserves reconnectable durable handles in the global table and clears
opinfo->conn/fp->conn, but opinfo->sess can still point to the freed
ksmbd_session.
Use opinfo->conn as the orphan sentinel, but make the check reliable by
serializing it with session_fd_check(). That path clears opinfo->conn
under fp->f_ci->m_lock, so hold the same lock while testing opinfo->conn
and while dereferencing opinfo->sess->file_table. Also avoid closing
through the session file table if the volatile id has already been
unpublished by session teardown.
Durable reconnect must keep the two fields consistent. Rebinding only
opinfo->conn leaves opinfo->sess pointing at the old freed session, so
a later app-instance supersede can pass the conn check and write-lock the
freed session's file table. Clear opinfo->sess when preserving a durable
handle during session teardown, and set it to the reconnecting session
when opinfo->conn is rebound in ksmbd_reopen_durable_fd().
Fixes: 16c3064970 ("ksmbd: handle durable v2 app instance id")
Reported-by: Gil Portnoy <dddhkts1@gmail.com>
Co-developed-by: Gil Portnoy <dddhkts1@gmail.com>
Signed-off-by: Gil Portnoy <dddhkts1@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
smb_grant_oplock() checks the previous oplock holder's o_fp to decide
whether a durable handle should be invalidated when the oplock break
cannot be delivered. prev_opinfo is obtained with opinfo_get_list(),
which pins only the oplock_info. It does not pin the ksmbd_file stored
in opinfo->o_fp.
A concurrent last close can unlink the opinfo from ci->m_op_list under
ci->m_lock and then free the ksmbd_file. The oplock_info can still be
kept alive by the refcount taken by opinfo_get_list(), but o_fp may
already point at freed memory by the time smb_grant_oplock() reads
is_durable, conn, or tcon.
Snapshot the previous holder's durable state while ci->m_lock is held,
then use only the copied values after dropping the lock. This keeps the
o_fp lifetime tied to the inode lock without taking an extra ksmbd_file
reference. Taking such a reference is unsafe here because smb_grant_oplock()
does not necessarily have the previous holder's session work, and dropping
the temporary reference can otherwise become the final putter.
Fixes: 26fa88dc87 ("ksmbd: invalidate durable handles on oplock break")
Reported-by: Gil Portnoy <dddhkts1@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
ksmbd_close_disconnected_durable_delete_on_close() collects disconnected
durable handles for a name being superseded by a new delete-on-close
open, drops ci->m_lock, then closes each collected handle directly with
__ksmbd_close_fd().
That bypasses the FP_CLOSED and refcount handoff used by the other close
paths. If a durable reconnect or the durable scavenger already took a
reference to the same fp, the direct __ksmbd_close_fd() can free the
ksmbd_file while that other holder still owns a live reference.
Claim the disconnected durable handle before unlinking it from m_fp_list.
While holding ci->m_lock and global_ft.lock, only take ownership when the
durable lifetime reference is the only remaining reference. Then take a
transient reference, remove the fp from global_ft, mark it FP_CLOSED, and
move it to the local dispose list. If another holder already has a
reference, leave the fp linked and let that holder complete its path.
The dispose loop then drops both references owned by the claim. This keeps
the force-close path in the same refcount handoff model as the durable
scavenger and avoids leaving a live reconnected fp detached from
m_fp_list.
Fixes: 166e4c0702 ("ksmbd: supersede disconnected delete-on-close durable handle")
Signed-off-by: Gil Portnoy <dddhkts1@gmail.com>
Co-developed-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
Two concurrent SMB2 durable reconnects (DH2C/DHnC) on the same
persistent_id race the fp->owner.name compare-read in
ksmbd_vfs_compare_durable_owner() against the kfree() in
ksmbd_reopen_durable_fd()'s reopen-success path. fp->owner.name is a
standalone kstrdup() buffer whose lifetime is independent of the fp
refcount, and the two sites share no lock: the compare reads the buffer
while the reopen frees it, so the strcmp() can dereference freed memory.
Commit 7ce4fc4001 ("ksmbd: fix durable reconnect double-bind race in
ksmbd_reopen_durable_fd") made the fp->conn claim atomic under
global_ft.lock (closing the owner.name double-free and the ksmbd_file
write-UAF), but the compare-read versus reopen-free pair was left
unserialized.
BUG: KASAN: slab-use-after-free in strcmp+0x2c/0x80
Read of size 1 by task kworker
strcmp
ksmbd_vfs_compare_durable_owner
smb2_check_durable_oplock
smb2_open
Freed by task kworker:
kfree
ksmbd_reopen_durable_fd
smb2_open
Allocated by task kworker:
kstrdup
session_fd_check
smb2_session_logoff
The buggy address belongs to the cache kmalloc-8
Serialize both sides of the race with fp->f_lock. The global durable
file-table lock still protects the durable reconnect claim, but
fp->owner.name is per-open state and does not need to block unrelated
durable table lookups or reconnects. The teardown is left at its
existing location after the reopen-success point so that an __open_id()
rollback still retains owner.name for a later legitimate reconnect to
verify.
Fixes: 49110a8ce6 ("ksmbd: validate owner of durable handle on reconnect")
Assisted-by: Henry (Claude):claude-opus-4
Signed-off-by: Gil Portnoy <dddhkts1@gmail.com>
Co-developed-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
When a cifs.ko client caches a read-handle (RH) lease via deferred close
and a conflicting open arrives, ksmbd breaks the lease and waits for the
acknowledgment in wait_for_break_ack() for up to OPLOCK_WAIT_TIME (35s).
__smb_break_all_levII_oplock() runs that wait while holding ci->m_lock
for read.
cifs.ko reacts to a handle-lease break by closing the deferred handle
rather than sending a lease break acknowledgment. That close path
(close_id_del_oplock() -> opinfo_del()) takes ci->m_lock for write and
is exactly what would wake the waiter, but it blocks on the read lock
held by the waiting thread. The break is then resolved only by the 35s
timeout, so xfstests generic/001 takes ~78s with leases enabled versus
~4s with oplocks only.
Collect the target opinfos (each pinned with a reference) while holding
ci->m_lock, then break them after releasing it, matching how
smb_grant_oplock() already breaks a conflicting lease using only a
reference. The reference keeps the opinfo (and its conn and lease)
alive across the unlocked window, and a close racing the break is
handled by the existing OPLOCK_CLOSING state check. Apply the same fix
to the parent lease break paths.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
session_fd_check() and ksmbd_reopen_durable_fd() walk ci->m_op_list with
list_for_each_entry_rcu() while holding ci->m_lock for write. That is
the local inode/oplock serializer, but the RCU-list iterator does not
currently tell lockdep about it.
Pass lockdep_is_held(&ci->m_lock) to these iterators so
CONFIG_PROVE_RCU_LIST can see the rwsem protection already in place.
This was found by our static analysis tool and then manually reviewed
against the current tree. The dynamic triage evidence is a
target-matched CONFIG_PROVE_RCU_LIST warning; the change is limited
to documenting the existing protection contract.
This is a lockdep annotation cleanup. It does not change oplock list
lifetime or durable-handle behavior.
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
smb2_validate_credit_charge() adds the request's CreditCharge to
conn->outstanding_credits when an SMB2 PDU is received, and
smb2_set_rsp_credits() subtracts it again when the response is built.
However smb2_set_rsp_credits() only runs on the normal response path:
- __process_request() returning SERVER_HANDLER_ABORT (unimplemented
command, command index out of range, signature check failure, or a
handler that sets send_no_response such as a cancelled blocking
lock) breaks out of the processing loop before set_rsp_credits() is
called;
- smb2_set_rsp_credits() itself returns early with -EINVAL (total
credit overflow or insufficient credits) before the subtraction.
On all of these paths the charge added at receive time is never
returned, so conn->outstanding_credits only grows. Because a client can
repeatedly trigger them (e.g. by sending unimplemented commands or by
issuing and cancelling blocking locks), outstanding_credits eventually
reaches total_credits and smb2_validate_credit_charge() then rejects
every subsequent request, wedging the connection.
Record the charge that was added in work->credit_charge and release any
charge still pending at the single send. exit point of
__handle_ksmbd_work(), which all abort and error paths fall through to.
smb2_set_rsp_credits() clears work->credit_charge once it has returned
the charge so the response path is unchanged and the credit is never
released twice. Paths that never charged a credit (no multi-credit
support, validation failure) leave work->credit_charge at zero and are
unaffected.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
smb2_validate_credit_charge() computes the credit charge a request is
allowed to consume from the payload size:
CreditCharge = (max(SendPayloadSize, ResponsePayloadSize) - 1)/65536 + 1
For SMB2 QUERY_INFO, the server must validate CreditCharge based on the
*maximum* of InputBufferLength and OutputBufferLength. ksmbd instead
summed the two lengths, which overestimates the required charge.
As a result a single-credit QUERY_INFO whose InputBufferLength and
OutputBufferLength each fit in 64KB but whose sum exceeds 64KB is
rejected with STATUS_INVALID_PARAMETER, even though it is a valid
request. IOCTL already uses max() of the request and response sizes;
make QUERY_INFO consistent by feeding InputBufferLength as the request
length and OutputBufferLength as the expected response length so that
smb2_validate_credit_charge() takes their maximum.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
smb2_read() allocates the read payload buffer with kvzalloc(), zeroing up
to max_read_size bytes (1MB or more with multichannel) on every read,
only to immediately overwrite the region with file data via kernel_read().
The zero-fill is pure overhead: ksmbd_vfs_read() returns the number of
bytes actually read ('nbytes'), and only those nbytes are ever consumed -
they are pinned into the response iov (ksmbd_iov_pin_rsp_read()), sent
over the RDMA channel (smb2_read_rdma_channel()), or copied by the
compression path (ksmbd_compress_response() uses iov_len == nbytes). The
ALIGN(length, 8) tail padding and any short-read remainder are never read
or transmitted, so they need not be initialized.
Use kvmalloc() instead to skip the redundant zeroing. This reduces CPU
and memory-bandwidth usage on large sequential reads.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
set_ntacl_dacl() copies each ACE from the attacker-controlled stored
security descriptor verbatim into the response DACL without checking
sid.num_subauth. The ACE bytes (including an unchecked num_subauth)
originate from an authenticated SMB2_SET_INFO(SecInfo=DACL) that is
stored raw via ksmbd_vfs_set_sd_xattr(); parse_dacl() rejects a bad ACE
with `break` rather than an error, so parse_sec_desc() still returns
success and the malformed SD reaches the xattr intact.
On a subsequent SMB2_QUERY_INFO(SecInfo=DACL) for an inode carrying a
POSIX access ACL, build_sec_desc() -> set_ntacl_dacl() ->
set_posix_acl_entries_dacl() walks the copied ACEs and reads
ntace->sid.sub_auth[ntace->sid.num_subauth - 1]
with num_subauth taken straight from the stored SD. Since sub_auth[]
is fixed at SID_MAX_SUB_AUTHORITIES (15), a crafted num_subauth (e.g.
255) drives an out-of-bounds heap read of ~1 KB with an offset fully
controlled by an authenticated client.
The sibling functions already gate this field:
parse_dacl() -- num_subauth == 0 || > SID_MAX_SUB_AUTHORITIES
parse_sid() -- num_subauth > SID_MAX_SUB_AUTHORITIES
smb_copy_sid() -- min_t(u8, num_subauth, SID_MAX_SUB_AUTHORITIES)
set_ntacl_dacl() is the lone inconsistent path that omits the check.
Add the same num_subauth validation in set_ntacl_dacl() before copying
the ACE, matching the gate already enforced by parse_dacl().
Signed-off-by: Haofeng Li <lihaofeng@kylinos.cn>
Reviewed-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
Suggested-by: Namjae Jeon <linkinjeon@kernel.org>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
parse_dacl() limits the attacker-controlled ACE count by comparing it
with the number of minimal ACEs that fit in the DACL size. The DACL size
field is 16 bits, but the expression subtracts sizeof(struct smb_acl).
Because sizeof() is unsigned, a DACL size smaller than the ACL header
underflows to a large size_t.
A malicious client can reach this with:
SMB2_SET_INFO (InfoType=SMB2_O_INFO_SECURITY)
-> smb2_set_info_sec()
-> set_info_sec()
-> parse_sec_desc()
-> parse_dacl()
-> init_acl_state(..., 0xffff)
-> init_acl_state(..., 0xffff)
-> kmalloc_objs(..., 0xffff)
Thus a malformed security descriptor can make num_aces pass the guard
and drive large temporary ACL state and pointer-array allocations.
Reject DACLs smaller than struct smb_acl before doing the subtraction,
so the ACE count check cannot be bypassed by the underflow.
Fixes: e2f34481b2 ("cifsd: add server-side procedures for SMB3")
Signed-off-by: Haofeng Li <lihaofeng@kylinos.cn>
Reviewed-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
Free ndr buffer data when ndr_encode_dos_attr() returns error
to avoid memory leak.
Signed-off-by: Qiang Liu <liuqiang@kylinos.cn>
Reviewed-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
1. When ndr_decode_v4_ntacl() fails, the code jumped to free_n_data
which only freed n.data, skipping kfree(acl.sd_buf) and leaking
the buffer. Zero-initialize struct xattr_ntacl acl, reorder error
labels to out_free to release acl.sd_buf on all error paths.
2. if (acl.sd_size < sizeof(struct smb_ntsd)) is true, original code
returned success without freeing sd_buf and left stale *pntsd.
Set rc = -EINVAL before jumping to out_free to return error code and
free buffer.
Signed-off-by: Qiang Liu <liuqiang@kylinos.cn>
Reviewed-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
ndr_encode_v4_ntacl() allocates sd_ndr.data via kzalloc() at entry.
If any subsequent ndr_write_*() call returns error during encoding,
the allocated sd_ndr.data won't be freed and causes memory leak.
Move kfree(sd_ndr.data) into out label to ensure the buffer gets
released on all success and error return paths.
Signed-off-by: Qiang Liu <liuqiang@kylinos.cn>
Reviewed-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
kernel test robot report missing kernel-doc descriptions for the 'wait_ack'
and 'inc_epoch' parameters of smb2_lease_break_noti():
Warning: fs/smb/server/oplock.c:937 function parameter 'wait_ack' not
described in 'smb2_lease_break_noti'
Warning: fs/smb/server/oplock.c:937 function parameter 'inc_epoch' not
described in 'smb2_lease_break_noti'
Document both parameters to silence the warnings.
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
Detected by Smatch.
fs/smb/server/oplock.c:1446 smb_grant_oplock()
warn: inconsistent indenting
Reported-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
ksmbd_auth_ntlmv2() derives the NTLMv2 session key into
sess->sess_key before it verifies the NTLMv2 response.
ksmbd_decode_ntlmssp_auth_blob() then continues into KEY_XCH even
when ksmbd_auth_ntlmv2() failed.
With SMB3 multichannel binding, the failed authentication operates on
an existing session and the session setup error path does not expire
binding sessions. A client can send a binding session setup with a
bad NT proof and KEY_XCH and still modify sess->sess_key before
STATUS_LOGON_FAILURE is returned.
Relevant path:
smb2_sess_setup()
-> conn->binding = true
-> ntlm_authenticate()
-> session_user()
-> ksmbd_decode_ntlmssp_auth_blob()
-> ksmbd_auth_ntlmv2()
-> calc_ntlmv2_hash()
-> hmac_md5_usingrawkey(..., sess->sess_key)
-> crypto_memneq() returns mismatch
-> KEY_XCH arc4_crypt(..., sess->sess_key, ...)
-> out_err without expiring the binding session
Derive the base session key into a local buffer and copy it to
sess->sess_key only after the proof matches. Return immediately on
authentication failure so KEY_XCH is only processed after successful
authentication.
Fixes: e2f34481b2 ("cifsd: add server-side procedures for SMB3")
Fixes: f9929ef6a2 ("ksmbd: add support for key exchange")
Cc: stable@vger.kernel.org
Signed-off-by: Haofeng Li <lihaofeng@kylinos.cn>
Reviewed-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
This patch raises `SMB3_DEFAULT_TRANS_SIZE` to 4MB to align it with
`smb2 max read/write`. This allows better I/O negotiation with modern
clients and improves sequential read/write performance on high-speed
networks.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
decode_compress_ctxt() walks CompressionAlgorithms[] using the client
supplied CompressionAlgorithmCount. That field is declared in
struct smb2_compression_capabilities_context as a fixed 4-element array,
but the number of algorithms is actually variable and clients such as
Windows advertise more than four (e.g. LZ77, LZ77+Huffman, LZNT1,
Pattern_V1 and LZ4).
The on-wire context length is already validated, so the access is within
the received buffer, but indexing the statically sized [4] array makes
UBSAN report an out-of-bounds access:
UBSAN: array-index-out-of-bounds in smb2pdu.c:1122:48
index 4 is out of range for type '__le16 [4]'
Call Trace:
smb2_handle_negotiate+0xda7/0xde0 [ksmbd]
ksmbd_smb_negotiate_common+0x27b/0x3e0 [ksmbd]
smb2_negotiate_request+0x14/0x20 [ksmbd]
handle_ksmbd_work+0x181/0x500 [ksmbd]
Walk the algorithms through a pointer so the fixed-array bounds check is
not applied, while keeping the existing length validation that bounds the
loop to the data actually received.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
The durable handle scavenger kthread waits up to DURABLE_HANDLE_MAX_TIMEOUT
(300 seconds) between scans using wait_event_timeout(), which sleeps in
TASK_UNINTERRUPTIBLE. When there are no durable handles pending expiry the
task stays in D state far longer than 120 seconds, so the hung task
detector prints a bogus "task ksmbd-durable-s blocked for more than 120
seconds" warning with a backtrace, even though the thread is only idle.
Use wait_event_interruptible_timeout() so the thread sleeps in
TASK_INTERRUPTIBLE, which the hung task detector ignores. This also suits
the already-freezable kthread. Treat a negative return (e.g. -ERESTARTSYS)
like a timeout when recomputing the next wake interval.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
ksmbd allocates both the volatile id (per-session file table) and the
persistent id (global file table) with idr_alloc_cyclic() starting at 0.
The first open after the module loads therefore gets volatile id 0 and
persistent id 0, and ksmbd returns an SMB2 FileId of {0, 0} in the create
response.
Clients treat an all-zero FileId as a null handle. smbtorture's
smb2_util_handle_empty() considers {0, 0} empty, so tests that guard the
close with it (e.g. smb2.oplock.statopen1, smb2.lease.statopen*) never
close that first handle. The leaked open keeps the inode's oplock count
non-zero, so a later batch oplock request on the same file is downgraded
to level II and the test fails.
Start the id allocation at 1 (KSMBD_START_FID) so no handle is ever
assigned a {0, 0} FileId, matching the behaviour of other SMB servers.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
