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

[Tzung-Bi Shih <tzungbi@google.com>]

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:
> > 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.

> > 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-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek