Re: [PATCH v6 1/5] drm/ras: Cancel and free message on get counter failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Concurrent device teardown causes Use-After-Free and potential Control-Flow Hijack due to unprotected `drm_ras_node` access via XArray.
- [High] Uninitialized variable `ret` is returned if the node iteration loops do not execute, corrupting netlink dump responses.
--

--- Patch [1]: [PATCH v6 1/5] drm/ras: Cancel and free message on get counter failure ---
commit 967dffc71497d8878a77ecf0c72775277b0ce183
Author: Raag Jadav <raag.jadav@intel.com>

drm/ras: Cancel and free message on get counter failure
    
doit_reply_value() directly returns on get counter failure, which results
in stale sk_buff and genetlink header that aren't cleaned up. Fix it and
while at it, consolidate error handling using goto.

> diff --git a/drivers/gpu/drm/drm_ras.c b/drivers/gpu/drm/drm_ras.c
> index 39155fb514de3..4fa1a257b0ed1 100644
> --- a/drivers/gpu/drm/drm_ras.c
> +++ b/drivers/gpu/drm/drm_ras.c
> @@ -234,25 +234,28 @@ static int doit_reply_value(struct genl_info *info, u32 node_id,
>  
>  	hdr = genlmsg_iput(msg, info);
>  	if (!hdr) {
> -		nlmsg_free(msg);
> -		return -EMSGSIZE;
> +		ret = -EMSGSIZE;
> +		goto free_msg;
>  	}
>  
>  	ret = get_node_error_counter(node_id, error_id,
>  				     &error_name, &value);
>  	if (ret)
> -		return ret;
> +		goto cancel_msg;
>  
>  	ret = msg_reply_value(msg, error_id, error_name, value);
> -	if (ret) {
> -		genlmsg_cancel(msg, hdr);
> -		nlmsg_free(msg);
> -		return ret;
> -	}
> +	if (ret)
> +		goto cancel_msg;
>  
>  	genlmsg_end(msg, hdr);
>  
>  	return genlmsg_reply(msg, info);
> +
> +cancel_msg:
> +	genlmsg_cancel(msg, hdr);
> +free_msg:
> +	nlmsg_free(msg);
> +	return ret;
>  }

[Severity: Critical]
This is a pre-existing issue, but does get_node_error_counter() have a
use-after-free risk during concurrent device teardown?

Since drm_ras_nl_family is configured with parallel_ops = true, these handlers
run concurrently without genl_mutex. If the device is concurrently unregistered
(e.g., driver unbind or hot-unplug), drm_ras_node_unregister() removes the node
from the xarray and the driver frees its memory while the netlink handler is
still executing.

This can happen in get_node_error_counter():

drivers/gpu/drm/drm_ras.c:get_node_error_counter() {
	...
	node = xa_load(&drm_ras_xa, node_id);
	if (!node || !node->query_error_counter)
		return -ENOENT;

	if (error_id < node->error_counter_range.first ||
	...
}

Here xa_load() retrieves the node pointer locklessly, but it is dereferenced
without holding an RCU read lock or taking a reference count. Could this result
in a use-after-free and potential control-flow hijack when executing
node->query_error_counter()?

This same pattern appears in drm_ras_nl_list_nodes_dumpit():

drivers/gpu/drm/drm_ras.c:drm_ras_nl_list_nodes_dumpit() {
	...
	xa_for_each_start(&drm_ras_xa, id, node, ctx->restart) {
		hdr = genlmsg_iput(skb, info);
	...
}

And in drm_ras_nl_clear_error_counter_doit():

drivers/gpu/drm/drm_ras.c:drm_ras_nl_clear_error_counter_doit() {
	...
	node = xa_load(&drm_ras_xa, node_id);
	if (!node || !node->clear_error_counter)
		return -ENOENT;
	...
}

[Severity: High]
This is also a pre-existing issue, but can drm_ras_nl_list_nodes_dumpit()
return an uninitialized value?

drivers/gpu/drm/drm_ras.c:drm_ras_nl_list_nodes_dumpit() {
	...
	int ret;

	xa_for_each_start(&drm_ras_xa, id, node, ctx->restart) {
		...
		ret = nla_put_u32(skb, DRM_RAS_A_NODE_ATTRS_NODE_ID, node->id);
		...
	}

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

	return ret;
}

If the xarray is empty, or on the final recvmsg() call where ctx->restart is
past the end of the items (signifying EOF), the iteration loop will not execute.
This leaves ret uninitialized. The function then checks if (ret == -EMSGSIZE),
which reads uninitialized stack memory, and then returns ret. This could cause
the dump to fail sporadically with arbitrary error codes.

A similar issue appears in drm_ras_nl_get_error_counter_dumpit():

drivers/gpu/drm/drm_ras.c:drm_ras_nl_get_error_counter_dumpit() {
	...
	int ret;

	if (!info->attrs || GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_ERROR_COUNTER_ATTRS_NODE_ID))
		return -EINVAL;
	...
	for (error_id = max(node->error_counter_range.first, ctx->restart);
	     error_id <= node->error_counter_range.last;
	     error_id++) {
		ret = get_node_error_counter(...);
		...
	}

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

	return ret;
}

If the requested error range iteration does not execute, ret remains
uninitialized here as well.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818135304.497098-1-raag.jadav@intel.com?part=1