[PATCH 1/2] smb: client: fix potential OOB in cifs_dump_detail()

[PATCH 1/2] smb: client: fix potential OOB in cifs_dump_detail()

[Paulo Alcantara]

Validate SMB message with ->check_message() before calling
->calc_smb_size().

Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
---
 fs/smb/client/cifs_debug.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
index 5596c9f30ccb..60027f5aebe8 100644
--- a/fs/smb/client/cifs_debug.c
+++ b/fs/smb/client/cifs_debug.c
@@ -40,11 +40,13 @@ void cifs_dump_detail(void *buf, struct TCP_Server_Info *server)
 #ifdef CONFIG_CIFS_DEBUG2
 	struct smb_hdr *smb = buf;
 
-	cifs_dbg(VFS, "Cmd: %d Err: 0x%x Flags: 0x%x Flgs2: 0x%x Mid: %d Pid: %d\n",
-		 smb->Command, smb->Status.CifsError,
-		 smb->Flags, smb->Flags2, smb->Mid, smb->Pid);
-	cifs_dbg(VFS, "smb buf %p len %u\n", smb,
-		 server->ops->calc_smb_size(smb));
+	cifs_dbg(VFS, "Cmd: %d Err: 0x%x Flags: 0x%x Flgs2: 0x%x Mid: %d Pid: %d Wct: %d\n",
+		 smb->Command, smb->Status.CifsError, smb->Flags,
+		 smb->Flags2, smb->Mid, smb->Pid, smb->WordCount);
+	if (!server->ops->check_message(buf, server->total_read, server)) {
+		cifs_dbg(VFS, "smb buf %p len %u\n", smb,
+			 server->ops->calc_smb_size(smb));
+	}
 #endif /* CONFIG_CIFS_DEBUG2 */
 }
 
-- 
2.43.0

[PATCH 2/2] smb: client: fix potential OOB in smb2_dump_detail()

[Paulo Alcantara]

Validate SMB message with ->check_message() before calling
->calc_smb_size().

This fixes CVE-2023-6610.

Reported-by: j51569436@gmail.com
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218219
Signed-off-by: Paulo Alcantara <pc@manguebit.com>
---
 fs/smb/client/smb2ops.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 62b0a8df867b..66b310208545 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -403,8 +403,10 @@ smb2_dump_detail(void *buf, struct TCP_Server_Info *server)
 	cifs_server_dbg(VFS, "Cmd: %d Err: 0x%x Flags: 0x%x Mid: %llu Pid: %d\n",
 		 shdr->Command, shdr->Status, shdr->Flags, shdr->MessageId,
 		 shdr->Id.SyncId.ProcessId);
-	cifs_server_dbg(VFS, "smb buf %p len %u\n", buf,
-		 server->ops->calc_smb_size(buf));
+	if (!server->ops->check_message(buf, server->total_read, server)) {
+		cifs_server_dbg(VFS, "smb buf %p len %u\n", buf,
+				server->ops->calc_smb_size(buf));
+	}
 #endif
 }
 
-- 
2.43.0

Re: [PATCH 2/2] smb: client: fix potential OOB in smb2_dump_detail()

[Maximilian Heyne]

On Sat, Dec 16, 2023 at 01:10:05AM -0300, Paulo Alcantara wrote:
> Validate SMB message with ->check_message() before calling
> ->calc_smb_size().
> 
> This fixes CVE-2023-6610.
> 
> Reported-by: j51569436@gmail.com
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218219
> Signed-off-by: Paulo Alcantara <pc@manguebit.com>
> ---
>  fs/smb/client/smb2ops.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index 62b0a8df867b..66b310208545 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -403,8 +403,10 @@ smb2_dump_detail(void *buf, struct TCP_Server_Info *server)
>  	cifs_server_dbg(VFS, "Cmd: %d Err: 0x%x Flags: 0x%x Mid: %llu Pid: %d\n",
>  		 shdr->Command, shdr->Status, shdr->Flags, shdr->MessageId,
>  		 shdr->Id.SyncId.ProcessId);
> -	cifs_server_dbg(VFS, "smb buf %p len %u\n", buf,
> -		 server->ops->calc_smb_size(buf));
> +	if (!server->ops->check_message(buf, server->total_read, server)) {

Why is this check negated? Sorry for this stupid question. I'm not
familiar with this code but only stumbled upon this commit due to a CVE.

> +		cifs_server_dbg(VFS, "smb buf %p len %u\n", buf,
> +				server->ops->calc_smb_size(buf));
> +	}
>  #endif
>  }
>  
> -- 
> 2.43.0
> 



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH 2/2] smb: client: fix potential OOB in smb2_dump_detail()

[Paulo Alcantara]

Maximilian Heyne <mheyne@amazon.de> writes:

