Re: [PATCH v3 2/3] drm/drm_ras: Extend DRM RAS interface to include telemetry nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/drm_ras: unsynchronized xa_load() node lookup in telemetry
  handlers
- [Medium] drm/drm_ras: broken status handling and non-standard UAPI in
  telemetry ops
--

commit 136edf60d4fd4d79aaeb6f30bb50fe8f150aa0fe
Author: Zack McKevitt <zachary.mckevitt@oss.qualcomm.com>

drm/drm_ras: Extend DRM RAS interface to include telemetry nodes

This commit extends the DRM RAS Generic Netlink specification and driver
interface with a telemetry node type and operations to list, read, and
write device telemetry fields.

> diff --git a/Documentation/netlink/specs/drm_ras.yaml b/Documentation/netlink/specs/drm_ras.yaml
> index a88fdab7a1c74..cf015bba020d0 100644
> --- a/Documentation/netlink/specs/drm_ras.yaml
> +++ b/Documentation/netlink/specs/drm_ras.yaml
[ ... ]
> @@ -100,6 +100,34 @@ attribute-sets:
>          name: error-value
>          type: u32
>          doc: Current value of the error counter.
> +  -
> +    name: telemetry-attrs
> +    attributes:
> +      -
> +        name: node-id
> +        type: u32
> +        doc: >-
> +             Unique identifier for the node.
> +             Assigned dynamically by the DRM RAS core upon registration.
> +      -
> +        name: field
> +        type: u32
> +        doc:  Telemetry field to be read/written, e.g., SoC temp.
> +      -
> +        name: field-name
> +        type: string
> +        doc:  Name of the telemetry field defined by the device's uAPI.
> +      -
> +        name: value
> +        type: sint
> +        doc: Value of telemetry field that was read/written.
> +      -
> +        name: status
> +        type: s32
> +        doc: >-
> +             Status of the telemetry operation.
> +             Populated by the device in response to the performed
> +             telemetry operation. Errors may be hardware specific.
>  
>  operations:
>    list:
> @@ -198,6 +226,52 @@ operations:
>              - node-id
>              - error-id
>              - error-threshold
> +    -
> +      name: telemetry-get
> +      doc: Perform a read of the specified telemetry field.
> +      attribute-set: telemetry-attrs
> +      do:
> +        pre: drm-ras-nl-pre-telemetry
> +        request:
> +          attributes:
> +            - node-id
> +            - field
> +        reply:
> +          attributes:
> +            - value
> +            - status
> +      dump:
> +        request:
> +          attributes:
> +            - node-id
> +        reply:
> +          attributes: &telemetry-field-attrs
> +            - field
> +            - field-name
> +    -
> +      name: telemetry-set
> +      doc: >-
> +           Perform a write of the specified telemetry field.
> +           Telemetry writes are device specific, and may be used to set
> +           device limits or other capacities, for example.
> +      attribute-set: telemetry-attrs
> +      flags: [admin-perm]
> +      do:
> +        pre: drm-ras-nl-pre-telemetry
> +        request:
> +          attributes:
> +            - node-id
> +            - field
> +            - value
> +        reply:
> +          attributes:
> +            - status
> +      dump:
> +        request:
> +          attributes:
> +            - node-id
> +        reply:
> +          attributes: *telemetry-field-attrs

[Severity: Medium]
Does the telemetry Netlink UAPI and its status handling work as intended
when a device reports an error?

In drm_ras_nl_telemetry_get_doit() and drm_ras_nl_telemetry_set_doit(), a
non-zero return from node->telemetry_get() or node->telemetry_set() frees
msg and returns immediately without serializing
DRM_RAS_A_TELEMETRY_ATTRS_STATUS.  Conversely, if the callback returns 0
with a non-zero status, drm_ras_nl_telemetry_get_doit() still serializes a
zero DRM_RAS_A_TELEMETRY_ATTRS_VALUE alongside status and bypasses standard
Netlink NLMSG_ERROR and extack reporting:

drivers/gpu/drm/drm_ras.c:drm_ras_nl_telemetry_get_doit() {
    ...
	ret = node->telemetry_get(node, &field, &value, &status);
	if (ret) {
		nlmsg_free(msg);
		return ret;
	}

	ret = nla_put_s32(msg, DRM_RAS_A_TELEMETRY_ATTRS_STATUS, status);
    ...
	ret = nla_put_sint(msg, DRM_RAS_A_TELEMETRY_ATTRS_VALUE, value);
    ...
}

In addition, telemetry-set attaches a dump callback to a set command to
enumerate writable fields, telemetry-get returns different objects in do
([value, status]) versus dump ([field, field-name]), and telemetry-attrs
exposes board temperatures, voltages, power, and TDP limits as device-
specific field IDs and unitless integers rather than through hwmon.

> diff --git a/drivers/gpu/drm/drm_ras.c b/drivers/gpu/drm/drm_ras.c
> index 4f4777060a4e8..fda50563c4a54 100644
> --- a/drivers/gpu/drm/drm_ras.c
> +++ b/drivers/gpu/drm/drm_ras.c
[ ... ]
> @@ -617,6 +644,285 @@ int drm_ras_nl_set_error_threshold_doit(struct sk_buff *skb, struct genl_info *i
> +int drm_ras_nl_pre_telemetry(const struct genl_split_ops *ops, struct sk_buff *skb,
> +			     struct genl_info *info)
> +{
> +	struct drm_ras_node *node;
> +	u32 node_id;
> +
> +	if (!info->attrs || GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID))
> +		return -EINVAL;
> +
> +	node_id = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID]);
> +	node = xa_load(&drm_ras_xa, node_id);
> +	if (!node)
> +		return -ENOENT;
> +
> +	if (node->telemetry_pre)
> +		return node->telemetry_pre(node);
> +
> +	return 0;
> +}
[ ... ]
> +int drm_ras_nl_telemetry_get_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct drm_ras_node *node;
> +	struct sk_buff *msg;
> +	struct nlattr *hdr;
> +	u32 node_id, field;
> +	s32 status = 0;
> +	s64 value = 0;
> +	int ret = 0;
> +
> +	if (!info->attrs ||
> +	    GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID) ||
> +	    GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_FIELD))
> +		return -EINVAL;
> +
> +	field = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_FIELD]);
> +	node_id = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID]);
> +
> +	node = xa_load(&drm_ras_xa, node_id);
> +	if (!node || !node->telemetry_get)
> +		return -ENOENT;
> +
> +	if (!telemetry_fields_contains(node->telemetry_fields_gettable, field))
> +		return -EINVAL;
> +
> +	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	hdr = genlmsg_iput(msg, info);
> +	if (!hdr) {
> +		nlmsg_free(msg);
> +		return -EMSGSIZE;
> +	}
> +
> +	ret = node->telemetry_get(node, &field, &value, &status);

[Severity: High]
Can the unlocked xa_load() lookups in the telemetry handlers race with
drm_ras_node_unregister() and cause a use-after-free?

drm_ras_nl_family sets .parallel_ops = true, and xa_load() only holds the
RCU read lock during the XArray walk itself.  drm_ras_nl_pre_telemetry(),
drm_ras_nl_telemetry_get_doit(), drm_ras_nl_telemetry_get_dumpit(),
drm_ras_nl_telemetry_set_doit(), and drm_ras_nl_telemetry_set_dumpit() use
the returned node pointer without holding a lock or reference across
GFP_KERNEL allocations and sleeping driver callbacks, while
drm_ras_node_unregister() only calls xa_erase() before the caller frees the
node:

CPU0                                  CPU1
drm_ras_nl_telemetry_get_doit()
  node = xa_load(&drm_ras_xa, id);
  genlmsg_new(..., GFP_KERNEL);
                                      drm_ras_node_unregister(node)
                                        xa_erase(&drm_ras_xa, node->id);
                                      kfree(node);
  node->telemetry_get(node, ...);  <-- use-after-free

Because drm_ras_nl_pre_telemetry() and the doit handlers also perform
separate unlocked xa_load() calls instead of pinning the node in pre_doit
and releasing it in post_doit, node_id can be unregistered and reallocated
to a different node between the permission check and the operation.

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