Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [High] The driver accepts any non-zero `linebytes` from the Device Tree without ensuring it is at least as large as `width * bytes_per_pixel`. - [High] The 64-bit result of `drm_format_info_min_pitch()` is silently truncated when assigned to the 32-bit `int linebytes` variable. - [High] The `round_up(fb_size, PAGE_SIZE)` calculation can overflow to 0 on 32-bit systems, leading to an undersized memory mapping. - [High] On 32-bit architectures with 64-bit physical addresses (PAE/LPAE), passing a 64-bit size to `ioremap` via `devm_ioremap` truncates it to a 32-bit `size_t`. -- --- Patch [1]: [PATCH] drm/sysfb: ofdrm: Fix integer overflow in fb_size calculation --- commit ad44f8947b62f451f5098acf1a574e6b16699aa6 Author: Shixiong Ou <[email protected]> drm/sysfb: ofdrm: Fix integer overflow in fb_size calculation The framebuffer size calculation fb_size = linebytes * height can overflow when both values are large (e.g., 46341 * 46341 > INT_MAX). Since linebytes and height are both int types, the multiplication is performed as int * int, which results in undefined behavior on overflow. Use check_mul_overflow() to detect and prevent this overflow, consistent with the approach used in simpledrm.c and corebootdrm.c. > diff --git a/drivers/gpu/drm/sysfb/ofdrm.c b/drivers/gpu/drm/sysfb/ofdrm.c > index 819aed466727e..a6dc34b9ec0f3 100644 > --- a/drivers/gpu/drm/sysfb/ofdrm.c > +++ b/drivers/gpu/drm/sysfb/ofdrm.c > @@ -913,7 +914,10 @@ static struct ofdrm_device *ofdrm_device_create(struct > drm_driver *drv, > return ERR_PTR(-EINVAL); > } > > - fb_size = linebytes * height; > + if (check_mul_overflow(linebytes, height, &fb_size)) { [Severity: High] This isn't a bug introduced by this patch, but does drm_format_info_min_pitch() called earlier in this function return a 64-bit value that is silently truncated when assigned to the 32-bit int linebytes variable? if (!linebytes) { linebytes = drm_format_info_min_pitch(format, 0, width); if (drm_WARN_ON(dev, !linebytes)) return ERR_PTR(-EINVAL); } If width is maliciously large (e.g., 0x40000001), the 64-bit result could be truncated to 4, bypassing the !linebytes check and the new check_mul_overflow() check. [Severity: High] This is a pre-existing issue, but is there a missing lower bound check for linebytes in the same block above? If the Device Tree provides a maliciously small but non-zero linebytes (e.g., 4), it bypasses the !linebytes check. It would also pass the new check_mul_overflow() check but could cause out-of-bounds memory writes downstream when DRM plane updates write data based on the logical width. [Severity: High] This is a pre-existing issue, but on 32-bit systems where resource_size_t is 32-bit, could round_up(fb_size, PAGE_SIZE) computed later in this function overflow and wrap around to 0? fb_pgbase = round_down(fb_base, PAGE_SIZE); fb_pgsize = fb_base - fb_pgbase + round_up(fb_size, PAGE_SIZE); If fb_size is near U32_MAX, the overflow check succeeds, but this calculation would result in an undersized memory mapping. [Severity: High] This is a pre-existing issue, but on 32-bit architectures with 64-bit physical addresses (PAE/LPAE), does devm_ioremap() silently truncate the 64-bit size to a 32-bit size_t? screen_base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem)); Since check_mul_overflow() checks against the 64-bit resource_size_t fb_size, a massive size wouldn't be caught there, and would be truncated here, leading to a smaller mapping than the DRM core expects. > + drm_err(dev, "framebuffer size exceeds maximum\n"); > + return ERR_PTR(-EINVAL); > + } > > /* > * Try to figure out the address of the framebuffer. Unfortunately, Open -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
