Re: [PATCH v2 2/4] drm/panel: Add driver for Novatek NT37705 panel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] drm/panel: resource leak on DSI command failure in prepare
- [High] drm/panel: Use-After-Free of dangling dsi->dsc pointer
- [Medium] drm/panel: unpowered device access in backlight update
- [Medium] drm/panel: state corruption of mode_flags on error path
- [Medium] drm/panel: protocol violation via malformed DCS commands
--

commit 8f6434d0603fb1133d406bd15a56e3de3bcce821
Author: Luca Weiss <luca.weiss@fairphone.com>

drm/panel: Add driver for Novatek NT37705 panel
    
Add support for the 2484x1116 AMOLED panel from BOE (BJ631JHM-T71-D900)
bundled with a NT37705 driver IC, as found on the Fairphone (Gen. 6)
smartphone.

> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt37705.c b/drivers/gpu/drm/panel/panel-novatek-nt37705.c
> index 0000000000000..27bd8072ccd1b
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt37705.c
> @@ -0,0 +1,413 @@
[ ... ]
> +static int nt37705_on(struct nt37705_panel *ctx)
> +{
[ ... ]
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x11, 0x00);
> +	mipi_dsi_msleep(&dsi_ctx, 120);
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x29, 0x00);

[Severity: Medium]
Are these standard zero-parameter DCS commands (Exit Sleep Mode 0x11,
Display On 0x29) intended to be transmitted with an extra 0x00 padding
byte? 

This creates a 1-parameter short write instead of a 0-parameter short
write, which violates the MIPI DCS specification and could cause panel
controller misbehavior.

> +	mipi_dsi_msleep(&dsi_ctx, 22);
> +
> +	return dsi_ctx.accum_err;
> +}
> +
> +static int nt37705_off(struct nt37705_panel *ctx)
> +{
> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> +
> +	ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x28, 0x00);
> +	mipi_dsi_msleep(&dsi_ctx, 20);
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x10, 0x00);

[Severity: Medium]
Similarly, do these zero-parameter DCS commands (Display Off 0x28, Enter
Sleep Mode 0x10) need the extra 0x00 padding byte?

> +	mipi_dsi_msleep(&dsi_ctx, 120);
> +
> +	return dsi_ctx.accum_err;
> +}
> +
> +static int nt37705_prepare(struct drm_panel *panel)
> +{
[ ... ]
> +	ret = mipi_dsi_picture_parameter_set(ctx->dsi, &pps);
> +	if (ret < 0) {
> +		dev_err(panel->dev, "failed to transmit PPS: %d\n", ret);
> +		return ret;

[Severity: High]
Does this early return leak resources? 

If mipi_dsi_picture_parameter_set() fails, the function returns directly
instead of jumping to a cleanup block to disable the regulators and assert
the reset GPIO, preventing subsequent recovery or clean shutdown of the
panel.

> +	}
> +
> +	ret = mipi_dsi_compression_mode(ctx->dsi, true);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable compression mode: %d\n", ret);
> +		return ret;

[Severity: High]
Similarly, does this early return also bypass resource cleanup?

> +	}
> +
> +	msleep(28); /* TODO: Is this panel-dependent? */
> +
> +	return 0;
> +}
[ ... ]
> +static int nt37705_bl_update_status(struct backlight_device *bl)
> +{
> +	struct mipi_dsi_device *dsi = bl_get_data(bl);
> +	u16 brightness = backlight_get_brightness(bl);
> +	int ret;
> +
> +	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +
> +	ret = mipi_dsi_dcs_set_display_brightness_large(dsi, brightness);

[Severity: Medium]
What happens if userspace modifies the sysfs brightness attribute when the
display is suspended or turned off? 

This function appears to unconditionally initiate DSI transactions without
checking the panel's power state, which could lead to DSI bus timeouts and
potential system sluggishness depending on the DSI host implementation.

> +	if (ret < 0)
> +		return ret;

[Severity: Medium]
If mipi_dsi_dcs_set_display_brightness_large() fails, we take this early
return path. Does this permanently disable the MIPI_DSI_MODE_LPM flag for
the DSI device? 

If so, subsequent commands might be transmitted in the wrong mode,
potentially breaking display functionality.

> +
> +	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> +	return 0;
> +}
[ ... ]
> +static int nt37705_probe(struct mipi_dsi_device *dsi)
> +{
[ ... ]
> +	/* This panel only supports DSC; unconditionally enable it */
> +	dsi->dsc = &ctx->dsc;

[Severity: High]
Will this lead to a Use-After-Free on driver unbind? 

Here we assign a pointer into the devres-managed ctx allocation to the
longer-lived mipi_dsi_device struct.

[ ... ]
> +static void nt37705_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct nt37705_panel *ctx = mipi_dsi_get_drvdata(dsi);
> +	int ret;
> +
> +	ret = mipi_dsi_detach(dsi);
> +	if (ret < 0)
> +		dev_err(&dsi->dev, "Failed to detach from DSI host: %d\n", ret);
> +
> +	drm_panel_remove(&ctx->panel);
> +}

[Severity: High]
When nt37705_remove() completes, ctx is freed by devres, leaving dsi->dsc
as a dangling pointer. 

If the DSI host accesses dsi->dsc afterwards, this could cause a UAF.
Should dsi->dsc be set to NULL here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918-fp6-panel-v2-0-6695c4008920@fairphone.com?part=2