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