Re: [PATCH RFC v4 03/13] drm: client: add splash client

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/client: splash: UAF of drm_framebuffer on hotplug error path
- [Medium] drm/client: splash: memory leak of scanout array on hotplug -ENODEV
- [Medium] drm/client: splash: firmware blob leak if render thread exits with -EBUSY
--

commit 86a0b0be0de60a4b9d33ab49ce7a129fe48448a6
Author: Francesco Valla <francesco@valla.it>
Date:   Thu Oct 1 19:58:37 2026 +0000

    drm: client: add splash client
    
    Add a DRM client that draws a simple splash, with possibility to show:
    
      - the logo provided by EFI BGRT;
      - a static BMP image (loaded as firmware);
      - a colored background.

> diff --git a/drivers/gpu/drm/clients/drm_splash.c b/drivers/gpu/drm/clients/drm_splash.c
> new file mode 100644
> index 0000000000000..66c192139d9c4
> --- /dev/null
> +++ b/drivers/gpu/drm/clients/drm_splash.c
> @@ -0,0 +1,1028 @@

[ ... ]

> +static void drm_splash_fw_callback(const struct firmware *fw, void *context)
> +{
> +	struct drm_splash *splash = context;
> +	struct drm_client_dev *client = &splash->client;
> +
> +	if (!fw || !fw->data) {
> +		drm_err(client->dev, "splash: no firmware");
> +		release_firmware(fw);
> +		fw = NULL;
> +	}
> +
> +	scoped_guard(spinlock, &splash->fw_lock)
> +		splash->fw = fw;

[ ... ]

> +static int drm_splash_render_thread(void *data)
> +{
> +	struct drm_splash *splash = data;
> +	struct drm_client_dev *client = &splash->client;
> +
> +	while (!kthread_should_stop()) {

[ ... ]

> +		if (draw_count > 0) {
> +			ret = drm_client_modeset_commit(client);
> +			/* If commit returns EBUSY, another master showed up.
> +			 * This means that the splash is no more required.
> +			 */
> +			if (ret == -EBUSY) {
> +				drm_info(client->dev, "splash: not master anymore, exiting");
> +				break;
> +			}

[Severity: Medium]
Can the firmware blob be leaked if the render thread exits early here?

If drm_client_modeset_commit() returns -EBUSY, the thread breaks out of the
loop and exits. If the firmware request later completes,
drm_splash_fw_callback() stores the firmware in splash->fw. Since the thread
is already dead, won't this firmware remain unreleased until the DRM device is
completely unregistered?

> +		}

[ ... ]

> +static int drm_splash_init_client(struct drm_splash *splash)
> +{

[ ... ]

> +	/* And point every modeset at the buffer of its scanout */
> +	i = 0;
> +	drm_client_for_each_modeset(modeset, client) {
> +		unsigned int idx = i++;
> +
> +		if (idx >= 32)
> +			break;
> +
> +		for (k = 0; k < splash->n_scanout; k++) {
> +			struct drm_splash_scanout *scanout = &splash->scanout[k];
> +
> +			if (scanout->buffer && (scanout->modesets & BIT(idx))) {
> +				modeset->fb = scanout->buffer->fb;
> +				break;
> +			}
> +		}
> +	}
> +
> +	mutex_unlock(&client->modeset_mutex);
> +
> +	return (fb_count == 0) ? -ENODEV : 0;
> +}

[ ... ]

> +static int drm_splash_client_hotplug(struct drm_client_dev *client)
> +{
> +	struct drm_splash *splash = client_to_drm_splash(client);
> +	struct task_struct *thread;
> +	int ret = 0;
> +
> +	guard(mutex)(&splash->hotplug_lock);
> +
> +	/* The modesets that get a splash are defined at first hotplug event */
> +	if (splash->initialized)
> +		return 0;
> +
> +	ret = drm_splash_init_client(splash);
> +	if (ret == -ENODEV) {
> +		drm_info(client->dev, "splash: no modeset found");
> +		return 0;
> +	} else if (ret) {

[Severity: Medium]
Could this error path leak the splash->scanout array?

If drm_splash_init_client() successfully extends the array using krealloc()
but fails to create any buffers, it returns -ENODEV. This early return handles
the -ENODEV case but does not free the array.

Could subsequent hotplug events keep appending to it and leaking memory on
every failure?

> +		drm_err(client->dev,
> +			"splash: failed to init client: %d", ret);
> +		return ret;
> +	}
> +
> +	/* Create the render thread, waken later */
> +	thread = kthread_create(drm_splash_render_thread, splash,
> +				"drm_splash_%s", client->dev->unique);
> +	if (IS_ERR(thread)) {
> +		ret = PTR_ERR(thread);
> +		drm_err(client->dev,
> +			"splash: failed to create render thread: %d", ret);
> +		drm_splash_free_scanout(client);
> +		return ret;
> +	}

[Severity: High]
Does this error path leave dangling pointers in the client's modeset array?

In drm_splash_init_client(), modeset->fb is assigned the framebuffer of the
newly created buffer. If we fail here and call drm_splash_free_scanout(), the
buffers and their framebuffers are freed, but modeset->fb is not cleared.

Since the client remains registered on dev->clientlist even after a hotplug
error, could a subsequent modeset commit or sysrq trigger a use-after-free
by dereferencing the freed modeset->fb?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001195847.141192-1-maximpedraza@gmail.com?part=3