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

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

[Guenter Roeck]

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.

Cc: Yong Wu <yong.wu@mediatek.com>
Cc: Tomasz Figa <tfiga@chromium.org>
Fixes: baf94e6ebff9 ("iommu/mediatek: Add device link for smi-common and m4u")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Guenter Roeck <linux@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;
 
 	for (i = 0; i < larb_nr; i++) {
 		u32 id;
-- 
2.33.0


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

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

[Yong Wu]

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.

> 
> Cc: Yong Wu <yong.wu@mediatek.com>
> Cc: Tomasz Figa <tfiga@chromium.org>
> Fixes: baf94e6ebff9 ("iommu/mediatek: Add device link for smi-common
> and m4u")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Guenter Roeck <linux@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;

>  
>  	for (i = 0; i < larb_nr; i++) {
>  		u32 id;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Tzung-Bi Shih]

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

Re: [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?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Guenter Roeck]

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
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@mediatek.com>
>> Cc: Tomasz Figa <tfiga@chromium.org>
>> Fixes: baf94e6ebff9 ("iommu/mediatek: Add device link for smi-common
>> and m4u")
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Signed-off-by: Guenter Roeck <linux@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,

- The check I introduced should probably be something like

	if (larb_nr == 0 || larb_nr > MTK_LARB_NR_MAX)
		return -EINVAL;

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

- 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 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" ]

- "id" should be range checked.

- The meaning of "last" larb node to use when looking for mediatek,smi should
   be explained in more detail.

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

Thanks,
Guenter

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


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

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

[Yong Wu]

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.

> 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@mediatek.com>
> > > Cc: Tomasz Figa <tfiga@chromium.org>
> > > Fixes: baf94e6ebff9 ("iommu/mediatek: Add device link for smi-
> > > common
> > > and m4u")
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > Signed-off-by: Guenter Roeck <linux@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;

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

> 
> - 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;
   }         

> 
> - "id" should be range checked.

It should be [0, MTK_LARB_NR_MAX).

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

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

> 
> Thanks,
> Guenter
> 
> > >   
> > >   	for (i = 0; i < larb_nr; i++) {
> > >   		u32 id;
> 
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [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@mediatek.com>
> > > > Cc: Tomasz Figa <tfiga@chromium.org>
> > > > Fixes: baf94e6ebff9 ("iommu/mediatek: Add device link for smi-
> > > > common
> > > > and m4u")
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > Signed-off-by: Guenter Roeck <linux@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;
> > 
> > 

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