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 <[email protected]> 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/[email protected]?part=1
