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