Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/gud: unsigned num_formats in gud_probe() misses errors and causes OOB read - [Medium] drm/gud: gud_get_max_buffer_size() sizes buffer using emulated XRGB8888 --
commit 19e6d0cc37d9a0a4a7292d2a1f0cf5690fddd459 Author: Ruben Wauters <[email protected]> drm/gud: move format init from gud_probe() This commit refactors pixel format initialization out of gud_probe() into dedicated helper functions gud_get_formats() and gud_get_max_buffer_size(). > diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c > index 680447f426f37..278cc9bf68d02 100644 > --- a/drivers/gpu/drm/gud/gud_drv.c > +++ b/drivers/gpu/drm/gud/gud_drv.c > @@ -435,11 +435,123 @@ static void gud_free_buffers_and_mutex(void *data) > mutex_destroy(&gdrm->ctrl_lock); > } > > -static int gud_probe(struct usb_interface *intf, const struct usb_device_id > *id) > +static size_t gud_get_max_buffer_size(struct gud_device *gdrm, u32 *formats, > u32 num_formats) > +{ > + struct drm_device *drm = &gdrm->drm; > + size_t max_buffer_size = 0; > + size_t fmt_buf_size; > + int i; > + > + for (i = 0; i < num_formats; i++) { > + const struct drm_format_info *info = > drm_format_info(formats[i]); > + > + fmt_buf_size = drm_format_info_min_pitch(info, 0, > drm->mode_config.max_width) * > + > drm->mode_config.max_height; > + max_buffer_size = max(max_buffer_size, fmt_buf_size); > + } > + > + /* GUD_DRM_FORMAT_R1 and GUD_DRM_FORMAT_XRGB1111 are not in the formats > array. */ > + /* Ensure there is enough space for them. */ > + fmt_buf_size = drm_format_info_min_pitch(&gud_drm_format_r1, 0, > + drm->mode_config.max_width) * > + drm->mode_config.max_height; > + max_buffer_size = max(max_buffer_size, fmt_buf_size); > + > + fmt_buf_size = drm_format_info_min_pitch(&gud_drm_format_xrgb1111, 0, > + drm->mode_config.max_width) * > + drm->mode_config.max_height; > + max_buffer_size = max(max_buffer_size, fmt_buf_size); > + > + return max_buffer_size; > +} [Severity: Medium] Does gud_get_max_buffer_size() end up computing max_buffer_size for a 32-bpp format regardless of the formats supported by the device? Previously in gud_probe(), max_buffer_size was calculated inside the loop over formats_dev before the emulated DRM_FORMAT_XRGB8888 was appended to formats[], because XRGB8888 framebuffers are converted down to gdrm->xrgb8888_emulation_format in bulk_buf. With this change, gud_get_formats() appends DRM_FORMAT_XRGB8888 to formats[] before gud_get_max_buffer_size() is called: if (!xrgb8888_supported && xrgb8888_emulation_format) { gdrm->xrgb8888_emulation_format = xrgb8888_emulation_format; formats[num_formats++] = DRM_FORMAT_XRGB8888; } Since every valid format returned by gud_to_fourcc() is either already 32-bpp (DRM_FORMAT_XRGB8888, DRM_FORMAT_ARGB8888) or sets xrgb8888_emulation_format, formats[] always contains a 32-bpp format when passed to gud_get_max_buffer_size(). As a result, when desc.max_buffer_size is not set, gdrm->bulk_buf and gdrm->compress_buf are sized for 4 bytes per pixel even on devices that only support smaller wire formats (such as R1, XRGB1111, R8, RGB332, or RGB565), and the subsequent max() checks for gud_drm_format_r1 and gud_drm_format_xrgb1111 become no-ops. [ ... ] > +static int gud_probe(struct usb_interface *intf, const struct usb_device_id > *id) > +{ > + unsigned int num_formats = 0; [ ... ] > @@ -510,81 +621,16 @@ static int gud_probe(struct usb_interface *intf, const > struct usb_device_id *id) > drm->mode_config.helper_private = &gud_mode_config_helpers; > > /* Format init */ > - formats_dev = devm_kmalloc(dev, GUD_FORMATS_MAX_NUM, GFP_KERNEL); > /* Add room for emulated XRGB8888 */ > formats = devm_kmalloc_array(dev, GUD_FORMATS_MAX_NUM + 1, > sizeof(*formats), GFP_KERNEL); > - if (!formats_dev || !formats) > + if (!formats) > return -ENOMEM; > > - ret = gud_usb_get(gdrm, GUD_REQ_GET_FORMATS, 0, formats_dev, > GUD_FORMATS_MAX_NUM); [ ... ] > + num_formats = gud_get_formats(gdrm, formats); > + if (num_formats <= 0) > + return num_formats; > > + max_buffer_size = gud_get_max_buffer_size(gdrm, formats, num_formats); [Severity: High] Since num_formats is declared as unsigned int in gud_probe(), won't negative error codes returned by gud_get_formats() (such as -ENOMEM, -EINVAL, or an error from gud_usb_get()) wrap to a large positive value? When gud_get_formats() fails, the check if (num_formats <= 0) evaluates to false (and triggers a -Wtype-limits warning), causing gud_probe() to pass the wrapped unsigned value into gud_get_max_buffer_size() and read past the end of the formats[] array in the loop over num_formats. Should num_formats be a signed int (or assigned via ret first)? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
