[PATCH v2] smb: client: fix integer underflow in receive_encrypted_read()

[PATCH v2] smb: client: fix integer underflow in receive_encrypted_read()

[Dudu Lu]

In receive_encrypted_read(), the length of data to read from the socket
is computed as:

  len = le32_to_cpu(tr_hdr->OriginalMessageSize) -
        server->vals->read_rsp_size;

OriginalMessageSize comes from the server's transform header and is
untrusted. If a malicious server sends a value smaller than
read_rsp_size, the unsigned subtraction wraps to a very large value
(~4GB). This value is then passed to netfs_alloc_folioq_buffer() and
cifs_read_iter_from_socket(), causing either a massive allocation
attempt that fails with -ENOMEM (DoS), or under extreme memory
pressure, potential heap corruption.

Fix by adding a check that OriginalMessageSize is at least
read_rsp_size before the subtraction. On failure, jump to
discard_data to drain the remaining PDU from the socket, preventing
desync of subsequent reads on the connection.

Signed-off-by: Dudu Lu <phx0fer@gmail.com>
---
 fs/smb/client/smb2ops.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 509fcea28a42..a2105f4b54db 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -4943,6 +4943,14 @@ receive_encrypted_read(struct TCP_Server_Info *server, struct mid_q_entry **mid,
 		goto free_dw;
 	server->total_read += rc;
 
+	if (le32_to_cpu(tr_hdr->OriginalMessageSize) <
+	    server->vals->read_rsp_size) {
+		cifs_server_dbg(VFS, "OriginalMessageSize %u too small for read response (%zu)\n",
+			le32_to_cpu(tr_hdr->OriginalMessageSize),
+			server->vals->read_rsp_size);
+		rc = -EINVAL;
+		goto discard_data;
+	}
 	len = le32_to_cpu(tr_hdr->OriginalMessageSize) -
 		server->vals->read_rsp_size;
 	dw->len = len;
-- 
2.39.3 (Apple Git-145)

Re: [PATCH v2] smb: client: fix integer underflow in receive_encrypted_read()

[Enzo Matsumiya]

On 04/15, Dudu Lu wrote:
>In receive_encrypted_read(), the length of data to read from the socket
>is computed as:
>
>  len = le32_to_cpu(tr_hdr->OriginalMessageSize) -
>        server->vals->read_rsp_size;
>
>OriginalMessageSize comes from the server's transform header and is
>untrusted. If a malicious server sends a value smaller than
>read_rsp_size, the unsigned subtraction wraps to a very large value
>(~4GB). This value is then passed to netfs_alloc_folioq_buffer() and
>cifs_read_iter_from_socket(), causing either a massive allocation
>attempt that fails with -ENOMEM (DoS), or under extreme memory
>pressure, potential heap corruption.
>
>Fix by adding a check that OriginalMessageSize is at least
>read_rsp_size before the subtraction. On failure, jump to
>discard_data to drain the remaining PDU from the socket, preventing
>desync of subsequent reads on the connection.
>
>Signed-off-by: Dudu Lu <phx0fer@gmail.com>
>---
> fs/smb/client/smb2ops.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
>diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
>index 509fcea28a42..a2105f4b54db 100644
>--- a/fs/smb/client/smb2ops.c
>+++ b/fs/smb/client/smb2ops.c
>@@ -4943,6 +4943,14 @@ receive_encrypted_read(struct TCP_Server_Info *server, struct mid_q_entry **mid,
> 		goto free_dw;
> 	server->total_read += rc;
>
>+	if (le32_to_cpu(tr_hdr->OriginalMessageSize) <
>+	    server->vals->read_rsp_size) {
>+		cifs_server_dbg(VFS, "OriginalMessageSize %u too small for read response (%zu)\n",
>+			le32_to_cpu(tr_hdr->OriginalMessageSize),
>+			server->vals->read_rsp_size);
>+		rc = -EINVAL;
>+		goto discard_data;
>+	}

You could replace it with:

	if (check_sub_overflow(le32_to_cpu(tr_hdr->OriginalMessageSize),
			       server->vals->read_rsp_size, &len) {
		...
	}

for subtraction + check in one shot.

Patch is ok nonetheless.


Cheers,

Enzo