Re: [PATCH 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 7 potential issue(s) to consider:
- [High] drm: NULL pointer dereference in drm_ras_nl_pre_telemetry() on invalid node
- [High] drm: missing netlink attribute presence checks cause NULL pointer dereferences
- [High] drm: Use-After-Free of drm_ras_node in netlink telemetry handlers
- [High] drm: DRM_RAS_CMD_TELEMETRY_WRITE lacks GENL_ADMIN_PERM capability
- [Medium] drm: uninitialized stack variables leaked in drm_ras_nl_telemetry_read_doit()
- [Low] drm_ras: drm_ras_nl_telemetry_write_doit validates wrong callback
- [Low] netlink: use of u64 and pad attributes instead of uint for new uAPI
--

commit 6e531a5b48e418d2f90c14c196ea1ac73f7170e4
Author: Zack McKevitt <zachary.mckevitt@oss.qualcomm.com>

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

This commit adds a new node type, attribute set, and doit function for
performing telemetry read and write operations using the DRM RAS interface.

> diff --git a/Documentation/netlink/specs/drm_ras.yaml b/Documentation/netlink/specs/drm_ras.yaml
> index a88fdab7a1c74..d558eb1cba3f1 100644
> --- a/Documentation/netlink/specs/drm_ras.yaml
> +++ b/Documentation/netlink/specs/drm_ras.yaml
[ ... ]
> @@ -100,6 +100,31 @@ attribute-sets:
>          name: error-value
>          type: u32
>          doc: Current value of the error counter.
> +  -
> +    name: telemetry-attrs
> +    attributes:
[ ... ]
> +      -
> +        name: value
> +        type: u64
> +        doc: Value of telemetry field that was read/written.
> +      -
> +        name: status
> +        type: s32
> +        doc: Status of the telemetry operation.
> +      -
> +        name: pad
> +        type: pad
> +        doc: Alignment padding used for 64-bit

[Severity: Low]
Does this new netlink family uAPI violate the generic netlink design
guidelines?

The telemetry-attrs set defines value as a fixed-width u64 and uses a pad
attribute. The generic netlink documentation explicitly prefers
variable-width 64-bit types (uint/sint) for new attributes to avoid padding
and 64-bit alignment issues on 32-bit architectures.

Since uAPI is immutable once merged, should this use uint natively instead
of introducing fixed-width structures requiring pad attributes?

> diff --git a/drivers/gpu/drm/drm_ras.c b/drivers/gpu/drm/drm_ras.c
> index b099eb67836e7..b88293295cda9 100644
> --- a/drivers/gpu/drm/drm_ras.c
> +++ b/drivers/gpu/drm/drm_ras.c
[ ... ]
> +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;
> +
> +	node_id = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID]);

[Severity: High]
Can this cause a NULL pointer dereference if userspace sends a malformed
message?

Generic netlink policies enforce the type of an attribute only if it
is present, but they do not enforce presence. If the
DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID attribute is missing from the request,
info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID] will be NULL.

Passing NULL to nla_get_u32() dereferences NULL + NLA_HDRLEN, leading to an
immediate kernel oops.

> +	node = xa_load(&drm_ras_xa, node_id);
> +
> +	if (node->telemetry_pre)

[Severity: High]
Could this dereference a NULL pointer if the provided node_id does not
exist?

The xa_load() function returns NULL if the ID is not registered, and
this code immediately evaluates node->telemetry_pre without verifying if
node is a valid pointer.

> +		return node->telemetry_pre(node);
> +
> +	return 0;
> +}
[ ... ]
> +int drm_ras_nl_telemetry_read_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;
> +	u64 value;

[Severity: Medium]
Are these variables guaranteed to be fully initialized by the driver
callback?

They are declared on the kernel stack and passed by reference to the
node->telemetry_read() callback below.

> +	int ret;
> +
> +	if (!info->attrs || !info->attrs[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_read)
> +		return -ENOENT;

[Severity: High]
Does this lockless usage of the node pointer risk a use-after-free?

The pointer is obtained via xa_load(), which uses RCU internally but drops
the read lock before returning. The pointer is then used locklessly without
a reference count across the node->telemetry_read() callback.

Since telemetry_read() is allowed to sleep, could a concurrent hardware
unbind or module unload unregister and free the node while this callback is
still executing?

> +
> +	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_read(node, &field, &value, &status);
> +	if (ret) {
> +		nlmsg_free(msg);
> +		return ret;
> +	}
> +
> +	ret = nla_put_s32(msg, DRM_RAS_A_TELEMETRY_ATTRS_STATUS, status);
> +	if (ret) {
> +		genlmsg_cancel(msg, hdr);
> +		nlmsg_free(msg);
> +		return ret;
> +	}
> +
> +	ret = nla_put_64bit(msg, DRM_RAS_A_TELEMETRY_ATTRS_VALUE, sizeof(value),
> +			    &value, DRM_RAS_A_TELEMETRY_ATTRS_PAD);

[Severity: Medium]
If the driver's telemetry_read() callback returns success (0) but fails to
explicitly populate the value or status fields, this writes uninitialized
kernel stack memory directly into the netlink reply.

Should these variables be initialized to zero when declared?

> +	if (ret) {
> +		genlmsg_cancel(msg, hdr);
> +		nlmsg_free(msg);
> +		return ret;
> +	}
[ ... ]
> +int drm_ras_nl_telemetry_write_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;
> +	u64 value;
> +	int ret;
> +
> +	if (!info->attrs || !info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_FIELD])
> +		return -EINVAL;
> +
> +	value = nla_get_u64(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_VALUE]);
> +	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]);

[Severity: High]
Similar to the pre_doit handler, is it possible for a malformed message
missing the VALUE or NODE_ID attributes to trigger a NULL pointer
dereference here?

The presence of DRM_RAS_A_TELEMETRY_ATTRS_FIELD is explicitly checked above,
but VALUE and NODE_ID are extracted unconditionally.

> +
> +	node = xa_load(&drm_ras_xa, node_id);
> +	if (!node || !node->telemetry_read)

[Severity: Low]
Does this code validate the wrong callback?

The telemetry write handler checks for the presence of node->telemetry_read,
but later invokes node->telemetry_write(). While node registration might
currently ensure both are populated, checking the read callback here creates
a brittle dependency on the registration logic.

> +		return -ENOENT;
[ ... ]
> diff --git a/drivers/gpu/drm/drm_ras_nl.c b/drivers/gpu/drm/drm_ras_nl.c
> --- a/drivers/gpu/drm/drm_ras_nl.c
> +++ b/drivers/gpu/drm/drm_ras_nl.c
[ ... ]
> @@ -83,6 +96,22 @@ static const struct genl_split_ops drm_ras_nl_ops[] = {
[ ... ]
> +	{
> +		.cmd		= DRM_RAS_CMD_TELEMETRY_WRITE,
> +		.pre_doit	= drm_ras_nl_pre_telemetry,
> +		.doit		= drm_ras_nl_telemetry_write_doit,
> +		.policy		= drm_ras_telemetry_write_nl_policy,
> +		.maxattr	= DRM_RAS_A_TELEMETRY_ATTRS_VALUE,
> +		.flags		= GENL_CMD_CAP_DO,
> +	},
>  };

[Severity: High]
Should this command require the GENL_ADMIN_PERM capability?

Unlike DRM_RAS_CMD_SET_ERROR_THRESHOLD, the telemetry write command is missing
the GENL_ADMIN_PERM flag. This allows unprivileged users to issue arbitrary
writes to hardware components, potentially leading to denial of service or
hardware state corruption.

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