[PATCH] drm/vmwgfx: validate backup buffer size in command buffer processing

[PATCH] drm/vmwgfx: validate backup buffer size in command buffer processing

[Zack Rusin]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

vmw_cmd_res_switch_backup lets userspace bind an arbitrary MOB to a
resource without checking that the MOB is large enough for the resource's
guest_memory_size. The dirty-tracking bitmap is allocated based on the
MOB size, but later accessed using the (larger) surface size, causing an
out-of-bounds write in the bitmap that corrupts adjacent slab memory.

Add a check that backup_offset + res->guest_memory_size does not exceed
the buffer size before allowing the backup switch.

Fixes: a97e21923b42 ("drm/vmwgfx: Hook up guest-backed surfaces")
Cc: stable@vger.kernel.org
Reported-by: Zero Day Initiative <zdi-disclosures@trendmicro.com>
Reported-by: zdi-disclosures@trendmicro.com # ZDI-CAN-34068
Closes: https://lore.kernel.org/all/BYAPR01MB38301CFC012E873340F77E3D80662@BYAPR01MB3830.prod.exchangelabs.com/
---
 drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index a9136a6523cb..0156aee84c9f 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
@@ -1718,6 +1718,7 @@ static int vmw_cmd_res_switch_backup(struct vmw_private *dev_priv,
 {
 	struct vmw_bo *vbo;
 	void *info;
+	u64 end;
 	int ret;
 
 	info = vmw_execbuf_info_from_res(sw_context, res);
@@ -1728,6 +1729,13 @@ static int vmw_cmd_res_switch_backup(struct vmw_private *dev_priv,
 	if (ret)
 		return ret;
 
+	if (check_add_overflow((u64)backup_offset,
+			       (u64)res->guest_memory_size, &end) ||
+	    end > vbo->tbo.base.size) {
+		VMW_DEBUG_USER("Backup buffer too small for resource.\n");
+		return -EINVAL;
+	}
+
 	vmw_validation_res_switch_backup(sw_context->ctx, info, vbo,
 					 backup_offset);
 	return 0;
-- 
2.53.0


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5414 bytes --]

Re: [PATCH] drm/vmwgfx: validate backup buffer size in command buffer processing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/vmwgfx: uninitialized guest_memory_size read in vmw_cmd_res_switch_backup()
--

commit 541f6121a2573592c8475e994e0c33c0a080bc71
Author: Zack Rusin <zack.rusin@broadcom.com>

drm/vmwgfx: validate backup buffer size in command buffer processing

This commit adds a bounds check in vmw_cmd_res_switch_backup() to ensure
that backup_offset plus res->guest_memory_size does not overflow or exceed
the backing MOB size before switching a resource's backup buffer.

> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> index a9136a6523cba..0156aee84c9fb 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> @@ -1718,6 +1718,7 @@ static int vmw_cmd_res_switch_backup(struct vmw_private *dev_priv,
>  {
>  	struct vmw_bo *vbo;
>  	void *info;
> +	u64 end;
>  	int ret;
>  
>  	info = vmw_execbuf_info_from_res(sw_context, res);
> @@ -1728,6 +1729,13 @@ static int vmw_cmd_res_switch_backup(struct vmw_private *dev_priv,
>  	if (ret)
>  		return ret;
>  
> +	if (check_add_overflow((u64)backup_offset,
> +			       (u64)res->guest_memory_size, &end) ||
> +	    end > vbo->tbo.base.size) {
> +		VMW_DEBUG_USER("Backup buffer too small for resource.\n");
> +		return -EINVAL;
> +	}

[Severity: High]
Can this check read uninitialized slab memory from res->guest_memory_size
when binding DX shaders or DX streamoutput resources?

In drivers/gpu/drm/vmwgfx/vmwgfx_resource.c, vmw_resource_init() initializes
struct vmw_resource fields individually without setting
res->guest_memory_size:

drivers/gpu/drm/vmwgfx/vmwgfx_resource.c:vmw_resource_init() {
	...
	res->id = -1;
	res->guest_memory_bo = NULL;
	res->guest_memory_offset = 0;
	res->guest_memory_dirty = false;
	res->res_dirty = false;
	res->coherent = false;
	res->used_prio = 3;
	res->dirty = NULL;
	...
}

For DX shaders (submitted via SVGA_3D_CMD_DX_DEFINE_SHADER and bound via
SVGA_3D_CMD_DX_BIND_SHADER), vmw_cmd_dx_define_shader() calls
vmw_dx_shader_add() in drivers/gpu/drm/vmwgfx/vmwgfx_shader.c, which
allocates struct vmw_dx_shader with kmalloc_obj() (without __GFP_ZERO) and
calls vmw_resource_init() without ever initializing
shader->res.guest_memory_size:

drivers/gpu/drm/vmwgfx/vmwgfx_shader.c:vmw_dx_shader_add() {
	...
	shader = kmalloc_obj(*shader);
	if (!shader) {
		return -ENOMEM;
	}

	res = &shader->res;
	...
	ret = vmw_resource_init(dev_priv, res, true,
				vmw_dx_shader_res_free, &vmw_dx_shader_func);
	...
}

Similarly, for DX streamoutput resources (submitted via
SVGA_3D_CMD_DX_DEFINE_STREAMOUTPUT and bound via
SVGA_3D_CMD_DX_BIND_STREAMOUTPUT), vmw_dx_streamoutput_add() in
drivers/gpu/drm/vmwgfx/vmwgfx_streamoutput.c allocates struct
vmw_dx_streamoutput with kmalloc_obj() without zeroing. When
vmw_cmd_dx_bind_streamoutput() in drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
processes the bind command, it calls vmw_dx_streamoutput_set_size(), which
stores cmd->body.sizeInBytes in so->size rather than
res->guest_memory_size before calling vmw_cmd_res_switch_backup():

drivers/gpu/drm/vmwgfx/vmwgfx_streamoutput.c:vmw_dx_streamoutput_set_size() {
	struct vmw_dx_streamoutput *so = vmw_res_to_dx_streamoutput(res);

	so->size = size;
}

When vmw_cmd_dx_bind_shader() or vmw_cmd_dx_bind_streamoutput() then calls
vmw_cmd_res_switch_backup(), res->guest_memory_size holds uninitialized heap
memory. Doesn't this cause valid DX shader and streamoutput bindings to fail
with -EINVAL whenever the uninitialized value is large, allow userspace to
probe the 64-bit uninitialized slab value by varying offsetInBytes or the
MOB size, and leave the actual shader and streamoutput byte sizes
unvalidated against the MOB size?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026213712.244907-1-zack.rusin@broadcom.com?part=1