Re: [PATCH 7/7] nouveau: honor HF-EEODB EDIDs by converting to struct drm_edid

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