Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
New issues: - [High] Strict error check on pan display may cause probe to unconditionally fail due to hardware deduplication. - [High] State desynchronization between kernel and hardware on timeout. Pre-existing issues: - [High] Unhandled hardware interrupt leak causing an interrupt storm. - [High] Hardware DMA Use-After-Free during probe error and device removal. - [High] DMA free size mismatch due to mutable screen resolution variables. - [Medium] Unconditional timeout delay and false-positive error during probe. - [Medium] Integer overflow in framebuffer DMA size calculation. -- commit e61a80b8db4bb16d36d4a68c1bf1c49ae2fe7019 Author: Pengpeng Hou <[email protected]> fbdev: goldfishfb: fail pan display on base-update timeout goldfish_fb_pan_display() waits for the device to acknowledge the new framebuffer base, but it only logs a timeout and still reports success. The probe path also ignores the initial pan-display result before registering the framebuffer. This patch returns -ETIMEDOUT when the base-update acknowledgment does not arrive. > diff --git a/drivers/video/fbdev/goldfishfb.c > b/drivers/video/fbdev/goldfishfb.c > index c9871281bc1da..e5d79886ea66b 100644 > --- a/drivers/video/fbdev/goldfishfb.c > +++ b/drivers/video/fbdev/goldfishfb.c > @@ -138,10 +138,12 @@ static int goldfish_fb_pan_display(struct > fb_var_screeninfo *var, > writel(fb->fb.fix.smem_start + fb->fb.var.xres * 2 * var->yoffset, > fb->reg_base + FB_SET_BASE); > spin_unlock_irqrestore(&fb->lock, irq_flags); > - wait_event_timeout(fb->wait, > - fb->base_update_count != base_update_count, HZ / 15); > - if (fb->base_update_count == base_update_count) > + if (!wait_event_timeout(fb->wait, > + fb->base_update_count != base_update_count, > + HZ / 15)) { > pr_err("%s: timeout waiting for base update\n", __func__); > + return -ETIMEDOUT; [Severity: High] Does returning an error here permanently desynchronize the kernel's fbdev state from the actual hardware scanout state? By returning -ETIMEDOUT, the generic fbdev core refuses to update the xoffset and yoffset software state. However, the hardware has already been programmed with the new offset via FB_SET_BASE and will switch to it when it finishes. > + } > return 0; > } > > @@ -251,7 +253,9 @@ static int goldfish_fb_probe(struct platform_device *pdev) [Severity: Medium] This is a pre-existing issue, but can the framesize calculation overflow? Earlier in goldfish_fb_probe(), width and height are read directly from 32-bit device registers: width = readl(fb->reg_base + FB_GET_WIDTH); height = readl(fb->reg_base + FB_GET_HEIGHT); ... framesize = width * height * 2 * 2; If large dimensions are provided, the 32-bit arithmetic could wrap around, resulting in a small allocation while the hardware attempts to scan out a much larger region. [Severity: Medium] This is a pre-existing issue, but does calling fb_set_var() earlier in this function cause an unconditional timeout delay? In goldfish_fb_probe(), fb_set_var() is called before request_irq(): ret = fb_set_var(&fb->fb, &fb->fb.var); if (ret) goto err_fb_set_var_failed; ret = request_irq(fb->irq, goldfish_fb_interrupt, ...); fb_set_var() implicitly invokes goldfish_fb_pan_display(), which waits for an interrupt. Because the interrupt handler is not yet registered and interrupts are not enabled, this wait will always time out. > goto err_request_irq_failed; > > writel(FB_INT_BASE_UPDATE_DONE, fb->reg_base + FB_INT_ENABLE); > - goldfish_fb_pan_display(&fb->fb.var, &fb->fb); /* updates base */ > + ret = goldfish_fb_pan_display(&fb->fb.var, &fb->fb); /* updates base */ > + if (ret) > + goto err_pan_display_failed; [Severity: High] Will this strict error check cause the probe to unconditionally fail due to hardware deduplication? The probe path implicitly writes the base offset to FB_SET_BASE during fb_set_var() (which currently times out). It then explicitly calls goldfish_fb_pan_display() here with the exact same offset, writing to FB_SET_BASE again. If the virtual hardware deduplicates identical writes and does not assert an interrupt for a redundant update, the wait will time out. Because this patch now aborts on timeout, won't the probe unconditionally fail? > > ret = register_framebuffer(&fb->fb); > if (ret) > @@ -259,6 +263,7 @@ static int goldfish_fb_probe(struct platform_device *pdev) > return 0; > > err_register_framebuffer_failed: > +err_pan_display_failed: > free_irq(fb->irq, fb); [Severity: High] This is a pre-existing issue, but does this error path leave device interrupts enabled? The hardware interrupt is enabled just before calling goldfish_fb_pan_display(): writel(FB_INT_BASE_UPDATE_DONE, fb->reg_base + FB_INT_ENABLE); If goldfish_fb_pan_display() fails, the code jumps to err_pan_display_failed and frees the IRQ handler. However, it never writes 0 to FB_INT_ENABLE to disable the device's interrupt generation. Can this cause an unhandled interrupt storm that permanently disables the shared IRQ line? > err_request_irq_failed: > err_fb_set_var_failed: [Severity: High] This is a pre-existing issue, but does this error path cause a DMA use-after-free by the hardware? The hardware is programmed to scan actively from the DMA address: goldfish_fb_pan_display() { ... writel(fb->fb.fix.smem_start + fb->fb.var.xres * 2 * var->yoffset, fb->reg_base + FB_SET_BASE); } If the probe fails after this point, or when goldfish_fb_remove() is called, the driver frees the DMA memory here in the error path: err_fb_set_var_failed: dma_free_coherent(&pdev->dev, framesize, (void *)fb->fb.screen_base, fb->fb.fix.smem_start); Since the driver never halts the hardware scanout (e.g., by setting the base address to 0 or writing to FB_SET_BLANK), won't the hardware continue to read from the freed memory? [ ... ] [Severity: High] This is a pre-existing issue, but does goldfish_fb_remove() use a mismatched size when freeing DMA memory? In goldfish_fb_probe(), memory is allocated with a fixed size: framesize = width * height * 2 * 2; fb->fb.screen_base = dma_alloc_coherent(..., framesize, ...); However, in goldfish_fb_remove(), the size to free is dynamically recalculated: goldfish_fb_remove() { ... framesize = fb->fb.var.xres_virtual * fb->fb.var.yres_virtual * 2; dma_free_coherent(..., framesize, ...); } Since userspace can modify yres_virtual via the FBIOPUT_VSCREENINFO ioctl, wouldn't remove() attempt to free a different size than was allocated, violating the DMA API? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
