Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Low] The patch updates all callers of the local static wrapper `simplefb_get_validated_int0()` to call `drm_sysfb_get_validated_int0()` directly, but leaves the static function definition behind, causing a compiler warning. Pre-existing issues: - [High] Missing validation between the firmware-provided framebuffer dimensions (stride, width, height) and the size of the memory region allows out-of-bounds virtual memory writes. -- commit 934b7fe8edd3f05582affbe138506d555774f080 Author: Thomas Zimmermann <[email protected]> drm/sysfb: simpledrm: Improve framebuffer-size validation Validate the framebuffer size from the firmware against the limitations of struct drm_display_mode. Fail probing on errors if the size exceeds the 16-bit fields. > diff --git a/drivers/gpu/drm/sysfb/simpledrm.c > b/drivers/gpu/drm/sysfb/simpledrm.c > index fc168920f2c64..03bd19fadccd5 100644 > --- a/drivers/gpu/drm/sysfb/simpledrm.c > +++ b/drivers/gpu/drm/sysfb/simpledrm.c > @@ -88,14 +88,14 @@ static int > simplefb_get_width_pd(struct drm_device *dev, > const struct simplefb_platform_data *pd) > { > - return simplefb_get_validated_int0(dev, "width", pd->width); > + return drm_sysfb_get_validated_int0(dev, "width", pd->width, U16_MAX); > } [Severity: Low] Does this leave simplefb_get_validated_int0() as an unused static function? Since all four callers (simplefb_get_width_pd, simplefb_get_height_pd, simplefb_get_width_of, simplefb_get_height_of) are updated to call drm_sysfb_get_validated_int0() directly, simplefb_get_validated_int0() appears to have zero callers remaining in simpledrm.c. This can trigger a -Wunused-function compiler warning. [Severity: High] This isn't a bug introduced by this patch, but while we are improving the validation of framebuffer dimensions from firmware, should we also validate that the resulting framebuffer size actually fits within the mapped memory region? The firmware-provided stride and height are used to calculate the framebuffer size, but when acquiring the memory region: drivers/gpu/drm/sysfb/simpledrm.c:simpledrm_device_create() { ... screen_base = devm_memremap(dev->dev, mem->start, resource_size(mem), MEMREMAP_WC); ... } or the fallback: drivers/gpu/drm/sysfb/simpledrm.c:simpledrm_device_create() { ... screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem)); ... } It seems we map exactly resource_size(mem) bytes without verifying if (stride * height) exceeds that size. If the firmware provides a mismatched configuration, out-of-bounds writes could occur during plane updates when copying pixel data into sysfb->fb_addr. [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
