[PATCH v2] smb: client: fix fiemap wild read on empty truncated response

[PATCH v2] smb: client: fix fiemap wild read on empty truncated response

[Abdifatah Suruur]

smb3_fiemap() treats -E2BIG from SMB2_ioctl() as a truncated range blob
and loops for the next chunk. If the server reports
STATUS_BUFFER_OVERFLOW with OutputCount == 0, SMB2_ioctl() returns
-E2BIG with out_data == NULL and out_data_len == 0, and the existing
length checks accept the empty result. The loop then evaluates

	next = out_data[num - 1].file_offset + ...

with num == 0, reading 16 bytes before NULL.

Reject a truncated range blob that contains no range, so the resume
offset is never computed from an empty response. Also require the last
range in a truncated blob to advance the resume offset without
overflowing, and to stay within the requested range; a malicious or
broken server could otherwise make no progress or underflow len and loop
forever. Free the previous response buffer before retrying.

Reproduced on 6.6.87.2 with a malicious SMB2 server that answers
FSCTL_QUERY_ALLOCATED_RANGES with STATUS_BUFFER_OVERFLOW and
OutputCount == 0; an FS_IOC_FIEMAP ioctl on the mount oopses in
cifs_fiemap() with CR2 = fffffffffffffff8. The code path is unchanged
on mainline.

Cc: stable@vger.kernel.org
Signed-off-by: Abdifatah Suruur <suruurism@gmail.com>
---
v2:
- Validate the resume offset in a truncated range blob (overflow, forward
  progress and remaining length) and free the previous response buffer
  before retrying, addressing the sashiko review of v1.

 fs/smb/client/smb2ops.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 20624f5dff9b..56b3b183db4b 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -4360,6 +4360,15 @@ static int smb3_fiemap(struct cifs_tcon *tcon,
 		goto out;
 	}
 
+	/*
+	 * A truncated range blob must contain at least one range, otherwise
+	 * there is no offset to resume from and out_data is NULL.
+	 */
+	if (!last_blob && out_data_len == 0) {
+		rc = -EINVAL;
+		goto out;
+	}
+
 	num = out_data_len / sizeof(struct file_allocated_range_buffer);
 	for (i = 0; i < num; i++) {
 		flags = 0;
@@ -4380,10 +4389,31 @@ static int smb3_fiemap(struct cifs_tcon *tcon,
 	}
 
 	if (!last_blob) {
-		next = le64_to_cpu(out_data[num - 1].file_offset) +
-		  le64_to_cpu(out_data[num - 1].length);
-		len = len - (next - start);
+		u64 last_start, last_len;
+
+		last_start = le64_to_cpu(out_data[num - 1].file_offset);
+		last_len = le64_to_cpu(out_data[num - 1].length);
+
+		/*
+		 * A truncated range blob must advance the resume offset and
+		 * stay within the requested range.  A malicious or broken
+		 * server could otherwise overflow the offset, make no
+		 * progress, or underflow len and keep us looping forever.
+		 */
+		if (check_add_overflow(last_start, last_len, &next) ||
+		    next <= start) {
+			rc = -EINVAL;
+			goto out;
+		}
+		if (next - start >= len) {
+			rc = 0;
+			goto out;
+		}
+
+		len -= next - start;
 		start = next;
+		kfree(out_data);
+		out_data = NULL;
 		goto again;
 	}
 
-- 
2.53.0

Re: [PATCH v2] smb: client: fix fiemap wild read on empty truncated response

[Paulo Alcantara]

Abdifatah Suruur <suruurism@gmail.com> writes:

> smb3_fiemap() treats -E2BIG from SMB2_ioctl() as a truncated range blob
> and loops for the next chunk. If the server reports
> STATUS_BUFFER_OVERFLOW with OutputCount == 0, SMB2_ioctl() returns
> -E2BIG with out_data == NULL and out_data_len == 0, and the existing
> length checks accept the empty result. The loop then evaluates
>
> 	next = out_data[num - 1].file_offset + ...
>
> with num == 0, reading 16 bytes before NULL.
>
> Reject a truncated range blob that contains no range, so the resume
> offset is never computed from an empty response. Also require the last
> range in a truncated blob to advance the resume offset without
> overflowing, and to stay within the requested range; a malicious or
> broken server could otherwise make no progress or underflow len and loop
> forever. Free the previous response buffer before retrying.
> ...

Applied.