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

[Yong Wu]

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

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?