Re: [SPAM][PATCH] iommu/mediatek: Validate number of phandles associated with "mediatek,larbs"

On Tue, Dec 14, 2021 at 03:31:25PM +0800, Yong Wu wrote:

On Fri, 2021-12-10 at 12:57 -0800, Guenter Roeck wrote:

quoted

Since commit baf94e6ebff9 ("iommu/mediatek: Add device link for smi-
common
and m4u"), the driver assumes that at least one phandle associated
with
"mediatek,larbs" exists. If that is not the case, for example if
reason
"mediatek,larbs" is provided as boolean property, the code will use
an
uninitialized pointer and may crash. To fix the problem, ensure that
the
number of phandles associated with "mediatek,larbs" is at least 1 and
bail out immediately if that is not the case.

From the dt-binding, "mediatek,larbs" always is a phandle-array. I 
assumed the dts should conform to the dt-binding before. Then the
problem is that if we should cover the case that someone abuses/attacks
the dts. Could you help add more comment in the commit message?
something like: this is for avoid abuse the dt-binding.

How could you make sure dts conform to dt-bindings in runtime?  Code shouldn't rely on the assumptions but try the best to prevent any abuse/misconfigured/malicious cases especially if the assumptions are controllable by other parties.

Taking this case as an example, of_count_phandle_with_args() could return 3 types of values.
1. Negative: an error, it is already handled in the original code.
2. Positive: normal case, it falls down to the rest of code.
3. Zero: it still falls down to the rest of code, however, some variables won't be filled.

The code should handle all of the above types.

quoted

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 25b834104790..0bbe32d0a2a6 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c

@@ -828,6 +828,8 @@ static int mtk_iommu_probe(struct platform_device

*pdev)
 					     "mediatek,larbs", NULL);
 	if (larb_nr < 0)
 		return larb_nr;
+	if (larb_nr == 0)
+		return -EINVAL;

Just assigning the larbnode to NULL may be simpler. In this case, it
won't enter the loop below, and return 0 in the
of_parse_phandle(larbnode, "mediatek,smi", 0).

-       struct device_node      *larbnode, *smicomm_node;
+       struct device_node      *larbnode = NULL, *smicomm_node;

Setting larbnode to NULL doesn't make sense to me.  It wastes some more instructions.  If the code can exit earlier, why does it need to call another of_parse_phandle()?

Also, it adds another dependency between the code blocks.  What if someone move the code blocks without awareness of the dependency?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel