Messages in this thread | | | From | Miles Chen <> | Subject | Re: [PATCH v2] iommu/mediatek: fix NULL pointer dereference when printing dev_name | Date | Wed, 27 Apr 2022 10:13:21 +0800 |
| |
Hi Yong,
>On Mon, 2022-04-25 at 11:03 +0100, Robin Murphy wrote: >> On 2022-04-25 09:24, Miles Chen via iommu wrote: >> > When larbdev is NULL (in the case I hit, the node is incorrectly >> > set >> > iommus = <&iommu NUM>), it will cause device_link_add() fail and >> >> Until the MT8195 infra MMU support lands, is there ever a case where >> it's actually valid for larbdev to be NULL? If not, I think it would >> be >> a lot clearer to explicitly fail the probe here, rather than >> silently >> continue and risk fatal errors, hangs, or other weird behaviour if >> there's no guarantee that the correct LARB is powered up (plus then >> the >> release callbacks wouldn't need to worry about it either). > >Yes. It should return fail for this case. This issue only happens when >the dts parameters doesn't respect the definition from the binding[1]. > >Locally Miles tested with a internal definition that have not send >upstream to get this KE. In this case, I'm not sure if we should >request the user use the right ID in dts. Anyway I have no objection to >modifying this, then something like this: (Avoid invalid input from >dtb) > >@@ -790,6 +790,8 @@ static struct iommu_device >*mtk_iommu_probe_device(struct device *dev) > * All the ports in each a device should be in the same larbs. > */ > larbid = MTK_M4U_TO_LARB(fwspec->ids[0]); >+ if (larbid >= MTK_LARB_NR_MAX) >+ return ERR_PTR(-EINVAL); > for (i = 1; i < fwspec->num_ids; i++) { > larbidx = MTK_M4U_TO_LARB(fwspec->ids[i]); > if (larbid != larbidx) { >@@ -799,6 +801,8 @@ static struct iommu_device >*mtk_iommu_probe_device(struct device *dev) > } > } > larbdev = data->larb_imu[larbid].dev; >+ if (!larbdev) >+ return ERR_PTR(-EINVAL); > link = device_link_add(dev, larbdev, > DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS); > if (!link)
Thanks for guilding me, I will put this in patch v2.
Thanks, Miles
> > >[1] >https://elixir.bootlin.com/linux/v5.18-rc1/source/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml#L116
| |