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...@mediatek.com> > > > > Cc: Tomasz Figa <tf...@chromium.org> > > > > Fixes: baf94e6ebff9 ("iommu/mediatek: Add device link for smi- > > > > common > > > > and m4u") > > > > Reported-by: kernel test robot <l...@intel.com> > > > > Reported-by: Dan Carpenter <dan.carpen...@oracle.com> > > > > Signed-off-by: Guenter Roeck <li...@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; > > > > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu