Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] OpenFirmware (OF) EDID parsing is broken because the EDID property blob is never updated. Pre-existing issues: - [High] Unprotected access to `nv_connector->edid` can cause a kernel panic via a NULL pointer dereference. - [High] PM runtime autosuspend reference leak on firmware EDID read failure. - [High] Concurrent execution of asynchronous atomic commit and hotplug detection leads to a Use-After-Free of EDID. - [High] Potential out-of-bounds heap read during ACPI EDID allocation due to untrusted length calculations. -- --- Patch [7]: [PATCH 7/7] nouveau: honor HF-EEODB EDIDs by converting to struct drm_edid --- commit 8141f1386a9c223f5c8dc8688b041f5d4419ab2c Author: Mohamed Ahmed <[email protected]> Subject: nouveau: honor HF-EEODB EDIDs by converting to struct drm_edid This commit converts the nouveau display driver to use the struct drm_edid API to correctly handle HF-EEODB EDIDs. It updates EDID retrieval paths to use drm_edid_read helpers and a custom block reader for GSP boards. The legacy edid pointer is retained as a borrowed view for raw-EDID consumers. > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c > b/drivers/gpu/drm/nouveau/dispnv50/disp.c > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c > @@ -1804,7 +1804,7 @@ nv50_sor_atomic_enable(struct drm_encoder *encoder, > struct drm_atomic_commit *st > lvds_8bpc = bios->fp.if_is_24bit; > } else { > if (nv_connector->type == DCB_CONNECTOR_LVDS_SPWG) { > - if (((u8 *)nv_connector->edid)[121] == 2) > + if (((const u8 *)nv_connector->edid)[121] == 2) [Severity: High] This is a pre-existing issue, but can this cause a NULL pointer dereference if the EDID retrieval failed? If a display mode is forced on via sysfs or a hardcoded VBIOS mode without a valid EDID, nv_connector->edid will be NULL. Unconditionally dereferencing it here to check byte 121 could crash the kernel. [Severity: High] This is another pre-existing issue, but is there a use-after-free risk here during concurrent hotplug events? During an asynchronous atomic commit, nv50_sor_atomic_enable() reads nv_connector->edid locklessly outside the connection_mutex. If a concurrent hotplug worker calls nouveau_connector_set_edid() and frees the EDID memory, this access could corrupt memory. Should atomic drivers read EDID information from the protected drm_connector_state instead of the mutable connector state? > lvds_dual = true; > } else > if (mode->clock >= bios->fp.duallink_transition_clk) { > diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c > b/drivers/gpu/drm/nouveau/nouveau_connector.c > --- a/drivers/gpu/drm/nouveau/nouveau_connector.c > +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c [ ... ] > @@ -490,8 +490,11 @@ nouveau_connector_of_detect(struct drm_connector > *connector) > int idx = name ? name[strlen(name) - 1] - 'A' : 0; > > if (nv_encoder->dcb->i2c_index == idx && edid) { > + drm_edid_free(nv_connector->drm_edid); > + nv_connector->drm_edid = > + drm_edid_alloc(edid, EDID_LENGTH); > + nv_connector->edid = > + drm_edid_raw(nv_connector->drm_edid); > return nv_encoder; [Severity: High] Does this bypass updating the core connector's blob pointer, breaking OF EDID parsing? Because nouveau_connector_of_detect() manually assigns nv_connector->drm_edid instead of calling nouveau_connector_set_edid(), it skips the drm_edid_connector_update() call. This leaves the core connector's edid_blob_ptr as NULL. > } > } [ ... ] > @@ -590,20 +618,36 @@ nouveau_connector_detect(struct drm_connector > *connector, bool force) > > nv_encoder = nouveau_connector_ddc_detect(connector); > if (nv_encoder) { > - struct edid *new_edid = NULL; > + const struct drm_edid *new_edid = NULL; > > if (nv_encoder->i2c) { > if ((vga_switcheroo_handler_flags() & > VGA_SWITCHEROO_CAN_SWITCH_DDC) && > nv_connector->type == DCB_CONNECTOR_LVDS) > - new_edid = drm_get_edid_switcheroo(connector, > nv_encoder->i2c); > + new_edid = drm_edid_read_switcheroo(connector, > nv_encoder->i2c); > else > - new_edid = drm_get_edid(connector, > nv_encoder->i2c); > + new_edid = drm_edid_read_ddc(connector, > nv_encoder->i2c); > } else { > - ret = nvif_outp_edid_get(&nv_encoder->outp, (u8 > **)&new_edid); > + struct nouveau_rm_edid rm = {}; > + > + ret = nvif_outp_edid_get(&nv_encoder->outp, &rm.data); > if (ret < 0) > return connector_status_disconnected; [Severity: High] This isn't a bug introduced by this patch, but does this early return leak a PM runtime reference? At the beginning of nouveau_connector_detect(), a PM runtime reference is acquired. If execution enters this fallback path and nvif_outp_edid_get() returns < 0, the early return bypasses the out label where pm_runtime_put_autosuspend() is called. Repeated failures here could permanently prevent the device from entering low-power states. > + > + /* ret is RM's true buffer size: an HF-EEODB EDID is [ ... ] > @@ -712,7 +756,13 @@ nouveau_connector_detect_lvds(struct drm_connector > *connector, bool force) > * valid - it's not (rh#613284) > */ > if (nv_encoder->dcb->lvdsconf.use_acpi_for_edid) { > - edid = nouveau_acpi_edid(dev, connector); > + struct edid *raw = nouveau_acpi_edid(dev, connector); > + > + if (raw) { > + edid = drm_edid_alloc(raw, > + EDID_LENGTH * (1 + > raw->extensions)); > + kfree(raw); > + } [Severity: High] This is a pre-existing issue, but could this lead to an out-of-bounds heap read if the firmware provides a malformed ACPI table? The allocation size is computed by trusting the raw->extensions byte. If the underlying ACPI buffer is physically smaller than what the extensions byte claims, drm_edid_alloc() will read past the end of the raw buffer when copying the data. > if (edid) { > status = connector_status_connected; > goto out; [ ... ] > @@ -973,8 +1024,8 @@ nouveau_connector_get_modes(struct drm_connector > *connector) > nv_connector->native_mode = NULL; > } > > - if (nv_connector->edid) > - ret = drm_add_edid_modes(connector, nv_connector->edid); > + if (nv_connector->drm_edid) > + ret = drm_edid_connector_add_modes(connector); [Severity: High] Since nouveau_connector_of_detect() left the core connector's edid_blob_ptr NULL, will this fail to parse any display modes? The drm_edid_connector_add_modes() helper relies exclusively on connector->edid_blob_ptr. If it remains NULL, this will silently fail and break display output for OpenFirmware displays. > else > if (nv_encoder->dcb->type == DCB_OUTPUT_LVDS && > (nv_encoder->dcb->lvdsconf.use_straps_for_mode || -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
