From c0ed656c94ffa11e1949ed2e4cc68469aa0d0cd0 Mon Sep 17 00:00:00 2001 From: Brian Gianforcaro Date: Sun, 13 Mar 2022 20:07:31 -0700 Subject: [PATCH] Kernel: Fix buffer overflow in VirtIOGPU create_3d_resource(..) This code attempts to copy the `Protocol::Resource3DSpecification` struct into request, starting at `Protocol::ResourceCreate3D::target` member of the `Protocol::ResourceCreate3D` struct. The problem is that the `Protocol::Resource3DSpecification` struct does not having the trailing `u32 padding` that the `ResourceCreate3D` struct has. Leading to memcopy overrunning the struct and corrupting 32 bits of data trailing the struct. Found by SonarCloud: - Memory copy function overflows the destination buffer. --- Kernel/Graphics/VirtIOGPU/GPU3DDevice.cpp | 3 ++- Kernel/Graphics/VirtIOGPU/GraphicsAdapter.cpp | 5 ++++- Kernel/Graphics/VirtIOGPU/Protocol.h | 1 + 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/Kernel/Graphics/VirtIOGPU/GPU3DDevice.cpp b/Kernel/Graphics/VirtIOGPU/GPU3DDevice.cpp index 0367376237..c7b8e34cd9 100644 --- a/Kernel/Graphics/VirtIOGPU/GPU3DDevice.cpp +++ b/Kernel/Graphics/VirtIOGPU/GPU3DDevice.cpp @@ -114,7 +114,8 @@ ErrorOr GPU3DDevice::ioctl(OpenFileDescription& description, unsigned requ .array_size = spec.array_size, .last_level = spec.last_level, .nr_samples = spec.nr_samples, - .flags = spec.flags + .flags = spec.flags, + .padding = 0, }; MutexLocker locker(m_graphics_adapter.operation_lock()); auto resource_id = m_graphics_adapter.create_3d_resource(resource_spec).value(); diff --git a/Kernel/Graphics/VirtIOGPU/GraphicsAdapter.cpp b/Kernel/Graphics/VirtIOGPU/GraphicsAdapter.cpp index 2ec8b461e3..23376d31f5 100644 --- a/Kernel/Graphics/VirtIOGPU/GraphicsAdapter.cpp +++ b/Kernel/Graphics/VirtIOGPU/GraphicsAdapter.cpp @@ -284,7 +284,10 @@ ResourceID GraphicsAdapter::create_3d_resource(Protocol::Resource3DSpecification request.resource_id = resource_id.value(); // TODO: Abstract this out a bit more u32* start_of_copied_fields = &request.target; - memcpy(start_of_copied_fields, &resource_3d_specification, sizeof(Protocol::Resource3DSpecification)); + + // Validate that the sub copy from the resource_3d_specification to the offset of the request fits. + static_assert((sizeof(request) - offsetof(Protocol::ResourceCreate3D, target) == sizeof(resource_3d_specification))); + memcpy(start_of_copied_fields, &resource_3d_specification, sizeof(resource_3d_specification)); synchronous_virtio_gpu_command(start_of_scratch_space(), sizeof(request), sizeof(response)); diff --git a/Kernel/Graphics/VirtIOGPU/Protocol.h b/Kernel/Graphics/VirtIOGPU/Protocol.h index e8c3483185..374fab657c 100644 --- a/Kernel/Graphics/VirtIOGPU/Protocol.h +++ b/Kernel/Graphics/VirtIOGPU/Protocol.h @@ -328,6 +328,7 @@ struct Resource3DSpecification { u32 last_level; u32 nr_samples; u32 flags; + u32 padding; }; }