linux-kernel - Re: [PATCH v5 06/11] drm/display: add CEC helpers code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250414-determined-kind-peacock-e9a47c@houat>
Date: Mon, 14 Apr 2025 16:58:48 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, 
	Simona Vetter <simona@...ll.ch>, Dave Stevenson <dave.stevenson@...pberrypi.com>, 
	Maíra Canal <mcanal@...lia.com>, Raspberry Pi Kernel Maintenance <kernel-list@...pberrypi.com>, 
	Andrzej Hajda <andrzej.hajda@...el.com>, Neil Armstrong <neil.armstrong@...aro.org>, 
	Robert Foss <rfoss@...nel.org>, Laurent Pinchart <Laurent.pinchart@...asonboard.com>, 
	Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	Dmitry Baryshkov <lumag@...nel.org>
Subject: Re: [PATCH v5 06/11] drm/display: add CEC helpers code

On Mon, Apr 07, 2025 at 06:11:03PM +0300, Dmitry Baryshkov wrote:
> +static void drm_connector_hdmi_cec_adapter_unregister(struct drm_connector *connector)
> +{
> +	struct drm_connector_hdmi_cec_data *data = connector->cec.data;
> +
> +	cec_delete_adapter(data->adapter);
> +
> +	if (data->funcs->uninit)
> +		data->funcs->uninit(connector);
> +
> +	kfree(data);
> +	connector->cec.data = NULL;
> +}
>
> [...]
>
> +int drm_connector_hdmi_cec_register(struct drm_connector *connector,
> +				    const struct drm_connector_hdmi_cec_funcs *funcs,
> +				    const char *name,
> +				    u8 available_las,
> +				    struct device *dev)
> +{
> +	struct drm_connector_hdmi_cec_data *data;
> +	struct cec_connector_info conn_info;
> +	struct cec_adapter *cec_adap;
> +	int ret;
> +
> +	if (!funcs->init || !funcs->enable || !funcs->log_addr || !funcs->transmit)
> +		return -EINVAL;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->funcs = funcs;
> +
> +	cec_adap = cec_allocate_adapter(&drm_connector_hdmi_cec_adap_ops, connector, name,
> +					CEC_CAP_DEFAULTS | CEC_CAP_CONNECTOR_INFO,
> +					available_las ? : CEC_MAX_LOG_ADDRS);
> +	ret = PTR_ERR_OR_ZERO(cec_adap);
> +	if (ret < 0)
> +		goto err_free;
> +
> +	cec_fill_conn_info_from_drm(&conn_info, connector);
> +	cec_s_conn_info(cec_adap, &conn_info);
> +
> +	data->adapter = cec_adap;
> +
> +	mutex_lock(&connector->cec.mutex);
> +
> +	connector->cec.data = data;
> +	connector->cec.funcs = &drm_connector_hdmi_cec_adapter_funcs;
> +
> +	ret = funcs->init(connector);
> +	if (ret < 0)
> +		goto err_delete_adapter;
> +
> +	ret = cec_register_adapter(cec_adap, dev);
> +	if (ret < 0)
> +		goto err_delete_adapter;

I'm a bit concerned about the respective lifetimes of CEC adapters and
DRM connectors.

When you register the CEC adapter, its associated structure is
kzalloc'd, and freed when the DRM connector is freed (so when nobody has
any reference to it anymore: either when the device is torn down, or a
DP-MST hotplug scenario).

The CEC adapter however will only be freed when its own users will close
their file descriptor. So we can have a scenario when the CEC adapter is
still live but the DRM connector has been unregistered. Thus, the CEC
adapter data will have been kfree'd.

You might consider safe because $REASONS, but those need to be properly
detailed and explained.

That's another reason why I think that just putting the connector
pointer as data is better: connectors are refcounted, so we know those
aren't an issue.

Maxime>

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)