Re: [PATCH 3/3] accel/qaic: Add callbacks for DRM RAS netlink telemetry

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] accel/qaic: NULL pointer dereference of qdev->tele_ch during telemetry requests
- [High] accel/qaic: Privilege escalation in qaic_telemetry_pre via mount namespace bypass
- [High] accel/qaic: Missing cleanup in qaic_pci_probe error paths causes UAF and leaks
- [High] qaic: UAF of qdev in tele_wq due to mismatched devm and drmm lifecycles
- [Medium] accel/qaic: Memory leak of drm_ras_node in qaic_drm_ras_register_node error path
- [Medium] accel/qaic: Misaligned DMA buffer embedded in struct wrapper_msg
- [Low] accel/qaic: Incompatible pointer type warning in qaic_telemetry_read/write
--

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

accel/qaic: Add callbacks for DRM RAS netlink telemetry
    
Use the DRM RAS netlink infrastructure to perform device telemetry.
During PCI probe, create a DRM RAS node with DRM_RAS_NODE_TYPE_TELEMETRY
type and add callbacks to telemetry_request for read and write operations.

> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
> index c1bb6db78d129..d2aed51052a66 100644
> --- a/drivers/accel/qaic/qaic_drv.c
> +++ b/drivers/accel/qaic/qaic_drv.c
[ ... ]
> @@ -443,6 +448,9 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev,
>  	qdev->ssr_wq = qaicm_wq_init(drm, "qaic_ssr");
>  	if (IS_ERR(qdev->ssr_wq))
>  		return NULL;
> +	qdev->tele_wq = qaicm_wq_init(drm, "qaic_tele");
> +	if (IS_ERR(qdev->tele_wq))
> +		return NULL;

[Severity: High]
Can this mismatched devm and drmm lifecycle lead to a use-after-free?

The qdev structure is allocated via devm_kzalloc() and is tied to the PCI
device lifecycle, getting freed immediately upon unbind. However, tele_wq
is tied to the DRM device lifecycle via drmm_add_action_or_reset().

If userspace holds the DRM file descriptor open during PCI device unbind,
the DRM device will outlive qdev. Any queued resp_worker() items running
on tele_wq will execute after qdev is freed and dereference it.

[ ... ]
> @@ -595,6 +604,10 @@ static int qaic_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (ret)
>  		return ret;
>  
> +	ret = qaic_drm_ras_register_node(qdev);
> +	if (ret)
> +		return ret;
> +

[Severity: High]
If qaic_drm_ras_register_node() fails here, we return an error directly
without calling qaic_destroy_drm_device(). Doesn't this leak the registered
DRM device?

Also, if qaic_mhi_register_controller() fails immediately after this, it
calls qaic_destroy_drm_device() but fails to call
qaic_drm_ras_unregister_node(). Since qdev is managed by devm_kzalloc and
will be freed by the driver core on probe failure, leaving the DRM RAS node
or DRM device registered with dangling pointers to the freed qdev will lead
to Use-After-Free crashes if userspace attempts to interact with them.

[Severity: High]
The telemetry node is exposed to userspace here before the MHI channel
is initialized on the next line. 

In qaic_pci_remove(), the MHI controller is freed (which asynchronously
clears qdev->tele_ch = NULL) before the DRM RAS node is unregistered.

Concurrent netlink requests from userspace during either of these windows
will trigger a NULL pointer dereference in mhi_queue_buf(), causing a
kernel oops, because qdev->tele_ch is accessed unconditionally in
telemetry_request().

>  	qdev->mhi_cntrl = qaic_mhi_register_controller(pdev, qdev->bar_mhi, mhi_irq,
>  						       qdev->single_msi, config->family);
[ ... ]
> diff --git a/drivers/accel/qaic/qaic_telemetry.c b/drivers/accel/qaic/qaic_telemetry.c
> new file mode 100644
> index 0000000000000..dbfb36cf319ce
> --- /dev/null
> +++ b/drivers/accel/qaic/qaic_telemetry.c
[ ... ]
> +int telemetry_request(struct qaic_device *qdev, u8 cmd, u8 cmd_type, s64 *val)
> +{
[ ... ]
> +	kref_get(&wrapper->ref_count);
> +	ret = mhi_queue_buf(qdev->tele_ch, DMA_TO_DEVICE, req, sizeof(*req), MHI_EOT);

[Severity: Medium]
Is it safe to pass this embedded buffer to the DMA engine?

The req buffer is a pointer to wrapper->msg, which is embedded inside
struct wrapper_msg immediately after struct kref ref_count (4 bytes).
This means the req buffer starts at a 4-byte offset from the kzalloc
allocation, sharing its cacheline with the kref.

Passing unaligned buffers to the DMA mapping API violates architecture DMA
requirements and can cause cacheline tearing and memory corruption on
non-cache-coherent architectures.

> +	if (ret) {
[ ... ]
> +static int qaic_telemetry_pre(struct drm_ras_node *ep)
> +{
> +	struct qaic_device *qdev = ep->priv;
> +	struct inode *dev_inode;
> +	struct path path;
> +	char devpath[24];
> +	int ret;
> +
> +	snprintf(devpath, sizeof(devpath), "/dev/accel/%s", to_accel_kdev(qdev->qddev)->kobj.name);
> +	ret = kern_path(devpath, LOOKUP_FOLLOW, &path);
> +	if (ret)
> +		return -EINVAL;
> +
> +	dev_inode = d_inode(path.dentry);
> +	ret = inode_permission(&nop_mnt_idmap, dev_inode, MAY_READ | MAY_WRITE);

[Severity: High]
Does this regression introduce a privilege escalation vector by relying on
the caller's mount namespace for security enforcement?

kern_path() resolves paths relative to the caller's root and mount
namespace. An unprivileged attacker could use unshare to create a new
mount namespace, mount a tmpfs over /dev/accel, and create a fake
device node with 0777 permissions.

When the attacker sends a netlink request, kern_path() would resolve to
their fake file, passing the inode_permission() check and granting
unauthorized execution of privileged telemetry commands.

> +	path_put(&path);
> +
> +	return ret;
> +}
> +
> +static int qaic_telemetry_read(struct drm_ras_node *ep, u32 *field, u64 *value, s32 *status)
> +{
> +	struct qaic_device *qdev = ep->priv;
> +
> +	*status = (s32)telemetry_request(qdev, *field, TYPE_READ, value);

[Severity: Low]
Will this implicit cast between signed and unsigned pointer types trigger
-Wincompatible-pointer-types warnings and break builds with -Werror?

qaic_telemetry_read() receives a u64 *value parameter from the DRM RAS
API, but it passes it directly to telemetry_request(), which takes an
s64 *val parameter.

> +
> +	return *status;
> +}
[ ... ]
> +int qaic_drm_ras_register_node(struct qaic_device *qdev)
> +{
> +	struct drm_ras_node *node;
[ ... ]
> +	ret = drm_ras_node_register(node);
> +	if (ret) {
> +		kfree(device_name);
> +		kfree(node_name);
> +		kfree(qdev->ras_node);

[Severity: Medium]
Should this be kfree(node) instead?

qdev->ras_node is not assigned until the end of the function, so it is
NULL at this point. kfree(NULL) is a no-op, which means the node
allocation is permanently leaked on this error path.

> +		return ret;
> +	}
> +
> +	qdev->ras_node = node;
> +	return 0;
> +}

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