Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Dangling pointer in `client->sva` leads to a Use-After-Free and double-free during DRM close if `iommu_sva_get_pasid()` fails. -- commit a925d70753a15e6bc99a30d4fbc4ec99fe4b925e Author: Lizhi Hou <[email protected]> accel/amdxdna: Require carveout when PASID and force_iova are disabled When both PASID and force_iova are disabled, this patch ensures carveout memory is configured, returning an error during DRM open if it is not. > diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.c > b/drivers/accel/amdxdna/amdxdna_pci_drv.c > index c677293c1ae7f..1f066ed8d2365 100644 > --- a/drivers/accel/amdxdna/amdxdna_pci_drv.c > +++ b/drivers/accel/amdxdna/amdxdna_pci_drv.c > @@ -120,8 +120,14 @@ static int amdxdna_drm_open(struct drm_device *ddev, > struct drm_file *filp) > > if (!amdxdna_iova_on(xdna)) { > /* No need to fail open since user may use pa + carveout later. > */ > - if (amdxdna_sva_init(client)) > + if (amdxdna_sva_init(client)) { > XDNA_WARN(xdna, "PASID not available for pid %d", > client->pid); > + if (!amdxdna_use_carveout(xdna)) { > + XDNA_ERR(xdna, "PASID unavailable and carveout > not configured"); > + kfree(client); > + return -EINVAL; > + } > + } [Severity: High] This is a pre-existing issue, but since this patch relies on the fallback behavior when amdxdna_sva_init() fails, does this leave a dangling pointer in client->sva? Looking at amdxdna_sva_init(), if iommu_sva_bind_device() succeeds but iommu_sva_get_pasid() fails, the sva handle is unbound but the pointer isn't cleared: amdxdna_sva_init() { ... client->pasid = iommu_sva_get_pasid(client->sva); if (client->pasid == IOMMU_PASID_INVALID) { iommu_sva_unbind_device(client->sva); XDNA_ERR(xdna, "SVA get pasid failed"); return -ENODEV; } ... } If amdxdna_use_carveout() returns true, amdxdna_drm_open() continues successfully, keeping the client object alive. When the DRM file is later closed, amdxdna_sva_fini() is called: amdxdna_sva_fini() { if (IS_ERR_OR_NULL(client->sva)) return; iommu_sva_unbind_device(client->sva); } Since client->sva still holds the old pointer instead of NULL or an error pointer, the IS_ERR_OR_NULL check will pass, and iommu_sva_unbind_device() will be called a second time. Can this cause a double-free or use-after-free of the iommu_sva structure during cleanup? > } > mmgrab(client->mm); > init_srcu_struct(&client->hwctx_srcu); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
