Re: [PATCH] drm/vboxvideo: validate VBVA ring buffer offsets and cursor dimensions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/vboxvideo: VRAM size validation uses uninitialized num_crtcs
- [High] drm/vboxvideo: TOCTOU race on vbva->data_len allows divide-by-zero
--

commit b1a081469cd7aae1113cfd3057268becc7a4f37a
Author: Hui Peng <benquike@gmail.com>

    drm/vboxvideo: validate VBVA ring buffer offsets and cursor dimensions
    
    In drivers/gpu/drm/vboxvideo/ (vbox_main.c, vbox_mode.c, vbva_base.c),
    validate VBVA ring buffer offsets against buffer_length and check cursor
    dimensions before copying cursor data into HGSMI buffers.

> diff --git a/drivers/gpu/drm/vboxvideo/vbox_main.c b/drivers/gpu/drm/vboxvideo/vbox_main.c
> index aa6664542b20d..8cf326661b57f 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_main.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_main.c
> @@ -115,6 +115,10 @@ int vbox_hw_init(struct vbox_private *vbox)
>  
>  	DRM_INFO("VRAM %08x\n", vbox->full_vram_size);
>  
> +	if (vbox->full_vram_size < VBVA_ADAPTER_INFORMATION_SIZE +
> +				   vbox->num_crtcs * VBVA_MIN_BUFFER_SIZE)
> +		return -EINVAL;
> +

[Severity: High]
Does this code use an uninitialized value for vbox->num_crtcs?

Looking at vbox_hw_init(), it appears the memory for vbox is allocated with
devm_drm_dev_alloc(), which zero-initializes the memory. Since
vbox->num_crtcs is not populated until later in the function via
hgsmi_query_conf(), is it strictly 0 at the time of this check?

Could this cause the validation to evaluate 0 * VBVA_MIN_BUFFER_SIZE,
allowing an inadequate VRAM size to slip through and resulting in a
potential out-of-bounds mapping later in vbox_accel_init()?

>  	ret = pcim_request_region(pdev, 0, "vboxvideo");
>  	if (ret)
>  		return ret;

[ ... ]

> diff --git a/drivers/gpu/drm/vboxvideo/vbva_base.c b/drivers/gpu/drm/vboxvideo/vbva_base.c
> index 36bc9824ec3f9..3a752be06f4e1 100644
> --- a/drivers/gpu/drm/vboxvideo/vbva_base.c
> +++ b/drivers/gpu/drm/vboxvideo/vbva_base.c

[ ... ]

> @@ -93,6 +110,9 @@ bool vbva_write(struct vbva_buf_ctx *vbva_ctx, struct gen_pool *ctx,
>  		vbva_buffer_place_data_at(vbva_ctx, p, chunk,
>  					  vbva->free_offset);
>  
> +		if (!vbva->data_len)
> +			return false;
> +
>  		vbva->free_offset = (vbva->free_offset + chunk) %
>  				    vbva->data_len;

[Severity: High]
Could this check introduce a time-of-check to time-of-use vulnerability?

Because vbva resides in VRAM, which is shared memory accessible by the host,
and the check does not use READ_ONCE() or store the length locally, might
the compiler emit two distinct memory loads for vbva->data_len?

Can a buggy host hypervisor bypass this check by setting data_len to a valid
non-zero value, then immediately flipping it to 0 before the modulo operation
executes, potentially leading to a divide-by-zero kernel panic in the guest?

>  		record->len_and_flags += chunk;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919223512.3889313-1-benquike@gmail.com?part=1