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 <[email protected]> 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/[email protected]?part=1
