Re: [PATCH 1/3] drm/drm_ras: Remove admin-perm flag from list-nodes operation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] core: UAF in drm_ras_xa lookups during Netlink operations
- [High] core: uninitialized variable leak in Netlink dumpit handlers
- [Low] netlink: missing NLM_F_DUMP_INTR flag for lockless dump iteration
--

commit 46092107989be3447885b1e58b5d130104004fba
Author: Zack McKevitt <zachary.mckevitt@oss.qualcomm.com>

drm/drm_ras: Remove admin-perm flag from list-nodes operation

Drivers may want to avoid requiring CAP_NET_ADMIN for subsequent
drm-ras operations. So, drop the CAP_NET_ADMIN requirement for dumping
RAS nodes so that drivers with different permission requirements are
still able to use the interface.

> diff --git a/drivers/gpu/drm/drm_ras_nl.c b/drivers/gpu/drm/drm_ras_nl.c
> index c9f5e0ceb3b5f..ff0dc406c27c8 100644
> --- a/drivers/gpu/drm/drm_ras_nl.c
> +++ b/drivers/gpu/drm/drm_ras_nl.c
> @@ -46,7 +46,7 @@ static const struct genl_split_ops drm_ras_nl_ops[] = {
>  	{
>  		.cmd	= DRM_RAS_CMD_LIST_NODES,
>  		.dumpit	= drm_ras_nl_list_nodes_dumpit,
> -		.flags	= GENL_ADMIN_PERM | GENL_CMD_CAP_DUMP,
> +		.flags	= GENL_CMD_CAP_DUMP,

[Severity: Critical]
Does dropping the admin permission here safely handle the pointer management
in drm_ras_nl_list_nodes_dumpit() and get_node_error_counter()?

In drm_ras_nl_list_nodes_dumpit(), xa_for_each_start() iterates over the
xarray:

	xa_for_each_start(&drm_ras_xa, id, node, ctx->restart) {
		hdr = genlmsg_iput(skb, info);
		...
		ret = nla_put_string(skb, DRM_RAS_A_NODE_ATTRS_DEVICE_NAME,
				     node->device_name);

The xa_for_each_start() macro drops the RCU read lock before returning,
leaving the node pointer unprotected against concurrent freeing.

Could an unprivileged user concurrently querying the list while nodes are
unregistered trigger a use-after-free when node->device_name is read,
leading to memory corruption or data leaks?

A similar unprotected access occurs in get_node_error_counter():

	node = xa_load(&drm_ras_xa, node_id);
	if (!node || !node->query_error_counter)
		return -ENOENT;
	...
	return node->query_error_counter(node, error_id, name, value);

Does xa_load() protect the returned node pointer from being freed before the
query_error_counter() function pointer is dereferenced?

[Severity: High]
Can exposing drm_ras_nl_list_nodes_dumpit() to unprivileged users leak kernel
stack memory?

In drm_ras_nl_list_nodes_dumpit(), the ret variable is uninitialized:

	int ret;

	xa_for_each_start(&drm_ras_xa, id, node, ctx->restart) {
		...

If the drm_ras_xa xarray is empty, the loop will not execute, and the
function will return the uninitialized stack value. The same issue exists
in drm_ras_nl_get_error_counter_dumpit().

Additionally, if the loop does execute and the socket buffer is full, ret is
set to -EMSGSIZE and returned:

	if (ret == -EMSGSIZE)
		ctx->restart = id;

	return ret;

Does returning -EMSGSIZE break multi-part netlink dumps entirely by failing
to return skb->len when the buffer fills up?

[Severity: Low]
Is the NLM_F_DUMP_INTR flag required for this lockless dump iteration?

drm_ras_nl_list_nodes_dumpit() iterates over the lockless drm_ras_xa data
structure. If concurrent node registrations or unregistrations occur, the
iteration could skip or repeat items.

According to the Netlink subsystem rules, should such dumps maintain a
generation counter and set NLM_F_DUMP_INTR on the affected messages to
notify userspace of inconsistent states?

>  	},
>  	{
>  		.cmd		= DRM_RAS_CMD_GET_ERROR_COUNTER,

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009173152.2436195-1-zachary.mckevitt@oss.qualcomm.com?part=1