> On Sat, Dec 16, 2023 at 01:10:05AM -0300, Paulo Alcantara wrote:
>> Validate SMB message with ->check_message() before calling
>> ->calc_smb_size().
>> 
>> This fixes CVE-2023-6610.
>> 
>> Reported-by: j51569436@gmail.com
>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218219
>> Signed-off-by: Paulo Alcantara <pc@manguebit.com>
>> ---
>>  fs/smb/client/smb2ops.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
>> index 62b0a8df867b..66b310208545 100644
>> --- a/fs/smb/client/smb2ops.c
>> +++ b/fs/smb/client/smb2ops.c
>> @@ -403,8 +403,10 @@ smb2_dump_detail(void *buf, struct TCP_Server_Info *server)
>>  	cifs_server_dbg(VFS, "Cmd: %d Err: 0x%x Flags: 0x%x Mid: %llu Pid: %d\n",
>>  		 shdr->Command, shdr->Status, shdr->Flags, shdr->MessageId,
>>  		 shdr->Id.SyncId.ProcessId);
>> -	cifs_server_dbg(VFS, "smb buf %p len %u\n", buf,
>> -		 server->ops->calc_smb_size(buf));
>> +	if (!server->ops->check_message(buf, server->total_read, server)) {
>
> Why is this check negated? Sorry for this stupid question. I'm not
> familiar with this code but only stumbled upon this commit due to a
> CVE.

Because smb2_check_message() returns 0 for a valid SMB response and then
it would be safe to call ->calc_smb_size() as we know the header has a
valid Command (< NUMBER_OF_SMB2_COMMANDS).

[PATCH v2] smb: client: fix potential OOB in smb2_dump_detail()

[Paulo Alcantara]

Validate SMB message with ->check_message() before calling
->calc_smb_size().

This fixes CVE-2023-6610.

Reported-by: j51569436@gmail.com
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218219
Signed-off-by: Paulo Alcantara <pc@manguebit.com>
---
Steve, I tried to make as little changes as possible for v1 to make
backporting easier, but unfortunately we still need to handle "@len <
@pdu_size" case in smb2_check_message() otherwise we could call
smb2_calc_size() with an invalid command, too.

 fs/smb/client/smb2misc.c | 30 +++++++++++++++---------------
 fs/smb/client/smb2ops.c  |  6 ++++--
 2 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/fs/smb/client/smb2misc.c b/fs/smb/client/smb2misc.c
index e20b4354e703..82b84a4941dd 100644
--- a/fs/smb/client/smb2misc.c
+++ b/fs/smb/client/smb2misc.c
@@ -173,6 +173,21 @@ smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *server)
 	}
 
 	mid = le64_to_cpu(shdr->MessageId);
+	if (check_smb2_hdr(shdr, mid))
+		return 1;
+
+	if (shdr->StructureSize != SMB2_HEADER_STRUCTURE_SIZE) {
+		cifs_dbg(VFS, "Invalid structure size %u\n",
+			 le16_to_cpu(shdr->StructureSize));
+		return 1;
+	}
+
+	command = le16_to_cpu(shdr->Command);
+	if (command >= NUMBER_OF_SMB2_COMMANDS) {
+		cifs_dbg(VFS, "Invalid SMB2 command %d\n", command);
+		return 1;
+	}
+
 	if (len < pdu_size) {
 		if ((len >= hdr_size)
 		    && (shdr->Status != 0)) {
@@ -193,21 +208,6 @@ smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *server)
 		return 1;
 	}
 
-	if (check_smb2_hdr(shdr, mid))
-		return 1;
-
-	if (shdr->StructureSize != SMB2_HEADER_STRUCTURE_SIZE) {
-		cifs_dbg(VFS, "Invalid structure size %u\n",
-			 le16_to_cpu(shdr->StructureSize));
-		return 1;
-	}
-
-	command = le16_to_cpu(shdr->Command);
-	if (command >= NUMBER_OF_SMB2_COMMANDS) {
-		cifs_dbg(VFS, "Invalid SMB2 command %d\n", command);
-		return 1;
-	}
-
 	if (smb2_rsp_struct_sizes[command] != pdu->StructureSize2) {
 		if (command != SMB2_OPLOCK_BREAK_HE && (shdr->Status == 0 ||
 		    pdu->StructureSize2 != SMB2_ERROR_STRUCTURE_SIZE2_LE)) {
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 62b0a8df867b..66b310208545 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -403,8 +403,10 @@ smb2_dump_detail(void *buf, struct TCP_Server_Info *server)
 	cifs_server_dbg(VFS, "Cmd: %d Err: 0x%x Flags: 0x%x Mid: %llu Pid: %d\n",
 		 shdr->Command, shdr->Status, shdr->Flags, shdr->MessageId,
 		 shdr->Id.SyncId.ProcessId);
-	cifs_server_dbg(VFS, "smb buf %p len %u\n", buf,
-		 server->ops->calc_smb_size(buf));
+	if (!server->ops->check_message(buf, server->total_read, server)) {
+		cifs_server_dbg(VFS, "smb buf %p len %u\n", buf,
+				server->ops->calc_smb_size(buf));
+	}
 #endif
 }
 
-- 
2.43.0