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

On Tue, 2021-12-14 at 17:04 +0800, Tzung-Bi Shih wrote:

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

quoted

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

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()?

Yes. it wastes more instrustions. But this function is only called in
the probe. it isn't called so often. Guenter has other suggestions.
Let's discuss in that thread.

Thanks very much for your comment.

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek