Re: [PATCH 2/2] drm/xe: gate observation streams with perf_allow_cpu()

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Unconditional call to `perf_allow_cpu()` causes a build failure when `CONFIG_PERF_EVENTS` is disabled.

Pre-existing issues:
- [High] Reference count leak of `xe_exec_queue` on the error path in `xe_oa_stream_open_ioctl()`.
- [High] Memory leak of the `stream->syncs` array when an OA stream is disabled or fails to emit config.
--

commit c928d2e2efb93b79ed4af6b5c4bcf6b62b0a9acb
Author: John Hubbard <jhubbard@nvidia.com>
Subject: drm/xe: gate observation streams with perf_allow_cpu()

This patch introduces xe_observation_paranoid_check() to replace open-coded 
privilege checks in the xe driver. It wraps perf_allow_cpu(), meaning xe 
observation now respects the system-wide kernel.perf_event_paranoid policy 
and the security_perf_event_open() LSM hook.

> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index 6337e671c97ae..f15847be01bc2 100644
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c
[ ... ]
> @@ -2052,10 +2053,12 @@ int xe_oa_stream_open_ioctl(struct drm_device *dev, u64 data, struct drm_file *f
>  		privileged_op = true;
>  	}
>  
> -	if (privileged_op && xe_observation_paranoid && !perfmon_capable()) {
> -		drm_dbg(&oa->xe->drm, "Insufficient privileges to open xe OA stream\n");
> -		ret = -EACCES;
> -		goto err_exec_q;
> +	if (privileged_op) {
> +		ret = xe_observation_paranoid_check();
> +		if (ret) {
> +			drm_dbg(&oa->xe->drm, "Insufficient privileges to open xe OA stream\n");
> +			goto err_exec_q;
> +		}
>  	}

[Severity: High]
This is a pre-existing issue, but does this code leak the xe_exec_queue
reference on the error path?

Looking earlier in xe_oa_stream_open_ioctl():

    param.exec_q = xe_exec_queue_lookup(xef, param.exec_queue_id);
    if (XE_IOCTL_DBG(oa->xe, !param.exec_q))
        return -ENOENT;

    if (XE_IOCTL_DBG(oa->xe, param.exec_q->width > 1))
        return -EOPNOTSUPP;

When the execution queue width is greater than 1, it directly returns
-EOPNOTSUPP instead of jumping to the err_exec_q label. This bypasses
xe_exec_queue_put() and leaves the reference incremented, which could allow
a malicious user space program to repeatedly leak execution queue references.

[Severity: High]
This isn't a bug introduced by this patch, but does the stream opening
process leak the stream->syncs array if the stream is disabled?

When xe_oa_stream_open_ioctl() calls xe_oa_stream_open_ioctl_locked(), the
stream->syncs array is allocated via xe_oa_parse_syncs(). This array is
usually freed inside xe_oa_emit_oa_config() when the stream is enabled.

If a stream is opened with the disabled parameter set to true,
xe_oa_enable_locked() is skipped, and xe_oa_emit_oa_config() is never
called. When the stream is subsequently destroyed upon file release,
xe_oa_stream_destroy() does not appear to free stream->syncs, which might
result in a memory leak.

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_observation.c b/drivers/gpu/drm/xe/xe_observation.c
> index e3f9b546207e4..39e05b9131a74 100644
> --- a/drivers/gpu/drm/xe/xe_observation.c
> +++ b/drivers/gpu/drm/xe/xe_observation.c
[ ... ]
> @@ -12,9 +13,28 @@
>  #include "xe_oa.h"
>  #include "xe_observation.h"
>  
> -u32 xe_observation_paranoid = true;
> +static u32 xe_observation_paranoid = true;
>  static struct ctl_table_header *sysctl_header;
>  
> +/**
> + * xe_observation_paranoid_check - Gate access to xe observation streams.
> + *
> + * When the xe-specific observation_paranoid sysctl is enabled (the
> + * default), defer to perf_allow_cpu() so that access is governed by the
> + * same policy as system-wide perf CPU events: kernel.perf_event_paranoid
> + * plus the security_perf_event_open() LSM hook. When the sysctl has been
> + * cleared by a privileged user, observation is open to all callers.
> + *
> + * Return: 0 if access is permitted, a negative errno otherwise.
> + */
> +int xe_observation_paranoid_check(void)
> +{
> +	if (!xe_observation_paranoid)
> +		return 0;
> +
> +	return perf_allow_cpu();
> +}

[Severity: Low]
Will this cause a build failure when CONFIG_PERF_EVENTS is disabled?

The function perf_allow_cpu() is only defined if CONFIG_PERF_EVENTS is
enabled, and there is no fallback stub provided in
include/linux/perf_event.h for the disabled case. Since the DRM_XE
subsystem does not strictly depend on CONFIG_PERF_EVENTS, building the
driver without perf events support might result in an undefined reference
linker error.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521024904.331912-1-jhubbard@nvidia.com?part=2