Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] dma-buf: NULL pointer dereference of importer_ops in dma_buf_dynamic_attach() --
commit 4415ca9de8f7d7c189e76efc4aa022c3564f88f1 Author: Jason Gunthorpe <[email protected]> dma-buf: Add dma_buf_mapping_attach() This commit introduces dma_buf_mapping_attach() to match importer-supported mapping types against exporter-supported mapping types during attachment. It also converts dma_buf_attach() and dma_buf_dynamic_attach() into wrappers around dma_buf_mapping_attach(). > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 4c9add51f9ef5..16a7fbb8aaabe 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c [ ... ] > @@ -1025,31 +1035,55 @@ dma_buf_pin_on_map(struct dma_buf_attachment *attach) [ ... ] > - attach->dev = dev; > attach->dmabuf = dmabuf; > - if (importer_ops) > - attach->peer2peer = importer_ops->allow_peer2peer; > attach->importer_ops = importer_ops; > attach->importer_priv = importer_priv; [ ... ] > @@ -1068,23 +1103,53 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct > device *dev, [ ... ] > +/** > + * dma_buf_dynamic_attach - Add the device to dma_buf's attachments list > + * @dmabuf: [in] buffer to attach device to. > + * @dev: [in] device to be attached. > + * @importer_ops: [in] importer operations for the attachment > + * @importer_priv: [in] importer private pointer for the attachment > + * > + * Wrapper to call dma_buf_mapping_attach() for drivers which only support > SGT. > + */ > +struct dma_buf_attachment * > +dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, > + const struct dma_buf_attach_ops *importer_ops, > + void *importer_priv) > +{ > + struct dma_buf_mapping_match sgt_match[] = { > + DMA_BUF_IMAPPING_SGT(dev, importer_ops->allow_peer2peer ? > + DMA_SGT_IMPORTER_ACCEPTS_P2P : > + DMA_SGT_NO_P2P), > + }; [Severity: High] Can importer_ops be NULL when passed to dma_buf_dynamic_attach(), causing a NULL pointer dereference when evaluating importer_ops->allow_peer2peer? Previously, dma_buf_dynamic_attach() guarded this dereference with if (importer_ops). In-tree callers still rely on passing a NULL importer_ops pointer here, such as xe_gem_prime_import() when running the non-dynamic KUnit test cases in drivers/gpu/drm/xe/tests/xe_dma_buf.c: drivers/gpu/drm/xe/xe_dma_buf.c:xe_gem_prime_import() { ... attach_ops = &xe_dma_buf_attach_ops; #if IS_ENABLED(CONFIG_DRM_XE_KUNIT_TEST) if (test) attach_ops = test->attach_ops; #endif attach = dma_buf_dynamic_attach(dma_buf, dev->dev, attach_ops, obj); ... } Note that a later commit in the series, 77ea3fee521c ("dma-buf: Rename dma_buf_dynamic_attach() to dma_buf_sgt_dynamic_attach()"), renames this function while keeping the unconditional importer_ops->allow_peer2peer dereference. Should dma_buf_dynamic_attach() check importer_ops && importer_ops->allow_peer2peer before dereferencing importer_ops? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
