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

[Guenter Roeck]

On Wed, Dec 15, 2021 at 01:30:45PM +0800, Yong Wu wrote:
> On Tue, 2021-12-14 at 07:02 -0800, Guenter Roeck wrote:
> > On 12/13/21 11:31 PM, 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.
> > > 
> > 
> > This doesn't have to be an abuse or attack. It can simply be an error
> > by the person who wrote the devicetree file. Sure, bugs or lack of
> 
> A minor question: If someone wrote error data that don't conform to the
> dtbinding, the error result is expected. He should fix that problem,
> right? If we could avoid abort and show error message at the beginning,
> it's better of course.
> 

Sure. However, such an error should not result in a crash but should be
caught in an error handler.

> > error checking can often be used for attacks, but that doesn't mean
> > that all bad data is an exploit or attack.
> > 
> > > > 
> > > > Cc: Yong Wu <yong.wu at mediatek.com>
> > > > Cc: Tomasz Figa <tfiga at chromium.org>
> > > > Fixes: baf94e6ebff9 ("iommu/mediatek: Add device link for smi-
> > > > common
> > > > and m4u")
> > > > Reported-by: kernel test robot <lkp at intel.com>
> > > > Reported-by: Dan Carpenter <dan.carpenter at oracle.com>
> > > > Signed-off-by: Guenter Roeck <linux at roeck-us.net>
> > > > ---
> > > >   drivers/iommu/mtk_iommu.c | 2 ++
> > > >   1 file changed, 2 insertions(+)
> > > > 
> > > > 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;
> > > 
> > 
> > It is an option, but it would need to be explained and would not be
> > as simple as it looks. And, yes, it would result in unnecessary code
> > execution.
> > 
> > Why does it need to be explained ? I spent quite some additional
> > time with the code trying to understand _why_ it works, and we should
> > make sure that others don't have to spend that time.
> > 
> > Anyway, that additional time made me find additional problems with
> > the code.
> > 
> > The for loop below assigns larbnode to the last node it finds.
> > However, that node can be disabled.
> > 
> > 		if (!of_device_is_available(larbnode)) {
> >                          of_node_put(larbnode);
> >                          continue;
> >                  }
> > 
> > Is such a disabled larbnode, if it is the last one, the node to use
> > when looking for "mediatek,smi" ?
> > 
> > Also, there is
> > 
> > 	ret = of_property_read_u32(larbnode, "mediatek,larb-id", &id);
> >          if (ret)/* The id is consecutive if there is no this
> > property */
> >                  id = i;
> > 
> > There are two problems with this code. First, neither i nor id are
> > range
> > checked, but used later in
> > 
> > 	data->larb_imu[id].dev = &plarbdev->dev;
> > 
> > That means a devicetree with a bad value for "mediatek,larb-id"
> > or more than MTK_LARB_NR_MAX larb nodes will result in writes after
> > the end of struct mtk_iommu_data.
> > 
> > On top of that, the comment states that the nodes are consecutive if
> > there
> > is no "mediatek,larb-id". However, that isn't really the case if
> > there
> > are disabled nodes. If there are disabled nodes, there will be a gap
> > in
> > larb_imu[]. I don't know if that matters; if it doesn't, there should
> > be
> > a comment about it in the code.
> > 
> > Last but not least, it would probably make sense to explain what the
> > "last"
> > larb node is expected to be in more detail. It is the last larb node
> > in
> > the devicetree file, but not the one with the highest id, and not
> > (necessarily) an enabled one. For example, in
> > arch/arm64/boot/dts/mediatek/mt2712e.dtsi, the code would pick
> > <&smi_common0> even though <&smi_common1> is associated with a higher
> > larb id.
> > 
> > One could of course argue that this all doesn't matter because it
> > would
> > suggest that the devicetree data is bad, but it is common practice to
> > validate
> > devicetree data and not just blindly accept it. One could also argue
> > that such bad data would be an "attack", but, again, we don't know
> > that.
> > 
> > In summary,
> 
> Thanks very much for your time to check here. All the issues are
> introduced by the values from dts are untrusted. The detail platform
> informations are replied below.
> 
> > 
> > - The check I introduced should probably be something like
> > 
> > 	if (larb_nr == 0 || larb_nr > MTK_LARB_NR_MAX)
> > 		return -EINVAL;
> 
> OK. Add a "else" to show it is a block with the "if" above?
> 
>         if (larb_nr < 0)
>              return larb_nr;
>         else if (larb_nr == 0 || larb_nr > MTK_LARB_NR_MAX)
>              return -EINVAL;
> 

Static checkers would complain with "else after return is unnecessary".

> > 
> > - It needs to be clarified if larbnode to use for finding
> > "mediatek,smi"
> >    is indeed always the last one, even if it is disabled. If so, we 
> 
> We could find the "mediatek,smi" with any available larb. Of course it
> should not be a disabled one. The code using the last larb is for
> reusing the variable "larbnode".
> 
> > should
> >    probably also handle the situation that of_node_put(larbnode); was
> > called
> >    on that larbnode. Alternatively, if the last larb node to use is
> > the last
> >    _active_ larb node, we'll probably need a separate variable to
> > save that
> >    larb node pointer for later use.
> 
> A new variable is ok.
> 
Ok, I'll change the code accordingly.

> > 
> > - It needs to be clarified if larb_imu[] may have gaps if there are
> > disabled
> >    larb nodes and "mediatek,larb-id" is not specified. If so, there 
> 
> Yes. It may have gaps. the commit message of this patch may be helpful.
> 
> 50fa3cd33f9d ("dt-bindings: mediatek: Add binding for mt2712 IOMMU and
> SMI")
> 
> > is still the
> >    problem that 'i' and a previous value of "mediatek,larb-id" may be
> > identical
> >    [ eg the first node provides mediatek,larb-id = <1> and the second
> > node
> >      doesn't provide "mediatek,larb-id" ]
> 
> This case did don't meet my expectation. OK, then we add a checking?
> like:
> 
>    if (data->larb_imu[i].dev) {
>          dev_err(dev, "the larb %d exist.", i);
>          return -EEXIST;
>    }         

Makes sense, I'll do that.

> 
> > 
> > - "id" should be range checked.
> 
> It should be [0, MTK_LARB_NR_MAX).
> 

I'll add this check.

> > 
> > - The meaning of "last" larb node to use when looking for
> > mediatek,smi should
> >    be explained in more detail.
> 
> We could use any available larb node to find mediatek,smi.
> 
> Their "mediatek,smi" node must be the same. OK, In this case, they are
> possible different. We should add a checking: return -EINVAL if they
> are not same.
> 
I'll see if and how I can do that without adding too much cmplexity
to the code.

> > 
> > Once we have determined the correct handling of all those situations,
> > I'll
> > be happy to send another revision of this patch (or possibly multiple
> > patches).
> 
> Appreciate for help enhance the safe here. I will test it.
> 
My pleasure.

Thanks,
Guenter

> > 
> > Thanks,
> > Guenter
> > 
> > > >   
> > > >   	for (i = 0; i < larb_nr; i++) {
> > > >   		u32 id;
> > 
> >