Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Unconditional recreation of DRM TV properties breaks atomic uAPI property updates and causes memory leaks of `drm_property` objects. Pre-existing issues: - [High] Out-of-bounds read in TV mode name processing via `strlen()` due to missing null-termination guarantees. -- --- Patch [1]: [PATCH] drm: Optimized by adding the TV modes attribute --- commit 5de13986f93b84f8b4c689b55a18d298dfc7431d Author: Edward Adam Davis <[email protected]> drm: Optimized by adding the TV modes attribute When adding gud properties for drm connector within the function gud_connector_add_properties(), if the TV modes property is not added first, drm_mode_create_tv_properties_legacy() would fail to add the TV modes property because the tv_select_subconnector_property has already been added. Closes: https://syzkaller.appspot.com/bug?extid=1944765c3659f63d3777 > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 8b4baed060f3a..7728c2941a701 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -2201,8 +2201,12 @@ int drm_mode_create_tv_properties_legacy(struct > drm_device *dev, > struct drm_property *tv_subconnector; > unsigned int i; > > - if (dev->mode_config.tv_select_subconnector_property) > + if (dev->mode_config.tv_select_subconnector_property) { > + if (num_modes && !dev->mode_config.legacy_tv_mode_property) > + goto other; > + > return 0; > + } > > /* > * Basic connector properties [ ... ] > @@ -2225,6 +2229,7 @@ int drm_mode_create_tv_properties_legacy(struct > drm_device *dev, > goto nomem; > dev->mode_config.tv_subconnector_property = tv_subconnector; > > +other: > /* > * Other, TV specific properties: margins & TV modes. > */ [Severity: High] Does this goto path unconditionally recreate properties like tv_brightness_property? If a GUD USB device exposes multiple TV properties, the driver calls drm_mode_create_tv_properties_legacy() multiple times. For subsequent properties, the goto other path is hit, but it proceeds to unconditionally call drm_property_create_range() for brightness, contrast, etc. Would this overwrite the pointers in dev->mode_config and leak the orphaned drm_property objects? Furthermore, during a userspace atomic commit to update the brightness property on the first connector, wouldn't the pointer mismatch cause the kernel to reject the valid property update with -EINVAL? drivers/gpu/drm/drm_atomic_uapi.c:drm_atomic_connector_set_property() { ... } else if (property == config->tv_brightness_property) { state->tv.brightness = val; ... } [Severity: High] This isn't a bug introduced by this patch, but does the TV mode name processing in gud_connector_add_tv_mode() have an out-of-bounds read? Untrusted data is read from the USB device into buf without enforcing null-termination: drivers/gpu/drm/gud/gud_connector.c:gud_connector_add_tv_mode() { ... ret = gud_usb_get(gdrm, GUD_REQ_GET_CONNECTOR_TV_MODE_VALUES, connector->index, buf, buf_len); ... for (i = 0; i < num_modes; i++) modes[i] = &buf[i * GUD_CONNECTOR_TV_MODE_NAME_LEN]; ... } If the USB device payload contains full 16-byte chunks without any null terminators, would the strlen() in drm_property_add_enum() read past the end of the kmalloced buffer? drivers/gpu/drm/drm_property.c:drm_property_add_enum() { ... if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN)) return -EINVAL; ... } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
