| Message ID | 20201029092202.900218-1-yukuai3@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu/rockchip: check return value of of_find_device_by_node() in rk_iommu_of_xlate() | expand |
On 2020-10-29 09:22, Yu Kuai wrote: > If of_find_device_by_node() failed in rk_iommu_of_xlate(), null pointer > dereference will be triggered. Thus return error code if > of_find_device_by_node() failed. How can that happen? (Given that ".suppress_bind_attrs = true") Robin. > Fixes: 5fd577c3eac3("iommu/rockchip: Use OF_IOMMU to attach devices automatically") > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > --- > drivers/iommu/rockchip-iommu.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c > index e5d86b7177de..090d149ef8e9 100644 > --- a/drivers/iommu/rockchip-iommu.c > +++ b/drivers/iommu/rockchip-iommu.c > @@ -1099,6 +1099,9 @@ static int rk_iommu_of_xlate(struct device *dev, > > iommu_dev = of_find_device_by_node(args->np); > > + if (!iommu_dev) > + return -ENODEV; > + > data->iommu = platform_get_drvdata(iommu_dev); > dev_iommu_priv_set(dev, data); > >
On 2020/10/29 18:08, Robin Murphy wrote: > On 2020-10-29 09:22, Yu Kuai wrote: >> If of_find_device_by_node() failed in rk_iommu_of_xlate(), null pointer >> dereference will be triggered. Thus return error code if >> of_find_device_by_node() failed. > > How can that happen? (Given that ".suppress_bind_attrs = true") > > Robin. I'm not sure if that could happen... My thought is that it's better to do such checking to aviod any possible problem. Thanks! Yu Kuai
On 2020-10-29 13:19, yukuai (C) wrote: > > On 2020/10/29 18:08, Robin Murphy wrote: >> On 2020-10-29 09:22, Yu Kuai wrote: >>> If of_find_device_by_node() failed in rk_iommu_of_xlate(), null pointer >>> dereference will be triggered. Thus return error code if >>> of_find_device_by_node() failed. >> >> How can that happen? (Given that ".suppress_bind_attrs = true") >> >> Robin. > > I'm not sure if that could happen... > > My thought is that it's better to do such checking to aviod any possible > problem. ->of_xlate() is only invoked on the specific set of ops returned by iommu_ops_from_fwnode(). In turn, iommu_ops_from_fwnode() will only return those ops if the driver has successfully probed and called iommu_register_device() with the relevant DT node. For the driver to have been able to probe at all, a platform device associated with that DT node must have been created, and therefore of_find_device_by_node() cannot fail. If there ever were some problem serious enough to break that fundamental assumption, then I *want* these drivers to crash right here, with a nice clear stack trace to start debugging from. So no, I firmly disagree that adding redundant code, which will never do anything except attempt to paper over catastrophic memory corruption, is "better". Sorry :) Robin.
On 2020/10/29 21:51, Robin Murphy wrote: > On 2020-10-29 13:19, yukuai (C) wrote: >> >> On 2020/10/29 18:08, Robin Murphy wrote: >>> On 2020-10-29 09:22, Yu Kuai wrote: >>>> If of_find_device_by_node() failed in rk_iommu_of_xlate(), null pointer >>>> dereference will be triggered. Thus return error code if >>>> of_find_device_by_node() failed. >>> >>> How can that happen? (Given that ".suppress_bind_attrs = true") >>> >>> Robin. >> >> I'm not sure if that could happen... >> >> My thought is that it's better to do such checking to aviod any possible >> problem. > > ->of_xlate() is only invoked on the specific set of ops returned by > iommu_ops_from_fwnode(). In turn, iommu_ops_from_fwnode() will only > return those ops if the driver has successfully probed and called > iommu_register_device() with the relevant DT node. For the driver to > have been able to probe at all, a platform device associated with that > DT node must have been created, and therefore of_find_device_by_node() > cannot fail. > > If there ever were some problem serious enough to break that fundamental > assumption, then I *want* these drivers to crash right here, with a nice > clear stack trace to start debugging from. So no, I firmly disagree that > adding redundant code, which will never do anything except attempt to > paper over catastrophic memory corruption, is "better". Sorry :) > Sounds reasonable, thanks for your explanation Yu Kuai
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index e5d86b7177de..090d149ef8e9 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -1099,6 +1099,9 @@ static int rk_iommu_of_xlate(struct device *dev, iommu_dev = of_find_device_by_node(args->np); + if (!iommu_dev) + return -ENODEV; + data->iommu = platform_get_drvdata(iommu_dev); dev_iommu_priv_set(dev, data);
If of_find_device_by_node() failed in rk_iommu_of_xlate(), null pointer dereference will be triggered. Thus return error code if of_find_device_by_node() failed. Fixes: 5fd577c3eac3("iommu/rockchip: Use OF_IOMMU to attach devices automatically") Signed-off-by: Yu Kuai <yukuai3@huawei.com> --- drivers/iommu/rockchip-iommu.c | 3 +++ 1 file changed, 3 insertions(+)